[Django] #29449: Regression in UserCreationForm (and UserChangeForm) for custom user models
Django
-------------------------------------+-------------------------------------
Reporter: Sławek | Owner: nobody
Ehlert |
Type: Bug | Status: new
Component: | Version: 2.1
contrib.auth | Keywords: auth forms
Severity: Normal | UserCreationForm UserChangeForm
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In 3333d935d2914cd80cf31f4803821ad5c0e2a51d (part of #28757 ticket)
{{{UserCreationForm}}} (and by extension also {{{UserChangeForm}}}) was
adjusted to support custom user models without overriding the form. The
problem is that the implementation there assumes that the
{{{USERNAME_FIELD}}} has to be a subclass of {{{CharField}}} since it uses
{{{UsernameField}}} in the {{{field_classes}}} mapping.
Now to the fun part. Django already has some tests that should fail in
such case (for example see
https://github.com/django/django/blob/825f0beda804e48e9197fcf3b0d909f9f548aa47/tests/auth_tests/test_management.py#L435-L491),
but the failure is actually "swallowed" when running the whole test suite.
When running those tests individually we can see the failure.
Tim Graham already noticed something similar in
https://code.djangoproject.com/ticket/28757#comment:7.
I.e.
{{{
./runtests.py auth_tests
}}}
works, but
{{{
./runtests.py
auth_tests.test_management.CreatesuperuserManagementCommandTestCase.test_fields_with_fk
}}}
doesn't. I'm not sure why the failure is hidden in the former case (I
suspect it has to do something with the {{{override_settings}}} decorator
and {{{importlib.reload}}}ing done in the {{{reload_auth_forms}}} function
from the commit I've mentioned). The stacktrace from the latter case looks
like this:
{{{
ERROR: test_fields_with_fk
(auth_tests.test_management.CreatesuperuserManagementCommandTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/slafs/testing/django/django/django/test/utils.py", line
364, in inner
with self as context:
File "/Users/slafs/testing/django/django/django/test/utils.py", line
336, in __enter__
return self.enable()
File "/Users/slafs/testing/django/django/django/test/utils.py", line
405, in enable
setting=key, value=new_value, enter=True)
File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py",
line 175, in send
for receiver in self._live_receivers(sender)
File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py",
line 175, in <listcomp>
for receiver in self._live_receivers(sender)
File "/Users/slafs/testing/django/django/django/test/signals.py", line
195, in user_model_swapped
from django.contrib.auth import forms
File "/Users/slafs/testing/django/django/django/contrib/auth/forms.py",
line 63, in <module>
class UserCreationForm(forms.ModelForm):
File "/Users/slafs/testing/django/django/django/forms/models.py", line
256, in __new__
apply_limit_choices_to=False,
File "/Users/slafs/testing/django/django/django/forms/models.py", line
172, in fields_for_model
formfield = f.formfield(**kwargs)
File
"/Users/slafs/testing/django/django/django/db/models/fields/related.py",
line 956, in formfield
**kwargs,
File
"/Users/slafs/testing/django/django/django/db/models/fields/related.py",
line 418, in formfield
return super().formfield(**defaults)
File
"/Users/slafs/testing/django/django/django/db/models/fields/__init__.py",
line 890, in formfield
return form_class(**defaults)
File "/Users/slafs/testing/django/django/django/forms/fields.py", line
213, in __init__
super().__init__(**kwargs)
TypeError: __init__() got an unexpected keyword argument
'limit_choices_to'
}}}
Since {{{auth_tests.CustomUserWithFK}}} model uses a {{{ForeignKey}}} as a
{{{USERNAME_FIELD}}}, the instantiation of
{{{django.contrib.auth.forms.UsernameField}}} is failing. However, that
test is a bit implicit as I'm not entirely sure why Django imports
{{{UserCreationForm}}} in the first place (the stacktrace makes me think
it's because of the signal mechanisms).
I'd suggest the following steps to go forward with this:
1) Write a more explicit regression test in {{{auth_tests.test_forms}}}
that's using {{{auth_tests.CustomUserWithFK}}} as a user model (I'll try
to do that).
2) Make adjustments in {{{auth_tests.test_forms}}} so that the mentioned
tests will actually fail even when running the whole test suite (I think
Tim Graham already tried this in
https://code.djangoproject.com/ticket/28757#comment:8).
3) Fix {{{UserCreationForm}}} so it supports different types of fields for
{{{USERNAME_FIELD}}}.
In my humble opinion this is a release blocker (I haven't marked this
ticket as such, though).
--
Ticket URL: <https://code.djangoproject.com/ticket/29449>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
I can confirm this.
Using something like
`UserModel._meta.get_field(UserModel.USERNAME_FIELD).formfield()` to
derive the form field for a the current username field _may_ make sense.
However, this won't work with the normalization we currently do in the
form's `UsernameField`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
I've added some explicit tests in
[https://github.com/django/django/pull/10020 PR#10020].
Unfortunately it causes more failures than the ones I've introduced with
those new tests there.
What happens is:
- the test I've added overrides the {{{AUTH_USER_MODEL}}} setting with a
model that has a FK as a username field
({{{auth_tests.CustomUserWithFK}}}).
- the test fails on the {{{override_settings}}} part already, since the
{{{reload_auth_forms}}} receiver fails to reload the
{{{django.contrib.auth.forms}}} module with that user model in place
(fails with a similar {{{TypeError}}} that refers to 'limit_choices_to'
argument when trying to instantiate the {{{UsernameField}}})[*]
- because of the {{{override_settings}}} bug (see #29467) other tests
still see the {{{auth_tests.CustomUserWithFK}}} as a current user model.
[*] - the exact traceback for the mentioned error is:
{{{
======================================================================
ERROR: test_custom_form_username_not_charfield
(auth_tests.test_forms.UserChangeFormTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/unittest/case.py",
line 59, in testPartExecutor
yield
File
"/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/unittest/case.py",
line 605, in run
testMethod()
File "/Users/slafs/testing/django/django/django/test/utils.py", line
364, in inner
with self as context:
File "/Users/slafs/testing/django/django/django/test/utils.py", line
336, in __enter__
return self.enable()
File "/Users/slafs/testing/django/django/django/test/utils.py", line
405, in enable
setting=key, value=new_value, enter=True)
File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py",
line 175, in send
for receiver in self._live_receivers(sender)
File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py",
line 175, in <listcomp>
for receiver in self._live_receivers(sender)
File
"/Users/slafs/testing/django/django/tests/auth_tests/test_forms.py", line
38, in reload_auth_forms
reload(django.contrib.auth.forms)
File
"/Users/slafs/.virtualenvs/django/lib/python3.6/importlib/__init__.py",
line 166, in reload
_bootstrap._exec(spec, module)
File "<frozen importlib._bootstrap>", line 618, in _exec
File "<frozen importlib._bootstrap_external>", line 678, in exec_module
File "<frozen importlib._bootstrap>", line 219, in
_call_with_frames_removed
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I don't understand why this issue is described as a regression. Using a
custom user model with `UserCreationForm` and `UserChangeForm` is a new
feature, so I wouldn't describe any issues there as a regression. A
regression would be something that worked in older versions of Django that
no longer works. Am I misunderstanding?
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
Replying to [comment:3 Tim Graham]:
> I don't understand why this issue is described as a regression. Using a
custom user model with `UserCreationForm` and `UserChangeForm` is a new
feature, so I wouldn't describe any issues there as a regression. A
regression would be something that worked in older versions of Django that
no longer works. Am I misunderstanding?
Nope, all good. You're correct about the naming here. My bad.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:4>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
If a patch isn't forthcoming (the best way to avoid the issue that Markus
pointed out in comment 1 isn't obvious to me), I think we could deescalate
from a release blocker by documenting this limitation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:5>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
I'd like to submit a patch for that, but currently I was focusing more on
solving #29467 first.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:6>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I've opened [https://github.com/django/django/pull/10062 PR] to adjust the
docs here.
> I think we could deescalate from a release blocker by documenting this
limitation.
Hopefully that lets us move forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:7>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
Well, I think the more pressing problem is not the feature set Django
supports and the docs, but the fact that the change mentioned here breaks
the test suite and can break people's projects.
Also by having a {{{auth_tests.CustomUserWithFK}}} model in the test
suite, one would think that this kind of model is supported by Django,
which is not true at this point.
Moreover, having a similar user model in a project means that a developer
can't even import from {{{django.contrib.auth.forms}}} module. And what if
something imports that module that's not in developer's control? E.g.
currently {{{django/contrib/admin/forms.py}}},
{{{django/contrib/auth/admin.py}}} and {{{django/contrib/auth/views.py}}}
are importing something from that module, so that means on fresh project
with admin enabled with that kind of a user model, developer can't even
run {{{./manage.py --help}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:8>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
So I think, regarding this feature - we should either fix it or ditch it
completely.
I made some substantial progress on #29467 lately. I'd like to try and
tackle this ticket next (although I have some time constraints and the
progress may be slow on this).
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:9>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
Comment:
@Sławek: Can you push your progress somewhere? I can have a look and see
if I can help.
> ...we should either fix it or ditch it completely.
Yes. OK.
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:10>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:11>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sławek Ehlert):
@Carlton I've opened https://github.com/django/django/pull/10068
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:12>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution:
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: felixxm (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:13>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution: fixed
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f" f3fa86a]:
{{{
#!CommitTicketReference repository=""
revision="f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f"
Fixed #29449 -- Reverted "Fixed #28757 -- Allowed using contrib.auth forms
without installing contrib.auth."
This reverts commit 3333d935d2914cd80cf31f4803821ad5c0e2a51d due to
a crash if USERNAME_FIELD isn't a CharField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:14>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution: fixed
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"78f502cd0bc834422c3f189e852564fe4b6459f2" 78f502c]:
{{{
#!CommitTicketReference repository=""
revision="78f502cd0bc834422c3f189e852564fe4b6459f2"
[2.1.x] Fixed #29449 -- Reverted "Fixed #28757 -- Allowed using
contrib.auth forms without installing contrib.auth."
This reverts commit 3333d935d2914cd80cf31f4803821ad5c0e2a51d due to
a crash if USERNAME_FIELD isn't a CharField.
Backport of f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:15>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution: fixed
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"d3449faaa915a08c275b35de01e66a7ef6bdb2dc" d3449fa]:
{{{
#!CommitTicketReference repository=""
revision="d3449faaa915a08c275b35de01e66a7ef6bdb2dc"
Refs #29449 -- Removed release note for "Allowed using contrib.auth forms
without installing contrib.auth."
The code was reverted in f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:16>
Django
CharField
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Release blocker | Resolution: fixed
Keywords: auth forms | Triage Stage: Accepted
UserCreationForm UserChangeForm |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"c903e905b3c5d66477f8d04717650644afc04e1f" c903e905]:
{{{
#!CommitTicketReference repository=""
revision="c903e905b3c5d66477f8d04717650644afc04e1f"
[2.1.x] Refs #29449 -- Removed release note for "Allowed using
contrib.auth forms without installing contrib.auth."
The code was reverted in f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f.
Backport of d3449faaa915a08c275b35de01e66a7ef6bdb2dc from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29449#comment:17>