Add support for BSON element 11: uint64 #4380

slowriot · 2024-05-24T12:59:40Z

This PR adds support for decoding and encoding BSON element 11: 64bit unsigned int.

This resolves #3894.

Changes

Decode element 0x11 correctly as uint64_t.
Update existing write_bson_unsigned to encode unsigned 64bit values correctly (using 0x11 instead of signed 0x12).
Update existing tests to expect support for uint64_t.

Reference

https://bsonspec.org/spec.html

Pull request checklist

Changes are described in the pull request, or an existing issue is referenced.
The test suite compiles and runs without error.
Code coverage is 100%. Test cases can be added by editing the test suite.
The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

coveralls · 2024-05-24T14:59:55Z

coverage: 99.984% (-0.02%) from 100.0%
when pulling 7c616a2 on slowriot:bson-element-11-uint64
into 8c391e0 on nlohmann:develop.

nlohmann

Thanks for the patience. The code looks good, but the documentation needs adjustment:

https://github.com/nlohmann/json/blob/develop/docs/mkdocs/docs/home/exceptions.md: remove BSON from the description of json.exception.out_of_range.407
https://github.com/nlohmann/json/blob/develop/docs/mkdocs/docs/features/binary_formats/bson.md: adjust mapping tables

slowriot · 2024-11-07T13:53:50Z

Thanks for the patience. The code looks good, but the documentation needs adjustment:

Thanks for the review. Documentation updated in latest. Please let me know if I've missed anything else.

coveralls · 2024-11-07T14:18:35Z

coverage: 99.624% (-0.02%) from 99.639%
when pulling 36ce984 on slowriot:bson-element-11-uint64
into 2d42229 on nlohmann:develop.

nlohmann · 2024-11-07T14:34:26Z

Coverage went down. Can you please check if a test is missing?

nlohmann · 2024-11-16T16:12:28Z

include/nlohmann/detail/output/binary_writer.hpp

        }
        else
        {
-            JSON_THROW(out_of_range::create(407, concat("integer number ", std::to_string(j.m_data.m_value.number_unsigned), " cannot be represented by BSON as it does not fit int64"), &j));
+            JSON_THROW(out_of_range::create(407, concat("unsigned integer number ", std::to_string(j.m_data.m_value.number_unsigned), " cannot be represented by BSON as it does not fit into uint64"), &j));


If I read the coverage information correctly, then this line is not covered in a test. Please check and add a test.

I think this can only happen if number_unsigned has a larger size than 64 bits. Does the library support that? If not, then this can't be hit.

So far, I failed to compile the library with 128-bit integers. For CBOR, we assume all unsigned integers to fit into 64 bits, so I think it's fair to do the same here. (Any code like the one above could not be tested anyway.)

nlohmann · 2024-11-16T16:13:01Z

(And also, please update to the latest develop branch so that the CI can be green)

nlohmann · 2024-11-16T22:16:14Z

Btw: have you looked at #4356?

# Conflicts: # docs/mkdocs/docs/features/binary_formats/bson.md # docs/mkdocs/docs/home/exceptions.md # include/nlohmann/detail/output/binary_writer.hpp # single_include/nlohmann/json.hpp # tests/src/unit-bson.cpp

github-actions · 2025-01-10T18:47:37Z

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @slowriot
Please read and follow the Contribution Guidelines.

github-actions · 2025-01-10T18:47:39Z

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @slowriot
Please read and follow the Contribution Guidelines.

slowriot · 2025-01-10T18:53:41Z

We recently merged #4590. I think this PR can be closed.

Thanks, just catching up with the changes since I last pushed to this PR - took a moment to resolve the conflicts as #4590 duplicated most of what I did, but it seems there may still have been a few changes in this PR that were missed in the other (unit test for example, and slightly different approach to choosing a format when encoding, documentation grammar, case ordering and a few other minor details).

Please review and let me know if any of it is still of use, and if it can be merged on top or if anything needs to be corrected or removed from this change. Otherwise of course it can be closed.

nlohmann · 2025-01-10T18:57:09Z

Sure, go ahead.

slowriot · 2025-01-10T19:12:23Z

@nlohmann Having rebased, I think the remaining changes in this PR's diffs may still be useful to merge - please have a look. Otherwise, please close.

nlohmann · 2025-01-11T14:51:43Z

@codenut Can you please have a look at the remaining diff?

nlohmann

The remaining changes seem reasonable, but I would also like to get @codenut 's take on this.

Please revert the whitespace changes in the documentation. I think there is only one grammar fix left then.
There is an uncovered branch that can be removed as it would only be triggered with 128-bit integers.
A call to max needs to be put in parentheses to fix the Windows builds.

nlohmann · 2025-01-11T14:52:00Z

include/nlohmann/detail/output/binary_writer.hpp

@@ -1076,7 +1076,7 @@ class binary_writer
    {
        return (value <= static_cast<std::uint64_t>((std::numeric_limits<std::int32_t>::max)()))
               ? sizeof(std::int32_t)
-               : sizeof(std::int64_t);
+               : sizeof(std::uint64_t);


This change seems reasonable.

nlohmann · 2025-01-11T14:56:20Z

include/nlohmann/detail/output/binary_writer.hpp

        }
        else
        {
-            JSON_THROW(out_of_range::create(407, concat("integer number ", std::to_string(j.m_data.m_value.number_unsigned), " cannot be represented by BSON as it does not fit int64"), &j));
+            JSON_THROW(out_of_range::create(407, concat("unsigned integer number ", std::to_string(j.m_data.m_value.number_unsigned), " cannot be represented by BSON as it does not fit into uint64"), &j));


So far, I failed to compile the library with 128-bit integers. For CBOR, we assume all unsigned integers to fit into 64 bits, so I think it's fair to do the same here. (Any code like the one above could not be tested anyway.)

nlohmann · 2025-01-11T15:02:53Z

include/nlohmann/detail/output/binary_writer.hpp

        {
-            write_bson_entry_header(name, 0x12 /* int64 */);
-            write_number<std::int64_t>(static_cast<std::int64_t>(j.m_data.m_value.number_unsigned), true);
+            write_bson_entry_header(name, 0x11 /* uint64 */);


This change seems reasonable.

This change is logical, but I have some concerns about backward compatibility. It may break the following scenario:

void write() { const uint64_t = 9223372036854775807L; json const j = { {"entry", l} }; const std::vector<uint8_t> bson = json::to_bson(j); saveToTable1(bson); saveToTable2(bson); } write(); // was called before the code changes in this PR

void read() { const std::vector<uint8_t> bson1 = loadFromTable1(); json const j = json::from_bson(bson); const std::vector<uint8_t> bson1_roundtrip = json::to_bson(j); const std::vector<uint8_t> bson2 = loadFromTable2(); if (equals(bson1_roundtrip, bson2)) { ... } } read();

With the changes in this PR, the comparison between bson1_roundtrip and bson2 will fail. It is hard to tell if clients rely on this behavior, but I would like to highlight this potential issue.

nlohmann · 2025-01-11T15:03:26Z

tests/src/unit-bson.cpp

@@ -1132,7 +1132,7 @@ TEST_CASE("BSON numerical data")
                    std::vector<std::uint8_t> const expected_bson =
                    {
                        0x14u, 0x00u, 0x00u, 0x00u, // size (little endian)
-                        0x12u, /// entry: int64
+                        0x11u, /// entry: uint64


This is a consequence of the change in write_bson_unsigned above.

include/nlohmann/detail/output/binary_writer.hpp

…ution

slowriot · 2025-01-11T16:28:14Z

The remaining changes seem reasonable, but I would also like to get @codenut 's take on this.

* Please revert the whitespace changes in the documentation. I think there is only one grammar fix left then.

* There is an uncovered branch that can be removed as it would only be triggered with 128-bit integers.

* A call to `max` needs to be put in parentheses to fix the Windows builds.

Thanks for reviewing. I've merged in the latest develop branch which already includes equivalent changes for a lot of the original PR, and tidied up the remaining diffs. Please let me know if there's anything outstanding to address.

nlohmann · 2025-01-11T16:37:32Z

Please check https://github.com/nlohmann/json/pull/4380/checks?check_run_id=35473431508on how to fix the DCO warning.

nlohmann · 2025-01-12T10:33:50Z

(The coverage job is broken also in other branches. I am looking into this)

slowriot added 5 commits May 24, 2024 13:24

Add support for bson element 11: uint64

9ff8686

Correctly encode unsigned 64bit values in BSON

4c4c209

Update tests to expect uint64_t support

d56e85e

make pretty

31863bc

Amalgamate

7c616a2

slowriot requested a review from nlohmann as a code owner May 24, 2024 12:59

github-actions bot added M tests labels May 24, 2024

slowriot mentioned this pull request May 24, 2024

See other repo slowriot/json#1

Closed

Remove unnecessary cast

3c4c28a

slowriot mentioned this pull request Nov 6, 2024

json::to_bson does not handle unsigned 64 bit integers #3894

Closed

2 tasks

nlohmann requested changes Nov 7, 2024

View reviewed changes

Remove BSON integer limit

d142863

github-actions bot added the documentation label Nov 7, 2024

Update bson mapping tables

8fa4628

Correct serialisation table

5345c07

nlohmann requested changes Nov 16, 2024

View reviewed changes

nlohmann added the aspect: binary formats BSON, CBOR, MessagePack, UBJSON label Nov 20, 2024

slowriot force-pushed the bson-element-11-uint64 branch from d864d43 to 5d0b29b Compare January 10, 2025 18:33

github-actions bot added L CI CMake and removed M labels Jan 10, 2025

slowriot force-pushed the bson-element-11-uint64 branch from 5d0b29b to d864d43 Compare January 10, 2025 18:38

github-actions bot added M and removed L CI CMake labels Jan 10, 2025

This comment was marked as outdated.

Sign in to view

Merge branch 'develop' into bson-element-11-uint64

df79b3f

# Conflicts: # docs/mkdocs/docs/features/binary_formats/bson.md # docs/mkdocs/docs/home/exceptions.md # include/nlohmann/detail/output/binary_writer.hpp # single_include/nlohmann/json.hpp # tests/src/unit-bson.cpp

slowriot force-pushed the bson-element-11-uint64 branch from d864d43 to df79b3f Compare January 10, 2025 18:43

amalgamate

36ce984

nlohmann requested changes Jan 11, 2025

View reviewed changes

slowriot added 4 commits January 11, 2025 16:18

revert leftover whitespace changes from previous merge conflict resol…

f24ecf7

…ution

don't attempt to handle unsigned ints over 64bit

465adfc

amalgamate

01215de

revert leftover whitespace change from merge

24de2ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for BSON element 11: uint64 #4380

Add support for BSON element 11: uint64 #4380

slowriot commented May 24, 2024 •

edited

Loading

coveralls commented May 24, 2024

nlohmann left a comment

slowriot commented Nov 7, 2024

coveralls commented Nov 7, 2024 •

edited

Loading

nlohmann commented Nov 7, 2024

nlohmann Nov 16, 2024

gregmarr Jan 10, 2025

nlohmann Jan 11, 2025

nlohmann commented Nov 16, 2024

nlohmann commented Nov 16, 2024

This comment was marked as outdated.

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

slowriot commented Jan 10, 2025

nlohmann commented Jan 10, 2025

slowriot commented Jan 10, 2025

nlohmann commented Jan 11, 2025

nlohmann left a comment

nlohmann Jan 11, 2025

nlohmann Jan 11, 2025

nlohmann Jan 11, 2025

codenut Jan 12, 2025

nlohmann Jan 11, 2025

slowriot commented Jan 11, 2025

nlohmann commented Jan 11, 2025

nlohmann commented Jan 12, 2025

Add support for BSON element 11: uint64 #4380

Are you sure you want to change the base?

Add support for BSON element 11: uint64 #4380

Conversation

slowriot commented May 24, 2024 • edited Loading

Changes

Reference

Pull request checklist

coveralls commented May 24, 2024

nlohmann left a comment

Choose a reason for hiding this comment

slowriot commented Nov 7, 2024

coveralls commented Nov 7, 2024 • edited Loading

nlohmann commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlohmann commented Nov 16, 2024

nlohmann commented Nov 16, 2024

This comment was marked as outdated.

github-actions bot commented Jan 10, 2025

🔴 Amalgamation check failed! 🔴

github-actions bot commented Jan 10, 2025

🔴 Amalgamation check failed! 🔴

slowriot commented Jan 10, 2025

nlohmann commented Jan 10, 2025

slowriot commented Jan 10, 2025

nlohmann commented Jan 11, 2025

nlohmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slowriot commented Jan 11, 2025

nlohmann commented Jan 11, 2025

nlohmann commented Jan 12, 2025

slowriot commented May 24, 2024 •

edited

Loading

coveralls commented Nov 7, 2024 •

edited

Loading