From 9672facf51338ca5f110b0c460f688fc61536c39 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Sat, 26 Oct 2019 14:53:33 -0700 Subject: [PATCH 1/9] Fix SOLR-13207 Traps error that arises when the < operator is used at the end of a query field. Also handles NumberFormatException when the operand isn't a number. --- .../java/org/apache/solr/util/SolrPluginUtils.java | 12 +++++++++++- .../org/apache/solr/util/SolrPluginUtilsTest.java | 11 +++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java index c8f7f73b85f5..dbbf72fe282c 100644 --- a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java +++ b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java @@ -678,7 +678,17 @@ static int calculateMinShouldMatch(int optionalClauseCount, String spec) { spec = spaceAroundLessThanPattern.matcher(spec).replaceAll("<"); for (String s : spacePattern.split(spec)) { String[] parts = lessThanPattern.split(s,0); - int upperBound = Integer.parseInt(parts[0]); + if (parts.length == 0) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Operator < must be followed by a number"); + } + int upperBound; + try { + upperBound = Integer.parseInt(parts[0]); + } catch (NumberFormatException e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Operator < must be followed by a number", e); + } if (optionalClauseCount <= upperBound) { return result; } else { diff --git a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java index 4f50cd031752..6de594e3ec56 100644 --- a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java @@ -33,6 +33,7 @@ import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; import org.apache.solr.request.SolrQueryRequest; import org.apache.solr.search.QParser; import org.apache.solr.util.SolrPluginUtils.DisjunctionMaxQueryParser; @@ -297,6 +298,16 @@ public void testMinShouldMatchCalculator() { assertEquals(1, calcMSM(4, "\n 3 < \n25%\n ")); assertEquals(1, calcMSM(5, "3<25%")); + /* malformed conditionals */ + try { + calcMSM(1, "1<"); + fail("Query '1<' should have thrown SolrException"); + } catch (SolrException expected) {} + try { + calcMSM(1, "1 Date: Mon, 28 Oct 2019 16:55:13 -0700 Subject: [PATCH 2/9] Separate invalid queries into a separate method --- .../apache/solr/util/SolrPluginUtilsTest.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java index 6de594e3ec56..67d36512ff4e 100644 --- a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java @@ -298,16 +298,6 @@ public void testMinShouldMatchCalculator() { assertEquals(1, calcMSM(4, "\n 3 < \n25%\n ")); assertEquals(1, calcMSM(5, "3<25%")); - /* malformed conditionals */ - try { - calcMSM(1, "1<"); - fail("Query '1<' should have thrown SolrException"); - } catch (SolrException expected) {} - try { - calcMSM(1, "1 Date: Mon, 28 Oct 2019 16:57:50 -0700 Subject: [PATCH 3/9] Update CHANGES.txt --- solr/CHANGES.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index e68a4821067a..17029769119d 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -31,6 +31,9 @@ Jetty 9.4.19.v20190610 Upgrade Notes ---------------------- +* SOLR-13207: Queries that use the < operator not followed by a number will now return an HTTP 400 error rather than 500, + and the error message will explain that < must be followed by a number. + * LUCENE-8738: Move to Java 11 as minimum Java version. (Adrien Grand, Uwe Schindler) From 501431d1b75dedb86c83527d9c72dc2e807708a5 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Mon, 28 Oct 2019 16:59:35 -0700 Subject: [PATCH 4/9] Add author name, per this file's de-facto standard --- solr/CHANGES.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 17029769119d..9bdee6098537 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -32,7 +32,7 @@ Upgrade Notes ---------------------- * SOLR-13207: Queries that use the < operator not followed by a number will now return an HTTP 400 error rather than 500, - and the error message will explain that < must be followed by a number. + and the error message will explain that < must be followed by a number. (Chris Hennick) * LUCENE-8738: Move to Java 11 as minimum Java version. (Adrien Grand, Uwe Schindler) From 21b448edf62e1ac633866c7b902e7653e12a0a80 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Mon, 28 Oct 2019 17:13:47 -0700 Subject: [PATCH 5/9] Move CHANGES.txt entry Per https://github.com/apache/lucene-solr/pull/978#discussion_r339845477 --- solr/CHANGES.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 9bdee6098537..5e1fd2184f58 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -31,9 +31,6 @@ Jetty 9.4.19.v20190610 Upgrade Notes ---------------------- -* SOLR-13207: Queries that use the < operator not followed by a number will now return an HTTP 400 error rather than 500, - and the error message will explain that < must be followed by a number. (Chris Hennick) - * LUCENE-8738: Move to Java 11 as minimum Java version. (Adrien Grand, Uwe Schindler) @@ -133,6 +130,9 @@ Bug Fixes * SOLR-12393: Compute score if requested even when expanded docs not sorted by score in ExpandComponent. (David Smiley, Munendra S N) +* SOLR-13207: In dismax and edismax "minimum should match" queries, the < operator not followed by a number will now return + an HTTP 400 error rather than 500, and the error message will explain that < must be followed by a number. (Chris Hennick) + Other Changes --------------------- From f7762f611dd34cedc4ea1541ad61cffecb735060 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Tue, 29 Oct 2019 19:31:50 -0700 Subject: [PATCH 6/9] Use expectThrows --- .../test/org/apache/solr/util/SolrPluginUtilsTest.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java index 67d36512ff4e..37da0e567a64 100644 --- a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java @@ -341,14 +341,8 @@ public void testMinShouldMatchCalculator() { @Test public void testMinShouldMatchBadQueries() { - try { - calcMSM(1, "1<"); - fail("Query '1<' should have thrown SolrException"); - } catch (SolrException expected) {} - try { - calcMSM(1, "1 calcMSM(1, "1<")); + expectThrows(SolrException.class, () -> calcMSM(1, "1 Date: Tue, 29 Oct 2019 19:39:23 -0700 Subject: [PATCH 7/9] Handle % operator's parseInt --- lucene/tools/src/groovy/install-markdown-filter.groovy | 2 +- .../src/java/org/apache/solr/util/SolrPluginUtils.java | 8 +++++++- .../test/org/apache/solr/util/SolrPluginUtilsTest.java | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lucene/tools/src/groovy/install-markdown-filter.groovy b/lucene/tools/src/groovy/install-markdown-filter.groovy index bd5ceb3ca4a5..e4edf14c1662 100644 --- a/lucene/tools/src/groovy/install-markdown-filter.groovy +++ b/lucene/tools/src/groovy/install-markdown-filter.groovy @@ -28,7 +28,7 @@ import com.vladsch.flexmark.html.HtmlRenderer; import com.vladsch.flexmark.parser.Parser; import com.vladsch.flexmark.parser.ParserEmulationProfile; import com.vladsch.flexmark.util.html.Escaping; -import com.vladsch.flexmark.util.options.MutableDataSet; +import com.vladsch.flexmark.util.data.MutableDataSet; import com.vladsch.flexmark.ext.abbreviation.AbbreviationExtension; import com.vladsch.flexmark.ext.autolink.AutolinkExtension; diff --git a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java index dbbf72fe282c..fd37e9e1a13a 100644 --- a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java +++ b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java @@ -704,7 +704,13 @@ static int calculateMinShouldMatch(int optionalClauseCount, String spec) { if (-1 < spec.indexOf('%')) { /* percentage - assume the % was the last char. If not, let Integer.parseInt fail. */ spec = spec.substring(0,spec.length()-1); - int percent = Integer.parseInt(spec); + int percent; + try { + percent = Integer.parseInt(spec); + } catch (NumberFormatException e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "% must be preceded by a number and not combined with other operators", e); + } float calc = (result * percent) * (1/100f); result = calc < 0 ? result + (int)calc : (int)calc; } else { diff --git a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java index 37da0e567a64..c7e7e454e91b 100644 --- a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java @@ -343,6 +343,8 @@ public void testMinShouldMatchCalculator() { public void testMinShouldMatchBadQueries() { expectThrows(SolrException.class, () -> calcMSM(1, "1<")); expectThrows(SolrException.class, () -> calcMSM(1, "1 calcMSM(1, "x%")); + expectThrows(SolrException.class, () -> calcMSM(1, "%%")); } @Test From 6cac06fbb2bbeb74ca1832ac2f572ad86f774b99 Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Tue, 29 Oct 2019 19:44:28 -0700 Subject: [PATCH 8/9] Factor out checkedParseInt --- .../org/apache/solr/util/SolrPluginUtils.java | 38 +++++++++++-------- .../apache/solr/util/SolrPluginUtilsTest.java | 1 + 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java index fd37e9e1a13a..c4ad454cf1ff 100644 --- a/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java +++ b/solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java @@ -682,13 +682,7 @@ static int calculateMinShouldMatch(int optionalClauseCount, String spec) { throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Operator < must be followed by a number"); } - int upperBound; - try { - upperBound = Integer.parseInt(parts[0]); - } catch (NumberFormatException e) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "Operator < must be followed by a number", e); - } + int upperBound = checkedParseInt(parts[0], "Operator < must be followed by a number"); if (optionalClauseCount <= upperBound) { return result; } else { @@ -704,17 +698,12 @@ static int calculateMinShouldMatch(int optionalClauseCount, String spec) { if (-1 < spec.indexOf('%')) { /* percentage - assume the % was the last char. If not, let Integer.parseInt fail. */ spec = spec.substring(0,spec.length()-1); - int percent; - try { - percent = Integer.parseInt(spec); - } catch (NumberFormatException e) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, - "% must be preceded by a number and not combined with other operators", e); - } + int percent = checkedParseInt(spec, + "% must be preceded by a number and not combined with other operators"); float calc = (result * percent) * (1/100f); result = calc < 0 ? result + (int)calc : (int)calc; } else { - int calc = Integer.parseInt(spec); + int calc = checkedParseInt(spec, "Input should be a number"); result = calc < 0 ? result + calc : calc; } @@ -723,6 +712,25 @@ static int calculateMinShouldMatch(int optionalClauseCount, String spec) { } + /** + * Wrapper of {@link Integer#parseInt(String)} that wraps any {@link NumberFormatException} in a + * {@link SolrException} with HTTP 400 Bad Request status. + * + * @param input the string to parse + * @param errorMessage the error message for any SolrException + * @return the integer value of {@code input} + * @throws SolrException when parseInt throws NumberFormatException + */ + private static int checkedParseInt(String input, String errorMessage) { + int percent; + try { + percent = Integer.parseInt(input); + } catch (NumberFormatException e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, errorMessage, e); + } + return percent; + } + /** * Recursively walks the "from" query pulling out sub-queries and diff --git a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java index c7e7e454e91b..58d56a171b75 100644 --- a/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java @@ -345,6 +345,7 @@ public void testMinShouldMatchBadQueries() { expectThrows(SolrException.class, () -> calcMSM(1, "1 calcMSM(1, "x%")); expectThrows(SolrException.class, () -> calcMSM(1, "%%")); + expectThrows(SolrException.class, () -> calcMSM(1, "x")); } @Test From 46644b0b57095b44402ef2b7764911a085f71c2c Mon Sep 17 00:00:00 2001 From: Chris Hennick <4961925+Pr0methean@users.noreply.github.com> Date: Wed, 30 Oct 2019 15:06:46 -0700 Subject: [PATCH 9/9] Revert an import only needed on my own machine --- lucene/tools/src/groovy/install-markdown-filter.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lucene/tools/src/groovy/install-markdown-filter.groovy b/lucene/tools/src/groovy/install-markdown-filter.groovy index e4edf14c1662..bd5ceb3ca4a5 100644 --- a/lucene/tools/src/groovy/install-markdown-filter.groovy +++ b/lucene/tools/src/groovy/install-markdown-filter.groovy @@ -28,7 +28,7 @@ import com.vladsch.flexmark.html.HtmlRenderer; import com.vladsch.flexmark.parser.Parser; import com.vladsch.flexmark.parser.ParserEmulationProfile; import com.vladsch.flexmark.util.html.Escaping; -import com.vladsch.flexmark.util.data.MutableDataSet; +import com.vladsch.flexmark.util.options.MutableDataSet; import com.vladsch.flexmark.ext.abbreviation.AbbreviationExtension; import com.vladsch.flexmark.ext.autolink.AutolinkExtension;