Add Query Parameters to Cluster Allocation Explain API #129342

joshua-adams-1 · 2025-06-12T13:43:32Z

The cluster allocation explain API now accepts parameters via the request body, via query parameters passed in the URL, but not via both.

#127028

The cluster allocation explain API now accepts parameters via the request body, via path parameters passed in the URL, but not via both. Issue: elastic#127028

DaveCTurner

Sorry got called away mid-review. I left a couple of comments, will come back to this.

DaveCTurner · 2025-06-12T14:56:17Z

server/src/test/java/org/elasticsearch/rest/RestRequestTests.java

@@ -44,6 +44,7 @@
 import static org.mockito.Mockito.when;

 public class RestRequestTests extends ESTestCase {
+    private static final String PARAMETER_KEY = "PARAMETER_KEY";


This indicates to me that this literal parameter name is important in these tests, but I think that's not the case. Instead, we should use randomIdentifier() (within each test, it won't work in static context) to indicate to readers that the parameter name doesn't matter as long as it's equal in all the places it needs to be.

Updated in 77080cb

DaveCTurner · 2025-06-12T14:59:32Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

+            if (hasAnyParameterBeenPassed(request)) {
+                throw new IllegalArgumentException("Parameters cannot be passed in both the URL and the request body");
+            }


Is this needed? I would expect that if the request contains a parameter which isn't consumed in this method then that'll automatically throw an IAE - see org.elasticsearch.rest.BaseRestHandler#handleRequest, particularly unconsumedParams.

You are correct, but this terminates the flow early before the body is parsed. In the case where parameters are passed alongside a request body and the request body contains an incorrect type, then the user would get the parsing exception thrown first. If they fixed that, then they would get the error about having parameters in the URL. I felt it more logical to be told: 1) you can't have parameters in both the body and the URL so pick one, and then 2) iterate through the errors found in the request body / parameters, whichever route selected. If this goes against conventions found in other APIs then happy to maintain consistency and remove this

I don't think there's a convention for this, at least not a very widely-implemented one. I doubt a user would ever really make these multiple mistakes in practice, and really doubt that they would care about having to solve the problems one-by-one; in contrast we're definitely going to be re-reading this code in future, so this is somewhere that we should prioritise simplicity of the implementation.

Removed in 96e219b

DaveCTurner · 2025-06-12T15:53:33Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.allocation_explain.json

+        "description": "Specifies the index within which we would like to describe the shard"
+      },
+      "shard": {
+        "type": "number",
+        "description": "Specifies the ID of the shard that you would like an explanation for"


Grammar nit: "we" or "you"? Tho other similar docs write this without needing these pronouns, maybe we can rephrase this.

Also (and this is really nitpicking to the max) avoid the dangling preposition: "the ID of the shard that you would like an explanation for" -> "the ID of the shard for which you would like an explanation"

I coped the description for shard verbatim from the existing API docs, here. I made up the description for index so will update it

Updated in 84817db

DaveCTurner · 2025-06-12T15:58:23Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

@@ -5,7 +5,105 @@
      cluster.allocation_explain: {}

 ---
-"cluster shard allocation explanation test":


I'd suggest keeping this test - it's not doing any harm IMO and without it the diff is awful.

I renamed this test to cluster shard allocation explanation test with 3 valid body parameters but I added tests above and below it, copying parts of this test as a template, and git has made a horrible diff of it.

To summarise my changes in this file:

I kept all existing tests, but I may have renamed them to specify exactly what they were testing.

I added extensive testing to the old functionality including:
2.1 Insufficient parameters passed in the body request since index, shard and primary are required despite not being explicitly stated in the API docs.
2.2 Parameters of invalid types passed and throwing errors

I added tests for path parameter support, including:
3.1 Insufficient parameters passed
3.2 Parameters of invalid types passed

For 2.1 and 2.2, if this is tested elsewhere then please point me to it! I felt this an appropriate place to add these tests, and increased test coverage is always good, even if I've added hundreds of line of very similar, repetitive tests. If this can be parameterized somehow, then again I'm happy to do so.

Yeah I see the new tests, they seem very thorough, I just recommend keeping this test as-is (including its name). Clean diffs are valuable when digging back through history looking for bugs. If you feel strongly about the naming then that can happen in a separate PR (maybe to go in first).

I added some pre-requisite tests, #130381, and now the diff is much cleaner

DaveCTurner · 2025-06-12T16:02:20Z

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -478,6 +478,18 @@ public int paramAsInt(String key, int defaultValue) {
        }
    }

+    public Integer paramAsInt(String key, Integer defaultValue) {


Naming nit: paramAsInteger?

Updated in 84817db

DaveCTurner · 2025-06-12T16:05:43Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

+            );
+        }
+
+        // This checks for optionally provided query parameters


