You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mk...@apache.org on 2019/02/09 20:59:00 UTC
[lucene-solr] branch master updated: SOLR-12330: rethow NPEs and
other json.facet syntax errors properly as 400
This is an automated email from the ASF dual-hosted git repository.
mkhl pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/master by this push:
new bf69a40 SOLR-12330: rethow NPEs and other json.facet syntax errors properly as 400
bf69a40 is described below
commit bf69a40d16e8aba74f09846bea08173b0ac3efc1
Author: Mikhail Khludnev <mk...@apache.org>
AuthorDate: Tue Feb 5 17:18:30 2019 +0300
SOLR-12330: rethow NPEs and other json.facet syntax errors properly as 400
---
solr/CHANGES.txt | 1 +
.../org/apache/solr/search/facet/FacetModule.java | 8 +-
.../apache/solr/search/facet/FacetProcessor.java | 39 +++----
.../org/apache/solr/search/facet/FacetRange.java | 29 +-----
.../org/apache/solr/search/facet/FacetRequest.java | 92 ++++++++++-------
.../apache/solr/search/facet/TestJsonFacets.java | 115 ++++++++++++++++++++-
6 files changed, 199 insertions(+), 85 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index d7331e7..4b3f992 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 6b4b303..a5d55fb 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<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;
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 15a3ccb..8a996b1 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<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;
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 e183a02..d792519 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,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);
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 07a10f3..752a71e 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<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);
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 0eccea8..460a936 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() {