Opened 5 years ago
Closed 5 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 , 5 years ago
comment:2 by , 5 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 5 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:5 by , 5 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_fieldschecks could be changed todb_field.name in self.raw_id_fields or db_field.attname in self.raw_id_fieldsbut 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 , 5 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 , 5 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Alternatively all
db_field.name in self.raw_id_fieldschecks could be changed todb_field.name in self.raw_id_fields or db_field.attname in self.raw_id_fieldsbut that seems like a lot of work for little benefit.