From f1ab009e1f5aa83251c82b2080ea0b662f0a8b70 Mon Sep 17 00:00:00 2001
From: Yonik Seeley <yonik@apache.org>
Date: Tue, 24 Mar 2015 20:05:23 +0000
Subject: [PATCH] SOLR-7254: invalid start/rows should throw 400 rather than
 500 error code

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1668976 13f79535-47bb-0310-9956-ffa450edef68
---
 solr/CHANGES.txt                              |  4 +++
 .../handler/component/QueryComponent.java     | 14 +++++++---
 .../apache/solr/TestDistributedSearch.java    | 26 ++++++++++++++++++-
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index d15a714cdd5..b1808c2bd32 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -292,6 +292,10 @@ Bug Fixes
 * SOLR-7294: Migrate API fails with 'Invalid status request: notfoundretried 6times' message.
   (Jessica Cheng Mallet, shalin)
 
+* SOLR-7254: Make an invalid negative start/rows throw a HTTP 400 error (Bad Request) instead
+  of causing a 500 error.  (Ramkumar Aiyengar, Hrishikesh Gadre, yonik)
+ 
+
 Optimizations
 ----------------------
 
diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
index 8dbfa81867d..c1bb95b1cd8 100644
--- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
+++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
@@ -216,6 +216,16 @@ public class QueryComponent extends SearchComponent
 
     if (params.getBool(GroupParams.GROUP, false)) {
       prepareGrouping(rb);
+    } else {
+      //Validate only in case of non-grouping search.
+      if(rb.getSortSpec().getCount() < 0) {
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'rows' parameter cannot be negative");
+      }
+    }
+
+    //Input validation.
+    if (rb.getQueryCommand().getOffset() < 0) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
     }
   }
 
@@ -306,10 +316,6 @@ public class QueryComponent extends SearchComponent
       statsCache.receiveGlobalStats(req);
     }
 
-    if (rb.getQueryCommand().getOffset() < 0) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' parameter cannot be negative");
-    }
-
     // -1 as flag if not set.
     long timeAllowed = params.getLong(CommonParams.TIME_ALLOWED, -1L);
     if (null != rb.getCursorMark() && 0 < timeAllowed) {
diff --git a/solr/core/src/test/org/apache/solr/TestDistributedSearch.java b/solr/core/src/test/org/apache/solr/TestDistributedSearch.java
index dc447556ab0..d7feb44de31 100644
--- a/solr/core/src/test/org/apache/solr/TestDistributedSearch.java
+++ b/solr/core/src/test/org/apache/solr/TestDistributedSearch.java
@@ -19,8 +19,8 @@ package org.apache.solr;
 
 import org.apache.commons.lang.StringUtils;
 import org.apache.lucene.util.LuceneTestCase.Slow;
-
 import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrQuery;
 import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.embedded.JettySolrRunner;
@@ -32,6 +32,7 @@ import org.apache.solr.client.solrj.response.FieldStatsInfo;
 import org.apache.solr.cloud.ChaosMonkey;
 import org.apache.solr.common.EnumFieldValue;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.ShardParams;
@@ -152,6 +153,9 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
     handle.put("timestamp", SKIPVAL);
     handle.put("_version_", SKIPVAL); // not a cloud test, but may use updateLog
 
+    //Test common query parameters.
+    validateCommonQueryParameters();
+
     // random value sort
     for (String f : fieldNames) {
       query("q","*:*", "sort",f+" desc");
@@ -1087,4 +1091,24 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
     super.validateControlData(control);
     assertNull("Expected the partialResults header to be null", control.getHeader().get("partialResults"));
   }
+
+  private void validateCommonQueryParameters() throws Exception {
+    try {
+      SolrQuery query = new SolrQuery();
+      query.setStart(-1).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.setRows(-1).setStart(0).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());
+   }
+  }
 }