SOLR-12330: rethow NPEs and other json.facet syntax errors properly as 400
authorMikhail Khludnev <mkhl@apache.org>
Tue, 5 Feb 2019 14:18:30 +0000 (17:18 +0300)
committerMikhail Khludnev <mkhl@apache.org>
Sat, 9 Feb 2019 20:57:30 +0000 (23:57 +0300)
solr/CHANGES.txt
solr/core/src/java/org/apache/solr/search/facet/FacetModule.java
solr/core/src/java/org/apache/solr/search/facet/FacetProcessor.java
solr/core/src/java/org/apache/solr/search/facet/FacetRange.java
solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java
solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java

index d7331e7..4b3f992 100644 (file)
@@ -56,6 +56,7 @@ New Features
 Bug Fixes
 ----------------------
 
+* SOLR-12330: 500 error code on json.facet syntax errors (Munendra S N, Mikhail Khludnev)
 
 Improvements
 ----------------------
index 6b4b303..a5d55fb 100644 (file)
@@ -75,7 +75,13 @@ public class FacetModule extends SearchComponent {
       if (!facetsEnabled) return;
       jsonFacet = new LegacyFacet(rb.req.getParams()).getLegacy();
     } else {
-      jsonFacet = (Map<String, Object>) json.get("facet");
+      Object jsonObj = json.get("facet");
+      if (jsonObj instanceof Map) {
+        jsonFacet = (Map<String, Object>) jsonObj;
+      } else if (jsonObj != null) {
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+            "Expected Map for 'facet', received " + jsonObj.getClass().getSimpleName() + "=" + jsonObj);
+      }
     }
     if (jsonFacet == null) return;
 
index 15a3ccb..8a996b1 100644 (file)
@@ -33,6 +33,7 @@ import org.apache.lucene.search.Query;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.request.SolrRequestInfo;
 import org.apache.solr.schema.SchemaField;
 import org.apache.solr.search.BitDocSet;
@@ -78,15 +79,7 @@ public abstract class FacetProcessor<FacetRequestT extends FacetRequest>  {
     // TODO: prevent parsing filters each time!
     for (Object rawFilter : filters) {
       if (rawFilter instanceof String) {
-        QParser parser = null;
-        try {
-          parser = QParser.getParser((String)rawFilter, fcontext.req);
-          parser.setIsFilter(true);
-          Query symbolicFilter = parser.getQuery();
-          qlist.add(symbolicFilter);
-        } catch (SyntaxError syntaxError) {
-          throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, syntaxError);
-        }
+        qlist.add(parserFilter((String) rawFilter, fcontext.req));
       } else if (rawFilter instanceof Map) {
 
         Map<String,Object> m = (Map<String, Object>) rawFilter;
@@ -113,17 +106,11 @@ public abstract class FacetProcessor<FacetRequestT extends FacetRequest>  {
 
         String[] qstrings = fcontext.req.getParams().getParams(tag);
 
+        // idea is to support multivalued parameter ie, 0 or more values
+        // so, when value not specified, it is ignored rather than throwing exception
         if (qstrings != null) {
           for (String qstring : qstrings) {
-            QParser parser = null;
-            try {
-              parser = QParser.getParser((String) qstring, fcontext.req);
-              parser.setIsFilter(true);
-              Query symbolicFilter = parser.getQuery();
-              qlist.add(symbolicFilter);
-            } catch (SyntaxError syntaxError) {
-              throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, syntaxError);
-            }
+            qlist.add(parserFilter(qstring, fcontext.req));
           }
         }
 
@@ -135,6 +122,22 @@ public abstract class FacetProcessor<FacetRequestT extends FacetRequest>  {
     return qlist;
   }
 
+  private static Query parserFilter(String rawFilter, SolrQueryRequest req) {
+    QParser parser = null;
+    try {
+      parser = QParser.getParser(rawFilter, req);
+      parser.setIsFilter(true);
+      Query symbolicFilter = parser.getQuery();
+      if (symbolicFilter == null) {
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+            "QParser yields null, perhaps unresolved parameter reference in: "+rawFilter);
+      }
+      return symbolicFilter;
+    } catch (SyntaxError syntaxError) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, syntaxError);
+    }
+  }
+
   private void handleDomainChanges() throws IOException {
     if (freq.domain == null) return;
 
index e183a02..d792519 100644 (file)
@@ -34,7 +34,6 @@ import org.apache.solr.schema.CurrencyFieldType;
 import org.apache.solr.schema.CurrencyValue;
 import org.apache.solr.schema.ExchangeRateProvider;
 import org.apache.solr.schema.FieldType;
-import org.apache.solr.schema.PointField;
 import org.apache.solr.schema.SchemaField;
 import org.apache.solr.schema.TrieDateField;
 import org.apache.solr.schema.TrieField;
@@ -177,30 +176,7 @@ class FacetRangeProcessor extends FacetProcessor<FacetRange> {
     Calc calc;
     final FieldType ft = sf.getType();
 
-    if (ft instanceof TrieField) {
-      switch (ft.getNumberType()) {
-        case FLOAT:
-          calc = new FloatCalc(sf);
-          break;
-        case DOUBLE:
-          calc = new DoubleCalc(sf);
-          break;
-        case INTEGER:
-          calc = new IntCalc(sf);
-          break;
-        case LONG:
-          calc = new LongCalc(sf);
-          break;
-        case DATE:
-          calc = new DateCalc(sf, null);
-          break;
-        default:
-          throw new SolrException
-              (SolrException.ErrorCode.BAD_REQUEST,
-                  "Expected numeric field type :" + sf);
-      }
-    } else if (ft instanceof PointField) {
-      // TODO, this is the same in Trie and Point now
+    if (ft instanceof TrieField || ft.isPointField()) {
       switch (ft.getNumberType()) {
         case FLOAT:
           calc = new FloatCalc(sf);
@@ -222,8 +198,7 @@ class FacetRangeProcessor extends FacetProcessor<FacetRange> {
               (SolrException.ErrorCode.BAD_REQUEST,
                   "Expected numeric field type :" + sf);
       }
-    } 
-    else {
+    } else {
       throw new SolrException
           (SolrException.ErrorCode.BAD_REQUEST,
               "Expected numeric field type :" + sf);
index 07a10f3..752a71e 100644 (file)
@@ -640,8 +640,9 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
         getDomain().excludeTags = excludeTags;
       }
 
-      Map<String,Object> domainMap = (Map<String,Object>) m.get("domain");
-      if (domainMap != null) {
+      Object domainObj =  m.get("domain");
+      if (domainObj instanceof Map) {
+        Map<String, Object> domainMap = (Map<String, Object>)domainObj;
         FacetRequest.Domain domain = getDomain();
 
         excludeTags = getStringList(domainMap, "excludeTags");
@@ -659,9 +660,9 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
                                     "'query' domain can not be combined with 'excludeTags'");
           }
         }
-        
-        String blockParent = (String)domainMap.get("blockParent");
-        String blockChildren = (String)domainMap.get("blockChildren");
+
+        String blockParent = getString(domainMap, "blockParent", null);
+        String blockChildren = getString(domainMap, "blockChildren", null);
 
         if (blockParent != null) {
           domain.toParent = true;
@@ -680,7 +681,9 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
           domain.filters = parseJSONQueryStruct(filterOrList);
         }
 
-      } // end "domain"
+      } else if (domainObj != null) {
+        throw err("Expected Map for 'domain', received " + domainObj.getClass().getSimpleName() + "=" + domainObj);
+      }
     }
   }
 
