Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Fix accessing builtin dict's attrs for a missing field #1557

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: dev
Choose a base branch
Loading
from
Open

Fix accessing builtin dict's attrs for a missing field #1557

wants to merge 1 commit into from

Conversation

mahenzon
Copy link
Contributor

@mahenzon mahenzon commented Apr 5, 2020 •
edited
Loading

I've faced this issue, when a schema has a field with name same as builtin dict's has attribute:

from marshmallow import fields, Schema


class MyNestedSchema(Schema):
    update = fields.Boolean(required=False)


class MySchema(Schema):
    data = fields.Nested(MyNestedSchema)


class MyObj:
    def __init__(self):
        self.data = {
            # NOT passing a value, so it's missing
            # 'update': True,
        }


def main():
    obj = MyObj()
    data = MySchema().dump(obj)
    print(data)


if __name__ == '__main__':
    main()

Fails with this:

Traceback (most recent call last):
  File "/home/khorenyan/.PyCharm2019.2/config/scratches/scratch_42.py", line 40, in <module>
    main()
  File "/home/khorenyan/.PyCharm2019.2/config/scratches/scratch_42.py", line 35, in main
    data = MySchema().dump(obj)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/schema.py", line 556, in dump
    result = self._serialize(processed_obj, many=many)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/schema.py", line 520, in _serialize
    value = field_obj.serialize(attr_name, obj, accessor=self.get_attribute)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/fields.py", line 311, in serialize
    return self._serialize(value, attr, obj, **kwargs)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/fields.py", line 566, in _serialize
    return schema.dump(nested_obj, many=many)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/schema.py", line 556, in dump
    result = self._serialize(processed_obj, many=many)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/schema.py", line 520, in _serialize
    value = field_obj.serialize(attr_name, obj, accessor=self.get_attribute)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/fields.py", line 311, in serialize
    return self._serialize(value, attr, obj, **kwargs)
  File "/home/khorenyan/projs/marshmallow/src/marshmallow/fields.py", line 1103, in _serialize
    elif value in self.truthy:
TypeError: unhashable type: 'dict'

It happens this way:

  • When being serialized, object is passed to the marshmallow.utils._get_value_for_key
  • because of the missing update field in data on the line obj[key] the KeyError exception is raised
  • and on the next line the getattr(obj, key, default) is returned

If the key is not found here, it returns default, so if my field is 'update_' or 'qwerty', it works as expected -- the result is returned and it's {'data': {}}

But the 'update' matches the builtin dict's attribute (method) so it's returned. And that's not expected at all.

Of course, I could override the get_attribute method in MyNestedSchema this way:

class MyNestedSchema(Schema):
    def get_attribute(self, obj: dict, attr: str, default: Any):
        try:
            return obj[attr]
        except KeyError:
            return default

    update = fields.Boolean(required=False)

But I think, that needs to be fixed.

I thought about checking if the getattr(obj, key, default) expression returns a method of a builtin's object, or checking a key to be one of the builtin's attrs, for example:

>>> dir(dict)
['__class__', '__contains__', '__delattr__', '__delitem__', '__dir__', '__doc__', '__eq__', '__format__', '__ge__', '__getattribute__', '__getitem__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__iter__', '__le__', '__len__', '__lt__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setitem__', '__sizeof__', '__str__', '__subclasshook__', 'clear', 'copy', 'fromkeys', 'get', 'items', 'keys', 'pop', 'popitem', 'setdefault', 'update', 'values']

I'm not sure if that's a proper way to do this check. Maybe we need to check even for isinstance instead of the object's type, but here's my current solution.

@mahenzon mahenzon force-pushed the fix-accessing-dicts-methods-on-missing branch from fa24661 to 4661e9a Compare April 5, 2020 21:53
mahenzon
mahenzon commented Apr 5, 2020
View reviewed changes
AUTHORS.rst Show resolved Hide resolved
deckar01
deckar01 reviewed Apr 6, 2020
View reviewed changes
src/marshmallow/utils.py Show resolved Hide resolved
@mahenzon mahenzon mentioned this pull request Apr 6, 2020
Open
@mahenzon
Copy link
Contributor Author

@deckar01 any ideas? it's been a long time already. If we need any changes, let's discuss them. I'm not sure about explicit types to be passed to the schemas' methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deckar01 deckar01 deckar01 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mahenzon @deckar01
Morty Proxy This is a proxified and sanitized view of the page, visit original site.