Skip to content

Allow SAX parsing of tagged CBOR - #5740

Merged
nlohmann merged 6 commits into
nlohmann:developfrom
Tyagiquamar:fix/sax-cbor-tags
Oct 7, 2026
Merged

nlohmann merged 6 commits into
nlohmann:developfrom
Tyagiquamar:fix/sax-cbor-tags

Conversation

@Tyagiquamar

@Tyagiquamar Tyagiquamar commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #5676.

sax_parse rejects CBOR tags because its public signatures did not pass a tag handler to the binary reader. This also rejects binary values with a subtype written by to_cbor.

Consolidate sax_parse by adding a defaulted const cbor_tag_handler_t tag_handler = cbor_tag_handler_t::error parameter directly to the existing sax_parse signatures (compatible inputs, iterator pairs, and the deprecated span adapter). The parameter passes the handler to the binary reader, allowing callers to choose ignore or store. Regression tests cover all three input forms, each handler mode, and the JSON path.

Validation (Docker, GCC 12, C++11):

  • unit-regression3 with multiple headers: 12 cases, 107 assertions passed.

  • unit-regression3 with the single header: 12 cases, 107 assertions passed.

  • unit-cbor --test-case="Tagged values": 1 case, 209 assertions passed.

  • Regenerated the single header with the repository amalgamation script and formatted with AStyle 3.4.13.

  • Changes are described with the cause and fix.

  • The existing issue is referenced.

  • OSS-Fuzz issue: not applicable.

  • Code coverage remains at 100%: not measured locally; the new branches have regression coverage.

  • Documentation is updated.

  • Ran make amalgamate: ran its amalgamation script and pinned formatter directly.

Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
Comment thread docs/mkdocs/docs/api/basic_json/sax_parse.md Outdated
Resolved conflicts in include/nlohmann/json.hpp and single_include/nlohmann/json.hpp:
kept the PR's new tag-handler sax_parse overloads inside develop's clang
-Wdocumentation-deprecated-sync pragma block, updated their binary_reader::sax_parse
calls to the no-format signature introduced by develop, and regenerated single_include
via make amalgamate.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann

nlohmann commented Oct 1, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR! I merged develop into your branch (857d3ba) to resolve conflicts with #5737 and #5745. Two things changed besides the conflict markers, so please pull before pushing again:

  • On develop, binary_reader now gets the input format when it is constructed, so binary_reader::sax_parse no longer takes a format argument. I updated your three new call sites in include/nlohmann/json.hpp to .sax_parse(sax, strict, tag_handler).
  • Fix lint debt: enum-macro NOLINTs, doctest as SYSTEM, no-op analyzer #5737 wrapped the deprecated span-adapter sax_parse overload in #pragma clang diagnostic ignored "-Wdocumentation-deprecated-sync". Your two new deprecated overloads are now inside that block as well.

single_include is regenerated. The CBOR "Tagged values" tests (209 assertions), unit-regression3 and unit-class_parser pass locally.

(This comment was written by Claude Code.)

Signed-off-by: Tyagiquamar <mohdquamartyagi@gmail.com>
@Tyagiquamar

Copy link
Copy Markdown
Contributor Author

Updated to consolidate the \sax_parse\ functions by adding the default \ ag_handler = cbor_tag_handler_t::error\ parameter directly to the existing signatures instead of having separate overloads. Documentation and \single_include\ have been regenerated, and all test suites pass locally in Docker.

@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 the update. Passing tag_handler as a defaulted parameter is exactly what we wanted. The code and tests look good to me. A few things before we can merge:

  1. Please merge develop into your branch. The current CI failures are not caused by this PR. They come from the state of develop on 10-02:

    • ci_test_gcc: -Wexperimental-fmv-target is not recognized by GCC
    • ci_cpplint: missing <memory> include in ordered_map.hpp
    • ci_test_noexceptions: crash in unit-bon8.cpp

    #5750 and #5754 address these on develop. After merging, please regenerate single_include with make amalgamate if there are conflicts in json.hpp.

  2. Please add a version history entry to docs/mkdocs/docs/api/basic_json/sax_parse.md, for example:

    - Added `tag_handler` in version 3.13.0.

    (next to the existing ignore_trailing_commas line).

  3. Please update the PR description. It still mentions "explicit tag-handler overloads" and says "existing overload signatures and defaults remain unchanged". Neither is true since 5a36060: the existing signatures now take an additional defaulted parameter.

