Address review comments

Author: pawankartik-elastic

server/src/main/java/org/elasticsearch/action/search/SearchRequest.java

@@ -175,6 +175,10 @@ public String getProjectRouting() {
     }
 
     public void setProjectRouting(@Nullable String projectRouting) {
+        if (this.projectRouting != null) {
+            throw new IllegalArgumentException("project_routing is already set to [" + this.projectRouting + "]");
+        }
+
         this.projectRouting = projectRouting;
     }
 

server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java

@@ -202,9 +202,9 @@ public static void parseSearchRequest(
         searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index")));
         if (requestContentParser != null) {
             if (searchUsageHolder == null) {
-                searchRequest.source().parseXContent(requestContentParser, true, clusterSupportsFeature);
+                searchRequest.source().parseXContent(searchRequest, requestContentParser, true, clusterSupportsFeature);
             } else {
-                searchRequest.source().parseXContent(requestContentParser, true, searchUsageHolder, clusterSupportsFeature);
+                searchRequest.source().parseXContent(searchRequest, requestContentParser, true, searchUsageHolder, clusterSupportsFeature);
             }
         }
 
@@ -250,7 +250,7 @@ public static void parseSearchRequest(
         }
         searchRequest.indicesOptions(indicesOptions);
 
-        validateSearchRequest(request, searchRequest, crossProjectEnabled);
+        validateSearchRequest(request, searchRequest);
 
         if (searchRequest.pointInTimeBuilder() != null) {
             preparePointInTime(searchRequest, request);
@@ -410,50 +410,10 @@ static void preparePointInTime(SearchRequest request, RestRequest restRequest) {
      * might modify the search request to align certain parameters.
      */
     public static void validateSearchRequest(RestRequest restRequest, SearchRequest searchRequest) {
-        validateSearchRequest(restRequest, searchRequest, false);
-    }
-
-    private static void validateSearchRequest(RestRequest restRequest, SearchRequest searchRequest, boolean crossProjectEnabled) {
         checkRestTotalHits(restRequest, searchRequest);
         checkSearchType(restRequest, searchRequest);
         // ensures that the rest param is consumed
         restRequest.paramAsBoolean(INCLUDE_NAMED_QUERIES_SCORE_PARAM, false);
-        checkProjectRouting(searchRequest, crossProjectEnabled);
-    }
-
-    private static void checkProjectRouting(SearchRequest searchRequest, boolean crossProjectEnabled) {
-        /*
-         * There are 2 ways of specifying project_routing:
-         *   - as a query parameter: /_search?project_routing=..., and,
-         *   - within the request's body.
-         *
-         * Because we do not have access to `IndicesRequest/SearchRequest` from `SearchSourceBuilder`, and, project_routing
-         * can be potentially specified in 2 different places, we need to explicitly check this scenario.
-         */
-        if (searchRequest.source() == null) {
-            return;
-        }
-
-        String projectRoutingInBody = searchRequest.source().projectRouting();
-        // If it's null, either the query parameter is also null or it isn't. Either way, we're fine with it.
-        if (projectRoutingInBody != null) {
-            if (crossProjectEnabled == false) {
-                throw new IllegalArgumentException("project_routing is allowed only when CPS is enabled and the endpoint supports CPS");
-            }
-
-            // Query parameter was also set. This is not allowed, irrespective of the values.
-            if (searchRequest.getProjectRouting() != null) {
-                throw new IllegalArgumentException(
-                    "project_routing is specified in both the places: as query parameter and in the request's body"
-                );
-            }
-
-            /*
-             * Bring forward the project_routing value so that TransportSearchAction can pick it up. Although TSA can pick it up
-             * from `SearchSourceBuilder`, let's use `IndicesRequest#getProjectRouting()` since that's the intended way.
-             */
-            searchRequest.setProjectRouting(projectRoutingInBody);
-        }
     }
 
     /**

server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

@@ -213,8 +213,6 @@ public static HighlightBuilder highlight() {
 
     private boolean skipInnerHits = false;
 
-    private String projectRouting;
-
     /**
      * Constructs a new search source builder.
      */
@@ -612,19 +610,6 @@ public TimeValue timeout() {
         return timeout;
     }
 
-    public String projectRouting() {
-        return projectRouting;
-    }
-
-    public SearchSourceBuilder projectRouting(String projectRouting) {
-        if (this.projectRouting != null) {
-            throw new IllegalArgumentException("project_routing is already set");
-        }
-
-        this.projectRouting = projectRouting;
-        return this;
-    }
-
     /**
      * An optional terminate_after to terminate the search after collecting
      * <code>terminateAfter</code> documents
@@ -1342,6 +1327,26 @@ private SearchSourceBuilder shallowCopy(
         return rewrittenBuilder;
     }
 
+    /**
+     * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent.
+     *
+     * @param searchRequest The SearchRequest object that's representing the request we're parsing which shall receive
+     *                      the parsed info.
+     * @param parser The xContent parser.
+     * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found after the main object.
+     * @param searchUsageHolder holder for the search usage statistics
+     * @param clusterSupportsFeature used to check if certain features are available on this cluster
+     */
+    public SearchSourceBuilder parseXContent(
+        SearchRequest searchRequest,
+        XContentParser parser,
+        boolean checkTrailingTokens,
+        SearchUsageHolder searchUsageHolder,
+        Predicate<NodeFeature> clusterSupportsFeature
+    ) throws IOException {
+        return parseXContent(searchRequest, parser, checkTrailingTokens, searchUsageHolder::updateUsage, clusterSupportsFeature);
+    }
+
     /**
      * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent.
      *
@@ -1356,7 +1361,27 @@ public SearchSourceBuilder parseXContent(
         SearchUsageHolder searchUsageHolder,
         Predicate<NodeFeature> clusterSupportsFeature
     ) throws IOException {
-        return parseXContent(parser, checkTrailingTokens, searchUsageHolder::updateUsage, clusterSupportsFeature);
+        return parseXContent(null, parser, checkTrailingTokens, searchUsageHolder::updateUsage, clusterSupportsFeature);
+    }
+
+    /**
+     * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent.
+     * This variant does not record search features usage. Most times the variant that accepts a {@link SearchUsageHolder} and records
+     * usage stats into it is the one to use.
+     *
+     * @param searchRequest The SearchRequest object that's representing the request we're parsing which shall receive
+     *                      the parsed info.
+     * @param parser The xContent parser.
+     * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found after the main object.
+     * @param clusterSupportsFeature used to check if certain features are available on this cluster
+     */
+    public SearchSourceBuilder parseXContent(
+        SearchRequest searchRequest,
+        XContentParser parser,
+        boolean checkTrailingTokens,
+        Predicate<NodeFeature> clusterSupportsFeature
+    ) throws IOException {
+        return parseXContent(searchRequest, parser, checkTrailingTokens, s -> {}, clusterSupportsFeature);
     }
 
     /**
@@ -1373,10 +1398,11 @@ public SearchSourceBuilder parseXContent(
         boolean checkTrailingTokens,
         Predicate<NodeFeature> clusterSupportsFeature
     ) throws IOException {
-        return parseXContent(parser, checkTrailingTokens, s -> {}, clusterSupportsFeature);
+        return parseXContent(null, parser, checkTrailingTokens, s -> {}, clusterSupportsFeature);
     }
 
     private SearchSourceBuilder parseXContent(
+        SearchRequest searchRequest,
         XContentParser parser,
         boolean checkTrailingTokens,
         Consumer<SearchUsage> searchUsageConsumer,
@@ -1399,7 +1425,11 @@ private SearchSourceBuilder parseXContent(
                 currentFieldName = parser.currentName();
             } else if (token.isValue()) {
                 if (PROJECT_ROUTING.match(currentFieldName, parser.getDeprecationHandler())) {
-                    projectRouting(parser.text());
+                    /*
+                     * If project_routing was specified as a query parameter too, setProjectRouting() will throw
+                     * an error to prevent setting twice or overwriting previously set value.
+                     */
+                    searchRequest.setProjectRouting(parser.text());
                 } else if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
                     from(parser.intValue());
                 } else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {