From: Stephen Burrows Date: Wed, 12 Jan 2011 18:15:39 +0000 (-0500) Subject: Shifted NavigationManager caching to use node instances rather than node pks. Improve... X-Git-Tag: philo-0.9~22^2~14 X-Git-Url: http://git.ithinksw.org/philo.git/commitdiff_plain/4809d9999c46311cc9fdbe81eefa8c3afd1290b9?ds=sidebyside;hp=c7161f34d92a2df6ed37b2ed061e64a47dbfb003 Shifted NavigationManager caching to use node instances rather than node pks. Improved (i.e. added) a distinction between having a cache for a node and having a Navigation instance somewhere in the cache. Refactored Navigation.is_active for better clarity. Added cache clearing for previously-uncached, directly-hosted navigation. Adjusted recursenavigation to accept either a node or a queryset/iterable. Added targeting_navigation filter. --- diff --git a/contrib/shipherd/models.py b/contrib/shipherd/models.py index a95546d..8be03e2 100644 --- a/contrib/shipherd/models.py +++ b/contrib/shipherd/models.py @@ -41,11 +41,9 @@ class NavigationManager(TreeManager): will be the most recent set of defined hosted navigation among the node's ancestors. Lookups are cached so that subsequent lookups for the same node don't hit the database. - - TODO: Should this create the auto-generated navigation in "physical" form? """ try: - return self._get_from_cache(self.db, node) + return self._get_cache_for(self.db, node) except KeyError: # Find the most recent host! ancestors = node.get_ancestors(ascending=True, include_self=True).annotate(num_navigation=models.Count("hosted_navigation")) @@ -56,57 +54,75 @@ class NavigationManager(TreeManager): nodes_to_cache = [] host_node = None for ancestor in ancestors: - if self.is_cached(ancestor) or ancestor.num_navigation > 0: + if self.has_cache_for(ancestor) or ancestor.num_navigation > 0: host_node = ancestor break else: nodes_to_cache.append(ancestor) - if not self.is_cached(host_node): + if not self.has_cache_for(host_node): self._add_to_cache(self.db, host_node) # Cache the queryset instance for every node that was passed over, as well. - hosted_navigation = self._get_from_cache(self.db, host_node) + hosted_navigation = self._get_cache_for(self.db, host_node) for node in nodes_to_cache: self._add_to_cache(self.db, node, hosted_navigation) return hosted_navigation - def is_cached(self, node): - return self._is_cached(self.db, node) - def _add_to_cache(self, using, node, qs=None): - key = getattr(node, 'pk', None) + if node.pk is None: + return if qs is None: - if key is None: - roots = self.none() - else: - roots = node.hosted_navigation.select_related('target_node') + roots = node.hosted_navigation.select_related('target_node') + qs = self.none() for root in roots: root_qs = root.get_descendants(include_self=True).complex_filter({'%s__lte' % root._mptt_meta.level_attr: root.get_level() + root.depth}).exclude(depth__isnull=True) - if qs is None: - qs = root_qs - else: - qs |= root_qs - - if qs is None: - qs = self.none() + qs |= root_qs - self.__class__._cache.setdefault(using, {})[key] = qs + self.__class__._cache.setdefault(using, {})[node] = qs + + def has_cache(self): + return self.db in self.__class__._cache and self.__class__._cache[self.db] + + def _get_cache_for(self, using, node): + return self.__class__._cache[self.db][node] + + def is_cached(self, navigation): + return self._is_cached(self.db, navigation) + + def _is_cached(self, using, navigation): + cache = self.__class__._cache[using] + for qs in cache.values(): + if navigation in qs: + return True + return False - def _get_from_cache(self, using, node): - key = getattr(node, 'pk', None) - return self.__class__._cache[self.db][key] + def has_cache_for(self, node): + return self._has_cache_for(self.db, node) - def _is_cached(self, using, node): + def _has_cache_for(self, using, node): try: - self._get_from_cache(using, node) + self._get_cache_for(using, node) except KeyError: return False return True + def clear_cache_for(self, node): + """Clear the cache for a node and all its descendants""" + self._clear_cache_for(self.db, node) + + def _clear_cache_for(self, using, node): + # Clear the cache for all descendants of the node. Ideally we would + # only clear up to another hosting node, but the complexity is not + # necessary and may not be possible. + descendants = node.get_descendants(include_self=True) + cache = self.__class__._cache[using] + for node in descendants: + cache.pop(node, None) + def clear_cache(self, navigation=None): """ Clear out the navigation cache. This needs to happen during database flushes @@ -116,9 +132,9 @@ class NavigationManager(TreeManager): self.__class__._cache.clear() elif self.db in self.__class__._cache: cache = self.__class__._cache[self.db] - for pk, qs in cache.items(): + for node, qs in cache.items(): if navigation in qs: - cache.pop(pk) + cache.pop(node) class Navigation(TreeEntity): @@ -175,23 +191,27 @@ class Navigation(TreeEntity): target_url = property(get_target_url) def is_active(self, request): - # First check if this particular navigation is active. It is considered active if: - # - the requested node is this instance's target node and its subpath matches the requested path. - # - the requested node is a descendant of this instance's target node and this instance's target - # node is not the hosting node of this navigation structure. - # - this instance has no target node and the url matches either the request path or the full url. - # - any of this instance's children are active. node = request.node - if self.target_node == node: - if self.target_url == request.path: - return True - elif self.target_node is None: - if self.url_or_subpath == request.path or self.url_or_subpath == "http%s://%s%s" % (request.is_secure() and 's' or '', request.get_host(), request.path): - return True - elif self.target_node.is_ancestor_of(node) and self.target_node != self.hosting_node: + if self.target_url == request.path: + # Handle the `default` case where the target_url and requested path + # are identical. + return True + + if self.target_node is None and self.url_or_subpath == "http%s://%s%s" % (request.is_secure() and 's' or '', request.get_host(), request.path): + # If there's no target_node, double-check whether it's a full-url + # match. return True + ancestors = node.get_ancestors(ascending=True, include_self=True).annotate(num_navigation=models.Count("hosted_navigation")).filter(num_navigation__gt=0) + if ancestors: + # If the target node is an ancestor of the requested node, this is + # active - unless the target node is the `home` node for this set of + # navigation or this navigation points to some other url. + host_node = ancestors[0] + if self.target_node.is_ancestor_of(node) and self.target_node != host_node and not self.url_or_subpath: + return True + # Always fall back to whether the node has active children. return self.has_active_children(request) @@ -215,17 +235,22 @@ class Navigation(TreeEntity): if self._has_changed(): self._initial_data = model_to_dict(self) - if self.is_cached(): - Navigation.objects.clear_cache(self) - else: - for navigation in self.get_ancestors(): - if navigation.hosting_node and navigation.is_cached() and self.get_level() <= (navigation.get_level() + navigation.depth): - Navigation.objects.clear_cache(navigation) + if Navigation.objects.has_cache(): + if self.is_cached(): + Navigation.objects.clear_cache(self) + else: + for navigation in self.get_ancestors(): + if navigation.hosting_node and navigation.is_cached() and self.get_level() <= (navigation.get_level() + navigation.depth): + Navigation.objects.clear_cache(navigation) + + if self.hosting_node and Navigation.objects.has_cache_for(self.hosting_node): + Navigation.objects.clear_cache_for(self.hosting_node) def delete(self, *args, **kwargs): super(Navigation, self).delete(*args, **kwargs) Navigation.objects.clear_cache(self) class Meta: - ordering = ['order'] + # Should I even try ordering? + ordering = ['order', 'lft'] verbose_name_plural = 'navigation' \ No newline at end of file diff --git a/contrib/shipherd/templatetags/shipherd.py b/contrib/shipherd/templatetags/shipherd.py index 8adef3e..db15d46 100644 --- a/contrib/shipherd/templatetags/shipherd.py +++ b/contrib/shipherd/templatetags/shipherd.py @@ -2,6 +2,7 @@ from django import template from django.conf import settings from django.utils.safestring import mark_safe from philo.contrib.shipherd.models import Navigation +from philo.models import Node from mptt.templatetags.mptt_tags import RecurseTreeNode, cache_tree_children @@ -33,7 +34,16 @@ class RecurseNavigationNode(RecurseTreeNode): return '' instance = self.instance_var.resolve(context) - roots = cache_tree_children(Navigation.objects.closest_navigation(instance)) + + if isinstance(instance, Node): + qs = Navigation.objects.closest_navigation(instance) + elif hasattr(instance, '__iter__'): + # Is this the right way to check? + qs = instance + else: + return settings.TEMPLATE_STRING_IF_INVALID + + roots = cache_tree_children(qs) bits = [self._render_node(context, node, request) for node in roots] return ''.join(bits) @@ -64,7 +74,7 @@ def recursenavigation(parser, token): if len(bits) != 2: raise template.TemplateSyntaxError(_('%s tag requires an instance') % bits[0]) - instance_var = template.Variable(bits[1]) + instance_var = parser.compile_filter(bits[1]) template_nodes = parser.parse(('endrecursenavigation',)) parser.delete_first_token() @@ -74,4 +84,9 @@ def recursenavigation(parser, token): @register.filter def has_navigation(node): - return bool(Navigation.objects.closest_navigation(node).count()) \ No newline at end of file + return bool(Navigation.objects.closest_navigation(node).count()) + + +@register.filter +def targeting_navigation(node): + return Navigation.objects.closest_navigation(node).filter(target_node=node).order_by('level', 'lft') \ No newline at end of file