SOLR-12330: rethow NPEs and other json.facet syntax errors properly as 400

This commit is contained in:
Mikhail Khludnev 2019-02-05 17:18:30 +03:00
parent a3a4ecd80b
commit bf69a40d16
6 changed files with 199 additions and 85 deletions

View 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
----------------------

View 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;

View 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;

View 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,7 +176,7 @@ class FacetRangeProcessor extends FacetProcessor<FacetRange> {
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<FacetRange> {
(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);

View 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);

View 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() {