use new style middleware and catch exceptions to be sure to clear request in context #1188
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
also, catch exceptions to be sure to clear request in context
ddabble
force-pushed
the
new-style-middleware
branch
from
ec41355
to
c9c02ae
Compare
Codecov Report
@@ Coverage Diff @@
## master #1188 +/- ##
==========================================
+ Coverage 97.30% 97.38% +0.08%
==========================================
Files 23 23
Lines 1260 1263 +3
Branches 204 204
==========================================
+ Hits 1226 1230 +4
Misses 16 16
+ Partials 18 17 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Added a changelog entry, and removed the "Using django-webtest with Middleware" section in `common_issues.rst`, as it's not an issue anymore now that `HistoricalRecords.context.request` is deleted in all cases (due to using a `finally` block).
|
@manelclos I added an exception variable to your code, in addition to adding a test and updating the documentation 🙂 |
This code was added in 341aec1 - when `HistoryRequestMiddleware` didn't delete the `HistoricalRecords.context.request` variable. The middleware class does now, so this can be removed.
ddabble
previously approved these changes
Aug 3, 2023
11 tasks
|
Hi @ddabble all good! I didn't notice the error was documented already. Thanks for adding the tests and documentation. |
Now it also tests that the `request` attribute of `HistoricalRecords.context` actually exists while handling each request.
ddabble
approved these changes
Aug 4, 2023
ddabble
added a commit
that referenced
this pull request
Sep 15, 2023
* Handle if `request` doesn't exist on the context in middleware 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. * Add changelog entry * Improvement of changelog for #1256 Also added a missing reference to the previously merged PR. --------- Co-authored-by: Anders <[email protected]>
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
We caught a problem were the request was reused, causing a strange random behaviour:
Related Issue
Resolves #1189.
How Has This Been Tested?
Extensive pytest codebase using django-simple-history
Fixes the problem we were facing
Types of changes
Checklist:
pre-commit runcommand to format and lint.AUTHORS.rstCHANGES.rst