This is correct, but it is the common pattern across all REST handlers (without needing comments elsewhere) so this sort of comment may actually cause more confusion: why is it called out here when this is just the common pattern? Is something different going on?

Updated in 84817db

pxsalehi · 2025-06-13T09:14:39Z

@elasticmachine update branch

pxsalehi · 2025-06-13T11:25:24Z

I think your yaml tests are failing on older ES versions where the parameters are not supported. You'd need to mark your yaml tests so they're skipped on older versions. See this doc.

pxsalehi · 2025-06-13T11:32:26Z

last time I did this, it was all version-based. It seems now we don't do that anymore and skipping yaml tests need to use the other options in the doc I linked above. From the looks of it, the capabilities/parameters one is related to what you do. I found one example where this is used that you can use as an example.

…meter-127028

joshua-adams-1 · 2025-06-13T14:15:55Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - is_true: rebalance_explanation
+
+---
+# Is this incorrect behaviour? Should an InvalidArgumentException be returned here?


I'll call attention to this since this is current, production behaviour but I wasn't sure if it was correct

ShardNotFoundException extends ElasticsearchException which according to the ExceptionHelper here, does not return a RestStatus.BAD_REQUEST, which may mean it's not converted to IAE which I would expect. Happy to be corrected

ShardNotFoundException is a ResourceNotFoundException and ResourceNotFoundException#status returns RestStatus#NOT_FOUND which maps onto a 404 Not Found response code. That sounds correct to me.

joshua-adams-1 · 2025-06-13T14:17:13Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - match: { acknowledged: true }
+
+  - do:
+      # Numerical values passed here are implicitly supported by RestRequest parsing all parameters as strings


Calling this out since the API claims to only support strings for this field, but the way we parse the parameters means we can support integer values as strings, such as "1". Worth adding to the API documentation?

This is a convention across the whole API surface, documented here.

To clarify, in this case it was the other way around.
The field is declared as string, but I am able to pass an integer. This is because RestRequest parses all parameters as strings. It seems counter-intuitive to be able to pass integers or floats or doubles when the API is expecting a string. In any of these cases, we don't get an IAE, we get an index_not_found_exception since the API has parsed the numerical value as a string and then searched for an index named accordingly. Either way, this is a separate issue to this PR

joshua-adams-1 · 2025-06-13T14:19:01Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - is_true: rebalance_explanation
+
+---
+# When a float is passed in the body, a ShardNotFoundException is thrown. This behaviour is corrected when passed via a path parameter


Related to a comment above about potential incorrect behaviour when a float is passed. I have changed the behaviour when one is passed as a path parameter since I assumed this to be logical to throw an IAE

I think I'd rather this were consistent. You're right, IllegalArgumentException seems preferable if shard isn't an integer. If that's a change in behaviour for the body-params case, let's fix that up first (in a separate PR) and then there's no concerns here.

PR to fix this is #130389

When I wrote this comment I was under the impression that a float would always result in a ShardNotFoundException. It seems that was wrong - instead AIUI a float argument in the body gets truncated to an integer, and if this integer is a valid shard number then you get a result for that shard rather than a ShardNotFoundException. That means we can't introduce an IllegalArgumentException here as I'd previously suggested.

In contrast, the parsing of query parameters is stricter and will reject a float-formatted argument with an IllegalArgumentException anyway.

Consistency is important, but there is already an inconsistency across the whole ES API that seems irreconcilable (without a breaking change, or significantly more effort than this corner case deserves). I think we have to live with it for now.

I opened a PR #130738 to mark the AbstractObjectParser with a V10 annotation to throw an IllegalArgumentException when, in this case, non-integer values are passed. This change will update the entirety of the ES API. In the meantime, for API requests with a request body, we will continue to support the existing behaviour.

I am also going to keep the stricter query parameter validation since this is consistent with other APIs.

In summary:

Floats / doubles passed in request body -> truncated to integers

Floats / doubles passed in query parameters -> IllegalArgumentException

…meter-127028

DaveCTurner

You've used the phrase "path parameters" in several places (including the PR title) but in fact these are query parameters. See e.g. RFC 3986 §3:

  URI         = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

Could you fix that? Other replies inline.

DaveCTurner · 2025-06-16T08:02:11Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

@@ -5,7 +5,105 @@
      cluster.allocation_explain: {}

 ---
-"cluster shard allocation explanation test":


Yeah I see the new tests, they seem very thorough, I just recommend keeping this test as-is (including its name). Clean diffs are valuable when digging back through history looking for bugs. If you feel strongly about the naming then that can happen in a separate PR (maybe to go in first).

