Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#25369 closed Bug (fixed)

Wrong exception being used in official example creates very subtle bugs for the syndication feed framework.

Reported by: Paul Graham Owned by: nobody
Component: Documentation Version: 1.8
Severity: Normal Keywords: syndication feed future subtle documentation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Paul Graham)

The Setup

A. Example code from documentation

from django.contrib.syndication.views import Feed
from django.shortcuts import get_object_or_404

class BeatFeed(Feed):
    description_template = 'feeds/beat_description.html'

    def get_object(self, request, beat_id):
        return get_object_or_404(Beat, pk=beat_id)

B. Code from Feed(object)

def __call__(self, request, *args, **kwargs):
        try:
            obj = self.get_object(request, *args, **kwargs)
        except ObjectDoesNotExist:
            raise Http404('Feed object does not exist.')

In the A complex example ( https://docs.djangoproject.com/en/1.8/ref/contrib/syndication/#a-complex-example ) example code the method get_object() uses get_object_or_404() to raise an Http404 exception (See A. above). However the the Feed class reference on the same page indicates that ObjectDoesNotExist exception should be raised on error. Looking at the code of the __call__() method of the Feed class shows that it calls the get_object() method and it does indeed catch ObjectDoesNotExist exceptions (See B. above). However, the reason the example code runs without any obvious errors and the root of the problem at hand is that upon catching an ObjectDoesNotExist exception the Feed class then raises an Http404 error. This means that the Http404 exception being raised in the example code for get_object() is not being caught where it should be, inside Feed.__call__() but ultimately falls through that try: except: block and is handled farther up the chain by the code that was meant to catch errors from Feed.__call__()

The Problem

It should be noted that people have been depending upon the example in question as reference and that changing how the Feed class handles exceptions or changing code that handles exceptions from
get_object() could be hazardous as it would could break previously conformant code created by users. In fact, I discovered this bug while reading a third party Django RSS tutorial that was based on the official Django documentation.

There are at least two problematic scenarios that spring to mind:

The first problematic scenario is that the way the Feed class handles ObjectDoesNotExist exceptions changes and the new error handling code is skipped by an Http404 error. Ex.

In class Feed:

def __call__(self, request, *args, **kwargs):
        try:
            obj = self.get_object(request, *args, **kwargs)
        except ObjectDoesNotExist:
            # Any new error handling code placed here will 
            # be skipped by the Http404 exception client code
            # could be using. <-- Problem 
            raise Http404('Feed object does not exist.')

The second problematic scenario if users have been calling the Feed.__call__() method for any reason (subclassing Feed and overidding call method perhaps) with the idea, backed by an example from the documentation, that they should only be catching Http404 on error Ex.

try:
        x = my_feed_instance = get_object(request, *args, **kwargs)
except Http404:
        # Code here won't be run if an ObjectDoesNotExist exception is raised.

Change History (7)

comment:1 by Paul Graham, 9 years ago

Description: modified (diff)

comment:2 by Paul Graham, 9 years ago

Description: modified (diff)

comment:3 by Paul Graham, 9 years ago

Description: modified (diff)

comment:4 by Paul Graham, 9 years ago

Description: modified (diff)

comment:5 by Tim Graham, 9 years ago

Component: contrib.syndicationDocumentation
Has patch: set
Triage Stage: UnreviewedAccepted

Does this patch fix the issue?

  • docs/ref/contrib/syndication.txt

    diff --git a/docs/ref/contrib/syndication.txt b/docs/ref/contrib/syndication.txt
    index 8da4073..4d0e3da 100644
    a b method along with the request object.  
    225225Here's the code for these beat-specific feeds::
    226226
    227227    from django.contrib.syndication.views import Feed
    228     from django.shortcuts import get_object_or_404
    229228
    230229    class BeatFeed(Feed):
    231230        description_template = 'feeds/beat_description.html'
    232231
    233232        def get_object(self, request, beat_id):
    234             return get_object_or_404(Beat, pk=beat_id)
     233            return Beat.objects.get(pk=beat_id)
    235234
    236235        def title(self, obj):
    237236            return "Police beat central: Crimes for beat %s" % obj.beat

comment:6 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 8388f24:

[1.8.x] Fixed #25369 -- Corrected syndication's get_object() example.

Backport of 64d7a553e1be20174b0aa2882111049abf392d4f from master

comment:7 by Tim Graham <timograham@…>, 9 years ago

In 64d7a55:

Fixed #25369 -- Corrected syndication's get_object() example.

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