Skip to content
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

Compiled templates / Detection of recursion in partials is too restrictive #110

Open
pysco68 opened this issue Feb 17, 2016 · 8 comments
Open

Comments

@pysco68
Copy link

pysco68 commented Feb 17, 2016

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.

@Romanx
Copy link
Collaborator

Romanx commented Feb 18, 2016

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,

@pysco68
Copy link
Author

pysco68 commented Feb 18, 2016

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.

@Romanx
Copy link
Collaborator

Romanx commented Feb 18, 2016

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

@pysco68
Copy link
Author

pysco68 commented Feb 18, 2016

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
Copy link
Author

pysco68 commented Feb 18, 2016

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

@Romanx
Copy link
Collaborator

Romanx commented Feb 18, 2016

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

@pysco68
Copy link
Author

pysco68 commented Feb 23, 2016

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

Thanks

@Romanx
Copy link
Collaborator

Romanx commented Mar 15, 2016

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

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

No branches or pull requests

2 participants