Opened 11 years ago
Closed 10 years ago
#21832 closed New feature (fixed)
allow USERNAME_FIELD to be a ForeignKey
Reported by: | Chris Jerdonek | Owned by: | ANUBHAV JOSHI |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | USERNAME_FIELD, ForeignKey, foreign key, auth, username |
Cc: | Baptiste Mispelon, chris.jerdonek@…, loic@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This issue is to add support for USERNAME_FIELD
being a ForeignKey
. Currently, this isn't supported because, for example, the createsuperuser
command doesn't support inputting the corresponding foreign key data (also see #21755 for another issue with the createsuperuser
command needed for this).
One use case for USERNAME_FIELD
being a ForeignKey
is if the username is an email address, and you would like to have a separate table for e-mail address related data (e.g. total e-mails sent, last e-mail sent, etc), as well as allow changing the email address on a user account.
Attachments (1)
Change History (18)
comment:1 by , 11 years ago
Cc: | added |
---|
comment:3 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
There seems to be three different use cases here:
- "Local FK" - USERNAME_FIELD points to local foreign key, and the value for that foreign key can be used to create the remote model. Example: USERNAME_FIELD = 'email', email points to class Email's field 'email', and that is the only required field of Email. This should be relatively easy to support.
- "Remote field's value". Like above, but now the user model's email field points to Email model's id field, but field 'email' of the remote model is also required. This is harder to support, we need more logic so that we know to ask the email for the related model. Syntax could be USERNAME_FIELD =
'email__email'
. - Remote model with multiple required fields. We need support for validating & creating the remote model before we create the user model. Support for REQUIRED_FIELDS =
'email__otherfield'
is required, and all the logic to then create the related models, validation and so on.
I am marking this accepted, but I think we should only support the first case above. It should be relatively straightforward to support. The other cases seem to be complex and I am not sure they are worth the added value.
comment:4 by , 11 years ago
I have had conversation regarding this issue with @loic and @akaariai.
I hope to work on this in GSoC this year(i.e if I am accepted).
comment:5 by , 11 years ago
Cc: | added |
---|
@akaariai summed up the situation pretty well, but to make sure everyone is on the same page as it took us some time to figure it out on IRC:
- "Local FK":
class Email(models.Model): email = models.CharField(max_length=254) class User(models.Model): email = models.ForeignKey(Email, to_field='email') USERNAME_FIELD = 'email'
- "Remote field's value":
class Email(models.Model): email = models.CharField(max_length=254) class User(models.Model): email = models.ForeignKey(Email) USERNAME_FIELD = 'email__email'
comment:6 by , 11 years ago
For the record, in my use case the email address model has two required fields: a "normalized" email address with the unique
option (think lower-cased, for example) and a non-normalized email address to store the user's preferred spelling/casing. This lets uniqueness be enforced at the database level. The user manager's get_by_natural_key()
method is overridden to normalize the given email address before querying for a user.
Is the issue with the other cases only the creation phase? It seems like an API to construct a model object from required fields might be worth having anyways. Such an API could handle foreign keys recursively.
In any case, it would be good if any commits related to this issue do not preclude or make it harder to solve more general use cases like the one I outlined.
comment:7 by , 11 years ago
Question: Other than createsuperuser, is there anything currently preventing this from working?
If the problem is just createsuperuer, would an acceptable fix to bail out (gracefully) from createsuperuser if USERNAME_FIELD can't be gathered at the command line, and leave it up to the user to open a shell and use UserModel.objects.create_superuser()?
comment:8 by , 11 years ago
Other than createsuperuser, is there anything currently preventing this from working?
Yes, pretty much anywhere that USERNAME_FIELD
appears in the code needs to be reviewed, for example AbstractBaseUser.get_username()
and RemoteUserBackend.authenticate()
. In my experience, though, most of these can be dealt with through straightforward subclassing and overriding. However, the createsuperuser
command was the most awkward to deal with. I wound up overriding the command, but the solution I'm currently using is brittle and somewhat of a hack (relying on internal details, etc).
My hope for this issue is to reduce these barriers. In other words, I wouldn't mind if foreign keys aren't supported out of the box, but it would be nice if it were easier to make things work (and in a way that's more likely to work in future versions of Django, etc).
comment:9 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Will work on this in my GSoC project
comment:10 by , 10 years ago
Hi, I was going through the comments and documentation and I have a question before I attempt to start this:
Why it says here in the docs for REQUIRED_FIELDS that "However, it will not work for ForeignKey
fields."
Because this ticket is related to #21755 and I think when we are making USERNAME_FIELD
to allow FK
, why not REQUIRED_FIELDS
? Is there some specific purpose as to why this is done?
comment:11 by , 10 years ago
We would probably need to fix that, yes. The documentation simply warns that it doesn't currently work; there is no reason I know of not to try to make it work.
comment:12 by , 10 years ago
Chris, we just committed a fix for #21755 and it's not clear to me what additional work, if anything, should be done here. I believe the "local FK" situation described above should work for createsuperuser
(Anubhav is confirming), although it requires an existing instance of that FK to already exist. I'm not sure the effort enhancing createsuperuser
to allow model creation for the ForeignKeys themselves is really worth it as I expect createsuperuser
isn't heavily used besides creating an initial user that can be used the login to the admin. What do you think?
follow-up: 15 comment:13 by , 10 years ago
Tim, I think you're right that it's probably not worth the effort at this point to add full-blown support to createsuperuser
. However, I think the following should still be done:
1) Confirm that a good error message is being given if the FK instance doesn't exist (e.g. if someone tries running createsuperuser
without an awareness of its current limitations.
2) Provide a clear way for the createsuperuser
command to be customized/overridden to support the case of the FK instance not existing (e.g. by adding a hook of some sort to the public API).
I originally filed this issue because of the pain of doing (2) myself. The approach I used was to create a command of the same name in a different app that "shadows" the current command. The new command sub-classed the current command. Prior to calling the base class's handle()
method, it runs code to prompt the user to enter the needed data to create the FK instance. This approach was hard to come by and is brittle because, for instance, it relies on internal details of the API. If I recall, even the "shadowing" of a command is brittle because it depends on the order of INSTALLED_APPS
if I remember right.
Also, question: with the current implementation, how would the user specify the FK instance on the command-line? Using the (integer) ID?
comment:14 by , 10 years ago
Updated the prompt and existing tests.
Also added a test to show that checks are working fine.
by , 10 years ago
Attachment: | 21832.diff added |
---|
comment:15 by , 10 years ago
Replying to cjerdonek:
Tim, I think you're right that it's probably not worth the effort at this point to add full-blown support to
createsuperuser
. However, I think the following should still be done:
1) Confirm that a good error message is being given if the FK instance doesn't exist (e.g. if someone tries running
createsuperuser
without an awareness of its current limitations.
2) Provide a clear way for thecreatesuperuser
command to be customized/overridden to support the case of the FK instance not existing (e.g. by adding a hook of some sort to the public API).
I originally filed this issue because of the pain of doing (2) myself. The approach I used was to create a command of the same name in a different app that "shadows" the current command. The new command sub-classed the current command. Prior to calling the base class's
handle()
method, it runs code to prompt the user to enter the needed data to create the FK instance. This approach was hard to come by and is brittle because, for instance, it relies on internal details of the API. If I recall, even the "shadowing" of a command is brittle because it depends on the order ofINSTALLED_APPS
if I remember right.
Also, question: with the current implementation, how would the user specify the FK instance on the command-line? Using the (integer) ID?
If the instance doesn't exists, appropriate error is raised by call to field's clean()
method.
eg.
Email instance with pk 3 does not exists.
On the command line, the value of to_field
is needed which is pk
or id
by default. This has been specified clearly in the docs. Also the prompt at the command line will specify which the to_field
's(id
by default) name to be entered.
eg.
Username (Email.id) or Username (Email.email)
comment:17 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Can we change the implementation of USERNAME_FIELD
from
to
In this way, we can get to know the model in superuser command and we can then pass an appropriate model_instance
(The tuple's second item being optional, ofc)
Thoughts??