Opened 18 years ago

Closed 13 years ago

#2713 closed Bug (fixed)

Sitemap Index with Cache gives a NoReverseMatch exception

Reported by: JayKlehr Owned by: Aymeric Augustin
Component: contrib.sitemaps Version: dev
Severity: Normal Keywords:
Cc: askfor@…, miracle2k, Michael Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I posted this in the Google Groups ( http://groups.google.com/group/django-users/browse_thread/thread/fc88c493b16623f/28bfe98a7b4f98c5 ) list but didn't get a response so I thought I'd create a ticket for it since it does seem like a bug to me.

Following the djangoproject.com Sitemap example ( source:djangoproject.com/django_website/urls.py ) and the documentation on Sitemaps ( http://www.djangoproject.com/documentation/sitemaps/ ) I'm trying to implement a caching Sitemap Index that will link out to other Sitemaps for my site (one for each main model basically). I had the Sitemap Index working with the separate sitemaps just fine until I decided I wanted to add caching in my urls.py (I am using the djangoproject.com urls.py as an example.)

Here's some snippets...

from django.conf.urls.defaults import *
from django.contrib.sitemaps import views as sitemap_views
from django.views.decorators.cache import cache_page

urlpatterns = patterns('',
    (r'^admin/', include('django.contrib.admin.urls')),
    (r'^sitemap.xml$', cache_page(sitemap_views.index, 60 * 60 * 24), {'sitemaps': my_sitemaps}),
    (r'^sitemap-(?P<section>.+).xml$', cache_page(sitemap_views.sitemap, 60 * 60 * 24), {'sitemaps': my_sitemaps}),
)

my_sitemaps is a dictionary of my sitemaps (built following the example in the Sitemap documentation). With this code I can access the individual sitemaps without trouble, but can't access the main sitemap.xml file (that's when I get the "NoReverseMatch at /sitemap.xml"
exception). However, the following code worked for all files:

from django.conf.urls.defaults import *

urlpatterns = patterns('',
    (r'^admin/', include('django.contrib.admin.urls')),
    (r'^sitemap.xml$', 'django.contrib.sitemaps.views.index', {'sitemaps': my_sitemaps}),
    (r'^sitemap-(?P<section>.+).xml$', 'django.contrib.sitemaps.views.sitemap, {'sitemaps': my_sitemaps}),
)

But I can't implement the cache using this method because you can't pass the string 'django.contrib.sitemaps.views.index' as the first parameter of cache_page since it's expecting a callable.

Attachments (3)

sitemaps.patch (811 bytes ) - added by Stavros Korokithakis 15 years ago.
2713.diff (1.9 KB ) - added by Chris Beaven 15 years ago.
2713_2.diff (3.3 KB ) - added by Michael 13 years ago.
Updated patch + unittest

Download all attachments as: .zip

Change History (29)

comment:1 by Adrian Holovaty, 18 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #2564.

comment:2 by Chris Beaven, 18 years ago

Resolution: duplicate
Status: closedreopened

Actually, that other ticket was user error, this ticket is the correct one.

comment:3 by Chris Beaven, 18 years ago

Triage Stage: UnreviewedDesign decision needed

So the problem here, is that the function is being decorated in the urlconf instead of the function being overridden in the view with the decorated version of itself.

The function != the decorated function, so of course it will give a NoReverseMatch. Unless we want to provide a way for decorators to pass through their decorated function to the reverse engine, I say this isn't a bug. But I'll let another developer look at this and decide that for sure.

comment:4 by Chris Beaven, 18 years ago

Actually, thinking some more about this, this has been fixed by named url patterns (see [4901])

comment:5 by Chris Beaven, 18 years ago

Resolution: fixed
Status: reopenedclosed

comment:6 by stephanejais, 16 years ago

I can see how this can be considered fixed with named urls, although
I don't manage to have it work.

Here's my conf:

urlpatterns = patterns('',
    ...
    url(r'^sitemap.xml$', cache_page(sitemap_views.index, 86400), kwargs={'sitemaps': sitemaps}, name="django.contrib.sitemaps.views.index"),
    url(r'^sitemap-(?P<section>.+)\.xml$', cache_page(sitemap_views.sitemap, 86400), kwargs={'sitemaps': sitemaps}, name="django.contrib.sitemaps.views.sitemap"),
    
)  

Still getting the NoReverseMatch error.
The problem seems to be this line in the sitemap.views file:

sitemap_url = urlresolvers.reverse('django.contrib.sitemaps.views.sitemap', kwargs={'section': section})

until 'django.contrib.sitemaps.views.sitemap' is hardcoded there,
and since it resolves to a valid view function,
naming my sitemap pattern after the view name doesn't work.

If I change the name of my url pattern to "foobar", and the reverse call in the views file of the sitemap contrib like this:

sitemap_url = urlresolvers.reverse('foobar', kwargs={'section': section})

I get it to work.
Not very satisfying though.
Any suggestions ?

comment:7 by Stavros Korokithakis, 15 years ago

I can confirm that this doesn't work. How about making a named view mandatory for the sitemap, such as "contrib.sitemap.index" and "contrib.sitemap.sitemap"?

comment:8 by Stavros Korokithakis, 15 years ago

Resolution: fixed
Status: closedreopened

by Stavros Korokithakis, 15 years ago

Attachment: sitemaps.patch added

comment:9 by Stavros Korokithakis, 15 years ago

Has patch: set

I have submitted a patch that checks for the normal view and falls back on a "contrib.sitemaps.sitemap" named view if there's an error. This fixes the problem and allows caching.

by Chris Beaven, 15 years ago

Attachment: 2713.diff added

comment:10 by Chris Beaven, 15 years ago

Needs tests: set
Owner: changed from Adrian Holovaty to Stavros Korokithakis
Status: reopenednew
Triage Stage: Design decision neededAccepted

Yes, named urls didn't directly fix this ticket.

Stavros: hard coding an alternate url name isn't the best way to do it - here's a better patch (with docs). [side note: your patch also seemed to be back-the-front]

Even though contrib.sitemaps currently has no tests, some regression tests should probably be added showing this works. Want to have a go at these?

comment:11 by Stavros Korokithakis, 15 years ago

That is indeed much better, for some reason I didn't think of it, even though I didn't like my solution myself.

My patch might have been off because I made it by hand (I've actually never tried to make one before).

Sure, I'll try to write some when I have some time (it should be soon). Thanks for the patch, I hope it gets implemented soon!

comment:12 by Stavros Korokithakis, 15 years ago

Any idea where the regression tests should go? There are no preexisting tests.

Does this code even need tests? It's pretty straightforward (I'm pretty sure this will come back to bite me in the ass).

in reply to:  12 comment:13 by Russell Keith-Magee, 15 years ago

Replying to stavros:

Any idea where the regression tests should go? There are no preexisting tests.

Then now is the time to write some.

Does this code even need tests? It's pretty straightforward (I'm pretty sure this will come back to bite me in the ass).

Yes, tests are required. Tests are not optional. You need to provide a very compelling reason that tests are either impractical or impossible before you get a pass on writing tests.

in reply to:  12 ; comment:14 by Chris Beaven, 15 years ago

Replying to stavros:

Any idea where the regression tests should go?

Answering the initial question (Russ must have missed this after the second question threw him into a rage :)), put them in the contrib app itself. I'd suggest

