Skip to content

Fixed #36410 -- Added named template partials to DTL #19643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FarhanAliRaza
Copy link
Contributor

  • Updated get_template in Django template backend to load a partial template
  • Added partialdef and partial template tags
  • Added documentation for the partials tags
  • Added tests for the partial tags

Trac ticket number

ticket-36410

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @FarhanAliRaza, amazing start! 🌟

Thank you for working on this. I have added a first round of initial comments. Let me know if you have any questions.

Looking forward to seeing this progressing!

@FarhanAliRaza
Copy link
Contributor Author

Thanks for the review.
Exciting times ahead. I will start fixing these things as soon as possible.

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FarhanAliRaza Thank you for the round of fixes, I appreciate the quick turnaround!

I added some more comments, specifically around tests we should use a bit more of subTest, but this is looking great overall.

Then, there is an outstanding design question about where best to place the "template proxy". I like Carlton's suggestions and I have added a few more notes. Also I'm looking forward of @ngnpope input!

Once all the above is resolved, I will do a top-bottom final review to ensure consistency and live testing to check correctness.

@carltongibson
Copy link
Member

@nessita I don't know why GitHub is not showing it on the main Conversations tab, but I replied to your #19643 (comment), and it does show in the Files view.

@FarhanAliRaza
Copy link
Contributor Author

Extra review points are fixed. Now, I'm just waiting for the code-moving bit to be finalized.

@nessita
Copy link
Contributor

nessita commented Jul 23, 2025

@nessita I don't know why GitHub is not showing it on the main Conversations tab, but I replied to your #19643 (comment), and it does show in the Files view.

Thank you for pointing that out, I commented in the same thread.

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astonishing progress, thank you @FarhanAliRaza and @carltongibson!

@FarhanAliRaza
Copy link
Contributor Author

Added syntax tests. Refactored tests to match other syntax tests like include tests.
Moved TemplateProxy to base.py and renamed it to PartialTemplate.
Moved SubDictionaryWrapper to utils.py

@nessita
Copy link
Contributor

nessita commented Jul 30, 2025

Added syntax tests. Refactored tests to match other syntax tests like include tests. Moved TemplateProxy to base.py and renamed it to PartialTemplate. Moved SubDictionaryWrapper to utils.py

Thank you @FarhanAliRaza for the updates, I'll be restarting work/review on this PR tomorrow morning.

@nessita
Copy link
Contributor

nessita commented Jul 30, 2025

@ngnpope Hello! Is this PR something you wanted/could take a look? Non worries if not, but let me know when you can.

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been doing a deep dive on tests, both the unit and manual tests.

I'll push some tweaks, along with some cleanup, including a squash of all commits. I'm also spent an embarrassing amount of time trying to replace the current mock-heavy test_partial_template_get_exception_info with a test that would be more realistic (more on that below). This has lead to the following potential bug discovery.

Local setup

  • Basic Django project with a testapp app and testapp/urls.py that include:
    path("partialtest/", TemplateView.as_view(template_name="partialtest.html")),
  • partialtest.html looks like this:
<h1>Title</h1>

{% partialdef testtestest %}
    <p>{{ nonexistent|default:"alsonotthere" }}</p>
{% endpartialdef %}

<h2>Sub Title</h2>
{% partial testtestest %}

Bug?

  1. Should path("partialtestpartial/", TemplateView.as_view(template_name="partialtest.html#testestest")) work? Currently is crashes with traceback:
Internal Server Error: /testapp/partialtestpartial/
Traceback (most recent call last):
  File "/home/nessita/fellowship/django/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/nessita/fellowship/django/django/core/handlers/base.py", line 221, in _get_response
    response = response.render()
  File "/home/nessita/fellowship/django/django/template/response.py", line 114, in render
    self.content = self.rendered_content
                   ^^^^^^^^^^^^^^^^^^^^^
  File "/home/nessita/fellowship/django/django/template/response.py", line 90, in rendered_content
    template = self.resolve_template(self.template_name)
  File "/home/nessita/fellowship/django/django/template/response.py", line 72, in resolve_template
    return select_template(template, using=self.using)
  File "/home/nessita/fellowship/django/django/template/loader.py", line 47, in select_template
    raise TemplateDoesNotExist(", ".join(template_name_list), chain=chain)
