Merge pull request #308 from open-craft/smarnach/serialisation-fix

Make Field.to_string() and Field.from_string() methods more consistent.

xblock/fields.py

@@ -439,7 +439,11 @@ def _check_or_enforce_type(self, value):
                 value, traceback.format_exc().splitlines()[-1])
             warnings.warn(message, FailingEnforceTypeWarning, stacklevel=3)
         else:
-            if value != new_value:
+            try:
+                equal = value == new_value
+            except TypeError:
+                equal = False
+            if not equal:
                 message = u"The value {} would be enforced to {}".format(
                     value, new_value)
                 warnings.warn(message, ModifyingEnforceTypeWarning, stacklevel=3)
@@ -598,7 +602,7 @@ def from_string(self, serialized):
         """
         self._warn_deprecated_outside_JSONField()
         value = yaml.safe_load(serialized)
-        return self._check_or_enforce_type(value)
+        return self.enforce_type(value)
 
     def enforce_type(self, value):
         """
@@ -838,31 +842,27 @@ def from_json(self, value):
         """
         Parse the date from an ISO-formatted date string, or None.
         """
-        if isinstance(value, basestring):
+        if value is None:
+            return None
 
+        if isinstance(value, basestring):
             # Parser interprets empty string as now by default
             if value == "":
                 return None
-
             try:
-                parsed_date = dateutil.parser.parse(value)
+                value = dateutil.parser.parse(value)
             except (TypeError, ValueError):
                 raise ValueError("Could not parse {} as a date".format(value))
 
-            if parsed_date.tzinfo is not None:  # pylint: disable=maybe-no-member
-                parsed_date.astimezone(pytz.utc)  # pylint: disable=maybe-no-member
-            else:
-                parsed_date = parsed_date.replace(tzinfo=pytz.utc)  # pylint: disable=maybe-no-member
-
-            return parsed_date
-
-        if value is None:
-            return None
-
-        if isinstance(value, datetime.datetime):
-            return value
+        if not isinstance(value, datetime.datetime):
+            raise TypeError(
+                "Value should be loaded from a string, a datetime object or None, not {}".format(type(value))
+            )
 
-        raise TypeError("Value should be loaded from a string, not {}".format(type(value)))
+        if value.tzinfo is not None:  # pylint: disable=maybe-no-member
+            return value.astimezone(pytz.utc)  # pylint: disable=maybe-no-member
+        else:
+            return value.replace(tzinfo=pytz.utc)  # pylint: disable=maybe-no-member
 
     def to_json(self, value):
         """
@@ -874,11 +874,11 @@ def to_json(self, value):
             return None
         raise TypeError("Value stored must be a datetime object, not {}".format(type(value)))
 
-    def enforce_type(self, value):
-        if isinstance(value, datetime.datetime) or value is None:
-            return value
+    def to_string(self, value):
+        """DateTime fields get serialized without quote marks."""
+        return self.to_json(value)
 
-        return self.from_json(value)
+    enforce_type = from_json
 
 
 class Any(JSONField):

xblock/test/test_fields.py

@@ -37,9 +37,9 @@ class FieldTest(unittest.TestCase):
 
     FIELD_TO_TEST = Mock()
 
-    def set_and_get_field(self, arg, enforce_type):
+    def get_block(self, enforce_type):
         """
-        Set the field to arg in a Block, get it and return it
+        Create a block with a field 'field_x' that is of type FIELD_TO_TEST.
         """
         class TestBlock(XBlock):
             """
@@ -48,7 +48,13 @@ class TestBlock(XBlock):
             field_x = self.FIELD_TO_TEST(enforce_type=enforce_type)
 
         runtime = TestRuntime(services={'field-data': DictFieldData({})})
-        block = TestBlock(runtime, scope_ids=Mock(spec=ScopeIds))
+        return TestBlock(runtime, scope_ids=Mock(spec=ScopeIds))
+
+    def set_and_get_field(self, arg, enforce_type):
+        """
+        Set the field to arg in a Block, get it and return the set value.
+        """
+        block = self.get_block(enforce_type)
         block.field_x = arg
         return block.field_x
 
@@ -223,6 +229,7 @@ def test_error(self):
         self.assertJSONOrSetTypeError({})
 
 
