Code

#19402 closed Cleanup/optimization (fixed)

django.contrib.auth.models.CustomUser.REQUIRED_FIELDS needs better docs

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

Description

Attachments (2)

19402.diff (1.0 KB) - added by ptone 20 months ago.
19402.2.diff (1.3 KB) - added by timo 18 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 20 months ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

I'm not disputing that this might be confusing, but this ticket doesn't serve as an effective call to action because it doesn't clarify what the source of confusion *is*. "Make it more better" doesn't help.

You have to list all the fields that are required. The example does this. I'm happy to oblige with an update to docs, but I need to hear a little more about what exactly is confusing here.

comment:2 Changed 20 months ago by pydanny

I completely understand the need for the REQUIRED_FIELDS feature. I just wanted to have it documented before people raised issues of the same sort as what we got with CBVs. In other words, I was trying to step up to back up your decision on the code.

I'll figure out a better way to articulate the need for this ticket, and the documentation, and more, and then I'll open it again.

comment:3 Changed 20 months ago by mattias

A link to a discussion that potentially inspired this ticket: http://news.ycombinator.com/item?id=4855590

In summary, django models already have ways of marking fields required via null=False or blank=True on the database and form-level validation. How do these interact with the REQUIRED_FIELDS list?

At the moment, REQUIRED_FIELDS appears to be read only by contrib/auth/management/commands/createsuperuser.py.
It is validated to not contain the USERNAME_FIELD in core/management/validation.py.

Some sources of confusion that may be helpful to document (or, if they should never happen, perhaps additional validation):

  • What are the use cases where REQUIRED_FIELDS differs from fields marked required with null=False or blank=True?
  • I assume user creation will fail if REQUIRED_FIELDS omits fields which are required on the database level - should this be validated?
  • What parts of django might check REQUIRED_FIELDS in the future? Will ModelForms or ModelAdmins read it and reject creating users with missing fields, or is that up to the user code to validate?
  • It's possible to bypass various levels of validation. For example, an user object could be created by createsuperuser.py, by a ModelForm, by a custom form, in the admin site, by code that creates and saves an instance of the swapped User model, or in the database directly. It's possible to load and read users after they have been created in various ways. Which of these will fail currently, or should fail in future versions due to entries in REQUIRED_FIELDS?
Last edited 20 months ago by mattias (previous) (diff)

comment:4 Changed 20 months ago by ptone

  • Resolution needsinfo deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.5-beta-1 to master

Mattias makes some good points.

I think it is well worth documenting the differences between user REQUIRED_FIELDS vs field required param in the context of the custom user docs.

When the username check was added, there was acknowledgement that this was a practical>purity move given that there should be a generic way to handle model validation.

Ideally I'd like to say that anyone adding a custom user should be able to see this for what it is, given that adding a custom user to a project will add some extra work or expertise requirement - but that isn't a good reason not to add some clarification.

Changed 20 months ago by ptone

comment:5 Changed 20 months ago by ptone

I've attached a quick stab at this.

This, currently really only used for the interactive creation of users so that it can prompt for fields before actually creating the user.

There is no check that REQUIRED_FIELDS actually contains all fields on the model that have blank=False/undefined

Furthermore, if there are errors in field validation during createsuperuser, the value is just set to None and sent on to model creation anyway:

https://github.com/django/django/blob/b3b3db3d954a5226f870a0b4403343c78efae8dc/django/contrib/auth/management/commands/createsuperuser.py#L96

https://github.com/django/django/blob/b3b3db3d954a5226f870a0b4403343c78efae8dc/django/contrib/auth/management/commands/createsuperuser.py#L115

Given this current limited use, I think it is worth at least considering renaming this attribute to something like "PROMPTED_FIELDS" - I actually don't think there are other places this attribute should be used outside of interactive/prompted creation (and even that could possibly be done by surveying what fields have blank=False). I think the main point of confusion that this attribute introduces is the idea that for user models, a "required field" is somehow distinct or unusual compared to other Django models - where really what this is about is "lets not try to create a user blindly with trial and error for fields we know you want to ask for".

The more I think about it - the more this attr could probably just go away, and instead be compiled dynamically in createsuper user from those fields where blank=False?

refs #19150, #19067 (and to a lesser degree: #19077, #19079)

comment:6 Changed 19 months ago by timo

  • Has patch set

comment:7 Changed 19 months ago by mjtamlyn

It should also be mentioned that it fails miserably for FKs

Changed 18 months ago by timo

comment:8 Changed 18 months ago by timo

I made some minor tweaks to Preston's patch, doesn't seem like we're going to rename this at this point in the 1.5 release cycle?

comment:9 Changed 18 months ago by russellm

For my money, a name change for REQUIRED_FIELDS is a bit of a bikeshed. REQUIRED_FIELDS isn't inaccurate -- it describes a list of fields that are required on the model. I'm not convinced that renaming the attribute will mean people understand what it's for any better.

If we're going to address this, lets look into at removing the need for REQUIRED_FIELDS altogether (which is something we should be able to add in a backwards compatible way -- we just stop paying attention to the setting, and maybe raise a warning if it is found). However, this isn't going to happen for 1.5.

comment:10 Changed 17 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

In 1702be89c86d1b1a01efb16fa7836335ad709184:

[1.5.X] Fixed #19402 - Clarified purpose of CustomUser.REQUIRED_FIELDS

Thanks pydanny for the report and ptone for the patch.

Backport of 24a2bcbcdd from master

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.