On Fri, 14 Aug 2026 18:19:46 GMT, Naoto Sato <[email protected]> wrote:
>> This PR implements [JEP 540: Simple JSON API >> (Incubator)](https://openjdk.org/jeps/540). >> >> It adds the `jdk.incubator.json` module which provides APIs for reading and >> writing JSON documents as specified by [RFC >> 8259](https://datatracker.ietf.org/doc/html/rfc8259). This is an incubating >> API. >> >> API documentation: >> https://cr.openjdk.org/~naoto/json/javadoc/api/jdk.incubator.json/module-summary.html >> Co-authored-by: Justin Lu >> ([@justin-curtis-lu](https://github.com/justin-curtis-lu)) >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 726 commits: > > - Merge remote-tracking branch 'jdk-sandbox/json' into > JDK-8381976-Implementation-for-Simple-JSON-API > - Address comment regarding asDouble wording in class spec > - Reflects Alan's comments > - Merge branch 'master' into JDK-8381976-Implementation-for-Simple-JSON-API > - Merge remote-tracking branch 'jdk-sandbox/json' into > JDK-8381976-Implementation-for-Simple-JSON-API > - Removed unused imports > - Removed unnecessary qualifiers > - Merge remote-tracking branch 'jdk-sandbox/json' into > JDK-8381976-Implementation-for-Simple-JSON-API > - Merge branch 'toDisplayString' into json > - wording > - ... and 716 more: https://git.openjdk.org/jdk/compare/16bf2730...76272098 The API of this looks quite nice; minimal but pretty useful! Thanks for all the work and time you have put into this! I still remember when you had published your first draft a few months ago. I am not an OpenJDK member, but hopefully these comments are useful nonetheless. Feel free to consider them only as suggestions. (I only had a look at the non-test sources though.) Note also that because I am not a member, I cannot mark my own comments as resolved (if I recall correctly), so feel free to resolve them in case you have considered them or don't think they are relevant. src/jdk.incubator.json/share/classes/jdk/incubator/json/Json.java line 102: > 100: Objects.requireNonNull(in); > 101: // Defensive copy on input. Ensure source is immutable. > 102: return new JsonParser(Arrays.copyOf(in, in.length)).parseRoot(); Can simplify this Suggestion: return new JsonParser(in.clone()).parseRoot(); src/jdk.incubator.json/share/classes/jdk/incubator/json/Json.java line 146: > 144: var prefix = indent.repeat(depth); > 145: if (isField) { > 146: s.append(" "); Here and below use `append(char)` where possible? src/jdk.incubator.json/share/classes/jdk/incubator/json/Json.java line 163: > 161: s.append(",\n"); > 162: }); > 163: s.setLength(s.length() - 2); // trim final comma For consistency with comment in `toDisplayString(JsonArray, ...)` Suggestion: s.setLength(s.length() - 2); // trim final comma/newline src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonArray.java line 56: > 54: * @throws JsonValueException if the given index is out of bounds > 55: */ > 56: default JsonValue get(int index) { Missing `@Override`? src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonNumber.java line 39: > 37: * <a href="https://datatracker.ietf.org/doc/html/rfc8259#section-6";> > 38: * syntax</a>. > 39: * Alternatively, {@link #of(int)}, {@link #of(long)}, {@link > #of(double)}, For consistency with the Javadoc of the other interfaces, have a separate paragraph for this? Suggestion: * <p> Alternatively, {@link #of(int)}, {@link #of(long)}, {@link #of(double)}, src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonNumber.java line 131: > 129: } > 130: var str = Double.toString(num); > 131: return new JsonNumberImpl(str.toCharArray(), 0, str.length(), > str.indexOf('.'), str.indexOf('E')); Will using the string representation here and treating it as `doc` lead to confusing error messages? For example `JsonNumber.of(1.5).asLong()` will report a confusing JSON document location? src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonNumber.java line 142: > 140: * @return a {@code JsonNumber} created from the {@code int} value > 141: */ > 142: static JsonNumber of(int num) { For these methods, would it make sense to have `JsonNumberImpl` constructors taking the `long` (including `int` cast to `long`) and `double` value, to avoid having to redundantly reparse them? src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonObject.java line 39: > 37: * The interface that represents JSON object. > 38: * <p> > 39: * A {@code JsonObject} can be produced by a {@link Json#parse(String)}. Suggestion: * A {@code JsonObject} can be produced by {@link Json#parse(String)}. src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonString.java line 34: > 32: > 33: /** > 34: * The interface that represents a JSON string. For consistency with the Javadoc of all the other interfaces: Suggestion: * The interface that represents JSON string. (respectively should the other ones without canonical value (`true`, `false` and `null`) have an article in the first sentence of their Javadoc?) src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonString.java line 36: > 34: * The interface that represents a JSON string. > 35: * <p> > 36: * A {@code JsonString} can be produced by a {@link Json#parse(String)}. Suggestion: * A {@code JsonString} can be produced by {@link Json#parse(String)}. src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonString.java line 65: > 63: * @throws NullPointerException if {@code src} is {@code null} > 64: */ > 65: static JsonString of(String src) { To avoid any confusion, should `of(String)` explicitly mention that it expects the unescaped value, without enclosing `"`? src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonString.java line 67: > 65: static JsonString of(String src) { > 66: var escaped = '"' + Utils.escape(Objects.requireNonNull(src)) + > '"'; > 67: return new JsonStringImpl(escaped.toCharArray(), 0, > escaped.length(), Similar to the comment for `JsonNumber#of`; could this lead to misleading exception messages because this sets `doc`, and then appears as path in the exception message? E.g. `JsonString.of("a").asDouble()` src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonString.java line 69: > 67: return new JsonStringImpl(escaped.toCharArray(), 0, > escaped.length(), > 68: escaped.length() != src.length() + 2); > 69: } Would it make sense to optimize this, e.g. by having a `JsonStringImpl` constructor which takes the unescaped value? Current behavior might not be ideal that `JsonString.of(...).asString()` can involve escaping and unescaping. src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonValue.java line 184: > 182: * {@return the {@code boolean} value represented by this {@code > JsonValue} if > 183: * it is an instance of {@link JsonBoolean}; otherwise, throws a > 184: * {@code JsonValueException}}. Suggestion: * {@code JsonValueException}} src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonValue.java line 269: > 267: * {@return the {@code String} value represented by this {@code > JsonValue} if > 268: * it is an instance of {@link JsonString}; otherwise, throws a > 269: * {@code JsonValueException}}. Results in duplicate period Suggestion: * {@code JsonValueException}} src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonValue.java line 288: > 286: * {@return an unmodifiable list of the {@code JsonValue}s if this > 287: * {@code JsonValue} is an instance of {@link JsonArray}; otherwise, > throws a > 288: * {@code JsonValueException}}. Suggestion: * {@code JsonValueException}} src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonValue.java line 304: > 302: * {@return an unmodifiable map of {@code String} to {@code > JsonValue} if this > 303: * {@code JsonValue} is an instance of {@link JsonObject}; > otherwise, throws a > 304: * {@code JsonValueException}}. Suggestion: * {@code JsonValueException}} src/jdk.incubator.json/share/classes/jdk/incubator/json/JsonValue.java line 388: > 386: } catch (IndexOutOfBoundsException _) { > 387: throw Utils.composeError(this, > 388: "JsonArray index %d out of bounds for length %d." Specify `Locale.ENGLISH` for `%d`? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonArrayImpl.java line 67: > 65: throw Utils.composeError(this, > 66: "JsonArray index %d out of bounds for length %d." > 67: .formatted(index, theValues.size())); Should this specify `Locale.ENGLISH`? Otherwise depending on the OS default locale you end up with an English exception message containing non-English numerals, which might make troubleshooting a bit cumbersome. (Not sure if other parts of the JDK code account for this though.) src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonArrayImpl.java line 91: > 89: s.setLength(s.length() - 1); // trim final comma > 90: } > 91: return s.append("]").toString(); Could use `append(char)` here Suggestion: s.append(v.toString()).append(','); } if (!list.isEmpty()) { s.setLength(s.length() - 1); // trim final comma } return s.append(']').toString(); src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonBooleanImpl.java line 2: > 1: /* > 2: * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. 2026 missing? Same also for `JsonNullImpl.java` and `JsonNull.java` src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonObjectImpl.java line 101: > 99: s.setLength(s.length() - 1); // trim final comma > 100: } > 101: return s.append("}").toString(); Use `append(char)`? Suggestion: s.append('"').append(Utils.escape(kv.getKey())).append("":") .append(kv.getValue().toString()) .append(','); } if (!map.isEmpty()) { s.setLength(s.length() - 1); // trim final comma } return s.append('}').toString(); src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 99: > 97: if (hasInput()) { > 98: switch (doc[offset]) { > 99: case ']', '}', ',', ' ', '\t','\r', '\n' -> {} Suggestion: case ']', '}', ',', ' ', '\t', '\r', '\n' -> {} src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 137: > 135: if (members.putIfAbsent(name, parseValue()) != null) { > 136: throw failure(nameStart, nameLine, nameLineStart, > 137: "The duplicate member name: \"%s\" was already > parsed".formatted(name), startO, true); Message sounds maybe a bit weird, should this either omit the leading "The ..." or omit the colon ":"? - "Duplicate member name: ..." - "The duplicate member name ..." src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 193: > 191: escape = true; > 192: continue; > 193: } else if (c == '\"') { Redundant escape Suggestion: } else if (c == '"') { src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 263: > 261: hasEscape = true; > 262: escape = true; > 263: } else if (c == '\"') { Redundant escape Suggestion: } else if (c == '"') { src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 419: > 417: private boolean notWhitespace() { > 418: return switch (doc[offset]) { > 419: case ' ', '\t','\r' -> false; Suggestion: case ' ', '\t', '\r' -> false; src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/JsonParser.java line 459: > 457: var path = Utils.getParsingPath(head, doc, structural); > 458: return new JsonParseException("%s.%s Location: line %d, position > %d." > 459: .formatted(message, path, l, pos), l, pos); Similar to other comments; use `Locale.ENGLISH` for `%d`? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 64: > 62: if (sb == null) { > 63: sb = new StringBuilder().append(str, 0, i); > 64: } Reduce code duplication by performing `sb.append('\')` here once? All the `if` branches below duplicate this currently. src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 69: > 67: sb.append('\\').append(c); > 68: } else if (c == '"') { > 69: sb.append('\\').append(c); Combine these into a single `if`? Suggestion: if (c == '\' || c == '"') { sb.append('\').append(c); src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 138: > 136: // Structural parsing cases > 137: if (doc[offset] == '[') { > 138: sb.append( '['); Suggestion: sb.append('['); src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 140: > 138: sb.append( '['); > 139: } > 140: if (doc[offset] == '{') { Can be `else if` Suggestion: else if (doc[offset] == '{') { src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 156: > 154: // After path is produced, line and pos should be value > bearing > 155: return " Path: \"%s\". Location: line %d, position > %d.".formatted( > 156: path, jp.line, jp.pos); Same as other comment; explicitly specify `Locale.ENGLISH` to avoid `%d` being OS default locale dependent? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 190: > 188: default -> throw new InternalError(); > 189: }; > 190: toPath(offset, sb); Instead of implementing this recursively, could this instead be implemented iteratively by wrapping the whole body of `toPath` inside a `while (offset > 0) { ... }`? src/jdk.incubator.json/share/classes/jdk/incubator/json/impl/Utils.java line 197: > 195: while (offset >= 0) { > 196: var ws = switch (doc[offset]) { > 197: case ' ', '\t','\r' -> true; Suggestion: case ' ', '\t', '\r' -> true; src/jdk.incubator.json/share/classes/jdk/incubator/json/package-info.java line 30: > 28: * generating JSON text. This package implements > 29: * <a href="https://datatracker.ietf.org/doc/html/rfc8259";>RFC 8259: The > JavaScript > 30: * Object Notation (JSON) Data Interchange Format</a> Missing period? Suggestion: * Object Notation (JSON) Data Interchange Format</a>. src/jdk.incubator.json/share/classes/jdk/incubator/json/package-info.java line 32: > 30: * Object Notation (JSON) Data Interchange Format</a> > 31: * > 32: * <h2><a>Parsing JSON documents</a></h2> Here and below, why these `<h2><a>`? Isn't Javadoc creating anchors for headings on its own? (or what is the intention here?) ------------- PR Review: https://git.openjdk.org/jdk/pull/32282#pullrequestreview-4947182226 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792878114 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792884186 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792889223 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792908451 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792912918 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792942494 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792936096 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792948219 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3793007828 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792957334 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792967342 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792972908 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792804942 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792997269 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792996337 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792996607 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792996855 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3793014420 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792685376 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792691000 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792924068 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792733866 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792816761 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792838796 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792847493 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792847868 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792865101 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792868658 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792748357 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792745214 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792758396 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792760232 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792762575 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792775207 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3792765185 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3793018581 PR Review Comment: https://git.openjdk.org/jdk/pull/32282#discussion_r3793021990
