Cluster Allocation Explain API Throws IAE #130389

joshua-adams-1 · 2025-07-01T13:43:19Z

Throws an Illegal Argument Exception when a non-integer value is passed as the shard parameter. The legacy behaviour of allowing strings to be passed, providing they are strings containing integers, is maintained. This is a prerequisite PR to #129342

Throws an Illegal Argument Exception when a non-integer value is passed as the `shard` parameter. The legacy behaviour of allowing strings to be passed, providing they are strings containing integers, is maintained.

Merging test fix into main

pxsalehi

Left a few comments.
Is this ready for a full review? If yes, please mark it as ready and not draft.

pxsalehi · 2025-07-04T12:04:11Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

@@ -31,7 +31,29 @@ public class ClusterAllocationExplainRequest extends MasterNodeRequest<ClusterAl
    private static final ObjectParser<ClusterAllocationExplainRequest, Void> PARSER = new ObjectParser<>("cluster/allocation/explain");
    static {
        PARSER.declareString(ClusterAllocationExplainRequest::setIndex, new ParseField("index"));
-        PARSER.declareInt(ClusterAllocationExplainRequest::setShard, new ParseField("shard"));


not clear to me what the problem with declareInt is. Is this where the lenience is and it would silently accept and convert a float?

Yes, declareInt implicitly allows floats, doubles, and stringified floats and doubles, to be passed where an integer parameter is expected. I have removed that explicit behaviour, and then added test cases

pxsalehi · 2025-07-04T12:06:10Z

.../java/org/elasticsearch/action/admin/cluster/allocation/ClusterAllocationExplainRequest.java

+                String text = xContentParser.text();
+                try {
+                    // Only accept if the string is a valid integer representation
+                    if (text.matches("[-+]?\\d+")) {


why do you need the regex? wouldn't relying on parseInt be enough?

you could also simplify these if/else statements. If you return or throw as the last and only statement of your if, you don't need the else and can just "do the else" after the if statement.

pxsalehi · 2025-07-04T12:09:24Z

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

@@ -47,6 +48,11 @@ public boolean allowSystemIndexAccessByDefault() {
        return true;
    }

+    @Override
+    public Set<String> supportedCapabilities() {
+        return Set.of("shard_parameter_non_integer_validation");


pxsalehi · 2025-07-04T12:12:12Z

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

@@ -84,18 +158,32 @@
  - match: { acknowledged: true }

  - do:
+      catch: /x_content_parse_exception/


I don't think we should expose this exception. It's rather cryptic and internal. I think something like bad request would be more fitting. this could be its error.type. One example I found skimming through the yaml tests is in resources/rest-api-spec/test/cluster.desired_nodes/10_basic.yml.

I don't think it's exposed as this exception. Happy to proven wrong, but from memory I asked about this and it gets converted somewhere to a BAD_REQUEST and this is a 4XX error. I'd be aligned with changing this errors, but this is the current, prod functionality and that has, by definition, been accepted.

Example test from https://github.com/elastic/elasticsearch/pull/129342/files

"cluster shard allocation explanation test with numerical index parameter in the body": - do: indices.create: index: test - match: { acknowledged: true } - do: catch: /x_content_parse_exception/ cluster.allocation_explain: body: { "index": 0, "shard": 0, "primary": true }

pxsalehi · 2025-07-04T12:15:04Z

You should also add the area label for allocation (:Distributed Coordination/Allocation). And the PR type. I think >enhancement would make sense. Since then it would end up in the change log for the release.

joshua-adams-1 · 2025-07-04T14:55:15Z

You should also add the area label for allocation (:Distributed Coordination/Allocation). And the PR type. I think >enhancement would make sense. Since then it would end up in the change log for the release.

Have done!

elasticsearchmachine · 2025-07-04T14:55:25Z

Hi @joshua-adams-1, I've created a changelog YAML for you.

joshua-adams-1 · 2025-07-04T14:55:35Z

Left a few comments. Is this ready for a full review? If yes, please mark it as ready and not draft.

Ahaha forgot to publish it, whoops!

elasticsearchmachine · 2025-07-04T14:56:02Z

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

DaveCTurner

I'm afraid I don't think we should do this, at least not as proposed here. The behaviour of declareInt encodes the way that we parse integer parameters across the entire Elasticsearch API surface. It'd be super-weird to make just this one API be stricter about this parsing.

That's not to say that I think it's a good thing to accept (and truncate) non-integer arguments in our APIs in general, just that I would prefer consistency across the whole API surface. If we're going to fix this, we should fix it everywhere.

Moreover this is a breaking change. We can't justify adopting this change in a minor release since it's not fixing a bug (or at least not a bug which is serious enough to warrant a breaking change in a minor release). Instead, let's mark declareInt with an @UpdateForV10 marker and come back to this lenience when we prepare for the next major release.

joshua-adams-1 · 2025-07-07T15:47:46Z

@DaveCTurner Happy to mark it for V10. Who would be the owner of this change? Us, or core infra?

DaveCTurner · 2025-07-07T15:49:33Z

I think it'd be core/infra really, the parsing code falls under :Core/Infra/REST API

elasticsearchmachine added the v9.2.0 label Jul 1, 2025

joshua-adams-1 added v9.1.0 v9.0.0 v8.19.0 and removed v9.0.0 v8.19.0 v9.1.0 labels Jul 3, 2025

joshua-adams-1 added 2 commits July 3, 2025 12:51

Cluster Allocation Explain API Throws IAE

83d0e25

Throws an Illegal Argument Exception when a non-integer value is passed as the `shard` parameter. The legacy behaviour of allowing strings to be passed, providing they are strings containing integers, is maintained.

Merge branch 'main' into add-iae-when-shard-is-non-integer

2a697ef

Merging test fix into main

joshua-adams-1 force-pushed the add-iae-when-shard-is-non-integer branch from a6a4efe to 2a697ef Compare July 3, 2025 11:53

[CI] Auto commit changes from spotless

4adfa4d

joshua-adams-1 requested review from pxsalehi and DaveCTurner July 3, 2025 13:38

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

Add Path Parameters to Cluster Allocation Explain API #129342

Draft

pxsalehi reviewed Jul 4, 2025

View reviewed changes

joshua-adams-1 added >enhancement :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. labels Jul 4, 2025

Update docs/changelog/130389.yaml

5069f17

joshua-adams-1 marked this pull request as ready for review July 4, 2025 14:55

github-actions bot deployed to docs-preview July 4, 2025 14:56 View deployment

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jul 4, 2025

Merge branch 'main' into add-iae-when-shard-is-non-integer

ef0232b

github-actions bot deployed to docs-preview July 4, 2025 15:07 View deployment

DaveCTurner requested changes Jul 7, 2025

View reviewed changes

Cluster Allocation Explain API Throws IAE #130389

Are you sure you want to change the base?

Cluster Allocation Explain API Throws IAE #130389

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pxsalehi left a comment

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

pxsalehi Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

joshua-adams-1 Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

pxsalehi commented Jul 4, 2025

Uh oh!

joshua-adams-1 commented Jul 4, 2025

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

joshua-adams-1 commented Jul 4, 2025

Uh oh!

elasticsearchmachine commented Jul 4, 2025

Uh oh!

DaveCTurner left a comment • 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!

joshua-adams-1 commented Jul 7, 2025

Uh oh!

DaveCTurner commented Jul 7, 2025

Uh oh!

Uh oh!

joshua-adams-1 commented Jul 1, 2025 •

edited

Loading

DaveCTurner left a comment •

edited

Loading