#21755 closed New feature (fixed)

Add ForeignKey support to REQUIRED_FIELDS

Reported by: cjerdonek Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: chris.jerdonek@…, timo, 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 19 months ago by cjerdonek

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

comment:2 Changed 19 months ago by bmispelon

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 19 months ago by cjerdonek

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 19 months ago by cjerdonek

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 18 months ago by bmispelon

  • Easy pickings unset
  • Triage Stage changed from Unreviewed to Accepted

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 18 months ago by cjerdonek

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 follow-up: Changed 18 months 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 18 months ago by cjerdonek

@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 18 months 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 18 months ago by dcreech (previous) (diff)

comment:10 in reply to: ↑ 7 Changed 17 months ago by anubhav9042

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 13 months ago by timo

  • Cc timo added
  • Summary changed from db.utils _route_db can raise AttributeError: 'NoneType' object has no attribute '_state' to Add ForeignKey support to REQUIRED_FIELDS
  • Type changed from Bug to New feature

comment:12 Changed 13 months ago by anubhav9042

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

comment:13 Changed 13 months ago by anubhav9042

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 13 months ago by anubhav9042 (previous) (diff)

comment:14 Changed 13 months ago by anubhav9042

  • Needs documentation unset

comment:15 Changed 13 months ago by Tim Graham <timograham@…>

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

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