diff --git a/AUTHORS b/AUTHORS index fcbfd8b58..eb05242dd 100644 --- a/AUTHORS +++ b/AUTHORS @@ -34,3 +34,4 @@ Eugeny Kolpakov David Bodor Dhruv Baldawa Matjaz Gregoric +Adam Palay diff --git a/xblock/fields.py b/xblock/fields.py index ff18ed96c..00d326887 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -405,7 +405,7 @@ def _mark_dirty(self, xblock, value): # Deep copy the value being marked as dirty, so that there # is a baseline to check against when saving later - if self not in xblock._dirty_fields or value is EXPLICITLY_SET: + if self not in xblock._dirty_fields: xblock._dirty_fields[self] = copy.deepcopy(value) def _is_dirty(self, xblock): @@ -498,11 +498,22 @@ def __set__(self, xblock, value): Setting a value does not update the underlying data store; the new value is kept in the cache and the xblock is marked as dirty until `save` is explicitly called. + + Note, if there's already a cached value and it's equal to the value + we're trying to cache, we won't do anything. """ value = self._check_or_enforce_type(value) - # Mark the field as dirty and update the cache: - self._mark_dirty(xblock, EXPLICITLY_SET) - self._set_cached_value(xblock, value) + cached_value = self._get_cached_value(xblock) + try: + value_has_changed = cached_value != value + except Exception: # pylint: disable=broad-except + # if we can't compare the values for whatever reason + # (i.e. timezone aware and unaware datetimes), just reset the value. + value_has_changed = True + if value_has_changed: + # Mark the field as dirty and update the cache + self._mark_dirty(xblock, EXPLICITLY_SET) + self._set_cached_value(xblock, value) def __delete__(self, xblock): """ diff --git a/xblock/mixins.py b/xblock/mixins.py index 04f282bbe..5dd261d50 100644 --- a/xblock/mixins.py +++ b/xblock/mixins.py @@ -7,6 +7,7 @@ import inspect import logging from lxml import etree +import copy try: import simplesjson as json # pylint: disable=F0401 @@ -244,33 +245,45 @@ def save(self): if not self._dirty_fields: # nop if _dirty_fields attribute is empty return + + fields_to_save = self._get_fields_to_save() + if fields_to_save: + self.force_save_fields(fields_to_save) + + def force_save_fields(self, field_names): + """ + Save all fields that are specified in `field_names`, even + if they are not dirty. + """ + fields = [self.fields[field_name] for field_name in field_names] + fields_to_save_json = {} + for field in fields: + fields_to_save_json[field.name] = field.to_json(self._field_data_cache[field.name]) + try: - fields_to_save = self._get_fields_to_save() # Throws KeyValueMultiSaveError if things go wrong - self._field_data.set_many(self, fields_to_save) - + self._field_data.set_many(self, fields_to_save_json) except KeyValueMultiSaveError as save_error: - saved_fields = [field for field in self._dirty_fields if field.name in save_error.saved_field_names] + saved_fields = [field for field in fields if field.name in save_error.saved_field_names] for field in saved_fields: # should only find one corresponding field - del self._dirty_fields[field] - raise XBlockSaveError(saved_fields, self._dirty_fields.keys()) + fields.remove(field) + # if the field was dirty, delete from dirty fields + self._reset_dirty_field(field) + raise XBlockSaveError(saved_fields, fields) # Remove all dirty fields, since the save was successful - self._clear_dirty_fields() + for field in fields: + self._reset_dirty_field(field) def _get_fields_to_save(self): """ - Create dictionary mapping between dirty fields and data cache values. - A `field` is an instance of `Field`. + Get an xblock's dirty fields. """ - fields_to_save = {} - for field in self._dirty_fields.keys(): - # If the field value isn't the same as the baseline we recorded - # when it was read, then save it - if field._is_dirty(self): # pylint: disable=protected-access - fields_to_save[field.name] = field.to_json(self._field_data_cache[field.name]) - return fields_to_save + # If the field value isn't the same as the baseline we recorded + # when it was read, then save it + # pylint: disable=protected-access + return [field.name for field in self._dirty_fields if field._is_dirty(self)] def _clear_dirty_fields(self): """ @@ -278,6 +291,15 @@ def _clear_dirty_fields(self): """ self._dirty_fields.clear() + def _reset_dirty_field(self, field): + """ + Resets dirty field value with the value from the field data cache. + """ + if field in self._dirty_fields: + self._dirty_fields[field] = copy.deepcopy( + self._field_data_cache[field.name] + ) + def __repr__(self): # `ScopedStorageMixin` obtains the `fields` attribute from the `ModelMetaclass`. # Since this is not understood by static analysis, silence this error. diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index aad116e7e..08dc05633 100644 --- a/xblock/test/test_core.py +++ b/xblock/test/test_core.py @@ -640,7 +640,8 @@ class MutableTester(XBlock): # Now test after having explicitly set the field. mutable_test.save() - assert_equals(len(mutable_test._dirty_fields), 0) + # _dirty_fields shouldn't be cleared here + assert_equals(len(mutable_test._dirty_fields), 1) _test_get = mutable_test.list_field assert_equals(len(mutable_test._dirty_fields), 1) diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index 45110eac8..fb6b80147 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -27,7 +27,7 @@ ) from xblock.test.tools import ( - assert_equals, assert_not_equals, assert_in, assert_not_in, assert_false, assert_true, TestRuntime + assert_equals, assert_not_equals, assert_in, assert_not_in, assert_false, TestRuntime ) from xblock.fields import scope_key, ScopeIds @@ -514,6 +514,23 @@ def test_values_dict(): assert_equals({"min": 1, "max": 100}, test_field.values) +def test_set_incomparable_fields(): + # if we can't compare a field's value to the value it's going to be reset to + # (i.e. timezone aware and unaware datetimes), just reset the value. + + class FieldTester(XBlock): + """Test block for this test.""" + incomparable = Field(scope=Scope.settings) + + not_timezone_aware = dt.datetime(2015, 1, 1) + timezone_aware = dt.datetime(2015, 1, 1, tzinfo=pytz.UTC) + runtime = TestRuntime(services={'field-data': DictFieldData({})}) + field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds)) + field_tester.incomparable = not_timezone_aware + field_tester.incomparable = timezone_aware + assert_equals(field_tester.incomparable, timezone_aware) + + def test_twofaced_field_access(): # Check that a field with different to_json and from_json representations # persists and saves correctly. @@ -548,8 +565,8 @@ class FieldTester(XBlock): def test_setting_the_same_value_marks_field_as_dirty(): """ - Check that setting field to the same value does not mark mutable fields as dirty. - This might be an unexpected behavior though + Check that setting field to the same value marks mutable fields as dirty. + However, since the value hasn't changed, these fields won't be saved. """ class FieldTester(XBlock): """Test block for set - get test.""" @@ -570,13 +587,13 @@ class FieldTester(XBlock): field_tester.list_field = field_tester.list_field field_tester.dict_field = field_tester.dict_field - assert_in(field_tester.fields['non_mutable'], field_tester._dirty_fields) + assert_not_in(field_tester.fields['non_mutable'], field_tester._dirty_fields) assert_in(field_tester.fields['list_field'], field_tester._dirty_fields) assert_in(field_tester.fields['dict_field'], field_tester._dirty_fields) - assert_true(field_tester.fields['non_mutable'].is_set_on(field_tester)) - assert_true(field_tester.fields['list_field'].is_set_on(field_tester)) - assert_true(field_tester.fields['dict_field'].is_set_on(field_tester)) + assert_false(field_tester.fields['non_mutable'].is_set_on(field_tester)) + assert_false(field_tester.fields['list_field'].is_set_on(field_tester)) + assert_false(field_tester.fields['dict_field'].is_set_on(field_tester)) class SentinelTest(unittest.TestCase): diff --git a/xblock/test/test_fields_api.py b/xblock/test/test_fields_api.py index 3899d3b6f..4be8b2e25 100644 --- a/xblock/test/test_fields_api.py +++ b/xblock/test/test_fields_api.py @@ -163,12 +163,26 @@ def test_delete_with_save_writes(self): assert_false(self.field_data.has(self.block, 'field')) assert_true(self.is_default()) - def test_set_after_get_always_saves(self): + def test_set_after_get_always_force_saves(self): with patch.object(self.field_data, 'set_many') as patched_set_many: self.set(self.get()) + + self.block.force_save_fields(['field']) + + patched_set_many.assert_called_with( + self.block, {'field': self.get()} + ) + + def test_set_after_get_doesnt_save(self): + with patch.object(self.field_data, 'set_many') as patched_set_many: + + self.set(self.get()) self.block.save() + assert_false(patched_set_many.called) - patched_set_many.assert_called_with(self.block, {'field': self.get()}) + self.set(self.new_value) + self.block.save() + assert_true(patched_set_many.called) class MutationProperties(object): @@ -206,6 +220,27 @@ def test_mutation_without_save_makes_non_default(self): self.mutate(self.get()) assert_false(self.is_default()) + def test_mutate_pointer_after_save(self): + pointer = self.get() + self.mutate(pointer) + self.block.save() + assert_equals(pointer, self.field_data.get(self.block, 'field')) + + # now check what happens when we mutate a field + # that we haven't retrieved through __get__ + # (which would have marked it as dirty) + self.mutate(pointer) + self.block.save() + assert_equals(pointer, self.field_data.get(self.block, 'field')) + + def test_set_save_mutate_save(self): + pointer = self.new_value + self.set(pointer) + self.block.save() + self.mutate(pointer) + self.block.save() + assert_equals(pointer, self.field_data.get(self.block, 'field')) + class InitialValueProperties(object): """