Skip to content

core: normalize service config number values#12826

Open
codingkiddo wants to merge 1 commit into
grpc:masterfrom
codingkiddo:fix/default-service-config-number-values
Open

core: normalize service config number values#12826
codingkiddo wants to merge 1 commit into
grpc:masterfrom
codingkiddo:fix/default-service-config-number-values

Conversation

@codingkiddo
Copy link
Copy Markdown

@codingkiddo codingkiddo commented May 24, 2026

Fixes #12815

This updates default service config validation to accept numeric values represented as Number, not only Double.

Common JSON parsers may deserialize integer-looking JSON values such as maxAttempts: 4 and backoffMultiplier: 2 as Integer, which previously caused defaultServiceConfig() to fail with IllegalArgumentException.

The values are normalized to Double when copied into the validated service config, preserving the existing internal representation expected by the service config parsing code.

Tests were updated to cover direct and nested integer numeric values.

Tested with:

./gradlew :grpc-core:test --tests io.grpc.internal.ManagedChannelImplBuilderTest \
  -PskipAndroid=true \
  -PskipCodegen=true

Note on existing Javadoc workaround

ManagedChannelBuilder.defaultServiceConfig() currently documents JSON numbers as Double and provides a recursive convertNumbers() helper for parsers that produce Integer or other Number values. This PR centralizes the same Number.doubleValue() normalization inside defaultServiceConfig() so callers do not need to duplicate the recursive conversion for nested maps/lists before passing common JSON parser output to the builder.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 24, 2026

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: codingkiddo / name: Vinod Kumar (6253f6e)

@codingkiddo codingkiddo force-pushed the fix/default-service-config-number-values branch from 6253f6e to 679fdae Compare May 24, 2026 08:26
@kannanjgithub
Copy link
Copy Markdown
Contributor

I didn't notice this and raised #12827. There are a few more cases handled in it, for all Number types.

@codingkiddo
Copy link
Copy Markdown
Author

I didn't notice this and raised #12827. There are a few more cases handled in it, for all Number types.

Thanks for pointing that out. I had focused this PR on the defaultServiceConfig() validation path by normalizing incoming Number values to Double, preserving the existing internal representation.

I see #12827 takes a broader approach by also updating JsonUtil numeric parsing for multiple Number types. I’m happy to either keep this PR as the narrower fix, adjust it based on maintainer preference, or close it if maintainers prefer #12827.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates ManagedChannelImplBuilder.defaultServiceConfig() validation to accept numeric values provided as any Number (e.g., Integer), normalizing them to Double to match the existing internal service-config representation and avoid IllegalArgumentException for integer-looking JSON inputs.

Changes:

  • Accept Number values in default service config maps/lists and normalize them via doubleValue().
  • Update tests to validate that integer numeric values (including nested ones) are accepted and stored as Double.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java Accept Number for service config validation and normalize to Double for downstream parsing.
core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java Update/add tests ensuring integer numeric values are accepted and converted to Double.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java Outdated
Signed-off-by: Vinod Kumar <codingkiddo@gmail.com>
@codingkiddo codingkiddo force-pushed the fix/default-service-config-number-values branch from 679fdae to da797e1 Compare June 5, 2026 05:35
@ejona86
Copy link
Copy Markdown
Member

ejona86 commented Jun 5, 2026

Using Number.doubleValue() is what we want, but I guess we can't validate that we didn't lose precision in the process. That's probably why ManagedChannelBuilder.defaultServiceConfig() provides the code snippet to do this conversion yourself.

It'd be good for someone to address that the code snippet exists and mention why it was too onerous.

@codingkiddo
Copy link
Copy Markdown
Author

Using Number.doubleValue() is what we want, but I guess we can't validate that we didn't lose precision in the process. That's probably why ManagedChannelBuilder.defaultServiceConfig() provides the code snippet to do this conversion yourself.

It'd be good for someone to address that the code snippet exists and mention why it was too onerous.

Thanks, that makes sense.

I noticed the existing defaultServiceConfig() Javadoc already provides a recursive convertNumbers() snippet for JSON parsers that produce Integer or other Number types, and that snippet also normalizes via Number.doubleValue().

My reason for proposing this change is that the workaround is easy to miss and somewhat onerous for callers: every caller using a common JSON parser has to recursively walk and mutate/copy the full service config tree before passing it to defaultServiceConfig(). It is also easy to get wrong for nested maps/lists, which is exactly where service config values such as retry policies commonly live.

This PR keeps the same conversion behavior as the documented snippet and keeps the internal representation as Double; it just centralizes that normalization in the validation/copy step already performed by defaultServiceConfig(). I agree that this has the same precision tradeoff as the documented workaround, but for the intended JSON service config fields this seems consistent with the existing API expectation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Service Configuration containing Integer values abort with IllegalArgumentException

4 participants