From cd9f3a9a46f1f4a7e7ba451d4a794bc7b0b04522 Mon Sep 17 00:00:00 2001 From: Munendra S N Date: Wed, 25 Sep 2019 10:36:02 +0530 Subject: [PATCH] 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 + .../search/facet/FacetFieldProcessor.java | 12 ++- .../solr/search/facet/FacetRequest.java | 99 +++++++++++++------ .../solr/search/facet/TestJsonFacets.java | 33 +++++++ 4 files changed, 115 insertions(+), 31 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index b1fa38c1802..f56972e9367 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -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 ---------------------- 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 40eb7854bfa..5caea4eca7b 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 { } /** - * 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 { 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 { SimpleOrderedMap findTopSlots(final int numSlots, final int slotCardinality, IntFunction bucketValFromSlotNumFunc, Function 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 { return cmp == 0 ? b.slot < a.slot : cmp < 0; }; } - final PriorityQueue queue = new PriorityQueue(maxTopVals) { + final PriorityQueue queue = new PriorityQueue<>(maxTopVals) { @Override protected boolean lessThan(Slot a, Slot b) { return orderPredicate.test(a, b); } }; 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 3d79a8abc73..6860a943841 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 { 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 { 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 + *

+ * Currently, supported sort specifications are 'mystat desc' OR {mystat: 'desc'} + * index - This is equivalent to 'index asc' + * count - This is equivalent to 'count desc' + *

+ * + * @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 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 map = (Map)sort; - // TODO: validate - Map.Entry 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> optional = ((Map)sort).entrySet().stream().findFirst(); + if (optional.isPresent()) { + Map.Entry 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 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 8658c2cc8ab..b3586ee1818 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)\"}}}}}") + ); }