Support HTTP QUERY method (RFC 10008) on _search (#22409)

[marceloboeira]

Description

Add QUERY as a first-class HTTP method on _search, _msearch routes, and count operations. The method is safe, idempotent, and cacheable per RFC 10008, filling the gap between GET (URI-only) and POST (mutating).

Key changes:

• RestRequest.Method enum now includes QUERY
• RestSearchAction routes register QUERY on _search and /{index}/_search
• RestMsearchAction registers QUERY similarly
• rest-api-spec schema documents QUERY as accepted method
• Unit tests added to RestSearchActionTests and RestControllerTests

Backward compatible: GET and POST _search behavior unchanged. QUERY is purely opt-in for new clients that support RFC 10008.

Related Issues

Integration tests deferred: elasticsearch-java and opensearch-java client libraries must add QUERY to their HttpMethod enums first. Coordination tracked in #22409 and elastic/elasticsearch#153255.

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 5a43f8a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Enum ordering change The Method enum was reordered: CONNECT was previously the last element and is now placed before the newly added QUERY. If any code (including serialization, ordinal() usage, or persisted state) relies on the ordinal of CONNECT, this reordering could introduce subtle bugs. Adding QUERY at the end without moving CONNECT would be safer. Also verify the trailing comma/formatting is intentional as the diff shows CONNECT, followed by QUERY — moving CONNECT changed its ordinal from 8 to 8 (unchanged here) but confirm no code depends on enum ordering. public enum Method { GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT, QUERY } Missing _msearch/_count QUERY routes The PR description states that RestMsearchAction and count operations also register QUERY, but only RestSearchAction changes are visible in this diff. If those other handlers were intended to be updated in this PR, they appear to be missing, which would leave the feature incomplete relative to the described scope. public List<Route> routes() { return unmodifiableList( asList( new Route(GET, "/_search"), new Route(POST, "/_search"), new Route(QUERY, "/_search"), new Route(GET, "/{index}/_search"), new Route(POST, "/{index}/_search"), new Route(QUERY, "/{index}/_search") ) ); } BWC version gate may be incorrect supportedMethodsForVersion filters out QUERY only when esVersion.before(Version.V_3_8_0). If this PR is being merged into a branch where the actual version containing QUERY differs from 3.8.0 (e.g., backported to 2.x or landing in a different minor), the version guard will be wrong and BWC tests could either falsely include QUERY on unsupported nodes or exclude it on supported ones. Confirm V_3_8_0 is the correct first version to include this change. static List<String> supportedMethodsForVersion(String[] methods, Version esVersion) { List<String> supportedMethods = Arrays.asList(methods); if (esVersion.before(Version.V_3_8_0)) { supportedMethods = supportedMethods.stream().filter(method -> "QUERY".equals(method) == false).collect(Collectors.toList()); } return supportedMethods; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 5a43f8a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Return a mutable list to avoid UnsupportedOperationException Arrays.asList returns a fixed-size list backed by the input array. If a caller mutates the returned list (e.g. RandomizingClient shuffling), the current-version branch will fail with UnsupportedOperationException. Wrap the result in a new mutable ArrayList to make the returned list safely mutable and consistent across branches. test/framework/src/main/java/org/opensearch/test/rest/yaml/ClientYamlTestClient.java [118-124] static List<String> supportedMethodsForVersion(String[] methods, Version esVersion) { - List<String> supportedMethods = Arrays.asList(methods); + List<String> supportedMethods = new ArrayList<>(Arrays.asList(methods)); if (esVersion.before(Version.V_3_8_0)) { supportedMethods = supportedMethods.stream().filter(method -> "QUERY".equals(method) == false).collect(Collectors.toList()); } return supportedMethods; } Suggestion importance[1-10]: 4 __ Why: The concern is theoretical - callers may or may not mutate the returned list, and the stream collect(Collectors.toList()) branch already returns a mutable list, so the inconsistency is real but low impact.	Low
	Ensure case-insensitive method matching The switch above uses constants like HttpPost.METHOD_NAME for case labels; using the string literal "QUERY" here is inconsistent and case-sensitive, so a caller passing "query" would fall through to the default. Since the surrounding switch appears to match on the raw method string, ensure the method is normalized (e.g. upper-cased) before the switch, or accept common variants, to avoid surprising UnsupportedOperationException for lower-case input. client/rest/src/main/java/org/opensearch/client/RestClient.java [807-808] +case "QUERY": + return addRequestBody(new HttpUriRequestBase("QUERY", uri), entity); - Suggestion importance[1-10]: 2 __ Why: The existing_code and improved_code are identical, and other cases in the switch also use uppercase method names (HTTP methods are conventionally uppercase), so this suggestion has minimal value.	Low

---

Previous suggestions

Suggestions up to commit 60a4546

Category	Suggestion	Impact
General	Return a mutable list from helper method Arrays.asList returns a fixed-size list backed by the array, and if callers later attempt to mutate supportedMethods (e.g., remove) it will throw UnsupportedOperationException. Wrap in a new ArrayList to ensure the returned list is mutable and consistent regardless of the version branch taken. test/framework/src/main/java/org/opensearch/test/rest/yaml/ClientYamlTestClient.java [118-124] static List<String> supportedMethodsForVersion(String[] methods, Version esVersion) { - List<String> supportedMethods = Arrays.asList(methods); + List<String> supportedMethods = new ArrayList<>(Arrays.asList(methods)); if (esVersion.before(Version.V_3_8_0)) { supportedMethods = supportedMethods.stream().filter(method -> "QUERY".equals(method) == false).collect(Collectors.toList()); } return supportedMethods; } Suggestion importance[1-10]: 2 __ Why: The current implementation works for callers that only read the list; there's no evidence in the PR that callers mutate it. The suggestion is a minor defensive improvement with low impact.	Low
	Avoid duplicating the QUERY string literal The case label uses a hardcoded literal while the other cases use HttpXxx.METHOD_NAME constants; also the constructor argument duplicates the same literal. Extract a constant (or reuse the case label) to avoid drift between the two strings and keep style consistent with surrounding cases. client/rest/src/main/java/org/opensearch/client/RestClient.java [807-808] case "QUERY": - return addRequestBody(new HttpUriRequestBase("QUERY", uri), entity); + final String queryMethod = "QUERY"; + return addRequestBody(new HttpUriRequestBase(queryMethod, uri), entity); Suggestion importance[1-10]: 2 __ Why: Minor stylistic improvement. Introducing a local final String inside a case still duplicates the literal in the case label, so it only marginally reduces drift risk.	Low

Suggestions up to commit 9a5211b

Category	Suggestion	Impact
General	Return a mutable list to callers Arrays.asList returns a fixed-size list backed by the input array. If the caller later tries to mutate the returned list (e.g., in the non-BWC path), it will throw UnsupportedOperationException. Return a new mutable ArrayList to be safe and consistent with the filtered branch. test/framework/src/main/java/org/opensearch/test/rest/yaml/ClientYamlTestClient.java [118-124] static List<String> supportedMethodsForVersion(String[] methods, Version esVersion) { - List<String> supportedMethods = Arrays.asList(methods); + List<String> supportedMethods = new ArrayList<>(Arrays.asList(methods)); if (esVersion.before(Version.V_3_8_0)) { supportedMethods = supportedMethods.stream().filter(method -> "QUERY".equals(method) == false).collect(Collectors.toList()); } return supportedMethods; } Suggestion importance[1-10]: 3 __ Why: The returned list is not mutated by the current caller, and Collectors.toList() in the filtered branch also does not guarantee mutability. The concern is speculative and low-impact.	Low
	Extract QUERY method name into a constant The other cases in this switch use constants (e.g., HttpPost.METHOD_NAME) for method names. Extracting the "QUERY" literal into a private static final String constant would prevent typos and keep the string in sync between the case label and the constructor argument. client/rest/src/main/java/org/opensearch/client/RestClient.java [807-808] -case "QUERY": - return addRequestBody(new HttpUriRequestBase("QUERY", uri), entity); +case QUERY_METHOD_NAME: + return addRequestBody(new HttpUriRequestBase(QUERY_METHOD_NAME, uri), entity); Suggestion importance[1-10]: 3 __ Why: Minor code style/maintainability improvement to avoid duplicated string literals, but has no functional impact.	Low

Suggestions up to commit 1fd1d8b

Category	Suggestion	Impact
General	Avoid per-request allocation on hot path HttpMethod.valueOf("QUERY") allocates a new HttpMethod instance on every request that reaches this branch, which is on the hot path for HTTP request handling. Cache the instance in a static final field (or compare via httpMethod.name().equals("QUERY")) to avoid the repeated allocation and equality overhead. modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java [155-158] // QUERY (RFC 10008) is not a cached Netty constant, so compare by name rather than identity. -if (HttpMethod.valueOf("QUERY").equals(httpMethod)) { +if ("QUERY".equals(httpMethod.name())) { return RestRequest.Method.QUERY; } Suggestion importance[1-10]: 6 __ Why: Valid performance improvement — HttpMethod.valueOf("QUERY") allocates a new object per request on the hot path. Comparing via httpMethod.name() avoids allocation, though the impact is moderate.	Low

Suggestions up to commit fe2927e

Category	Suggestion	Impact
General	Cache non-standard HTTP method constant to avoid repeated allocation HttpMethod.valueOf("QUERY") creates a new HttpMethod object on every request, which is wasteful in a hot path. Cache the constant as a static final field to avoid repeated object allocation and improve performance. modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java [156-158] -if (HttpMethod.valueOf("QUERY").equals(httpMethod)) { +private static final HttpMethod HTTP_QUERY = HttpMethod.valueOf("QUERY"); + +// ... in method(): +if (HTTP_QUERY.equals(httpMethod)) { return RestRequest.Method.QUERY; } Suggestion importance[1-10]: 6 __ Why: Creating HttpMethod.valueOf("QUERY") on every request in a hot path is wasteful. Caching it as a static final field is a valid performance improvement, though the impact depends on request volume.	Low
	Remove duplicate test providing no additional coverage testQueryMethodWithoutBody is functionally identical to testQueryMethodIsAccepted — both send a QUERY request to /_search with no body and assert the same thing. This duplicate test adds no additional coverage and should be removed or differentiated (e.g., by asserting body-less behavior specifically). server/src/test/java/org/opensearch/rest/action/search/RestSearchActionTests.java [314-326] -public void testQueryMethodWithoutBody() throws Exception { - SetOnce<ActionType<?>> capturedActionType = new SetOnce<>(); - try (NodeClient nodeClient = createMockNodeClient(capturedActionType)) { - RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.QUERY) - .withPath("/_search") - .build(); - FakeRestChannel channel = new FakeRestChannel(request, false, 0); +// Remove testQueryMethodWithoutBody as it duplicates testQueryMethodIsAccepted. +// If body-less behavior needs explicit coverage, assert that no body parsing error occurs +// or that the search request has a null/empty source. - new RestSearchAction().handleRequest(request, channel, nodeClient); - - assertThat(capturedActionType.get(), equalTo(SearchAction.INSTANCE)); - } -} - Suggestion importance[1-10]: 4 __ Why: testQueryMethodWithoutBody is indeed functionally identical to testQueryMethodIsAccepted, both sending a QUERY request to /_search with no body and asserting the same outcome. Removing or differentiating it would improve test suite quality, but this is a minor maintainability issue.	Low

Suggestions up to commit 95c9051

Category	Suggestion	Impact
General	Avoid duplicate test or assert body absence This test is a duplicate of testQueryMethodIsAccepted — both send a QUERY to /_search with no body and no params. To meaningfully verify "without body" behavior distinct from the accepted case, also assert on the resulting SearchRequest (e.g., that source is empty/default) or remove the redundant test. server/src/test/java/org/opensearch/rest/action/search/RestSearchActionTests.java [314-326] public void testQueryMethodWithoutBody() throws Exception { SetOnce<ActionType<?>> capturedActionType = new SetOnce<>(); - try (NodeClient nodeClient = createMockNodeClient(capturedActionType)) { + SetOnce<ActionRequest> capturedRequest = new SetOnce<>(); + try (NodeClient nodeClient = createMockNodeClient(capturedActionType, capturedRequest)) { RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.QUERY) .withPath("/_search") .build(); FakeRestChannel channel = new FakeRestChannel(request, false, 0); new RestSearchAction().handleRequest(request, channel, nodeClient); assertThat(capturedActionType.get(), equalTo(SearchAction.INSTANCE)); + SearchRequest sr = (SearchRequest) capturedRequest.get(); + assertNull(sr.source().query()); } } Suggestion importance[1-10]: 4 __ Why: The observation that testQueryMethodWithoutBody duplicates testQueryMethodIsAccepted is accurate, and the suggestion to strengthen the assertion is reasonable. However, the improved code references a createMockNodeClient overload that may not exist, making it a moderate improvement in test quality only.	Low

[github-actions]

❌ Gradle check result for a3e34db: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit e32ad6b

[github-actions]

❌ Gradle check result for e32ad6b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit fa8abe4

[github-actions]

❌ Gradle check result for fa8abe4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit dfb9cd6

[github-actions]

❌ Gradle check result for dfb9cd6: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[msfroh]

@marceloboeira -- It looks like we need to add QUERY as a valid method to some enum:

> Task :rest-api-spec:validateRestSpec FAILED
[validate JSON][ERROR][search.json][$.search.url.paths[0].methods[2]: does not have a value in the enumeration [DELETE, GET, HEAD, POST, PUT]]
[validate JSON][ERROR][search.json][$.search.url.paths[1].methods[2]: does not have a value in the enumeration [DELETE, GET, HEAD, POST, PUT]]

Specifically, I think you need to add QUERY to list of valid methods here:

OpenSearch/rest-api-spec/src/main/resources/schema.json

Line 256 in 920677a

"enum": ["DELETE", "GET", "HEAD", "POST", "PUT"],

[github-actions]

Persistent review updated to latest commit 2492d68

[github-actions]

Persistent review updated to latest commit ae71a80

[github-actions]

❌ Gradle check result for ae71a80: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 95c9051

[github-actions]

❌ Gradle check result for 95c9051: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit fe2927e

[github-actions]

❌ Gradle check result for fe2927e: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 1fd1d8b

[github-actions]

❌ Gradle check result for 1fd1d8b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 9a5211b

[github-actions]

Persistent review updated to latest commit 60a4546

[github-actions]

❌ Gradle check result for 60a4546: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[marceloboeira]

The failure is a flaky test unrelated to this change.

IngestFromKafkaIT > testKafkaIngestion_RewindByTimeStamp FAILED
    org.awaitility.core.ConditionTimeoutException: Condition ... was not fulfilled within 1 minutes.
        at org.opensearch.plugin.kafka.KafkaUtils.createTopic(KafkaUtils.java:56)
        at org.opensearch.plugin.kafka.KafkaIngestionBaseIT.setupKafka(KafkaIngestionBaseIT.java:80)
        at org.opensearch.plugin.kafka.KafkaIngestionBaseIT.setup(KafkaIngestionBaseIT.java:64)

it fails in @Before setup, timing out while creating a Kafka topic against the test fixture (broker readiness), in plugins:ingestion-kafka unrelated to my change, which only touches the REST/search layer (RestRequest, RestSearchAction, the netty transports, client/rest, and the yaml test framework).

This test is already tracked as flaky in #17215 ([AUTOCUT] Gradle Check Flaky Test Report for IngestFromKafkaIT).

[github-actions]

Persistent review updated to latest commit 5a43f8a

[github-actions]

❌ Gradle check result for 5a43f8a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?