django.template.exceptions.TemplateDoesNotExist: partialtest.html#testestest
[30/Jul/2025 19:24:50] "GET /testapp/partialtestpartial/ HTTP/1.1" 500 87799
  1. If we change "alsonotthere" with alsonotthere (i.e. make the literal string a missing var in the context), IRL we get the expected exception VariableDoesNotExist and I see PartialTemplate.get_exception_info being called. But I tried to model a test after this and is close but it needs tweaks to fully mimic a real scenario, could you take a look and see what's missing?
from django.template import Context, Engine, Template, VariableDoesNotExist
from django.template.base import Token, TokenType
from django.test import SimpleTestCase, override_settings


class TemplateExceptionInfoTests(SimpleTestCase):

    @override_settings(DEBUG=True)
    def test_get_exception_info_reports_correct_location_locmem(self):
        buggy_template = """<h1>Title</h1>
{% partialdef testtestest %}
    <p>{{ nonexistent|default:alsonotthere }}</p>
{% endpartialdef %}
<h2>Sub Title</h2>
{% partial testtestest %}
"""
        engine = Engine(
            loaders=[
                ("django.template.loaders.locmem.Loader", {"template": buggy_template}),
            ]
        )
        # This does NOT trigger PartialTemplate.get_exception_info AND has an origin.
        template = engine.get_template("template")

        # This DOES trigger PartialTemplate.get_exception_info, but it has NO origin.
        template = Template(buggy_template, origin=template.origin)
        try:
            template.render(Context({}))
        except VariableDoesNotExist as e:
            # Simulate token from the failed parse.
            fake_token = Token(
                token_type=TokenType.VAR,
                contents="nonexistent|default:alsonotthere",
                position=(10, len(buggy_template)),
            )
            exc_info = template.get_exception_info(e, token=fake_token)

        # exc_info has the same content whether `PartialTemplate.get_exception_info`
        # returns something or {}!
        self.assertEqual(exc_info.get("message"), "Failed lookup for key [%s] in %r")

I need to stop for now but as said I will some changes so please pull them before making any further changes. Thanks! 🌟

@nessita nessita force-pushed the template-partials branch from a7a6263 to 16804e4 Compare July 30, 2025 18:55
@carltongibson
Copy link
Member

carltongibson commented Jul 30, 2025

@nessita I don't know if this is the issue but, the partial name in the url is testestest :

path("partialtestpartial/", TemplateView.as_view(template_name="partialtest.html#testestest"))

But the partial in the template is testtestest:

{% partialdef testtestest %}

There's an extra t in the latter one, which would explain the template not found exception.

@nessita
Copy link
Contributor

nessita commented Jul 30, 2025

@nessita I don't know if this is the issue but, the partial name in the url is testestest :

path("partialtestpartial/", TemplateView.as_view(template_name="partialtest.html#testestest"))

But the partial in the template is testtestest:

{% partialdef testtestest %}

There's an extra t in the latter one, which would explain the template not found exception.

Thank you for looking so quickly, that was a typo (caused by one of the manual testing stages to test an undefined partial name). I've swicthed to partialtest for clarity and I still get:

Exception Type: TemplateDoesNotExist at /testapp/partialtestpartial/
Exception Value: partialtest.html#partialtest

Template:

<h1>Title</h1>

{% partialdef partialtest %}
    <p>{{ nonexistent|default:alsonotthere }}</p>
{% endpartialdef %}

<h2>Sub Title</h2>
{% partial partialtest %}

@nessita nessita force-pushed the template-partials branch from 16804e4 to 0024ea2 Compare July 30, 2025 19:21
@carltongibson
Copy link
Member

carltongibson commented Jul 30, 2025

@FarhanAliRaza See if you can reproduce.

Against template-partials (at b6adac3) I get:

  1. TemplateNotFound for a missing partial name: #not-a-partial say. (Expected)
  2. <p>alsonotthere</p> output for <p>{{ nonexistent|default:"alsonotthere" }}</p> (Expected)
  3. VariableDoesNotExist for <p>{{ nonexistent|default:alsonotthere }}</p> (I guess we're saying Expected right? — Didn't think about it fully, but going by what Natalia said.)

