-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Added protections to AttributeErrors raised within properties #9455
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
Added protections to AttributeErrors raised within properties #9455
Conversation
0476816
to
845d2c4
Compare
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.
Did a timing analysis on request.data
similar to the one made here, and observed a time increase.
Timings | requests.data |
---|---|
before | 0.022 s |
after | 0.106 s |
I don't think that @safe_property
should be used on every property.
Using with wrap_attributeerrors:
to the data
should be sufficient IMO.
Good call running the timings. I didn't realize a custom property could cause that much of a slow down. That's good to know. I'll make the necessary updates.
As for which properties need it, it could definitely be dropped for some of the simple properties like I think the custom property is useful, as it creates a repeatable pattern for how properties can be safely created in |
b6084b1
to
fca7e6b
Compare
@reddytocode Thank you for the review. I've dropped the I also moved the unit test under the Please let me know if any additional updates are needed |
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.
pls squash.
Signed-off-by: James Riley McHugh <[email protected]>
fca7e6b
to
baf9a1f
Compare
Done! |
Just in case you weren't aware, Github allows projects to change the merge strategies used for PRs. For some of the projects I maintain, we've disabled merge methods other than squash, making it so all PR branches are automatically squashed when merged. You can configure it to pull the squashed commit message from the PR title and description too. It's pretty handy. I definitely recommend checking it out if you guys haven't and if this is a common requirement for PRs. |
I know the feature, but thanks for pointing it out. The reason for my squashing request was rather that the previous commits make changes that were undone one or two commits later, and it was difficult to review. |
Oh, interesting. Do you typically review PRs by looking at individual commits rather than the overall diff? I typically just look at the overall diff, but I'm wondering if I should start paying closer attention to individual commits as well. Also, please let me know if any additional changes are needed. @auvipy has approved, but it looks like your request for changes is still preventing the PR from being merged. Please let me know if additional changes are needed besides squashing the commits. |
Some PRs are larger, and some people organize them such that each commit is a mental block, and that aids comprehension during reviewing things one by one. It kinda works if a commit contains to mental block (e.g. a test and a docs change), but it does not work so well if one commit undoes changes of a previous one. Also, reviewing the combination only allows sneaking unreviewed code into the codebase (although not at the HEAD of the branch), and if the merge doesn't squash (which I don't usually assume), then those unreviewed commits show up in the main branch history. For these two reasons, I like to review commits explicitly. But I get it that that's a matter of taste, and others may do it differently.
Sorry! Approved. |
Description of Changes
Updates the
Request
class'sdata
,FILES
, andPOST
properties to utilize thewrap_attributeerrors
context manager to ensure any properties that run_load_data_and_files
in theRequest
class properly handle anyAttributeError
s raised internally.Updates the
__getattr__
function to explicitly raise anAttributeError
rather than calling__getattribute__
again, as calling__getattribute__
can have unexpected side effects (see #9433).Adds a unit test to cover the case where a Parser raises an attribute error when the
Request.data
property is accessed.Benefits
AttributeError
s raised in properties such as from parsing will now produce sane error messages and will no longer result in errors being suppressed.Possible drawbacks
None that I am currently aware of.
Applicable issues
Additional information
There appears to already be a precedent for this type of handling in request.py with the
wrap_attributeerrors
context manager that is currently only used for authentication. This extends that behavior to other properties via a commonsafe_property
decorator that can be used instead of the builtinproperty
decorator.I had originally utilized a
safe_property
decorator to automatically add safe attribute handling to all properties, but that a significant performance penalty when accessing the properties as pointed out by @reddytocode. It has been dropped.