lucene-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From yo...@apache.org
Subject lucene-solr:branch_7_4: SOLR-9685: fix parsing of tagged sub-queries
Date Tue, 12 Jun 2018 20:08:22 GMT
Repository: lucene-solr
Updated Branches:
  refs/heads/branch_7_4 3ed77ebd8 -> 7db0895f2


SOLR-9685: fix parsing of tagged sub-queries


Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/7db0895f
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/7db0895f
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/7db0895f

Branch: refs/heads/branch_7_4
Commit: 7db0895f24df4003b90dfc2dcf97076ba4f75c4a
Parents: 3ed77eb
Author: yonik <yonik@apache.org>
Authored: Tue Jun 12 15:15:38 2018 -0400
Committer: yonik <yonik@apache.org>
Committed: Tue Jun 12 16:08:06 2018 -0400

----------------------------------------------------------------------
 .../solr/request/json/JsonQueryConverter.java   | 28 ++++++++++++++------
 .../solr/search/json/TestJsonRequest.java       | 11 ++++----
 2 files changed, 26 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/7db0895f/solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java b/solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java
index 8f8d04a..22e15c7 100644
--- a/solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java
+++ b/solr/core/src/java/org/apache/solr/request/json/JsonQueryConverter.java
@@ -43,14 +43,27 @@ class JsonQueryConverter {
     return name;
   }
 
+  // when isQParser==true, "val" is a query object of the form {query_type:{param1:val1,
param2:val2}}
+  // when isQParser==false, "val" is a parameter on an existing qparser (which could be a
simple parameter like 42, or a sub-query)
   private void buildLocalParams(StringBuilder builder, Object val, boolean isQParser, Map<String,
String[]> additionalParams) {
     if (!isQParser && !(val instanceof Map)) {
       // val is value of a query parser, and it is not a map
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
           "Error when parsing json query, expect a json object here, but found : "+val);
+      // NOTE: a top-level query *can* be a String, so we should really allow it here.  This
currently only works because
+      // we special-case String in toLocalParams() and don't call this method.
     }
+    // We don't want to introduce unnecessary variable at root level
+    boolean useSubBuilder = builder.length() > 0;
+
     if (val instanceof String) {
-      builder.append('$').append(putParam(val.toString(), additionalParams));
+      if (!useSubBuilder) {
+        // Top level, so just use the value.  NOTE: this case is also short-circuited in
toLocalParams() for performance.
+        builder.append(val.toString());
+      } else {
+        // val is a parameter in a qparser, so use param deref and skip escaping: ...=$param1}&param1=<val>
+        builder.append('$').append(putParam(val.toString(), additionalParams));
+      }
       return;
     }
     if (val instanceof Number) {
@@ -75,20 +88,19 @@ class JsonQueryConverter {
         Object taggedQueryObject = map.get(qtype);
         tagName = qtype.substring(1);
         if (taggedQueryObject instanceof String) {
-          builder.append("{!tag=").append(tagName).append("}");
-          builder.append(taggedQueryObject);
+          StringBuilder sb = new StringBuilder();
+          sb.append("{!tag=").append(tagName).append("}");
+          sb.append(taggedQueryObject.toString());
+          buildLocalParams(builder, sb.toString(), true, additionalParams);
           return;
         } else if (taggedQueryObject instanceof Map) {
           map = (Map<String, Object>) taggedQueryObject;
           qtype = map.keySet().iterator().next();
+          // FUTURE: might want to recurse here instead to handle nested tags (and add tagName
as a parameter?)
         }
       }
 
-      // We don't want to introduce unnecessary variable at root level
-      boolean useSubBuilder = builder.length() > 0;
-      StringBuilder subBuilder = builder;
-
-      if (useSubBuilder) subBuilder = new StringBuilder();
+      StringBuilder subBuilder = useSubBuilder ? new StringBuilder() : builder;
 
       Object subVal = map.get(qtype);
       subBuilder = subBuilder.append("{!").append(qtype).append(' ');

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/7db0895f/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java b/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java
index 68ee8f0..6efe840 100644
--- a/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java
+++ b/solr/core/src/test/org/apache/solr/search/json/TestJsonRequest.java
@@ -232,7 +232,7 @@ public class TestJsonRequest extends SolrTestCaseHS {
             "  'query': {" +
             "    'bool' : {" +
             "      'should' : [" +
-            "        'id:1'," +
+            "        {'#MYTAG' : 'id:1'}," +  // tagged query (the tag should do nothing
here)
             "        'id:2'" +
             "      ]" +
             "    }" +
@@ -267,7 +267,7 @@ public class TestJsonRequest extends SolrTestCaseHS {
             "     query : A" +
             "    }" +
             "   }" +
-            "   must_not : {lucene : {query:'id: 1'}}" +
+            "   must_not : {'#NOT':{lucene : {query:'id: 1'}}}" +  // testing tagging syntax
at the same time (the tag should do nothing here)
             "  }" +
             " }" +
             "}")
@@ -377,10 +377,10 @@ public class TestJsonRequest extends SolrTestCaseHS {
     );
 
     try {
-      client.testJQ(params("json", "{query:{'lucene':'id:1'}}"));
+      client.testJQ(params("json", "{query:{'lucene':'foo_s:ignore_exception'}}"));  // TODO:
this seems like a reasonable capability that we would want to support in the future.  It should
be OK to make this pass.
       fail();
     } catch (Exception e) {
-      assertTrue(e.getMessage().contains("id:1"));
+      assertTrue(e.getMessage().contains("foo_s"));
     }
 
     try {
@@ -401,7 +401,7 @@ public class TestJsonRequest extends SolrTestCaseHS {
     try {
       client.testJQ( params("json","{" +
           " query : '*:*'," +
-          " filter : { \"RCAT\" : \"cat_s:A\" }" +
+          " filter : { \"RCAT\" : \"cat_s:A OR ignore_exception\" }" + // without the pound,
the tag would be interpreted as a query type
           "}", "json.facet", "{" +
           "categories:{ type:terms, field:cat_s, domain:{excludeTags:\"RCAT\"} }  " +
           "}"), "facets=={ count:2, " +
@@ -410,6 +410,7 @@ public class TestJsonRequest extends SolrTestCaseHS {
       );
       fail("no # no tag");
     } catch (Exception e) {
+      // This is just the current mode of failure.  It's fine if it fails a different way
(with a 400 error) in the future.
       assertTrue(e.getMessage().contains("expect a json object"));
     }
 


Mime
View raw message