I didn't test this branch, but if that's not what we're seeing a difference snuck in, that we'll need to pin down.

@carltongibson
Copy link
Member

carltongibson commented Jul 30, 2025

I didn't test this branch, ...

OK, I tested. I can't reproduce this @nessita. With the "alsonotthere" version, I get the expected output in the browser.

Screenshot 2025-07-30 at 21 43 25

natalia.zip

@nessita
Copy link
Contributor

nessita commented Jul 30, 2025

I didn't test this branch, ...

OK, I tested. I can't reproduce this @nessita. With the "alsonotthere" version, I get the expected output in the browser.

Thank you! I'll debug more tomorrow, will start from scratch with your project.

@nessita
Copy link
Contributor

nessita commented Jul 30, 2025

I didn't test this branch, ...

OK, I tested. I can't reproduce this @nessita. With the "alsonotthere" version, I get the expected output in the browser.

Thank you! I'll debug more tomorrow, will start from scratch with your project.

I can confirm this works, as Carlton pointed out. I have just now tested with a bunch of prints removed, I wonder if that was causing the issues I saw before 🤔

As a side note, and not related to this branch nor to the issue above, a print of exception as the first statement in get_exception_info generates a max recursion error when there is an exception in a template. This is informational since it was very hard to pin point!

@FarhanAliRaza I guess that one thing that would be useful for me is if you could take a look at the test trying to test more realistically PartialTemplate.get_exception_info to, if possible, drop the current mocks.

@FarhanAliRaza
Copy link
Contributor Author

@FarhanAliRaza See if you can reproduce.

@carltongibson I can not reproduce it. Working as expected.

TemplateNotFound for a missing partial name: #not-a-partial say. (Expected)

alsonotthere

output for

{{ nonexistent|default:"alsonotthere" }}

(Expected) VariableDoesNotExist for

{{ nonexistent|default:alsonotthere }}

(I guess we're saying Expected right? — Didn't >think about it fully, but going by what Natalia said.)

The same applies to me for this current branch.

@nessita Working on the test case.

@FarhanAliRaza
Copy link
Contributor Author

@nessita I have added tests for get_exception_info by using the template_debug that I believe is the output of the get_exception_info

with self.assertRaises(VariableDoesNotExist) as cm:
            template.render(context)

exc_info = cm.exception.template_debug

@nessita
Copy link
Contributor

nessita commented Jul 31, 2025

@nessita I have added tests for get_exception_info by using the template_debug that I believe is the output of the get_exception_info

with self.assertRaises(VariableDoesNotExist) as cm:
            template.render(context)

exc_info = cm.exception.template_debug

Thank you! I can't look right away because I'm deep in other review, but could you please double check that if the implementation for PartialTemplate.get_exception_info would get changed to return {}, the proper test fail with meaningful information?

@FarhanAliRaza
Copy link
Contributor Author

@nessita

Yes, I made sure it gave KeyError
Should I add asserts to provide more descriptive errors?

ERROR: test_partial_runtime_error_exception_info (template_tests.syntax_tests.test_partials.PartialTagTests.test_partial_runtime_error_exception_info)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/farhan/code/django/tests/template_tests/syntax_tests/test_partials.py", line 448, in test_partial_runtime_error_exception_info
    self.assertIn("badsimpletag", exc_debug["during"])
                                  ~~~~~~~~~^^^^^^^^^^
KeyError: 'during'

======================================================================
ERROR: test_partial_runtime_exception_has_debug_info (template_tests.syntax_tests.test_partials.PartialTagTests.test_partial_runtime_exception_has_debug_info)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/farhan/code/django/tests/template_tests/syntax_tests/test_partials.py", line 303, in test_partial_runtime_exception_has_debug_info
    self.assertEqual(exc_info["during"], "{{ nonexistent|default:alsonotthere }}")
                     ~~~~~~~~^^^^^^^^^^
KeyError: 'during'

