From bf69a40d16e8aba74f09846bea08173b0ac3efc1 Mon Sep 17 00:00:00 2001 From: Mikhail Khludnev Date: Tue, 5 Feb 2019 17:18:30 +0300 Subject: [PATCH] SOLR-12330: rethow NPEs and other json.facet syntax errors properly as 400 --- solr/CHANGES.txt | 1 + .../apache/solr/search/facet/FacetModule.java | 8 +- .../solr/search/facet/FacetProcessor.java | 39 +++--- .../apache/solr/search/facet/FacetRange.java | 29 +---- .../solr/search/facet/FacetRequest.java | 92 ++++++++------ .../solr/search/facet/TestJsonFacets.java | 115 +++++++++++++++++- 6 files changed, 199 insertions(+), 85 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index d7331e7fd9d..4b3f9921f55 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -56,6 +56,7 @@ New Features Bug Fixes ---------------------- +* SOLR-12330: 500 error code on json.facet syntax errors (Munendra S N, Mikhail Khludnev) Improvements ---------------------- diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetModule.java b/solr/core/src/java/org/apache/solr/search/facet/FacetModule.java index 6b4b303670b..a5d55fbe604 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetModule.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetModule.java @@ -75,7 +75,13 @@ public class FacetModule extends SearchComponent { if (!facetsEnabled) return; jsonFacet = new LegacyFacet(rb.req.getParams()).getLegacy(); } else { - jsonFacet = (Map) json.get("facet"); + Object jsonObj = json.get("facet"); + if (jsonObj instanceof Map) { + jsonFacet = (Map) 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; diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetProcessor.java b/solr/core/src/java/org/apache/solr/search/facet/FacetProcessor.java index 15a3ccb2220..8a996b19c41 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetProcessor.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetProcessor.java @@ -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 { // 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 m = (Map) rawFilter; @@ -113,17 +106,11 @@ public abstract class FacetProcessor { 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 { 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; diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java b/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java index e183a02ce8b..d7925193491 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java @@ -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,7 +176,7 @@ class FacetRangeProcessor extends FacetProcessor { Calc calc; final FieldType ft = sf.getType(); - if (ft instanceof TrieField) { + if (ft instanceof TrieField || ft.isPointField()) { switch (ft.getNumberType()) { case FLOAT: calc = new FloatCalc(sf); @@ -199,31 +198,7 @@ class FacetRangeProcessor extends FacetProcessor { (SolrException.ErrorCode.BAD_REQUEST, "Expected numeric field type :" + sf); } - } else if (ft instanceof PointField) { - // TODO, this is the same in Trie and Point now - 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 { + } else { throw new SolrException (SolrException.ErrorCode.BAD_REQUEST, "Expected numeric field type :" + sf); diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java b/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java index 07a10f35ce3..752a71e8118 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java @@ -640,8 +640,9 @@ abstract class FacetParser { getDomain().excludeTags = excludeTags; } - Map domainMap = (Map) m.get("domain"); - if (domainMap != null) { + Object domainObj = m.get("domain"); + if (domainObj instanceof Map) { + Map domainMap = (Map)domainObj; FacetRequest.Domain domain = getDomain(); excludeTags = getStringList(domainMap, "excludeTags"); @@ -659,9 +660,9 @@ abstract class FacetParser { "'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 { 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 { return (Boolean)o; } + public Boolean getBooleanOrNull(Map 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 args, String paramName, String defVal) { Object o = args.get(paramName); if (o == null) { @@ -786,7 +799,19 @@ abstract class FacetParser { return (String)o; } + public Object getVal(Map args, String paramName, boolean required) { + Object o = args.get(paramName); + if (o == null && required) { + throw err("Missing required parameter: '" + paramName + "'"); + } + return o; + } + public List getStringList(Map args, String paramName) { + return getStringList(args, paramName, true); + } + + public List getStringList(Map args, String paramName, boolean decode) { Object o = args.get(paramName); if (o == null) { return null; @@ -795,10 +820,12 @@ abstract class FacetParser { return (List)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 { // 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 { // 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 { // 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 { ? FacetRequest.SortDirection.asc : FacetRequest.SortDirection.desc)); } - } else { + } else if (sort instanceof Map) { // sort : { myvar : 'desc' } Map map = (Map)sort; // TODO: validate @@ -996,6 +1029,9 @@ class FacetFieldParser extends FacetParser { 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 { 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 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 lst = null; - - if (o instanceof List) { - lst = (List)o; - } else if (o instanceof String) { - lst = StrUtils.splitSmart((String)o, ','); - } - - for (String otherStr : lst) { + List 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); diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java index 0eccea80a66..460a9360de5 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java +++ b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java @@ -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() {