Integer Precision for JSON <-> Protobuf conversions. * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral. * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles. * Added one test for integer precision between String <-> JSON <-> Protobuf conversions. * Added one test for JSON::Number comparisons.
Review: https://reviews.apache.org/r/38031 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4807db6c Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4807db6c Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4807db6c Branch: refs/heads/master Commit: 4807db6ceb7fafe59c28cbcfb31334ca7061dd59 Parents: ae9579d Author: Joseph Wu <[email protected]> Authored: Wed Sep 16 13:40:02 2015 -0400 Committer: Joris Van Remoortere <[email protected]> Committed: Wed Sep 16 17:45:36 2015 -0400 ---------------------------------------------------------------------- .../3rdparty/stout/include/stout/json.hpp | 157 +++++++++++++++++-- .../3rdparty/stout/include/stout/protobuf.hpp | 54 ++----- .../3rdparty/stout/tests/json_tests.cpp | 74 ++++++--- .../3rdparty/stout/tests/protobuf_tests.cpp | 82 +++++++++- 4 files changed, 289 insertions(+), 78 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/4807db6c/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp index f28138c..6e667a1 100644 --- a/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp @@ -21,6 +21,7 @@ #include <limits> #include <map> #include <string> +#include <type_traits> #include <vector> #include <boost/type_traits/is_arithmetic.hpp> @@ -72,11 +73,71 @@ struct String }; +// NOTE: Due to how PicoJson parses unsigned integers, a roundtrip from Number +// to JSON and back to Number will result in: +// - a signed integer, if the value is less than or equal to INT64_MAX; +// - or a double, if the value is greater than INT64_MAX. +// See: https://github.com/kazuho/picojson/blob/rel/v1.3.0/picojson.h#L777-L781 struct Number { Number() : value(0) {} - Number(double _value) : value(_value) {} - double value; + + template <typename T> + Number( + T _value, + typename std::enable_if<std::is_floating_point<T>::value, int>::type = 0) + : type(FLOATING), value(_value) {} + + template <typename T> + Number( + T _value, + typename std::enable_if< + std::is_integral<T>::value && std::is_signed<T>::value, + int>::type = 0) + : type(SIGNED_INTEGER), signed_integer(_value) {} + + template <typename T> + Number( + T _value, + typename std::enable_if< + std::is_integral<T>::value && std::is_unsigned<T>::value, + int>::type = 0) + : type(UNSIGNED_INTEGER), unsigned_integer(_value) {} + + template <typename T> + T as() const + { + switch (type) { + case FLOATING: + return static_cast<T>(value); + case SIGNED_INTEGER: + return static_cast<T>(signed_integer); + case UNSIGNED_INTEGER: + return static_cast<T>(unsigned_integer); + + // NOTE: By not setting a default we leverage the compiler + // errors when the enumeration is augmented to find all + // the cases we need to provide. + } + } + + enum Type + { + FLOATING, + SIGNED_INTEGER, + UNSIGNED_INTEGER, + } type; + +private: + friend struct Value; + friend struct Comparator; + friend std::ostream& operator<<(std::ostream& stream, const Number& number); + + union { + double value; + int64_t signed_integer; + uint64_t unsigned_integer; + }; }; @@ -207,6 +268,8 @@ struct Value : internal::Variant bool contains(const Value& other) const; private: + friend struct Comparator; + // A class which follows the visitor pattern and implements the // containment rules described in the documentation of 'contains'. // See 'bool Value::contains(const Value& other) const'. @@ -257,7 +320,6 @@ inline const Value& Value::as<Value>() const } - template <typename T> Result<T> Object::find(const std::string& path) const { @@ -384,7 +446,53 @@ inline bool Value::ContainmentComparator::operator()(const Number& other) const if (!self.is<Number>()) { return false; } - return self.as<Number>().value == other.value; + + // NOTE: For the following switch statements, we do not set a default + // case in order to leverage the compiler errors when the enumeration + // is augmented to find all the cases we need to provide. + + // NOTE: Using '==' is unsafe for unsigned-signed integer comparisons. + // The compiler will automatically cast the signed integer to an + // unsigned integer. i.e. If the signed integer was negative, it + // might be converted to a large positive number. + // Using the '==' operator *is* safe for double-integer comparisons. + + const Number& number = self.as<Number>(); + switch (number.type) { + case Number::FLOATING: + switch (other.type) { + case Number::FLOATING: + return number.value == other.value; + case Number::SIGNED_INTEGER: + return number.value == other.signed_integer; + case Number::UNSIGNED_INTEGER: + return number.value == other.unsigned_integer; + } + + case Number::SIGNED_INTEGER: + switch (other.type) { + case Number::FLOATING: + return number.signed_integer == other.value; + case Number::SIGNED_INTEGER: + return number.signed_integer == other.signed_integer; + case Number::UNSIGNED_INTEGER: + // See note above for why this is not a simple '==' expression. + return number.signed_integer >= 0 && + number.as<uint64_t>() == other.unsigned_integer; + } + + case Number::UNSIGNED_INTEGER: + switch (other.type) { + case Number::FLOATING: + return number.unsigned_integer == other.value; + case Number::SIGNED_INTEGER: + // See note above for why this is not a simple '==' expression. + return other.signed_integer >= 0 && + number.unsigned_integer == other.as<uint64_t>(); + case Number::UNSIGNED_INTEGER: + return number.unsigned_integer == other.unsigned_integer; + } + } } @@ -446,12 +554,11 @@ struct Comparator : boost::static_visitor<bool> return false; } - bool operator()(const Number& number) const + bool operator()(const Number& other) const { - if (value.is<Number>()) { - return value.as<Number>().value == number.value; - } - return false; + // Delegate to the containment comparator. + // See Value::ContainmentComparator::operator(Number). + return Value::ContainmentComparator(value)(other); } bool operator()(const Array& array) const @@ -502,10 +609,32 @@ inline std::ostream& operator<<(std::ostream& out, const String& string) inline std::ostream& operator<<(std::ostream& out, const Number& number) { - // Use the guaranteed accurate precision, see: - // http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2005.pdf - return out << std::setprecision(std::numeric_limits<double>::digits10) - << number.value; + switch (number.type) { + case Number::FLOATING: { + // Prints a floating point value, with the specified precision, see: + // http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2005.pdf + // Additionally ensures that a decimal point is in the output. + char buffer[50] {}; // More than long enough for the specified precision. + snprintf( + buffer, + sizeof(buffer), + "%#.*g", + std::numeric_limits<double>::digits10, + number.value); + + // Get rid of trailing zeroes before outputting. + // Otherwise, printing 1.0 would result in "1.00000000000000". + return out << strings::trim(buffer, strings::SUFFIX, "0"); + } + case Number::SIGNED_INTEGER: + return out << number.signed_integer; + case Number::UNSIGNED_INTEGER: + return out << number.unsigned_integer; + + // NOTE: By not setting a default we leverage the compiler + // errors when the enumeration is augmented to find all + // the cases we need to provide. + } } @@ -575,6 +704,8 @@ inline Value convert(const picojson::value& value) array.values.push_back(convert(value)); } return array; + } else if (value.is<int64_t>()) { + return Number(value.get<int64_t>()); } else if (value.is<double>()) { return Number(value.get<double>()); } else if (value.is<std::string>()) { http://git-wip-us.apache.org/repos/asf/mesos/blob/4807db6c/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp index 115cc0c..2285ce9 100644 --- a/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp @@ -391,80 +391,50 @@ struct Parser : boost::static_visitor<Try<Nothing> > switch (field->type()) { case google::protobuf::FieldDescriptor::TYPE_DOUBLE: if (field->is_repeated()) { - reflection->AddDouble(message, field, number.value); + reflection->AddDouble(message, field, number.as<double>()); } else { - reflection->SetDouble(message, field, number.value); + reflection->SetDouble(message, field, number.as<double>()); } break; case google::protobuf::FieldDescriptor::TYPE_FLOAT: if (field->is_repeated()) { - reflection->AddFloat( - message, - field, - static_cast<float>(number.value)); + reflection->AddFloat(message, field, number.as<float>()); } else { - reflection->SetFloat( - message, - field, - static_cast<float>(number.value)); + reflection->SetFloat(message, field, number.as<float>()); } break; case google::protobuf::FieldDescriptor::TYPE_INT64: case google::protobuf::FieldDescriptor::TYPE_SINT64: case google::protobuf::FieldDescriptor::TYPE_SFIXED64: if (field->is_repeated()) { - reflection->AddInt64( - message, - field, - static_cast<int64_t>(number.value)); + reflection->AddInt64(message, field, number.as<int64_t>()); } else { - reflection->SetInt64( - message, - field, - static_cast<int64_t>(number.value)); + reflection->SetInt64(message, field, number.as<int64_t>()); } break; case google::protobuf::FieldDescriptor::TYPE_UINT64: case google::protobuf::FieldDescriptor::TYPE_FIXED64: if (field->is_repeated()) { - reflection->AddUInt64( - message, - field, - static_cast<uint64_t>(number.value)); + reflection->AddUInt64(message, field, number.as<uint64_t>()); } else { - reflection->SetUInt64( - message, - field, - static_cast<uint64_t>(number.value)); + reflection->SetUInt64(message, field, number.as<uint64_t>()); } break; case google::protobuf::FieldDescriptor::TYPE_INT32: case google::protobuf::FieldDescriptor::TYPE_SINT32: case google::protobuf::FieldDescriptor::TYPE_SFIXED32: if (field->is_repeated()) { - reflection->AddInt32( - message, - field, - static_cast<int32_t>(number.value)); + reflection->AddInt32(message, field, number.as<int32_t>()); } else { - reflection->SetInt32( - message, - field, - static_cast<int32_t>(number.value)); + reflection->SetInt32(message, field, number.as<int32_t>()); } break; case google::protobuf::FieldDescriptor::TYPE_UINT32: case google::protobuf::FieldDescriptor::TYPE_FIXED32: if (field->is_repeated()) { - reflection->AddUInt32( - message, - field, - static_cast<uint32_t>(number.value)); + reflection->AddUInt32(message, field, number.as<uint32_t>()); } else { - reflection->SetUInt32( - message, - field, - static_cast<uint32_t>(number.value)); + reflection->SetUInt32(message, field, number.as<uint32_t>()); } break; default: http://git-wip-us.apache.org/repos/asf/mesos/blob/4807db6c/3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp b/3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp index 850650c..88b6147 100644 --- a/3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp +++ b/3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp @@ -50,18 +50,50 @@ TEST(JsonTest, BinaryData) TEST(JsonTest, NumberFormat) { - // Test whole numbers. - EXPECT_EQ("0", stringify(JSON::Number(0.0))); - EXPECT_EQ("1", stringify(JSON::Number(1.0))); + // Test whole numbers (as doubles). + EXPECT_EQ("0.", stringify(JSON::Number(0.0))); + EXPECT_EQ("1.", stringify(JSON::Number(1.0))); // Negative. - EXPECT_EQ("-1", stringify(JSON::Number(-1.0))); + EXPECT_EQ("-1.", stringify(JSON::Number(-1.0))); + + // Test integers. + EXPECT_EQ("0", stringify(JSON::Number(0))); + EXPECT_EQ("2", stringify(JSON::Number(2))); + EXPECT_EQ("-2", stringify(JSON::Number(-2))); // Expect at least 15 digits of precision. EXPECT_EQ("1234567890.12345", stringify(JSON::Number(1234567890.12345))); } +TEST(JsonTest, NumberComparisons) +{ + // Unsigned and signed comparisons. + EXPECT_EQ(JSON::Number(1U), JSON::Number((int64_t) 1)); + EXPECT_EQ(JSON::Number(0U), JSON::Number((int64_t) 0)); + + EXPECT_NE(JSON::Number(1U), JSON::Number(-1)); + EXPECT_NE(JSON::Number((uint64_t) -1), JSON::Number(-1)); + + // Signed and unsigned comparisons (opposite order of above). + EXPECT_EQ(JSON::Number((int64_t) 1), JSON::Number(1U)); + EXPECT_EQ(JSON::Number((int64_t) 0), JSON::Number(0U)); + + EXPECT_NE(JSON::Number(-1), JSON::Number(1U)); + EXPECT_NE(JSON::Number(-1), JSON::Number((uint64_t) -1)); + + // Make sure we aren't doing an implicit cast from int64_t to uint64_t. + // These have the same bit representation (64h'8000_0000_0000_0001). + EXPECT_NE( + JSON::Number(9223372036854775809U), + JSON::Number(-9223372036854775807)); + EXPECT_NE( + JSON::Number(-9223372036854775807), + JSON::Number(9223372036854775809U)); +} + + TEST(JsonTest, BooleanFormat) { EXPECT_EQ("false", stringify(JSON::False())); @@ -121,44 +153,50 @@ TEST(JsonTest, NumericAssignment) s.st_nlink = 1; JSON::Value v = s.st_nlink; JSON::Number d = s.st_nlink; - EXPECT_EQ(get<JSON::Number>(v).value, 1.0); - EXPECT_EQ(d.value, 1.0); + EXPECT_NE(get<JSON::Number>(v).type, JSON::Number::FLOATING); + EXPECT_EQ(get<JSON::Number>(v).as<int64_t>(), 1); + EXPECT_EQ(d.as<int64_t>(), 1); s.st_size = 2; v = s.st_size; d = s.st_size; - EXPECT_EQ(get<JSON::Number>(v).value, 2.0); - EXPECT_EQ(d.value, 2.0); + EXPECT_NE(get<JSON::Number>(v).type, JSON::Number::FLOATING); + EXPECT_EQ(get<JSON::Number>(v).as<int64_t>(), 2); + EXPECT_EQ(d.as<int64_t>(), 2); s.st_mtime = 3; v = s.st_mtime; d = s.st_mtime; - EXPECT_EQ(get<JSON::Number>(v).value, 3.0); - EXPECT_EQ(d.value, 3.0); + EXPECT_EQ(get<JSON::Number>(v).as<int64_t>(), 3); + EXPECT_EQ(d.as<int64_t>(), 3); size_t st = 4; v = st; d = st; - EXPECT_EQ(get<JSON::Number>(v).value, 4.0); - EXPECT_EQ(d.value, 4.0); + EXPECT_EQ(get<JSON::Number>(v).type, JSON::Number::UNSIGNED_INTEGER); + EXPECT_EQ(get<JSON::Number>(v).as<uint64_t>(), 4); + EXPECT_EQ(d.as<uint64_t>(), 4); uint64_t ui64 = 5; v = ui64; d = ui64; - EXPECT_EQ(get<JSON::Number>(v).value, 5.0); - EXPECT_EQ(d.value, 5.0); + EXPECT_EQ(get<JSON::Number>(v).type, JSON::Number::UNSIGNED_INTEGER); + EXPECT_EQ(get<JSON::Number>(v).as<uint64_t>(), 5); + EXPECT_EQ(d.as<uint64_t>(), 5); const unsigned int ui = 6; v = ui; d = ui; - EXPECT_EQ(get<JSON::Number>(v).value, 6.0); - EXPECT_EQ(d.value, 6.0); + EXPECT_EQ(get<JSON::Number>(v).type, JSON::Number::UNSIGNED_INTEGER); + EXPECT_EQ(get<JSON::Number>(v).as<uint64_t>(), 6); + EXPECT_EQ(d.as<uint64_t>(), 6); int i = 7; v = i; d = i; - EXPECT_EQ(get<JSON::Number>(v).value, 7.0); - EXPECT_EQ(d.value, 7.0); + EXPECT_EQ(get<JSON::Number>(v).type, JSON::Number::SIGNED_INTEGER); + EXPECT_EQ(get<JSON::Number>(v).as<int64_t>(), 7); + EXPECT_EQ(d.as<int64_t>(), 7); } http://git-wip-us.apache.org/repos/asf/mesos/blob/4807db6c/3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp b/3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp index 87737dd..01d5ec7 100644 --- a/3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp +++ b/3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp @@ -108,18 +108,18 @@ TEST(ProtobufTest, JSON) "{" " \"b\": true," " \"bytes\": \"Ynl0ZXM=\"," - " \"d\": 1," + " \"d\": 1.," " \"e\": \"ONE\"," - " \"f\": 1," + " \"f\": 1.," " \"int32\": -1," " \"int64\": -1," " \"nested\": { \"str\": \"nested\"}," - " \"optional_default\": 42," + " \"optional_default\": 42.," " \"repeated_bool\": [true]," " \"repeated_bytes\": [\"cmVwZWF0ZWRfYnl0ZXM=\"]," - " \"repeated_double\": [1, 2]," + " \"repeated_double\": [1., 2.]," " \"repeated_enum\": [\"TWO\"]," - " \"repeated_float\": [1]," + " \"repeated_float\": [1.]," " \"repeated_int32\": [-2]," " \"repeated_int64\": [-2]," " \"repeated_nested\": [ { \"str\": \"repeated_nested\" } ]," @@ -233,3 +233,75 @@ TEST(ProtobufTest, ParseJSONArray) EXPECT_EQ(message, repeated.Get(0)); EXPECT_EQ(message, repeated.Get(1)); } + + +// Tests that integer precision is maintained between +// JSON <-> Protobuf conversions. +TEST(ProtobufTest, JsonLargeIntegers) +{ + // These numbers are equal or close to the integer limits. + tests::Message message; + message.set_int32(-2147483647); + message.set_int64(-9223372036854775807); + message.set_uint32(4294967295U); + message.set_uint64(9223372036854775807); + message.set_sint32(-1234567890); + message.set_sint64(-1234567890123456789); + message.add_repeated_int32(-2000000000); + message.add_repeated_int64(-9000000000000000000); + message.add_repeated_uint32(3000000000U); + message.add_repeated_uint64(7000000000000000000); + message.add_repeated_sint32(-1000000000); + message.add_repeated_sint64(-8000000000000000000); + + // Parts of the protobuf that are required. Copied from the above test. + message.set_b(true); + message.set_str("string"); + message.set_bytes("bytes"); + message.set_f(1.0); + message.set_d(1.0); + message.set_e(tests::ONE); + message.mutable_nested()->set_str("nested"); + + // The keys are in alphabetical order. + string expected = strings::remove( + "{" + " \"b\": true," + " \"bytes\": \"Ynl0ZXM=\"," + " \"d\": 1.," + " \"e\": \"ONE\"," + " \"f\": 1.," + " \"int32\": -2147483647," + " \"int64\": -9223372036854775807," + " \"nested\": {\"str\": \"nested\"}," + " \"optional_default\": 42.," + " \"repeated_int32\": [-2000000000]," + " \"repeated_int64\": [-9000000000000000000]," + " \"repeated_sint32\": [-1000000000]," + " \"repeated_sint64\": [-8000000000000000000]," + " \"repeated_uint32\": [3000000000]," + " \"repeated_uint64\": [7000000000000000000]," + " \"sint32\": -1234567890," + " \"sint64\": -1234567890123456789," + " \"str\": \"string\"," + " \"uint32\": 4294967295," + " \"uint64\": 9223372036854775807" + "}", + " "); + + // Check JSON -> String. + JSON::Object object = JSON::Protobuf(message); + EXPECT_EQ(expected, stringify(object)); + + // Check JSON -> Protobuf. + Try<tests::Message> parse = protobuf::parse<tests::Message>(object); + ASSERT_SOME(parse); + + // Check Protobuf -> JSON. + EXPECT_EQ(object, JSON::Protobuf(parse.get())); + + // Check String -> JSON. + Try<JSON::Object> json = JSON::parse<JSON::Object>(expected); + ASSERT_SOME(json); + EXPECT_EQ(object, json.get()); +}
