Conversation
…splayString Upstream API changes: - JsonString.value() -> JsonString.string() - JsonNumber.toNumber() -> JsonNumber.toLong()/toDouble() - JsonBoolean.value() -> JsonBoolean.bool() - JsonArray.values() -> JsonArray.elements() - Json.fromUntyped()/toUntyped() removed - New navigation: JsonValue.get(String), element(int), getOrAbsent() Backporting: - Added LazyConstant.java polyfill (upstream switched from StableValue) - Added Utils.powExact() polyfill for Math.powExact(long, int) - Replaced unnamed variables _ with ignored (Java 21 compat) - Updated all modules to use new API Verify: mvnd verify (390 tests pass)
Summary of ChangesHello @simbo1905, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project's JSON API to align with recent developments in the OpenJDK Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
The upstream API removed fromUntyped/toUntyped methods, so we deleted the test file that tested those methods (12 tests removed: 509 -> 497).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request syncs the project with a significant upstream API update. Key changes include renaming accessor methods (e.g., .value() to .string(), .bool()), replacing .toNumber() with .toLong()/.toDouble(), and removing the fromUntyped/toUntyped methods in favor of typed factories. New navigation methods on JsonValue have been added, along with polyfills for LazyConstant and Math.powExact to ensure Java 21 compatibility. My review confirms that most of these changes are correctly implemented across the codebase. However, I've identified a critical bug in JsonNumber.of(double) where it fails to correctly initialize offsets, potentially causing runtime failures. A code suggestion to resolve this is provided. Overall, this is a substantial and well-executed update.
| static JsonNumber of(double num) { | ||
| // non-integral types | ||
| return new JsonNumberImpl(num); | ||
| if (!Double.isFinite(num)) { | ||
| throw new IllegalArgumentException("Not a valid JSON number"); | ||
| } | ||
| var str = Double.toString(num); | ||
| return new JsonNumberImpl(str.toCharArray(), 0, str.length(), 0, 0); | ||
| } |
There was a problem hiding this comment.
The implementation of JsonNumber.of(double) incorrectly hardcodes the decimal and exponent offsets to 0 when creating a JsonNumberImpl. This will cause issues in methods like toLong(), which rely on these offsets to parse the number correctly from its string representation. For example, for a number like 123.45, the decimal offset is not 0, and this will lead to a NumberFormatException or incorrect parsing.
To fix this, we can reuse the JsonNumber.of(String) factory, which correctly parses the number string and sets the offsets via Json.parse().
static JsonNumber of(double num) {
if (!Double.isFinite(num)) {
throw new IllegalArgumentException(![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c47212e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var shiftedFractionPart = new String(doc, decimalOffset + 1, Math.min(exp, exponentOffset - decimalOffset - 1)); | ||
| exp = exp - shiftedFractionPart.length(); | ||
| sig = Long.parseLong(new String(doc, startOffset, decimalOffset - startOffset) + shiftedFractionPart); | ||
| } | ||
| scale = Utils.powExact(10L, exp); |
There was a problem hiding this comment.
The exponent+decimal path always shifts fractional digits into sig before the sign check, which is correct for positive exponents but wrong for negative exponents because the decimal point should move left, not right. With this logic, inputs like "1.0e-1" or "120.0e-1" end up as 10/1200 instead of throwing or returning 0/12, so toLong() can return an incorrect integer for non-integral values. The negative‑exponent branch needs to avoid this right‑shift (or include the fractional length in the divisor) so toLong() only succeeds when the post‑exponent value is actually integral.
Useful? React with 👍 / 👎.
Added 10 tests for LazyConstant.java polyfill covering: - Lazy initialization behavior - Thread safety with concurrent access - Caching/memoization - Exception propagation - Usage in JSON parsing context Test count: 509 -> 507 (removed 12 fromUntyped/toUntyped tests, added 10 LazyConstant tests)
1 participant
Summary
Upstream API Changes
JsonString.value()JsonString.string()JsonNumber.toNumber()JsonNumber.toLong()/toDouble()JsonBoolean.value()JsonBoolean.bool()JsonArray.values()JsonArray.elements()Json.fromUntyped()Json.toUntyped()New APIs
JsonValue.get(String)- navigate to object memberJsonValue.element(int)- navigate to array elementJsonValue.getOrAbsent(String)- safe navigationBackporting
LazyConstant.javapolyfill (upstream switched from StableValue)Utils.powExact()polyfill forMath.powExact(long, int)_withignored(Java 21 compat)Tests
All 390 tests pass (
mvnd verify)