Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22266 closed Bug (fixed)

CharField primary keys with underscores are (un)escaped differently in the admin pages

Reported by: victor.varvariuc@… Owned by: nott
Component: contrib.admin Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Migrated from here: https://code.djangoproject.com/ticket/18381#comment:6

Although that bug was fixed 2 years ago, related issues seem still present in the latest stable Django release 1.6.2.
CharField primary keys with underscores are (un)escaped differently in the admin pages, and when reversing admin URLs with django.core.urlresolvers.reverse() or the {% url %} template tag.
The admin pages add "5F" after the underscore in when rendering URLs, but reverse() does not. As a result, links from custom pages into the admin break when the object's primary key contains underscores.

Steps to reproduce:

  • Create a model with "id = models.CharField(max_length=100, primary_key=True)" or other similar text primary key field,
  • Register the model to the admin site,
  • Create a model instance with test_123 as the primary key,
  • Visit the admin, and observe admin URLs: they are of the format "/admin/<app>/<model>/test_5F123/"
  • Open the shell, and call reverse('admin:app_model_delete', args=test_123)
  • The URLs returned are of the format "/admin/<app>/<model>/test_123/" - no 5F
  • As a result, if these links are embedded in pages, they don't work - the admin page tries to decode the primary key, expects 5F, and does not find the object.
[trains] vic@vic ~/projects/trains (master *) $ ./manage.py shell
Python 3.3.2+ (default, Oct  9 2013, 14:50:09) 

>>> from django.core.urlresolvers import reverse

>>> reverse('admin:trains_route_change', args=('test_123',))
>>> '/trains/route/test_123/'

Change History (8)

comment:1 by Claude Paroz, 10 years ago

Might be related to #22223

comment:2 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:3 by nott, 10 years ago

Owner: changed from nobody to nott
Status: newassigned

comment:4 by nott, 10 years ago

This issue is not related to #22223

comment:5 by nott, 10 years ago

ModelAdmin.get_urls defines the urls like these:

            url(r'^$', wrap(self.changelist_view), name='%s_%s_changelist' % info),
            url(r'^add/$', wrap(self.add_view), name='%s_%s_add' % info),
            url(r'^(.+)/history/$', wrap(self.history_view), name='%s_%s_history' % info),
            url(r'^(.+)/delete/$', wrap(self.delete_view), name='%s_%s_delete' % info),
            url(r'^(.+)/$', wrap(self.change_view), name='%s_%s_change' % info),

As long as we do not know the exact primary key regexp, we have to use (.+)
Consider we have 2 entries with the following PKs: "test_123" and "test_123/history". There is no way to distinguish change view for "test_123/history" and history view for "test_123".
That is why PK quoting was introduced. django.contrib.admin.utils contains 2 functions: quote and unquote. So reversing admin url should usually look like:

reverse('admin:app_model_change', args=(quote(pk),))

Quoting and unquoting doesn't change numerical PKs, that is why this problem haven't been noticed before.
By the way, there is a bug with quoting not being applied in add_view, but it can be easily fixed - that is why the first redirect doesn't work.

I suppose we are really lacking the documentation (https://docs.djangoproject.com/en/1.6/ref/contrib/admin/#overriding-vs-replacing-an-admin-template is evertyhing I could find).

There is no way to get rid of quoting if we leave (.+) url pattern for PK. So we can leave quoting/unquoting as is and fix the redirect in add_view.
Or we can make this pattern configurable, e.g.:

class ModelAdmin(BaseModelAdmin):
    [...]
    pk_regexp = ".+"
    [...]
    def get_pk_regexp(self):
        return self.pk_regexp
Last edited 10 years ago by nott (previous) (diff)

comment:7 by Russell Keith-Magee <russell@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In ebd70d4d00c252d5122c13906da5bddc8de0bce5:

Fixed #22266 - quote PK before redirecting away from add_view (django.contrib.admin)

comment:8 by Russell Keith-Magee <russell@…>, 10 years ago

In 75d2da797e100442d3573dfa7ae490972cea32d8:

[1.7.x] Fixed #22266 - quote PK before redirecting away from add_view (django.contrib.admin)

Backport of ebd70d4d00c252d5122c13906da5bddc8de0bce5 from master.

Note: See TracTickets for help on using tickets.
Back to Top