======================================================================
FAIL: test_partial_template_get_exception_info_delegation (template_tests.syntax_tests.test_partials.PartialTagTests.test_partial_template_get_exception_info_delegation)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/farhan/code/django/tests/template_tests/syntax_tests/test_partials.py", line 335, in test_partial_template_get_exception_info_delegation
    self.assertIn("message", exc_info)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'message' not found in {}

----------------------------------------------------------------------
Ran 25 tests in 0.016s

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @carltongibson and @nessita. I managed to do a review this evening - hope it helps. This is shaping up to be very nice and feels like it'll provide a lot of power with very little code to implement it - thanks for your efforts @FarhanAliRaza! 🥇

@FarhanAliRaza
Copy link
Contributor Author

Implemented the suggestion from @ngnpope .
Handled nested partials handling by updating find_partial_source to use a single regex and handle search in nested case.
Refactored SubDictionaryWrapper to move error handling out of it to RenderPartialNode
and moved it to django/utils/datastructures.py
And other refactors.

@carltongibson
Copy link
Member

@FarhanAliRaza @nessita on the new-features repo there's a suggestion to move _render() and render() into a common base mixin, that we might want to pick up here?

Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @FarhanAliRaza! This is shaping nicely and with a delight precision. 🌟

Today I focused mostly on the new syntax tests. Overall I would like to see more meaningful names used for the tests and the templates, and also more of subTest when semantically adequate. I added some suggestions for both of these points.

Then, a minor nitpick: I think that the partial name used for testing could be partial-name or testing-name to signal clearly to readers that this is a name identifier.

Thinking about performance, we may need one or two tests with big chunky templates (using html test files) making heavy use of the new tags to see if the regexes show any obvious catastrophic side-effect.

Comment on lines +337 to +334
def _render(self, context):
return self.nodelist.render(context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are missing some tests or something else is going on. If I change this code to be this instead:

Suggested change
def _render(self, context):
return self.nodelist.render(context)
def _render(self, context):
return ""

No test fails. I'm sure there is a reasonable explanation but I can't find it :-).

@nessita
Copy link
Contributor

nessita commented Aug 6, 2025

@FarhanAliRaza @nessita on the new-features repo there's a suggestion to move _render() and render() into a common base mixin, that we might want to pick up here?

I have read the new feature request and caught up with the comments in the django-components issue. My gut feeling is that this is a good idea and it'd solve somehow nicely the itchiness that the duplicated render/_render methods is causing me.

There is the outstanding question I just posted in my latest review about whether the PartialTemplate._render method is used at all (since making it return "" does not have any test failing).

@carltongibson
Copy link
Member

Thinking about performance, we may need one or two tests with big chunky templates (using html test files) making heavy use of the new tags to see if the regexes show any obvious catastrophic side-effect.

What do you have in mind here @nessita? (Im struggling to imagine the kind of case you want to look at.)

The regex is just used for the debug view right, fetching the source. Is there a DoS vector there? 🤔

@FarhanAliRaza
Copy link
Contributor Author

FarhanAliRaza commented Aug 7, 2025

There is the outstanding question I just posted in my latest review about whether the PartialTemplate._render method is used at all (since making it return "" does not have any test failing).

@nessita @carltongibson
The issue is that during testing, _render is replaced with the instrumented _render method.

Have to find a better way to do this.

@FarhanAliRaza
Copy link
Contributor Author

FarhanAliRaza commented Aug 7, 2025

@nessita Now tests will fail if we change _render method because now even after instrumentation, it calls the original method.

Comment on lines 114 to 116
if isinstance(self, PartialTemplate):
return _TestState.saved_data.partial_template_render(self, context)

Copy link
Member

@carltongibson carltongibson Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on, this can't be right. Let's slow down.

We don't do this for Template._render() — which is replaced in exactly the same way — so why are we doing it here?

If you make the same change (return "") to Template._render() the only failures are in admin_scripts, which are using call_command so in a subprocess likely.

So I don't think there's actual coverage here, even on Template._render()

