Allow deep data in TemplateProcessor

[HSGamer]

As I mentioned in the previous PR, this one is to allow us to set a complex data map with keys from main and inner template.

An example for it can be seen in the tests:

    @Test
    public void test_DeepData_ListForInner() {
        TemplateProcessor mainTemplate = buildProcessor("Hall of {{ type }}:\n{{ list }}");
        TemplateProcessor listTemplate = buildProcessor("- {{ name }}");
        mainTemplate.registerInnerTemplate("list", listTemplate);

        Map<String, Object> data = Map.of(
                "type", "Fame",
                "list", List.of(
                        Map.of("name", "A"),
                        Map.of("name", "B"),
                        Map.of("name", "C")
                )
        );

        assertEquals(
                """
                Hall of Fame:
                - A
                - B
                - C""",
                mainTemplate.renderDeepTemplate(data)
        );
    }

[HSGamer]

Possibly related to #21

[byronka]

This is really good stuff, but I will need to ask your patience while I review. Thank you so much for your help!

[byronka]

I've been reviewing, this is looking good. A few design points:

1. There would be two ways to handle nested templates, we would want to consider deprecation of one of the approaches, so there is one clear preferred way to do it (or given that we are considering making a major change in the other PR, might as well make the change completely and leave only the new approach)
2. From your point of view, what is the benefit of this versus the previous way?
3. one friction for users, which existed before your change but this illuminated the problem, is demonstrated in the test called "test_DeepData_InnerInInner", where a user accidentally registers the templates in the wrong order (flip lines 671 and 672 in TemplatingTests), then it will render wrong values.

[HSGamer]

1. For now, I would prefer removing the ones with Map<String, String>. But for the long term, I might make a whole rework on how the parser works, since the thing about indentation complicates things here. I think a more lenient parser with line-by-line sections would be better so that we don't specifically care about indentation.
2. I would prefer the one with Map<String, Object>. It has the possibility of the value being any type not just String. So that we can check for special types like List and Map, and leave everything else as toString.
3. That's what I noticed when running that test. Since the registration of the inner template doesn't register itself, but rather make a copy of it. Render-To-Section doesn't care about Inner Template so we have to set them back to make it work. That's also why the order of registration is important.

[HSGamer]

I reworked the processor to accept any type of data, as long as it's an Object.
That way we won't care about whether we have to specify the data as Map<String, Object> or Map<String, String>. We can use var and don't have to update all the current tests.
All data object will now go through a normalize method to map them as either Map for keys in the template or List for data list in the inner template.
If a data for the template is not a Map, it will be parsed as Map.of("value", data), meaning we can parse that data by setting a {{ value }} key in the template (as seen in the test test_EdgeCase_PrimitiveData).

@byronka can you do a review again?

[byronka]

I appreciate your ambition with these changes, but I think this requires a bit more design, or possibly pulling back somewhat.

This is a large change, and it is hard for me to comprehend all the consequences. I am concerned to to make such significant changes without a pressing need.

Some notes:

The documentation above the new registerData(Object) and renderTemplate(Object) is obsolete. I am not even sure how I would document that sufficiently to avoid confusing the users, given the further points listed below.

The design to use "value" as a special keyword for templates is contrary to the premise of keeping simple. A user would need to remember that a key of "value" has special significance.

Using Object as the type, in java, removes helpful type information. The previous types were based on Map<String,String>, on the basis that the template operates with key-to-value pairs, or lists of them. With methods taking an Object, all sorts of things could be used (JSON objects, boxed-primitives like Boolean, records, etc), which adds complexity without clear benefit. What is enabled that wasn't possible before?

Even before this change, it was questionable that the data could be provided in different ways, like pre-registering (registerData) versus using renderTemplate. This is adding unexpected edge cases to this already-complicated API. Given that we are probably going to issue a major-version number because of the breaking changes on the IResponse improvements you provided, I would suggest we use this as an opportunity to actually lower the surface area and complexity of the TemplateProcessor, if anything. Minum is built on the premise of providing the fewest moving parts, and figuring out how to provide high-quality capability with fewer parts is a creative challenge.

All templates will end up as a string. Thus, inputs can reliably always be strings, so any necessary processing can be done beforehand. If something goes awry, the user is able to put a breakpoint on an accessible spot in their own code to see exactly what went into the template. There's essentially no adjustments made after the data goes in. Typical templating tools enable all sorts of sigil-and-convention-driven auto-magic mischief on data, which I would prefer to avoid.

