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):
However, ForeignKeyRawIdWidget and RelatedFieldWidgetWrapper create links to this view without calling quote():
Steps to reproduce:
- Create two models: Topping with a CharField primary key, and Pizza with a ForeignKey to Topping. Register both models with the admin site.
- Create a Topping with the primary key '_40', and a Pizza with that topping
- 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'.
- See message 'Topping with ID "@" doesn't exist. Perhaps it was deleted?'.
Attachments (3)
Change History (29)
comment:1 by , 6 years ago
Summary: | Admin foreign key widgets don't quote keys → Admin foreign key widgets don't quote keys. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 2.2 → master |
comment:2 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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
comment:3 by , 6 years ago
Has patch: | set |
---|
comment:4 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
follow-up: 9 comment:5 by , 5 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
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 , 5 years ago
Attachment: | ticket_30386.zip added |
---|
comment:6 by , 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', ) ]
comment:7 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:8 by , 21 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
follow-up: 11 comment:9 by , 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 onadmin/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 , 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)
comment:11 by , 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 onadmin/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:13 by , 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?
comment:14 by , 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 , 21 months ago
Attachment: | issue_30386_with_patch_and_save.mp4 added |
---|
comment:15 by , 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 , 21 months ago
Attachment: | issue_30386.mp4 added |
---|
follow-up: 19 comment:16 by , 21 months ago
With the new PR I can reproduce the original issue, see issue_30386.mp4
follow-up: 18 comment:17 by , 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?
comment:18 by , 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
follow-up: 20 comment:19 by , 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.
- 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 ?
- 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
- Are you trying to say that both issues exist differently and the solution doesn't affect each other?
follow-up: 21 comment:20 by , 20 months ago
- 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.
- 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.
comment:21 by , 20 months ago
Replying to Mariusz Felisiak:
- 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.
- 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 , 19 months ago
Patch needs improvement: | unset |
---|
comment:23 by , 19 months ago
Patch needs improvement: | set |
---|
comment:24 by , 18 months ago
For those reading from the top that can't find the "original PR", this is the one: PR
comment:25 by , 14 months ago
Patch needs improvement: | unset |
---|
(marking for review as we think the bug is fixed 🤞)
comment:26 by , 14 months ago
Patch needs improvement: | set |
---|
Reproduced at 25b5eea8cdc69a353bb2d22ea2012b09df6c62e4.