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

Allow ability to set a max recursion depth in config. #308

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

mattcoley
Copy link
Collaborator

This is a middle ground option for #91. You can now set enableRecursiveMacroCalls to true and maxMacroRecursionDepth to a reasonable value to prevent a template from killing your thread due to stack overflows.

@mattcoley mattcoley requested a review from boulter March 12, 2019 19:58
@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #308 into master will decrease coverage by 0.9%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #308      +/-   ##
============================================
- Coverage     71.58%   70.67%   -0.91%     
- Complexity     1553     1559       +6     
============================================
  Files           239      239              
  Lines          4863     4945      +82     
  Branches        787      801      +14     
============================================
+ Hits           3481     3495      +14     
- Misses         1097     1165      +68     
  Partials        285      285
Impacted Files Coverage Δ Complexity Δ
...a/com/hubspot/jinjava/el/ext/AstMacroFunction.java 86.66% <100%> (+2.66%) 8 <0> (+2) ⬆️
...c/main/java/com/hubspot/jinjava/JinjavaConfig.java 88.09% <100%> (+0.59%) 20 <3> (+1) ⬆️
.../java/com/hubspot/jinjava/interpret/CallStack.java 100% <100%> (ø) 13 <2> (+3) ⬆️
...ava/com/hubspot/jinjava/doc/JinjavaDocFactory.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../java/com/hubspot/jinjava/doc/JinjavaDocParam.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...n/java/com/hubspot/jinjava/doc/JinjavaDocItem.java 0% <0%> (ø) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1d2ee0...f5e2351. Read the comment docs.

@mattcoley
Copy link
Collaborator Author

mattcoley commented Mar 12, 2019

The need for this is to be able to build nesting structures like site menus without resolving to copying and pasting a macro n times to support a menu with depth n. It is still dangerous to allow infinite recursion so we can set this max value to something reasonable like 10-20.

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, what should a reasonable limit be? :)

} else {
macroStack.push(getName(), -1, -1);
}
} catch (MacroTagCycleException e) {

int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth();
String message;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: tertiary statement

}

public JinjavaConfig(Charset charset, Locale locale, ZoneId timeZone, int maxRenderDepth) {
this(charset, locale, timeZone, maxRenderDepth, new HashMap<>(), false, false, true, false, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
this(charset, locale, timeZone, maxRenderDepth, new HashMap<>(), false, false, true, false, 0, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nigh time for a builder here.

@mattcoley
Copy link
Collaborator Author

Java 8 stack frame size in the worst case is going to be about 1kb so I was thinking 50 as a limit.

@boulter
Copy link
Contributor

boulter commented Mar 13, 2019

I think 10 or 20 would be more reasonable since you could possibly interleave other functions recursively.

@mattcoley
Copy link
Collaborator Author

I think 20 would be good. This is total macro stack size but I think it is unlikely that someone currently has a template that calls 20 nested macros.

@mattcoley mattcoley merged commit a67a861 into master Mar 13, 2019
@mattcoley mattcoley deleted the max-recursion-depth branch March 13, 2019 14:44
@TheWebTech
Copy link

I think 20 would be good. This is total macro stack size but I think it is unlikely that someone currently has a template that calls 20 nested macros.

especially on HubSpot, where nesting macros previously didn't actually work.

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

Successfully merging this pull request may close these issues.

4 participants