-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement recursive type checking in PynamoDB #601
base: master
Are you sure you want to change the base?
Conversation
- MapAttribute and Model share the same serialization and type check logic - Add new serialization to MapAttribute, while still keeps the old one - Replace Model serialization and type check with the new serialization
pynamodb/attributes.py
Outdated
attrs = {} | ||
for name, attr in attr_container.get_attributes().items(): | ||
value = getattr(attr_container, name) | ||
if isinstance(value, MapAttribute) and type(value) != MapAttribute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!=
-> is not
?
pynamodb/attributes.py
Outdated
@classmethod | ||
def _serialize_container(cls, attr_container, null_check=True): | ||
""" | ||
Retrive attribute container recursively and do null check at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve
pynamodb/models.py
Outdated
@@ -1318,17 +1318,12 @@ def _serialize(self, attr_map=False, null_check=True): | |||
:param null_check: If True, then attributes are checked for null | |||
""" | |||
attributes = pythonic(ATTRIBUTES) | |||
# use the new recursive type check and serialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - this comment will become obsolete once we merge
pynamodb/attributes.py
Outdated
serialized = cls._serialize_container(value, null_check) | ||
else: | ||
serialized = cls._serialize_value(attr, value, null_check) | ||
if NULL in serialized: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - I know it's been even worse in the original code, but we can do:
if NULL not in serialized:
attrs[name] = serialized
pynamodb/attributes.py
Outdated
continue | ||
|
||
rval[attr_name] = {attr_key: serialized} | ||
# if not raw MapAttribute, use the new recursive type check and serialization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re "new" - comment will be obsolete once merged
pynamodb/attributes.py
Outdated
rval[attr_name] = {attr_key: serialized} | ||
# if not raw MapAttribute, use the new recursive type check and serialization | ||
if not self.is_raw(): | ||
rval = self._serialize_container(values, null_check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - rval =
—> return
and then we can save on indent for the next lines
@@ -145,7 +145,6 @@ class MapAttribute(Generic[_KT, _VT], Attribute[Mapping[_KT, _VT]], metaclass=Ma | |||
@overload | |||
def __get__(self: _MT, instance: Any, owner: Any) -> _MT: ... | |||
def is_type_safe(self, key: Any, value: Any) -> bool: ... | |||
def validate(self) -> bool: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we changing API contract here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, what do you suggest here? recursive check as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because the old same level check is no longer used, but can definitely add it back if it changes the behavior too much for certain users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate
and is_type_safe
do not start with an underscore, so they're part of the API. It's hard to say how much they're used in practice, but we should probably bump the major version if we remove them. Is it possible to separate that change from what you're doing? Is the previous way, where this method was used during serialization, not suitable anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably have been made private from the beginning, but agreed that we don't want to break the API contract. We can mark these for removal in 5.0, but let's maintain their behavior for now (even if we move the implementation elsewhere)
thanks for the review, gonna update. |
ok, I didn't do a functional review yet, just nitpicking :) |
np, this is a big change. Just figure out I need to support ListAttribute's input as a list of dict as well. |
Going to sleep. We can sync tomorrow. |
- it is not about the cls, it is the values
Just add the change to make it work for LIstAttribute of raw MapAttributes inputed as list of dict. Now it works on list of dicts as well
|
Also how can I run all the test cases locally? Seems like some need a bit of setup. |
but both so now serialization pass in both attribute type and its value, and when a MapAttribute is passed in as a dict, we can still use its class's metadata to check for the values
I think I fixed all issues on top of my head, so I'm gonna stop committing and wait for suggestions. |
Passed all test cases. |
@@ -7,4 +7,4 @@ | |||
""" | |||
__author__ = 'Jharrod LaFon' | |||
__license__ = 'MIT' | |||
__version__ = '3.3.3' | |||
__version__ = '3.3.13' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this?
@@ -145,7 +145,6 @@ class MapAttribute(Generic[_KT, _VT], Attribute[Mapping[_KT, _VT]], metaclass=Ma | |||
@overload | |||
def __get__(self: _MT, instance: Any, owner: Any) -> _MT: ... | |||
def is_type_safe(self, key: Any, value: Any) -> bool: ... | |||
def validate(self) -> bool: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably have been made private from the beginning, but agreed that we don't want to break the API contract. We can mark these for removal in 5.0, but let's maintain their behavior for now (even if we move the implementation elsewhere)
@@ -143,6 +142,7 @@ def commit(self): | |||
delete_items=delete_items | |||
) | |||
unprocessed_items = data.get(UNPROCESSED_ITEMS, {}).get(self.model.Meta.table_name) | |||
self.pending_operations = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this? Looks like it may introduce a bug when data is None
after the batch write
if not self.is_raw(): | ||
return type(self)(**deserialized_dict) | ||
return deserialized_dict | ||
if values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need this check?
@@ -272,6 +272,56 @@ def _set_attributes(self, **attributes): | |||
raise ValueError("Attribute {0} specified does not exist".format(attr_name)) | |||
setattr(self, attr_name, attr_value) | |||
|
|||
@staticmethod | |||
def _serialize_value(attr, value, null_check=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add type stubs for these methods in the .pyi
file?
else: | ||
_null_check = null_check | ||
if value is None and attr.default is not None: | ||
if attr.default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check necessary given the above line? Wouldn't it break defaults of []
or False
?
_null_check = null_check | ||
if value is None and attr.default is not None: | ||
if attr.default: | ||
value = attr.default() if callable(attr.default) else attr.default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind assigning defaults here? Why do we need a second place to do this (the first being _set_defaults
)?
WOW, I was wondering if this PR will ever be looked at. I will look into this again when I have time. We are still using a forked 3.x version for our product. |
Currently, MapAttribute (even for non-raw MapAttribute) and Model use different serialization and type check logic, and that creates a lot of unexpected behavior especially when it comes to nested data structures such as nested ListAttribute and MapAttribute. This commit proposes changes to move serialization and type check to AttributeContainer, and replace or add that logic to MapAttribute and Model to make the behavior consistent between the two.
Changes
Old behavior
New behavior
Optional TODO for next step
Runtime Test
Interestingly, the old ListAttribute example doesn't even work, but now it does.
https://pynamodb.readthedocs.io/en/latest/attributes.html#list-attributes
Assigned as a list of dicts
Normal case