Skip to content

Remove '| string' in query parameters#5312

Merged
pquentin merged 2 commits intomainfrom
query-parameter-string-unions
Sep 19, 2025
Merged

Remove '| string' in query parameters#5312
pquentin merged 2 commits intomainfrom
query-parameter-string-unions

Conversation

@pquentin
Copy link
Copy Markdown
Member

@pquentin pquentin commented Sep 18, 2025

The | string isn't meaningful in those two cases and this helps #5215. I'm skipping backports since this is a breaking change, though.

@pquentin pquentin requested review from a team as code owners September 18, 2025 12:03
@pquentin pquentin added the skip-backport This pull request should not be backported label Sep 18, 2025
pquentin
pquentin commented Sep 18, 2025
View reviewed changes
specification/ml/get_overall_buckets/MlGetOverallBucketsRequest.ts
* Refer to the description for the `overall_score` query parameter.
*/
overall_score?: double | string
overall_score?: double
Copy link
Copy Markdown
Member Author

@pquentin pquentin Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this one isn't a query parameter, it feels like a straight-up bug.

Parsing is done in https://github.com/elastic/elasticsearch/blob/eb3bc2f2929209461802884ea01f77d3b3fcf96a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/GetOverallBucketsAction.java#L73

@pquentin pquentin requested a review from l-trotta September 18, 2025 12:04
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 18, 2025 •
edited
Loading

Following you can find the validation changes against the target branch for the API.

API Status Request Response
cat.nodes 🟢 → 🔴 11/11 → 10/11 11/11

You can validate this API yourself by using the make validate target.

@pquentin pquentin mentioned this pull request Sep 18, 2025
Merged
@pquentin
Copy link
Copy Markdown
Member Author

pquentin commented Sep 18, 2025 •
edited
Loading

At least I have the reason why this was done: YAML tests!

davidkyle
davidkyle approved these changes Sep 18, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ML change LGTM

@pquentin
Copy link
Copy Markdown
Member Author

pquentin commented Sep 19, 2025

Merging! There's one failure left on cat.nodes, but it will be fixed with the next clients-flight-recorder run.

@pquentin pquentin merged commit 2c0d3a6 into main Sep 19, 2025
9 checks passed
@pquentin pquentin deleted the query-parameter-string-unions branch September 19, 2025 13:58
@pquentin pquentin mentioned this pull request Oct 30, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidkyle davidkyle davidkyle approved these changes

@l-trotta l-trotta l-trotta approved these changes

Assignees

No one assigned

Labels

skip-backport This pull request should not be backported specification

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pquentin @davidkyle @l-trotta