Allow both group/numeric ID and selection to be specified at the same time

Resulting selection expression is a conjunction of the group/number ID
sub-expression and the provided selection sub-expression.

2 files changed, 36 insertions, 24 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
index 8639d4fad87..1dedbd857c3 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
@@ -308,22 +308,26 @@ public class RestApi extends LoggingRequestHandler {
         }
     }
 
+    private static String validateAndBuildLocationSubExpression(RestUri.Group group) {
+        if (group.name == 'n') {
+            return String.format("id.user==%d", parseAndValidateVisitNumericId(group.value));
+        } else {
+            // TODO first pass through Text.validateTextString? Cannot create doc IDs that don't match that anyway...
+            return String.format("id.group=='%s'", singleQuoteEscapedString(group.value));
+        }
+    }
+
     private static String documentSelectionFromRequest(RestUri restUri, HttpRequest request) throws BadRequestParameterException {
+        // TODO try to preemptively parse sub expression to ensure it is complete
         String documentSelection = Optional.ofNullable(request.getProperty(SELECTION)).orElse("");
         if (restUri.getGroup().isPresent() && ! restUri.getGroup().get().value.isEmpty()) {
-            if (! documentSelection.isEmpty()) {
-                // TODO why is this restriction in place? Document selection allows composition of location predicate and other expressions
-                throw new BadRequestParameterException(SELECTION, "Visiting does not support setting value for group/value in combination with expression, try using only expression parameter instead.");
-            }
-            RestUri.Group group = restUri.getGroup().get();
-            if (group.name == 'n') {
-                documentSelection = String.format("id.user==%d", parseAndValidateVisitNumericId(group.value));
+            String locationSubExpression = validateAndBuildLocationSubExpression(restUri.getGroup().get());
+            if (documentSelection.isEmpty()) {
+                documentSelection = locationSubExpression;
             } else {
-                // TODO first pass through Text.validateTextString? Cannot create doc IDs that don't match that anyway...
-                documentSelection = String.format("id.group=='%s'", singleQuoteEscapedString(group.value));
+                documentSelection = String.format("%s and (%s)", locationSubExpression, documentSelection);
             }
         }
-        // TODO try to preemptively parse sub expression to ensure it is complete
         return documentSelection;
     }
 
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
index be8915496ac..ed7935b698b 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
@@ -280,23 +280,9 @@ public class RestApiTest {
         assertThat(rest, containsString(visit_response_part3));
     }
 
-    // TODO why is this a limitation?
-    String visit_test_bad_uri = "/document/v1/namespace/document-type/group/abc?continuation=abc&selection=foo";
-    String visit_test_bad_response = "Visiting does not support setting value for group/value in combination with expression";
-
-
-    @Test
-    public void testBadVisit() throws Exception {
-        Request request = new Request("http://localhost:" + getFirstListenPort() + visit_test_bad_uri);
-        HttpGet get = new HttpGet(request.getUri());
-        String rest = doRest(get);
-        assertThat(rest, containsString(visit_test_bad_response));
-    }
-
     String visit_test_uri_selection_rewrite = "/document/v1/namespace/document-type/group/abc?continuation=abc";
     String visit_test_response_selection_rewrite = "doc selection: 'id.group=='abc''";
 
-
     @Test
     public void testUseExpressionOnVisit() throws Exception {
         Request request = new Request("http://localhost:" + getFirstListenPort() + visit_test_uri_selection_rewrite);
@@ -354,6 +340,28 @@ public class RestApiTest {
     }
 
     @Test
+    public void can_specify_numeric_id_without_explicit_selection() {
+        assertResultingDocumentSelection("number/1234", "id.user==1234");
+    }
+
+    @Test
+    public void can_specify_group_id_without_explicit_selection() {
+        assertResultingDocumentSelection("group/foo", "id.group=='foo'");
+    }
+
+    @Test
+    public void can_specify_both_numeric_id_and_explicit_selection() {
+        assertResultingDocumentSelection(String.format("number/1234?selection=%s", encoded("1 != 2")),
+                "id.user==1234 and (1 != 2)");
+    }
+
+    @Test
+    public void can_specify_both_group_id_and_explicit_selection() {
+        assertResultingDocumentSelection(String.format("group/bar?selection=%s", encoded("3 != 4")),
+                "id.group=='bar' and (3 != 4)");
+    }
+
+    @Test
     public void wanted_document_count_returned_parameter_is_propagated() throws IOException {
         Request request = new Request(String.format("http://localhost:%s/document/v1/namespace/document-type/docid/?wantedDocumentCount=321", getFirstListenPort()));
         HttpGet get = new HttpGet(request.getUri());