These changes did raise the issue of the complexity of handling twice-nested templates. Right now there's some edge cases. On master, you can see how in TemplatingTests.test_EdgeCase_DeeplyNested it is necessary to apply inner-template registrations on the TemplateProcessors that are returned. In the new code you provided, it is necessary to register inner templates in the correct order (see TemplatingTests.test_DeepData_InnerInInner and flip the order of registerInnerTemplate lines, and watch it fail). In either case, it has annoying edge cases and expects non-intuitive actions from the user. On Minum, the framework should lean over backwards in efforts to help the user, by preventing ugly edge cases, using simpler types, or providing sufficient documentation and examples if nothing else works, even if that means providing less powerful tools.

The performance is slightly worsened in this change. Taking averages, I calculated this as about 7% slower with checks and 11% slower without checks. For the following, I ran the test test_Templating_LargeComplex_Performance with 500_000 as the value of warmup and main iterations, set at the top of the method.

With new changes:

STARTING WARMUP
with checks, processed 500000 templates in 3083 millis
without checks, processed 500000 templates in 1927 millis
STARTING MAIN
with checks, processed 500000 templates in 2198 millis
without checks, processed 500000 templates in 1779 millis
with checks, processed 500000 templates in 2213 millis
without checks, processed 500000 templates in 1781 millis
with checks, processed 500000 templates in 2262 millis
without checks, processed 500000 templates in 1783 millis

STARTING WARMUP
with checks, processed 500000 templates in 2987 millis
without checks, processed 500000 templates in 1841 millis
STARTING MAIN
with checks, processed 500000 templates in 2306 millis
without checks, processed 500000 templates in 1795 millis
with checks, processed 500000 templates in 2221 millis
without checks, processed 500000 templates in 1793 millis
with checks, processed 500000 templates in 2222 millis
without checks, processed 500000 templates in 1855 millis

Old:

STARTING WARMUP
with checks, processed 500000 templates in 2776 millis
without checks, processed 500000 templates in 1635 millis
STARTING MAIN
with checks, processed 500000 templates in 2107 millis
without checks, processed 500000 templates in 1614 millis
with checks, processed 500000 templates in 2082 millis
without checks, processed 500000 templates in 1596 millis
with checks, processed 500000 templates in 2073 millis
without checks, processed 500000 templates in 1577 millis

STARTING WARMUP
with checks, processed 500000 templates in 2587 millis
without checks, processed 500000 templates in 1631 millis
STARTING MAIN
with checks, processed 500000 templates in 2066 millis
without checks, processed 500000 templates in 1638 millis
with checks, processed 500000 templates in 2193 millis
without checks, processed 500000 templates in 1720 millis
with checks, processed 500000 templates in 2050 millis
without checks, processed 500000 templates in 1564 millis

[HSGamer]

