LUCENE-8905: Better Error Handling For Illegal Arguments (#769)

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

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

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);
     }
 

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);
+    IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> {
+      tdc.topDocs(-1);
+    });
 
-    // start > pq.size()
-    assertEquals(0, tdc.topDocs(numResults + 1).scoreDocs.length);
+    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 {
@@ -210,6 +215,22 @@ public class TestTopDocsCollector extends LuceneTestCase {
     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.
   public void testResultsOrder() throws Exception {