What needs to happen here — if we need tests — is a test_private_render_is_actually_called() (or similar) that restores the _render() method, and then checks the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So add a test test_private_render_is_actually_called.
or
We do the same solution with Template class.
return _TestState.saved_data.template_render(self, context)
As this makes more sense because this way always the original function is being called, we are getting the signal that we want from instrumenting. It will make it easier to test any future changes if a contributor makes any changes, and they will see the changes during testing.
If this causes no other unexpected problems ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option 3 is to do nothing, but a regression test is likely worthwhile.

I don't like the suggested change here. If we want to go this kind of route then we want to adjust instrumented_test_render to decorate the original method, and apply that, rather than using a pair of isinstance checks.

Given how long instrumented_test_render has worked how it has, I'd suggest that's a separate cleanup not part of this PR. (Would it not need a ticket?)

Copy link
Contributor Author

@FarhanAliRaza FarhanAliRaza Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to go to the Renderer Mixin route to refactor _render and render method to it. We will not even need that isinstance check (I think).

@ngnpope
Copy link
Member

ngnpope commented Aug 8, 2025

Thanks for the updates @FarhanAliRaza. I have some additional test cases I knocked up the other day after I reviewed that could be added if you haven't already thought of them - I didn't have time to check thoroughly if you have covered these, but wanted to give you what I had.

Here are some for tests/template_tests/test_partials.py for duplicate partial names:

    def test_nested_partials_with_duplicate_name(self):
        template_source = """
        {% partialdef duplicate %}{% partialdef duplicate %}
        CONTENT
        {% endpartialdef %}{% endpartialdef %}
        """
        with self.assertRaisesMessage(
            TemplateSyntaxError,
            "Partial 'duplicate' is already defined in the 'template.html' template.",
        ):
            Template(template_source, origin=Origin(name="template.html"))

    def test_conditional_partials_with_duplicate_names(self):
        # The correct way to resolve this is to push the conditional inside the partial.
        template_source = """
        {% if ... %}
          {% partialdef duplicate %}
          CONTENT
          {% endpartialdef %}
        {% else %}
          {% partialdef duplicate %}
          OTHER-CONTENT
          {% endpartialdef %}
        {% endif %}
        """
        with self.assertRaisesMessage(
            TemplateSyntaxError,
            "Partial 'duplicate' is already defined in the 'template.html' template.",
        ):
            Template(template_source, origin=Origin(name="template.html"))

Here is one for tests/template_tests/test_partials.py for checking support of named end tag:

    def test_find_partial_source_supports_named_end_tag(self):
        template_source = "{% partialdef thing %}CONTENT{% endpartialdef thing %}"
        template = Template(template_source)
        partial_proxy = template.extra_data["template-partials"]["thing"]

        result = partial_proxy.find_partial_source(template_source, "thing")
        self.assertEqual(
            result, "{% partialdef thing %}CONTENT{% endpartialdef thing %}"
        )

(This is already implicitly tested in test_nested_partials_rendering_with_context which you added since my last review, but it should probably be explicitly tested.)

Here are some for tests/template_tests/syntax_tests/test_partials.py for broken nesting:

    @setup(
        {
            "partial-broken-nesting": (
                "<div>Before partial</div>\n"
                "{% partialdef outer %}\n"
                "{% partialdef inner %}...{% endpartialdef outer %}\n"
                "{% endpartialdef inner %}\n"
                "<div>After partial content</div>"
            )
        }
    )
    def test_broken_partial_nesting(self):
        with self.assertRaises(TemplateSyntaxError) as cm:
            self.engine.get_template("partial-broken-nesting")

        self.assertIn("endpartialdef", str(cm.exception))
        self.assertIn("Invalid block tag", str(cm.exception))
        self.assertIn("'endpartialdef inner'", str(cm.exception))

        reporter = ExceptionReporter(None, cm.exception.__class__, cm.exception, None)
        traceback_data = reporter.get_traceback_data()

        exception_value = str(traceback_data.get("exception_value", ""))
        self.assertIn("Invalid block tag", exception_value)
        self.assertIn("'endpartialdef inner'", str(cm.exception))

    @setup(
        {
            "partial-broken-nesting-mixed": (
                "<div>Before partial</div>\n"
                "{% partialdef outer %}\n"
                "{% partialdef inner %}...{% endpartialdef %}\n"
                "{% endpartialdef inner %}\n"
                "<div>After partial content</div>"
            )
        }
    )
    def test_broken_partial_nesting_mixed(self):
        with self.assertRaises(TemplateSyntaxError) as cm:
            self.engine.get_template("partial-broken-nesting-mixed")

        self.assertIn("endpartialdef", str(cm.exception))
        self.assertIn("Invalid block tag", str(cm.exception))
        self.assertIn("'endpartialdef outer'", str(cm.exception))

        reporter = ExceptionReporter(None, cm.exception.__class__, cm.exception, None)
        traceback_data = reporter.get_traceback_data()

        exception_value = str(traceback_data.get("exception_value", ""))
        self.assertIn("Invalid block tag", exception_value)
        self.assertIn("'endpartialdef outer'", str(cm.exception))

