Fixed #30237 -- Made Authentication/SessionMiddleware and ModelBackend admin checks allow subclasses. #11057
Conversation
|
I left a few comments for improvements but this would also require a regression test. They can be added in |
|
Thank you so much @charettes ! I'll add a regression test later after work. Appreciate your feedback 🚀 |
|
Hi @hermansc. Django 2.2rc1 is due next Friday. This issue is currently a release blocker for that. Can I ask, are you able to add the test in the next few days? Thanks for your efforts! |
|
Sure thing @carltongibson. I submitted a patch now, however it tests the case where no errors are found. Unsure if there are better ways of testing this? |
There was a problem hiding this comment.
Hi @hermansc. This looks good. I had just a few really small points.
Wondering if we want to adjust the check message to mention a subclass? (@charettes?)
"'django.contrib.auth.middleware.AuthenticationMiddleware (or a subclass)' "
"must be in MIDDLEWARE in order to use the admin application.",
Could you squash and adjust the commit message format?
Other than that, looks good. Thank you!
hermansc
force-pushed
the
ticket-30237
branch
3 times, most recently
from
331afb6
to
0f86474
Compare
I think that it makes sense to adjust the message. |
There was a problem hiding this comment.
Hi @hermansc. Thanks for the quick turn-around! Super.
I just had a couple more observations... (Sorry 😬)
hermansc
force-pushed
the
ticket-30237
branch
3 times, most recently
from
20801d9
to
40e958c
Compare
There was a problem hiding this comment.
Super. Great effort @hermansc. Thanks.
Just the one code-style niggle... but I'm going to mark this RFC.
My first patch to Django (❤️)...
Welcome aboard! 🎁
tests/admin_checks/tests.py
Outdated
| "must be in MIDDLEWARE in order to use the admin application.", | ||
| id='admin.E409', | ||
| ) | ||
| ] | ||
| self.assertEqual(errors, expected) | ||
|
|
||
| # Subclasses are permitted | ||
| with override_settings(MIDDLEWARE=[ | ||
| 'admin_checks.tests.MyAuthMiddleware', 'admin_checks.tests.MyMessageMiddleware']): |
There was a problem hiding this comment.
So probably just, "use hanging indent" here.
There was a problem hiding this comment.
As a separate issue, the subclass approach could also be applied to the security and XFrameOptions middleware checks -- I'm not sure if adding "or a subclass" to every such message adds much value. If the consensus is to keep it, then the modified strings like "'django.contrib.messages.middleware.MessageMiddleware' (or a subclass) must " should be wrapped at 79 characters.
tests/admin_checks/tests.py
Outdated
| "must be in MIDDLEWARE in order to use the admin application.", | ||
| id='admin.E409', | ||
| ) | ||
| ] | ||
| self.assertEqual(errors, expected) | ||
|
|
||
| # Subclasses are permitted |
There was a problem hiding this comment.
This doesn't depend on anything above, so use a separate test: test_middleware_subclasses
There was a problem hiding this comment.
Hehe, I initially did, however @carltongibson made the comment that perhaps they should be merged – #11057 (comment)
There was a problem hiding this comment.
Sorry. 😬 I thought they made more sense together, but let's go with what Tim said. (Would have needed a full-stop anyhow...)
tests/admin_checks/tests.py
Outdated
| @@ -37,6 +39,14 @@ def check(self, **kwargs): | |||
| return ['error!'] | |||
|
|
|||
|
|
|||
| class MyAuthMiddleware(AuthenticationMiddleware): | |||
There was a problem hiding this comment.
Doesn't matter much, but something like AuthenticationMiddlewareSubclass would be more descriptive about the purpose.
| for path in candidate_paths: | ||
| try: | ||
| candidate_cls = import_string(path) | ||
| except ImportError: |
There was a problem hiding this comment.
Why should an import error pass silently? This doesn't seem to be covered by a test. Assuming it's not needed, this could be simplified to return any(_issubclass(import_string(path), subclass) for path in candidate_paths).
There was a problem hiding this comment.
Why should an import error pass silently?
@timgraham the idea was that we probably want the ImportError to be raised from the MIDDLEWARE loading code then from an admin check. I agree that it should be tested though.
There was a problem hiding this comment.
Agreed. How about something like?
@override_settings(MIDDLEWARE=[
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'django.contrib.does.not.Exist',
])
def test_admin_check_does_not_raise_import_error(self):
self.assertEqual(admin.checks.check_dependencies(), [])
I half-agree, it's a bit naff, but this issue did come up... Would you just leave it as is Tim? (Someone using a subclass isn't going to hit the check, so it's only really going to come up if you leave it out... — so maybe yes.) |
|
Yea, I think the message is targeted toward beginners some subclasses isn't critical to mention. That's a more advanced case and users probably don't need hand holding (and won't see the message if they're already doing it, as you said). |
There was a problem hiding this comment.
Hi @hermansc. Thanks for adding the extra test. You need to adjust the (or subclass) strings in checks.py as that's still causing a failure. (I think you just missed the references in the one file.)
@timgraham Are you happy with the new test? Any last comments/suggestions?
Thanks all.
timgraham
changed the title
timgraham
changed the title
|
Thanks for the updates Tim. CI failures don't (instantly) reproduce locally, so I'm guessing that's an artefact. |
|
New version of PyYAML... #11082 |
…d admin checks allow subclasses.
django/django#11057 fixed in in django
Trac: https://code.djangoproject.com/ticket/30237
My first patch to Django (:heart:) so I'm still new to naming conventions, styling and utility methods which might be available. Please suggest improvements if any!