You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mu...@apache.org on 2019/09/25 05:36:53 UTC
[lucene-solr] branch branch_8x updated: SOLR-13022: validate sort
parameters in JSON facet after parsing
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/branch_8x by this push:
new f439091 SOLR-13022: validate sort parameters in JSON facet after parsing
f439091 is described below
commit f43909111f02c76e86c4775fe5490ef1bdc40c4e
Author: Munendra S N <mu...@apache.org>
AuthorDate: Wed Sep 25 10:36:02 2019 +0530
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
---
solr/CHANGES.txt | 2 +
.../solr/search/facet/FacetFieldProcessor.java | 10 ++-
.../org/apache/solr/search/facet/FacetRequest.java | 99 +++++++++++++++-------
.../apache/solr/search/facet/TestJsonFacets.java | 33 ++++++++
4 files changed, 114 insertions(+), 30 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index a0a8df7..093c524 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -172,6 +172,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
----------------------
diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessor.java b/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessor.java
index 40eb785..5fb69d8 100644
--- a/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessor.java
+++ b/solr/core/src/java/org/apache/solr/search/facet/FacetFieldProcessor.java
@@ -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;
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 3d79a8a..6860a94 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
@@ -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;
}
+
}
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 8658c2c..b3586ee 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
@@ -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)\"}}}}}")
+ );
}