From 3ef5c0ee74ec3fc939a530c7733e6632a98661a2 Mon Sep 17 00:00:00 2001 From: Munendra S N Date: Mon, 24 Jun 2019 22:58:58 +0530 Subject: [PATCH] SOLR-13187: Fix NPE when invalid qParser is specified * When non-existent qParser is specified return 400 error code * SOLR-13197: Fix NPE when createQParser is called in StatsField --- solr/CHANGES.txt | 3 ++ .../solr/handler/component/StatsField.java | 12 +++-- .../java/org/apache/solr/search/QParser.java | 9 +++- .../apache/solr/search/QueryParsingTest.java | 46 ++++++++++++++++++- 4 files changed, 63 insertions(+), 7 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 6496beb7042..dfae69eacb3 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -139,6 +139,9 @@ Bug Fixes * SOLR-12127: Updates containing 'set' operation with value null or empty list should be considered as Atomic Update (Oliver Kuldmäe, Munendra S N, Ishan Chattopadhyaya) +* 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) + Other Changes ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/StatsField.java b/solr/core/src/java/org/apache/solr/handler/component/StatsField.java index 3ca35f3b1f7..0483bb0a59f 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/StatsField.java +++ b/solr/core/src/java/org/apache/solr/handler/component/StatsField.java @@ -149,7 +149,7 @@ public class StatsField { } /** - * Given a String, returns the corrisponding Stat enum value if any, otherwise returns null. + * Given a String, returns the corresponding Stat enum value if any, otherwise returns null. */ public static Stat forName(String paramKey) { try { @@ -245,16 +245,20 @@ public class StatsField { } else { // we have a non trivial request to compute stats over a query (or function) - // NOTE we could use QParser.getParser(...) here, but that would redundently + // NOTE we could use QParser.getParser(...) here, but that would redundantly // reparse everything. ( TODO: refactor a common method in QParser ?) QParserPlugin qplug = rb.req.getCore().getQueryPlugin(parserName); - QParser qp = qplug.createParser(localParams.get(QueryParsing.V), + if (qplug == null) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid query parser '" + parserName + + (originalParam == null? "'": "' for query '" + originalParam + "'")); + } + QParser qp = qplug.createParser(localParams.get(QueryParsing.V), localParams, params, rb.req); // figure out what type of query we are dealing, get the most direct ValueSource vs = extractValueSource(qp.parse()); - // if this ValueSource directly corrisponds to a SchemaField, act as if + // if this ValueSource directly corresponds to a SchemaField, act as if // we were asked to compute stats on it directly // ie: "stats.field={!func key=foo}field(foo)" == "stats.field=foo" sf = extractSchemaField(vs, searcher.getSchema()); diff --git a/solr/core/src/java/org/apache/solr/search/QParser.java b/solr/core/src/java/org/apache/solr/search/QParser.java index 8a89a70cbbe..5ebb05e13ce 100644 --- a/solr/core/src/java/org/apache/solr/search/QParser.java +++ b/solr/core/src/java/org/apache/solr/search/QParser.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map; import org.apache.lucene.search.Query; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.SolrParams; @@ -360,10 +361,16 @@ public abstract class QParser { } parserName = localParams.get(QueryParsing.TYPE,parserName); - qstr = localParams.get("v"); + qstr = localParams.get(QueryParsing.V); } QParserPlugin qplug = req.getCore().getQueryPlugin(parserName); + if (qplug == null) { + // there should a way to include parameter for which parsing failed + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "invalid query parser '" + parserName + (stringIncludingLocalParams == null? + "'": "' for query '" + stringIncludingLocalParams + "'")); + } QParser parser = qplug.createParser(qstr, localParams, req.getParams(), req); parser.stringIncludingLocalParams = stringIncludingLocalParams; diff --git a/solr/core/src/test/org/apache/solr/search/QueryParsingTest.java b/solr/core/src/test/org/apache/solr/search/QueryParsingTest.java index 20ea9412d59..2e695be1871 100644 --- a/solr/core/src/test/org/apache/solr/search/QueryParsingTest.java +++ b/solr/core/src/test/org/apache/solr/search/QueryParsingTest.java @@ -17,6 +17,7 @@ package org.apache.solr.search; import org.apache.lucene.search.Query; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.request.SolrQueryRequest; import org.junit.BeforeClass; @@ -54,10 +55,10 @@ public class QueryParsingTest extends SolrTestCaseJ4 { try { parser = QParser.getParser(qstr, defType, req); } catch (Exception e) { - throw new RuntimeException("getParser excep using defType=" + + throw new RuntimeException("getParser excep using defType=" + defType + " with qstr="+qstr, e); } - + Query q = parser.parse(); assertNull("expected no query",q); } @@ -94,4 +95,45 @@ public class QueryParsingTest extends SolrTestCaseJ4 { ("strdist(\"a value\",literal('a value'),edit)", NAME, req).getQuery()); } + + public void testGetQParser() throws Exception { + // invalid defType + SolrException exception = expectThrows(SolrException.class, () -> h.query(req("q", "ad", "defType", "bleh"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query 'ad'", exception.getMessage()); + + // invalid qparser override in the local params + exception = expectThrows(SolrException.class, () -> h.query(req("q", "{!bleh}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query '{!bleh}'", exception.getMessage()); + + // invalid qParser with fq params + exception = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "fq", "{!some}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'some' for query '{!some}'", exception.getMessage()); + + // invalid qparser with function queries + exception = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "defType", "edismax", "boost", "{!hmm}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'hmm' for query '{!hmm}'", exception.getMessage()); + + exception = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "defType", "edismax", "boost", "query({!bleh v=ak})"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query '{!bleh v=ak}'", exception.getMessage()); + + exception = expectThrows(SolrException.class, () -> + h.query(req("q", "*:*", "defType", "edismax", "boost", "query($qq)", "qq", "{!bleh v=a}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query '{!bleh v=a}'", exception.getMessage()); + + // ranking doesn't use defType + exception = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "rq", "{!bleh}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query '{!bleh}'", exception.getMessage()); + + // with stats.field + exception = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "stats", "true", "stats.field", "{!bleh}"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code()); + assertEquals("invalid query parser 'bleh' for query '{!bleh}'", exception.getMessage()); + } }