feat: cache queries in filters (#124)

* flake8

* cache the filter results of model objects

* fix test failure

* add test for filter caching

* improve filter caching

* use csv for logging

* PR feedback

* update changelog

* bump version to 0.26.0

Author: dshafer

.bumpversion.cfg

@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 0.25.3
+current_version = 0.26.0
 
 [bumpversion:file:pyproject.toml]
 

CHANGELOG.md

@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+# [0.26.0] - 2023-03-08
+- [PR 124](https://github.com/salesforce/django-declarative-apis/pull/124) Add query caching in filters
+
 # [0.25.3] - 2023-02-09
 ### Fixed
 - [PR 122](https://github.com/salesforce/django-declarative-apis/pull/122) Handle malformed OAuth header

django_declarative_apis/machinery/filtering.py

@@ -7,12 +7,16 @@
 
 from collections import defaultdict
 import inspect
+import logging
 import types
 
+from django.conf import settings
 from django.db import models
 from django.db.models import ManyToOneRel
 from django.core.exceptions import FieldDoesNotExist
 
+logger = logging.getLogger()
+
 NEVER = 0
 ALWAYS = 1
 IF_TRUTHY = 2
@@ -63,8 +67,14 @@ def _get_unexpanded_field_value(inst, field_name, field_type):
         return {display_key: getattr(obj, display_key)}
 
 
-def _get_filtered_field_value(
-    inst, field_name, field_type, filter_def, expand_this, expand_children
+def _get_filtered_field_value(  # noqa: C901
+    inst,
+    field_name,
+    field_type,
+    filter_def,
+    expand_this,
+    expand_children,
+    filter_cache,
 ):
     # get the value from inst
     if field_type == NEVER:
@@ -80,6 +90,25 @@ def _get_filtered_field_value(
             val = _get_unexpanded_field_value(inst, field_name, field_type)
     else:
         try:
+            if isinstance(inst, (models.Model)):
+                try:
+                    field_meta = inst._meta.get_field(field_name)
+                    if field_meta.is_relation:
+                        val_pk = getattr(inst, field_meta.attname)
+                        val_cls = field_meta.related_model
+                        val_expand_children = expand_children.get(field_name, {})
+                        cache_key = _make_filter_cache_key(
+                            val_expand_children, val_cls, val_pk
+                        )
+                        if cache_key in filter_cache:
+                            logger.debug(
+                                "ev=filter_cache, status=hit, key=%s", cache_key
+                            )
+                            return filter_cache[cache_key]
+                except FieldDoesNotExist:
+                    # this happens when you reference the special field "pk" in filters
+                    pass
+
             val = getattr(inst, field_name)
         except (AttributeError, FieldDoesNotExist) as e:  # noqa
             return None
@@ -93,7 +122,11 @@ def _get_filtered_field_value(
         val, (list, tuple, models.Model, models.query.QuerySet)
     ):
         val = _apply_filters_to_object(
-            val, filter_def, expand_children=expand_children, klass=val.__class__
+            val,
+            filter_def,
+            expand_children=expand_children,
+            klass=val.__class__,
+            filter_cache=filter_cache,
         )
 
     if (
@@ -107,22 +140,53 @@ def _get_filtered_field_value(
         return None
 
 
+def _make_filter_cache_key(expand_children, klass, pk):
+    return (str(expand_children), klass, str(pk))
+
+
 # TODO: make this method less complex and remove the `noqa`
 def _apply_filters_to_object(  # noqa: C901
-    inst, filter_def, expand_children=None, klass=None
+    inst,
+    filter_def,
+    expand_children=None,
+    klass=None,
+    filter_cache=None,
 ):
+    is_cacheable = False
+    caching_enabled = getattr(settings, "DDA_FILTER_MODEL_CACHING_ENABLED", False)
+    if (
+        caching_enabled
+        and isinstance(inst, (models.Model,))
+        and filter_cache is not None
+    ):
+        is_cacheable = True
+        pk = getattr(inst, "pk")
+        cache_key = _make_filter_cache_key(expand_children, klass, pk)
+        if cache_key in filter_cache:
+            logger.debug("ev=filter_cache, status=hit, key=%s", cache_key)
+            return filter_cache[cache_key]
+        else:
+            logger.debug("ev=filter_cache, status=miss, key=%s", cache_key)
     if isinstance(inst, (list, tuple, models.query.QuerySet)):
         # if it's a tuple or list, iterate over the collection and call _apply_filters_to_object on each item
         return [
             _apply_filters_to_object(
-                item, filter_def, expand_children=expand_children, klass=item.__class__
+                item,
+                filter_def,
+                expand_children=expand_children,
+                klass=item.__class__,
+                filter_cache=filter_cache,
             )
             for item in inst
         ]
     elif isinstance(inst, (dict,)):
         return {
             k: _apply_filters_to_object(
-                v, filter_def, expand_children=expand_children, klass=v.__class__
+                v,
+                filter_def,
+                expand_children=expand_children,
+                klass=v.__class__,
+                filter_cache=filter_cache,
             )
             for k, v in inst.items()
         }
@@ -134,16 +198,28 @@ def _apply_filters_to_object(  # noqa: C901
         for base_class in inspect.getmro(klass):
             if base_class in filter_def:
                 result = _apply_filters_to_object(
-                    inst, filter_def, expand_children=expand_children, klass=base_class
+                    inst,
+                    filter_def,
+                    expand_children=expand_children,
+                    klass=base_class,
+                    filter_cache=filter_cache,
                 )
                 break
+
+        if is_cacheable:
+            filter_cache[cache_key] = result
+
         return result
     else:
         # first, recursively populate from any ancestor classes in the inheritance hierarchy
         result = defaultdict(list)
         for base in klass.__bases__:
             filtered_ancestor = _apply_filters_to_object(
-                inst, filter_def, expand_children=expand_children, klass=base
+                inst,
+                filter_def,
+                expand_children=expand_children,
+                klass=base,
+                filter_cache=filter_cache,
             )
             if filtered_ancestor:
                 result.update(filtered_ancestor)
@@ -166,6 +242,7 @@ def _apply_filters_to_object(  # noqa: C901
                     filter_def,
                     expand_this=field_name in expand_children,
                     expand_children=expand_children.get(field_name, {}),
+                    filter_cache=filter_cache,
                 )
 
                 if value is not None and value != DEFAULT_UNEXPANDED_VALUE:
@@ -176,6 +253,9 @@ def _apply_filters_to_object(  # noqa: C901
         if expandables:
             result[EXPANDABLE_FIELD_KEY] += expandables
 
+        if is_cacheable:
+            filter_cache[cache_key] = result
+
         return result
 
 
@@ -195,5 +275,9 @@ def apply_filters_to_object(inst, filter_def, expand_header=""):
     else:
         expand_dict = {}
     return _apply_filters_to_object(
-        inst, filter_def, expand_children=expand_dict, klass=inst.__class__
+        inst,
+        filter_def,
+        expand_children=expand_dict,
+        klass=inst.__class__,
+        filter_cache={},
     )

django_declarative_apis/models.py

@@ -76,8 +76,8 @@ def create(self, **kwargs):
 
 
 class OauthConsumer(BaseConsumer):
-    """:code:`OAuthConsumer` inherits from :code:`BaseConsumer` and it based on :code:`OAuth1.0a`. It adds the additional
-    properties of key, secret, and rsa_public_key_pem.
+    """:code:`OAuthConsumer` inherits from :code:`BaseConsumer` and it based on :code:`OAuth1.0a`.
+    It adds the additional properties of key, secret, and rsa_public_key_pem.
 
     **Example**
 

docs/source/conf.py

@@ -74,7 +74,7 @@
 # built documents.
 
 # The full version, including alpha/beta/rc tags.
-release = '0.25.3'  # set by bumpversion
+release = '0.26.0'  # set by bumpversion
 
 # The short X.Y version.
 version = release.rsplit('.', 1)[0]

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "django-declarative-apis"
-version = "0.25.3"  # set by bumpversion
+version = "0.26.0"  # set by bumpversion
 description = "Simple, readable, declarative APIs for Django"
 readme = "README.md"
 dependencies = [

tests/filters.py

@@ -36,6 +36,10 @@
         "parent_field": expandable(model_class=models.ParentModel),
         "parents": expandable(model_class=models.ParentModel),
     },
+    models.InefficientLeaf: {"id": ALWAYS},
+    models.InefficientBranchA: {"leaf": ALWAYS},
+    models.InefficientBranchB: {"leaf": ALWAYS},
+    models.InefficientRoot: {"branch_a": ALWAYS, "branch_b": ALWAYS},
 }
 
 RENAMED_EXPANDABLE_MODEL_FIELDS = {

tests/machinery/test_base.py

@@ -11,6 +11,7 @@
 
 import django.core.exceptions
 import django.test
+from django.test.utils import override_settings
 import kombu.exceptions
 from unittest import mock
 from django.core.cache import cache
@@ -21,7 +22,7 @@
 from django_declarative_apis.machinery import errors, filtering, tasks
 from django_declarative_apis.machinery.tasks import future_task_runner
 from django_declarative_apis.resources.utils import HttpStatusCode
-from tests import testutils, models
+from tests import testutils, models, filters
 
 _TEST_RESOURCE = {"foo": "bar"}
 
@@ -703,6 +704,36 @@ def resource(self):
         self.assertRaises(errors.ClientErrorForbidden, bound_endpoint.get_response)
 
 
+class FilterCachingTestCase(django.test.TestCase):
+    def setUp(self):
+        super().setUp()
+        leaf = models.InefficientLeaf.objects.create(id=1)
+        branch_a = models.InefficientBranchA.objects.create(id=1, leaf=leaf)
+        branch_b = models.InefficientBranchB.objects.create(id=1, leaf=leaf)
+        root = models.InefficientRoot.objects.create(
+            id=4, branch_a=branch_a, branch_b=branch_b
+        )
+        self.root_id = root.id
+
+    def test_filter_query_cache_reduces_queries(self):
+        root = models.InefficientRoot.objects.get(id=self.root_id)
+        with self.assertNumQueries(4):
+            filtering.apply_filters_to_object(
+                root,
+                filters.DEFAULT_FILTERS,
+            )
+
+        root = models.InefficientRoot.objects.get(id=self.root_id)
+        with self.assertNumQueries(3):
+            with override_settings(
+                DDA_FILTER_MODEL_CACHING_ENABLED=True, DDA_FILTER_CACHE_DEBUG_LOG=True
+            ):
+                filtering.apply_filters_to_object(
+                    root,
+                    filters.DEFAULT_FILTERS,
+                )
+
+
 class ResourceUpdateEndpointDefinitionTestCase(
     testutils.RequestCreatorMixin, django.test.TestCase
 ):

tests/models.py

@@ -53,6 +53,26 @@ class RootNode(models.Model):
     parent_field = models.ForeignKey(ParentModel, on_delete=models.CASCADE)
 
 
+class InefficientLeaf(models.Model):
+    id = models.IntegerField(primary_key=True)
+
+
+class InefficientBranchA(models.Model):
+    id = models.IntegerField(primary_key=True)
+    leaf = models.ForeignKey(InefficientLeaf, on_delete=models.CASCADE)
+
+
+class InefficientBranchB(models.Model):
+    id = models.IntegerField(primary_key=True)
+    leaf = models.ForeignKey(InefficientLeaf, on_delete=models.CASCADE)
+
+
+class InefficientRoot(models.Model):
+    id = models.IntegerField(primary_key=True)
+    branch_a = models.ForeignKey(InefficientBranchA, on_delete=models.CASCADE)
+    branch_b = models.ForeignKey(InefficientBranchB, on_delete=models.CASCADE)
+
+
 try:
     import dirtyfields