You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by jp...@apache.org on 2019/09/05 14:14:42 UTC
[lucene-solr] branch master updated: LUCENE-8905: Better Error
Handling For Illegal Arguments (#769)
This is an automated email from the ASF dual-hosted git repository.
jpountz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/master by this push:
new 02792de LUCENE-8905: Better Error Handling For Illegal Arguments (#769)
02792de is described below
commit 02792de0e5501bab780a50f7f692da89c1460165
Author: Atri Sharma <at...@gmail.com>
AuthorDate: Thu Sep 5 19:44:34 2019 +0530
LUCENE-8905: Better Error Handling For Illegal Arguments (#769)
---
lucene/CHANGES.txt | 3 ++
lucene/MIGRATE.txt | 7 ++++
.../org/apache/lucene/search/TopDocsCollector.java | 15 +++++---
.../apache/lucene/search/TestTopDocsCollector.java | 41 ++++++++++++++++------
4 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index 43b46e8..4fe5228 100644
--- a/lucene/CHANGES.txt
+++ b/lucene/CHANGES.txt
@@ -35,6 +35,9 @@ API Changes
* LUCENE-8933: Validate JapaneseTokenizer user dictionary entry. (Tomoko Uchida)
+* LUCENE-8905: Better defence against malformed arguments in TopDocsCollector
+ (Atri Sharma)
+
Improvements
* LUCENE-8757: When provided with an ExecutorService to run queries across
diff --git a/lucene/MIGRATE.txt b/lucene/MIGRATE.txt
index ef34b4e..a4c181b 100644
--- a/lucene/MIGRATE.txt
+++ b/lucene/MIGRATE.txt
@@ -221,3 +221,10 @@ set shard indices for hits as they are seen during the merge process. This is do
to be more dynamic in terms of passing in custom tie breakers.
If shard indices are to be used for tie breaking docs with equal scores during TopDocs.merge, then it is
mandatory that the input ScoreDocs have their shard indices set to valid values prior to calling TopDocs.merge
+
+## TopDocsCollector Shall Throw IllegalArgumentException For Malformed Arguments ##
+
+TopDocsCollector shall no longer return an empty TopDocs for malformed arguments.
+Rather, an IllegalArgumentException shall be thrown. This is introduced for better
+defence and to ensure that there is no bubbling up of errors when Lucene is
+used in multi level applications
diff --git a/lucene/core/src/java/org/apache/lucene/search/TopDocsCollector.java b/lucene/core/src/java/org/apache/lucene/search/TopDocsCollector.java
index 3e9f834..1d31160 100644
--- a/lucene/core/src/java/org/apache/lucene/search/TopDocsCollector.java
+++ b/lucene/core/src/java/org/apache/lucene/search/TopDocsCollector.java
@@ -136,11 +136,16 @@ public abstract class TopDocsCollector<T extends ScoreDoc> implements Collector
// pq.size() or totalHits.
int size = topDocsSize();
- // Don't bother to throw an exception, just return an empty TopDocs in case
- // the parameters are invalid or out of range.
- // TODO: shouldn't we throw IAE if apps give bad params here so they dont
- // have sneaky silent bugs?
- if (start < 0 || start >= size || howMany <= 0) {
+ if (howMany < 0) {
+ throw new IllegalArgumentException("Number of hits requested must be greater than 0 but value was " + howMany);
+ }
+
+ if (start < 0) {
+ throw new IllegalArgumentException("Expected value of starting position is between 0 and " + size +
+ ", got " + start);
+ }
+
+ if (start >= size || howMany == 0) {
return newTopDocs(null, start);
}
diff --git a/lucene/core/src/test/org/apache/lucene/search/TestTopDocsCollector.java b/lucene/core/src/test/org/apache/lucene/search/TestTopDocsCollector.java
index d7fa540..f4128b9 100644
--- a/lucene/core/src/test/org/apache/lucene/search/TestTopDocsCollector.java
+++ b/lucene/core/src/test/org/apache/lucene/search/TestTopDocsCollector.java
@@ -96,6 +96,10 @@ public class TestTopDocsCollector extends LuceneTestCase {
private TopDocsCollector<ScoreDoc> doSearch(int numResults) throws IOException {
Query q = new MatchAllDocsQuery();
+ return doSearch(numResults, q);
+ }
+
+ private TopDocsCollector<ScoreDoc> doSearch(int numResults, Query q) throws IOException {
IndexSearcher searcher = newSearcher(reader);
TopDocsCollector<ScoreDoc> tdc = new MyTopsDocCollector(numResults);
searcher.search(q, tdc);
@@ -155,20 +159,21 @@ public class TestTopDocsCollector extends LuceneTestCase {
TopDocsCollector<ScoreDoc> tdc = doSearch(numResults);
// start < 0
- assertEquals(0, tdc.topDocs(-1).scoreDocs.length);
-
- // start > pq.size()
- assertEquals(0, tdc.topDocs(numResults + 1).scoreDocs.length);
-
+ IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> {
+ tdc.topDocs(-1);
+ });
+
+ assertEquals("Expected value of starting position is between 0 and 5, got -1", exception.getMessage());
+
// start == pq.size()
assertEquals(0, tdc.topDocs(numResults).scoreDocs.length);
// howMany < 0
- assertEquals(0, tdc.topDocs(0, -1).scoreDocs.length);
-
- // howMany == 0
- assertEquals(0, tdc.topDocs(0, 0).scoreDocs.length);
-
+ exception = expectThrows(IllegalArgumentException.class, () -> {
+ tdc.topDocs(0, -1);
+ });
+
+ assertEquals("Number of hits requested must be greater than 0 but value was -1", exception.getMessage());
}
public void testZeroResults() throws Exception {
@@ -209,6 +214,22 @@ public class TestTopDocsCollector extends LuceneTestCase {
// get the last 5 only.
assertEquals(5, tdc.topDocs(10).scoreDocs.length);
}
+
+ public void testIllegalArguments() throws Exception {
+ final TopDocsCollector<ScoreDoc> tdc = doSearch(15);
+
+ IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> {
+ tdc.topDocs(-1);
+ });
+
+ assertEquals("Expected value of starting position is between 0 and 15, got -1", expected.getMessage());
+
+ expected = expectThrows(IllegalArgumentException.class, () -> {
+ tdc.topDocs(9, -1);
+ });
+
+ assertEquals("Number of hits requested must be greater than 0 but value was -1", expected.getMessage());
+ }
// This does not test the PQ's correctness, but whether topDocs()
// implementations return the results in decreasing score order.