Deprecates _search request with trailing tokens

jimczi · jimczi · commit 02ca61c41e7d · 2018-04-12T10:29:05.000+02:00
This change validates that the _search request does not have trailing tokens after the main object and prints a deprecation message otherwise. Relates #29428
diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java
@@ -1157,7 +1157,7 @@ public void testMultiSearch() throws IOException {
 
         List<SearchRequest> requests = new ArrayList<>();
         CheckedBiConsumer<SearchRequest, XContentParser, IOException> consumer = (searchRequest, p) -> {
-            SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p);
+            SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p, false);
             if (searchSourceBuilder.equals(new SearchSourceBuilder()) == false) {
                 searchRequest.source(searchSourceBuilder);
             }
diff --git a/docs/reference/migration/migrate_6_0/search.asciidoc b/docs/reference/migration/migrate_6_0/search.asciidoc
@@ -201,3 +201,10 @@ as each additional term demands extra processing and memory.
 To safeguard against this, the maximum number of terms that can be used in a
 Terms Query request has been limited to 65536. This default maximum can be changed
 for a particular index with the index setting `index.max_terms_count`.
+
+==== Invalid `_search` request body
+
+For 6.x and starting in 6.3 a deprecation warning will be printed to warn
+against search requests that contain extra tokens after the main object.
+These extra tokens were ignored by the query parser before 6.3 but the next
+ major version will not accept invalid body anymore.
diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java
@@ -112,7 +112,7 @@ static SearchRequest convert(SearchTemplateRequest searchTemplateRequest, Search
         try (XContentParser parser = XContentFactory.xContent(XContentType.JSON)
                 .createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, source)) {
             SearchSourceBuilder builder = SearchSourceBuilder.searchSource();
-            builder.parseXContent(parser);
+            builder.parseXContent(parser, false);
             builder.explain(searchTemplateRequest.isExplain());
             builder.profile(searchTemplateRequest.isProfile());
             searchRequest.source(builder);
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java
@@ -223,7 +223,7 @@ public void addSummaryFields(List<String> summaryFields) {
             return RatedDocument.fromXContent(p);
         }, RATINGS_FIELD);
         PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) ->
-                SearchSourceBuilder.fromXContent(p), REQUEST_FIELD);
+                SearchSourceBuilder.fromXContent(p, false), REQUEST_FIELD);
         PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> p.map(), PARAMS_FIELD);
         PARSER.declareStringArray(RatedRequest::addSummaryFields, FIELDS_FIELD);
         PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), TEMPLATE_ID_FIELD);
diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java
@@ -107,7 +107,7 @@ protected void doExecute(RankEvalRequest request, ActionListener<RankEvalRespons
                 String resolvedRequest = templateScript.newInstance(params).execute();
                 try (XContentParser subParser = createParser(namedXContentRegistry,
                     LoggingDeprecationHandler.INSTANCE, new BytesArray(resolvedRequest), XContentType.JSON)) {
-                    ratedSearchSource = SearchSourceBuilder.fromXContent(subParser);
+                    ratedSearchSource = SearchSourceBuilder.fromXContent(subParser, false);
                 } catch (IOException e) {
                     // if we fail parsing, put the exception into the errors map and continue
                     errors.put(ratedRequest.getId(), e);
diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestReindexAction.java
@@ -77,7 +77,7 @@ public class RestReindexAction extends AbstractBaseReindexRestHandler<ReindexReq
             try (InputStream stream = BytesReference.bytes(builder).streamInput();
                  XContentParser innerParser = parser.contentType().xContent()
                      .createParser(parser.getXContentRegistry(), parser.getDeprecationHandler(), stream)) {
-                request.getSearchRequest().source().parseXContent(innerParser);
+                request.getSearchRequest().source().parseXContent(innerParser, false);
             }
         };
 
diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestMultiSearchAction.java
@@ -94,7 +94,7 @@ public static MultiSearchRequest parseRequest(RestRequest restRequest, boolean a
 
 
         parseMultiLineRequest(restRequest, multiRequest.indicesOptions(), allowExplicitIndex, (searchRequest, parser) -> {
-            searchRequest.source(SearchSourceBuilder.fromXContent(parser));
+            searchRequest.source(SearchSourceBuilder.fromXContent(parser, false));
             multiRequest.add(searchRequest);
         });
         List<SearchRequest> requests = multiRequest.requests();
diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.rest.action.search;
 
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.client.node.NodeClient;
@@ -109,7 +110,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r
         }
         searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
         if (requestContentParser != null) {
-            searchRequest.source().parseXContent(requestContentParser);
+            searchRequest.source().parseXContent(requestContentParser, true);
         }
 
         final int batchedReduceSize = request.paramAsInt("batched_reduce_size", searchRequest.getBatchedReduceSize());
@@ -128,7 +129,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r
             // only set if we have the parameter passed to override the cluster-level default
             searchRequest.allowPartialSearchResults(request.paramAsBoolean("allow_partial_search_results", null));
         }
-        
+
         // do not allow 'query_and_fetch' or 'dfs_query_and_fetch' search types
         // from the REST layer. these modes are an internal optimization and should
         // not be specified explicitly by the user.
diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
@@ -19,6 +19,8 @@
 
 package org.elasticsearch.search.builder;
 
