From 6a7ee8aa35cde364801f1b2b79d28eb4389cab29 Mon Sep 17 00:00:00 2001
From: Nicholas Ormrod <njormrod@fb.com>
Date: Tue, 10 Sep 2013 09:59:04 -0700
Subject: [PATCH] range checks in DynamicConverter

Summary:
Add range-checking to convertTo for small numeric types.

Internally, dynamics represent a numeric with an int64_t or a double.
When converting to a smaller numeric type, DynamicConverter uses a
static_cast. This causes some confusion (re D936940). The code now uses
folly::to, which throws a std::range_error on overflow.

While working on this I also added some light comments to the new
toDynamic section, for consistency with the original convertTo
commenting. I also renamed the internal trait is_associative_container
to is_map, since is_associative_container is looking for a mapped_type
typedef and hence excludes such associative containers as sets.

While adding the overflow tests, I also augmented the typetraits test to
include the is_map and is_range traits, which hitherto had no test
coverage.

Test Plan: build and run tests, both in dbg and opt

Reviewed By: cberner@fb.com

FB internal diff: D961605
---
 folly/DynamicConverter.h            | 32 +++++++++++++++++--------
 folly/test/DynamicConverterTest.cpp | 37 +++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/folly/DynamicConverter.h b/folly/DynamicConverter.h
index 2c3d2c1d..444d1f79 100644
--- a/folly/DynamicConverter.h
+++ b/folly/DynamicConverter.h
@@ -81,7 +81,7 @@ template <typename T> struct is_range
       std::false_type
     >::type {};
 
-template <typename T> struct is_associative_container
+template <typename T> struct is_map
   : std::integral_constant<
       bool,
       is_range<T>::value && has_mapped_type<T>::value
@@ -178,13 +178,14 @@ conversionIterator(const It& it) {
 ///////////////////////////////////////////////////////////////////////////////
 // DynamicConverter specializations
 
-template <typename T, typename Enable = void> struct DynamicConverter;
-
 /**
  * Each specialization of DynamicConverter has the function
- *     'static T convert(const dynamic& d);'
+ *     'static T convert(const dynamic&);'
  */
 
+// default - intentionally unimplemented
+template <typename T, typename Enable = void> struct DynamicConverter;
+
 // boolean
 template <>
 struct DynamicConverter<bool> {
@@ -199,7 +200,7 @@ struct DynamicConverter<T,
     typename std::enable_if<std::is_integral<T>::value &&
                             !std::is_same<T, bool>::value>::type> {
   static T convert(const dynamic& d) {
-    return static_cast<T>(d.asInt());
+    return folly::to<T>(d.asInt());
   }
 };
 
@@ -208,7 +209,7 @@ template <typename T>
 struct DynamicConverter<T,
     typename std::enable_if<std::is_floating_point<T>::value>::type> {
   static T convert(const dynamic& d) {
-    return static_cast<T>(d.asDouble());
+    return folly::to<T>(d.asDouble());
   }
 };
 
@@ -261,9 +262,17 @@ struct DynamicConverter<C,
       throw TypeError("object or array", d.type());
     }
   }
-
 };
 
+///////////////////////////////////////////////////////////////////////////////
+// DynamicConstructor specializations
+
+/**
+ * Each specialization of DynamicConstructor has the function
+ *     'static dynamic construct(const C&);'
+ */
+
+// default
 template <typename C, typename Enable = void>
 struct DynamicConstructor {
   static dynamic construct(const C& x) {
@@ -271,10 +280,11 @@ struct DynamicConstructor {
   }
 };
 
+// maps
 template<typename C>
 struct DynamicConstructor<C,
     typename std::enable_if<
-      dynamicconverter_detail::is_associative_container<C>::value>::type> {
+      dynamicconverter_detail::is_map<C>::value>::type> {
   static dynamic construct(const C& x) {
     dynamic d = dynamic::object;
     for (auto& pair : x) {
@@ -284,10 +294,11 @@ struct DynamicConstructor<C,
   }
 };
 
+// other ranges
 template<typename C>
 struct DynamicConstructor<C,
     typename std::enable_if<
-      !dynamicconverter_detail::is_associative_container<C>::value &&
+      !dynamicconverter_detail::is_map<C>::value &&
       !std::is_constructible<StringPiece, const C&>::value &&
       dynamicconverter_detail::is_range<C>::value>::type> {
   static dynamic construct(const C& x) {
@@ -299,6 +310,7 @@ struct DynamicConstructor<C,
   }
 };
 
+// pair
 template<typename A, typename B>
 struct DynamicConstructor<std::pair<A, B>, void> {
   static dynamic construct(const std::pair<A, B>& x) {
@@ -310,7 +322,7 @@ struct DynamicConstructor<std::pair<A, B>, void> {
 };
 
 ///////////////////////////////////////////////////////////////////////////////
-// convertTo implementation
+// implementation
 
 template <typename T>
 T convertTo(const dynamic& d) {
diff --git a/folly/test/DynamicConverterTest.cpp b/folly/test/DynamicConverterTest.cpp
index c94a00c6..fe79b2bd 100644
--- a/folly/test/DynamicConverterTest.cpp
+++ b/folly/test/DynamicConverterTest.cpp
@@ -46,6 +46,26 @@ TEST(DynamicConverter, template_metaprogramming) {
   EXPECT_EQ(c1t, true);
   EXPECT_EQ(c2t, true);
   EXPECT_EQ(c3t, true);
+
+
+  bool m1f = is_map<int>::value;
+  bool m2f = is_map<std::set<int>>::value;
+
+  bool m1t = is_map<std::map<int, int>>::value;
+
+  EXPECT_EQ(m1f, false);
+  EXPECT_EQ(m2f, false);
+  EXPECT_EQ(m1t, true);
+
+
+  bool r1f = is_range<int>::value;
+
+  bool r1t = is_range<std::set<int>>::value;
+  bool r2t = is_range<std::vector<int>>::value;
+
+  EXPECT_EQ(r1f, false);
+  EXPECT_EQ(r1t, true);
+  EXPECT_EQ(r2t, true);
 }
 
 TEST(DynamicConverter, arithmetic_types) {
@@ -57,10 +77,6 @@ TEST(DynamicConverter, arithmetic_types) {
   auto i2 = convertTo<int64_t>(d2);
   EXPECT_EQ(i2, 123456789012345);
 
-  dynamic d3 = 123456789012345;
-  auto i3 = convertTo<uint8_t>(d3);
-  EXPECT_EQ(i3, 121);
-
   dynamic d4 = 3.141;
   auto i4 = convertTo<float>(d4);
   EXPECT_EQ((int)(i4*100), 314);
@@ -332,6 +348,19 @@ TEST(DynamicConverter, construct) {
   }
 }
 
+TEST(DynamicConverter, errors) {
+  const auto int32Over =
+    static_cast<int64_t>(std::numeric_limits<int32_t>().max()) + 1;
+  const auto floatOver =
+    static_cast<double>(std::numeric_limits<float>().max()) * 2;
+
+  dynamic d1 = int32Over;
+  EXPECT_THROW(convertTo<int32_t>(d1), std::range_error);
+
+  dynamic d2 = floatOver;
+  EXPECT_THROW(convertTo<float>(d2), std::range_error);
+}
+
 int main(int argc, char ** argv) {
   testing::InitGoogleTest(&argc, argv);
   google::ParseCommandLineFlags(&argc, &argv, true);
-- 
2.34.1