fix issue: Unhandled query size parameter. the github issue is:elasti…

…c#22530
makeyang · Jan 12, 2017 · b6cc4cb · b6cc4cb
1 parent baef86b
commit b6cc4cb
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 17 deletions.
diff --git a/core/src/main/java/org/elasticsearch/action/search/SearchRequest.java b/core/src/main/java/org/elasticsearch/action/search/SearchRequest.java
@@ -39,6 +39,8 @@
 import java.util.Collections;
 import java.util.Objects;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 /**
  * A request to execute search against one or more indices (or all). Best created using
  * {@link org.elasticsearch.client.Requests#searchRequest(String...)}.
@@ -100,7 +102,20 @@ public SearchRequest(String[] indices, SearchSourceBuilder source) {
 
     @Override
     public ActionRequestValidationException validate() {
-        return null;
+        ActionRequestValidationException validationException = null;
+        if (source().from() != null) {
+            int from = source().from().intValue();
+            if (from  < 0) {
+                validationException = addValidationError("from must be no negative but was [" + from + "]", validationException);
+            }
+        }
+        if(source().size() != null) {
+            int size = source().size().intValue();
+            if (size < 0) {
+                validationException = addValidationError("size must be no negative but was [" + size + "]", validationException);
+            }
+        }
+        return validationException;
     }
 
     /**

diff --git a/core/src/main/java/org/elasticsearch/rest/RestRequest.java b/core/src/main/java/org/elasticsearch/rest/RestRequest.java
@@ -186,6 +186,18 @@ public int paramAsInt(String key, int defaultValue) {
         }
     }
 
+    public Integer paramAsInteger(String key, Integer defaultValue) {
+        String sValue = param(key);
+        if(sValue == null) {
+            return defaultValue;
+        }
+        try {
+            return Integer.valueOf(sValue);
+        } catch (NumberFormatException e) {
+            throw new IllegalArgumentException("Failed to parse Integer parameter [" + key + "] with value [" + sValue + "]", e);
+        }
+    }
+
     public long paramAsLong(String key, long defaultValue) {
         String sValue = param(key);
         if (sValue == null) {

diff --git a/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/core/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
@@ -130,15 +130,12 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil
             searchSourceBuilder.query(queryBuilder);
         }
 
-        int from = request.paramAsInt("from", -1);
-        if (from != -1) {
-            searchSourceBuilder.from(from);
+        if (request.hasParam("from")) {
+            searchSourceBuilder.from(request.paramAsInteger("from", null));
         }
-        int size = request.paramAsInt("size", -1);
-        if (size != -1) {
-            searchSourceBuilder.size(size);
+        if (request.hasParam("size")) {
+            searchSourceBuilder.size(request.paramAsInteger("size", null));
         }
-
         if (request.hasParam("explain")) {
             searchSourceBuilder.explain(request.paramAsBoolean("explain", null));
         }

diff --git a/core/src/main/java/org/elasticsearch/search/SearchService.java b/core/src/main/java/org/elasticsearch/search/SearchService.java
@@ -663,8 +663,16 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
             return;
         }
         QueryShardContext queryShardContext = context.getQueryShardContext();
-        context.from(source.from());
-        context.size(source.size());
+        if (source.from() != null) {
+            context.from(source.from());
+        } else {
+            context.from(-1);
+        }
+        if (source.size() != null) {
+            context.size(source.size());
+        } else {
+            context.size(-1);
+        }
         Map<String, InnerHitBuilder> innerHitBuilders = new HashMap<>();
         if (source.query() != null) {
             InnerHitBuilder.extractInnerHits(source.query(), innerHitBuilders);

diff --git a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
@@ -126,9 +126,9 @@ public static HighlightBuilder highlight() {
 
     private QueryBuilder postQueryBuilder;
 
-    private int from = -1;
+    private Integer from = null;
 
-    private int size = -1;
+    private Integer size = null;
 
     private Boolean explain;
 
@@ -303,30 +303,30 @@ public QueryBuilder postFilter() {
     /**
      * From index to start the search from. Defaults to <tt>0</tt>.
      */
-    public SearchSourceBuilder from(int from) {
+    public SearchSourceBuilder from(Integer from) {
         this.from = from;
         return this;
     }
 
     /**
      * Gets the from index to start the search from.
      **/
-    public int from() {
+    public Integer from() {
         return from;
     }
 
     /**
      * The number of search hits to return. Defaults to <tt>10</tt>.
      */
-    public SearchSourceBuilder size(int size) {
+    public SearchSourceBuilder size(Integer size) {
         this.size = size;
         return this;
     }
 
     /**
      * Gets the number of search hits to return.
      */
-    public int size() {
+    public Integer size() {
         return size;
     }
 

diff --git a/core/src/test/java/org/elasticsearch/search/SearchRequestTests.java b/core/src/test/java/org/elasticsearch/search/SearchRequestTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.search;
 
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.action.support.IndicesOptions;
@@ -27,6 +28,7 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.util.ArrayUtils;
+import org.elasticsearch.search.builder.SearchSourceBuilder;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -84,6 +86,21 @@ public void testEqualsAndHashcode() throws IOException {
         checkEqualsAndHashCode(createSearchRequest(), SearchRequestTests::copyRequest, this::mutate);
     }
 
+    public void testInvalidFromAndSize() throws IOException {
+        SearchRequest searchRequest = createSearchRequest();
+        searchRequest.source(new SearchSourceBuilder()).source().from(new Integer(-1));
+        ActionRequestValidationException ex = searchRequest.validate();
+        assertNotNull("from validation should fail", ex);
+        assertTrue(ex.getMessage().contains("from must be no negative but was [-1]"));
+
+        searchRequest =  createSearchRequest();
+        searchRequest.source(new SearchSourceBuilder()).source().size(new Integer(-1));
+        ex = searchRequest.validate();
+        assertNotNull("size validation should fail", ex);
+        assertTrue(ex.getMessage().contains("size must be no negative but was [-1]"));
+
+    }
+
     private SearchRequest mutate(SearchRequest searchRequest) throws IOException {
         SearchRequest mutation = copyRequest(searchRequest);
         List<Runnable> mutators = new ArrayList<>();

diff --git a/...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java b/...es/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java
@@ -137,7 +137,7 @@ public AbstractBulkByScrollRequest(SearchRequest searchRequest, boolean setDefau
     @Override
     public ActionRequestValidationException validate() {
         ActionRequestValidationException e = searchRequest.validate();
-        if (searchRequest.source().from() != -1) {
+        if (searchRequest.source().from() != null) {
             e = addValidationError("from is not supported in this context", e);
         }
         if (searchRequest.source().storedFields() != null) {