diff options
12 files changed, 189 insertions, 71 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0f3bd2f5140..b10f29edfb4 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/container-search/src/main/java/com/yahoo/fs4/QueryPacket.java b/container-search/src/main/java/com/yahoo/fs4/QueryPacket.java index 77fb740043e..028b0176c85 100644 --- a/container-search/src/main/java/com/yahoo/fs4/QueryPacket.java +++ b/container-search/src/main/java/com/yahoo/fs4/QueryPacket.java @@ -237,7 +237,7 @@ public class QueryPacket extends Packet { * 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; diff --git a/container-search/src/main/java/com/yahoo/prelude/query/Highlight.java b/container-search/src/main/java/com/yahoo/prelude/query/Highlight.java index 4910f63423e..81c68ccc2b9 100644 --- a/container-search/src/main/java/com/yahoo/prelude/query/Highlight.java +++ b/container-search/src/main/java/com/yahoo/prelude/query/Highlight.java @@ -8,7 +8,7 @@ import static com.yahoo.language.LinguisticsCase.toLowerCase; /** * 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 { diff --git a/container-search/src/main/java/com/yahoo/search/Query.java b/container-search/src/main/java/com/yahoo/search/Query.java index 36a04bb86e8..7c01f2d994b 100644 --- a/container-search/src/main/java/com/yahoo/search/Query.java +++ b/container-search/src/main/java/com/yahoo/search/Query.java @@ -424,7 +424,7 @@ public class Query extends com.yahoo.processing.Request implements Cloneable { /** 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 @@ public class Query extends com.yahoo.processing.Request implements Cloneable { /** * 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 class Query extends com.yahoo.processing.Request implements Cloneable { 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); } } diff --git a/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java b/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java index d2490ec9532..14a15e71bf7 100644 --- a/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java +++ b/container-search/src/main/java/com/yahoo/search/handler/SearchHandler.java @@ -517,47 +517,36 @@ public class SearchHandler extends LoggingRequestHandler { 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 class SearchHandler extends LoggingRequestHandler { 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; } diff --git a/container-search/src/main/java/com/yahoo/search/query/SelectParser.java b/container-search/src/main/java/com/yahoo/search/query/SelectParser.java index e4e44985b53..46c8a8af41f 100644 --- a/container-search/src/main/java/com/yahoo/search/query/SelectParser.java +++ b/container-search/src/main/java/com/yahoo/search/query/SelectParser.java @@ -150,15 +150,14 @@ public class SelectParser implements Parser { 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 @@ public class SelectParser implements Parser { 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 @@ public class SelectParser implements Parser { 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 @@ public class SelectParser implements Parser { @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)); } diff --git a/container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java b/container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java index 9617e0ceb25..87d92e1344a 100644 --- a/container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/handler/test/JSONSearchHandlerTestCase.java @@ -304,11 +304,11 @@ public class JSONSearchHandlerTestCase { 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); @@ -345,6 +345,54 @@ public class JSONSearchHandlerTestCase { } @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(); root.put("yql", "select * from sources * where default contains 'bad';"); diff --git a/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java b/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java index 9a7a57d5e10..11dc1313f82 100644 --- a/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileVariantsTestCase.java @@ -8,7 +8,6 @@ import com.yahoo.search.query.Properties; 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; @@ -16,6 +15,7 @@ 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; @@ -83,6 +83,27 @@ public class QueryProfileVariantsTestCase { } @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"); a1.set("b","a.b", null); diff --git a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java index a761c91f64f..f199fefd185 100644 --- a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java +++ b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java @@ -1,9 +1,12 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.slime; +import com.yahoo.text.Text; import com.yahoo.text.Utf8; +import org.w3c.dom.CharacterData; import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; /** * A port of the C++ json decoder intended to be fast. @@ -56,7 +59,7 @@ public class JsonDecoder { case '-': case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': decodeNumber(inserter); return; } - in.fail("invalid initial character for value"); + in.fail("Expected start of value but got " + characterToReadableString(c)); } @SuppressWarnings("fallthrough") @@ -86,14 +89,13 @@ public class JsonDecoder { } } - private void expect(byte [] expected) { - int i; - for (i = 0; i < expected.length && skip(expected[i]); i++) - ; - if (i != expected.length) { - in.fail("unexpected character"); + private void expect(byte[] expected) { + for (int i = 0; i < expected.length; i++) { + if ( ! skip(expected[i])) { + in.fail("Unexpected " + characterToReadableString(c)); + return; + } } - } private void decodeArray(Inserter inserter) { @@ -170,7 +172,7 @@ public class JsonDecoder { case 't': buf.write((byte) '\t'); break; case 'u': writeUtf8(dequoteUtf16(), buf, 0xffffff80); continue; default: - in.fail("invalid quoted char(" + c + ")"); + in.fail("Invalid quoted char(" + c + ")"); break; } next(); @@ -185,7 +187,7 @@ public class JsonDecoder { } break; case '\0': - in.fail("unterminated string"); + in.fail("Unterminated string"); return Utf8.toString(buf.toByteArray()); default: buf.write(c); @@ -216,10 +218,10 @@ public class JsonDecoder { if (low >= 0xdc00 && low < 0xe000) { codepoint = 0x10000 + ((codepoint - 0xd800) << 10) + (low - 0xdc00); } else { - in.fail("missing low surrogate"); + in.fail("Missing low surrogate"); } } else if (codepoint < 0xe000) { // low - in.fail("unexpected low surrogate"); + in.fail("Unexpected low surrogate"); } } return codepoint; @@ -246,7 +248,7 @@ public class JsonDecoder { case 'e': case 'E': ret = (ret << 4) | 0xe; break; case 'f': case 'F': ret = (ret << 4) | 0xf; break; default: - in.fail("invalid hex character"); + in.fail("Invalid hex character"); return 0; } next(); @@ -255,7 +257,7 @@ public class JsonDecoder { } private void next() { - if (!in.eof()) { + if ( ! in.eof()) { c = in.getByte(); } else { c = 0; @@ -281,4 +283,13 @@ public class JsonDecoder { } } + private String characterToReadableString(int codePoint) { + if (codePoint == 0) + return "end of data"; + else if (Text.isDisplayable(codePoint)) + return "character '" + String.valueOf(Character.toChars(c)) + "'"; + else + return "character code " + c; + } + } diff --git a/vespajlib/src/main/java/com/yahoo/slime/SlimeInserter.java b/vespajlib/src/main/java/com/yahoo/slime/SlimeInserter.java index 2b048c42f73..30ebb5d2a3d 100644 --- a/vespajlib/src/main/java/com/yahoo/slime/SlimeInserter.java +++ b/vespajlib/src/main/java/com/yahoo/slime/SlimeInserter.java @@ -4,8 +4,9 @@ package com.yahoo.slime; /** * Helper class for inserting values into a Slime object. * For justification read Inserter documentation. - **/ + */ public final class SlimeInserter implements Inserter { + private Slime target; public SlimeInserter(Slime target) { @@ -26,4 +27,5 @@ public final class SlimeInserter implements Inserter { public final Cursor insertDATA(byte[] value) { return target.setData(value); } public final Cursor insertARRAY() { return target.setArray(); } public final Cursor insertOBJECT() { return target.setObject(); } + } diff --git a/vespajlib/src/main/java/com/yahoo/text/Text.java b/vespajlib/src/main/java/com/yahoo/text/Text.java index 027521ec1ad..0acb2407e68 100644 --- a/vespajlib/src/main/java/com/yahoo/text/Text.java +++ b/vespajlib/src/main/java/com/yahoo/text/Text.java @@ -109,6 +109,33 @@ public final class Text { return OptionalInt.empty(); } + /** Returns whether the given code point is displayable. */ + public static boolean isDisplayable(int codePoint) { + switch (Character.getType(codePoint)) { + case Character.CONNECTOR_PUNCTUATION : + case Character.DASH_PUNCTUATION : + case Character.START_PUNCTUATION : + case Character.END_PUNCTUATION : + case Character.INITIAL_QUOTE_PUNCTUATION : + case Character.FINAL_QUOTE_PUNCTUATION: + case Character.OTHER_PUNCTUATION : + case Character.LETTER_NUMBER : + case Character.OTHER_LETTER : + case Character.LOWERCASE_LETTER : + case Character.TITLECASE_LETTER : + case Character.MODIFIER_LETTER : + case Character.UPPERCASE_LETTER : + case Character.DECIMAL_DIGIT_NUMBER : + case Character.OTHER_NUMBER : + case Character.CURRENCY_SYMBOL : + case Character.OTHER_SYMBOL : + case Character.MATH_SYMBOL : + return true; + default : + return false; + } + } + private static StringBuilder lazy(StringBuilder sb, String s, int i) { if (sb == null) { sb = new StringBuilder(s.substring(0, i)); diff --git a/vespajlib/src/test/java/com/yahoo/text/TextTestCase.java b/vespajlib/src/test/java/com/yahoo/text/TextTestCase.java index f6831e3d221..e733b838c39 100644 --- a/vespajlib/src/test/java/com/yahoo/text/TextTestCase.java +++ b/vespajlib/src/test/java/com/yahoo/text/TextTestCase.java @@ -7,6 +7,7 @@ import java.util.OptionalInt; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; public class TextTestCase { @@ -47,4 +48,17 @@ public class TextTestCase { assertEquals(OptionalInt.of(0xD800), Text.validateTextString(new StringBuilder().appendCodePoint(0xD800).append(0x0000).toString())); } + @Test + public void testIsDisplayable() { + assertTrue(Text.isDisplayable('A')); + assertTrue(Text.isDisplayable('a')); + assertTrue(Text.isDisplayable('5')); + assertTrue(Text.isDisplayable(',')); + assertTrue(Text.isDisplayable('\"')); + assertTrue(Text.isDisplayable('}')); + assertTrue(Text.isDisplayable('-')); + assertFalse(Text.isDisplayable(' ')); + assertFalse(Text.isDisplayable(0)); + } + } |