Django

Code

Ticket #4720 (closed: fixed)

Opened 11 months ago

Last modified 2 months ago

feed class get_object has no opportunity to object to not having a url

Reported by: Bill Fenner <fenner@gmail.com> Assigned to: cablehead
Component: RSS framework Version: SVN
Keywords: Cc: cablehead
Triage Stage: Ready for checkin Has patch: 1
Needs documentation: 0 Needs tests: 0
Patch needs improvement: 0

Description

I have a feed that looks almost exactly like the "complex example" given in the manual for chicagocrime.org:

class DocumentComments(Feed):
    feed_type = Atom1Feed
    def get_object(self, bits):
        if len(bits) != 1:
            raise InternetDraft.DoesNotExist
        return InternetDraft.objects.get(filename=bits[0])

I assumed that since that method existed, it would be called for all requests, and a request for the "base" url of the feed would result in the DoesNotExist? exception. Instead, I get "AttributeError?: 'NoneType?' object has no attribute 'filename'", since in link() I use obj.filename and obj is getting set to None.

I don't like having a known url for a 500 error, since I expect users to try to hack the urls and see what they can see.

I've attached a proposed diff. It always calls get_object, and creates a default get_object method that returns None. This should keep the existing behavior (and, in fact, works fine with my other feeds that don't take objects), and allows the user's class to handle a bits=[] if they would like to.

With the existing setup, the only way I can think of to not provide a 500 error when visiting this feed without an additional URL portion is to test for obj == None in the link method, and I can only do that because I know that the link one is the first one called - way too much internal knowledge of the feed class.

Attachments

syndication-500.diff (1.0 kB) - added by Bill Fenner <fenner@gmail.com> on 06/28/07 14:43:45.
syndication-500-2.diff (3.4 kB) - added by Andy Gayton <andy-django@thecablelounge.com> on 12/02/07 23:15:22.
syndication-500-2-alt.diff (4.2 kB) - added by Andy Gayton <andy-django@thecablelounge.com> on 12/03/07 00:46:04.
When in rome …

Change History

06/28/07 14:43:45 changed by Bill Fenner <fenner@gmail.com>

  • attachment syndication-500.diff added.

09/30/07 07:19:03 changed by cablehead

  • cc set to cablehead.
  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

I was banging my head reading the docs, trying to work out how this could have worked as is, and finally checked chicagocrime.org. They are throwing a 500 error there: http://www.chicagocrime.org/rss/beats/

09/30/07 20:42:20 changed by ubernostrum

  • stage changed from Unreviewed to Accepted.

Yeah, this is an annoyance in the feeds system and should be fixed.

12/01/07 23:40:21 changed by Andy Gayton <andy-django@thecablelounge.com>

The attached patch has been working well at http://rst2a.com for the last 2 or so months.

12/02/07 16:19:17 changed by Andy Gayton <andy-django@thecablelounge.com>

  • owner changed from nobody to cablehead.
  • needs_docs set to 1.
  • needs_tests set to 1.

Grabbing to add tests, and to update doco for this patch.

12/02/07 23:15:22 changed by Andy Gayton <andy-django@thecablelounge.com>

  • attachment syndication-500-2.diff added.

12/02/07 23:20:20 changed by Andy Gayton <andy-django@thecablelounge.com>

  • needs_docs deleted.
  • needs_tests deleted.
  • stage changed from Accepted to Ready for checkin.
  • fixed up usage of tabs in the original patch
  • added stripping out empty strings from bits to the patch - it seems like
        if len(bits) != 1:
            raise InternetDraft.DoesNotExist

should handle the case of an empty url, instead of having:

        return InternetDraft.objects.get(filename=bits[0])

hit the db with an empty string.

  • added a failing (now passing) unit test
  • updated docs

12/03/07 00:46:04 changed by Andy Gayton <andy-django@thecablelounge.com>

  • attachment syndication-500-2-alt.diff added.

When in rome ...

12/03/07 00:51:19 changed by Andy Gayton <andy-django@thecablelounge.com>

I feel like I'm going against the grain by strictly testing at the unit level.

syndication-500-2-alt.diff is an alternate patch, which tests things by invoking the full request stack. It's less unit-y, but might be more inline with the django way of doing things.

03/20/08 01:35:53 changed by mtredinnick

  • status changed from new to closed.
  • resolution set to fixed.

(In [7328]) Merged the tests and documentation from #4720 to support the changes in [7295]. Thanks, Andy Gayton. Fixed #4720. Refs #5855.


Add/Change #4720 (feed class get_object has no opportunity to object to not having a url)




Change Properties
Action