Opened 4 years ago
Closed 4 years ago
#32494 closed Bug (fixed)
Admin's raw_id_field check admin.E002 doesn't catch .attname mis-references
Reported by: | Simon Charette | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | contrib.admin | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Since admin.E002
relies on models.Options.get_field
which allows retrieval of fields by both name
and attname
referring to fields by attname
while only name
is taken into consideration allows the check to pass while raw_id_fields
is not honoured.
e.g.
class BookAdmin(ModelAdmin): raw_id_fields = ['author_id']
passes admin.E002
but the author
field won't use the raw_id feature.
The _check_raw_id_fields_item
method should also make sure to check field.name == field_name
on field retrieval success and return refer_to_missing_field(field=field_name, option=label, obj=obj, id='admin.E002')
when it's not the case.
Change History (8)
comment:1 by , 4 years ago
comment:2 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 4 years ago
I've created a PR to check field.name
with field_name
in _check_raw_id_fields_item
method.
Alternatively all
db_field.name in self.raw_id_fields
checks could be changed todb_field.name in self.raw_id_fields or db_field.attname in self.raw_id_fields
but that seems like a lot of work for little benefit.
I found two lines in the django.contrib.admin.options
which have db_field.name in self.raw_id_fields
check:
- https://github.com/django/django/blob/d3ecef26b9fda02b88f925a800ae38dd5873c878/django/contrib/admin/options.py#L229
- https://github.com/django/django/blob/d3ecef26b9fda02b88f925a800ae38dd5873c878/django/contrib/admin/options.py#L262
Should I change them?
comment:6 by , 4 years ago
Hasan, I think it's one way or the other.
We either add tested support for referring to fields by .attname
by changing all db_field.name in raw_id_fields
checks to {db_field.name, db_field.attname}.union(raw_id_fields)
or adjust admin.E002
not to return false negative when dealing with .attname
references.
Either way works and I don't have strong feeling about one approach over the other as long as its not silently allowed to be misconfigured.
comment:7 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Alternatively all
db_field.name in self.raw_id_fields
checks could be changed todb_field.name in self.raw_id_fields or db_field.attname in self.raw_id_fields
but that seems like a lot of work for little benefit.