If we were going to reduce moving parts in TemplateProcessor, I would suggest removing the use of INNER_TEMPLATE and the alternative way of registerData and renderTemplate() completely, and keeping only the renderTemplate(Map<String, String>) and renderTemplate(List<Map<String, String>>. The ultimate goal is still to split the constructor and the rendering and allow using the same processor for multiple threads with different data.
The support for Complex Object in this PR can be moved to a separated processor. Probably name it ComplexTemplateProcessor.

[HSGamer]

The problem with the current code is that when we can use the same processor for different list of map data, we can't do that in inner template. Inner template is treated the same as Static Data, with a fancy hat of "indentation". That means we cannot use the same inner template for different data. If we want to do that, we have to construct another processor for that exact data, which defeats the purpose of using one processor for different threads of different data.
So it's a matter of choosing either performance with less features or usability with slightly worse performance.

[HSGamer]

I think we can remove the use of inner template and just support multi-line string directly.

[HSGamer]

I propose a simpler processor in #32

[byronka]

The problem with the current code is that when we can use the same processor for different list of map data, we can't do that in inner template. Inner template is treated the same as Static Data, with a fancy hat of "indentation".

I'm not sure I understand this - could you provide a simple test to demonstrate what you mean?

[HSGamer]

The problem with the current code is that when we can use the same processor for different list of map data, we can't do that in inner template. Inner template is treated the same as Static Data, with a fancy hat of "indentation".

I'm not sure I understand this - could you provide a simple test to demonstrate what you mean?

In short, this test would fail

    @Test
    public void test_Template_Multi_Thread_WithInner() {
        TemplateProcessor templateProcessor = buildProcessor("Hello {{thread}}");
        TemplateProcessor innerTemplate = buildProcessor("Thread #{{name}}");
        templateProcessor.registerInnerTemplate("thread", innerTemplate);

        int threadCount = 10;
        var futures = new ArrayList<CompletableFuture<String>>();

        // Create multiple concurrent render tasks with different data
        for (int i = 0; i < threadCount; i++) {
            final int threadNum = i;
            var future = CompletableFuture.supplyAsync(() -> {
                templateProcessor.getInnerTemplate("thread").registerData(List.of(Map.of("name", Integer.toString(threadNum))));
                return templateProcessor.renderTemplate();
            });
            futures.add(future);
        }

        // Verify all threads completed successfully with correct results
        for (int i = 0; i < threadCount; i++) {
            String result = futures.get(i).join();
            assertEquals(result, "Hello Thread #" + i);
        }
    }

The test is a modification of test_Template_Multi_Thread_WithInner with the use of Inner Template (the same use as test_Templating_LargeComplex_Performance).

The point is that we can only do that if we want to change the data in the inner template, but since the registerData method assigns the data to the field of the inner template, making it similar to #28 (a typical case of race condition where multiple threads modify the same field).

But that brings us to another question: Since the inner template always use its field value defaultDataList, is it treated the same as STATIC_TEXT?
If we want to change the value of the inner template, we have to use registerData and change its defaultDataList. It's like we change the value before the template construction, so it sounds like STATIC_TEXT.

Changing the test to be like this would make it passed

    @Test
    public void test_Template_Multi_Thread_WithInner() {
        TemplateProcessor templateProcessor = buildProcessor("Hello {{thread}}");
        TemplateProcessor innerTemplate = buildProcessor("Thread #{{name}}");

        int threadCount = 10;
        var futures = new ArrayList<CompletableFuture<String>>();

        // Create multiple concurrent render tasks with different data
        for (int i = 0; i < threadCount; i++) {
            final int threadNum = i;
            var future = CompletableFuture.supplyAsync(() -> {
                String thread = innerTemplate.renderTemplate(Map.of("name", Integer.toString(threadNum)));
                return templateProcessor.renderTemplate(Map.of("thread", thread));
            });
            futures.add(future);
        }

        // Verify all threads completed successfully with correct results
        for (int i = 0; i < threadCount; i++) {
            String result = futures.get(i).join();
            assertEquals(result, "Hello Thread #" + i);
        }
    }

But then at this point I don't see the reason to use inner template.

The possible case I can think of is to create a common template and assign it as inner template to multiple templates. Like making a HTML <head> template with a title key and registering it with different title values to several HTML page template.

TemplateProcessor headerTemplate = buildProcessor("<title>{{title}}</title>");

TemplateProcessor homeTemplate = buildProcessor("...");
homeTemplate.registerInnerTemplate("header", headerTemplate);
homeTemplate.getInnerTemplate("header").registerData(List.of(Map.of("title", "Homepage")));

TemplateProcessor adminTemplate = buildProcessor("...");
adminTemplate.registerInnerTemplate("header", headerTemplate);
adminTemplate.getInnerTemplate("header").registerData(List.of(Map.of("title", "Admin Page")));

But it's a bit overkill, and the processor would waste time rendering the same headerTemplate everytime renderTemplate is called. Since we want to reduce the moving parts and move them out of renderTemplate, calling the same headerTemplate with its defaultDataList as the INNER_TEMPLATE part of renderTemplate doesn't seem like a good practice.

The simpler way is just doing a string replacement before constructing the page processors:

TemplateProcessor headerTemplate = buildProcessor("<title>{{title}}</title>");

TemplateProcessor homeTemplate = buildProcessor("...".replace("{{title}}", headerTemplate.renderTemplate(Map.of("title", "Homepage"))));

TemplateProcessor adminTemplate = buildProcessor("...".replace("{{title}}", headerTemplate.renderTemplate(Map.of("title", "Admin Page"))));

This way we only call headerTemplate when we construct homeTemplate and adminTemplate, and the string headerTemplate renders would be treated as one of the Static text in homeTemplate and adminTemplate.

Based on that, I suggest we consider the possibility of removing the use of Inner Template. I made an example in #32.

@byronka