Opened 3 years ago

Closed 2 years ago

#21755 closed New feature (fixed)

Add ForeignKey support to REQUIRED_FIELDS

Reported by: Chris Jerdonek Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: chris.jerdonek@…, Tim Graham, anubhav9042@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If instance=None is passed to _route_db in db.utils, then an exception like the following is raised:

  ...
  File ".../django/core/management/base.py", line 285, in execute
    output = self.handle(*args, **options)
  File ".../django/contrib/auth/management/commands/createsuperuser.py", line 96, in handle
    username = self.username_field.clean(raw_value, None)
  File ".../django/db/models/fields/__init__.py", line 255, in clean
    self.validate(value, model_instance)
  File ".../django/db/models/fields/related.py", line 1199, in validate
    using = router.db_for_read(model_instance.__class__, instance=model_instance)
  File ".../django/db/utils.py", line 250, in _route_db
    return hints['instance']._state.db or DEFAULT_DB_ALIAS
AttributeError: 'NoneType' object has no attribute '_state'

The code is here:

try:
    return hints['instance']._state.db or DEFAULT_DB_ALIAS
except KeyError:
    return DEFAULT_DB_ALIAS

It would be better if the None case were handled in the same way that KeyError is being caught.

Change History (15)

comment:1 Changed 3 years ago by Chris Jerdonek

Cc: chris.jerdonek@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 3 years ago by Baptiste Mispelon

Hi,

It's not clear to me whether you've managed to trigger this error while using the ORM or if you have a use-case where you'd like to be able to pass instance=None as a hint to the db router.

If it's the former, do you have a bit of code that reproduces the isssue and if it's the latter, could you explain why you need to pass instance=None rather than not passing an instance at all?

Thanks.

comment:3 Changed 3 years ago by Chris Jerdonek

Hi, I've been meaning to elaborate on this because I'm not sure if this should be changed as I've described.

This error occurred when setting the USERNAME_FIELD to a foreign key field and running the createsuperuser command. The base field class's validate() method doesn't use the model_instance argument, but the ForeignKey field version's does.

Obviously, I needed to change the createsuperuser command in other ways to get createsuperuser to work in my use case above, but this error I encountered in the process seemed unnecessarily obscure.

The possible resolutions of this issue I can think of are:

  • pass a valid model_instance instead of None in the default implementation of createsuperuser, even though it is not needed in the out-of-the-box case,
  • allow None to be passed in the foreign key field version of validate() (for example by catching the AttributeError in db.utils _route_db() as I described in my original suggestion),
  • raising a simpler-to-decipher error earlier in the process (e.g. by raising in createsuperuser or elsewhere that foreign key fields aren't allowed), or
  • leaving things as is.

Thanks for considering this.

comment:4 Changed 3 years ago by Chris Jerdonek

By the way, this ticket isn't the place to resolve the following issue in general, but my preference would be for Django to make it easier to support the following two use cases out of the box: (1) USERNAME_FIELD being a foreign key, and (2) USERNAME_FIELD being a tuple of fields. I may open tickets for these two issues at some point if they haven't already been suggested. If the resolution of this ticket can reduce one roadblock towards use case (1), that would be good. The current issue is simply one of the first places where things went wrong.

comment:5 Changed 3 years ago by Baptiste Mispelon

Easy pickings: unset
Triage Stage: UnreviewedAccepted

OK, I'm starting to understand a bit better (thanks for clarifying).

I couldn't find any official documentation for models.Field.validate() and in particular, I don't think we've documented what's acceptable to pass as model_instance or not.

Typically, Field.validate is called as part of model validation (Model.clean_fields) which always passes a valid model_instance) so there's no issue. In this case though, the problem arises because createsuperuser calls field validation directly, in which case it doesn't make much sense to pass a model_instance.

I think your option 2 is a valid approach, though I would use something like this rather than catching AttributeError (I feel that it's not specific enough):

instance = hints.get('instance')
if instance is not None and instance._state.db:
    return instance._state.db
return DEFAULT_DB_ALIAS

Regarding using a ForeignKey as a USERNAME_FIELD, while it doesn't seem like a very common use-case, it's not explicitly forbidden by the documentation [1] which only requires the field to be unique.
I suggest opening a ticket regarding this feature so that we can either support it or at least document that it's not possible.

Thanks.

[1] https://docs.djangoproject.com/en/1.6/topics/auth/customizing/#django.contrib.auth.models.CustomUser.USERNAME_FIELD

comment:6 Changed 3 years ago by Chris Jerdonek

I suggest opening a ticket regarding this feature so that we can either support it or at least document that it's not possible.

Thanks for your thoughts. Okay, I created #21832 for this.

comment:7 Changed 3 years ago by davidc

I am running into the same problem for a foreign key in the REQUIRED_FIELDS of my custom user model. It does not have to be the USERNAME_FIELD. I think this would be much more common. In my case I need all users to have a department.

comment:8 Changed 3 years ago by Chris Jerdonek

@davidc, for future reference, can you include the stack trace you are getting? The stack trace I included in my original post references the username field (in the line, username = self.username_field.clean(raw_value, None)), so it looks like there is another place in Django's code base that is passing None as the model instance.

comment:9 Changed 3 years ago by dcreech

@cjerdonek, Here is the stack trace:

...
  File "C:\Python27\lib\site-packages\django\core\management\base.py", line 285, in execute
    output = self.handle(*args, **options)
  File "C:\Python27\lib\site-packages\django\contrib\auth\management\commands\createsuperuser.py", line 116, in handle
    user_data[field_name] = field.clean(raw_value, None)
  File "C:\Python27\lib\site-packages\django\db\models\fields\__init__.py", line 255, in clean
    self.validate(value, model_instance)
  File "C:\Python27\lib\site-packages\django\db\models\fields\related.py", line 1199, in validate
    using = router.db_for_read(model_instance.__class__, instance=model_instance)
  File "C:\Python27\lib\site-packages\django\db\utils.py", line 250, in _route_db
    return hints['instance']._state.db or DEFAULT_DB_ALIAS
AttributeError: 'NoneType' object has no attribute '_state'

Last edited 3 years ago by dcreech (previous) (diff)

comment:10 in reply to:  7 Changed 3 years ago by ANUBHAV JOSHI

Replying to davidc:

I am running into the same problem for a foreign key in the REQUIRED_FIELDS of my custom user model. It does not have to be the USERNAME_FIELD. I think this would be much more common. In my case I need all users to have a department.

I think we cannot use ForeignKey here. See:https://docs.djangoproject.com/en/1.6/topics/auth/customizing/#django.contrib.auth.models.CustomUser.REQUIRED_FIELDS

comment:11 Changed 2 years ago by Tim Graham

Cc: Tim Graham added
Summary: db.utils _route_db can raise AttributeError: 'NoneType' object has no attribute '_state'Add ForeignKey support to REQUIRED_FIELDS
Type: BugNew feature

comment:12 Changed 2 years ago by ANUBHAV JOSHI

Cc: anubhav9042@… added
Component: Database layer (models, ORM)contrib.auth
Has patch: set
Needs documentation: set
Version: 1.6master

comment:13 Changed 2 years ago by ANUBHAV JOSHI

For now, I have skipped the call to clean in case of FK and left the validation part of Descriptors.
I tried to implement what @bmispelon commented, but there seemed to a be problem with that. In validate() in related.py, after we get the correct qs, qs.exists() somehow retriggers call to validate() and we get empty qs and error is raised. Can anyone explain this?

EDITS:
I think that problem was due to some changes that I made myself.
Updated the patch with new approach,

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

comment:14 Changed 2 years ago by ANUBHAV JOSHI

Needs documentation: unset

comment:15 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 9bc2d766a0c31701a4c0e20a8b04164bb22cf6d5:

Fixed #21755 -- Added ForeignKey support to REQUIRED_FIELDS.

This allows specifying ForeignKeys in REQUIRED_FIELDS when using a
custom User model.

Thanks cjerdonek and bmispelon for suggestion and timgraham for review.

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