And finally I had some more for testing find_partial_source() with nested partials for tests/template_tests/test_partials.py:

    def test_find_partial_source_supports_nested_partials(self):
        template_source = (
            "{% partialdef outer %}"
            "{% partialdef inner %}...{% endpartialdef %}"
            "{% endpartialdef %}"
        )
        template = Template(template_source)

        empty_proxy = template.extra_data["template-partials"]["outer"]
        other_proxy = template.extra_data["template-partials"]["inner"]

        outer_result = empty_proxy.find_partial_source(template_source, "outer")
        self.assertEqual(
            outer_result,
            (
                "{% partialdef outer %}{% partialdef inner %}"
                "...{% endpartialdef %}{% endpartialdef %}"
            ),
        )

        inner_result = other_proxy.find_partial_source(template_source, "inner")
        self.assertEqual(inner_result, "{% partialdef inner %}...{% endpartialdef %}")

    def test_find_partial_source_supports_nested_partials_and_named_end_tags(self):
        template_source = (
            "{% partialdef outer %}"
            "{% partialdef inner %}...{% endpartialdef inner %}"
            "{% endpartialdef outer %}"
        )
        template = Template(template_source)

        empty_proxy = template.extra_data["template-partials"]["outer"]
        other_proxy = template.extra_data["template-partials"]["inner"]

        outer_result = empty_proxy.find_partial_source(template_source, "outer")
        self.assertEqual(
            outer_result,
            (
                "{% partialdef outer %}{% partialdef inner %}"
                "...{% endpartialdef inner %}{% endpartialdef outer %}"
            ),
        )

        inner_result = other_proxy.find_partial_source(template_source, "inner")
        self.assertEqual(
            inner_result, "{% partialdef inner %}...{% endpartialdef inner %}"
        )

    def test_find_partial_source_supports_nested_partials_and_mixed_end_tags_1(self):
        template_source = (
            "{% partialdef outer %}"
            "{% partialdef inner %}...{% endpartialdef %}"
            "{% endpartialdef outer %}"
        )
        template = Template(template_source)

        empty_proxy = template.extra_data["template-partials"]["outer"]
        other_proxy = template.extra_data["template-partials"]["inner"]

        outer_result = empty_proxy.find_partial_source(template_source, "outer")
        self.assertEqual(
            outer_result,
            (
                "{% partialdef outer %}{% partialdef inner %}"
                "...{% endpartialdef %}{% endpartialdef outer %}"
            ),
        )

        inner_result = other_proxy.find_partial_source(template_source, "inner")
        self.assertEqual(inner_result, "{% partialdef inner %}...{% endpartialdef %}")

    def test_find_partial_source_supports_nested_partials_and_mixed_end_tags_2(self):
        template_source = (
            "{% partialdef outer %}"
            "{% partialdef inner %}...{% endpartialdef inner %}"
            "{% endpartialdef %}"
        )
        template = Template(template_source)

        empty_proxy = template.extra_data["template-partials"]["outer"]
        other_proxy = template.extra_data["template-partials"]["inner"]

        outer_result = empty_proxy.find_partial_source(template_source, "outer")
        self.assertEqual(
            outer_result,
            (
                "{% partialdef outer %}{% partialdef inner %}"
                "...{% endpartialdef inner %}{% endpartialdef %}"
            ),
        )

        inner_result = other_proxy.find_partial_source(template_source, "inner")
        self.assertEqual(
            inner_result, "{% partialdef inner %}...{% endpartialdef inner %}"
        )

