From 46e1d078ab2d0a4202c2beffdf21e18efa005fe1 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Mon, 10 Aug 2015 20:29:02 +0000 Subject: [PATCH 1/5] Prevent comparisons of naive and non-naive datetimes from crashing before conversions. --- xblock/fields.py | 6 +++++- xblock/test/test_fields.py | 32 +++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/xblock/fields.py b/xblock/fields.py index d9f672a..67d4eaa 100644 --- a/xblock/fields.py +++ b/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) diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index 2e5cbc2..d5be912 100644 --- a/xblock/test/test_fields.py +++ b/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,25 @@ 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. + """ + block = self.get_block(True) + block.field_x = original + block.field_x = replacement + def test_none(self): self.assertJSONOrSetEquals(None, None) self.assertJSONOrSetEquals(None, '') From dd3542222ad3595c620cfe03688fb074d5a00336 Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Fri, 31 Jul 2015 15:58:06 +0200 Subject: [PATCH 2/5] Make Field.to_string() and Field.from_string() methods more consistent. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old version of these methods was inherently assymmetric. The deserialisation in from_string() only called from_json() when enforce_type() was overridden to do so (which all classes with a non-trivial from_json() implementation do) and enable_enforce_type was set to True. This commit makes the two functions more consistent by not considering enable_enforce_type at all for deserialisation and calling from_json() in enforce_type() by default. Serialisation and deserialisation requires correct types to work already, so there is no harm in always enforciing the type – the code fails in the same cases as before. This commit fixes a bug in serialising DateTime fields. The old version included spurious double quotes around the string, which would not be correctly deserialised when enable_enforce_type was not set (the default). --- xblock/fields.py | 52 +++++++++++++------------------------- xblock/test/test_fields.py | 21 ++++++++++----- 2 files changed, 31 insertions(+), 42 deletions(-) diff --git a/xblock/fields.py b/xblock/fields.py index 67d4eaa..5dc9f9d 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -602,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): """ @@ -614,7 +614,7 @@ def enforce_type(self, value): This must not have side effects, since it will be executed to trigger a DeprecationWarning even if enforce_type is disabled """ - return value + return self.from_json(value) def read_from(self, xblock): """ @@ -678,8 +678,6 @@ def from_json(self, value): return None return int(value) - enforce_type = from_json - class Float(JSONField): """ @@ -697,8 +695,6 @@ def from_json(self, value): return None return float(value) - enforce_type = from_json - class Boolean(JSONField): """ @@ -735,8 +731,6 @@ def from_json(self, value): else: return bool(value) - enforce_type = from_json - class Dict(JSONField): """ @@ -753,8 +747,6 @@ def from_json(self, value): else: raise TypeError('Value stored in a Dict must be None or a dict, found %s' % type(value)) - enforce_type = from_json - class List(JSONField): """ @@ -771,8 +763,6 @@ def from_json(self, value): else: raise TypeError('Value stored in a List must be None or a list, found %s' % type(value)) - enforce_type = from_json - class Set(JSONField): """ @@ -825,8 +815,6 @@ def to_string(self, value): """String gets serialized and deserialized without quote marks.""" return self.to_json(value) - enforce_type = from_json - class DateTime(JSONField): """ @@ -842,31 +830,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): """ @@ -878,11 +862,9 @@ 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 - - return self.from_json(value) + def to_string(self, value): + """DateTime fields get serialized without quote marks.""" + return self.to_json(value) class Any(JSONField): diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index d5be912..257bba8 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -678,14 +678,14 @@ 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) @ddt.unpack @@ -726,7 +726,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) @@ -738,7 +741,7 @@ 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) @ddt.unpack @@ -806,7 +809,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) @@ -817,7 +824,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 @@ -827,4 +834,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) From 1d02fda5ac8c80b379695e15d8a72711bff38b68 Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Thu, 13 Aug 2015 13:47:37 +0200 Subject: [PATCH 3/5] Revert calling from_json() in enforce_type() unconditionally. This change would cause test failures in the LMS, though I can't figure out why. This part of the original commit was more of a clean-up than a fix, so it's not necessary to address the bug with DateTime deserialisation. I still think the clean-up would actually make sense, but I currently don't have the time for further debugging. --- xblock/fields.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/xblock/fields.py b/xblock/fields.py index 5dc9f9d..7d44611 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -614,7 +614,7 @@ def enforce_type(self, value): This must not have side effects, since it will be executed to trigger a DeprecationWarning even if enforce_type is disabled """ - return self.from_json(value) + return value def read_from(self, xblock): """ @@ -678,6 +678,8 @@ def from_json(self, value): return None return int(value) + enforce_type = from_json + class Float(JSONField): """ @@ -695,6 +697,8 @@ def from_json(self, value): return None return float(value) + enforce_type = from_json + class Boolean(JSONField): """ @@ -731,6 +735,8 @@ def from_json(self, value): else: return bool(value) + enforce_type = from_json + class Dict(JSONField): """ @@ -747,6 +753,8 @@ def from_json(self, value): else: raise TypeError('Value stored in a Dict must be None or a dict, found %s' % type(value)) + enforce_type = from_json + class List(JSONField): """ @@ -763,6 +771,8 @@ def from_json(self, value): else: raise TypeError('Value stored in a List must be None or a list, found %s' % type(value)) + enforce_type = from_json + class Set(JSONField): """ @@ -815,6 +825,8 @@ def to_string(self, value): """String gets serialized and deserialized without quote marks.""" return self.to_json(value) + enforce_type = from_json + class DateTime(JSONField): """ @@ -866,6 +878,8 @@ def to_string(self, value): """DateTime fields get serialized without quote marks.""" return self.to_json(value) + enforce_type = from_json + class Any(JSONField): """ From e9ef8df7b55782cff43dd616bd5a500f8fef61cc Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Thu, 13 Aug 2015 15:27:09 +0200 Subject: [PATCH 4/5] Improve test coverage for _ceck_or_enforce_type(). --- xblock/test/test_fields.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index 257bba8..dd034ed 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -280,9 +280,10 @@ def test_naive(self, original, replacement): """ Make sure field comparison doesn't crash when comparing naive and non-naive datetimes. """ - block = self.get_block(True) - block.field_x = original - block.field_x = replacement + 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) From 6bc255678498be106771e452d0514b152ed952be Mon Sep 17 00:00:00 2001 From: Sven Marnach Date: Thu, 3 Sep 2015 16:56:23 +0200 Subject: [PATCH 5/5] Add comments clarifying the field data serialisation tests. --- xblock/test/test_fields.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index dd034ed..e272b7b 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -689,6 +689,8 @@ def assert_from_string(self, _type, string, value): 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'), @@ -745,6 +747,10 @@ def test_to_string_regexp_matches(self, _type, value, regexp): 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),