Handle missing request #1256
Merged
Handle missing request #1256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The change in #1188 removed a check for if the request context had a "request" entry in it before trying to delete the request data. This has a side effect of breaking tests in order projects in very specific circumstances, where multiple requests pass through the middleware, concurrently, inside the same thread, as it means the middleware then may try to remove the request key on a context where it already has been removed. This shouldn't have any impact in real life scenarios.
Also added a missing reference to the previously merged PR.
Codecov Report
@@ Coverage Diff @@
## master #1256 +/- ##
==========================================
- Coverage 97.09% 96.94% -0.16%
==========================================
Files 23 23
Lines 1272 1275 +3
Branches 209 209
==========================================
+ Hits 1235 1236 +1
- Misses 19 21 +2
Partials 18 18
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Thanks! |
|
Any idea when a new release might be cut that includes this update? |
|
@Tenzer This is now included in version 3.5.0 🙂 |
|
Great, thanks! I noticed earlier today and have upgraded to the latest version already. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The change in #1188 removed a check for if the request context had a "request" entry in it before trying to delete the request data.
Motivation and Context
This has a side effect of breaking tests in order projects in very specific circumstances, where multiple requests pass through the middleware, concurrently, inside the same thread, as it means the middleware then may try to remove the request key on a context where it already has been removed.
In our case we make us of before_after to test some behaviours that are specific to concurrent requests, which allows us to execute concurrent requests inside one test. In those cases the test have started failing with django-simple-history 3.4.0 because an
AttributeErroris raised when the second request passes back up through the middleware, and tries to delete the request data after it already has been done as part of the first request.This shouldn't have any impact in real-life scenarios.
How Has This Been Tested?
I have run our test suite (for proprietary code) before and after this change to verify it no longer raises the
AttributeErrorand our test suite passes.Types of changes
Checklist:
pre-commit runcommand to format and lint.AUTHORS.rstCHANGES.rst