@FarhanAliRaza
Copy link
Contributor Author

Thank you, @ngnpope and @nessita . I added the tests you suggested and made other refactors, including changes to test and template names.

@carltongibson I removed that change of instrumented method.

@nessita nessita force-pushed the template-partials branch 2 times, most recently from 4ad61a0 to 264fc67 Compare August 11, 2025 18:30
@nessita
Copy link
Contributor

nessita commented Aug 11, 2025

Thinking about performance, we may need one or two tests with big chunky templates (using html test files) making heavy use of the new tags to see if the regexes show any obvious catastrophic side-effect.

What do you have in mind here @nessita? (Im struggling to imagine the kind of case you want to look at.)

I'll try to think of a concrete example and I'll share here.

The regex is just used for the debug view right, fetching the source. Is there a DoS vector there? 🤔

Good point, while going deep in the review vertical I lost track of this detail. Can you help me understand which flow exactly triggers this code path?

@nessita nessita force-pushed the template-partials branch from 264fc67 to 44c2bbd Compare August 11, 2025 20:25
…plate Language.

Introduced `{% partialdef %}` and `{% partial %}` template tags to
define and render reusable named fragments within a template file.
Partials can also be accessed using the `template_name#partial_name`
syntax via `get_template()`, `render()`, `{% include %}`, and other
template-loading tools.

Adjusted `get_template()` behavior to support partial resolution, with
appropriate error handling for invalid names and edge cases. Introduced
`PartialTemplate` to encapsulate partial rendering behavior.

Includes tests and internal refactors to support partial context
binding, exception reporting, and tag validation.

Co-authored-by: Carlton Gibson <carlton@noumenal.es>
Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
Co-authored-by: Nick Pope <nick@nickpope.me.uk>
@nessita nessita force-pushed the template-partials branch from 44c2bbd to eb8c218 Compare August 11, 2025 20:29
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FarhanAliRaza Thank you for the steady and great work on this PR! 🌟

I went thru "tests vertical" today. I've pushed some cleanups, including two tests that are now commented out because they are failing. The tests are the ones below and I think using the {% include 'partial_included.html#included-partial' %} should work, right?

    @setup(
        {
            "partial_as_include_in_other_template": (
                "MAIN TEMPLATE START\n"
                "{% include 'partial_included.html#included-partial' %}\n"
                "MAIN TEMPLATE END"
            )
        },
        partial_templates,
    )
    def _test_partial_as_include_in_template(self):
        # XXX: Needs fixing?
        output = self.engine.render_to_string("partial_as_include_in_other_template")
        expected = (
            "MAIN TEMPLATE START\n"
            "THIS IS CONTENT FROM THE INCLUDED PARTIAL\n"
            "MAIN TEMPLATE END"
        )
        self.assertEqual(output, expected)

    @setup(
        {
            "nested_simple": (
                "{% extends 'base.html' %}"
                "{% block content %}"
                "This is my main page."
                "{% partialdef outer inline %}"
                "    It hosts a couple of partials."
                "    {% partialdef inner inline %}"
                "        And an inner one."
                "    {% endpartialdef inner %}"
                "{% endpartialdef outer %}"
                "{% endblock content %}"
            ),
            "use_outer": "{% include 'nested_simple#outer' %}",
            "use_inner": "{% include 'nested_simple#inner' %}",
        }
    )
    def _test_nested_partials(self):
        # XXX: Needs fixing?
        with self.subTest(template_name="use_outer"):
            output = self.engine.render_to_string("use_outer")
            self.assertEqual(output, "It hosts a couple of partials. And an inner one.")
        with self.subTest(template_name="use_inner"):
            output = self.engine.render_to_string("use_inner")
            self.assertEqual(output, "And an inner one.")

@carltongibson do you think I'm missing something? I double checked and I don't have local changes that could be messing things up. I have errors such as:

django.template.exceptions.TemplateDoesNotExist: partial_included.html#included-partial

@FarhanAliRaza
Copy link
Contributor Author

The tests are the ones below and using the {% include 'partial_included.html#included-partial' %} should work, right?

