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 Simon Charette, 4 years ago

Alternatively all db_field.name in self.raw_id_fields checks could be changed to db_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.

comment:2 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Hasan Ramezani, 4 years ago

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:4 by Jacob Walls, 4 years ago

Has patch: set

comment:5 by Hasan Ramezani, 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 to db_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:

Should I change them?

comment:6 by Simon Charette, 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.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:7 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 20ddc3b:

Fixed #32494 -- Adjusted system check for raw_id_fields to warn about Field.attname.

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