Skip to content

Restore a duplicate key's prior value when the callback rejects its new value - #5466

Merged
nlohmann merged 2 commits into
fix/ordered-json-diff-orderfrom
fix/parser-callback-duplicate-key
Sep 22, 2026
Merged

nlohmann merged 2 commits into
fix/ordered-json-diff-orderfrom
fix/parser-callback-duplicate-key

Conversation

@nlohmann

@nlohmann nlohmann commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

In json_sax_dom_callback_parser::key(), when the callback accepts a key, the code does:

object_element = &(ref_stack.back()->m_data.m_value.object->operator[](val) = discarded);

For a duplicate key (legal JSON), this immediately overwrites the already-stored member with a discarded placeholder before the new value is even parsed. If the callback then rejects the new value, remove_discarded_value() finds the (still) discarded slot and erases the member entirely — losing the original value, not just skipping the new one.

parser_callbacks.md documents that "the parser will behave as if the discarded value was never read", which for a duplicate key should mean the first value stays in place:

auto drop_two = [](int, json::parse_event_t ev, json& v) {
    return !(ev == json::parse_event_t::value && v == 2);
};
json j = json::parse(R"({"a":1,"a":2})", drop_two);
// before: j.dump() == "{}"
// after:  j.dump() == "{\"a\":1}"

The same problem occurs when the rejected duplicate value is a container, rejected at object_end/array_end.

Fix

Add a small stash (duplicate_key_stash, a vector of slot-pointer/previous-value pairs) that key() populates when it detects the key already exists. The stash entry is dropped (superseded, no restore) once the new value is definitively accepted — for a scalar, in handle_value(); for a container, in end_object()/end_array()'s "kept" branch. It's consumed with a restore instead of an erase wherever a rejected value's placeholder would otherwise be erased — remove_discarded_value() (scalar rejection) and the discard branches of end_object()/end_array() (container rejection). Brand-new (non-duplicate) keys are completely unaffected, since no stash entry ever exists for them — that path is byte-for-byte unchanged.

Note: an separate, currently open PR (#5457) also touches remove_discarded_value() in this same file (an unrelated performance optimization, converting a linear scan to an erase-by-key lookup). Whichever of the two lands second will likely need a small rebase against the other.

Test plan

  • Added tests/src/unit-regression2.cpp coverage for: both reproducers above (scalar and object), an array variant, accepted-duplicate regression guards (last value still wins, scalar and container), a brand-new-key-rejected regression guard, a two-level-nested duplicate key, and a three-occurrence key case.
  • Ran the existing tests/src/unit-class_parser.cpp suite offline (10299 assertions, unaffected).
  • Compiled and ran the two parser_callbacks.md doc examples (reject_duplicate_keys.cpp, parse__string__parser_callback_t.cpp) against the patched header — output byte-identical to the committed expected output.
  • Regenerated single_include/nlohmann/json.hpp via make amalgamate.

Breaking change?

No breaking changes to the public API. This only changes behavior for the specific, previously-buggy case of a duplicate object key whose second occurrence's value is rejected by a parser callback — the member now correctly retains its first value instead of being erased, matching the documented "as if never read" contract. All other parsing/callback behavior (including the well-known "duplicate key keeps last value" rule when the callback accepts the value) is unchanged.

🤖 Generated with Claude Code

@nlohmann
nlohmann changed the base branch from develop to fix/ordered-json-diff-order September 5, 2026 14:53
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch 2 times, most recently from 77390de to 0ec4bdb Compare September 5, 2026 15:06
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 5, 2026
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 9ca179e to 740c847 Compare September 5, 2026 16:43
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 740c847 to 658f9a2 Compare September 5, 2026 20:01
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 658f9a2 to 3d12ee7 Compare September 9, 2026 11:17
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 3d12ee7 to c03f8df Compare September 11, 2026 08:30
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from c03f8df to d4fd004 Compare September 11, 2026 15:56
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from d4fd004 to a9f487f Compare September 14, 2026 07:17
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from a9f487f to a0f2dfc Compare September 14, 2026 19:21
@nlohmann nlohmann added 🚀 ready to merge Ready to merge - just waiting for CI to complete. and removed review needed It would be great if someone could review the proposed changes. labels Sep 15, 2026
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from a0f2dfc to 1ef3677 Compare September 15, 2026 18:46
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 1ef3677 to 65a3f96 Compare September 16, 2026 06:43
…ew value

json_sax_dom_callback_parser::key() unconditionally overwrote the object
slot for a key with a `discarded` placeholder as soon as the key was
accepted by the parser callback. For a duplicate key (legal JSON), this
destroyed the pre-existing value from an earlier occurrence of the same
key before the new value was even parsed. If the new value was then
rejected by the callback, remove_discarded_value() erased the member
entirely instead of leaving the original value in place, contradicting
the documented behavior that a discarded value behaves as if it was
never read.

Add a small stash of (slot pointer, previous value) pairs so that when
key() overwrites an existing member with the discarded placeholder, the
previous value can be restored later if the corresponding value (scalar,
object, or array) is rejected, instead of being erased. The stash entry
is dropped without restoring once the new value is definitively
accepted (in handle_value() for scalars, end_object()/end_array() for
containers), so a duplicate key whose new value is accepted still keeps
the last value as before. Non-duplicate keys are unaffected: rejecting
their value still removes the member entirely, since there is nothing
to restore.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
GCC's libstdc++ std::function move assignment evaluates a noexcept
check that invokes a wrapped callable in an unevaluated context; a
non-noexcept parser_callback_t lambda then trips -Wnoexcept ("noexcept-
expression evaluates to 'false'"), which CI's ci_test_gcc job builds
with -Werror. The pre-existing parser_callback_t test lambdas in this
file already work around this by declaring themselves noexcept; apply
the same fix to the three added lambdas that didn't.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the fix/parser-callback-duplicate-key branch from 65a3f96 to 31b281f Compare September 16, 2026 18:12
@nlohmann
nlohmann merged commit e485441 into develop Sep 22, 2026
165 of 309 checks passed
@nlohmann
nlohmann deleted the fix/parser-callback-duplicate-key branch September 22, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants