Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added `SingleObjectMixin.lookup_field`. #1205

Closed
wants to merge 5 commits into from

3 participants

@tomchristie

Deprecates pk_url_kwarg, slug_url_kwarg, and slug_field.

References trac20342.

Quoting the trac ticket...

SingleObjectMixin currently exposes these three attributes and one method all dealing with queryset filter arguments...

pk_url_kwarg = 'pk'
slug_field = 'slug'
slug_url_kwarg = 'slug'
get_slug_field()

I was wondering if it would be worth considering simplifying the API somewhat, by moving those three attributes, and that method, into a PendingDeprecation state, and replacing their usage with a single attribute instead, that is used both as the URL kwarg, and as the queryset filter.

lookup_field = 'pk'

That'd (eventually) lead to a simpler get_object implementation internally, and (immediately) present a slightly nicer, simpler API.

docs/releases/1.6.txt
@@ -334,6 +334,21 @@ deprecated but will not be removed from Django until version 1.8.
.. _recommendations in the Python documentation: http://docs.python.org/2/library/doctest.html#unittest-api
+Simplified DetailView object lookups
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In previous versions, object lookup in the :class:`DetailView` was controlled
+by the ``pk_url_kwarg``, ``slug_field``, and ``slug_url_kwarg`` attributes.
+These have now been replaced with a single ``slug_field`` attribute.
+
+The ``slug_field`` attribute should correspond with the model field that
+should be used for object lookups, and should also correspond with a named
+argument in the URLconf.
+
+The default value for the ``slug_field`` attribute is ``'pk'``. If you are
@mjtamlyn Collaborator

lookup_field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mjtamlyn
Collaborator

Whilst I appreciate the API simplification, and I agree that the current number of field is a bit excessive, I am quite strongly against having to explicity set slug every time you want to use it. To me, this is, and maybe should be the more normal use case than pk in general web development. Django is quite strongly in favour of nice urls and provides all these helpers for creating and using slugs, it seems weird to me that we should remove this default behaviour. I know this results in more magic, but it makes a vastly smoother upgrade process for most users and also doesn't discourage what to me is a better practice.

docs/releases/1.6.txt
@@ -334,6 +334,21 @@ deprecated but will not be removed from Django until version 1.8.
.. _recommendations in the Python documentation: http://docs.python.org/2/library/doctest.html#unittest-api
+Simplified DetailView object lookups
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In previous versions, object lookup in the :class:`DetailView` was controlled
+by the ``pk_url_kwarg``, ``slug_field``, and ``slug_url_kwarg`` attributes.
+These have now been replaced with a single ``slug_field`` attribute.
@bmispelon Collaborator

slug_field should be lookup_field, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
docs/releases/1.6.txt
@@ -334,6 +334,21 @@ deprecated but will not be removed from Django until version 1.8.
.. _recommendations in the Python documentation: http://docs.python.org/2/library/doctest.html#unittest-api
+Simplified DetailView object lookups
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In previous versions, object lookup in the :class:`DetailView` was controlled
+by the ``pk_url_kwarg``, ``slug_field``, and ``slug_url_kwarg`` attributes.
+These have now been replaced with a single ``lookup_field`` attribute.
+
+The ``lookup_field`` attribute should correspond with the model field that
+should be used for object lookups, and should also correspond with a named
@bmispelon Collaborator

I'm not a native speaker, but isn't there one "should" too many?

The lookup_field attribute should correspond with the model field that should be used for object lookups...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tomchristie

I guess it's really a matter of design style - I'd personally really like to see the GCBVs move to a simpler, more explicit API where possible, and this feels like one of the lower hanging fruits.

From my point of view it's actually beneficially to have to specify lookup_field = 'slug' in the slug field case. It's trivial to add to the view, makes the underlying behavior more obvious, and helps clean up the API. Cases like lookup_field = 'username' or lookup_field = 'uuid' are also less jarring than the corresponding slug_field = 'username' case, where 'slug' is used in the URLconf, but 'username' is used as the lookup field.

