Skip to content

Add @UpdateForV10 to AbstractObjectParser methods #130738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 11, 2025

Conversation

joshua-adams-1
Copy link
Contributor

@joshua-adams-1 joshua-adams-1 commented Jul 7, 2025

Adds an @UpdateForV10 annotation to the following methods from the AbstractObjectParser class:

  • declareInt
  • declareIntOrNull
  • declareLong
  • declareLongOrNull
  • declareIntArray
  • declareLongArray

and assigns it to the Core Infra team. The required change is explained in more detail in #130797. These current parsing methods do not throw exceptions when non-integer, or non-long, values, such as floats, doubles and stringified floats and doubles, are passed. The current behaviour is to truncate the floats / doubles into ints / longs. This is problematic for two reasons:

  1. Customers may depend on this implicit behaviour which is not officially supported
  2. Parameters passed via query parameters have stricter validation on floats, and there is a discrepancy between which values are supported via each method

The proposed solution will be to throw IllegalArgumentException's whenever non-integer and non-long values are passed.

Adds an `@UpdateForV10` annotation to the `AbstractObjectParser
.declareInt` method and assigns it to the Core Infra team.
 This method needs to throw an `IllegalArgumentException` for
 non-integer values being passed, such as floats, doubles, and
 stringified floats and doubles.
@joshua-adams-1 joshua-adams-1 force-pushed the declare-int-update-v10 branch from 2bfb7f6 to 5bacefc Compare July 8, 2025 08:37
@@ -245,6 +246,7 @@ public void declareLongOrNull(BiConsumer<Value, Long> consumer, long nullValue,
);
}

@UpdateForV10(owner = UpdateForV10.Owner.CORE_INFRA) // https://github.com/elastic/elasticsearch/issues/130797
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link to created issue - #130797

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I guess the same also applies to declareLong and the ...OrNull variants thereof?

Adds an `@UpdateForV10` annotation to the following methods in
`AbstractObjectParser`: `declareInt`, `declareIntOrNull`,
`declareLong`,
 `declareLongOrNull`, `declareIntArray` and `declareLongArray`.

 The required change is explained in https://github
 .com/elastic/issues/130797. The current parsing methods
 do not throw exceptions when floats or doubles are passed in place of
 longs or ints. The current behaviour is to truncate the float /
 double but this can lead to incorrect behaviour, and is also a
 discrepancy versus the stricter validation found when floats are passed
  as query parameters.
@joshua-adams-1
Copy link
Contributor Author

@DaveCTurner Good catch! Have expanded the PR accordingly

@joshua-adams-1 joshua-adams-1 changed the title Add @UpdateForV10 to AbstractObjectParser.declareInt Add @UpdateForV10 to AbstractObjectParser methods Jul 8, 2025
@joshua-adams-1 joshua-adams-1 marked this pull request as ready for review July 8, 2025 12:05
@joshua-adams-1 joshua-adams-1 requested a review from a team as a code owner July 8, 2025 12:05
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 8, 2025
@joshua-adams-1 joshua-adams-1 added >non-issue Team:Core/Infra Meta label for core/infra team labels Jul 8, 2025
@elasticsearchmachine elasticsearchmachine removed the Team:Core/Infra Meta label for core/infra team label Jul 8, 2025
@joshua-adams-1 joshua-adams-1 added the Team:Core/Infra Meta label for core/infra team label Jul 8, 2025
@elasticsearchmachine elasticsearchmachine removed the Team:Core/Infra Meta label for core/infra team label Jul 8, 2025
@joshua-adams-1 joshua-adams-1 self-assigned this Jul 9, 2025
@joshua-adams-1 joshua-adams-1 added the Team:Core/Infra Meta label for core/infra team label Jul 9, 2025
@elasticsearchmachine elasticsearchmachine removed the Team:Core/Infra Meta label for core/infra team label Jul 9, 2025
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@joshua-adams-1 joshua-adams-1 added the :Core/Infra/REST API REST infrastructure and utilities label Jul 11, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jul 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jul 11, 2025
@joshua-adams-1 joshua-adams-1 merged commit 932d51c into elastic:main Jul 11, 2025
33 checks passed
@joshua-adams-1 joshua-adams-1 deleted the declare-int-update-v10 branch July 11, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >non-issue Team:Core/Infra Meta label for core/infra team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants