Skip to content

Strict size checks for conversions to fixed size containers#2558

Closed
domsoll wants to merge 6 commits into
nlohmann:developfrom
domsoll:conversion_checks
Closed

Strict size checks for conversions to fixed size containers#2558
domsoll wants to merge 6 commits into
nlohmann:developfrom
domsoll:conversion_checks

Conversation

@domsoll
Copy link
Copy Markdown

@domsoll domsoll commented Dec 30, 2020

The current implementation accepts conversion of JSON arrays to fixed size containers like std::array, std::pair or std::tuple, as long as there are sufficient elements in the JSON array to fill the container.
The conversion only implicitly fails if there are to few elements via the out of range exception when trying to access further elements.

I suggest to strictly check for matching JSON array size when converting to fixed size containers instead.
In my opinion a mismatch in size should be considered a type error, as in the sense, that i.e. a std::array<int, 4> is a different type than a std::array<int, 5>.
Silently discarding extra JSON array elements might hide problems with mismatching serialized data and result in unexpected behavior.
If a fixed size container is extracted, it explicitly expresses that a certain number of elements is expected, if this expectation is not met by the JSON object, throwing a type error seems to be the safest way to handle the situation.

Though according to the test cases, the current behavior seems at least partially intentional, so I guess, this is up for debate, which approach is preferable.

Anyway, the commits in this pull request add strict size checks for the conversion to std::array, std::pair, std::tuple and c-style arrays.
There are also explicit JSON type checks for more expressive exceptions when the JSON type is not an array at all.
Test cases are adapted accordingly and some extra test cases are added to check for the new size restrictions.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • 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 file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

…ion of fixed size containers from json

MNT: updated amalgamation header
ENH: test cases for std::pair and std::tuple added
ENH: more test cases for c arrays and std::array added
MNT: redundant array type check for std::array conversion reverted
MNT: updated amalgamation header
@domsoll domsoll requested a review from nlohmann as a code owner December 30, 2020 15:02
@nlohmann
Copy link
Copy Markdown
Owner

The change will break existing code, so it cannot be introduced in the 3.x.x versions. I would propose to guard the test/throwing code with a preprocessor symbol so that the feature can be switched on if needed. What do you think?

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 30, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 6d9bfe6 on domsoll:conversion_checks into 0b345b2 on nlohmann:develop.

…disabled

ENH: test cases adapted for strict size checks option
MNT: updated amalgamation header
@domsoll
Copy link
Copy Markdown
Author

domsoll commented Dec 31, 2020

Thank's for your input, you are right, an introduction in 3.x.x is problematic, I guess the preprocessor symbol is the best option then.
I've added the according preprocessor conditionals and a cmake option to define the symbol which is disabled by default.

The behavior without the strict container size option should now be unchanged, including the type of exceptions thrown in case of errors.
Code coverage is 100% and all test cases pass with and without the option.
I left the extra test cases for c-style arrays active without the new option enabled and extended them to check for the non-strict variant also, I guess that makes sense?

@nlohmann
Copy link
Copy Markdown
Owner

@domsoll Can you please update to the latest develop version to resolve the conflicts?

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Sep 10, 2021
@nlohmann
Copy link
Copy Markdown
Owner

(Related: #3010)

# Conflicts:
#	CMakeLists.txt
#	include/nlohmann/detail/conversions/from_json.hpp
#	include/nlohmann/detail/macro_scope.hpp
#	single_include/nlohmann/json.hpp
@domsoll
Copy link
Copy Markdown
Author

domsoll commented Sep 16, 2021

The pull request is now updated to the latest develop version.

@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Sep 16, 2021
@nlohmann
Copy link
Copy Markdown
Owner

Thanks! Can you please run the amalgamation (make amalgamate)?

@domsoll
Copy link
Copy Markdown
Author

domsoll commented Sep 20, 2021

I did update amalgamation before with make amalgamate.
Though without astyle/make pretty there is one closing brace placed differently (which the check complained about) and with astyle/make pretty I get a huge diff in the amalgamation and the source files.
For some reason my version of astyle seems to behave differently to what was used on the sources before.

Anyway, I reverted the brace placement manually again, so the check should hopefully pass now.

@stale
Copy link
Copy Markdown

stale Bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale 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 Jan 9, 2022
@gregmarr
Copy link
Copy Markdown
Contributor

@domsoll This PR has conflicts again.
@nlohmann Anything else outstanding here?

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Jun 18, 2022
@stale stale 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 Jun 18, 2022
@nlohmann nlohmann closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please rebase Please rebase your branch to origin/develop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants