Fix parsing of array query properties in JSON payload

CONTRIBUTING.md

@@ -1,6 +1,7 @@
 <!-- Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -->
 # Contributing to Vespa
 Contributions to [Vespa](http://github.com/vespa-engine/vespa),
+[Vespa system tests](http://github.com/vespa-engine/system-test),
 [Vespa samples](https://github.com/vespa-engine/sample-apps)
 and the [Vespa documentation](http://github.com/vespa-engine/documentation) are welcome.
 This documents tells you what you need to know to contribute.

container-search/src/main/java/com/yahoo/fs4/QueryPacket.java

@@ -237,7 +237,7 @@ private int getFlagInt() {
          * such a way the comparing SORTDATA for two different hits
          * will reproduce the order in which the data were returned when
          * using sortspec.  For now we simply drop these. If they
-         * become necessar, QueryResultPacket must be
+         * become necessary, QueryResultPacket must be
          * updated to be able to read the sort data.
          */
         flags |= QFLAG_DROP_SORTDATA;

container-search/src/main/java/com/yahoo/prelude/query/Highlight.java

@@ -8,7 +8,7 @@
 /**
  * Class encapsulating information on extra highlight-terms for a query
  *
- * @author <a href="mailto:mathiasm@yahoo-inc.com">Mathias Lidal</a>
+ * @author Mathias Lidal
  */
 public class Highlight implements Cloneable {
 

container-search/src/main/java/com/yahoo/search/Query.java

@@ -424,7 +424,7 @@ else if (field.getType() instanceof QueryProfileFieldType) { // Nested arguments
 
     /** Calls properties.set on all entries in requestMap */
     private void setPropertiesFromRequestMap(Map<String, String> requestMap, Properties properties) {
-        for (Map.Entry<String, String> entry : requestMap.entrySet()) {
+        for (var entry : requestMap.entrySet()) {
             try {
                 properties.set(entry.getKey(), entry.getValue(), requestMap);
             }
@@ -771,7 +771,7 @@ private String queryTreeText() {
 
     /**
      * Serialize this query as YQL+. This method will never throw exceptions,
-     * but instead return a human readable error message if a problem occured
+     * but instead return a human readable error message if a problem occurred while
      * serializing the query. Hits and offset information will be included if
      * different from default, while linguistics metadata are not added.
      *
@@ -783,9 +783,10 @@ public String yqlRepresentation() {
             return yqlRepresentation(true);
         } catch (NullItemException e) {
             return "Query currently a placeholder, NullItem encountered.";
+        } catch (IllegalArgumentException e) {
+            return "Invalid query: " + Exceptions.toMessageString(e);
         } catch (RuntimeException e) {
-            return "Failed serializing query as YQL+, please file a ticket including the query causing this: "
-                    + Exceptions.toMessageString(e);
+            return "Unepected error parsing or serializing query: " + Exceptions.toMessageString(e);
         }
     }
 

container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java

@@ -517,47 +517,36 @@ static private String getMediaType(HttpRequest request) {
         return com.yahoo.text.Lowercase.toLowerCase(header.trim());
     }
 
+    /** Add properties POSTed as a JSON payload, if any, to the request map */
     private Map<String, String> requestMapFromRequest(HttpRequest request) {
+        if (request.getMethod() != com.yahoo.jdisc.http.HttpRequest.Method.POST
+            ||  ! JSON_CONTENT_TYPE.equals(getMediaType(request)))
+            return request.propertyMap();
 
-        if (request.getMethod() == com.yahoo.jdisc.http.HttpRequest.Method.POST
-            && JSON_CONTENT_TYPE.equals(getMediaType(request)))
-        {
-            Inspector inspector;
-            try {
-                byte[] byteArray = IOUtils.readBytes(request.getData(), 1 << 20);
-                inspector = SlimeUtils.jsonToSlime(byteArray).get();
-                if (inspector.field("error_message").valid()){
-                    throw new QueryException("Illegal query: " + inspector.field("error_message").asString() + ", at: " +
-                                             new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8));
-                }
-
-            } catch (IOException e) {
-                throw new RuntimeException("Problem with reading from input-stream", e);
-            }
-
-            // Create request-mapping
-            Map<String, String> requestMap = new HashMap<>();
-
-            createRequestMapping(inspector, requestMap, "");
-
-            requestMap.putAll(request.propertyMap());
-
-            // Throws QueryException if query contains both yql- and select-parameter
-            if (requestMap.containsKey("yql") && (requestMap.containsKey("select.where") || requestMap.containsKey("select.grouping")) ) {
-                throw new QueryException("Illegal query: Query contains both yql- and select-parameter");
+        Inspector inspector;
+        try {
+            byte[] byteArray = IOUtils.readBytes(request.getData(), 1 << 20);
+            inspector = SlimeUtils.jsonToSlime(byteArray).get();
+            if (inspector.field("error_message").valid()) {
+                throw new QueryException("Illegal query: " + inspector.field("error_message").asString() + " at: '" +
+                                         new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8) + "'");
             }
 
-            // Throws QueryException if query contains both query- and select-parameter
-            if (requestMap.containsKey("query") && (requestMap.containsKey("select.where") || requestMap.containsKey("select.grouping")) ) {
-                throw new QueryException("Illegal query: Query contains both query- and select-parameter");
-            }
+        } catch (IOException e) {
+            throw new RuntimeException("Problem reading POSTed data", e);
+        }
 
-            return requestMap;
+        // Add fields from JSON to the request map
+        Map<String, String> requestMap = new HashMap<>();
+        createRequestMapping(inspector, requestMap, "");
+        requestMap.putAll(request.propertyMap());
 
-        } else {
-            return request.propertyMap();
+        if (requestMap.containsKey("yql") && (requestMap.containsKey("select.where") || requestMap.containsKey("select.grouping")) )
+            throw new QueryException("Illegal query: Query contains both yql and select parameter");
+        if (requestMap.containsKey("query") && (requestMap.containsKey("select.where") || requestMap.containsKey("select.grouping")) )
+            throw new QueryException("Illegal query: Query contains both query and select parameter");
 
-        }
+        return requestMap;
     }
 
     public void createRequestMapping(Inspector inspector, Map<String, String> map, String parent) {
@@ -577,14 +566,14 @@ public void createRequestMapping(Inspector inspector, Map<String, String> map, S
                     map.put(qualifiedKey , value.asString());
                     break;
                 case ARRAY:
-                    map.put(qualifiedKey, value.asString());
+                    map.put(qualifiedKey, value.toString()); // XXX: Causes parsing the JSON twice (Query.setPropertiesFromRequestMap)
                     break;
                 case OBJECT:
                     if (qualifiedKey.equals("select.where") || qualifiedKey.equals("select.grouping")) {
-                        map.put(qualifiedKey, value.toString());
+                        map.put(qualifiedKey, value.toString());  // XXX: Causes parsing the JSON twice (Query.setPropertiesFromRequestMap)
                         break;
                     }
-                    createRequestMapping(value, map, qualifiedKey+".");
+                    createRequestMapping(value, map, qualifiedKey + ".");
                     break;
             }
 

container-search/src/main/java/com/yahoo/search/query/SelectParser.java

@@ -150,15 +150,14 @@ public QueryTree parse(Parsable query) {
 
     private QueryTree buildTree() {
         Inspector inspector = SlimeUtils.jsonToSlime(this.query.getSelect().getWhereString().getBytes()).get();
-        if (inspector.field("error_message").valid()){
-            throw new QueryException("Illegal query: "+inspector.field("error_message").asString() + ", at: "+ new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8));
+        if (inspector.field("error_message").valid()) {
+            throw new QueryException("Illegal query: " + inspector.field("error_message").asString() +
+                                     " at: '" + new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8) + "'");
         }
 
         Item root = walkJson(inspector);
         connectItems();
-        QueryTree newTree = new QueryTree(root);
-
-        return newTree;
+        return new QueryTree(root);
     }
 
     private Item walkJson(Inspector inspector){
@@ -213,7 +212,8 @@ private List<String> getOperations(String grouping) {
         List<String> operations = new ArrayList<>();
         Inspector inspector = SlimeUtils.jsonToSlime(grouping.getBytes()).get();
         if (inspector.field("error_message").valid()){
-            throw new QueryException("Illegal query: "+inspector.field("error_message").asString() + ", at: "+ new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8));
+            throw new QueryException("Illegal query: "+inspector.field("error_message").asString() +
+                                     " at: '" + new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8) + "'");
         }
 
         inspector.traverse( (ArrayTraverser) (key, value) -> {
@@ -786,7 +786,7 @@ private Item instantiateLeafItem(String field, String key, Inspector value) {
         String possibleLeafFunctionName = (possibleLeafFunction.size() > 1) ? getInspectorKey(possibleLeafFunction.get(1)) : "";
         if (FUNCTION_CALLS.contains(key)) {
             return instantiateCompositeLeaf(field, key, value);
-        } else if(!possibleLeafFunctionName.equals("")){
+        } else if ( ! possibleLeafFunctionName.equals("")){
            return instantiateCompositeLeaf(field, possibleLeafFunctionName, valueListFromInspector(value).get(1).field(possibleLeafFunctionName));
         } else {
             return instantiateWordItem(field, key, value);
@@ -815,7 +815,11 @@ private Item instantiateCompositeLeaf(String field, String key, Inspector value)
 
     @NonNull
     private Item instantiateWordItem(String field, String key, Inspector value) {
-        String wordData = getChildrenMap(value).get(1).asString();
+        var children = getChildrenMap(value);
+        if (children.size() < 2)
+            throw new IllegalArgumentException("Expected at least 2 children of '" + key + "', but got " + children.size());
+
+        String wordData = children.get(1).asString();
         return instantiateWordItem(field, wordData, key, value, false, decideParsingLanguage(value, wordData));
     }
 

container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java

@@ -304,11 +304,11 @@ public void testSelectParameters() throws Exception {
 
         JSONObject select = new JSONObject();
 
-            JSONObject where = new JSONObject();
-            where.put("where", "where");
+        JSONObject where = new JSONObject();
+        where.put("where", "where");
 
-            JSONObject grouping = new JSONObject();
-            grouping.put("grouping", "grouping");
+        JSONObject grouping = new JSONObject();
+        grouping.put("grouping", "grouping");
 
         select.put("where", where);
         select.put("grouping", grouping);
@@ -344,6 +344,54 @@ public void testJsonQueryWithSelectWhere() throws Exception {
                      result);
     }
 
+    @Test
+    public void testJsonWithWhereAndGroupingUnderSelect() {
+        String query = "{\n" +
+                       "  \"select\": {\n" +
+                       "    \"where\": {\n" +
+                       "      \"contains\": [\n" +
+                       "        \"field\",\n" +
+                       "        \"term\"\n" +
+                       "      ]\n" +
+                       "    },\n" +
+                       "    \"grouping\":[\n" +
+                       "      {\n" +
+                       "        \"all\": {\n" +
+                       "          \"output\": \"count()\"\n" +
+                       "        }\n" +
+                       "      }\n" +
+                       "    ]\n" +
+                       "  }\n" +
+                       "}\n";
+        String result = driver.sendRequest(uri + "searchChain=echoingQuery", com.yahoo.jdisc.http.HttpRequest.Method.POST, query, JSON_CONTENT_TYPE).readAll();
+
+        String expected = "{\"root\":{\"id\":\"toplevel\",\"relevance\":1.0,\"fields\":{\"totalCount\":0},\"children\":[{\"id\":\"Query\",\"relevance\":1.0,\"fields\":{\"query\":\"select * from sources * where field contains \\\"term\\\" | all(output(count()));\"}}]}}";
+        assertEquals(expected, result);
+    }
+
+    @Test
+    public void testJsonWithWhereAndGroupingSeparate() {
+        String query = "{\n" +
+                       "  \"select.where\": {\n" +
+                       "    \"contains\": [\n" +
+                       "      \"field\",\n" +
+                       "      \"term\"\n" +
+                       "    ]\n" +
+                       "  },\n" +
+                       "  \"select.grouping\":[\n" +
+                       "    {\n" +
+                       "      \"all\": {\n" +
+                       "        \"output\": \"count()\"\n" +
+                       "      }\n" +
+                       "    }\n" +
+                       "  ]\n" +
+                       "}\n";
+        String result = driver.sendRequest(uri + "searchChain=echoingQuery", com.yahoo.jdisc.http.HttpRequest.Method.POST, query, JSON_CONTENT_TYPE).readAll();
+
+        String expected = "{\"root\":{\"id\":\"toplevel\",\"relevance\":1.0,\"fields\":{\"totalCount\":0},\"children\":[{\"id\":\"Query\",\"relevance\":1.0,\"fields\":{\"query\":\"select * from sources * where field contains \\\"term\\\" | all(output(count()));\"}}]}}";
+        assertEquals(expected, result);
+    }
+
     @Test
     public void testJsonQueryWithYQL() throws Exception {
         JSONObject root = new JSONObject();

container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java

@@ -8,14 +8,14 @@
 import com.yahoo.search.query.profile.BackedOverridableQueryProfile;
 import com.yahoo.search.query.profile.QueryProfile;
 import com.yahoo.search.query.profile.QueryProfileProperties;
-import com.yahoo.search.query.profile.QueryProfileRegistry;
 import com.yahoo.search.query.profile.compiled.CompiledQueryProfile;
 import org.junit.Test;
 
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 
@@ -82,6 +82,27 @@ public void testVariantsOfInlineCompound() {
         assertEquals("a.b.x2",cprofile.get("a.b", toMap("x=x2")));
     }
 
+    @Test
+    public void testOverlappingProfiles() {
+        QueryProfile profile = new QueryProfile("test");
+        profile.setDimensions(new String[] {"region", "model", "bucket"});
+        profile.set("a", "us,nokia,*  : a", new String[] {"us", "nokia", null     }, null);
+        profile.set("b", "us,nokia,*  : b", new String[] {"us", "nokia", null     }, null);
+        profile.set("c", "us,*,bucket1: c", new String[] {"us", null,    "bucket1"}, null);
+        profile.set("d", "us,*,bucket1: d", new String[] {"us", null,    "bucket1"}, null);
+
+        CompiledQueryProfile cprofile = profile.compile(null);
+        var parameters = toMap("region=us", "model=nokia", "bucket=bucket1");
+        assertEquals("us,nokia,*  : a", cprofile.get("a", parameters));
+        assertEquals("us,nokia,*  : b", cprofile.get("b", parameters));
+        assertEquals("us,*,bucket1: c", cprofile.get("c", parameters));
+        assertEquals("us,*,bucket1: d", cprofile.get("d", parameters));
+
+        String listedKeys =
+                cprofile.listValues("", parameters).keySet().stream().sorted().collect(Collectors.joining(", "));
+        assertEquals("a, b, c, d", listedKeys);
+    }
+
     @Test
     public void testVariantsOfExplicitCompound() {
         QueryProfile a1=new QueryProfile("a1");

vespajlib/abi-spec.json

@@ -2712,6 +2712,7 @@
     "methods": [
       "public static boolean isTextCharacter(int)",
       "public static java.util.OptionalInt validateTextString(java.lang.String)",
+      "public static boolean isDisplayable(int)",
       "public static java.lang.String stripInvalidCharacters(java.lang.String)"
     ],
     "fields": []