I think when the include tag is used, it tries to load using the base Engine instance, which does not use Django Template Backend

Some options that I can think of to fix this are to change

Engine.get_template method to make it aware of partials.

Change the loading logic in the Include tag to somehow load using the django backend.

@carltongibson
Copy link
Member

@FarhanAliRaza The examples from those tests work standardly. I use that pattern a lot. Wondering if it's a test artifact? 🤔

The base Engine.get_template goes via the loader, which traverses the backends to find the correct template. This still goes via the Django backend, which is partial aware.

@carltongibson
Copy link
Member

I'm not seeing the failure here locally. eb8c218
eed7f44 passes. The CI run looks to have passed too (the two failures look unrelated).

@FarhanAliRaza
Copy link
Contributor Author

FarhanAliRaza commented Aug 12, 2025

@carltongibson
I ll retest.

I'm not seeing the failure here locally. eb8c218

The first time that happened to me, too. Can you check if that specific test is running because it has _ in front of it? So it did not run for me for first try.

@carltongibson
Copy link
Member

The first time that happened to me, too. Can you check if that specific test is running because it has _ in front of it? So it did not run for me for first try.

Thanks. Got it.

@carltongibson
Copy link
Member

carltongibson commented Aug 12, 2025

@FarhanAliRaza Yep, so you're bang on. It's that the @setup helper is using the base Engine directly, rather than the DjangoTemplates backend (and then each backend, Django/Jinja/..., extends Engine).

We setup the loaders on Engine to use locmem and then it has no awareness of partials. Hence the error.

Engine.get_template method to make it aware of partials.

I don't think that's right: Partials are being added to the Django Backend, not the backend agnostic template functionality per se.

Change the loading logic in the Include tag to somehow load using the django backend.

No, include is fine. It's the test that's at fault here.

I think we just need to not use the setup helper here, and test the Django backend itself (which works).

Update:

Engine.get_template method to make it aware of partials.

I guess we could create a mini Engine subclass just with the necessary change for the setup helper to use. It would only need to pull off the fragment, call the super implementation, and then check for the partial at the end. (As the loader does in template-partials.)

Personally I'd rewrite the test but 🤷

@FarhanAliRaza
Copy link
Contributor Author

FarhanAliRaza commented Aug 12, 2025

I tried without @setup decorator also .
It gives the same error .
I changed the test to load using template files using filesystem loader and also by creating custom django backend in the test function, it still fails.

Because in include the loading happens through context.template loading that somehow does not use the Django backend. I am not sure how exactly but I added print statement loading during include, it does not Django backend for that.

@carltongibson
Copy link
Member

@FarhanAliRaza Can you push your rewritten test?

This pattern...

         "MAIN TEMPLATE START\n"
          "{% include 'partial_included.html#included-partial' %}\n"
          "MAIN TEMPLATE END"

... has been working since the package was created.

We've had tests on that since ≈ day one no? Suddenly there's a failure: it's unlikely to now need a change to include. (It's a test artefact is first suspect.)

FarhanAliRaza and others added 2 commits August 12, 2025 11:38
Co-authored-by: Carlton Gibson <carlton@noumenal.es>
@FarhanAliRaza
Copy link
Contributor Author

I only tested the usage of partial inside the included HTML file. Not tested partial loading like this {% include 'partial_included.html#included-partial' %} that is failing now.

only tested this about include

@setup(
        {
            "partial_with_include": (
                "MAIN TEMPLATE START\n"
                "{% include 'partial_included.html' %}\n"
                "MAIN TEMPLATE END"
            )
        },
        partial_templates,
    )
    def test_partial_in_included_template(self):
        output = self.engine.render_to_string("partial_with_include")
        expected = (
            "MAIN TEMPLATE START\nINCLUDED TEMPLATE START\n\n\n"
            "Now using the partial: \n"
            "THIS IS CONTENT FROM THE INCLUDED PARTIAL\n\n\n"
            "INCLUDED TEMPLATE END\n\nMAIN TEMPLATE END"
        )
        self.assertEqual(output, expected)

@FarhanAliRaza
Copy link
Contributor Author

test.zip
experimental project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants