Skip to content
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

Fix the error type for missing attributes and getattr compatibility issues #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fjxmlzn
Copy link

@fjxmlzn fjxmlzn commented Aug 19, 2022

The library gives incorrect behavior with getattr:

>>> from addict import Dict
>>> body = Dict(a=1)
>>> body.freeze()
>>> getattr(body, 'missing', 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict.py", line 67, in __getattr__
    return self.__getitem__(item)
  File ".../addict/addict.py", line 71, in __missing__
    raise KeyError(name)
KeyError: 'missing'

The expected result should be 2 (when the attribute does not exist, the default value 2 should be returned).

The error type for missing attributes is not consistent with Python standards:

>>> body = Dict()
>>> body.freeze()
>>> body.missing_key
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict.py", line 67, in __getattr__
    return self.__getitem__(item)
  File ".../addict/addict.py", line 71, in __missing__
    raise KeyError(name)
KeyError: 'missing_key'

The correct error type should be AttributeError.

These issues are all from the same root cause: when the Dict object is frozen and a missing attribute is accessed, AttributeError should be raised (instead of KeyError). getattr uses AttributeError to detect if the default value should be supplied. When KeyError is raised instead, getattr will not supply the default value.

This pull request fixes these issues and adds related tests.

In more detail, the changes are:

Error for body.missing_key

Before:

>>> body = Dict()
>>> body.freeze()
>>> body.missing_key
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict.py", line 67, in __getattr__
    return self.__getitem__(item)
  File ".../addict/addict.py", line 71, in __missing__
    raise KeyError(name)
KeyError: 'missing_key'

However, the error type should be AttributeError instead. This pull request fixes this issue by catching it and throwing the correct error type:

>>> body = Dict()
>>> body.freeze()
>>> body.missing_key
Traceback (most recent call last):
  File ".../addict/addict/addict.py", line 74, in __getattr__
    return self.__getitem__(item)
  File ".../addict/addict/addict.py", line 82, in __missing__
    raise KeyError(name)
KeyError: 'missing_key'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict/addict.py", line 77, in __getattr__
    raise AttributeError("'{}' object has no attribute '{}'".format(
AttributeError: 'Dict' object has no attribute 'missing_key'

Error for body["missing_key"]

The error type for missing key access is still the same as before (KeyError) as this is the correct behavior:

>>> body["missing_key"]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict/addict.py", line 82, in __missing__
    raise KeyError(name)
KeyError: 'missing_key'

getattr

Before:

>>> body = Dict(a=1)
>>> body.freeze()
>>> getattr(body, 'missing', 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../addict/addict.py", line 67, in __getattr__
    return self.__getitem__(item)
  File ".../addict/addict.py", line 71, in __missing__
    raise KeyError(name)
KeyError: 'missing'

The expected result should be 2. This pull request fixes the issue:

>>> body = Dict(a=1)
>>> body.freeze()
>>> getattr(body, 'missing', 2)
2

Note: This pull request is to replace #145 with a better implementation: #145 throws AttributeError for missing key access (body['missing_key']), but this pull request throws KeyError to be consistent with Python standards.

@fjxmlzn
Copy link
Author

fjxmlzn commented Aug 29, 2022

Gentle ping for this. One of my projects that uses addict depends on this fix. Thank you! @mewwts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant