Summary

Update djblets to support MIDDLEWARE setting.

Review Request #11910 — Created Jan. 7, 2022 and submitted Jan. 12, 2022, 9:18 a.m.

Information

Owner

david

Repository

Djblets

Branch

release-2.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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.

Testing Done


Ran unit tests.
Used djblets middleware (logging, settings) with Review Board.

Commits

Summary	ID
Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	4c140ad76253c5a0b6ff6ab385641aa6bc5f2e19

Summary

Update djblets to support MIDDLEWARE setting.

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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.

4c140ad76253c5a0b6ff6ab385641aa6bc5f2e19

Issues

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 …	chipx86	Jan. 10, 2022, 12:23 p.m.
Can you add a "Version Added" to each of these?	chipx86	Jan. 10, 2022, 12:27 p.m.
We should use our deprecation classes. We can probably deprecate for.. Djblets 4.0?	chipx86	Jan. 10, 2022, 12:46 p.m.
Let's use +=. Not that it really matters here, but it's technically faster.	chipx86	Jan. 10, 2022, 12:24 p.m.
Let's use += here as well.	chipx86	Jan. 10, 2022, 12:24 p.m.
Can we use += here?	chipx86	Jan. 10, 2022, 12:25 p.m.
Let's move to +=.	chipx86	Jan. 10, 2022, 12:25 p.m.
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 …	chipx86	Jan. 10, 2022, 12:26 p.m.
F401 'djblets.deprecation.RemovedInDjblets30Warning' imported but unused	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:04 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:04 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:04 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'extension' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
F841 local variable 'response' is assigned to but never used	reviewbot	Jan. 10, 2022, 1:02 p.m.
import should go before from.	chipx86	Jan. 10, 2022, 10:15 p.m.
I have a change for the rest of the codebase coming, but we'll need to avoid prefixing any non-test suite …	chipx86	Jan. 10, 2022, 10:15 p.m.
Can we explicitly say "Testing new-style extension middleware ..." and such in these docstrings?	chipx86	Jan. 10, 2022, 10:16 p.m.

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

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.

david Jan. 10, 2022, 12:52 p.m.

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.

djblets/extensions/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add a "Version Added" to each of these?
```

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

The issue has been resolved. Show all issues

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

david Jan. 10, 2022, 12:52 p.m.

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.

djblets/extensions/manager.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use +=. Not that it really matters here, but it's technically faster.
```
djblets/extensions/manager.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use += here as well.
```
djblets/extensions/middleware.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we use += here?
```
djblets/extensions/middleware.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's move to +=.
```

djblets/integrations/manager.py (Diff revision 1)

The issue has been resolved. Show all issues

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:

	Summary	ID
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: Ran unit tests.	df8f729cab83605160b3da1be72bfee864875282
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	dc088403e01e61e734437e6d05322b382d5c7769

Diff:

Revision 2 (+788 -74)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/manager.py
	djblets/extensions/middleware.py
	djblets/extensions/tests/test_middleware.py
	djblets/http/middleware.py
	djblets/integrations/manager.py
	djblets/integrations/middleware.py
	djblets/integrations/tests/test_manager.py
	3 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F401 'djblets.deprecation.RemovedInDjblets30Warning' imported but unused
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'extension' is assigned to but never used
```
djblets/extensions/tests/test_middleware.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F841 local variable 'response' is assigned to but never used
```

Commits:

	Summary	ID
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	dc088403e01e61e734437e6d05322b382d5c7769
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	0dbcf3d33c62a70154cbf96def6c70e696395d1f

Diff:

Revision 3 (+786 -74)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/manager.py
	djblets/extensions/middleware.py
	djblets/extensions/tests/test_middleware.py
	djblets/http/middleware.py
	djblets/integrations/manager.py
	djblets/integrations/middleware.py
	djblets/integrations/tests/test_manager.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix an issue with init_js_extensions that was caused by RB's check_updates_required_middleware

Commits:

	Summary	ID
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	0dbcf3d33c62a70154cbf96def6c70e696395d1f
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	33b1a30d1b05dd77102f2871cb7d2078de0326fc

Diff:

Revision 4 (+802 -74)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/manager.py
	djblets/extensions/middleware.py
	djblets/extensions/templatetags/djblets_extensions.py
	djblets/extensions/tests/test_middleware.py
	djblets/http/middleware.py
	djblets/integrations/manager.py
	djblets/integrations/middleware.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

djblets/extensions/tests/test_middleware.py (Diff revision 4)
The issue has been resolved. Show all issues
```
import should go before from.
```

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

Commits:

	Summary	ID
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	33b1a30d1b05dd77102f2871cb7d2078de0326fc
	Update djblets to support MIDDLEWARE setting. 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. Testing Done: - Ran unit tests. - Used djblets middleware (logging, settings) with Review Board.	4c140ad76253c5a0b6ff6ab385641aa6bc5f2e19

Diff:

Revision 5 (+814 -74)

Show changes

	djblets/extensions/extension.py
	djblets/extensions/manager.py
	djblets/extensions/middleware.py
	djblets/extensions/templatetags/djblets_extensions.py
	djblets/extensions/tests/test_middleware.py
	djblets/http/middleware.py
	djblets/integrations/manager.py
	djblets/integrations/middleware.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.x (d6b7fca)