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

[Marwan Alsabbagh <marwan.alsabbagh@…>]

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

[Julien Phalip]

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.

[Luke Plant]

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

[Julien Phalip]

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

[Ramiro Morales]

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 when resolving hthe URLs in the model CRUD workflow. There is an example of a model with a CharField PK and with an instance with an 'add' value for it 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 customziation 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 remove_url() method in the example linked above). Patches to make this easier are welcome.

[Carl Meyer]

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.

[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

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

[Claude Paroz]

​https://github.com/django/django/pull/4088

[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

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

[Simon Charette]

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.

[Tim Graham]

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

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

[Claude Paroz]

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

[Claude Paroz <claude@…>]

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.

[Tim Graham]

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

[Claude Paroz]

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.

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

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

[Claude Paroz]

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

[Claude Paroz]

​https://github.com/django/django/pull/4400

[Claude Paroz <claude@…>]

In c2bfd76e:

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

Thanks Tim Graham for reporting the regression.