-
-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Add NLOHMANN_JSON_SERIALIZE_ENUM_STRICT macro that throws type_error.302 on unknown JSON value (Fixes #3992) #4989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add NLOHMANN_JSON_SERIALIZE_ENUM_STRICT macro that throws type_error.302 on unknown JSON value (Fixes #3992) #4989
Conversation
nlohmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change the existing macros. This would break existing applications. Instead, add a macro NLOHMANN_JSON_SERIALIZE_ENUM_STRICT with the new behavior.
95a9a52 to
28866e1
Compare
|
i have made changes accordingly pls review |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @Ash-Jose |
nlohmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please add a new line at the end of
unit-serialize_enum_strict.cpp. - Please run
make amalgamationto properly indent the code.
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @Ash-Jose |
69b82a1 to
a18ff7c
Compare
|
is this because of using concat |
You need to include |
|
included the header |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @Ash-Jose |
|
why are tests still failing?? |
|
Maybe some issue with the includes. Please try:
Sorry for the confusion. |
|
i have reverted it |
|
should i redo in a different branch?? |
| }); \ | ||
| if (it == std::end(m)) \ | ||
| { \ | ||
| throw ::nlohmann::detail::type_error::create( \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should need the exceptions.hpp header which again includes macro_scope.hpp. I think this is the core of the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill try including the exceptions header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this needs to use JSON_THROW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be changed, or this macro can't be used by people that disable exceptions.
|
i dont understand why they still fail |
|
Because there is more to forward-declare. I'm sorry, but it seems to be a more complicated issue to use an exception in the macro header. |
|
The forward declarations aren't required for the macro definition, they're required where the things are used.
|
| throw ::nlohmann::detail::type_error::create( \ | ||
| 302, \ | ||
| std::string("invalid value for ") + #ENUM_TYPE + ": " + j.dump(), \ | ||
| j); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is your problem, it takes a pointer, it should be &j.
| { | ||
| class type_error; | ||
| } // namespace detail | ||
| } // namespace nlohmann |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed.
|
i have made changes accordingly |
|
its still failing a few tests |
test.json
Outdated
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be in the root of the repo. Is this just from local testing and got committed accidentally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad ill take it out
Signed-off-by: Ash-Jose <[email protected]>
…_JSON_SERIALIZE_ENUM_STRICT - Added missing include for <nlohmann/detail/string_concat.hpp> - Replaced string concatenation with concat() call as suggested by maintainer - Re-ran `make amalgamate` to update single-header files Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
…nown values Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
included string header Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash Jose <[email protected]>
Signed-off-by: Ash Jose <[email protected]>
Signed-off-by: Ash Jose <[email protected]>
Signed-off-by: Ash Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ashlin Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
Signed-off-by: Ash-Jose <[email protected]>
9cadcca to
d191b5d
Compare
Signed-off-by: Ash-Jose <[email protected]> Signed-off-by: Ash-Jose <[email protected]>
d191b5d to
1231904
Compare
|
are my changes sufficient this time around? |
|
@nlohmann Looks like the workflows here are awaiting approval. Not sure why since they ran before. |
Summary
This PR fixes #3992 by introducing a new macro
NLOHMANN_JSON_SERIALIZE_ENUM_STRICT, which throws ajson.exception.type_error.302when deserializing a JSON string that does not match any enumerator.
The existing
NLOHMANN_JSON_SERIALIZE_ENUMmacro remains unchanged to preserve backward compatibility.Changes
NLOHMANN_JSON_SERIALIZE_ENUM_STRICTinmacro_scope.hppthat throwstype_error.302for unmapped enum strings.unit-serialize_enum_strict.cppvalidating the strict throwing behavior.make amalgamate.ctest --output-on-failure.Expected test impact
All existing tests for
NLOHMANN_JSON_SERIALIZE_ENUMcontinue to pass unchanged.New tests for
NLOHMANN_JSON_SERIALIZE_ENUM_STRICTconfirm that unknown JSON values now trigger exceptions.Other unrelated floating-point mismatches (e.g.,
test-regression1_cpp11) are not affected by this change.Documentation
A new documentation entry will be added at
docs/api/macros/nlohmann_json_serialize_enum_strict.mddescribing the stricter deserialization behavior.Checklist
NLOHMANN_JSON_SERIALIZE_ENUM_STRICT)make amalgamate)