#21755 closed New feature (fixed)
Add ForeignKey support to REQUIRED_FIELDS
Reported by: | Chris Jerdonek | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
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 (17)
comment:1 by , 11 years ago
Cc: | added |
---|
comment:2 by , 11 years ago
comment:3 by , 11 years ago
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 ofNone
in the default implementation ofcreatesuperuser
, even though it is not needed in the out-of-the-box case, - allow
None
to be passed in the foreign key field version ofvalidate()
(for example by catching the AttributeError indb.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 by , 11 years ago
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 by , 11 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → 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 by , 11 years ago
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.
follow-up: 10 comment:7 by , 11 years ago
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 by , 11 years ago
@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 by , 11 years ago
@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'
comment:10 by , 11 years ago
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 theUSERNAME_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 by , 10 years ago
Cc: | added |
---|---|
Summary: | db.utils _route_db can raise AttributeError: 'NoneType' object has no attribute '_state' → Add ForeignKey support to REQUIRED_FIELDS |
Type: | Bug → New feature |
comment:12 by , 10 years ago
Cc: | added |
---|---|
Component: | Database layer (models, ORM) → contrib.auth |
Has patch: | set |
Needs documentation: | set |
Version: | 1.6 → master |
comment:13 by , 10 years ago
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,
comment:14 by , 10 years ago
Needs documentation: | unset |
---|
comment:15 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.