guigrpa / docx-templates

Template-based docx report creation
MIT License
882 stars 145 forks source link

Shift + Enter between two IF conditions #340

Closed matcobat closed 5 months ago

matcobat commented 9 months ago

Hi,

First, I would like to say that I really love docx-templates ! I recently found a bug, that makes the "createReport" method looping in an infinite way (so, my application do not answer anymore :().

I am actually using the NodeJS version.

Here is an example of a template that cause the bug. image

If I replace the "Shift Enter" by a simple "Enter" after the first IF, it is working fine. On my application, since I let users to create their own templates, I can not check all the templates. Since this is completely crashing my app, it is very annoying.

if it could work, of throw an error, it would be fantastic !

Thanks !

KingofGnome commented 9 months ago

On my application, since I let users to create their own templates, I can not check all the templates.

Similar in my case, was fighting endless loops aswell. I think in the end I've monkeypatched an loop-counter in there, to at least stop the loop and have a proper error handling.

Let me check tomorrow on my workaround, mby it helps you. Would be awesome to get rid of this bug so.

KingofGnome commented 9 months ago

Possibly related to #154? (IF's are only for loops with 0 or 1 iteration)

Hi all, I encountered the same issue, with two IF on the same line (version 4.11.3). Here is a template that blocks my program:

+++ IF variable1 +++ +++ IF variable2 +++
Hello
+++ END-IF +++
+++ END-IF +++
matcobat commented 9 months ago

Hi @KingofGnome, You're right, it is certainly the same problem.

dseiler commented 7 months ago

@matcobat @KingofGnome I was also able to reproduce this issue. Looks like this is a big problem for us now because we cannot guarantee that our users are not creating such a corrupt template. @jjhbw could we treat this issue with priority? I could also help to resolve. Any idea where I should start? Thanks a lot for your support!!

jjhbw commented 7 months ago

I don't know of an immediate solution to this. Because the result of this bug is an infinite loop, the most likely culprit is the walkTemplate function (because of its while loop) https://github.com/guigrpa/docx-templates/blob/ac067bd6219f95471c3d20c93976c78090fa6ef4/src/processTemplate.ts#L170

This function is very much 'core' to the application and hence challenging to change without breaking other behaviour. If you want to give this a go, we have a pretty solid suite of snapshot tests to use as guardrails.

I would start by creating a snapshot test for your template that triggers the infinite loop (using a Jest timeout or by instrumenting runJS to make it fail after e.g. 1000 executions) and then developing against that until the test case is green. I fixed a similar infinite loop earlier https://github.com/guigrpa/docx-templates/issues/213, but that didn't require changes to walkTemplate.

dseiler commented 7 months ago

Thanks for the hint. I did a first debugging and found that the while(true) loop at line 183 in the processTemplate.ts file is never exiting. There is only one "break" statement in this loop at line 217 which could exit this loop. This statement is never true so we have an infinite loop.

processTemplate.ts , line 215-221 } else { const parent = nodeIn._parent; if (parent == null) break; nodeIn = parent; ctx.level -= 1; move = 'UP'; } So, it looks like the nodeIn._parent is never null if the two IF statements in the template are on the same line. Any idea why this could happen and how to fix? Thanks!

KingofGnome commented 7 months ago

Thanks for the hint. I did a first debugging and found that the while(true) loop at line 183 in the processTemplate.ts file is never exiting. There is only one "break" statement in this loop at line 217 which could exit this loop. This statement is never true so we have an infinite loop.

processTemplate.ts , line 215-221 } else { const parent = nodeIn._parent; if (parent == null) break; nodeIn = parent; ctx.level -= 1; move = 'UP'; } So, it looks like the nodeIn._parent is never null if the two IF statements in the template are on the same line. Any idea why this could happen and how to fix? Thanks!

Mby try to unzip two similar docx files and compare both xml's. Never had the time to figure out the difference between return and shift return in dicx xml struct.

dseiler commented 6 months ago

I did some in depth debugging on this issue and I found that two nested IF commands within the same paragraph (shift enter does not create a new paragraph) is causing the walkTemplate function on the processTemplate.ts to jump to the wrong node and therefore enter in an endless loop. After deep analysis I came to the conclusion that this issue probably cannot be solved because when two IF statements are following each other within the same w:p or w:tr tag the current logic has now way to find out that the first END-IF statement actually belongs to the second nested IF and not to the first IF as usual. I therefore think the "fix" for this issue is to validate the template so that such a nested IF will not be allowed within the same w:p or w:tr tags. @jjhbw I enhanced the code to throw an exception if such a nested IF is found in the template. How could I send my suggestion to you? Can I do a pull request? I created also a jest test for this issue and all tests are green.

jjhbw commented 6 months ago

Pull requests are welcome and very much appreciated, especially if they come with tests!

jjhbw commented 5 months ago

Should be fixed thanks to @dseiler #353