Stricter parameters for new visits features

2 files changed, 94 insertions, 45 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
index 1f5c3e93572..7c80db2ddc3 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
@@ -55,6 +55,7 @@ import com.yahoo.jdisc.handler.ResponseHandler;
 import com.yahoo.jdisc.handler.UnsafeContentInputStream;
 import com.yahoo.jdisc.http.HttpRequest;
 import com.yahoo.jdisc.http.HttpRequest.Method;
+import com.yahoo.messagebus.DynamicThrottlePolicy;
 import com.yahoo.messagebus.Message;
 import com.yahoo.messagebus.StaticThrottlePolicy;
 import com.yahoo.messagebus.Trace;
@@ -148,10 +149,12 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     private static final String FIELD_SET = "fieldSet";
     private static final String SELECTION = "selection";
     private static final String CLUSTER = "cluster";
+    private static final String DESTINATION_CLUSTER = "destinationCluster";
     private static final String CONTINUATION = "continuation";
     private static final String WANTED_DOCUMENT_COUNT = "wantedDocumentCount";
     private static final String CONCURRENCY = "concurrency";
     private static final String BUCKET_SPACE = "bucketSpace";
+    private static final String TIME_CHUNK = "timeChunk";
     private static final String TIMEOUT = "timeout";
     private static final String TRACELEVEL = "tracelevel";
 
