Apache
/
lucene
mirror of https://github.com/apache/lucene.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SOLR-13022: validate sort parameters in JSON facet after parsing 

* This fixes NPE in case of non-existent aggregate functions in sort/prelim_sort
* validate sort direction
This commit is contained in:
Munendra S N 2019-09-25 10:36:02 +05:30
parent d279fe8a80
commit cd9f3a9a46
4 changed files with 115 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
solr/CHANGES.txt
View File

@ -234,6 +234,8 @@ Bug Fixes
* SOLR-13725: Allow negative values for limit in TermsFacetMap (Richard Walker, Munendra S N)
* SOLR-13022: Fix NPE when sorting by non-existent aggregate function in JSON Facet (hossman, Munendra S N)
Other Changes
----------------------

12
solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessor.java
View File

@ -34,6 +34,7 @@ import java.util.function.IntFunction;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.Query;
import org.apache.lucene.util.PriorityQueue;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.schema.FieldType;
import org.apache.solr.schema.SchemaField;
@ -148,7 +149,7 @@ abstract class FacetFieldProcessor extends FacetProcessor<FacetField> {
}
/**
* Simple helper for checking if a {@FacetRequest.FacetSort} is on "count" or "index" and picking
* Simple helper for checking if a {@link FacetRequest.FacetSort} is on "count" or "index" and picking
* the existing SlotAcc
* @return an existing SlotAcc for sorting, else null if it should be built from the Aggs
*/
@ -224,6 +225,12 @@ abstract class FacetFieldProcessor extends FacetProcessor<FacetField> {
boolean needOtherAccs = freq.allBuckets; // TODO: use for missing too...
if (sortAcc == null) {
// as sort is already validated, in what case sortAcc would be null?
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
"Invalid sort '" + sort + "' for field '" + sf.getName() + "'");
}
if (!needOtherAccs) {
// we may need them later, but we don't want to create them now
// otherwise we won't know if we need to call setNextReader on them.
@ -287,6 +294,7 @@ abstract class FacetFieldProcessor extends FacetProcessor<FacetField> {
SimpleOrderedMap<Object> findTopSlots(final int numSlots, final int slotCardinality,
IntFunction<Comparable> bucketValFromSlotNumFunc,
Function<Comparable, String> fieldQueryValFunc) throws IOException {
assert this.sortAcc != null;
int numBuckets = 0;
final int off = fcontext.isShard() ? 0 : (int) freq.offset;
@ -326,7 +334,7 @@ abstract class FacetFieldProcessor extends FacetProcessor<FacetField> {
return cmp == 0 ? b.slot < a.slot : cmp < 0;
};
}
final PriorityQueue<Slot> queue = new PriorityQueue<Slot>(maxTopVals) {
final PriorityQueue<Slot> queue = new PriorityQueue<>(maxTopVals) {
@Override
protected boolean lessThan(Slot a, Slot b) { return orderPredicate.test(a, b); }
};

99
solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java
View File

@ -21,8 +21,9 @@ import java.util.ArrayList;
import java.util.EnumSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Objects;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import org.apache.lucene.search.Query;
import org.apache.solr.common.SolrException;
@ -96,6 +97,20 @@ public abstract class FacetRequest {
this.multiplier = multiplier;
}
public static SortDirection fromObj(Object direction) {
if (direction == null) {
// should we just default either to desc/asc??
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Missing Sort direction");
}
switch (direction.toString()) {
case "asc": return asc;
case "desc": return desc;
default:
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Unknown Sort direction '" + direction + "'");
}
}
// asc==-1, desc==1
public int getMultiplier() {
return multiplier;
@ -986,11 +1001,10 @@ class FacetFieldParser extends FacetParser<FacetField> {
Object o = m.get("facet");
parseSubs(o);
// TODO: SOLR-13022 ... validate the sortVariabls against the subs.
facet.sort = parseSort( m.get(SORT) );
facet.prelim_sort = parseSort( m.get("prelim_sort") );
facet.sort = parseAndValidateSort(facet, m, SORT);
facet.prelim_sort = parseAndValidateSort(facet, m, "prelim_sort");
} else if (arg != null) {
// something lke json.facet.facet.field=2
// something like json.facet.facet.field=2
throw err("Expected string/map for facet field, received " + arg.getClass().getSimpleName() + "=" + arg);
}
@ -1001,42 +1015,69 @@ class FacetFieldParser extends FacetParser<FacetField> {
return facet;
}
// Sort specification is currently
// sort : 'mystat desc'
// OR
// sort : { mystat : 'desc' }
private static FacetRequest.FacetSort parseSort(Object sort) {
/**
* Parses, validates and returns the {@link FacetRequest.FacetSort} for given sortParam
* and facet field
* <p>
* Currently, supported sort specifications are 'mystat desc' OR {mystat: 'desc'}
* index - This is equivalent to 'index asc'
* count - This is equivalent to 'count desc'
* </p>
*
* @param facet {@link FacetField} for which sort needs to be parsed and validated
* @param args map containing the sortVal for given sortParam
* @param sortParam parameter for which sort needs to parsed and validated
* @return parsed facet sort
*/
private static FacetRequest.FacetSort parseAndValidateSort(FacetField facet, Map<String, Object> args, String sortParam) {
Object sort = args.get(sortParam);
if (sort == null) {
return null;
} else if (sort instanceof String) {
}
FacetRequest.FacetSort facetSort = null;
if (sort instanceof String) {
String sortStr = (String)sort;
if (sortStr.endsWith(" asc")) {
return new FacetRequest.FacetSort(sortStr.substring(0, sortStr.length()-" asc".length()),
FacetRequest.SortDirection.asc);
facetSort = new FacetRequest.FacetSort(sortStr.substring(0, sortStr.length()-" asc".length()),
FacetRequest.SortDirection.asc);
} else if (sortStr.endsWith(" desc")) {
return new FacetRequest.FacetSort(sortStr.substring(0, sortStr.length()-" desc".length()),
FacetRequest.SortDirection.desc);
facetSort = new FacetRequest.FacetSort(sortStr.substring(0, sortStr.length()-" desc".length()),
FacetRequest.SortDirection.desc);
} else {
return new FacetRequest.FacetSort(sortStr,
// default direction for "index" is ascending
("index".equals(sortStr)
? FacetRequest.SortDirection.asc
: FacetRequest.SortDirection.desc));
facetSort = new FacetRequest.FacetSort(sortStr,
// default direction for "index" is ascending
("index".equals(sortStr)
? FacetRequest.SortDirection.asc
: FacetRequest.SortDirection.desc));
}
} else if (sort instanceof Map) {
// sort : { myvar : 'desc' }
Map<String,Object> map = (Map<String,Object>)sort;
// TODO: validate
Map.Entry<String,Object> entry = map.entrySet().iterator().next();
String k = entry.getKey();
Object v = entry.getValue();
return new FacetRequest.FacetSort(k, FacetRequest.SortDirection.valueOf(v.toString()));
// { myvar : 'desc' }
Optional<Map.Entry<String,Object>> optional = ((Map<String,Object>)sort).entrySet().stream().findFirst();
if (optional.isPresent()) {
Map.Entry<String, Object> entry = optional.get();
facetSort = new FacetRequest.FacetSort(entry.getKey(), FacetRequest.SortDirection.fromObj(entry.getValue()));
}
} else {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
"Expected string/map for 'sort', received "+ sort.getClass().getSimpleName() + "=" + sort);
"Expected string/map for '" + sortParam +"', received "+ sort.getClass().getSimpleName() + "=" + sort);
}
Map<String, AggValueSource> facetStats = facet.facetStats;
// validate facet sort
boolean isValidSort = facetSort == null ||
"index".equals(facetSort.sortVariable) ||
"count".equals(facetSort.sortVariable) ||
(facetStats != null && facetStats.containsKey(facetSort.sortVariable));
if (!isValidSort) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
"Invalid " + sortParam + " option '" + sort + "' for field '" + facet.field + "'");
}
return facetSort;
}
}

