Update djblets to support MIDDLEWARE setting.
Review Request #11910 — Created Jan. 7, 2022 and submitted
Information | |
---|---|
david | |
Djblets | |
release-2.x | |
Reviewers | |
djblets | |
In 1.10, Django changed the way middleware works. Several of the
built-in methods were coalesced together into a callable that processes
the request, delegates to other middleware, and then processes the
response. In order to distinguish between the invocation methods, they
renamed the setting fromMIDDLEWARE_CLASSES
toMIDDLEWARE
.For most of our middleware classes, we can just inherit from the new
MiddlewareMixin
class. This allows middleware classes to work
correctly no matter which version of Django they're using.The big wrinkle here was in extension middleware. To start with, I'm
pretty sure that nobody is using it (our documentation claims that the
middleware
attribute should be a list of module paths to the
middleware classes to match the setting, but the implementation was
expecting a list of classes themselves). More importantly, the way that
they were processed (instantiating the classes at init time and then
calling the oldprocess_*
methods) wouldn't work with new-style
middleware.I've added some detection to figure out whether each entry in the list
is old- or new-style, and then depending on what the setting is, do the
right thing. We can run old-style middleware with the new setting, but
can't run new-style middleware with the old setting. Appropriate
warnings have been added in these situations. This includes new test
cases to check that the various different combinations run the
middleware as expected.
- Ran unit tests.
- Used djblets middleware (logging, settings) with Review Board.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
Is this meant to be on release-2.x or for 3.x? Branch says 2.x, but I assume 3.x. The 2.x branch … |
|
|
Can you add a "Version Added" to each of these? |
|
|
We should use our deprecation classes. We can probably deprecate for.. Djblets 4.0? |
|
|
Let's use +=. Not that it really matters here, but it's technically faster. |
|
|
Let's use += here as well. |
|
|
Can we use += here? |
|
|
Let's move to +=. |
|
|
On 1.6, settings.MIDDLEWARE doesn't exist, so it'll fail there. If this is scheduled for release-2.x, as stated in Branch. I … |
|
|
F401 'djblets.deprecation.RemovedInDjblets30Warning' imported but unused |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
F841 local variable 'extension' is assigned to but never used |
![]() |
|
F841 local variable 'response' is assigned to but never used |
![]() |
|
import should go before from. |
|
|
I have a change for the rest of the codebase coming, but we'll need to avoid prefixing any non-test suite … |
|
|
Can we explicitly say "Testing new-style extension middleware ..." and such in these docstrings? |
|
-
-
Is this meant to be on
release-2.x
or for 3.x? Branch says 2.x, but I assume 3.x. The 2.x branch still has compatibility with Django 1.6 (well, in theory) and won't haveMiddlwareMixin
. -
-
djblets/extensions/extension.py (Diff revision 1) We should use our deprecation classes. We can probably deprecate for.. Djblets 4.0?
-
djblets/extensions/manager.py (Diff revision 1) Let's use
+=
. Not that it really matters here, but it's technically faster. -
-
-
-
djblets/integrations/manager.py (Diff revision 1) On 1.6,
settings.MIDDLEWARE
doesn't exist, so it'll fail there. If this is scheduled for release-2.x, as stated in Branch.I think we can make this logic a lot more simple and safe:
if (middleware not in getattr(settings, 'MIDDLEWARE', []) and middleware not in getattr(settings, 'MIDDLEWARE_CLASSES', [])): ...
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+788 -74) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F401 'djblets.deprecation.RemovedInDjblets30Warning' imported but unused
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'extension' is assigned to but never used
-
djblets/extensions/tests/test_middleware.py (Diff revision 2) F841 local variable 'response' is assigned to but never used
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+786 -74) |
Checks run (2 succeeded)
Change Summary:
Fix an issue with
init_js_extensions
that was caused by RB'scheck_updates_required_middleware
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+802 -74) |
Checks run (2 succeeded)
-
-
djblets/extensions/extension.py (Diff revision 4) I was going to say "Hey, let's use
RemovedInDjblets30Warning.warn()
, but apparently we never broughtwarn()
over from RB's deprecation classes.. Oops. -
-
djblets/extensions/tests/test_middleware.py (Diff revision 4) I have a change for the rest of the codebase coming, but we'll need to avoid prefixing any non-test suite classes with
Test
as a prefix. I've been changing them toMyTest
. Otherwise we get warnings inpytest
, sinceTest*
is considered a candidate for unit tests. -
djblets/extensions/tests/test_middleware.py (Diff revision 4) Can we explicitly say "Testing new-style extension middleware ..." and such in these docstrings?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+814 -74) |