From 2806b201a85d418d6b1aa3f4af25f198495e0744 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20DELRIEU?= Date: Mon, 1 Apr 2019 13:09:38 +0200 Subject: [PATCH 1/3] make sure values are overwritten in from_json overloads Caused unexpected behaviors when using get_to with values previously set. Fixes !1511 --- .../nlohmann/detail/conversions/from_json.hpp | 17 ++- include/nlohmann/detail/meta/type_traits.hpp | 43 ++++-- single_include/nlohmann/json.hpp | 60 +++++--- test/src/unit-conversions.cpp | 131 +++++++++++++++++- 4 files changed, 208 insertions(+), 43 deletions(-) diff --git a/include/nlohmann/detail/conversions/from_json.hpp b/include/nlohmann/detail/conversions/from_json.hpp index 8f8162ff94..bb049aa8d0 100644 --- a/include/nlohmann/detail/conversions/from_json.hpp +++ b/include/nlohmann/detail/conversions/from_json.hpp @@ -136,6 +136,7 @@ void from_json(const BasicJsonType& j, std::forward_list& l) { JSON_THROW(type_error::create(302, "type must be array, but is " + std::string(j.type_name()))); } + l.clear(); std::transform(j.rbegin(), j.rend(), std::front_inserter(l), [](const BasicJsonType & i) { @@ -182,14 +183,16 @@ auto from_json_array_impl(const BasicJsonType& j, ConstructibleArrayType& arr, p { using std::end; - arr.reserve(j.size()); + ConstructibleArrayType ret; + ret.reserve(j.size()); std::transform(j.begin(), j.end(), - std::inserter(arr, end(arr)), [](const BasicJsonType & i) + std::inserter(ret, end(ret)), [](const BasicJsonType & i) { // get() returns *this, this won't call a from_json // method when value_type is BasicJsonType return i.template get(); }); + arr = std::move(ret); } template @@ -198,14 +201,16 @@ void from_json_array_impl(const BasicJsonType& j, ConstructibleArrayType& arr, { using std::end; + ConstructibleArrayType ret; std::transform( - j.begin(), j.end(), std::inserter(arr, end(arr)), + j.begin(), j.end(), std::inserter(ret, end(ret)), [](const BasicJsonType & i) { // get() returns *this, this won't call a from_json // method when value_type is BasicJsonType return i.template get(); }); + arr = std::move(ret); } template (); using value_type = typename ConstructibleObjectType::value_type; std::transform( inner_object->begin(), inner_object->end(), - std::inserter(obj, obj.begin()), + std::inserter(ret, ret.begin()), [](typename BasicJsonType::object_t::value_type const & p) { return value_type(p.first, p.second.template get()); }); + obj = std::move(ret); } // overload for arithmetic types, not chosen for basic_json template arguments @@ -319,6 +326,7 @@ void from_json(const BasicJsonType& j, std::map& { JSON_THROW(type_error::create(302, "type must be array, but is " + std::string(j.type_name()))); } + m.clear(); for (const auto& p : j) { if (JSON_UNLIKELY(not p.is_array())) @@ -338,6 +346,7 @@ void from_json(const BasicJsonType& j, std::unordered_map::value and - std::is_same::value) or - (has_from_json::value or - has_non_default_from_json::value); + (std::is_default_constructible::value and + (std::is_move_assignable::value or + std::is_copy_assignable::value) and + (std::is_constructible::value and + std::is_same < + typename object_t::mapped_type, + typename ConstructibleObjectType::mapped_type >::value)) or + (has_from_json::value or + has_non_default_from_json < + BasicJsonType, + typename ConstructibleObjectType::mapped_type >::value); }; template @@ -278,20 +287,24 @@ struct is_constructible_array_type_impl < BasicJsonType, ConstructibleArrayType, enable_if_t::value and - is_detected::value and - is_detected::value and - is_complete_type< - detected_t>::value >> + std::is_default_constructible::value and +(std::is_move_assignable::value or + std::is_copy_assignable::value) and +is_detected::value and +is_detected::value and +is_complete_type< +detected_t>::value >> { static constexpr bool value = // This is needed because json_reverse_iterator has a ::iterator type, - // furthermore, std::back_insert_iterator (and other iterators) have a base class `iterator`... - // Therefore it is detected as a ConstructibleArrayType. - // The real fix would be to have an Iterable concept. - not is_iterator_traits < - iterator_traits>::value and - - (std::is_same::value or + // furthermore, std::back_insert_iterator (and other iterators) have a + // base class `iterator`... Therefore it is detected as a + // ConstructibleArrayType. The real fix would be to have an Iterable + // concept. + not is_iterator_traits>::value and + + (std::is_same::value or has_from_json::value or has_non_default_from_json < diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 1f11538cba..240b08f2ca 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -1059,10 +1059,19 @@ struct is_constructible_object_type_impl < using object_t = typename BasicJsonType::object_t; static constexpr bool value = - (std::is_constructible::value and - std::is_same::value) or - (has_from_json::value or - has_non_default_from_json::value); + (std::is_default_constructible::value and + (std::is_move_assignable::value or + std::is_copy_assignable::value) and + (std::is_constructible::value and + std::is_same < + typename object_t::mapped_type, + typename ConstructibleObjectType::mapped_type >::value)) or + (has_from_json::value or + has_non_default_from_json < + BasicJsonType, + typename ConstructibleObjectType::mapped_type >::value); }; template @@ -1145,20 +1154,24 @@ struct is_constructible_array_type_impl < BasicJsonType, ConstructibleArrayType, enable_if_t::value and - is_detected::value and - is_detected::value and - is_complete_type< - detected_t>::value >> + std::is_default_constructible::value and +(std::is_move_assignable::value or + std::is_copy_assignable::value) and +is_detected::value and +is_detected::value and +is_complete_type< +detected_t>::value >> { static constexpr bool value = // This is needed because json_reverse_iterator has a ::iterator type, - // furthermore, std::back_insert_iterator (and other iterators) have a base class `iterator`... - // Therefore it is detected as a ConstructibleArrayType. - // The real fix would be to have an Iterable concept. - not is_iterator_traits < - iterator_traits>::value and - - (std::is_same::value or + // furthermore, std::back_insert_iterator (and other iterators) have a + // base class `iterator`... Therefore it is detected as a + // ConstructibleArrayType. The real fix would be to have an Iterable + // concept. + not is_iterator_traits>::value and + + (std::is_same::value or has_from_json::value or has_non_default_from_json < @@ -1411,6 +1424,7 @@ void from_json(const BasicJsonType& j, std::forward_list& l) { JSON_THROW(type_error::create(302, "type must be array, but is " + std::string(j.type_name()))); } + l.clear(); std::transform(j.rbegin(), j.rend(), std::front_inserter(l), [](const BasicJsonType & i) { @@ -1457,14 +1471,16 @@ auto from_json_array_impl(const BasicJsonType& j, ConstructibleArrayType& arr, p { using std::end; - arr.reserve(j.size()); + ConstructibleArrayType ret; + ret.reserve(j.size()); std::transform(j.begin(), j.end(), - std::inserter(arr, end(arr)), [](const BasicJsonType & i) + std::inserter(ret, end(ret)), [](const BasicJsonType & i) { // get() returns *this, this won't call a from_json // method when value_type is BasicJsonType return i.template get(); }); + arr = std::move(ret); } template @@ -1473,14 +1489,16 @@ void from_json_array_impl(const BasicJsonType& j, ConstructibleArrayType& arr, { using std::end; + ConstructibleArrayType ret; std::transform( - j.begin(), j.end(), std::inserter(arr, end(arr)), + j.begin(), j.end(), std::inserter(ret, end(ret)), [](const BasicJsonType & i) { // get() returns *this, this won't call a from_json // method when value_type is BasicJsonType return i.template get(); }); + arr = std::move(ret); } template (); using value_type = typename ConstructibleObjectType::value_type; std::transform( inner_object->begin(), inner_object->end(), - std::inserter(obj, obj.begin()), + std::inserter(ret, ret.begin()), [](typename BasicJsonType::object_t::value_type const & p) { return value_type(p.first, p.second.template get()); }); + obj = std::move(ret); } // overload for arithmetic types, not chosen for basic_json template arguments @@ -1594,6 +1614,7 @@ void from_json(const BasicJsonType& j, std::map& { JSON_THROW(type_error::create(302, "type must be array, but is " + std::string(j.type_name()))); } + m.clear(); for (const auto& p : j) { if (JSON_UNLIKELY(not p.is_array())) @@ -1613,6 +1634,7 @@ void from_json(const BasicJsonType& j, std::unordered_map") + { + std::map o{{"previous", "value"}}; + j.get_to(o); + CHECK(json(o) == j); + } + + SECTION("std::multimap") + { + std::multimap o{{"previous", "value"}}; + j.get_to(o); + CHECK(json(o) == j); + } + + SECTION("std::unordered_map") + { + std::unordered_map o{{"previous", "value"}}; + j.get_to(o); + CHECK(json(o) == j); + } + + SECTION("std::unordered_multimap") + { + std::unordered_multimap o{{"previous", "value"}}; + j.get_to(o); + CHECK(json(o) == j); + } + } + + SECTION("get an object (implicit)") { json::object_t o_reference = {{"object", json::object()}, @@ -226,11 +274,6 @@ TEST_CASE("value conversion") #if not defined(JSON_NOEXCEPTION) SECTION("reserve is called on containers that supports it") { - // making the call to from_json throw in order to check capacity - std::vector v; - CHECK_THROWS_AS(nlohmann::from_json(j, v), json::type_error&); - CHECK(v.capacity() == j.size()); - // make sure all values are properly copied std::vector v2 = json({1, 2, 3, 4, 5, 6, 7, 8, 9, 10}); CHECK(v2.size() == 10); @@ -302,6 +345,55 @@ TEST_CASE("value conversion") } } + SECTION("get an array (explicit, get_to)") + { + json::array_t a_reference{json(1), json(1u), json(2.2), + json(false), json("string"), json()}; + json j(a_reference); + + SECTION("json::array_t") + { + json::array_t a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + + SECTION("std::valarray") + { + std::valarray a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + + SECTION("std::list") + { + std::list a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + + SECTION("std::forward_list") + { + std::forward_list a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + + SECTION("std::vector") + { + std::vector a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + + SECTION("std::deque") + { + std::deque a{"previous", "value"}; + j.get_to(a); + CHECK(json(a) == j); + } + } + SECTION("get an array (implicit)") { json::array_t a_reference{json(1), json(1u), json(2.2), @@ -433,6 +525,35 @@ TEST_CASE("value conversion") #endif } + SECTION("get a string (explicit, get_to)") + { + json::string_t s_reference{"Hello world"}; + json j(s_reference); + + SECTION("string_t") + { + json::string_t s = "previous value"; + j.get_to(s); + CHECK(json(s) == j); + } + + SECTION("std::string") + { + std::string s = "previous value"; + j.get_to(s); + CHECK(json(s) == j); + } +#if defined(JSON_HAS_CPP_17) + SECTION("std::string_view") + { + std::string s = "previous value"; + std::string_view sv = s; + j.get_to(sv); + CHECK(json(sv) == j); + } +#endif + } + SECTION("get null (explicit)") { std::nullptr_t n = nullptr; From e6e6805c6c80d6ec82ba719afd435fd2280535da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20DELRIEU?= Date: Mon, 1 Apr 2019 13:32:06 +0200 Subject: [PATCH 2/3] add built-in array support in get_to --- .../nlohmann/detail/conversions/from_json.hpp | 10 ++++++++ include/nlohmann/json.hpp | 13 +++++++++++ single_include/nlohmann/json.hpp | 23 +++++++++++++++++++ test/src/unit-conversions.cpp | 10 ++++++++ 4 files changed, 56 insertions(+) diff --git a/include/nlohmann/detail/conversions/from_json.hpp b/include/nlohmann/detail/conversions/from_json.hpp index bb049aa8d0..08a841b4c4 100644 --- a/include/nlohmann/detail/conversions/from_json.hpp +++ b/include/nlohmann/detail/conversions/from_json.hpp @@ -157,6 +157,16 @@ void from_json(const BasicJsonType& j, std::valarray& l) std::copy(j.m_value.array->begin(), j.m_value.array->end(), std::begin(l)); } +template +auto from_json(const BasicJsonType& j, T (&arr)[N]) +-> decltype(j.template get(), void()) +{ + for (std::size_t i = 0; i < N; ++i) + { + arr[i] = j.at(i).template get(); + } +} + template void from_json_array_impl(const BasicJsonType& j, typename BasicJsonType::array_t& arr, priority_tag<3> /*unused*/) { diff --git a/include/nlohmann/json.hpp b/include/nlohmann/json.hpp index b9558a939e..017ce92984 100644 --- a/include/nlohmann/json.hpp +++ b/include/nlohmann/json.hpp @@ -2684,6 +2684,19 @@ class basic_json return v; } + template < + typename T, std::size_t N, + typename Array = T (&)[N], + detail::enable_if_t < + detail::has_from_json::value, int > = 0 > + Array get_to(T (&v)[N]) const + noexcept(noexcept(JSONSerializer::from_json( + std::declval(), v))) + { + JSONSerializer::from_json(*this, v); + return v; + } + /*! @brief get a pointer value (implicit) diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 240b08f2ca..1f82da447e 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -1445,6 +1445,16 @@ void from_json(const BasicJsonType& j, std::valarray& l) std::copy(j.m_value.array->begin(), j.m_value.array->end(), std::begin(l)); } +template +auto from_json(const BasicJsonType& j, T (&arr)[N]) +-> decltype(j.template get(), void()) +{ + for (std::size_t i = 0; i < N; ++i) + { + arr[i] = j.at(i).template get(); + } +} + template void from_json_array_impl(const BasicJsonType& j, typename BasicJsonType::array_t& arr, priority_tag<3> /*unused*/) { @@ -15475,6 +15485,19 @@ class basic_json return v; } + template < + typename T, std::size_t N, + typename Array = T (&)[N], + detail::enable_if_t < + detail::has_from_json::value, int > = 0 > + Array get_to(T (&v)[N]) const + noexcept(noexcept(JSONSerializer::from_json( + std::declval(), v))) + { + JSONSerializer::from_json(*this, v); + return v; + } + /*! @brief get a pointer value (implicit) diff --git a/test/src/unit-conversions.cpp b/test/src/unit-conversions.cpp index 800dded1dd..c81720d25b 100644 --- a/test/src/unit-conversions.cpp +++ b/test/src/unit-conversions.cpp @@ -386,6 +386,16 @@ TEST_CASE("value conversion") CHECK(json(a) == j); } + SECTION("built-in arrays") + { + const int nbs[] = {0, 1, 2}; + int nbs2[] = {0, 0, 0}; + + json j2 = nbs; + j2.get_to(nbs2); + CHECK(std::equal(std::begin(nbs), std::end(nbs), std::begin(nbs2))); + } + SECTION("std::deque") { std::deque a{"previous", "value"}; From d66abda4ee24111809dfe1846a0840eb1a1709af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20DELRIEU?= Date: Wed, 3 Apr 2019 12:34:15 +0200 Subject: [PATCH 3/3] tests: fix coverage --- test/src/unit-conversions.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/src/unit-conversions.cpp b/test/src/unit-conversions.cpp index c81720d25b..25d4f18ad8 100644 --- a/test/src/unit-conversions.cpp +++ b/test/src/unit-conversions.cpp @@ -634,13 +634,19 @@ TEST_CASE("value conversion") CHECK(json(b) == j); } + SECTION("uint8_t") + { + auto n = j.get(); + CHECK(n == 1); + } + SECTION("bool") { bool b = j.get(); CHECK(json(b) == j); } - SECTION("exception in case of a non-string type") + SECTION("exception in case of a non-number type") { CHECK_THROWS_AS(json(json::value_t::null).get(), json::type_error&); @@ -650,6 +656,8 @@ TEST_CASE("value conversion") json::type_error&); CHECK_THROWS_AS(json(json::value_t::string).get(), json::type_error&); + CHECK_THROWS_AS(json(json::value_t::string).get(), + json::type_error&); CHECK_THROWS_AS( json(json::value_t::number_integer).get(), json::type_error&);