Test and fix removes in JSON feed parser

3 files changed, 84 insertions, 28 deletions

diff --git a/vespa-feed-client/src/main/java/ai/vespa/feed/client/JsonFeeder.java b/vespa-feed-client/src/main/java/ai/vespa/feed/client/JsonFeeder.java
index eb163a02bf0..68e102ab079 100644
--- a/vespa-feed-client/src/main/java/ai/vespa/feed/client/JsonFeeder.java
+++ b/vespa-feed-client/src/main/java/ai/vespa/feed/client/JsonFeeder.java
@@ -3,6 +3,7 @@ package ai.vespa.feed.client;
 
 import ai.vespa.feed.client.FeedClient.OperationType;
 import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonLocation;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
 
@@ -358,6 +359,12 @@ public class JsonFeeder implements Closeable {
 
         abstract String getDocumentJson(long start, long end);
 
+        OperationParseException parseException(String error) {
+            JsonLocation location = parser.getTokenLocation();
+            return new OperationParseException(error + " at offset " + location.getByteOffset() +
+                                               " (line " + location.getLineNr() + ", column " + location.getColumnNr() + ")");
+        }
+
         CompletableFuture<Result> next() throws IOException {
             JsonToken token = parser.nextToken();
             if (multipleOperations && ! arrayPrefixParsed && token == START_ARRAY) {
@@ -366,7 +373,7 @@ public class JsonFeeder implements Closeable {
             }
             if (token == END_ARRAY && multipleOperations) return null;
             else if (token == null && ! arrayPrefixParsed) return null;
-            else if (token != START_OBJECT) throw new OperationParseException("Unexpected token '" + parser.currentToken() + "' at offset " + parser.getTokenLocation().getByteOffset());
+            else if (token != START_OBJECT) throw parseException("Unexpected token '" + parser.currentToken() + "'");
             long start = 0, end = -1;
             OperationType type = null;
             DocumentId id = null;
@@ -392,8 +399,7 @@ public class JsonFeeder implements Closeable {
                                 end = parser.getTokenLocation().getByteOffset() + 1;
                                 break;
                             }
-                            default: throw new OperationParseException("Unexpected field name '" + parser.getText() + "' at offset " +
-                                    parser.getTokenLocation().getByteOffset());
+                            default: throw parseException("Unexpected field name '" + parser.getText() + "'");
                         }
                         break;
 
@@ -401,15 +407,20 @@ public class JsonFeeder implements Closeable {
                         break loop;
 
                     default:
-                        throw new OperationParseException("Unexpected token '" + parser.currentToken() + "' at offset " +
-                                parser.getTokenLocation().getByteOffset());
+                        throw parseException("Unexpected token '" + parser.currentToken() + "'");
                 }
             }
             if (id == null)
-                throw new OperationParseException("No document id for document at offset " + start);
+                throw parseException("No document id for document");
+            if (type == REMOVE) {
+                if (end >= start)
+                    throw parseException("Illegal 'fields' object for remove operation");
+                else
+                    start = end = parser.getTokenLocation().getByteOffset(); // getDocumentJson advances buffer overwrite head.
+            }
+            else if (end < start)
+                throw parseException("No 'fields' object for document");
 
-            if (end < start)
-                throw new OperationParseException("No 'fields' object for document at offset " + parser.getTokenLocation().getByteOffset());
             String payload = getDocumentJson(start, end);
             switch (type) {
                 case PUT:    return client.put   (id, payload, parameters);
@@ -479,4 +490,5 @@ public class JsonFeeder implements Closeable {
         }
 
     }
+
 }
diff --git a/vespa-feed-client/src/main/java/ai/vespa/feed/client/Result.java b/vespa-feed-client/src/main/java/ai/vespa/feed/client/Result.java
index 7be7aadc188..790b5cf0b4a 100644
--- a/vespa-feed-client/src/main/java/ai/vespa/feed/client/Result.java
+++ b/vespa-feed-client/src/main/java/ai/vespa/feed/client/Result.java
@@ -33,4 +33,14 @@ public class Result {
     public Optional<String> resultMessage() { return Optional.ofNullable(resultMessage); }
     public Optional<String> traceMessage() { return Optional.ofNullable(traceMessage); }
 
+    @Override
+    public String toString() {
+        return "Result{" +
+               "type=" + type +
+               ", documentId=" + documentId +
+               ", resultMessage='" + resultMessage + '\'' +
+               ", traceMessage='" + traceMessage + '\'' +
+               '}';
+    }
+
 }
diff --git a/vespa-feed-client/src/test/java/ai/vespa/feed/client/JsonFeederTest.java b/vespa-feed-client/src/test/java/ai/vespa/feed/client/JsonFeederTest.java
index 79dd68f241a..da0c45c94b5 100644
--- a/vespa-feed-client/src/test/java/ai/vespa/feed/client/JsonFeederTest.java
+++ b/vespa-feed-client/src/test/java/ai/vespa/feed/client/JsonFeederTest.java
@@ -10,6 +10,7 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -26,6 +27,7 @@ import static java.util.stream.Collectors.joining;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class JsonFeederTest {
@@ -60,8 +62,8 @@ class JsonFeederTest {
             long startNanos = System.nanoTime();
             MockClient feedClient = new MockClient();
             JsonFeeder.builder(feedClient).build()
-                    .feedMany(in, 1 << 7,
-                            new JsonFeeder.ResultCallback() { // TODO: hangs when buffer is smaller than largest document
+                    .feedMany(in, 1 << 10,
+                            new JsonFeeder.ResultCallback() {
                                 @Override
                                 public void onNextResult(Result result, FeedException error) { resultsReceived.incrementAndGet(); }
 
@@ -108,22 +110,30 @@ class JsonFeederTest {
     public void multipleJsonLOperationsAreDispatchedToFeedClient() throws IOException, ExecutionException, InterruptedException {
         MockClient client = new MockClient();
         try (JsonFeeder feeder = JsonFeeder.builder(client).build()) {
-            String json = "{" +
-                          "  \"put\": \"id:ns:type::abc1\",\n" +
+            String json = "{\n" +
+                          "  \"remove\": \"id:ns:type::abc1\"\n" +
+                          "}\n" +
+                          "{\n" +
                           "  \"fields\": {\n" +
-                          "    \"lul\":\"lal\"\n" +
-                          "  }\n" +
+                          "    \"lul\": { \"assign\": \"lal\" }\n" +
+                          "  },\n" +
+                          "  \"update\": \"id:ns:type::abc2\"\n" +
                           "}\n" +
-                          "{" +
-                          "  \"put\": \"id:ns:type::abc2\",\n" +
+                          "{\n" +
+                          "  \"put\": \"id:ns:type::abc3\",\n" +
                           "  \"fields\": {\n" +
-                          "    \"lul\":\"lal\"\n" +
+                          "    \"lul\": \"lal\"\n" +
                           "  }\n" +
                           "}\n";
-            feeder.feedMany(new ByteArrayInputStream(json.getBytes(UTF_8))).get();
-            client.assertPutDocumentIds("abc1", "abc2");
-            client.assertPutOperation("abc1", "{\"fields\":{\n    \"lul\":\"lal\"\n  }}");
-            client.assertPutOperation("abc2", "{\"fields\":{\n    \"lul\":\"lal\"\n  }}");
+
+            feeder.feedMany(new ByteArrayInputStream(json.getBytes(UTF_8)),
+                            new JsonFeeder.ResultCallback() { })
+                  .get();
+            client.assertRemoveDocumentIds("abc1");
+            client.assertUpdateDocumentIds("abc2");
+            client.assertUpdateOperation("abc2", "{\"fields\":{\n    \"lul\": { \"assign\": \"lal\" }\n  }}");
+            client.assertPutDocumentIds("abc3");
+            client.assertPutOperation("abc3", "{\"fields\":{\n    \"lul\": \"lal\"\n  }}");
         }
     }
 
@@ -146,6 +156,8 @@ class JsonFeederTest {
 
     private static class MockClient implements FeedClient {
         final Map<DocumentId, String> putOperations = new LinkedHashMap<>();
+        final Map<DocumentId, String> updateOperations = new LinkedHashMap<>();
+        final Map<DocumentId, String> removeOperations = new LinkedHashMap<>();
 
         @Override
         public CompletableFuture<Result> put(DocumentId documentId, String documentJson, OperationParameters params) {
@@ -155,11 +167,13 @@ class JsonFeederTest {
 
         @Override
         public CompletableFuture<Result> update(DocumentId documentId, String updateJson, OperationParameters params) {
+            updateOperations.put(documentId, updateJson);
             return createSuccessResult(documentId);
         }
 
         @Override
         public CompletableFuture<Result> remove(DocumentId documentId, OperationParameters params) {
+            removeOperations.put(documentId, null);
             return createSuccessResult(documentId);
         }
 
@@ -176,23 +190,43 @@ class JsonFeederTest {
             return CompletableFuture.completedFuture(new Result(Result.Type.success, documentId, "success", null));
         }
 
-        void assertPutDocumentIds(String... expectedUserSpecificIds) {
+        void assertDocumentIds(Collection<DocumentId> keys, String... expectedUserSpecificIds) {
             List<String> expected = Arrays.stream(expectedUserSpecificIds)
-                    .map(userSpecific -> "id:ns:type::" + userSpecific)
-                    .sorted()
-                    .collect(Collectors.toList());
-            List<String> actual = putOperations.keySet().stream()
-                    .map(DocumentId::toString).sorted()
-                    .collect(Collectors.toList());
+                                          .map(userSpecific -> "id:ns:type::" + userSpecific)
+                                          .sorted()
+                                          .collect(Collectors.toList());
+            List<String> actual = keys.stream()
+                                      .map(DocumentId::toString).sorted()
+                                      .collect(Collectors.toList());
             assertEquals(expected, actual, "Document ids must match");
         }
 
+        void assertPutDocumentIds(String... expectedUserSpecificIds) {
+            assertDocumentIds(putOperations.keySet(), expectedUserSpecificIds);
+        }
+
+        void assertUpdateDocumentIds(String... expectedUserSpecificIds) {
+            assertDocumentIds(updateOperations.keySet(), expectedUserSpecificIds);
+        }
+
+        void assertRemoveDocumentIds(String... expectedUserSpecificIds) {
+            assertDocumentIds(removeOperations.keySet(), expectedUserSpecificIds);
+        }
+
         void assertPutOperation(String userSpecificId, String expectedJson) {
             DocumentId docId = DocumentId.of("id:ns:type::" + userSpecificId);
             String json = putOperations.get(docId);
             assertNotNull(json);
             assertEquals(expectedJson.trim(), json.trim());
         }
+
+        void assertUpdateOperation(String userSpecificId, String expectedJson) {
+            DocumentId docId = DocumentId.of("id:ns:type::" + userSpecificId);
+            String json = updateOperations.get(docId);
+            assertNotNull(json);
+            assertEquals(expectedJson.trim(), json.trim());
+        }
+
     }
 
 }