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

Detect fromTag cycle. #221

Merged
merged 2 commits into from
Aug 16, 2018
Merged

Detect fromTag cycle. #221

merged 2 commits into from
Aug 16, 2018

Conversation

hs-lsong
Copy link
Collaborator

Recursive fromTag will cause stack overflow.

@hs-lsong hs-lsong requested a review from mattcoley August 16, 2018 16:36
Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

Quick question.

public void pushFromStack(String path, int lineNumber, int startPosition) {
fromStack.push(path, lineNumber, startPosition);
}
public void popFromStack() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing newline.

@@ -54,6 +56,13 @@ public void importedContextExposesVars() {
.contains("wrap-padding: padding-left:42px;padding-right:42px");
}

@Test
public void importedCycleDected() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for A to import from B which imports from C which imports from A? If so can we have a test to make sure it catches this cycle as well?

@codecov-io
Copy link

Codecov Report

Merging #221 into master will increase coverage by 0.16%.
The diff coverage is 89.13%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #221      +/-   ##
============================================
+ Coverage     71.28%   71.44%   +0.16%     
- Complexity     1365     1373       +8     
============================================
  Files           217      218       +1     
  Lines          4315     4336      +21     
  Branches        687      689       +2     
============================================
+ Hits           3076     3098      +22     
  Misses          998      998              
+ Partials        241      240       -1
Impacted Files Coverage Δ Complexity Δ
...bspot/jinjava/interpret/FromTagCycleException.java 100% <100%> (ø) 1 <1> (?)
...in/java/com/hubspot/jinjava/interpret/Context.java 77.38% <100%> (+0.83%) 77 <3> (+4) ⬆️
...pret/errorcategory/BasicTemplateErrorCategory.java 100% <100%> (ø) 1 <0> (ø) ⬇️
.../hubspot/jinjava/interpret/JinjavaInterpreter.java 78.48% <50%> (-0.5%) 47 <1> (ø)
...m/hubspot/jinjava/interpret/TagCycleException.java 70% <50%> (+3.33%) 6 <0> (+1) ⬆️
...main/java/com/hubspot/jinjava/lib/tag/FromTag.java 88.09% <90.9%> (+6.27%) 9 <0> (+1) ⬆️
...va/com/hubspot/jinjava/el/MacroFunctionMapper.java 100% <0%> (+5.88%) 8% <0%> (+1%) ⬆️

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 0d63899...5b0af3a. Read the comment docs.

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

LGTM

@hs-lsong hs-lsong merged commit c0ab6c4 into master Aug 16, 2018
@hs-lsong hs-lsong deleted the from-cycle branch August 16, 2018 17:54
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.

3 participants