From dbb5ab771be406d90ac522e0ecd644e5ddb21e6f Mon Sep 17 00:00:00 2001 From: Stephen Burrows Date: Wed, 2 Feb 2011 11:48:40 -0500 Subject: [PATCH] Refactored Node.get_absolute_url and related functions (such as MultiView.reverse) to use a new Node.construct_url function, which handles constructing any type of url involving a node or raises an appropriate error if this is not possible. Moved MultiView.reverse to View and merged View.get_subpath into it. Added APPEND_SLASH support to node_view, including support for resolving non-philo targets (resolves issue 75). Made urlpatterns for penfield and waldo MultiViews unambiguous. Altered LazyNode to always return a subpath of at least "/". Added handles_subpath methods to View and MultiView. --- contrib/penfield/models.py | 42 +++++++-------- contrib/penfield/utils.py | 6 +-- contrib/shipherd/models.py | 8 +-- contrib/waldo/models.py | 21 ++++---- exceptions.py | 4 +- middleware.py | 4 ++ models/nodes.py | 102 +++++++++++++++++++++++++++---------- templatetags/nodes.py | 5 +- views.py | 31 +++++++++-- 9 files changed, 147 insertions(+), 76 deletions(-) diff --git a/contrib/penfield/models.py b/contrib/penfield/models.py index 7ca879d..27e5b5d 100644 --- a/contrib/penfield/models.py +++ b/contrib/penfield/models.py @@ -111,49 +111,49 @@ class BlogView(MultiView, FeedMultiViewMixin): ) if self.feeds_enabled: urlpatterns += patterns('', - url(r'^%s/(?P[-\w]+[-+/\w]*)/%s/' % (self.tag_permalink_base, self.feed_suffix), self.feed_view(self.get_entries_by_tag, 'entries_by_tag_feed'), name='entries_by_tag_feed'), + url(r'^%s/(?P[-\w]+[-+/\w]*)/%s$' % (self.tag_permalink_base, self.feed_suffix), self.feed_view(self.get_entries_by_tag, 'entries_by_tag_feed'), name='entries_by_tag_feed'), ) urlpatterns += patterns('', - url(r'^%s/(?P[-\w]+[-+/\w]*)/' % self.tag_permalink_base, self.page_view(self.get_entries_by_tag, self.tag_page), name='entries_by_tag') + url(r'^%s/(?P[-\w]+[-+/\w]*)$' % self.tag_permalink_base, self.page_view(self.get_entries_by_tag, self.tag_page), name='entries_by_tag') ) if self.tag_archive_page: urlpatterns += patterns('', - url((r'^(?:%s)/?$' % self.tag_permalink_base), self.tag_archive_view, 'tag_archive') + url((r'^%s$' % self.tag_permalink_base), self.tag_archive_view, name='tag_archive') ) if self.entry_archive_page: if self.entry_permalink_style in 'DMY': urlpatterns += patterns('', - url(r'^(?P\d{4})/', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_year'))) + url(r'^(?P\d{4})', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_year'))) ) if self.entry_permalink_style in 'DM': urlpatterns += patterns('', - url(r'^(?P\d{4})/(?P\d{2})/?$', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_month'))), + url(r'^(?P\d{4})/(?P\d{2})$', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_month'))), ) if self.entry_permalink_style == 'D': urlpatterns += patterns('', - url(r'^(?P\d{4})/(?P\d{2})/(?P\d{2})/?$', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_day'))) + url(r'^(?P\d{4})/(?P\d{2})/(?P\d{2})$', include(self.feed_patterns(self.get_entries_by_ymd, self.entry_archive_page, 'entries_by_day'))) ) if self.entry_permalink_style == 'D': urlpatterns += patterns('', - url(r'^(?P\d{4})/(?P\d{2})/(?P\d{2})/(?P[-\w]+)/?$', self.entry_view) + url(r'^(?P\d{4})/(?P\d{2})/(?P\d{2})/(?P[-\w]+)$', self.entry_view) ) elif self.entry_permalink_style == 'M': urlpatterns += patterns('', - url(r'^(?P\d{4})/(?P\d{2})/(?P[-\w]+)/?$', self.entry_view) + url(r'^(?P\d{4})/(?P\d{2})/(?P[-\w]+)$', self.entry_view) ) elif self.entry_permalink_style == 'Y': urlpatterns += patterns('', - url(r'^(?P\d{4})/(?P[-\w]+)/?$', self.entry_view) + url(r'^(?P\d{4})/(?P[-\w]+)$', self.entry_view) ) elif self.entry_permalink_style == 'B': urlpatterns += patterns('', - url((r'^(?:%s)/(?P[-\w]+)/?$' % self.entry_permalink_base), self.entry_view) + url((r'^%s/(?P[-\w]+)$' % self.entry_permalink_base), self.entry_view) ) else: urlpatterns = patterns('', - url(r'^(?P[-\w]+)/?$', self.entry_view) + url(r'^(?P[-\w]+)$', self.entry_view) ) return urlpatterns @@ -355,44 +355,44 @@ class NewsletterView(MultiView, FeedMultiViewMixin): def urlpatterns(self): urlpatterns = patterns('', url(r'^', include(self.feed_patterns(self.get_all_articles, self.index_page, 'index'))), - url(r'^(?:%s)/(?P.+)/' % self.issue_permalink_base, include(self.feed_patterns(self.get_articles_by_issue, self.issue_page, 'issue'))) + url(r'^%s/(?P.+)' % self.issue_permalink_base, include(self.feed_patterns(self.get_articles_by_issue, self.issue_page, 'issue'))) ) if self.issue_archive_page: urlpatterns += patterns('', - url(r'^(?:%s)/$' % self.issue_permalink_base, self.issue_archive_view, 'issue_archive') + url(r'^%s$' % self.issue_permalink_base, self.issue_archive_view, 'issue_archive') ) if self.article_archive_page: urlpatterns += patterns('', - url(r'^(?:%s)/' % self.article_permalink_base, include(self.feed_patterns(self.get_all_articles, self.article_archive_page, 'articles'))) + url(r'^%s' % self.article_permalink_base, include(self.feed_patterns(self.get_all_articles, self.article_archive_page, 'articles'))) ) if self.article_permalink_style in 'DMY': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_year'))) + url(r'^%s/(?P\d{4})' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_year'))) ) if self.article_permalink_style in 'DM': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/(?P\d{2})/' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_month'))) + url(r'^%s/(?P\d{4})/(?P\d{2})' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_month'))) ) if self.article_permalink_style == 'D': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/(?P\d{2})/(?P\d{2})/' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_day'))) + url(r'^%s/(?P\d{4})/(?P\d{2})/(?P\d{2})' % self.article_permalink_base, include(self.feed_patterns(self.get_articles_by_ymd, self.article_archive_page, 'articles_by_day'))) ) if self.article_permalink_style == 'Y': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/(?P[\w-]+)/$' % self.article_permalink_base, self.article_view) + url(r'^%s/(?P\d{4})/(?P[\w-]+)$' % self.article_permalink_base, self.article_view) ) elif self.article_permalink_style == 'M': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/(?P\d{2})/(?P[\w-]+)/$' % self.article_permalink_base, self.article_view) + url(r'^%s/(?P\d{4})/(?P\d{2})/(?P[\w-]+)$' % self.article_permalink_base, self.article_view) ) elif self.article_permalink_style == 'D': urlpatterns += patterns('', - url(r'^(?:%s)/(?P\d{4})/(?P\d{2})/(?P\d{2})/(?P[\w-]+)/$' % self.article_permalink_base, self.article_view) + url(r'^%s/(?P\d{4})/(?P\d{2})/(?P\d{2})/(?P[\w-]+)$' % self.article_permalink_base, self.article_view) ) else: urlpatterns += patterns('', - url(r'^(?:%s)/(?P[-\w]+)/?$' % self.article_permalink_base, self.article_view) + url(r'^%s/(?P[-\w]+)$' % self.article_permalink_base, self.article_view) ) return urlpatterns diff --git a/contrib/penfield/utils.py b/contrib/penfield/utils.py index 43c7c91..2c550db 100644 --- a/contrib/penfield/utils.py +++ b/contrib/penfield/utils.py @@ -56,13 +56,13 @@ class FeedMultiViewMixin(object): current_site = Site.objects.get_current() #Could this be done with request.path instead somehow? feed_kwargs = { - 'link': 'http://%s/%s/%s/' % (current_site.domain, request.node.get_absolute_url().strip('/'), reverse(reverse_name, urlconf=self, kwargs=kwargs).strip('/')) + 'link': request.node.construct_url(subpath=self.reverse(reverse_name, kwargs=kwargs), request=request, with_domain=True) } feed = self.get_feed(feed_type, extra_context, feed_kwargs) for obj in objects: kwargs = { - 'link': 'http://%s/%s/%s/' % (current_site.domain, request.node.get_absolute_url().strip('/'), self.get_subpath(obj).strip('/')) + 'link': request.node.construct_url(subpath=self.reverse(obj=obj), request=request, with_domain=True) } self.add_item(feed, obj, kwargs=kwargs) @@ -93,7 +93,7 @@ class FeedMultiViewMixin(object): if self.feeds_enabled: feed_name = '%s_feed' % base_name urlpatterns = patterns('', - url(r'^%s/$' % self.feed_suffix, self.feed_view(object_fetcher, feed_name), name=feed_name), + url(r'^%s$' % self.feed_suffix, self.feed_view(object_fetcher, feed_name), name=feed_name), ) + urlpatterns return urlpatterns diff --git a/contrib/shipherd/models.py b/contrib/shipherd/models.py index dee16e9..010aa9f 100644 --- a/contrib/shipherd/models.py +++ b/contrib/shipherd/models.py @@ -248,12 +248,12 @@ class NavigationItem(TreeEntity): params = self.reversing_parameters args = isinstance(params, list) and params or None kwargs = isinstance(params, dict) and params or None - return node.view.reverse(view_name, args=args, kwargs=kwargs, node=node) + subpath = node.view.reverse(view_name, args=args, kwargs=kwargs) else: subpath = self.url_or_subpath - while subpath and subpath[0] == '/': - subpath = subpath[1:] - return '%s%s' % (node.get_absolute_url(), subpath) + if subpath[0] != '/': + subpath = '/' + subpath + return node.construct_url(subpath) elif node is not None: return node.get_absolute_url() else: diff --git a/contrib/waldo/models.py b/contrib/waldo/models.py index 2f40da7..3286aa0 100644 --- a/contrib/waldo/models.py +++ b/contrib/waldo/models.py @@ -41,32 +41,31 @@ class LoginMultiView(MultiView): @property def urlpatterns(self): urlpatterns = patterns('', - url(r'^login/$', self.login, name='login'), - url(r'^logout/$', self.logout, name='logout'), + url(r'^login$', self.login, name='login'), + url(r'^logout$', self.logout, name='logout'), - url(r'^password/reset/$', csrf_protect(self.password_reset), name='password_reset'), - url(r'^password/reset/(?P\w+)/(?P[^/]+)/$', self.password_reset_confirm, name='password_reset_confirm'), + url(r'^password/reset$', csrf_protect(self.password_reset), name='password_reset'), + url(r'^password/reset/(?P\w+)/(?P[^/]+)$', self.password_reset_confirm, name='password_reset_confirm'), - url(r'^register/$', csrf_protect(self.register), name='register'), - url(r'^register/(?P\w+)/(?P[^/]+)/$', self.register_confirm, name='register_confirm') + url(r'^register$', csrf_protect(self.register), name='register'), + url(r'^register/(?P\w+)/(?P[^/]+)$', self.register_confirm, name='register_confirm') ) if self.password_change_page: urlpatterns += patterns('', - url(r'^password/change/$', csrf_protect(self.login_required(self.password_change)), name='password_change'), + url(r'^password/change$', csrf_protect(self.login_required(self.password_change)), name='password_change'), ) return urlpatterns def make_confirmation_link(self, confirmation_view, token_generator, user, node, token_args=None, reverse_kwargs=None): - current_site = Site.objects.get_current() token = token_generator.make_token(user, *(token_args or [])) kwargs = { 'uidb36': int_to_base36(user.id), 'token': token } kwargs.update(reverse_kwargs or {}) - return 'http://%s%s' % (current_site.domain, self.reverse(confirmation_view, kwargs=kwargs, node=node)) + return node.construct_url(subpath=self.reverse(confirmation_view, kwargs=kwargs), with_domain=True) def display_login_page(self, request, message, extra_context=None): request.session.set_test_cookie() @@ -336,8 +335,8 @@ class AccountMultiView(LoginMultiView): def urlpatterns(self): urlpatterns = super(AccountMultiView, self).urlpatterns urlpatterns += patterns('', - url(r'^account/$', self.login_required(self.account_view), name='account'), - url(r'^account/email/(?P\w+)/(?P[\w.]+[+][\w.]+)/(?P[^/]+)/$', self.email_change_confirm, name='email_change_confirm') + url(r'^account$', self.login_required(self.account_view), name='account'), + url(r'^account/email/(?P\w+)/(?P[\w.]+[+][\w.]+)/(?P[^/]+)$', self.email_change_confirm, name='email_change_confirm') ) return urlpatterns diff --git a/exceptions.py b/exceptions.py index 1e4b9d9..f53083d 100644 --- a/exceptions.py +++ b/exceptions.py @@ -5,12 +5,12 @@ MIDDLEWARE_NOT_CONFIGURED = ImproperlyConfigured("""Philo requires the RequestNo class ViewDoesNotProvideSubpaths(Exception): - """ Raised by get_subpath when the View does not provide subpaths (the default). """ + """ Raised by View.reverse when the View does not provide subpaths (the default). """ silent_variable_failure = True class ViewCanNotProvideSubpath(Exception): - """ Raised by get_subpath when the View can not provide a subpath for the supplied object. """ + """ Raised by View.reverse when the View can not provide a subpath for the supplied arguments. """ silent_variable_failure = True diff --git a/middleware.py b/middleware.py index ad660ec..832bd16 100644 --- a/middleware.py +++ b/middleware.py @@ -20,6 +20,10 @@ class LazyNode(object): except Node.DoesNotExist: node = None + if subpath is None: + subpath = "" + subpath = "/" + subpath + if node: node.subpath = subpath diff --git a/models/nodes.py b/models/nodes.py index 526400c..ab8adf7 100644 --- a/models/nodes.py +++ b/models/nodes.py @@ -1,9 +1,8 @@ from django.db import models from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes import generic -from django.contrib.sites.models import Site -from django.http import HttpResponse, HttpResponseServerError, HttpResponseRedirect -from django.core.exceptions import ViewDoesNotExist +from django.contrib.sites.models import Site, RequestSite +from django.http import HttpResponse, HttpResponseServerError, HttpResponseRedirect, Http404 from django.core.servers.basehttp import FileWrapper from django.core.urlresolvers import resolve, clear_url_caches, reverse, NoReverseMatch from django.template import add_to_builtins as register_templatetags @@ -30,23 +29,53 @@ class Node(TreeEntity): return self.view.accepts_subpath return False + def handles_subpath(self, subpath): + return self.view.handles_subpath(subpath) + def render_to_response(self, request, extra_context=None): return self.view.render_to_response(request, extra_context) - def get_absolute_url(self): + def get_absolute_url(self, request=None, with_domain=False, secure=False): + return self.construct_url(request=request, with_domain=with_domain, secure=secure) + + def construct_url(self, subpath=None, request=None, with_domain=False, secure=False): + """ + This method will construct a URL based on the Node's location. + If a request is passed in, that will be used as a backup in case + the Site lookup fails. The Site lookup takes precedence because + it's what's used to find the root node. This will raise: + - NoReverseMatch if philo-root is not reverseable + - Site.DoesNotExist if a domain is requested but not buildable. + - AncestorDoesNotExist if the root node of the site isn't an + ancestor of this instance. + """ + # Try reversing philo-root first, since we can't do anything if that fails. + root_url = reverse('philo-root') + try: - root = Site.objects.get_current().root_node + current_site = Site.objects.get_current() except Site.DoesNotExist: - root = None + if request is not None: + current_site = RequestSite(request) + elif with_domain: + # If they want a domain and we can't figure one out, + # best to reraise the error to let them know. + raise + else: + current_site = None - try: - path = self.get_path(root=root) - if path: - path += '/' - root_url = reverse('philo-root') - return '%s%s' % (root_url, path) - except AncestorDoesNotExist, ViewDoesNotExist: - return None + root = getattr(current_site, 'root_node', None) + path = self.get_path(root=root) + + if current_site and with_domain: + domain = "http%s://%s" % (secure and "s" or "", current_site.domain) + else: + domain = "" + + if not path: + subpath = subpath[1:] + + return '%s%s%s%s' % (domain, root_url, path, subpath or "") class Meta: app_label = 'philo' @@ -61,15 +90,34 @@ class View(Entity): accepts_subpath = False - def get_subpath(self, obj): + def handles_subpath(self, subpath): + if not self.accepts_subpath and subpath != "/": + return False + return True + + def reverse(self, view_name=None, args=None, kwargs=None, node=None, obj=None): + """Shortcut method to handle the common pattern of getting the + absolute url for a view's subpaths.""" if not self.accepts_subpath: raise ViewDoesNotProvideSubpaths - view_name, args, kwargs = self.get_reverse_params(obj) + if obj is not None: + # Perhaps just override instead of combining? + obj_view_name, obj_args, obj_kwargs = self.get_reverse_params(obj) + if view_name is None: + view_name = obj_view_name + args = list(obj_args) + list(args or []) + obj_kwargs.update(kwargs or {}) + kwargs = obj_kwargs + try: - return reverse(view_name, args=args, kwargs=kwargs, urlconf=self) + subpath = reverse(view_name, urlconf=self, args=args or [], kwargs=kwargs or {}) except NoReverseMatch: raise ViewCanNotProvideSubpath + + if node is not None: + return node.construct_url(subpath) + return subpath def get_reverse_params(self, obj): """This method should return a view_name, args, kwargs tuple suitable for reversing a url for the given obj using self as the urlconf.""" @@ -105,12 +153,18 @@ class MultiView(View): def urlpatterns(self): raise NotImplementedError("MultiView subclasses must implement urlpatterns.") + def handles_subpath(self, subpath): + if not super(MultiView, self).handles_subpath(subpath): + return False + try: + resolve(subpath, urlconf=self) + except Http404: + return False + return True + def actually_render_to_response(self, request, extra_context=None): clear_url_caches() subpath = request.node.subpath - if not subpath: - subpath = "" - subpath = "/" + subpath view, args, kwargs = resolve(subpath, urlconf=self) view_args = getargspec(view) if extra_context is not None and ('extra_context' in view_args[0] or view_args[2] is not None): @@ -119,14 +173,6 @@ class MultiView(View): kwargs['extra_context'] = extra_context return view(request, *args, **kwargs) - def reverse(self, view_name, args=None, kwargs=None, node=None): - """Shortcut method to handle the common pattern of getting the absolute url for a multiview's - subpaths.""" - subpath = reverse(view_name, urlconf=self, args=args or [], kwargs=kwargs or {}) - if node is not None: - return '/%s/%s/' % (node.get_absolute_url().strip('/'), subpath.strip('/')) - return subpath - def get_context(self): """Hook for providing instance-specific context - such as the value of a Field - to all views.""" return {} diff --git a/templatetags/nodes.py b/templatetags/nodes.py index 73492d4..5ae507d 100644 --- a/templatetags/nodes.py +++ b/templatetags/nodes.py @@ -55,10 +55,7 @@ class NodeURLNode(template.Node): raise return settings.TEMPLATE_STRING_IF_INVALID else: - if subpath[0] == '/': - subpath = subpath[1:] - - url = node.get_absolute_url() + subpath + url = node.construct_url(subpath) if self.as_var: context[self.as_var] = url diff --git a/views.py b/views.py index 255e54e..cec9c57 100644 --- a/views.py +++ b/views.py @@ -1,5 +1,6 @@ from django.conf import settings -from django.http import Http404 +from django.core.urlresolvers import resolve +from django.http import Http404, HttpResponseRedirect from django.views.decorators.vary import vary_on_headers from philo.exceptions import MIDDLEWARE_NOT_CONFIGURED @@ -15,6 +16,30 @@ def node_view(request, path=None, **kwargs): node = request.node subpath = request.node.subpath - if subpath and not node.accepts_subpath: - raise Http404 + if not node.handles_subpath(subpath): + # If the subpath isn't handled, check settings.APPEND_SLASH. If + # it's True, try to correct the subpath. + if not settings.APPEND_SLASH: + raise Http404 + + if subpath[-1] == "/": + subpath = subpath[:-1] + else: + subpath += "/" + + redirect_url = node.construct_url(subpath) + + if node.handles_subpath(subpath): + return HttpResponseRedirect(redirect_url) + + # Perhaps there is a non-philo view at this address. Can we + # resolve *something* there besides node_view? If not, + # raise a 404. + view, args, kwargs = resolve(redirect_url) + + if view == node_view: + raise Http404 + else: + return HttpResponseRedirect(redirect_url) + return node.render_to_response(request, kwargs) \ No newline at end of file -- 2.20.1