Do not serialize all attributes for updates and deletes #905
Conversation
| @@ -474,34 +461,51 @@ def refresh(self, consistent_read: bool = False, settings: OperationSettings = O | |||
| raise ValueError("Cannot refresh this item from the returned class: {}".format(stored_cls.__name__)) | |||
| self.deserialize(item_data) | |||
|
|
|||
| def get_operation_kwargs_from_instance( | |||
There was a problem hiding this comment.
break into 3 variants even at the cost of slight repetition
| @@ -928,16 +932,14 @@ def _get_indexes(cls): | |||
| cls._indexes['local_secondary_indexes'].append(idx) | |||
| return cls._indexes | |||
|
|
|||
| def _get_save_args(self, attributes=True, null_check=True): | |||
There was a problem hiding this comment.
The attributes flag is no longer needed (always True).
| return args, kwargs | ||
|
|
||
| def _handle_version_attribute(self, serialized_attributes, actions=None): | ||
| def _get_hash_range_key_serialized_values(self) -> Tuple[Any, Optional[Any]]: |
There was a problem hiding this comment.
This method replaces _get_save_args for non-save scenarios by serializing only the HK and RK.
| if not TestModel.exists(): | ||
| print("Creating table") | ||
| TestModel.create_table(read_capacity_units=1, write_capacity_units=1, wait=True) | ||
| if TestModel.exists(): |
There was a problem hiding this comment.
Clean table between tests
ikonst
changed the title
| elif 'range_key' in save_kwargs: | ||
| kwargs['range_key'] = save_kwargs['range_key'] | ||
| return self._get_connection().get_operation_kwargs(*args, **kwargs) | ||
| return self._get_connection().get_operation_kwargs(hk_value, range_key=rk_value, key=KEY, actions=actions, condition=condition, return_values_on_condition_failure=return_values_on_condition_failure) |
There was a problem hiding this comment.
I've really been meaning to setup black to lint 😅
There was a problem hiding this comment.
So I'll withhold formatting cause there's no point.
| def _handle_version_attribute(self, serialized_attributes, actions=None): | ||
| def _get_hash_range_key_serialized_values(self) -> Tuple[Any, Optional[Any]]: | ||
| if self._hash_keyname is None: | ||
| raise Exception("The model has no hash key") |
There was a problem hiding this comment.
Seems like precedent is a ValueError -
Line 304 in 6d6a8be
There was a problem hiding this comment.
Considered that, but in that precedent it actually relates to an argument that was passed in.
also lol {range_key}
|
🚀 |
Not only it is more efficient not to serialize attributes you don't need, it also manifested in an issue where an
updateoperation would fail null-checks on attributes that are not even affected by the update -- see addedtest_update_doesnt_do_validation_on_null_attributestest.p.s. this uncovered another bug where we fail to propagate
null_checks=Truethrough MapAttributes, but we'll address it separately