Admin throws 500 instead of 404 error when passing a string as the PK argument for a model with an integer PK field.

[Tai Lee]

The situation where a PK of the incorrect type (string instead of integer) is specified in an admin URL is probably very rare, as most people will only ever navigate to the add/change form via the admin which hopefully generates a correct link, or at least the user types in a PK of the correct type (even if no object exists with that PK). I only discovered this through another bug in my code that was making requests to URLs that don't exist, e.g. /admin/transfers/booking/global.css/.

Such a request *should* return a 404 File Not Found error. Even though the PK is of an incorrect type, the URL does not exist, rather than exists but encounters a fatal exception. The error is easy to reproduce. Just fire up the admin, navigate to any model which expects an integer PK (e.g. /admin/auth/user/1/) and change the PK to a string (e.g. /admin/auth/user/abc/).

The exception that is generated is:

Environment:

Request Method: GET
Request URL: http://localhost:8000/admin/auth/user/abc/
Django Version: 1.1 beta 1 SVN-10838
Python Version: 2.4.6
Installed Applications:
['django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware')


Traceback:
File "/path/to/django/core/handlers/base.py" in get_response
  92.                 response = callback(request, *callback_args, **callback_kwargs)
File "/path/to/django/contrib/admin/options.py" in wrapper
  226.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/path/to/django/contrib/admin/sites.py" in inner
  184.             return view(request, *args, **kwargs)
File "/path/to/django/db/transaction.py" in _commit_on_success
  240.                 res = func(*args, **kw)
File "/path/to/django/contrib/admin/options.py" in change_view
  792.             obj = self.queryset(request).get(pk=unquote(object_id))
File "/path/to/django/db/models/query.py" in get
  268.         clone = self.filter(*args, **kwargs)
File "/path/to/django/db/models/query.py" in filter
  466.         return self._filter_or_exclude(False, *args, **kwargs)
File "/path/to/django/db/models/query.py" in _filter_or_exclude
  484.             clone.query.add_q(Q(*args, **kwargs))
File "/path/to/django/db/models/sql/query.py" in add_q
  1665.                             can_reuse=used_aliases)
File "/path/to/django/db/models/sql/query.py" in add_filter
  1608.                 connector)
File "/path/to/django/db/models/sql/where.py" in add
  56.                 obj, params = obj.process(lookup_type, value)
File "/path/to/django/db/models/sql/where.py" in process
  269.                 params = self.field.get_db_prep_lookup(lookup_type, value)
File "/path/to/django/db/models/fields/__init__.py" in get_db_prep_lookup
  210.             return [self.get_db_prep_value(value)]
File "/path/to/django/db/models/fields/__init__.py" in get_db_prep_value
  361.         return int(value)

Exception Type: ValueError at /admin/auth/user/abc/
Exception Value: invalid literal for int(): abc

This bug is present in Django version 1.1 beta 1 SVN-10838.

[Tai Lee]

Failing test case.

[Tai Lee]

I've uploaded a patch with a failing test case.

I suspect that the best place to fix this is in the admin application's change_view. The raw ORM should probably continue to fail with an exception when a user tries to get an object with a PK of an invalid type, but this particular (and perhaps other) core admin views which have been designed to work with multiple models which have integer and string PKs need to catch this and return a correct response.

Likewise, if users have their own URLs which need to work with multiple models which have both integer or string PKs, they will need to catch this in their own code. For cases where the URL is only specifying one type of PK (integer or string) for one model (or several which all use the same type of PK), the user can achieve the correct response by changing their URL pattern to accept only digits or letters in the PK field.

One alternative would be for the get() (and related/affected) methods to always raise DoesNotExist when a PK of an invalid type is specified. This would avoid the user having to catch this in their own code, but could mask other problems and feels like the wrong approach.

[Karen Tracey]

There's an easy workaround for this -- don't try to access admin with incorrect urls. Thus I don't see how this is critical to fix before 1.1.

[Tai Lee]

Sorry, I thought that all straight bugs were targeted for 1.1.

[Chris Beaven]

This has always annoyed me too. Here's the fix.

[Chris Beaven]

Right, so this should handle PKs of any type and also abstracts some common code.

[Luke Plant]

(In [12011]) Fixed #11191 - Admin throws 500 instead of 404 for PK of incorrect type

Thanks to mmachine for report and test, and Chris Beaven for the patch

[Luke Plant]

(In [12012]) [1.1.X] Fixed #11191 - Admin throws 500 instead of 404 for PK of incorrect type

Thanks to mmachine for report and test, and Chris Beaven for the patch

Backport of r12011 from trunk

[hmmurray@…]

When is this fix going to be applied to trunk? I get heaps of 500 server error emails from browsers requesting weird urls such as:

/admin/myapp/mymodel/favicon.ico/

These really should be 404's.

[Luke Plant]

It was applied to trunk six months ago.

[Jacob]

Milestone 1.2 deleted