@@ -347,7 +350,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
     private ContentChannel getDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) {
         enqueueAndDispatch(request, handler, () -> {
-            VisitorParameters parameters = parseParameters(request, path);
+            VisitorParameters parameters = parseGetParameters(request, path);
             return () -> {
                 visitAndWrite(request, parameters, handler);
                 return true; // VisitorSession has its own throttle handling.
@@ -358,8 +361,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
     private ContentChannel postDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) {
         enqueueAndDispatch(request, handler, () -> {
+            StorageCluster destination = resolveCluster(Optional.of(requireProperty(request, DESTINATION_CLUSTER)), clusters);
             VisitorParameters parameters = parseParameters(request, path);
-            parameters.setRemoteDataHandler(getProperty(request, ROUTE).orElseThrow(() -> new IllegalArgumentException("Missing required property '"  + ROUTE + "'")));
+            parameters.setRemoteDataHandler("[Content:cluster=" + destination.name() + "]"); // Bypass indexing.
+            parameters.setFieldSet(AllFields.NAME);
             return () -> {
                 visitWithRemote(request, parameters, handler);
                 return true; // VisitorSession has its own throttle handling.
@@ -369,19 +374,17 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     }
 
     private ContentChannel putDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) {
-        if (getProperty(request, SELECTION).isEmpty())
-            throw new IllegalArgumentException("Missing required property '" + SELECTION + "'");
-
         return new ForwardingContentChannel(in -> {
             enqueueAndDispatch(request, handler, () -> {
-                String type = path.documentType().orElseThrow(() -> new IllegalStateException("Document type must be specified for mass updates"));
-                IdIdString dummyId = new IdIdString("dummy", type, "", "");
+                StorageCluster cluster = resolveCluster(Optional.of(requireProperty(request, CLUSTER)), clusters);
                 VisitorParameters parameters = parseParameters(request, path);
                 parameters.setFieldSet(DocIdOnly.NAME);
+                String type = path.documentType().orElseThrow(() -> new IllegalStateException("Document type must be specified for mass updates"));
+                IdIdString dummyId = new IdIdString("dummy", type, "", "");
                 DocumentUpdate update = parser.parseUpdate(in, dummyId.toString());
-                update.setCondition(new TestAndSetCondition(parameters.getDocumentSelection()));
+                update.setCondition(new TestAndSetCondition(requireProperty(request, SELECTION)));
                 return () -> {
-                    visitAndUpdate(request, parameters, handler, update, getProperty(request, ROUTE));
+                    visitAndUpdate(request, parameters, handler, update, cluster.name());
                     return true; // VisitorSession has its own throttle handling.
                 };
             });
@@ -389,15 +392,13 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     }
 
     private ContentChannel deleteDocuments(HttpRequest request, DocumentPath path, ResponseHandler handler) {
-        if (getProperty(request, SELECTION).isEmpty())
-            throw new IllegalArgumentException("Missing required property '" + SELECTION + "'");
-
         enqueueAndDispatch(request, handler, () -> {
             VisitorParameters parameters = parseParameters(request, path);
             parameters.setFieldSet(DocIdOnly.NAME);
-            TestAndSetCondition condition = new TestAndSetCondition(parameters.getDocumentSelection());
+            TestAndSetCondition condition = new TestAndSetCondition(requireProperty(request, SELECTION));
+            StorageCluster cluster = resolveCluster(Optional.of(requireProperty(request, CLUSTER)), clusters);
             return () -> {
-                visitAndDelete(request, parameters, handler, condition, getProperty(request, ROUTE));
+                visitAndDelete(request, parameters, handler, condition, cluster.name());
                 return true; // VisitorSession has its own throttle handling.
             };
         });
@@ -916,10 +917,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
     // ------------------------------------------------- Visits ------------------------------------------------
 
-    private VisitorParameters parseParameters(HttpRequest request, DocumentPath path) {
+    private VisitorParameters parseGetParameters(HttpRequest request, DocumentPath path) {
         int wantedDocumentCount = Math.min(1 << 10, getProperty(request, WANTED_DOCUMENT_COUNT, integerParser).orElse(1));
         if (wantedDocumentCount <= 0)
-             throw new IllegalArgumentException("wantedDocumentCount must be positive");
+            throw new IllegalArgumentException("wantedDocumentCount must be positive");
 
         int concurrency = Math.min(100, getProperty(request, CONCURRENCY, integerParser).orElse(1));
         if (concurrency <= 0)
@@ -929,6 +930,25 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
         if (cluster.isEmpty() && path.documentType().isEmpty())
             throw new IllegalArgumentException("Must set 'cluster' parameter to a valid content cluster id when visiting at a root /document/v1/ level");
 
+        VisitorParameters parameters = parseCommonParameters(request, path, cluster);
+        parameters.setFieldSet(getProperty(request, FIELD_SET).orElse(path.documentType().map(type -> type + ":[document]").orElse(AllFields.NAME)));
+        parameters.setMaxTotalHits(wantedDocumentCount);
+        parameters.setThrottlePolicy(new StaticThrottlePolicy().setMaxPendingCount(concurrency));
+        parameters.visitInconsistentBuckets(true);
+        parameters.setSessionTimeoutMs(Math.max(1, request.getTimeout(TimeUnit.MILLISECONDS) - 5000));
+        return parameters;
+    }
+
+    private VisitorParameters parseParameters(HttpRequest request, DocumentPath path) {
+        disallow(request, CONCURRENCY, FIELD_SET, ROUTE, WANTED_DOCUMENT_COUNT);
+        requireProperty(request, SELECTION);
+        VisitorParameters parameters = parseCommonParameters(request, path, Optional.of(requireProperty(request, CLUSTER)));
+        parameters.setThrottlePolicy(new DynamicThrottlePolicy().setMinWindowSize(1).setWindowSizeIncrement(1));
+        parameters.setSessionTimeoutMs(Math.max(1, getProperty(request, TIME_CHUNK, timeoutMillisParser).orElse(60_000L)));
+        return parameters;
+    }
+
+    private VisitorParameters parseCommonParameters(HttpRequest request, DocumentPath path, Optional<String> cluster) {
         VisitorParameters parameters = new VisitorParameters(Stream.of(getProperty(request, SELECTION),
                                                                        path.documentType(),
                                                                        path.namespace().map(value -> "id.namespace=='" + value + "'"),
@@ -940,11 +960,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                                                                    .toString());
 
         getProperty(request, CONTINUATION).map(ProgressToken::fromSerializedString).ifPresent(parameters::setResumeToken);
-        parameters.setFieldSet(getProperty(request, FIELD_SET).orElse(path.documentType().map(type -> type + ":[document]").orElse(AllFields.NAME)));
-        parameters.setMaxTotalHits(wantedDocumentCount);
-        parameters.setThrottlePolicy(new StaticThrottlePolicy().setMaxPendingCount(concurrency));
-        parameters.setSessionTimeoutMs(Math.max(1, request.getTimeout(TimeUnit.MILLISECONDS) - 5000));
-        parameters.visitInconsistentBuckets(true);
         parameters.setPriority(DocumentProtocol.Priority.NORMAL_4);
 
         StorageCluster storageCluster = resolveCluster(cluster, clusters);
@@ -969,7 +984,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     }
 
     private void visitAndDelete(HttpRequest request, VisitorParameters parameters, ResponseHandler handler,
-                                TestAndSetCondition condition, Optional<String> route) {
+                                TestAndSetCondition condition, String route) {
         visitAndProcess(request, parameters, handler, route, (id, operationParameters) -> {
             DocumentRemove remove = new DocumentRemove(id);
             remove.setCondition(condition);
@@ -978,7 +993,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     }
 
     private void visitAndUpdate(HttpRequest request, VisitorParameters parameters, ResponseHandler handler,
-                                DocumentUpdate protoUpdate, Optional<String> route) {
+                                DocumentUpdate protoUpdate, String route) {
         visitAndProcess(request, parameters, handler, route, (id, operationParameters) -> {
                 DocumentUpdate update = new DocumentUpdate(protoUpdate);
                 update.setId(id);
@@ -987,11 +1002,10 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
     }
 
     private void visitAndProcess(HttpRequest request, VisitorParameters parameters, ResponseHandler handler,
-                                 Optional<String> route, BiFunction<DocumentId, DocumentOperationParameters, Result> operation) {
+                                 String route, BiFunction<DocumentId, DocumentOperationParameters, Result> operation) {
         visit(request, parameters, handler, new VisitCallback() {
             @Override public void onDocument(JsonResponse response, Document document, Runnable ack, Consumer<String> onError) {
-                DocumentOperationParameters operationParameters = (route.isEmpty() ? parameters()
-                                                                                   : parameters().withRoute(route.get()))
+                DocumentOperationParameters operationParameters = parameters().withRoute(route)
                         .withResponseHandler(operationResponse -> {
                             outstanding.decrementAndGet();
                             switch (operationResponse.outcome()) {
@@ -1113,6 +1127,11 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
 
     // ------------------------------------------------ Helpers ------------------------------------------------
 
+    private static String requireProperty(HttpRequest request, String name) {
+        return getProperty(request, name)
+                .orElseThrow(() -> new IllegalArgumentException("Must specify '" + name + "' at '" + request.getUri().getRawPath() + "'"));
+    }
+
     /** Returns the last property with the given name, if present, or throws if this is empty or blank. */
     private static Optional<String> getProperty(HttpRequest request, String name) {
         if ( ! request.parameters().containsKey(name))
@@ -1130,6 +1149,12 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
         return getProperty(request, name).map(parser::parse);
     }
 
+    private static void disallow(HttpRequest request, String... properties) {
+        for (String property : properties)
+            if (request.parameters().containsKey(property))
+                throw new IllegalArgumentException("May not specify '" + property + "' at '" + request.getUri().getRawPath() + "'");
+    }
+
     @FunctionalInterface
     interface Parser<T> extends Function<String, T> {
         default T parse(String value) {
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
index 1147dc7962a..cee02618cd7 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
@@ -252,24 +252,24 @@ public class DocumentV1ApiTest {
                        "}", response.readAll());
         assertEquals(400, response.getStatus());
 
-        // POST with namespace and document type is a restricted visit with a required remote data handler ("route")
+        // POST with namespace and document type is a restricted visit with a required destination cluster ("destinationCluster")
         access.expect(parameters -> {
             fail("Not supposed to run");
         });
         response = driver.sendRequest("http://localhost/document/v1/space/music/docid", POST);
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/docid\"," +
-                       "  \"message\": \"Missing required property 'route'\"" +
+                       "  \"message\": \"Must specify 'destinationCluster' at '/document/v1/space/music/docid'\"" +
                        "}", response.readAll());
         assertEquals(400, response.getStatus());
 
-        // POST with namespace and document type is a restricted visit with a require remote data handler ("route")
+        // POST with namespace and document type is a restricted visit with a required destination cluster ("destinationCluster")
         access.expect(parameters -> {
-            assertEquals("zero", parameters.getRemoteDataHandler());
-            assertEquals("music:[document]", parameters.fieldSet());
+            assertEquals("[Content:cluster=content]", parameters.getRemoteDataHandler());
+            assertEquals("[all]", parameters.fieldSet());
             parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.SUCCESS, "We made it!");
         });
-        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?route=zero", POST);
+        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?destinationCluster=content&selection=true&cluster=content", POST);
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/docid\"" +
                        "}", response.readAll());
@@ -280,19 +280,20 @@ public class DocumentV1ApiTest {
         access.expect(parameters -> {
             assertEquals("(true) and (music) and (id.namespace=='space')", parameters.getDocumentSelection());
             assertEquals("[id]", parameters.fieldSet());
+            assertEquals(10_000, parameters.getSessionTimeoutMs());
             parameters.getLocalDataHandler().onMessage(new PutDocumentMessage(new DocumentPut(doc3)), tokens.get(2));
             parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.SUCCESS, "Huzzah!");
         });
         access.session.expect((update, parameters) -> {
             DocumentUpdate expectedUpdate = new DocumentUpdate(doc3.getDataType(), doc3.getId());
             expectedUpdate.addFieldUpdate(FieldUpdate.createAssign(doc3.getField("artist"), new StringFieldValue("Lisa Ekdahl")));
-            expectedUpdate.setCondition(new TestAndSetCondition("(true) and (music) and (id.namespace=='space')"));
+            expectedUpdate.setCondition(new TestAndSetCondition("true"));
             assertEquals(expectedUpdate, update);
             parameters.responseHandler().get().handleResponse(new UpdateResponse(0, false));
-            assertEquals(parameters().withRoute("zero"), parameters);
+            assertEquals(parameters().withRoute("content"), parameters);
             return new Result(Result.ResultType.SUCCESS, null);
         });
-        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=true&route=zero", PUT,
+        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=true&cluster=content&timeChunk=10", PUT,
                                       "{" +
                                       "  \"fields\": {" +
                                       "    \"artist\": { \"assign\": \"Lisa Ekdahl\" }" +
@@ -303,14 +304,25 @@ public class DocumentV1ApiTest {
                        "}", response.readAll());
         assertEquals(200, response.getStatus());
 
+        // PUT with namespace, document type and group is also a restricted visit which requires a cluster.
+        access.expect(parameters -> {
+            fail("Not supposed to run");
+        });
+        response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe?selection=false", PUT);
+        assertSameJson("{" +
+                       "  \"pathId\": \"/document/v1/space/music/group/troupe\"," +
+                       "  \"message\": \"Must specify 'cluster' at '/document/v1/space/music/group/troupe'\"" +
+                       "}", response.readAll());
+        assertEquals(400, response.getStatus());
+
         // PUT with namespace, document type and group is also a restricted visit which requires a selection.
         access.expect(parameters -> {
             fail("Not supposed to run");
         });
-        response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe", PUT);
+        response = driver.sendRequest("http://localhost/document/v1/space/music/group/troupe?cluster=content", PUT);
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/group/troupe\"," +
-                       "  \"message\": \"Missing required property 'selection'\"" +
+                       "  \"message\": \"Must specify 'selection' at '/document/v1/space/music/group/troupe'\"" +
                        "}", response.readAll());
         assertEquals(400, response.getStatus());
 
@@ -319,32 +331,44 @@ public class DocumentV1ApiTest {
         access.expect(parameters -> {
             assertEquals("(false) and (music) and (id.namespace=='space')", parameters.getDocumentSelection());
             assertEquals("[id]", parameters.fieldSet());
+            assertEquals(60_000, parameters.getSessionTimeoutMs());
             parameters.getLocalDataHandler().onMessage(new PutDocumentMessage(new DocumentPut(doc2)), tokens.get(0));
             parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.ABORTED, "Huzzah?");
         });
         access.session.expect((remove, parameters) -> {
             DocumentRemove expectedRemove = new DocumentRemove(doc2.getId());
-            expectedRemove.setCondition(new TestAndSetCondition("(false) and (music) and (id.namespace=='space')"));
-            assertEquals(new DocumentRemove(doc2.getId()), remove);
-            assertEquals(parameters().withRoute("zero"), parameters);
+            expectedRemove.setCondition(new TestAndSetCondition("false"));
+            assertEquals(expectedRemove, remove);
+            assertEquals(parameters().withRoute("content"), parameters);
             parameters.responseHandler().get().handleResponse(new DocumentIdResponse(0, doc2.getId(), "boom", Response.Outcome.ERROR));
             return new Result(Result.ResultType.SUCCESS, null);
         });
-        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=false&route=zero", DELETE);
+        response = driver.sendRequest("http://localhost/document/v1/space/music/docid?selection=false&cluster=content", DELETE);
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/docid\"," +
                        "  \"message\": \"boom\"" +
                        "}", response.readAll());
         assertEquals(500, response.getStatus());
 
-        // DELETE at the root is also a deletion visit. These require a selection.
+        // DELETE at the root is also a deletion visit. These also require a selection.
         access.expect(parameters -> {
             fail("Not supposed to run");
         });
-        response = driver.sendRequest("http://localhost/document/v1/space/music/docid", DELETE);
+        response = driver.sendRequest("http://localhost/document/v1/", DELETE);
         assertSameJson("{" +
-                       "  \"pathId\": \"/document/v1/space/music/docid\"," +
-                       "  \"message\": \"Missing required property 'selection'\"" +
+                       "  \"pathId\": \"/document/v1/\"," +
+                       "  \"message\": \"Must specify 'selection' at '/document/v1/'\"" +
+                       "}", response.readAll());
+        assertEquals(400, response.getStatus());
+
+        // DELETE at the root is also a deletion visit. These also require a cluster.
+        access.expect(parameters -> {
+            fail("Not supposed to run");
+        });
+        response = driver.sendRequest("http://localhost/document/v1/?selection=true", DELETE);
+        assertSameJson("{" +
+                       "  \"pathId\": \"/document/v1/\"," +
+                       "  \"message\": \"Must specify 'cluster' at '/document/v1/'\"" +
                        "}", response.readAll());
         assertEquals(400, response.getStatus());