SOLR-12979: fail fast when collapse field is non-docValued & non-uninvertible

* Improve error message when collapse field is non-docValued & non-uninvertible.
  Return error code 400 instead of 500 in the above case
This commit is contained in:
Munendra S N 2019-06-25 09:42:10 +05:30
parent 85ec39d931
commit e0e5296abc
3 changed files with 28 additions and 11 deletions

View File

@ -185,6 +185,9 @@ Bug Fixes
* SOLR-13187: Fix NPE when invalid query parser is specified and return 400 error code. * SOLR-13187: Fix NPE when invalid query parser is specified and return 400 error code.
(Cesar Rodriguez, Marek, Charles Sanders, Munendra S N, Mikhail Khludnev) (Cesar Rodriguez, Marek, Charles Sanders, Munendra S N, Mikhail Khludnev)
* SOLR-12979: Improve error message and change error code to 400 when collapse field is non-docValued and
non-uninvertible (hossman, Munendra S N)
Other Changes Other Changes
---------------------- ----------------------

View File

@ -286,8 +286,12 @@ public class CollapsingQParserPlugin extends QParserPlugin {
// if unknown field, this would fail fast // if unknown field, this would fail fast
SchemaField collapseFieldSf = request.getSchema().getField(this.collapseField); SchemaField collapseFieldSf = request.getSchema().getField(this.collapseField);
// collapseFieldSf won't be null if (!(collapseFieldSf.isUninvertible() || collapseFieldSf.hasDocValues())) {
if (collapseFieldSf.multiValued()) { // uninvertible=false and docvalues=false
// field can't be indexed=false and uninvertible=true
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Collapsing field '" + collapseField +
"' should be either docValues enabled or indexed with uninvertible enabled");
} else if (collapseFieldSf.multiValued()) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Collapsing not supported on multivalued fields"); throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Collapsing not supported on multivalued fields");
} }

View File

@ -814,8 +814,12 @@ public class TestCollapseQParserPlugin extends SolrTestCaseJ4 {
// this is currently ok on fields that don't exist on any docs in the index // this is currently ok on fields that don't exist on any docs in the index
for (String f : Arrays.asList("not_indexed_sS", "indexed_s_not_uninvert")) { for (String f : Arrays.asList("not_indexed_sS", "indexed_s_not_uninvert")) {
for (String hint : Arrays.asList("", " hint=top_fc")) { for (String hint : Arrays.asList("", " hint=top_fc")) {
assertQ(req(params("q", "*:*", "fq", "{!collapse field="+f+hint+"}")) SolrException e = expectThrows(SolrException.class,
, "*[count(//doc)=0]"); () -> h.query(req("q", "*:*", "fq", "{!collapse field="+f + hint +"}")));
assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
assertTrue("unexpected Message: " + e.getMessage(),
e.getMessage().contains("Collapsing field '" + f + "' " +
"should be either docValues enabled or indexed with uninvertible enabled"));
} }
} }
} }
@ -941,18 +945,24 @@ public class TestCollapseQParserPlugin extends SolrTestCaseJ4 {
req("q","*:*", "fq","{!collapse field=bleh}"), SolrException.ErrorCode.BAD_REQUEST); req("q","*:*", "fq","{!collapse field=bleh}"), SolrException.ErrorCode.BAD_REQUEST);
// if a field is uninvertible=false, it should behave the same as a field that is indexed=false ... // if a field is uninvertible=false, it should behave the same as a field that is indexed=false ...
// this also tests docValues=false along with indexed=false or univertible=false
for (String f : Arrays.asList("not_indexed_sS", "indexed_s_not_uninvert")) { for (String f : Arrays.asList("not_indexed_sS", "indexed_s_not_uninvert")) {
{ // this currently propogates up the low level DocValues error in the common case... {
Exception e = expectThrows(RuntimeException.class, IllegalStateException.class, SolrException e = expectThrows(SolrException.class,
() -> h.query(req(params("q", "*:*", () -> h.query(req(params("q", "*:*",
"fq", "{!collapse field="+f+"}")))); "fq", "{!collapse field="+f+"}"))));
assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
assertTrue("unexpected Message: " + e.getMessage(), assertTrue("unexpected Message: " + e.getMessage(),
e.getMessage().contains("Re-index with correct docvalues type")); e.getMessage().contains("Collapsing field '" + f + "' " +
"should be either docValues enabled or indexed with uninvertible enabled"));
} }
{ // ... but in the case of hint=top_fc a bare NPE gets propogated up (SOLR-12979)... {
expectThrows(RuntimeException.class, NullPointerException.class, SolrException e = expectThrows(SolrException.class,
() -> h.query(req(params("q", "*:*", () -> h.query(req("q", "*:*", "fq", "{!collapse field="+f+" hint=top_fc}")));
"fq", "{!collapse field="+f+" hint=top_fc}")))); assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
assertTrue("unexpected Message: " + e.getMessage(),
e.getMessage().contains("Collapsing field '" + f + "' " +
"should be either docValues enabled or indexed with uninvertible enabled"));
} }
} }