Opened 4 years ago

Closed 4 months ago

#15779 closed Bug (fixed)

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

Reported by: Marwan Alsabbagh <marwan.alsabbagh@…> Owned by: claudep
Component: contrib.admin Version: master
Severity: Release blocker Keywords:
Cc: charettes 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 4 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 4 years ago by lukeplant

  • Triage Stage changed from Design decision needed to Accepted

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 4 years ago by julien

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

comment:4 Changed 4 years ago by ramiro

  • Easy pickings unset
  • Resolution set to fixed
  • Status changed from new to closed
  • 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 4 years ago by ramiro (previous) (diff)

comment:5 Changed 4 years ago by carljm

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 2 years ago by aaugustin

  • Status changed from reopened to new

comment:7 Changed 6 months ago by claudep

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 6 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 6 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 6 months ago by timgraham

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 6 months ago by charettes

  • Cc charettes 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 follow-up: Changed 6 months ago by timgraham

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 6 months ago by charettes

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 6 months ago by claudep

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 6 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

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

  • Resolution set to fixed
  • Status changed from new to closed

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 4 months ago by timgraham

  • Has patch unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Ready for checkin to Accepted

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 4 months ago by timgraham

  • Resolution fixed deleted
  • Status changed from closed to new

comment:20 Changed 4 months ago by claudep

  • Owner changed from nobody to claudep
  • Status changed from new to assigned
  • Version changed from 1.3 to master

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 4 months ago by gertsteyn

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 4 months ago by claudep

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 4 months ago by claudep

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

comment:25 Changed 4 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:26 Changed 4 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 4 months ago by claudep

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top