Add Path 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 path 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.

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).

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.

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.

…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.

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

Add Versioning to REST Tests

4e77b90

DaveCTurner reviewed Jun 16, 2025

View reviewed changes

Add Path Parameters to Cluster Allocation Explain API #129342

Are you sure you want to change the base?

Add Path Parameters to Cluster Allocation Explain API #129342

Uh oh!

Conversation

joshua-adams-1 commented Jun 12, 2025

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!

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!

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!

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!

Uh oh!

joshua-adams-1 Jun 13, 2025 •

edited

Loading

joshua-adams-1 Jun 13, 2025 •

edited

Loading