+@ddt.ddt
 class DateTest(FieldTest):
     """
     Tests of the Date field.
@@ -258,6 +265,26 @@ def test_serialize(self):
             dt.datetime(2014, 4, 1, 2, 3, 4).replace(tzinfo=pytz.utc)
         )
 
+    @ddt.unpack
+    @ddt.data(
+        (
+            dt.datetime(2014, 4, 1, 2, 3, 4).replace(tzinfo=pytz.utc),
+            dt.datetime(2014, 4, 1, 2, 3, 5)
+        ),
+        (
+            dt.datetime(2014, 4, 1, 2, 3, 4),
+            dt.datetime(2014, 4, 1, 2, 3, 4).replace(tzinfo=pytz.utc),
+        )
+    )
+    def test_naive(self, original, replacement):
+        """
+        Make sure field comparison doesn't crash when comparing naive and non-naive datetimes.
+        """
+        for enforce_type in (False, True):
+            block = self.get_block(enforce_type)
+            block.field_x = original
+            block.field_x = replacement
+
     def test_none(self):
         self.assertJSONOrSetEquals(None, None)
         self.assertJSONOrSetEquals(None, '')
@@ -652,16 +679,18 @@ def assert_to_string(self, _type, value, string):
         """
         Helper method: checks if _type's to_string given instance of _type returns expected string
         """
-        result = _type(enforce_type=True).to_string(value)
+        result = _type().to_string(value)
         self.assertEquals(result, string)
 
     def assert_from_string(self, _type, string, value):
         """
         Helper method: checks if _type's from_string given string representation of type returns expected value
         """
-        result = _type(enforce_type=True).from_string(string)
+        result = _type().from_string(string)
         self.assertEquals(result, value)
 
+    # Serialisation test data that is tested both ways, i.e. whether serialisation of the value
+    # yields the string and deserialisation of the string yields the value.
     @ddt.unpack
     @ddt.data(
         (Integer, 0, '0'),
@@ -700,7 +729,10 @@ def assert_from_string(self, _type, string, value):
                 2,
                 3
               ]
-            }""")))
+            }""")),
+        (DateTime, dt.datetime(2014, 4, 1, 2, 3, 4, 567890).replace(tzinfo=pytz.utc), '2014-04-01T02:03:04.567890'),
+        (DateTime, dt.datetime(2014, 4, 1, 2, 3, 4).replace(tzinfo=pytz.utc), '2014-04-01T02:03:04.000000'),
+    )
     def test_both_directions(self, _type, value, string):
         """Easy cases that work in both directions."""
         self.assert_to_string(_type, value, string)
@@ -712,9 +744,13 @@ def test_both_directions(self, _type, value, string):
         (Float, 1.0, r"1|1\.0*"),
         (Float, -10.0, r"-10|-10\.0*"))
     def test_to_string_regexp_matches(self, _type, value, regexp):
-        result = _type(enforce_type=True).to_string(value)
+        result = _type().to_string(value)
         self.assertRegexpMatches(result, regexp)
 
+    # Test data for non-canonical serialisations of values that we should be able to correctly
+    # deserialise.  These values are not serialised to the representation given here for various
+    # reasons; some of them are non-standard number representations, others are YAML data that
+    # isn't valid JSON, yet others use non-standard capitalisation.
     @ddt.unpack
     @ddt.data(
         (Integer, "0xff", 0xff),
@@ -780,7 +816,11 @@ def test_to_string_regexp_matches(self, _type, value, regexp):
                   kaw: null
             """),
             [1, 2.345, {"foo": True, "bar": [1, 2, 3]}, {"meow": False, "woof": True, "kaw": None}]
-        )
+        ),
+        # Test that legacy DateTime format including double quotes can still be imported for compatibility with
+        # old data export tar balls.
+        (DateTime, '"2014-04-01T02:03:04.567890"', dt.datetime(2014, 4, 1, 2, 3, 4, 567890).replace(tzinfo=pytz.utc)),
+        (DateTime, '"2014-04-01T02:03:04.000000"', dt.datetime(2014, 4, 1, 2, 3, 4).replace(tzinfo=pytz.utc)),
     )
     def test_from_string(self, _type, string, value):
         self.assert_from_string(_type, string, value)
@@ -791,7 +831,7 @@ def test_float_from_NaN_is_nan(self):  # pylint: disable=invalid-name
         This special test case is necessary since
         float('nan') compares inequal to everything.
         """
-        result = Float(enforce_type=True).from_string('NaN')
+        result = Float().from_string('NaN')
         self.assertTrue(math.isnan(result))
 
     @ddt.unpack
@@ -801,4 +841,4 @@ def test_float_from_NaN_is_nan(self):  # pylint: disable=invalid-name
     def test_from_string_errors(self, _type, string):
         """ Cases that raises various exceptions."""
         with self.assertRaises(StandardError):
-            _type(enforce_type=True).from_string(string)
+            _type().from_string(string)