Skip to content

Throw type_error.318 when serializing discarded values to binary formats - #5382

Closed
ameliabarnabyhub wants to merge 6 commits into
nlohmann:developfrom
ameliabarnabyhub:fix/discarded-binary-serialization-4714
Closed

ameliabarnabyhub wants to merge 6 commits into
nlohmann:developfrom
ameliabarnabyhub:fix/discarded-binary-serialization-4714

Conversation

@ameliabarnabyhub

Copy link
Copy Markdown
Contributor

Fixes #4714

Discarded JSON values silently produced broken CBOR, MessagePack, and UBJSON output when nested in arrays or objects (element count/header written but payload skipped). Serialization now throws type_error.318 instead, matching the approach discussed in the issue thread.

Adds regression tests for top-level, array, and object cases. Documents the new exception id.

Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
Signed-off-by: ameliabarnabyhub <ameliabarnabyhub@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

oa->write_character(to_char_type(0xDB));
write_number(static_cast<std::uint32_t>(N));
}
else

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks to be left over from #5381


#ifndef INCLUDE_NLOHMANN_JSON_FWD_HPP_
#define INCLUDE_NLOHMANN_JSON_FWD_HPP_
#define INCLUDE_NLOHMANN_JSON_FWD_HPP_

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like you have issues with your amalgamate setup.

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for tackling #4714 — the exception-based fix is the right approach. A few issues to resolve before merge, found by Claude Code:

Build-breaking

  1. calc_bson_element_size doesn't compile under -Werror (include/nlohmann/detail/output/binary_writer.hpp:1264). The discarded/default case calls throw_on_discarded(j) with no return afterward, and throw_on_discarded isn't marked [[noreturn]], so GCC can't prove the non-void function always exits via exception. Confirmed in this PR's own CI: error: control reaches end of non-void function [-Werror=return-type] (ci_test_gcc) — this is why essentially every other job in the run is currently red too.

    • Fix: mark throw_on_discarded [[noreturn]]/JSON_HEDLEY_NO_RETURN, or add an unreachable return after the call.
  2. single_include/nlohmann/json.hpp and tests/src/unit-msgpack.cpp don't match the pinned amalgamate/astyle output — the diff includes large unrelated reformatting instead of a minimal diff mirroring the include/ changes. The check job's own amalgamate+astyle step overwrites these files and then fails (exit 1). Please re-run make amalgamate with the pinned toolchain and re-commit.

Other issues

  1. out_of_range.412 is reused for a second, unrelated condition. It already means "BSON length exceeds int32 max" (documented); this PR repurposes it for "MessagePack size exceeds uint32 max" without updating the docs, which still only describes the BSON case. Please give this its own id or document both meanings.

  2. No to_bson() regression test for discarded values — write_bson_element/calc_bson_element_size now also throw type_error.318 there, but unit-bson.cpp didn't get the same "discarded" test sections CBOR/MessagePack/UBJSON received.

  3. The new MessagePack oversized-length branches (else { assert_msgpack_size(N, &j); }) never write a header before falling through to the payload write — this relies entirely on the throw actually unwinding, otherwise it silently writes an unprefixed payload, the same corruption class this PR fixes for discarded values.


Written by Claude Code on behalf of @niels.

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions!

@github-actions github-actions Bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Sep 19, 2026
@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Sep 23, 2026
@github-actions github-actions Bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Sep 24, 2026
@nlohmann

Copy link
Copy Markdown
Owner

@ameliabarnabyhub Are you willing to continue working on this?

@nlohmann

nlohmann commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thanks a lot for this, @ameliabarnabyhub! Since we didn't hear back, we continued in #5761. The binary writers have changed a lot since this PR, so your fix was redone on current develop with you as the commit author. It now uses type_error.321, because 318 has been taken in the meantime, and also covers BSON.

This comment was written by Claude Code on behalf of @nlohmann.

@nlohmann nlohmann closed this Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation L please rebase Please rebase your branch to origin/develop tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Binary formats invalid encoding for <discarded> values in arrays and objects

3 participants