Opened 4 years ago

Last modified 14 months ago

#30386 new Bug

Admin foreign key widgets don't quote keys.

Reported by: Joshua Goodwin Owned by:
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In django.contrib.admin.ModelAdmin, _changeform_view, _delete_view and history_view and do unquote(object_id):

https://github.com/django/django/blob/917fd9d03fdd21538864af4b412ac30b36d99268/django/contrib/admin/options.py#L1537

However, ForeignKeyRawIdWidget and RelatedFieldWidgetWrapper create links to this view without calling quote():

https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/widgets.py#L191

Steps to reproduce:

  1. Create two models: Topping with a CharField primary key, and Pizza with a ForeignKey to Topping. Register both models with the admin site.
  2. Create a Topping with the primary key '_40', and a Pizza with that topping
  3. In the admin site, go the the /change/ page for the Pizza, and click on the 'change' icon for the Topping foreign key, or (if using ForeignKeyRawIdWidget) the link to the Topping '_40'.
  4. See message 'Topping with ID "@" doesn't exist. Perhaps it was deleted?'.

Attachments (1)

ticket_30386.zip (16.5 KB) - added by Mariusz Felisiak 3 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by Mariusz Felisiak

Summary: Admin foreign key widgets don't quote keysAdmin foreign key widgets don't quote keys.
Triage Stage: UnreviewedAccepted
Version: 2.2master
Last edited 4 years ago by Mariusz Felisiak (previous) (diff)

comment:2 Changed 4 years ago by zeynel

Owner: changed from nobody to zeynel
Status: newassigned

I believe there are two main parts to handle to fix this bug.

comment:3 Changed 4 years ago by zeynel

Has patch: set

comment:4 Changed 3 years ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:5 Changed 3 years ago by Mariusz Felisiak

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I still have some issue when saving models, please check attached project and try to add a new pizza with e.g. _40 topping on admin/test_one/pizza/add/.

Changed 3 years ago by Mariusz Felisiak

Attachment: ticket_30386.zip added

comment:6 Changed 14 months ago by Alexander Pervakov

I'm dive deeper into this and found that quote(obj.id) maybe not the best option and should be debated but the presence of a warning message when not isinstance(obj.pk, int) would be great and will lead people to this thread. Because stackoverflow is full of questions about quoting the link in admin but no one provide refs to this ticket that I found after couple hours of code investigating and testing.

Something like this in django.db.models.fields.related.ForeignKey:

class ForeignKey(ForeignObject):

    def check(self, **kwargs):
        return [
            *super().check(**kwargs),
            *self._check_on_delete(),
            *self._check_unique(),
            *self._check_pk_int(),
        ]

    def _check_pk_int(self):
        remote_pk = getattr(self.remote_field, 'pk', None)
        if isinstance(remote_pk, int):
            return []
        else:
            return [
                checks.Warning(
                    'Not integer primary key may lead you to unpredictable url in admin when ForeignKeyRawIdWidget is used',
                    hint='Use extra quote() in this relation',
                    obj=self,
                    id='fields.W346',
                )
            ]
Version 2, edited 14 months ago by Alexander Pervakov (previous) (next) (diff)

comment:7 Changed 14 months ago by Mariusz Felisiak

Owner: zeynel deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top