Throw JsonSyntaxException instead of raw exceptions in built-in adapters

[yusmer96-maker]

Purpose

Gson.fromJson is documented as @throws JsonSyntaxException if json is not a valid representation for an object of type, but four groups of built-in adapters let a raw NumberFormatException, IllegalArgumentException or NullPointerException escape to the caller instead. Callers that guard with catch (JsonParseException e) — the documented way to handle untrusted input — do not catch these.

The same defect class was already fixed for AtomicLongArray in #3038, and #3047 / #3095 cover AtomicIntegerArray. The four groups below are different types, none of them covered by those changes.

Description

Input	Target type	Result today	Neighbouring type that already complies
["0AA"]	BitSet	NumberFormatException	int[], List<Integer>, AtomicIntegerArray
[1,null]	int[], double[], char[], long[], boolean[], int[][]	IllegalArgumentException: null	BitSet, AtomicLongArray, List<Integer>
"0AA"	double, float, Double, Float	NumberFormatException	byte, short, int, long, BigDecimal, BigInteger
[null]	TreeSet, ArrayDeque, PriorityQueue, EnumSet	NullPointerException	ArrayList, HashSet

In each case the neighbouring target type already behaves correctly, which is what makes the inconsistency visible. No public API is added or changed.

Verified on main (2.14.1-SNAPSHOT) with JDK 21:

• mvn spotless:check — passes.
• mvn -pl gson test — Tests run: 4636, Failures: 0, Errors: 0.
• Reverting only the production changes fails exactly 4 tests, one per group.
• mvn clean verify javadoc:jar succeeds for Gson Parent, Gson, the JPMS / GraalVM Native Image / ProGuard-R8 test modules, Gson Extras and Gson Metrics. It fails only in Gson Protobuf Support, where protobuf-maven-plugin:5.1.7 cannot load its mojo — the same failure occurs on unmodified main in my environment, so it is unrelated to this change.

Checklist

• New code follows the Google Java Style Guide
• If necessary, new public API validates arguments, for example rejects null — n/a, no new public API
• New public API has Javadoc — n/a, no new public API
  • Javadoc uses @since $next-version$ — n/a
• If necessary, new unit tests have been added
  • Assertions in unit tests use Truth, see existing tests
  • No JUnit 3 features are used (such as extending class TestCase)
  • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
• mvn clean verify javadoc:jar passes without errors — see the note above: it passes for every module except Gson Protobuf Support, which fails identically on unmodified main here

[Marcono1234]

Thanks for the pull request!

Regarding the Collection#add null check, maybe something similar would be useful for the MapTypeAdapterFactory as well. Though triggering the NullPointerException there is a bit more contrived.

Consider these examples; both cause a not helpful NullPointerException:

var type = new TypeToken<TreeMap<String, Integer>>() {};
// Case 1: map in array notation
new Gson().fromJson("[[null, 1]]", type);

// Case 2: custom adapter for key type which can return null
var gson = new GsonBuilder()
    .registerTypeAdapter(String.class, new TypeAdapter<String>() {
      @Override
      public String read(JsonReader in) throws IOException {
        in.skipValue();
        return null;
      }

      @Override
      public void write(JsonWriter out, String value) throws IOException {
        throw new AssertionError("not needed");
      }
    })
    .create();
gson.fromJson("{\"arbitrary\": 1}", type);

However, fixing this for MapTypeAdapterFactory might be a bit trickier because Map#containsKey or Map#put might throw the NullPointerException.

---

I am not a direct member of this project; feel free to consider my comments only as suggestions.

[yusmer96-maker]

@Marcono1234 Thanks for the review.

I removed both rationale comments from the FLOAT and DOUBLE adapters — you're right that they explain why the change was made rather than what the code does. I kept the BitSet one, since that explains why nextInt() can throw there at all.

ClassCastException / IllegalArgumentException from Collection#add

I'd lean towards leaving those. A null is rejected by a target type that is otherwise perfectly usable, so the document is at fault. TreeSet<NotComparable>, by contrast, throws on the first element for any input — no JSON document can ever deserialize into it — so that is an unusable target type rather than malformed JSON, and ClassCastException names it more precisely. (TreeSet<Object> with [1,"a"] is the one case that does depend on the document, but that seems a degenerate type rather than one worth designing around.)

There is also a practical reason: the null handling here only wraps collection.add(null), so an NPE from anywhere else still propagates. Catching ClassCastException would mean wrapping the general collection.add(instance) call, which would also swallow one thrown from inside a user's custom TypeAdapter.

On IllegalArgumentException, I couldn't construct a case where a JDK collection throws it from add for a value Gson can produce — happy to look if you have one in mind.