@@ -774,6 +777,16 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
     return (Boolean)o;
   }
 
+  public Boolean getBooleanOrNull(Map<String, Object> args, String paramName) {
+    Object o = args.get(paramName);
+
+    if (o != null && !(o instanceof Boolean)) {
+      throw err("Expected boolean type for param '"+paramName + "' but got " + o.getClass().getSimpleName() + " = " + o);
+    }
+    return (Boolean) o;
+  }
+
+
   public String getString(Map<String,Object> args, String paramName, String defVal) {
     Object o = args.get(paramName);
     if (o == null) {
@@ -786,7 +799,19 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
     return (String)o;
   }
 
+  public Object getVal(Map<String, Object> args, String paramName, boolean required) {
+    Object o = args.get(paramName);
+    if (o == null && required) {
+      throw err("Missing required parameter: '" + paramName + "'");
+    }
+    return o;
+  }
+
   public List<String> getStringList(Map<String,Object> args, String paramName) {
+    return getStringList(args, paramName, true);
+  }
+
+  public List<String> getStringList(Map<String, Object> args, String paramName, boolean decode) {
     Object o = args.get(paramName);
     if (o == null) {
       return null;
@@ -795,10 +820,12 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
       return (List<String>)o;
     }
     if (o instanceof String) {
-      return StrUtils.splitSmart((String)o, ",", true);
+      // TODO: SOLR-12539 handle spaces in b/w comma & value ie, should the values be trimmed before returning??
+      return StrUtils.splitSmart((String)o, ",", decode);
     }
 
-    throw err("Expected list of string or comma separated string values.");
+    throw err("Expected list of string or comma separated string values for '" + paramName +
+        "', received " + o.getClass().getSimpleName() + "=" + o);
   }
 
   public IndexSchema getSchema() {
@@ -873,6 +900,9 @@ class FacetQueryParser extends FacetParser<FacetQuery> {
       // OK to parse subs before we have parsed our own query?
       // as long as subs don't need to know about it.
       parseSubs( m.get("facet") );
+    } else if (arg != null) {
+      // something lke json.facet.facet.query=2
+      throw err("Expected string/map for facet query, received " + arg.getClass().getSimpleName() + "=" + arg);
     }
 
     // TODO: substats that are from defaults!!!
@@ -946,7 +976,7 @@ class FacetFieldParser extends FacetParser<FacetField> {
       // TODO: pull up to higher level?
       facet.refine = FacetField.RefineMethod.fromObj(m.get("refine"));
 
-      facet.perSeg = (Boolean)m.get("perSeg");
+      facet.perSeg = getBooleanOrNull(m, "perSeg");
 
       // facet.sort may depend on a facet stat...
       // should we be parsing / validating this here, or in the execution environment?
@@ -956,6 +986,9 @@ class FacetFieldParser extends FacetParser<FacetField> {
       // TODO: SOLR-13022 ... validate the sortVariabls against the subs.
       facet.sort = parseSort( m.get(SORT) );
       facet.prelim_sort = parseSort( m.get("prelim_sort") );
+    } else if (arg != null) {
+      // something lke json.facet.facet.field=2
+      throw err("Expected string/map for facet field, received " + arg.getClass().getSimpleName() + "=" + arg);
     }
 
     if (null == facet.sort) {
@@ -988,7 +1021,7 @@ class FacetFieldParser extends FacetParser<FacetField> {
                                            ? FacetRequest.SortDirection.asc
                                            : FacetRequest.SortDirection.desc));
       }
-    } else {
+    } else if (sort instanceof Map) {
      // sort : { myvar : 'desc' }
       Map<String,Object> map = (Map<String,Object>)sort;
       // TODO: validate
@@ -996,6 +1029,9 @@ class FacetFieldParser extends FacetParser<FacetField> {
       String k = entry.getKey();
       Object v = entry.getValue();
       return new FacetRequest.FacetSort(k, FacetRequest.SortDirection.valueOf(v.toString()));
+    } else {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+          "Expected string/map for 'sort', received "+ sort.getClass().getSimpleName() + "=" + sort);
     }
   }
 }
