Skip to content

Commit

Permalink
Merge pull request #8697 from vespa-engine/bratseth/fix-parsing-array…
Browse files Browse the repository at this point in the history 
…-in-grouping-select-json

Fix parsing of array query properties in JSON payload
  • Loading branch information
Jon Bratseth committed Mar 7, 2019
2 parents 78a9d7d + bb8ce19 commit a62d29d
Show file tree
Hide file tree
Showing 13 changed files with 190 additions and 71 deletions.
1 change: 1 addition & 0 deletions CONTRIBUTING.md
View file
@@ -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.
Expand Down
2 changes: 1 addition & 1 deletion container-search/src/main/java/com/yahoo/fs4/QueryPacket.java
View file
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion container-search/src/main/java/com/yahoo/prelude/query/Highlight.java
View file
Expand Up @@ -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 {

Expand Down
9 changes: 5 additions & 4 deletions container-search/src/main/java/com/yahoo/search/Query.java
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
*
Expand All @@ -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 "Unexpected error parsing or serializing query: " + Exceptions.toMessageString(e);
}
}

Expand Down
63 changes: 26 additions & 37 deletions container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java
View file
Expand Up @@ -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) {
Expand All @@ -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;
}

Expand Down
20 changes: 12 additions & 8 deletions container-search/src/main/java/com/yahoo/search/query/SelectParser.java
View file
Expand Up @@ -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){
Expand Down Expand Up @@ -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) -> {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
}

Expand Down
56 changes: 52 additions & 4 deletions container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
23 changes: 22 additions & 1 deletion ...earch/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
Expand Down
1 change: 1 addition & 0 deletions vespajlib/abi-spec.json
View file
Expand Up @@ -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": []
Expand Down

0 comments on commit a62d29d

Please sign in to comment.