Opened 10 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)

21832.diff (5.7 KB ) - added by ANUBHAV JOSHI 10 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Chris Jerdonek, 10 years ago

Cc: chris.jerdonek@… added

comment:2 by ANUBHAV JOSHI, 10 years ago

Can we change the implementation of USERNAME_FIELD
from

USERNAME_FIELD = 'somefield'

to

USERNAME_FIELD = ('somefield',)
USERNAME_FIELD = ('somefield','somemodel')


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??

Last edited 10 years ago by ANUBHAV JOSHI (previous) (diff)

comment:3 by Anssi Kääriäinen, 10 years ago

Triage Stage: UnreviewedAccepted

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 ANUBHAV JOSHI, 10 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 loic84, 10 years ago

Cc: loic@… 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'
Last edited 10 years ago by loic84 (previous) (diff)

comment:6 by Chris Jerdonek, 10 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 Russell Keith-Magee, 10 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 Chris Jerdonek, 10 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 ANUBHAV JOSHI, 10 years ago

Owner: changed from nobody to ANUBHAV JOSHI
Status: newassigned

Will work on this in my GSoC project

comment:10 by ANUBHAV JOSHI, 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 Tim Graham, 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 Tim Graham, 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?

comment:13 by Chris Jerdonek, 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 ANUBHAV JOSHI, 10 years ago

Updated the prompt and existing tests.
Also added a test to show that checks are working fine.

by ANUBHAV JOSHI, 10 years ago

Attachment: 21832.diff added

in reply to:  13 comment:15 by ANUBHAV JOSHI, 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 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?


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 Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 75ff7b8fb8335e181ad05e1d4244b4295cc7a105:

Fixed #21832 -- Updated prompt, tests, and docs to show that USERNAME_FIELD supports FK after 9bc2d76.

Also added get_input_data() hook in createsuperuser.

Thanks Chris Jerdonek and Tim Graham for review.

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