Opened 6 months ago

Last modified 6 months ago

#28461 new Cleanup/optimization

Clarify how UserManager.create_superuser() must be implemented with a ForeignKey in REQUIRED_FIELDS

Reported by: Kirill B. Owned by: nobody
Component: Documentation Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Kirill B.)

These are my models:

from uuid import uuid4

from django.contrib.auth.models import AbstractUser
from django.db import models


class Company(models.Model):
    name = models.CharField(max_length=64, primary_key=True)


class User(AbstractUser):
    uuid = models.UUIDField(primary_key=True, editable=False, default=uuid4)
    company = models.ForeignKey(Company)

    REQUIRED_FIELDS = ['email', 'company']

When I run manage.py createsuperuser I got this result:

Username: superuser
Email address: test@example.com
Company (Company.name): TEST
Password: 
Password (again): 
Traceback (most recent call last):
  File "manage.py", line 22, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/core/management/__init__.py", line 363, in execute_from_command_line
    utility.execute()
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/core/management/__init__.py", line 355, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/contrib/auth/management/commands/createsuperuser.py", line 63, in execute
    return super(Command, self).execute(*args, **options)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/contrib/auth/management/commands/createsuperuser.py", line 183, in handle
    self.UserModel._default_manager.db_manager(database).create_superuser(**user_data)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/contrib/auth/models.py", line 170, in create_superuser
    return self._create_user(username, email, password, **extra_fields)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/contrib/auth/models.py", line 151, in _create_user
    user = self.model(username=username, email=email, **extra_fields)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/contrib/auth/base_user.py", line 68, in __init__
    super(AbstractBaseUser, self).__init__(*args, **kwargs)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/db/models/base.py", line 554, in __init__
    _setattr(self, field.name, rel_obj)
  File "/Users/ookami/venv/swportal/lib/python3.5/site-packages/django/db/models/fields/related_descriptors.py", line 216, in __set__
    self.field.remote_field.model._meta.object_name,
ValueError: Cannot assign "'TEST'": "User.company" must be a "Company" instance.

PR

Attachments (1)

local_history.patch (889 bytes) - added by Kirill B. 6 months ago.
Possible solution

Download all attachments as: .zip

Change History (12)

Changed 6 months ago by Kirill B.

Attachment: local_history.patch added

Possible solution

comment:1 Changed 6 months ago by Kirill B.

Has patch: set

comment:2 Changed 6 months ago by Tim Graham

Patch needs improvement: set
Summary: Can't create superuser with ForeignKey required fieldcreate_superuser crashes with a ForeignKey to a CharField in REQUIRED_FIELDS
Triage Stage: UnreviewedAccepted

I can reproduce the crash, however, the proposed patch breaks the test_fields_with_fk_interactive test from #21832.

comment:3 Changed 6 months ago by Tim Graham

Summary: create_superuser crashes with a ForeignKey to a CharField in REQUIRED_FIELDScreatesuperuser crashes with a ForeignKey to a CharField in REQUIRED_FIELDS

comment:4 Changed 6 months ago by Kirill B.

Description: modified (diff)
Has patch: unset
Patch needs improvement: unset

comment:5 Changed 6 months ago by Kirill B.

The problem is not in CharField – actually any ForeignKey will fail. Current tests are using customized UserManager, where ForeignKey is resolved manually. I've created new test where custom user is inherited from default User model.

I've created PR with my proposal to fix this problem.

comment:6 Changed 6 months ago by Kirill B.

Has patch: set

comment:7 Changed 6 months ago by Tim Graham

Do you see a downside to documenting that UserManager.create_superuser() must be implemented as it is now in the tests? The proposed change will break currently working code (as the change in CustomUserWithFKManager.create_superuser() demonstrates).

comment:8 Changed 6 months ago by Kirill B.

Yeah, I think this moment should be clarified in docs, but I'm in doubt what exactly should go there.

I guess there are 2 places where this can be described: createsuperuser management command docs and models.CustomUserManager.create_superuser section in auth/customizing.txt.

comment:9 Changed 6 months ago by Tim Graham

Component: contrib.authDocumentation
Has patch: unset
Summary: createsuperuser crashes with a ForeignKey to a CharField in REQUIRED_FIELDSClarify how UserManager.create_superuser() must be implemented with a ForeignKey in REQUIRED_FIELDS
Type: BugCleanup/optimization

I think I'd put a note in UserManager.create_superuser() as you mentioned and also in the docs for REQUIRED_FIELDS intopics/auth/customizing.txt. In the latter place, there's already a note there about ForeignKey that could be elaborated upon.

comment:10 Changed 6 months ago by Kirill B.

I thought you meant to update documentation in addition to this change. The problem is that the current implementation of createsuperuser management command doesn't work with the default implementation of UserManager (see the code in my first post). So either the implementation of default UserManager.create_superuser() or createsuperuser management command should be changed.

comment:11 Changed 6 months ago by Tim Graham

I understand your point and am not completely convinced about which way to go, however, technically, I think your code doesn't conform to the documentation which says:

If your user model defines [the same fields] as Django’s default user, you can just install Django’s UserManager; however, if your user model defines different fields, you’ll need to define a custom manager that extends BaseUserManager providing two additional methods:
...
The prototype of create_superuser() should accept the username field, plus all required fields as arguments.

Since you added 'company' in REQUIRED_FIELDS, the documentation suggests that a custom manager must be added for this situation.

Note: See TracTickets for help on using tickets.
Back to Top