Optional, not blocking: to pass tag_handler, callers have to spell out strict, ignore_comments, and ignore_trailing_commas first. That is consistent with from_cbor, so I'm fine with it as is.

(This comment was written by Claude Code.)

@Tyagiquamar

Copy link
Copy Markdown
Contributor Author

Merged \develop\ into the branch, added the version history entry for \ ag_handler\ in \docs/mkdocs/docs/api/basic_json/sax_parse.md, verified amalgamation and formatting, and updated the PR description.

@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Oct 7, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 7, 2026
Signed-off-by: Niels Lohmann <mail@nlohmann.me>

# Conflicts:
#	include/nlohmann/json.hpp
#	single_include/nlohmann/json.hpp
@nlohmann
nlohmann enabled auto-merge (squash) October 7, 2026 05:38

@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.

Looks good to me.

@nlohmann
nlohmann merged commit 675e519 into nlohmann:develop Oct 7, 2026
5 of 6 checks passed
@nlohmann

nlohmann commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Thanks!

nlohmann added a commit that referenced this pull request Oct 7, 2026
#5740 changed include/nlohmann/json.hpp without regenerating
single_include/nlohmann/json.hpp.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 7, 2026
…functions

The #5676 regression test (#5740) calls the deprecated
sax_parse(span_input_adapter&&, ...), which JSON_DELETE_DEPRECATED_FUNCTIONS
deletes, so ci_test_delete_deprecated_functions failed to build.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
nlohmann added a commit that referenced this pull request Oct 7, 2026
* Fix CI on develop after #5585

- test-diagnostics-optimized: -O3 makes GCC's -Winline and
  -Wsuggest-attribute=pure/const warnings fire with the ci_test_gcc flag
  set; turn them off for this test.
- test-diagnostics-optimized: suppress Clang's -Wexit-time-destructors for
  the static table in to_json.
- Infer: raise pulse-max-disjuncts from 20 to 40. With the default,
  Pulse loses the stored type in basic_json::replace_value() and reports
  false null dereferences of get_ptr() results in unit-pointer_access.cpp.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Ignore Infer's false USE_AFTER_DELETE in ordered_map::erase

Infer's std::string model keeps the buffer of a moved-from string, so the
destroy-and-reconstruct loop in erase(first, last) looks like it destroys a
buffer twice.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Mark throw_on_discarded()'s parameters as used without exceptions

With JSON_NOEXCEPTION, JSON_THROW expands to std::abort(), so Clang's
-Wunused-parameter breaks test-disabled_exceptions (since #5761).

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Skip the span_input_adapter sax_parse checks with deleted deprecated functions

The #5676 regression test (#5740) calls the deprecated
sax_parse(span_input_adapter&&, ...), which JSON_DELETE_DEPRECATED_FUNCTIONS
deletes, so ci_test_delete_deprecated_functions failed to build.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Fall back to the first entry in test-diagnostics-optimized's to_json

clang-tidy (clang-analyzer-security.ArrayBound) flagged it->second for a
value not in the table. Use the same fallback as
NLOHMANN_JSON_SERIALIZE_ENUM; the test still fails with -Werror=array-bounds
on the headers from before #5585.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Fix clang-tidy findings in tests from #5762 and #5774

- unit-regression2.cpp (#5762): const/auto for the destroy() test values;
  NOLINT the intended copy in check_destroy_edge_case().
- unit-serialization.cpp (#5774): build the expected strings with += instead
  of chained operator+ (performance-inefficient-string-concatenation).

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aspect: binary formats BSON, CBOR, MessagePack, UBJSON documentation M 🚀 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.

sax_parse cannot read CBOR that contains tags, including binary values with a subtype written by to_cbor

3 participants