You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mk...@apache.org on 2019/02/24 13:41:15 UTC
[lucene-solr] branch branch_8x updated: SOLR-11883: reporting
syntax errors as Bad Request instead of 500.
This is an automated email from the ASF dual-hosted git repository.
mkhl 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 d68e992 SOLR-11883: reporting syntax errors as Bad Request instead of 500.
d68e992 is described below
commit d68e992d0326298c3a6a2fc15ba2b7cd91421d80
Author: Mikhail Khludnev <mk...@apache.org>
AuthorDate: Sat Feb 23 23:46:48 2019 +0300
SOLR-11883: reporting syntax errors as Bad Request instead of 500.
---
solr/CHANGES.txt | 3 ++
.../org/apache/solr/search/FunctionQParser.java | 60 ++++++++++++++++------
.../solr/search/function/TestFunctionQuery.java | 47 +++++++++++++++++
3 files changed, 93 insertions(+), 17 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 84a8cca..62b3bf1 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -53,6 +53,9 @@ Bug Fixes
* SOLR-12708: Async collection actions should not hide internal failures (Mano Kovacs, Varun Thacker, Tomás Fernández Löbbe)
+* SOLR-11883: 500 code on functional query syntax errors and parameter dereferencing errors
+(Munendra S N via Mikhail Khludnev)
+
Improvements
----------------------
* SOLR-12999: Index replication could delete segments before downloading segments from master if there is not enough
diff --git a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
index d3a311d..ed5379a 100644
--- a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
+++ b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
@@ -16,9 +16,16 @@
*/
package org.apache.solr.search;
+import java.util.ArrayList;
+import java.util.List;
+
import org.apache.lucene.queries.function.FunctionQuery;
import org.apache.lucene.queries.function.ValueSource;
-import org.apache.lucene.queries.function.valuesource.*;
+import org.apache.lucene.queries.function.valuesource.ConstValueSource;
+import org.apache.lucene.queries.function.valuesource.DoubleConstValueSource;
+import org.apache.lucene.queries.function.valuesource.LiteralValueSource;
+import org.apache.lucene.queries.function.valuesource.QueryValueSource;
+import org.apache.lucene.queries.function.valuesource.VectorValueSource;
import org.apache.lucene.search.Query;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
@@ -27,9 +34,6 @@ import org.apache.solr.schema.SchemaField;
import org.apache.solr.search.facet.AggValueSource;
import org.apache.solr.search.function.FieldNameValueSource;
-import java.util.ArrayList;
-import java.util.List;
-
public class FunctionQParser extends QParser {
public static final int FLAG_CONSUME_DELIMITER = 0x01; // consume delimiter after parsing arg
@@ -130,7 +134,11 @@ public class FunctionQParser extends QParser {
*/
public String parseId() throws SyntaxError {
String value = parseArg();
- if (argWasQuoted) throw new SyntaxError("Expected identifier instead of quoted string:" + value);
+ if (argWasQuoted()) {
+ throw new SyntaxError("Expected identifier instead of quoted string:" + value);
+ } else if (value == null) {
+ throw new SyntaxError("Expected identifier instead of 'null' for function " + sp);
+ }
return value;
}
@@ -142,8 +150,11 @@ public class FunctionQParser extends QParser {
public Float parseFloat() throws SyntaxError {
String str = parseArg();
if (argWasQuoted()) throw new SyntaxError("Expected float instead of quoted string:" + str);
- float value = Float.parseFloat(str);
- return value;
+ try {
+ return Float.parseFloat(str);
+ } catch (NumberFormatException | NullPointerException e) {
+ throw new SyntaxError("Expected float instead of '" + str + "' for function " + sp);
+ }
}
/**
@@ -153,8 +164,11 @@ public class FunctionQParser extends QParser {
public double parseDouble() throws SyntaxError {
String str = parseArg();
if (argWasQuoted()) throw new SyntaxError("Expected double instead of quoted string:" + str);
- double value = Double.parseDouble(str);
- return value;
+ try {
+ return Double.parseDouble(str);
+ } catch (NumberFormatException | NullPointerException e) {
+ throw new SyntaxError("Expected double instead of '" + str + "' for function " + sp);
+ }
}
/**
@@ -163,9 +177,12 @@ public class FunctionQParser extends QParser {
*/
public int parseInt() throws SyntaxError {
String str = parseArg();
- if (argWasQuoted()) throw new SyntaxError("Expected double instead of quoted string:" + str);
- int value = Integer.parseInt(str);
- return value;
+ if (argWasQuoted()) throw new SyntaxError("Expected integer instead of quoted string:" + str);
+ try {
+ return Integer.parseInt(str);
+ } catch (NumberFormatException | NullPointerException e) {
+ throw new SyntaxError("Expected integer instead of '" + str + "' for function " + sp);
+ }
}
@@ -248,6 +265,12 @@ public class FunctionQParser extends QParser {
String qstr = getParam(param);
qstr = qstr==null ? "" : qstr;
nestedQuery = subQuery(qstr, null).getQuery();
+
+ // nestedQuery would be null when de-referenced query value is not specified
+ // Ex: query($qq) in request with no qq param specified
+ if (nestedQuery == null) {
+ throw new SyntaxError("Missing param " + param + " while parsing function '" + sp.val + "'");
+ }
}
else {
int start = sp.pos;
@@ -277,9 +300,14 @@ public class FunctionQParser extends QParser {
sp.pos += end-start; // advance past nested query
nestedQuery = sub.getQuery();
+ // handling null check on nestedQuery separately, so that proper error can be returned
+ // one case this would be possible when v is specified but v's value is empty or has only spaces
+ if (nestedQuery == null) {
+ throw new SyntaxError("Nested function query returned null for '" + sp.val + "'");
+ }
}
consumeArgumentDelimiter();
-
+
return nestedQuery;
}
@@ -369,8 +397,7 @@ public class FunctionQParser extends QParser {
}
valueSource = argParser.parse(this);
sp.expect(")");
- }
- else {
+ } else {
if ("true".equals(id)) {
valueSource = new BoolConstValueSource(true);
} else if ("false".equals(id)) {
@@ -413,14 +440,13 @@ public class FunctionQParser extends QParser {
hasParen = sp.opt("(");
ValueSourceParser argParser = req.getCore().getValueSourceParser(id);
- argParser = req.getCore().getValueSourceParser(id);
if (argParser == null) {
throw new SyntaxError("Unknown aggregation " + id + " in (" + sp + ")");
}
ValueSource vv = argParser.parse(this);
if (!(vv instanceof AggValueSource)) {
- if (argParser == null) {
+ if (argParser == null) { // why this??
throw new SyntaxError("Expected aggregation from " + id + " but got (" + vv + ") in (" + sp + ")");
}
}
diff --git a/solr/core/src/test/org/apache/solr/search/function/TestFunctionQuery.java b/solr/core/src/test/org/apache/solr/search/function/TestFunctionQuery.java
index edce3bb..de10cee 100644
--- a/solr/core/src/test/org/apache/solr/search/function/TestFunctionQuery.java
+++ b/solr/core/src/test/org/apache/solr/search/function/TestFunctionQuery.java
@@ -423,6 +423,53 @@ public class TestFunctionQuery extends SolrTestCaseJ4 {
,"*//doc[1]/str[.='120']"
,"*//doc[2]/str[.='121']"
);
+
+ // test a query that doesn't specify nested query val
+ assertQEx("Should fail because of missing qq",
+ "Missing param qq while parsing function 'query($qq)'",
+ req("q", "*:*", "fq","id:120 OR id:121", "defType","edismax", "boost","query($qq)"),
+ SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail because of missing sortfunc in sort",
+ "Can't determine a Sort Order (asc or desc) in sort spec '{!func v=$sortfunc} desc'",
+ req("q", "*:*", "fq","id:120 OR id:121", "sort","{!func v=$sortfunc} desc", "sortfunc","query($qq)"),
+ SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail because of missing qq in boost",
+ "Nested local params must have value in v parameter. got 'query({!dismax v=$qq})",
+ req("q", "*:*", "fq","id:120 OR id:121", "defType","edismax", "boost","query({!dismax v=$qq})"),
+ SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail as empty value is specified for v",
+ "Nested function query returned null for 'query({!v=})'",
+ req("q", "*:*", "defType","edismax", "boost","query({!v=})"), SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail as v's value contains only spaces",
+ "Nested function query returned null for 'query({!v= })'",
+ req("q", "*:*", "defType","edismax", "boost","query({!v= })"), SolrException.ErrorCode.BAD_REQUEST
+ );
+
+ // no field specified in ord()
+ assertQEx("Should fail as no field is specified in ord func",
+ "Expected identifier instead of 'null' for function 'ord()'",
+ req("q", "*:*", "defType","edismax","boost","ord()"), SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail as no field is specified in rord func",
+ "Expected identifier instead of 'null' for function 'rord()'",
+ req("q", "*:*", "defType","edismax","boost","rord()"), SolrException.ErrorCode.BAD_REQUEST
+ );
+
+ // test parseFloat
+ assertQEx("Should fail as less args are specified for recip func",
+ "Expected float instead of 'null' for function 'recip(1,2)'",
+ req("q", "*:*","defType","edismax", "boost","recip(1,2)"), SolrException.ErrorCode.BAD_REQUEST
+ );
+ assertQEx("Should fail as invalid value is specified for recip func",
+ "Expected float instead of 'f' for function 'recip(1,2,3,f)'",
+ req("q", "*:*","defType","edismax", "boost","recip(1,2,3,f)"), SolrException.ErrorCode.BAD_REQUEST
+ );
+ // this should pass
+ assertQ(req("q", "*:*","defType","edismax", "boost","recip(1, 2, 3, 4)"));
}
@Test