Skip to content

Fix duplicate key detection when first value is null#3006

Merged
eamonnmcmanus merged 2 commits intogoogle:mainfrom
andrewstellman:fix-duplicate-key-null-value
Apr 5, 2026
Merged

Fix duplicate key detection when first value is null#3006
eamonnmcmanus merged 2 commits intogoogle:mainfrom
andrewstellman:fix-duplicate-key-null-value

Conversation

@andrewstellman
Copy link
Copy Markdown
Contributor

Summary

MapTypeAdapterFactory detected duplicate map keys during deserialization by checking the return value of Map.put():

V replaced = map.put(key, value);
if (replaced != null) {
    throw new JsonSyntaxException("duplicate key: " + key);
}

This fails when the first occurrence of a duplicate key maps to null. Map.put() returns the previous value, which is null, so the check silently passes. The duplicate is accepted without throwing.

Example: gson.fromJson("{\"a\":null,\"a\":1}", type) returns {a=1} instead of throwing JsonSyntaxException("duplicate key: a").

Fix

Check Map.containsKey(key) before calling put() instead of relying on the return value of put():

if (map.containsKey(key)) {
    throw new JsonSyntaxException("duplicate key: " + key);
}
map.put(key, value);

Applied to both the object-form path and the array-of-entries path (enableComplexMapKeySerialization).

Tests added

  • MapTest.testMapDeserializationWithDuplicateKeysNullFirstValue — null first value, non-null second
  • MapTest.testMapDeserializationWithDuplicateKeysBothNull — both values null
  • MapAsArrayTypeAdapterTest.testDuplicateKeyWithNullFirstValueArrayForm — array-of-entries form

All 4,589 existing tests continue to pass.

How this was found

This bug was found by an AI-assisted code review generated by quality-playbook, an open-source skill for AI-driven code review. The playbook derived null-handling requirements from Gson's community documentation (GitHub issues #676, #913, #948, #1558), then used those requirements to ground a three-pass code review. The review flagged the replaced != null check as insufficient for null values — a finding that was then manually verified with a failing test.

MapTypeAdapterFactory detected duplicate map keys by checking the
return value of Map.put(): `if (replaced != null)`. This missed
duplicates where the first value was null, because Map.put() returns
null (the previous value) which passed the != null check.

For example, {"a":null,"a":1} was silently accepted as {a=1} instead
of throwing JsonSyntaxException("duplicate key: a").

Fix: check Map.containsKey() before put() instead of relying on the
return value of put(). This correctly detects duplicates regardless
of whether the first value is null.

Fixes both the object form and the array-of-entries form
(enableComplexMapKeySerialization).
@google-cla
Copy link
Copy Markdown

google-cla Bot commented Apr 3, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@eamonnmcmanus
Copy link
Copy Markdown
Member

Thanks! There's a small compatibility risk, in that some client code may have been implicitly relying on the old behaviour. I'm going to run this change past all of Google's internal tests and if there are no problems I think we can assume that the change is OK.

Comment thread gson/src/test/java/com/google/gson/functional/MapTest.java
Copy link
Copy Markdown
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found when running all of Google's internal tests against this change.

@eamonnmcmanus eamonnmcmanus merged commit f4d371d into google:main Apr 5, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants