SOLR-12427: Correct status for invalid 'start', 'rows'

Prior to this commit we correctly handled negative start/rows param
values by returning a 400 (BAD REQUEST) with an appropriate error
message, but would return an ugly 500 with stack trace for non-numeric
input values.  This commit corrects this later case to also return
a 400 status code with a nicer error message.

solr/CHANGES.txt

@@ -127,6 +127,8 @@ Bug Fixes
 * SOLR-12326: JSON Facet API: terms facet shard requests now indicate if they have more buckets to prevent
   unnecessary refinement requests. (yonk)
 
+* SOLR-12427: Improve error message for invalid 'start', 'rows' parameters. (Munendra S N via Jason Gerlowski)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/search/QParser.java

@@ -16,6 +16,12 @@
  */
 package org.apache.solr.search;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
 import org.apache.lucene.search.Query;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
@@ -24,8 +30,6 @@ import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.StrUtils;
 import org.apache.solr.request.SolrQueryRequest;
 
-import java.util.*;
-
 /**
  * <b>Note: This API is experimental and may change in non backward-compatible ways in the future</b>
  * 
@@ -244,16 +248,16 @@ public abstract class QParser {
     getQuery(); // ensure query is parsed first
 
     String sortStr = null;
-    String startS = null;
+    Integer start = null;
-    String rowsS = null;
+    Integer rows = null;
 
     if (localParams != null) {
       sortStr = localParams.get(CommonParams.SORT);
-      startS = localParams.get(CommonParams.START);
+      start = localParams.getInt(CommonParams.START);
-      rowsS = localParams.get(CommonParams.ROWS);
+      rows = localParams.getInt(CommonParams.ROWS);
 
       // if any of these parameters are present, don't go back to the global params
-      if (sortStr != null || startS != null || rowsS != null) {
+      if (sortStr != null || start != null || rows != null) {
         useGlobalParams = false;
       }
     }
@@ -262,16 +266,16 @@ public abstract class QParser {
       if (sortStr ==null) {
           sortStr = params.get(CommonParams.SORT);
       }
-      if (startS==null) {
+      if (start == null) {
-        startS = params.get(CommonParams.START);
+        start = params.getInt(CommonParams.START);
       }
-      if (rowsS==null) {
+      if (rows == null) {
-        rowsS = params.get(CommonParams.ROWS);
+        rows = params.getInt(CommonParams.ROWS);
       }
     }
 
-    int start = startS != null ? Integer.parseInt(startS) : CommonParams.START_DEFAULT;
+    start = start != null ? start : CommonParams.START_DEFAULT;
-    int rows = rowsS != null ? Integer.parseInt(rowsS) : CommonParams.ROWS_DEFAULT;
+    rows = rows != null ? rows : CommonParams.ROWS_DEFAULT;
 
     SortSpec sort = SortSpecParsing.parseSortSpec(sortStr, req);
 

solr/core/src/test/org/apache/solr/TestDistributedSearch.java

@@ -1211,6 +1211,16 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
 
   private void validateCommonQueryParameters() throws Exception {
     ignoreException("parameter cannot be negative");
+
+    try {
+      SolrQuery query = new SolrQuery();
+      query.setParam("start", "non_numeric_value").setQuery("*");
+      QueryResponse resp = query(query);
+      fail("Expected the last query to fail, but got response: " + resp);
+    } catch (SolrException e) {
+      assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
+    }
+
     try {
       SolrQuery query = new SolrQuery();
       query.setStart(-1).setQuery("*");
@@ -1228,6 +1238,15 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
     } catch (SolrException e) {
       assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
     }
+
+    try {
+      SolrQuery query = new SolrQuery();
+      query.setParam("rows", "non_numeric_value").setQuery("*");
+      QueryResponse resp = query(query);
+      fail("Expected the last query to fail, but got response: " + resp);
+    } catch (SolrException e) {
+      assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
+    }
     resetExceptionIgnores();
   }
 }