Opened 5 years ago

Closed 18 months ago

#15779 closed Bug (fixed)

admin cannot edit records with value 'add' as primary key

Reported by: Marwan Alsabbagh <marwan.alsabbagh@…> Owned by: Claude Paroz
Component: contrib.admin Version: master
Severity: Release blocker Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Problem

If you have a model with a primary key field that has the value 'add' you won't be able to edit it in the admin screens. the admin will take you to the add page instead of the change page for that record. this is a problem with the design of the urls in the admin module. Example code follows:

admin.py

from polls.models import poll
from django.contrib import admin

admin.site.register(poll)

model.py

from django.db import models

class poll(models.Model):
   id = models.CharField(max_length=200, primary_key=True)
   question = models.CharField(max_length=200)

Reproducing

  1. Create project and app, enable admin, add the above files and syncdb.
  2. Go to the admin interface and select Add poll
  3. specify 'add' as the id and 'test' as question
  4. Save
  5. Go back to the list
  6. Open the same object
  7. you will be taken to the add poll screen instead of the change poll screen for the selected object

Change History (27)

comment:1 Changed 5 years ago by Julien Phalip

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

Yes this is an issue. Now we need to find the proper way to fix it.

One (naive) idea is to make the "add/" url configurable, perhaps using a setting. But still, this wouldn't be ideal as a conflict would still potentially exist if whatever value you give to that setting is then used as a primary key.

Really, the url schema for the admin is slightly flawed here. Down the track we might want to redesign it to avoid any sort of conflict -- but that would likely be heavily backwards incompatible.

comment:2 Changed 5 years ago by Luke Plant

Triage Stage: Design decision neededAccepted

I agree with julien that the only sensible way to do this is to fix the URL structure.

Technically I don't believe our backwards compatibility guarantee ever included the URL structure of admin apps. Since Django 1.1 we've had a fully capable URL reverser for the admin, and this has been the documented way to reverse admin views. So I'm going to accept on that basis - since we've documented the correct way to do it, if people have hard coded URLs then that is like using private methods instead of the exposed public interface. We should nonetheless include a note in the backwards compatibilities section of the release note.

The patch would be quite simple, but there are some redirects which also assume the current structure and would also need to be fixed.

(/me goes off to fix those hard-coded admin URLs in a really old Django app...)

comment:3 Changed 5 years ago by Julien Phalip

By the way, there's a ticket for changing those hard coded urls in the admin, if someone's interested: #15294 :-)

comment:4 Changed 5 years ago by Ramiro Morales

Easy pickings: unset
Resolution: fixed
Status: newclosed
UI/UX: unset

I'm going to call this ticket fixed in [16857]. After that revision, it is possible to implement a custom URL scheme in the admin app because it uses named URLs in the model CRUD workflow. There is an example of an instance of a model with a CharField PK with an 'add' value in the test case added in such commit: https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/admin_custom_urls/models.py?rev=16857

The customization process isn't completely straightforward because the named URL entry one is replacing needs to be removed from the ModelAdmin URL map to force the admin app to take in account the one we are installing (see the get_urls()/remove_url() methods in the example linked above). Patches to make this easier are welcome.

Last edited 5 years ago by Ramiro Morales (previous) (diff)

comment:5 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: closedreopened

After discussion with Ramiro and Julien in IRC, reopening this because it really ought to "just work" and the right way to make it "just work" is to reconsider the admin URL structure to remove ambiguity, as discussed above.

Since this likely won't happen immediately, the workaround enabled by r16857 can be a useful stopgap for those who need it.

comment:6 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:7 Changed 20 months ago by Claude Paroz

I've done the first step of the work, by using reverse as much as possible in the various admin tests.
https://github.com/django/django/pull/4076

comment:8 Changed 20 months ago by Claude Paroz <claude@…>

In 32e6a7d3a57b2287d55e8b8efa4e8cb7643b1720:

Replaced hardcoded URLs in admin_* tests

Refs #15779. This will allow easier admin URL changes, when needed.
Thanks Simon Charette for the review.

