feat: sync API with latest upstream java.util.json#115
feat: sync API with latest upstream java.util.json#115
Conversation
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 brings the backported JSON API up-to-date with the latest changes from the OpenJDK sandbox 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
|
![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: 1ffcc80a29
ℹ️ 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 str = Double.toString(num); | ||
| return new JsonNumberImpl(str.toCharArray(), 0, str.length(), 0, 0); |
There was a problem hiding this comment.
Here JsonNumber.of(double) always passes (dec=0, exp=0) to JsonNumberImpl, which signals “exponent present” to initNumLong(). That method then tries to parse an exponent substring starting at index 1, so values like JsonNumber.of(1.0) or JsonNumber.of(1e3) hit NumberFormatException and toLong() throws JsonAssertionException even though the numeric value is an integer. This breaks the contract that whole-number doubles can be retrieved as long. You need to compute actual ./e offsets from str (or use -1 when absent) before constructing JsonNumberImpl.
Useful? React with 👍 / 👎.
![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 does an excellent job of synchronizing the API with the latest upstream changes from the OpenJDK sandbox. The updates are comprehensive, covering method renames, API removals, and implementation refactoring across the core library, documentation, and tests. The changes are consistent and correctly applied. I've found one critical issue in the JsonParser's duplicate key detection logic that could lead to incorrect parsing behavior, which I've detailed in a specific comment. Besides that, the quality of this large-scale refactoring is very high.
| if (members.putIfAbsent(name, parseValue()) != null) { | ||
| throw failure(nameOffset, "The duplicate member name: \"%s\" was already parsed".formatted(name)); | ||
| } |
There was a problem hiding this comment.
The current logic for detecting duplicate keys has a subtle but critical bug. parseValue() is called within members.putIfAbsent(), which means the value is parsed even if the key is a duplicate. This incorrectly advances the parser's offset, and if there are more members after the duplicate, it will likely lead to a subsequent syntax error because the parser is in the wrong position.
The check for a duplicate key should happen before parsing the value to avoid this side effect. The previous implementation was safer in this regard.
| if (members.putIfAbsent(name, parseValue()) != null) { | |
| throw failure(nameOffset, "The duplicate member name: \"%s\" was already parsed".formatted(name)); | |
| } | |
| if (members.containsKey(name)) { | |
| throw failure(nameOffset, "The duplicate member name: \"%s\" was already parsed".formatted(name)); | |
| } | |
| members.put(name, parseValue()); |
- Update upstream reference to 4de0eb4f0c867df2d420501cf6741e50dee142d9 (2024-10-10) - Fix API examples: JsonNumber uses toNumber().intValue() not intValue() - Update conformance to 99.3% - Update CI section to reflect auto-issue creation for API drift - Clarify security section about recursive parser - Fix typos throughout - Add Eclipse project files to .gitignore
Response to AI Code Review CommentsRe: Gemini's concern about
|
1 participant
Summary
Synchronized the backport API with the latest changes from the OpenJDK sandbox
jsonbranch. This sync addresses the drift reported by the daily API tracker.Key Changes
values()toelements()inJsonArray.value()tobool()inJsonBooleanandstring()inJsonString.toNumber()andtoBigDecimal()withtoLong()andtoDouble()inJsonNumber.Json.fromUntyped()andJson.toUntyped()as they are no longer in the upstream API.JsonParseExceptionto usegetErrorLine()andgetErrorPosition().JsonParser,Utils, and allJson*Implclasses to match upstream logic.Math.powExactfor Java 21 compatibility._) with named alternatives.StableValuebackport logic.README.mdandindex.htmlto reflect the new API.json-java21,json-java21-jtd, andjson-java21-api-tracker.JsonTypedUntypedTests.javawhich relied on the removed untyped conversion methods.Verified with
mvn clean compile testpassing across all modules.