Skip to content

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

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

james-mchugh
Copy link
Contributor

@james-mchugh james-mchugh commented Jun 29, 2024

Description of Changes

Updates the Request class's data, FILES, and POST properties to utilize the wrap_attributeerrors context manager to ensure any properties that run _load_data_and_files in the Request class properly handle any AttributeErrors raised internally.

Updates the __getattr__ function to explicitly raise an AttributeError 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

AttributeErrors 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 common safe_property decorator that can be used instead of the builtin property 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.

Copy link

@reddytocode reddytocode left a 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.

@james-mchugh
Copy link
Contributor Author

james-mchugh commented Jul 7, 2024

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

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.

I don't think that @safe_property should be used on every property. Using with wrap_attributeerrors: to the data should be sufficient IMO.

As for which properties need it, it could definitely be dropped for some of the simple properties like content_type. However, any property that runs _load_data_and_files, which would include data, POST, and FILESis susceptible to the original issue. If we want to drop the property, I could just explicitly add the context manager to those.

I think the custom property is useful, as it creates a repeatable pattern for how properties can be safely created in Request class without having to manage the pitfalls of using __getattr__ and property together for each property individually, but it's not worth it if it slows things down so much.

@james-mchugh james-mchugh force-pushed the fix-request-attribute-handling branch 2 times, most recently from b6084b1 to fca7e6b Compare July 7, 2024 04:46
@james-mchugh
Copy link
Contributor Author

@reddytocode Thank you for the review. I've dropped the safe_property decorator and updated the properties that call _load_data_and_files to use the wrap_attributeerrors contextmanager.

I also moved the unit test under the TestContentParsing class.

Please let me know if any additional updates are needed

@auvipy auvipy requested a review from a team July 7, 2024 07:23
Copy link
Contributor

@peterthomassen peterthomassen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls squash.

@james-mchugh james-mchugh force-pushed the fix-request-attribute-handling branch from fca7e6b to baf9a1f Compare July 8, 2024 13:13
@james-mchugh
Copy link
Contributor Author

pls squash.

Done!

@james-mchugh
Copy link
Contributor Author

pls squash.

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.

@peterthomassen
Copy link
Contributor

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.

@james-mchugh
Copy link
Contributor Author

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.

@peterthomassen
Copy link
Contributor

Oh, interesting. Do you typically review PRs by looking at individual commits rather than the overall diff?

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.

@auvipy has approved, but it looks like your request for changes is still preventing the PR from being merged.

Sorry! Approved.

@auvipy auvipy merged commit 8e304e1 into encode:master Jul 17, 2024
7 checks passed
@browniebroke browniebroke added this to the 3.16 milestone Jan 16, 2025
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.

Attribute Errors raised in Parser Silently Ignored
5 participants