Opened 6 years ago

Last modified 14 months ago

#30386 assigned Bug

Admin foreign key widgets don't quote keys.

Reported by: Joshua Goodwin Owned by: Oluwayemisi Ismail
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 (3)

ticket_30386.zip (16.5 KB ) - added by Mariusz Felisiak 5 years ago.
issue_30386_with_patch_and_save.mp4 (118.2 KB ) - added by Mariusz Felisiak 21 months ago.
issue_30386.mp4 (230.6 KB ) - added by Mariusz Felisiak 21 months ago.

Download all attachments as: .zip

Change History (29)

comment:1 by Mariusz Felisiak, 6 years ago

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

comment:2 by zeynel, 6 years ago

Owner: changed from nobody to zeynel
Status: newassigned

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

comment:3 by zeynel, 6 years ago

Has patch: set

comment:4 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by Mariusz Felisiak, 5 years ago

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/.

by Mariusz Felisiak, 5 years ago

Attachment: ticket_30386.zip added

comment:6 by Alexander Pervakov, 3 years ago

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.RelatedField:

class RelatedField(FieldCacheMixin, Field):

    def check(self, **kwargs):
        return [
            # ...
            *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',
                )
            ]
Last edited 3 years ago by Alexander Pervakov (previous) (diff)

comment:7 by Mariusz Felisiak, 3 years ago

Owner: zeynel removed
Status: assignednew

comment:8 by Yhemisi, 21 months ago

Owner: set to Oluwayemisi Ismail
Status: newassigned

in reply to:  5 ; comment:9 by Yhemisi, 21 months ago

Replying to Mariusz Felisiak:

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/.

@Mariusz Felisiak I have checked the attached project and also tried to add a new pizza which I was able to save models. Could you please confirm if you are unable to save.

comment:10 by Yhemisi, 21 months ago

As regards to this ticket, I worked on it following this steps

class House(models.Model):
    name = models.CharField(max_length=255, primary_key=True)
    def __str__(self):
        return self.name

class Room(models.Model):
    house = models.ForeignKey(House, on_delete=models.CASCADE)
    def __str__(self):
        return self.house.name

using the quote function from django.contrib.admin.utils should fix this

 def test_foreign_key_raw_id_widget_renders_quoted_pk_in_change_url(self):
        house = House.objects.create(name='?a=b')
        rel = Room._meta.get_field('house').remote_field
        w = widgets.ForeignKeyRawIdWidget(rel, widget_admin_site)

        # apply quote function to primary key value
        pk_quoted = quote(str(house.pk))

        # render the widget
        rendered = w.render('test', house.pk, attrs={})

        # check that the primary key is properly quoted in the rendered HTML
        self.assertIn(f'href="/admin_widgets/house/{pk_quoted}/change/"', rendered)
Last edited 21 months ago by Mariusz Felisiak (previous) (diff)

in reply to:  9 comment:11 by Mariusz Felisiak, 21 months ago

Replying to Yhemisi:

Replying to Mariusz Felisiak:

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/.

@Mariusz Felisiak I have checked the attached project and also tried to add a new pizza which I was able to save models. Could you please confirm if you are unable to save.

The issue is that you will not be able to add a new pizza with the proposed patch.

comment:12 by Yhemisi, 21 months ago

Last edited 21 months ago by Yhemisi (previous) (diff)

comment:13 by Carlton Gibson, 21 months ago

The issue is that you will not be able to add a new pizza with the proposed patch.

OK, I'm struggling to reproduce this now.

Even at 25b5eea8cdc69a353bb2d22ea2012b09df6c62e4 — which was the reproduce commit above, with the test project, in Firefox, Edge and Safari, I'm able to create Pizzas with the Topping _40 without error. I can't work out why I'm not seeing this. (Like, did browsers change? 🤔) (

The particular tests from the PRs checking the quoting fail — but pausing at those points — there's no error saving — i.e. the key isn't quoted but its working (AFAICS 🤔)

@Yhemisi: given that you're working on this, can you adapt the Selenium test from the original PR to visit the add a Pizza page and create a new instance with a topping with e.g. _40 PK? Does that pass on main with no other changes?