DaveCTurner · 2025-06-16T08:03:58Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - is_true: rebalance_explanation
+
+---
+# Is this incorrect behaviour? Should an InvalidArgumentException be returned here?


ShardNotFoundException is a ResourceNotFoundException and ResourceNotFoundException#status returns RestStatus#NOT_FOUND which maps onto a 404 Not Found response code. That sounds correct to me.

DaveCTurner · 2025-06-16T08:06:12Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - match: { acknowledged: true }
+
+  - do:
+      # Numerical values passed here are implicitly supported by RestRequest parsing all parameters as strings


This is a convention across the whole API surface, documented here.

DaveCTurner · 2025-06-16T08:09:20Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+  - is_true: rebalance_explanation
+
+---
+# When a float is passed in the body, a ShardNotFoundException is thrown. This behaviour is corrected when passed via a path parameter


I think I'd rather this were consistent. You're right, IllegalArgumentException seems preferable if shard isn't an integer. If that's a change in behaviour for the body-params case, let's fix that up first (in a separate PR) and then there's no concerns here.

DaveCTurner · 2025-06-16T08:13:17Z

...ain/java/org/elasticsearch/rest/action/admin/cluster/RestClusterAllocationExplainAction.java

+            if (hasAnyParameterBeenPassed(request)) {
+                throw new IllegalArgumentException("Parameters cannot be passed in both the URL and the request body");
+            }


I don't think there's a convention for this, at least not a very widely-implemented one. I doubt a user would ever really make these multiple mistakes in practice, and really doubt that they would care about having to solve the problems one-by-one; in contrast we're definitely going to be re-reading this code in future, so this is somewhere that we should prioritise simplicity of the implementation.

…meter-127028

joshua-adams-1 · 2025-07-08T14:57:52Z

rest-api-spec/src/main/resources/rest-api-spec/api/cluster.allocation_explain.json

