[Django] #30386: Admin foreign key widgets don't quote keys
Django
-----------------------------------------+------------------------
Reporter: jclgoodwin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
In django.contrib.admin.ModelAdmin, _changeform_view, _delete_view and
history_view and do unquote(object_id):
However, ForeignKeyRawIdWidget and RelatedFieldWidgetWrapper create links
to this view without calling quote():
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?'.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------+------------------------------------
Reporter: Joshua Goodwin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* version: 2.2 => master
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:1>
Django
--------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by zeynel):
* owner: nobody => zeynel
* status: new => assigned
Comment:
I believe there are two main parts to handle to fix this bug.
- Quoting `obj.pk` value on
https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/widgets.py#L191
- Quoting object pk(or sending quoted values in context) on client side
when user changes the selection on RelatedFieldWidget
https://github.com/django/django/blob/89a2216486fa8a0513cbb1d49d2d587d4116c60b/django/contrib/admin/static/admin/js/admin/RelatedObjectLookups.js#L69
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:2>
Django
--------------------------------+------------------------------------
Reporter: Joshua Goodwin | Owner: zeynel
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by zeynel):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner: zeynel
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:4>
Django
--------------------------------+------------------------------------
Reporter: Joshua Goodwin | Owner: zeynel
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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/`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:5>
Django
--------------------------------+------------------------------------
Reporter: Joshua Goodwin | Owner: zeynel
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* Attachment "ticket_30386.zip" added.
Django
--------------------------------+------------------------------------
Reporter: Joshua Goodwin | Owner: zeynel
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:6>
Django
--------------------------------+------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: zeynel => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Oluwayhemisi):
* owner: (none) => Oluwayemisi Ismail
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
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)
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:9 Yhemisi]:
> Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
Doe it mean the quote function isn't a better way of solving this?
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> The issue is that you will not be able to add a new pizza with the
proposed patch.
OK, I'm struggle 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
This added on top of the model changes from the
[https://github.com/django/django/pull/11280 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."
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* Attachment "issue_30386_with_patch_and_save.mp4" added.
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
I can still reproduce the issue with save when PR 11280 is applied, see
[https://code.djangoproject.com/attachment/ticket/30386/issue_30386_with_patch_and_save.mp4
issue_30386_with_patch_and_save.mp4].
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* Attachment "issue_30386.mp4" added.
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
With the new [https://github.com/django/django/pull/16589 PR] I can
reproduce the original issue, see
[https://code.djangoproject.com/attachment/ticket/30386/issue_30386.mp4
issue_30386.mp4]
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
Replying to [comment:17 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
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
Replying to [comment:16 Mariusz Felisiak]:
> With the new [https://github.com/django/django/pull/16589 PR] I can
reproduce the original issue, see
[https://code.djangoproject.com/attachment/ticket/30386/issue_30386.mp4
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 ?
2. 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
3. Are you trying to say that both issues exist differently and the
solution doesn't affect each other?
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by 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
[https://code.djangoproject.com/attachment/ticket/30386/issue_30386.mp4
issue_30386.mp4]. Your patch doesn't fix it.
> 2. 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 [https://github.com/django/django/pull/11280
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Yhemisi):
Replying to [comment:20 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
[https://code.djangoproject.com/attachment/ticket/30386/issue_30386.mp4
issue_30386.mp4]. Your patch doesn't fix it.
>
> > 2. 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 [https://github.com/django/django/pull/11280
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Yhemisi):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
For those reading from the top that can't find the "original PR", this is
the one: [https://github.com/django/django/pull/11280 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
(marking for review as we think the bug is fixed 🤞)
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Joshua Goodwin | Owner:
| Oluwayemisi Ismail
Type: Bug | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30386#comment:26>