Update djblets to support MIDDLEWARE setting.

Review Request #11910 — Created Jan. 7, 2022 and submitted

david
Djblets
release-2.x
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 from MIDDLEWARE_CLASSES to MIDDLEWARE.

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 old process_* 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
Update djblets to support MIDDLEWARE setting.
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 ...

chipx86chipx86

Can you add a "Version Added" to each of these?

chipx86chipx86

We should use our deprecation classes. We can probably deprecate for.. Djblets 4.0?

chipx86chipx86

Let's use +=. Not that it really matters here, but it's technically faster.

chipx86chipx86

Let's use += here as well.

chipx86chipx86

Can we use += here?

chipx86chipx86

Let's move to +=.

chipx86chipx86

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 ...

chipx86chipx86

F401 'djblets.deprecation.RemovedInDjblets30Warning' imported but unused

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

F841 local variable 'extension' is assigned to but never used

reviewbotreviewbot

F841 local variable 'response' is assigned to but never used

reviewbotreviewbot

import should go before from.

chipx86chipx86

I have a change for the rest of the codebase coming, but we'll need to avoid prefixing any non-test suite ...

chipx86chipx86

Can we explicitly say "Testing new-style extension middleware ..." and such in these docstrings?

chipx86chipx86
chipx86
  1. Mostly some really minor nits. Otherwise it looks sane to me.

  2. 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 have MiddlwareMixin.

    1. This is for 2.x, since we need it to get RBCommons onto RB4. I can conditionally import MiddlewareMixin and have it default to object if it doesn't exist.

  3. djblets/extensions/extension.py (Diff revision 1)
     
     
     
     
     
     
     

    Can you add a "Version Added" to each of these?

  4. djblets/extensions/extension.py (Diff revision 1)
     
     

    We should use our deprecation classes. We can probably deprecate for.. Djblets 4.0?

    1. Djblets 3 will be Django 2+ only, so I think we need to use RemovedInDjblets30Warning. That said, this will require some changes to my tests, which I just noticed I forgot to add to the commit at all. So that is incoming.

  5. djblets/extensions/manager.py (Diff revision 1)
     
     
     

    Let's use +=. Not that it really matters here, but it's technically faster.

  6. djblets/extensions/manager.py (Diff revision 1)
     
     
     
     
     
     

    Let's use += here as well.

  7. djblets/extensions/middleware.py (Diff revision 1)
     
     

    Can we use += here?

  8. djblets/extensions/middleware.py (Diff revision 1)
     
     

    Let's move to +=.

  9. 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', [])):
        ...
    
  10. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
david
chipx86
  1. 
      
  2. djblets/extensions/extension.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     

    I was going to say "Hey, let's use RemovedInDjblets30Warning.warn(), but apparently we never brought warn() over from RB's deprecation classes.. Oops.

  3. djblets/extensions/tests/test_middleware.py (Diff revision 4)
     
     
     
     

    import should go before from.

  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 to MyTest. Otherwise we get warnings in pytest, since Test* is considered a candidate for unit tests.

  5. Can we explicitly say "Testing new-style extension middleware ..." and such in these docstrings?

  6. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.x (d6b7fca)
Loading...