@@ -22,6 +22,22 @@
      ]
    },
    "params":{
+      "index": {
+        "type": "string",
+        "description": "Specifies the name of the index that you would like an explanation for"


Matches this JSON with the API spec - https://github.com/elastic/elasticsearch-specification/blob/main/specification/cluster/allocation_explain/ClusterAllocationExplainRequest.ts#L65

elasticsearchmachine · 2025-07-09T15:50:11Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner

Looks good, just a couple of nits in the tests.

DaveCTurner · 2025-07-10T12:34:45Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+        primary: "truee"
+
+---
+"cluster shard allocation explanation test with numerical current node parameter passed in URL":


This test is kinda misleading - there's nothing invalid about current_node: 1 as the title suggests, the problem is the primary: 0, and we're already testing that elsewhere.

Fixed in e87058d

DaveCTurner · 2025-07-10T12:36:37Z

...i-spec/src/yamlRestTest/resources/rest-api-spec/test/cluster.allocation_explain/10_basic.yml

+        body: { "shard": 0, "primary": true }
+
+---
+"cluster shard allocation explanation test with numerical index parameter passed in URL":


This one's a bit odd too - it's no different from the index: "test2" in the next test case, there's nothing special about the index name 0.

Fixed in e87058d

DaveCTurner · 2025-07-10T12:38:37Z

server/src/test/java/org/elasticsearch/rest/RestRequestTests.java

+        assertEquals(123, value);
+    }
+
+    public void testParamAsIntWithoutIntegerParameter() {


naming nit: "without" suggests the parameter is absent, but in fact it's present, it's just that its value is not numeric (likewise below)

Suggested change

public void testParamAsIntWithoutIntegerParameter() {

public void testParamAsIntWithNonIntegerParameter() {

Fixed in e87058d

DaveCTurner · 2025-07-10T12:39:05Z

server/src/test/java/org/elasticsearch/rest/RestRequestTests.java

+        RestRequest restRequest = contentRestRequest("", singletonMap(parameterKey, "123T"));
+        int defaultValue = randomInt();
+
+        assertThrows(IllegalArgumentException.class, () -> { restRequest.paramAsInt(parameterKey, defaultValue); });


nit: no need for the {}, we can use an expression lambda here (also in other cases below)

Suggested change

assertThrows(IllegalArgumentException.class, () -> { restRequest.paramAsInt(parameterKey, defaultValue); });

assertThrows(IllegalArgumentException.class, () -> restRequest.paramAsInt(parameterKey, defaultValue));

Fixed in e87058d

…meter-127028

Add Path Parameters to Cluster Allocation Explain API

d3a29bb

The cluster allocation explain API now accepts parameters via the request body, via path parameters passed in the URL, but not via both. Issue: elastic#127028

joshua-adams-1 requested review from pxsalehi and DaveCTurner June 12, 2025 13:43

joshua-adams-1 self-assigned this Jun 12, 2025

joshua-adams-1 added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels Jun 12, 2025

DaveCTurner reviewed Jun 12, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

pxsalehi added v9.0.3 auto-backport Automatically create backport pull requests when merged and removed auto-backport Automatically create backport pull requests when merged v9.0.3 labels Jun 13, 2025

joshua-adams-1 added 2 commits June 13, 2025 14:28

Update PARAMETER_KEY in RestRequestTests

77080cb

Merge branch 'main' into feature/allocation-explain-query-string-para…

a8042af

…meter-127028

joshua-adams-1 force-pushed the feature/allocation-explain-query-string-parameter-127028 branch from 47e5479 to a8042af Compare June 13, 2025 13:37

joshua-adams-1 commented Jun 13, 2025

View reviewed changes

joshua-adams-1 added 2 commits June 13, 2025 15:34

David Turner revision 1 comments fixed

84817db

Merge branch 'main' into feature/allocation-explain-query-string-para…

17ac269

…meter-127028

DaveCTurner reviewed Jun 16, 2025

View reviewed changes

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

joshua-adams-1 mentioned this pull request Jul 3, 2025

Cluster Allocation Explain API Throws IAE #130389

Closed

joshua-adams-1 added 4 commits July 8, 2025 11:14

Remove unnecessary parameter check

96e219b

Merge branch 'main' into feature/allocation-explain-query-string-para…

ec073da

…meter-127028

Rename test reason to 9.2.0

6b3d902

Merge branch 'main' into feature/allocation-explain-query-string-para…

b6df9a5

…meter-127028

joshua-adams-1 changed the title ~~Add Path Parameters to Cluster Allocation Explain API~~ Add Query Parameters to Cluster Allocation Explain API Jul 8, 2025

joshua-adams-1 added 8 commits July 8, 2025 11:59

Remove PATH_PARAMETERS constant

f004bf4

Remove comments from 10_basic.yml

78f5f3b

Update path params to query params

842a67e

Merge branch 'main' into feature/allocation-explain-query-string-para…

78f074c

…meter-127028

Add YAML test capabilities

0451f37

Merge branch 'main' into feature/allocation-explain-query-string-para…

a7e8fa6

…meter-127028

Make rest API equal to api spec

a8ca621

Merge branch 'main' into feature/allocation-explain-query-string-para…

bc11bdd

…meter-127028

joshua-adams-1 force-pushed the feature/allocation-explain-query-string-parameter-127028 branch from a8572cb to bc11bdd Compare July 8, 2025 14:54

joshua-adams-1 commented Jul 8, 2025

View reviewed changes

This was referenced Jul 8, 2025

Allocation explain should accept query-string parameters #127028

Open

Add Cluster/Allocation/Explain Query Param Example elastic/elasticsearch-specification#4849

Open

joshua-adams-1 marked this pull request as ready for review July 9, 2025 15:49

joshua-adams-1 requested a review from DaveCTurner July 9, 2025 16:11

DaveCTurner reviewed Jul 10, 2025

View reviewed changes

joshua-adams-1 added 2 commits July 11, 2025 11:55

Fix David Turner comments

e87058d

Merge branch 'main' into feature/allocation-explain-query-string-para…

cd00b33

…meter-127028

joshua-adams-1 force-pushed the feature/allocation-explain-query-string-parameter-127028 branch from 0ce9e43 to cd00b33 Compare July 11, 2025 10:55

[CI] Auto commit changes from spotless

d88c054

joshua-adams-1 requested a review from DaveCTurner July 11, 2025 13:01

	public void testParamAsIntWithoutIntegerParameter() {
	public void testParamAsIntWithNonIntegerParameter() {

	assertThrows(IllegalArgumentException.class, () -> { restRequest.paramAsInt(parameterKey, defaultValue); });
	assertThrows(IllegalArgumentException.class, () -> restRequest.paramAsInt(parameterKey, defaultValue));

Add Query Parameters to Cluster Allocation Explain API #129342

Are you sure you want to change the base?

Add Query Parameters to Cluster Allocation Explain API #129342

Uh oh!

Conversation

joshua-adams-1 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

pxsalehi commented Jun 13, 2025

Uh oh!

pxsalehi commented Jun 13, 2025

Uh oh!

pxsalehi commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

joshua-adams-1 commented Jun 12, 2025 •

edited

Loading

joshua-adams-1 Jun 13, 2025 •

edited

Loading

joshua-adams-1 Jun 13, 2025 •

edited

Loading