Last edited 21 months ago by Carlton Gibson (previous) (diff)

comment:14 by Carlton Gibson, 21 months ago

This added on top of the model changes from the original PR already passes:

# In class RelatedFieldWidgetSeleniumTests...

    def test_pizzas_with_topping_can_be_created(self):
        from selenium.webdriver.common.by import By

        self.admin_login(username="super", password="secret", login_url="/")
        self.selenium.get(
            self.live_server_url + reverse("admin:admin_widgets_pizza_add")
        )
        self.select_option("#id_topping", self.toppings[0].pk)
        name_field = self.selenium.find_element(By.ID, "id_name")
        name_field.send_keys("testing pizza")
        save_button_css_selector = ".submit-row > input[type=submit]"
        self.selenium.find_element(By.CSS_SELECTOR, save_button_css_selector).click()
        self.wait_for_text(
            "li.success", "The pizza “testing pizza” was added successfully."
        )

by Mariusz Felisiak, 21 months ago

comment:15 by Mariusz Felisiak, 21 months ago

I can still reproduce the issue with save when PR 11280 is applied, see issue_30386_with_patch_and_save.mp4.

by Mariusz Felisiak, 21 months ago

Attachment: issue_30386.mp4 added

comment:16 by Mariusz Felisiak, 21 months ago

With the new PR I can reproduce the original issue, see issue_30386.mp4

comment:17 by Carlton Gibson, 21 months ago

Yes, good — thanks for the clarification — got it now. 👍 Thanks!

It looks like the quoting needs to occur in RelatedObjectLookups.js updateRelatedObjectLinks() as per the second part of comment:2

@Yhemisi: does that give you enough pointers to work on?

in reply to:  17 comment:18 by Yhemisi, 21 months ago

Replying to Carlton Gibson:

Yes, good — thanks for the clarification — got it now. 👍 Thanks!

It looks like the quoting needs to occur in RelatedObjectLookups.js updateRelatedObjectLinks() as per the second part of comment:2

@Yhemisi: does that give you enough pointers to work on?

Yes It does. Thank you. I will look it up

in reply to:  16 ; comment:19 by Yhemisi, 20 months ago

Replying to Mariusz Felisiak:

With the new PR I can reproduce the original issue, see issue_30386.mp4

@Mariusz Felisiak please i want to ask these questions below to be sure that i understand the problem.

  1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?
  1. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters
  2. Are you trying to say that both issues exist differently and the solution doesn't affect each other?

in reply to:  19 ; comment:20 by Mariusz Felisiak, 20 months ago

  1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?

No. There is an original issue from the ticket description, that you can see on the attached record issue_30386.mp4. Your patch doesn't fix it.

  1. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters

No. It's a side-effect of patch proposed by Zeynel, so Zeynel's patch fixes the original issue (see issue_30386.mp4) but introduces a regression (see issue_30386_with_patch_and_save.mp4​). That's why we couldn't accept it.

in reply to:  20 comment:21 by Yhemisi, 20 months ago

Replying to Mariusz Felisiak:

  1. Are you trying to say that the issue is not "Admin foreign key widgets don't quote keys" but the issue is that one will not be able to add a new pizza ?

No. There is an original issue from the ticket description, that you can see on the attached record issue_30386.mp4. Your patch doesn't fix it.

  1. Is that the inability to add a new pizza is caused by the issue of admin foreign key widgets not quoting keys with special characters

No. It's a side-effect of patch proposed by Zeynel, so Zeynel's patch fixes the original issue (see issue_30386.mp4) but introduces a regression (see issue_30386_with_patch_and_save.mp4​). That's why we couldn't accept it.

I got it now. Thank you.

comment:22 by Yhemisi, 19 months ago

Patch needs improvement: unset

comment:23 by Mariusz Felisiak, 19 months ago

Patch needs improvement: set

comment:24 by Natalia Bidart, 18 months ago

For those reading from the top that can't find the "original PR", this is the one: PR

comment:25 by Sarah Boyce, 14 months ago

Patch needs improvement: unset

(marking for review as we think the bug is fixed 🤞)

comment:26 by Mariusz Felisiak, 14 months ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top