carltongibson / django-template-partials

Reusable named inline partials for the Django Template Language.
MIT License
387 stars 15 forks source link

Cached template loader bypassed by using it as a sub-loader on template-partials loader #36

Closed garbelini closed 3 months ago

garbelini commented 3 months ago

The automatic configuration of settings.TEMPLATES adds Django's cached loader as child loader.

It seem like the cached Loader is meant to be used only as a top level loader. If used as a child loader it passes calls directly to it's children, bypassing the caching mechanism. We noticed a considerable increase in our CI test time once we introduced it.

  1. The engine calls the top loader get_template method
  2. the template_partials implements loader get_template and calls super().get_template https://github.com/carltongibson/django-template-partials/blob/71128aab3007be18e56f761509e6d536a5378c51/src/template_partials/loader.py#L37
  3. Django's base.Loader will then proceed to iterate over all nested possible template_sources https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/base.py#L17-L24
  4. Back in the template_partials Loader the get_template_sources forwards the calls down it's child loaders https://github.com/carltongibson/django-template-partials/blob/71128aab3007be18e56f761509e6d536a5378c51/src/template_partials/loader.py#L56-L58
  5. At this point the first one is the cached.Loader that will, unfortunately, do exactly the same thing bypassing the actual method that caches templates get_templates https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/cached.py#L68-L70
  6. This will eventually hit the filesystem.Loader that will return the template Origin https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/filesystem.py#L27-L45
  7. Next call to the same template, the same will happen not touching the cached loader get_template

This is a simple test case showing the behavior.

class ChildCachedLoaderTest(TestCase):
    def test_child_cached_loader(self):
        wrap_loaders("django")
        engine = engines["django"].engine
        partial_loader = engine.template_loaders[0]
        self.assertEqual(type(partial_loader).__module__, "template_partials.loader")
        cached_loader = partial_loader.loaders[0]
        self.assertEqual(type(cached_loader).__module__, "django.template.loaders.cached")
        self.assertEqual(len(cached_loader.get_template_cache), 0)
        engine.get_template("example.html")
        self.assertEqual(len(cached_loader.get_template_cache), 1)  # <-- Failure

@override_settings(
    TEMPLATES=[{"BACKEND": "django.template.backends.django.DjangoTemplates", "APP_DIRS": True}]
)
class SanityCheckCachedTest(TestCase):
    def test_no_wrap_loaders(self):
        tpl_engine = django.template.engines["django"].engine
        # Cached tin the top level
        cached_loader = tpl_engine.template_loaders[0]
        self.assertIsInstance(cached_loader, django.template.loaders.cached.Loader)
        self.assertEqual(len(cached_loader.get_template_cache), 0)
        tpl_engine.get_template("example.html")
        self.assertEqual(len(cached_loader.get_template_cache), 1)

Not entirely sure how to work around or fix the issue. The Django test cases for the loaders don't seem to go beyond one level of nesting so it doesn't feel like it's something planned for. I just skimmed them so I might have overlooked something. It does feel quite unintuitive and a missed opportunity if that's the case.

carltongibson commented 3 months ago

Fixed in #37