Nustache icon indicating copy to clipboard operation
Nustache copied to clipboard

Compiled templates / Detection of recursion in partials is too restrictive

Open pysco68 opened this issue 9 years ago • 8 comments

Hello,

I was struggling while trying to use a partial more than once within the same compiled template. The compiler raises the exception "Unsupported: Compiled recursive templates will be supported in a later release" as it thinks (erroneously) that there is a recursion in the partial.

Here's a test case that raises the error:

[Test]
public void Partials_Multiple_Usage()
{
    var template = Template("{{>text}} after partial {{>text}}");
    var compiled = template.Compile<TestObject>(name =>
        name == "text" ? Template("{{TestString}} {{#Sub.TestBool}}I am in you{{/Sub.TestBool}}") : null);
    var result = compiled(new TestObject { TestString = "a", Sub = new SubObject { TestBool = true } });
    Assert.AreEqual("a I am in you after partial a I am in you", result);
}

IMO. the problem is that the recusion check in CompileContext.cs (line 198-199) is not really checking for recursion, rather for multiple inclusion. An those are legal in Mustache as far as I know.

I'll try to provide a true recursion check in the next days if you'd like.

pysco68 avatar Feb 17 '16 13:02 pysco68

Hi there, there's currently no active development on Nustache as i'm working on a competing side project. However if you can provide a reproduction case I can take a quick look to see if it's an easy fix, alternatively i'd be willing to accept a pull request with a fix.

Thanks,

Romanx avatar Feb 18 '16 08:02 Romanx

I'm investigating this right now. I didn't fully figure out what your motivation was when you added the "recusion test" in CompileContext.cs (line 198-199) as I actually didn't break any test case when simply commented it out.

pysco68 avatar Feb 18 '16 09:02 pysco68

That was actually added by the original creator of Nustache jdiamond, I'm just a maintainer so unfortunately I've no idea either

Romanx avatar Feb 18 '16 09:02 Romanx

Well I went through "all of it" and my conclusion is that a true recursion check is really not feasible in a clean way in the architecture of Nustache (we don't know anything about the context in the case of the includes) but as the spec of mustache isn't really foreseeing recursions in templates in any way there's no point in having anything relse than the call depth check that's implemented a few lines above.

What do you think about it? I mean could we possibly remove that (harmful) check? I added these two test cases;

        [Test]
        public void Partials_Multiple_Usage()
        {
            var template = Template("{{>text}} after partial {{>text}}");
            var compiled = template.Compile<TestObject>(name =>
                name == "text" ? Template("{{TestString}} {{#Sub.TestBool}}I am in you{{/Sub.TestBool}}") : null);
            var result = compiled(new TestObject { TestString = "a", Sub = new SubObject { TestBool = true } });
            Assert.AreEqual("a I am in you after partial a I am in you", result);
        }

        [Test]
        public void Partials_Recursion_Not_Supported()
        {
            Assert.That(() =>
            {
                var template = Template(@"{{<recursive_partial}}{{#Children}}<li>{{Name}}<ul>{{>recursive_partial}}</ul></li>{{/Children}}{{/recursive_partial}}{{>recursive_partial}}");
                var compiled = template.Compile<TreeNode>(null);
            },
            Throws.Exception.InstanceOf<NustacheException>().With.Message.StartsWith("You have reached the include limit of"));
        }

To make sure we get no unexpected behavior changes otherwise

If you agree to that I'll make the PR

pysco68 avatar Feb 18 '16 09:02 pysco68

Note: look at my fork for my proposal: https://github.com/pysco68/Nustache/tree/partials-recursion

pysco68 avatar Feb 18 '16 10:02 pysco68

Hey there,

I took a quick look and all seems good. I agree the original error wasn't that useful and raising an error based on depth is probably a better idea. Feel free to open a pull

Romanx avatar Feb 18 '16 15:02 Romanx

Hello, I opened the pull request however the AppVeyor check failed for some not understandable reason. Could you have a look at that?

Thanks

pysco68 avatar Feb 23 '16 14:02 pysco68

Hi there,

I've taken a look at this (finally) sorry about the delay. It seems that we're getting a stack overflow from the tests when running on .net 3.5

Would you mind taking a look and seeing if you can reproduce on your machine? I re-ran the appveyor build in-case of intermittent build failure but seemed to happen again.

Thanks

Romanx avatar Mar 15 '16 11:03 Romanx