Try to re-use settings.py template loader config before using defaults

[ekaj2]

Fix #30

wrap_loaders checks if loaders[0][0] == "template_partials.loader.Loader". Which it doesn't if you had set TEMPLATES["OPTIONS"]["loaders"] to anything else. django-components suggests you do this in the installation section:

TEMPLATES = [
    {
        ...,
        'OPTIONS': {
            'context_processors': [
                ...
            ],
            'loaders':[(
                'django.template.loaders.cached.Loader', [
                    'django.template.loaders.filesystem.Loader',
                    'django.template.loaders.app_directories.Loader',
                    'django_components.template_loader.Loader',
                ]
            )],
        },
    },
]

So loaders[0][0] is actually "django.template.loaders.cached.Loader". Then wrap_loaders overrides it:

            already_configured = (
                loaders
                and isinstance(loaders, (list, tuple))
                and isinstance(loaders[0], tuple)
                and loaders[0][0] == "template_partials.loader.Loader"
            )
            if not already_configured:
                template_config.pop("APP_DIRS", None)
                default_loaders = [
                    "django.template.loaders.filesystem.Loader",
                    "django.template.loaders.app_directories.Loader",
                ]
                cached_loaders = [
                    ("django.template.loaders.cached.Loader", default_loaders)
                ]
                partial_loaders = [("template_partials.loader.Loader", cached_loaders)]
                template_config["OPTIONS"]["loaders"] = partial_loaders
            break

So the easy fix for anyone running into this is to wrap it yourself:

...
        "OPTIONS": {
            'loaders': [(
                'template_partials.loader.Loader', [(
                    'django.template.loaders.cached.Loader', [
                        'django.template.loaders.filesystem.Loader',
                        'django.template.loaders.app_directories.Loader',
                        'django_components.template_loader.Loader',
                    ])
                ]
            )],
...

This does that by default by wrap_loaders and uses the default config as a fallback.

[carltongibson]

We'd need a regression test here.

[ekaj2]

We'd need a regression test here.

For sure, just added one modeling after the other SimpleAppConfig test. Let me know if that's sufficient.

[carltongibson]

Hi @ekaj2.

So I don't think this works for the general case.

Especially when using docker, folks often disable the cached loader in development.

That gives them a config that looks like this:

                "OPTIONS": {
                    "loaders":[
                        "django.template.loaders.filesystem.Loader",
                        "django.template.loaders.app_directories.Loader",
                        "custom.custom_loader.Loader",
                    ],
                },

We can't then assume that the cached loader is wanted.

Can I ask you to add a test with this kind of config?

If I'm eyeballing it right, I think we can set the default loaders and wrap them in the cached loader when not loaders. Otherwise we should just wrap the set value of loaders.

[garbelini]

It doesn't seem to me that the cached Loader is meant to be used as a child loader and passes calls directly through to it's children bypassing the caching mechanism. We noticed a considerable increase in our CI test time once we introduced it.

I might be missing something but this is what I understand currently:

1. The engine calls the top loader get_template method
2. the template_partials implements loader get_template and calls super().get_template

   django-template-partials/src/template_partials/loader.py

   Line 37 in 71128aa

   template = super().get_template(template_name, skip)

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

   django-template-partials/src/template_partials/loader.py

   Lines 56 to 58 in 71128aa

   	def get_template_sources(self, template_name):
   	for loader in self.loaders:
   	yield from loader.get_template_sources(template_name)

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]

Hi @garbelini — good spot. Yes, just eyeballing it, your analysis looks good.

Do you want to move your comment wholesale to a new issue (or perhaps a PR adding the two tests)?

The fix, I'd think, will be to call the cached loader's get_template (if len(self.loaders) == 1 and isinstance(...)) falling back to the current pathway otherwise.

[garbelini]

@carltongibson sounds good. I'll create an issue for it.

Thanks for the prompt reply!

[garbelini]

Created #36
I'll take a stab at the suggested fix and open a PR