diff --git a/.bumpversion.cfg b/.bumpversion.cfg index 0629799..b905a22 100644 --- a/.bumpversion.cfg +++ b/.bumpversion.cfg @@ -1,5 +1,5 @@ [bumpversion] -current_version = 0.25.3 +current_version = 0.26.0 [bumpversion:file:pyproject.toml] diff --git a/CHANGELOG.md b/CHANGELOG.md index 5958298..a587bac 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/django_declarative_apis/machinery/filtering.py b/django_declarative_apis/machinery/filtering.py index 7657c06..58899e8 100644 --- a/django_declarative_apis/machinery/filtering.py +++ b/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={}, ) diff --git a/django_declarative_apis/models.py b/django_declarative_apis/models.py index e962b63..481fff5 100644 --- a/django_declarative_apis/models.py +++ b/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** diff --git a/docs/source/conf.py b/docs/source/conf.py index 99d0e8c..50b4431 100644 --- a/docs/source/conf.py +++ b/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] diff --git a/pyproject.toml b/pyproject.toml index b12ce6e..abdb2c6 100644 --- a/pyproject.toml +++ b/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 = [ diff --git a/tests/filters.py b/tests/filters.py index 3024ec2..c94b894 100644 --- a/tests/filters.py +++ b/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 = { diff --git a/tests/machinery/test_base.py b/tests/machinery/test_base.py index 3cdf9ce..93e66ac 100644 --- a/tests/machinery/test_base.py +++ b/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 ): diff --git a/tests/models.py b/tests/models.py index 568106b..b90692d 100644 --- a/tests/models.py +++ b/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