Opened 16 months ago

Closed 14 months ago

Last modified 14 months 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 Changed 16 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Might be related to #22223

comment:2 Changed 16 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 14 months ago by nott

  • Owner changed from nobody to nott
  • Status changed from new to assigned

comment:4 Changed 14 months ago by nott

This issue is not related to #22223

comment:5 Changed 14 months ago by nott

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 14 months ago by nott (previous) (diff)

comment:7 Changed 14 months ago by Russell Keith-Magee <russell@…>

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

In ebd70d4d00c252d5122c13906da5bddc8de0bce5:

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

comment:8 Changed 14 months ago by Russell Keith-Magee <russell@…>

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