core: support Integer and Long in service configuration parsing#12827
Open
kannanjgithub wants to merge 2 commits into
Open
core: support Integer and Long in service configuration parsing#12827kannanjgithub wants to merge 2 commits into
kannanjgithub wants to merge 2 commits into
Conversation
kannanjgithub
force-pushed
the
service-config-parser-integer
branch
from
635be65 to
b08fc78
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make gRPC-Java’s service config handling tolerant of numeric values represented as Integer/Long (common with JSON parsers like Jackson), instead of requiring Double everywhere.
Changes:
- Allow
ManagedChannelImplBuilder.defaultServiceConfig()to acceptNumbervalues (not justDouble). - Update
JsonUtilnumeric accessors to acceptNumberand convert appropriately. - Expand
ManagedChannelImplBuilderTestto cover integer/long numeric values in service configs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java | Adds tests asserting defaultServiceConfig() accepts Integer/Long (including in lists). |
| core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java | Broadens accepted numeric types in service config maps/lists from Double to Number. |
| core/src/main/java/io/grpc/internal/JsonUtil.java | Accepts Number for numeric getters and performs conversions/integrality validation. |
Comments suppressed due to low confidence (1)
core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java:589
ManagedChannelImplBuildernow accepts anyNumberin service configs, but some downstream parsing still assumes numeric values areDouble(e.g.,ServiceConfigUtil#getStatusCodesFromListchecksstatus instanceof Double). This means configs produced by Jackson that useInteger/Longcan still fail later during service config parsing (not just atdefaultServiceConfig()time). Consider updating the remaininginstanceof Doubleservice-config parsing sites to acceptNumber, or narrowing the accepted numeric types here to match what the parsers support.
parsedMap.put(key, checkListEntryTypes((List<?>) value));
} else if (value instanceof String) {
parsedMap.put(key, value);
} else if (value instanceof Number) {
parsedMap.put(key, value);
} else if (value instanceof Boolean) {
parsedMap.put(key, value);
} else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+194
to
200
| if (value instanceof Number) { | ||
| Number n = (Number) value; | ||
| double d = n.doubleValue(); | ||
| long l = n.longValue(); | ||
| if (l != d) { | ||
| throw new ClassCastException("Number expected to be long: " + d); | ||
| throw new ClassCastException("Number expected to be long: " + n); | ||
| } |
Previously, ManagedChannelImplBuilder and JsonUtil only supported Double or String for numeric values in service configurations. This caused IllegalArgumentException when using service configurations produced by JSON parsers like Jackson that represent non-decimal numbers as Integer or Long. This change: - Updates ManagedChannelImplBuilder to accept any Number type. - Enhances JsonUtil to robustly handle numeric conversions while maintaining necessary integral checks. - Adds ServiceConfigNumericTypeTest to verify support for Integer, Double, and Long types. Fixes grpc#12815
kannanjgithub
force-pushed
the
service-config-parser-integer
branch
from
b08fc78 to
cbf05d2
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, ManagedChannelImplBuilder and JsonUtil only supported Double or String for numeric values in service configurations. This caused IllegalArgumentException when using service configurations produced by JSON parsers like Jackson that represent non-decimal numbers as Integer or Long.
This change:
Fixes #12815