/django/contrib/sitemaps/tests/__init__.py   <- put the test here
/django/contrib/sitemaps/tests/urls.py       <- your unit test will need a custom urlconf

At the bare minimum, just write some tests to prove the new behaviour works. But feel free to add additional tests of base sitemap functionality for extra brownie points.

in reply to:  14 comment:15 by Russell Keith-Magee, 15 years ago

Replying to SmileyChris:

Replying to stavros:

Any idea where the regression tests should go?

Answering the initial question (Russ must have missed this after the second question threw him into a rage :)),

Hulk Angry! Crush patch with no tests! :-)

Apologies - you're right. I should have given some direction on test location.

Chris' feedback is pretty much on the money, with one modification. Rather than putting the tests directly into in init.py, init.py should be used as a staging module to load the tests from other locations. See contrib.auth for an example of the way to organize the tests.

comment:16 by Stavros Korokithakis, 15 years ago

I tried and don't seem to be able to write the tests, as I don't know how to pass a request object or make reverse() work... Maybe someone can provide guidance on this?

comment:17 by askfor, 15 years ago

Cc: askfor@… added

Got the same problem here.
I was wondering why the cache_page decorator works for www.djangoproject.com according to the code on the trac (http://code.djangoproject.com/browser/djangoproject.com/django_website/urls.py) ?
And there's also another problem. Since sitemap reverse the url by 'django.contrib.sitemaps.views.sitemap' , if there's a app also use sitemap, there will be a conflict.

comment:18 by miracle2k, 15 years ago

Cc: miracle2k added

comment:19 by Stavros Korokithakis, 15 years ago

The patch fixes the issue, but it needs tests. Can anyone experienced write some so we can get this functionality?

comment:20 by Gabriel Hurley, 14 years ago

Component: Contrib appscontrib.sitemaps

comment:21 by Łukasz Rekucki, 14 years ago

Severity: normalNormal
Type: defectBug

comment:22 by Jannis Leidel, 13 years ago

Easy pickings: unset
UI/UX: unset

#13908 was a duplicate.

comment:23 by Michael, 13 years ago

Cc: Michael added

by Michael, 13 years ago

Attachment: 2713_2.diff added

Updated patch + unittest

comment:24 by Michael, 13 years ago

Needs tests: unset

I've updated the patch to work with trunk and added one unittest

comment:25 by Aymeric Augustin, 13 years ago

Owner: changed from Stavros Korokithakis to Aymeric Augustin

comment:26 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: newclosed

In [17408]:

Fixed #2713 -- Made the name of the sitemap view a parameter of the sitemap index view, in order to allow decorators. Also cleaned up code in the area and removed redundant comments from the tests.

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