From 02792de0e5501bab780a50f7f692da89c1460165 Mon Sep 17 00:00:00 2001
From: Atri Sharma <atri.jiit@gmail.com>
Date: Thu, 5 Sep 2019 19:44:34 +0530
Subject: [PATCH] LUCENE-8905: Better Error Handling For Illegal Arguments
 (#769)

---
 lucene/CHANGES.txt                            |  3 ++
 lucene/MIGRATE.txt                            |  7 ++++
 .../lucene/search/TopDocsCollector.java       | 15 ++++---
 .../lucene/search/TestTopDocsCollector.java   | 41 ++++++++++++++-----
 4 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt
index 43b46e886df..4fe5228c98d 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 ef34b4e43d2..a4c181ba365 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 3e9f8344e92..1d311605698 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 d7fa540c781..f4128b94e80 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.