+import com.fasterxml.jackson.core.JsonParseException;
+import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Nullable;
@@ -111,8 +113,12 @@ public final class SearchSourceBuilder implements Writeable, ToXContentObject, R
     public static final ParseField ALL_FIELDS_FIELDS = new ParseField("all_fields");
 
     public static SearchSourceBuilder fromXContent(XContentParser parser) throws IOException {
+        return fromXContent(parser, true);
+    }
+
+    public static SearchSourceBuilder fromXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException {
         SearchSourceBuilder builder = new SearchSourceBuilder();
-        builder.parseXContent(parser);
+        builder.parseXContent(parser, checkTrailingTokens);
         return builder;
     }
 
@@ -951,12 +957,19 @@ private SearchSourceBuilder shallowCopy(QueryBuilder queryBuilder, QueryBuilder
         return rewrittenBuilder;
     }
 
+    public void parseXContent(XContentParser parser) throws IOException {
+        parseXContent(parser, true);
+    }
+
     /**
      * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent. Use this if you need to set up
-     * different defaults than a regular SearchSourceBuilder would have and use
-     * {@link #fromXContent(XContentParser)} if you have normal defaults.
+     * different defaults than a regular SearchSourceBuilder would have and use {@link #fromXContent(XContentParser, boolean)} if you have
+     * normal defaults.
+     *
+     * @param parser The xContent parser.
+     * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found after the main object.
      */
-    public void parseXContent(XContentParser parser) throws IOException {
+    public void parseXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException {
         XContentParser.Token token = parser.currentToken();
         String currentFieldName = null;
         if (token != XContentParser.Token.START_OBJECT && (token = parser.nextToken()) != XContentParser.Token.START_OBJECT) {
@@ -1106,6 +1119,19 @@ public void parseXContent(XContentParser parser) throws IOException {
                         parser.getTokenLocation());
             }
         }
+        if (checkTrailingTokens) {
+            boolean success;
+            try {
+                token = parser.nextToken();
+                success = token == null;
+            } catch (JsonParseException exc) {
+                success = false;
+            }
+            if (success == false) {
+                DEPRECATION_LOGGER.deprecated("Found extra tokens after the _search request body, " +
+                    "an error will be thrown in the next major version");
+            }
+        }
     }
 
     @Override
diff --git a/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java b/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java
@@ -222,7 +222,7 @@ public void testMultiLineSerialization() throws IOException {
             byte[] originalBytes = MultiSearchRequest.writeMultiLineFormat(originalRequest, xContentType.xContent());
             MultiSearchRequest parsedRequest = new MultiSearchRequest();
             CheckedBiConsumer<SearchRequest, XContentParser, IOException> consumer = (r, p) -> {
-                SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p);
+                SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p, false);
                 if (searchSourceBuilder.equals(new SearchSourceBuilder()) == false) {
                     r.source(searchSourceBuilder);
                 }
@@ -270,7 +270,7 @@ private static MultiSearchRequest createMultiSearchRequest() throws IOException
             if (randomBoolean()) {
                 searchRequest.allowPartialSearchResults(true);
             }
-            
+
             // scroll is not supported in the current msearch api, so unset it:
             searchRequest.scroll((Scroll) null);
 
diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java
@@ -67,6 +67,20 @@ public void testFromXContent() throws IOException {
         assertParseSearchSource(testSearchSourceBuilder, createParser(builder));
     }
 
+    public void testFromXContentInvalid() throws IOException {
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}}")) {
+            SearchSourceBuilder.fromXContent(parser);
+            assertWarnings("Found extra tokens after the _search request body, " +
+                "an error will be thrown in the next major version");
+        }
+
+        try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}{}")) {
+            SearchSourceBuilder.fromXContent(parser);
+            assertWarnings("Found extra tokens after the _search request body, " +
+                "an error will be thrown in the next major version");
+        }
+    }
+
     private static void assertParseSearchSource(SearchSourceBuilder testBuilder, XContentParser parser) throws IOException {
         if (randomBoolean()) {
             parser.nextToken(); // sometimes we move it on the START_OBJECT to

Original file line number	Diff line number	Diff line change
`@@ -1157,7 +1157,7 @@ public void testMultiSearch() throws IOException {`
`1157`	`1157`
`1158`	`1158`	`List<SearchRequest> requests = new ArrayList<>();`
`1159`	`1159`	`CheckedBiConsumer<SearchRequest, XContentParser, IOException> consumer = (searchRequest, p) -> {`
`1160`		`- SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p);`
	`1160`	`+ SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p, false);`
`1161`	`1161`	`if (searchSourceBuilder.equals(new SearchSourceBuilder()) == false) {`
`1162`	`1162`	`searchRequest.source(searchSourceBuilder);`
`1163`	`1163`	`}`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ public class RestReindexAction extends AbstractBaseReindexRestHandler<ReindexReq`
`77`	`77`	`try (InputStream stream = BytesReference.bytes(builder).streamInput();`
`78`	`78`	`XContentParser innerParser = parser.contentType().xContent()`
`79`	`79`	`.createParser(parser.getXContentRegistry(), parser.getDeprecationHandler(), stream)) {`
`80`		`- request.getSearchRequest().source().parseXContent(innerParser);`
	`80`	`+ request.getSearchRequest().source().parseXContent(innerParser, false);`
`81`	`81`	`}`
`82`	`82`	`};`
`83`	`83`