comment:10 Changed 20 months ago by Tim Graham <timograham@…>

In cd260d03bd7c43b78ad3a394e1b62f526f40f4ce:

[1.8.x] Replaced hardcoded URLs in admin_* tests

Refs #15779. This will allow easier admin URL changes, when needed.
Thanks Simon Charette for the review.

Backport of 32e6a7d3a57b2287d55e8b8efa4e8cb7643b1720 from master

comment:11 Changed 20 months ago by Tim Graham

I am afraid that some users will have bookmarked existing edit URLs and are going to be annoyed when those now 404. I wonder if we should issue redirects for the old patterns and then have an option to turn them off for users affected by this issue.

comment:12 Changed 20 months ago by Simon Charette

Cc: Simon Charette added

I think we could safely add a temporary redirect raising a deprecation warning from the r'^(.+)/$' pattern as long as it appears after the r'^(.+)/edit/$' one.

comment:13 Changed 20 months ago by Tim Graham

Site users won't learn of that warning though. Is there a downside to keeping that redirect indefinitely?

comment:14 in reply to:  13 Changed 20 months ago by Simon Charette

Replying to timgraham:

Site users won't learn of that warning though.

Right.

Is there a downside to keeping that redirect indefinitely?

I don't see one but I would avoid using a permanent redirect here.

comment:15 Changed 20 months ago by Claude Paroz

I've just added a complementary commit for the redirect. However, I think we should get rid of it after "some" releases...

comment:16 Changed 20 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:17 Changed 20 months ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 1791a7e75af8c9e7ca54425592379fd1f840fe88:

Fixed #15779 -- Allowed 'add' primary key in admin edition

Thanks Marwan Alsabbagh for the report, and Simon Charette and
Tim Graham for the reviews.

comment:18 Changed 18 months ago by Tim Graham

Has patch: unset
Severity: NormalRelease blocker
Triage Stage: Ready for checkinAccepted

This broke the password change link at /admin/auth/user/1/change/.
We could change the URL in UserAdmin.get_urls() or update the link in the form:

diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py
index 928c4c7..84fc757 100644
--- a/django/contrib/auth/forms.py
+++ b/django/contrib/auth/forms.py
@@ -98,7 +98,7 @@ class UserChangeForm(forms.ModelForm):
     password = ReadOnlyPasswordHashField(label=_("Password"),
         help_text=_("Raw passwords are not stored, so there is no way to see "
                     "this user's password, but you can change the password "
-                    "using <a href=\"password/\">this form</a>."))
+                    "using <a href=\"../password/\">this form</a>."))
 
     class Meta:
         model = User

comment:19 Changed 18 months ago by Tim Graham

Resolution: fixed
Status: closednew

comment:20 Changed 18 months ago by Claude Paroz

Owner: changed from nobody to Claude Paroz
Status: newassigned
Version: 1.3master

Having this relative link into help_text is doomed to fail. However, I don't see any simple workaround for now, so Tim's patch just needs a test.

comment:21 Changed 18 months ago by Gert Steyn

The first paragraph of the Django URL dispatcher documentation says: "Cool URIs don’t change"...

Do we really want to change the URL structure of every Django site out there for an extreme edge case like this?

If I use a CharField as primary key nothing stops me from using "1/change" as key and we'll have exactly the same problem. If you insist on using a CharField as primary key adding some validation to exclude some "reserved" words is not to much of an ask.

comment:22 Changed 18 months ago by Claude Paroz

If I use a CharField as primary key nothing stops me from using "1/change" as key and we'll have exactly the same problem.

No, "1/change" would be converted to "change_2F1" in the URL.

comment:23 Changed 18 months ago by Claude Paroz

Err... "1_2Fchange", of course.

comment:25 Changed 18 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:26 Changed 18 months ago by Claude Paroz <claude@…>

In c2bfd76e:

Refs #15779 -- Fixed UserChangeForm regression introduced by 1791a7e75

Thanks Tim Graham for reporting the regression.

comment:27 Changed 18 months ago by Claude Paroz

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top