TagCycleException was thrown when rendering template that doesn't have any cycles#445
Conversation
|
It seems like every PR has that build problem, so I suppose there's nothing to fix in the request itself. |
|
@mattcoley Please take a look |
|
@boulter Could you please check this out? |
|
Thanks for the contribution. I've been terribly busy, but I'll try to take a look by the end of the week. |
|
It could be better to investigate why this is throwing a tag cycle exception when there isn't a cycle. |
| public void itRunsWithoutTagCycleException() throws Exception { | ||
| String result = jinjava.render( | ||
| Resources.toString( | ||
| Resources.getResource("tags/extendstag/tagcycleexception/main-template.jinja"), |
There was a problem hiding this comment.
I find it a bit easier to reason about these import chains if you rename them to something sequential like template-a template-b,template-c so you can easily track a extends b imports c etc.
There was a problem hiding this comment.
Ok, templates were renamed. To better understanding please check the inheritance tree:
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
Conceptionally it looks good. Joe's comments are good and I added a few more.
| .push(templateFile, interpreter.getLineNumber(), interpreter.getPosition()); | ||
|
|
||
| return interpreter.render(node); | ||
| return interpreter.render(node, false); |
There was a problem hiding this comment.
could you explain a bit more why this is more correct?
There was a problem hiding this comment.
I’ve been trying to check other examples of Cycle Exception being thrown. It was the only combination of tags like “import/export/extends” that reacted like that, ending up with TagCycleException which seems some kind of last resort in the case when common types of exceptions weren’t handled.
In that instance, we have an error because we went to render() method again before we actually pop the node from CurrentPathStack.
Comparing to “import”: there we also try to render the template when we got to the imported part, but unlike “include” we’re creating a new interpreter instance. It’s alright because jinja documentation told us so: “It’s important to know that imports are cached and imported templates don’t have access to the current template variables”
However, we cannot use the same trick with “import”. About this tag jinja has that statement: “Included templates have access to the variables of the active context by default”. So we must use the same interpreter for both the original templates and the imported ones.
Knowing that I found the only way to get rid of that problem: forbid the included template to render whatever we have inside ExtendedRoots. It shouldn’t break anything because eventually we will process them but it’s better to do that outside the imported template.
Sources for the behavior of each tag:
https://jinja.palletsprojects.com/en/2.11.x/templates/#include
https://jinja.palletsprojects.com/en/2.11.x/templates/#import
| @@ -0,0 +1,2 @@ | |||
| {{ variable1 }} | |||
There was a problem hiding this comment.
are these variables relevant to the test?
There was a problem hiding this comment.
No, you're right, we don't really need them here.
Removed them.
| } | ||
|
|
||
| @Test | ||
| public void itRunsWithoutTagCycleException() throws Exception { |
There was a problem hiding this comment.
maybe explain this better in the name. Something about extended files not triggering tag cycle exceptions?
There was a problem hiding this comment.
Sure, renamed to "itAvoidsTagCycleExceptionInsideExtendedFiles"
Roobun
force-pushed
the
tagCycleException
branch
from
da3f60e to
a1ad554
Compare
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
Thanks for the changes. Looks good to me.
|
@boulter if it is alright could you please merge the changes? |
|
Hi @boulter
Do you have something like a schedule for new releases?
If not, could you please share the information when you intend to release
Jinjava 2.5.5?
|
|
@mattcoley are you planning a release anytime soon? |
|
I can do a release either today or tomorrow. |
|
@mattcoley could you please do a release for 2.5.5 version? |
4 participants
Before these changes templates as I have created couldn't be rendered without errors.
Jinjava had mistakenly thrown TagCycleException in such cases.
Adding "false" as a parameter to render() method will reduce harm from that bug to zero.