@@ -1019,47 +1055,29 @@ class FacetRangeParser extends FacetParser<FacetRange> {
 
     facet.field = getString(m, "field", null);
 
-    facet.start = m.get("start");
-    facet.end = m.get("end");
-    facet.gap = m.get("gap");
+    facet.start = getVal(m, "start", true);
+    facet.end = getVal(m, "end", true);
+    facet.gap = getVal(m, "gap", true);
     facet.hardend = getBoolean(m, "hardend", facet.hardend);
     facet.mincount = getLong(m, "mincount", 0);
 
     // TODO: refactor list-of-options code
 
-    Object o = m.get("include");
+    List<String> list = getStringList(m, "include", false);
     String[] includeList = null;
-    if (o != null) {
-      List lst = null;
-
-      if (o instanceof List) {
-        lst = (List)o;
-      } else if (o instanceof String) {
-        lst = StrUtils.splitSmart((String)o, ',');
-      }
-
-      includeList = (String[])lst.toArray(new String[lst.size()]);
+    if (list != null) {
+      includeList = (String[])list.toArray(new String[list.size()]);
     }
     facet.include = FacetParams.FacetRangeInclude.parseParam( includeList );
-
     facet.others = EnumSet.noneOf(FacetParams.FacetRangeOther.class);
 
-    o = m.get("other");
-    if (o != null) {
-      List<String> lst = null;
-
-      if (o instanceof List) {
-        lst = (List)o;
-      } else if (o instanceof String) {
-        lst = StrUtils.splitSmart((String)o, ',');
-      }
-
-      for (String otherStr : lst) {
+    List<String> other = getStringList(m, "other", false);
+    if (other != null) {
+      for (String otherStr : other) {
         facet.others.add( FacetParams.FacetRangeOther.get(otherStr) );
       }
     }
 
-
     Object facetObj = m.get("facet");
     parseSubs(facetObj);
 
index 0eccea8..460a936 100644 (file)
@@ -16,8 +16,6 @@
  */
 package org.apache.solr.search.facet;
 
-import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
-import com.tdunning.math.stats.AVLTreeDigest;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -28,6 +26,7 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.Random;
 import java.util.concurrent.atomic.AtomicLong;
+
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.solr.JSONTestUtil;
 import org.apache.solr.SolrTestCaseHS;
@@ -43,6 +42,9 @@ import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+import com.tdunning.math.stats.AVLTreeDigest;
+
 // Related tests:
 //   TestCloudJSONFacetJoinDomain for random field faceting tests with domain modifications
 //   TestJsonFacetRefinement for refinement tests
@@ -3181,6 +3183,115 @@ public class TestJsonFacets extends SolrTestCaseHS {
 
   }
 
+  @Test
+  public void testDomainErrors() throws Exception {
+    Client client = Client.localClient();
+    client.deleteByQuery("*:*", null);
+    indexSimple(client);
+
+    // using assertQEx so that, status code and error message can be asserted
+    assertQEx("Should Fail as filter with qparser in domain becomes null",
+        "QParser yields null, perhaps unresolved parameter reference in: {!query v=$NOfilt}",
+        req("q", "*:*", "json.facet", "{cat_s:{type:terms,field:cat_s,domain:{filter:'{!query v=$NOfilt}'}}}"),
+        SolrException.ErrorCode.BAD_REQUEST
+    );
+
+    assertQEx("Should Fail as filter in domain becomes null",
+        "QParser yields null, perhaps unresolved parameter reference in: {!v=$NOfilt}",
+        req("q", "*:*", "json.facet", "{cat_s:{type:terms,field:cat_s,domain:{filter:'{!v=$NOfilt}'}}}"),
+        SolrException.ErrorCode.BAD_REQUEST
+    );
+
+    // when domain type is invalid
+    assertQEx("Should Fail as domain not of type map",
+        "Expected Map for 'domain', received String=bleh , path=facet/cat_s",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,domain:bleh}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    // when domain = null, should not throw exception
+    assertQ("Should pass as no domain is specified",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s}}"));
+
+    // when blockChildren or blockParent is passed but not of string
+    assertQEx("Should Fail as blockChildren is of type map",
+        "Expected string type for param 'blockChildren' but got LinkedHashMap = {} , path=facet/cat_s",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,domain:{blockChildren:{}}}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    assertQEx("Should Fail as blockParent is of type map",
+        "Expected string type for param 'blockParent' but got LinkedHashMap = {} , path=facet/cat_s",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,domain:{blockParent:{}}}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+  }
+
+  @Test
+  public void testOtherErrorCases() throws Exception {
+    Client client = Client.localClient();
+    client.deleteByQuery("*:*", null);
+    indexSimple(client);
+
+    // test for sort
+    assertQEx("Should fail as sort is of type list",
+        "Expected string/map for 'sort', received ArrayList=[count desc]",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,sort:[\"count desc\"]}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+
+    assertQEx("Should fail as facet is not of type map",
+        "Expected Map for 'facet', received ArrayList=[{}]",
+        req("q", "*:*", "rows", "0", "json.facet", "[{}]"), SolrException.ErrorCode.BAD_REQUEST);
+
+    // range facets
+    assertQEx("Should fail as 'other' is of type Map",
+        "Expected list of string or comma separated string values for 'other', " +
+            "received LinkedHashMap={} , path=facet/f",
+        req("q", "*:*", "json.facet", "{f:{type:range, field:num_d, start:10, end:12, gap:1, other:{}}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    assertQEx("Should fail as 'include' is of type Map",
+        "Expected list of string or comma separated string values for 'include', " +
+            "received LinkedHashMap={} , path=facet/f",
+        req("q", "*:*", "json.facet", "{f:{type:range, field:num_d, start:10, end:12, gap:1, include:{}}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    // missing start parameter
+    assertQEx("Should Fail with missing field error",
+        "Missing required parameter: 'start' , path=facet/f",
+        req("q", "*:*", "json.facet", "{f:{type:range, field:num_d}}"), SolrException.ErrorCode.BAD_REQUEST);
+
+    // missing end parameter
+    assertQEx("Should Fail with missing field error",
+        "Missing required parameter: 'end' , path=facet/f",
+        req("q", "*:*", "json.facet", "{f:{type:range, field:num_d, start:10}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    // missing gap parameter
+    assertQEx("Should Fail with missing field error",
+        "Missing required parameter: 'gap' , path=facet/f",
+        req("q", "*:*", "json.facet", "{f:{type:range, field:num_d, start:10, end:12}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+
+    // invalid value for facet field
+    assertQEx("Should Fail as args is of type long",
+        "Expected string/map for facet field, received Long=2 , path=facet/facet",
+        req("q", "*:*", "rows", "0", "json.facet.facet.field", "2"), SolrException.ErrorCode.BAD_REQUEST);
+
+    // invalid value for facet query
+    assertQEx("Should Fail as args is of type long for query",
+        "Expected string/map for facet query, received Long=2 , path=facet/facet",
+        req("q", "*:*", "rows", "0", "json.facet.facet.query", "2"), SolrException.ErrorCode.BAD_REQUEST);
+
+    // valid facet field
+    assertQ("Should pass as this is valid query",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s}}"));
+
+    // invalid perSeg
+    assertQEx("Should fail as perSeg is not of type boolean",
+        "Expected boolean type for param 'perSeg' but got Long = 2 , path=facet/cat_s",
+        req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,perSeg:2}}"),
+        SolrException.ErrorCode.BAD_REQUEST);
+  }
 
 
   public void XtestPercentiles() {