Having said that, there's clearly validity in sticking with the existing style. It works out-of-the box for the two most common cases, and I'm sure there'd be some complaints about the required explicitness if this change did make it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
42 django/views/generic/detail.py
@@ -5,6 +5,7 @@
from django.http import Http404
from django.utils.translation import ugettext as _
from django.views.generic.base import TemplateResponseMixin, ContextMixin, View
+import warnings
class SingleObjectMixin(ContextMixin):
@@ -13,8 +14,11 @@ class SingleObjectMixin(ContextMixin):
"""
model = None
queryset = None
- slug_field = 'slug'
context_object_name = None
+ lookup_field = 'pk'
+
+ # The following are pending deprecation
+ slug_field = 'slug'
slug_url_kwarg = 'slug'
pk_url_kwarg = 'pk'
@@ -30,22 +34,40 @@ def get_object(self, queryset=None):
if queryset is None:
queryset = self.get_queryset()
- # Next, try looking up by primary key.
- pk = self.kwargs.get(self.pk_url_kwarg, None)
- slug = self.kwargs.get(self.slug_url_kwarg, None)
- if pk is not None:
+ lookup = self.kwargs.get(self.lookup_field)
+ pk = self.kwargs.get(self.pk_url_kwarg)
+ slug = self.kwargs.get(self.slug_url_kwarg)
+
+ # Try looking up by whichever field is specified in `lookup_field`.
+ if lookup is not None:
+ queryset = queryset.filter(**{self.lookup_field: lookup})
+
+ # Next, try looking up by primary key. Note that we only attempt this
+ # deprecated lookup style if `lookup_field` has not been explicitly set.
+ elif pk is not None and self.lookup_field == 'pk':
+ warnings.warn(
+ "Usage of `pk_url_kwarg` is pending deprecation. "
+ "Set `lookup_field` on the '%s' view instead." %
+ self.__class__.__name__,
+ PendingDeprecationWarning)
queryset = queryset.filter(pk=pk)
- # Next, try looking up by slug.
- elif slug is not None:
+ # Next, try looking up by slug. Note that we only attempt this
+ # deprecated lookup style if `lookup_field` has not been explicitly set.
+ elif slug is not None and self.lookup_field == 'pk':
+ warnings.warn(
+ "Usage of `slug_field` and/or `slug_url_kwarg` is pending "
+ "deprecation. Set `lookup_field` on the '%s' view instead." %
+ self.__class__.__name__,
+ PendingDeprecationWarning)
slug_field = self.get_slug_field()
queryset = queryset.filter(**{slug_field: slug})
# If none of those are defined, it's an error.
else:
- raise AttributeError("Generic detail view %s must be called with "
- "either an object pk or a slug."
- % self.__class__.__name__)
+ raise AttributeError("Generic detail view %s uses lookup field "
+ "'%s', which was not passed by the URL conf."
+ % (self.__class__.__name__, self.lookup_field))
try:
# Get the single item from the filtered queryset
View
15 docs/ref/class-based-views/mixins-single-object.txt
@@ -23,18 +23,33 @@ SingleObjectMixin
A ``QuerySet`` that represents the objects. If provided, the value of
``queryset`` supersedes the value provided for :attr:`model`.
+ .. attribute:: lookup_field
+
+ The name of the field on the model that should be used for object
+ lookups. This name should also be used as the URLConf keyword argument.
+ By default, ``lookup_field`` is ``'pk'``.
+
+ Note that the ``lookup_field`` attribute replaces the previous usage
+ of ``slug_field``, ``slug_url_kwarg`` and ``pk_url_kwarg``.
+
.. attribute:: slug_field
+ .. deprecated: 1.6
+
The name of the field on the model that contains the slug. By default,
``slug_field`` is ``'slug'``.
.. attribute:: slug_url_kwarg
+ .. deprecated: 1.6
+
The name of the URLConf keyword argument that contains the slug. By
default, ``slug_url_kwarg`` is ``'slug'``.
.. attribute:: pk_url_kwarg
+ .. deprecated: 1.6
+
The name of the URLConf keyword argument that contains the primary key.
By default, ``pk_url_kwarg`` is ``'pk'``.
View
15 docs/releases/1.6.txt
@@ -334,6 +334,21 @@ deprecated but will not be removed from Django until version 1.8.
.. _recommendations in the Python documentation: http://docs.python.org/2/library/doctest.html#unittest-api
+Simplified DetailView object lookups
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In previous versions, object lookup in the :class:`DetailView` was controlled
+by the ``pk_url_kwarg``, ``slug_field``, and ``slug_url_kwarg`` attributes.
+These have now been replaced with a single ``lookup_field`` attribute.
+
+The ``lookup_field`` attribute should correspond with the model field that
+is used for object lookups, and should also correspond with a named argument
+in the URLconf.
+
+The default value for the ``lookup_field`` attribute is ``'pk'``. If you are
+using ``slug`` lookups for a :class:`DetailView`, you should now explicitly
+specify ``lookup_field='slug'`` on the view class.
+
Addition of ``QuerySet.datetimes()``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
View
4 docs/topics/class-based-views/generic-display.txt
@@ -423,9 +423,9 @@ object -- so we simply override it and wrap the call::
.. note::
The URLconf here uses the named group ``pk`` - this name is the default
- name that ``DetailView`` uses to find the value of the primary key used to
+ name that ``DetailView`` uses to find the value of the field used to
filter the queryset.
- If you want to call the group something else, you can set ``pk_url_kwarg``
+ If you want to call the group something else, you can set ``lookup_field``
on the view. More details can be found in the reference for
:class:`~django.views.generic.detail.DetailView`
View
57 tests/generic_views/test_detail.py
@@ -6,6 +6,7 @@
from .models import Artist, Author, Page
+import warnings
class DetailViewTest(TestCase):
fixtures = ['generic-views-test-data.json']
@@ -25,26 +26,54 @@ def test_detail_by_pk(self):
self.assertEqual(res.context['author'], Author.objects.get(pk=1))
self.assertTemplateUsed(res, 'generic_views/author_detail.html')
- def test_detail_by_custom_pk(self):
- res = self.client.get('/detail/author/bycustompk/1/')
- self.assertEqual(res.status_code, 200)
- self.assertEqual(res.context['object'], Author.objects.get(pk=1))
- self.assertEqual(res.context['author'], Author.objects.get(pk=1))
- self.assertTemplateUsed(res, 'generic_views/author_detail.html')
-
- def test_detail_by_slug(self):
- res = self.client.get('/detail/author/byslug/scott-rosenberg/')
+ def test_detail_by_custom_lookup(self):
+ res = self.client.get('/detail/author/bycustomlookup/scott-rosenberg/')
self.assertEqual(res.status_code, 200)
self.assertEqual(res.context['object'], Author.objects.get(slug='scott-rosenberg'))
self.assertEqual(res.context['author'], Author.objects.get(slug='scott-rosenberg'))
self.assertTemplateUsed(res, 'generic_views/author_detail.html')
+ def test_detail_by_custom_pk(self):
+ with warnings.catch_warnings(record=True) as warned:
+ warnings.simplefilter("always")
+ res = self.client.get('/detail/author/bycustompk/1/')
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(pk=1))
+ self.assertEqual(res.context['author'], Author.objects.get(pk=1))
+ self.assertTemplateUsed(res, 'generic_views/author_detail.html')
+ self.assertEqual(len(warned), 1)
+ self.assertTrue(issubclass(warned[0].category, PendingDeprecationWarning))
+ self.assertEqual(str(warned[0].message),
+ "Usage of `pk_url_kwarg` is pending deprecation. "
+ "Set `lookup_field` on the 'AuthorDetail' view instead.")
+
+ def test_detail_by_slug(self):
+ with warnings.catch_warnings(record=True) as warned:
+ warnings.simplefilter("always")
+ res = self.client.get('/detail/author/byslug/scott-rosenberg/')
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(slug='scott-rosenberg'))
+ self.assertEqual(res.context['author'], Author.objects.get(slug='scott-rosenberg'))
+ self.assertTemplateUsed(res, 'generic_views/author_detail.html')
+ self.assertEqual(len(warned), 1)
+ self.assertTrue(issubclass(warned[0].category, PendingDeprecationWarning))
+ self.assertEqual(str(warned[0].message),
+ "Usage of `slug_field` and/or `slug_url_kwarg` is pending deprecation. "
+ "Set `lookup_field` on the 'AuthorDetail' view instead.")
+
def test_detail_by_custom_slug(self):
- res = self.client.get('/detail/author/bycustomslug/scott-rosenberg/')
- self.assertEqual(res.status_code, 200)
- self.assertEqual(res.context['object'], Author.objects.get(slug='scott-rosenberg'))
- self.assertEqual(res.context['author'], Author.objects.get(slug='scott-rosenberg'))
- self.assertTemplateUsed(res, 'generic_views/author_detail.html')
+ with warnings.catch_warnings(record=True) as warned:
+ warnings.simplefilter("always")
+ res = self.client.get('/detail/author/bycustomslug/scott-rosenberg/')
+ self.assertEqual(res.status_code, 200)
+ self.assertEqual(res.context['object'], Author.objects.get(slug='scott-rosenberg'))
+ self.assertEqual(res.context['author'], Author.objects.get(slug='scott-rosenberg'))
+ self.assertTemplateUsed(res, 'generic_views/author_detail.html')
+ self.assertEqual(len(warned), 1)
+ self.assertTrue(issubclass(warned[0].category, PendingDeprecationWarning))
+ self.assertEqual(str(warned[0].message),
+ "Usage of `slug_field` and/or `slug_url_kwarg` is pending deprecation. "
+ "Set `lookup_field` on the 'AuthorDetail' view instead.")
def test_verbose_name(self):
res = self.client.get('/detail/artist/1/')
View
2  tests/generic_views/urls.py
@@ -35,6 +35,8 @@
url(r'^detail/author/(?P<pk>\d+)/$',
views.AuthorDetail.as_view(),
name="author_detail"),
+ (r'^detail/author/bycustomlookup/(?P<slug>[\w-]+)/$',
+ views.AuthorDetail.as_view(lookup_field='slug')),
(r'^detail/author/bycustompk/(?P<foo>\d+)/$',
views.AuthorDetail.as_view(pk_url_kwarg='foo')),
(r'^detail/author/byslug/(?P<slug>[\w-]+)/$',
Something went wrong with that request. Please try again.