33
solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java
View File

@ -3523,6 +3523,39 @@ public class TestJsonFacets extends SolrTestCaseHS {
"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);
assertQEx("Should fail as sort is invalid",
"Invalid sort option 'bleh' for field 'cat_s'",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,sort:bleh}}"),
SolrException.ErrorCode.BAD_REQUEST);
assertQEx("Should fail as sort order is invalid",
"Unknown Sort direction 'bleh'",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,sort:{count: bleh}}}"),
SolrException.ErrorCode.BAD_REQUEST);
// test for prelim_sort
assertQEx("Should fail as prelim_sort is invalid",
"Invalid prelim_sort option 'bleh' for field 'cat_s'",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,prelim_sort:bleh}}"),
SolrException.ErrorCode.BAD_REQUEST);
assertQEx("Should fail as prelim_sort map is invalid",
"Invalid prelim_sort option '{bleh=desc}' for field 'cat_s'",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,prelim_sort:{bleh:desc}}}"),
SolrException.ErrorCode.BAD_REQUEST);
// with nested facet
assertQEx("Should fail as prelim_sort is invalid",
"Invalid sort option 'bleh' for field 'id'",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,sort:bleh,facet:" +
"{bleh:\"unique(cat_s)\",id:{type:terms,field:id,sort:bleh}}}}"),
SolrException.ErrorCode.BAD_REQUEST);
assertQ("Should pass as sort is proper",
req("q", "*:*", "rows", "0", "json.facet", "{cat_s:{type:terms,field:cat_s,sort:bleh,facet:" +
"{bleh:\"unique(cat_s)\",id:{type:terms,field:id,sort:{bleh:desc},facet:{bleh:\"unique(id)\"}}}}}")
);
}