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

Add validation mode #264

Merged
merged 3 commits into from
Jan 7, 2019
Merged

Add validation mode #264

merged 3 commits into from
Jan 7, 2019

Conversation

boulter
Copy link
Contributor

@boulter boulter commented Jan 2, 2019

Currently jinjava only parses expressions inside active code paths. This is efficient, but also does not look at expressions that might have syntax errors.

This PR adds a validation mode to the interpreter that parses inactive code blocks for syntax, but does not set any variables nor actually execute any macros, functions or filters.

This allows full syntax checking of templates by calling setValidationMode(true) when building the context.

@boulter boulter requested review from hs-lsong and mattcoley January 2, 2019 21:19
@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #264 into master will increase coverage by 0.19%.
The diff coverage is 82.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #264      +/-   ##
============================================
+ Coverage     72.05%   72.25%   +0.19%     
- Complexity     1453     1468      +15     
============================================
  Files           231      231              
  Lines          4534     4563      +29     
  Branches        718      731      +13     
============================================
+ Hits           3267     3297      +30     
+ Misses         1014     1009       -5     
- Partials        253      257       +4
Impacted Files Coverage Δ Complexity Δ
...c/main/java/com/hubspot/jinjava/JinjavaConfig.java 82.85% <ø> (ø) 17 <0> (ø) ⬇️
.../hubspot/jinjava/el/ext/JinjavaBeanELResolver.java 74.35% <ø> (ø) 10 <0> (ø) ⬇️
...main/java/com/hubspot/jinjava/lib/tag/CallTag.java 80% <0%> (-10%) 4 <0> (ø)
.../main/java/com/hubspot/jinjava/lib/tag/RawTag.java 52.63% <0%> (-6.2%) 6 <2> (ø)
...in/java/com/hubspot/jinjava/lib/tag/UnlessTag.java 66.66% <0%> (ø) 5 <0> (ø) ⬇️
...om/hubspot/jinjava/lib/exptest/IsUpperExpTest.java 40% <0%> (ø) 2 <0> (ø) ⬇️
...ain/java/com/hubspot/jinjava/lib/tag/PrintTag.java 60% <0%> (-15%) 3 <0> (ø)
.../hubspot/jinjava/interpret/JinjavaInterpreter.java 78.69% <100%> (+1.12%) 51 <2> (+3) ⬆️
...ain/java/com/hubspot/jinjava/lib/tag/MacroTag.java 87.5% <100%> (ø) 9 <1> (ø) ⬇️
...c/main/java/com/hubspot/jinjava/lib/tag/IfTag.java 100% <100%> (+3.44%) 18 <0> (+1) ⬆️
... and 14 more

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 43ccadb...776075a. Read the comment docs.

@mattcoley
Copy link
Collaborator

Only issue I can think of is if a later expression becomes invalid due to not evaluating a macro or set tag.

@boulter
Copy link
Contributor Author

boulter commented Jan 2, 2019

Only issue I can think of is if a later expression becomes invalid due to not evaluating a macro or set tag.

Any example you can think of? This only checks the syntax, not the expression values in validation mode.

@mattcoley
Copy link
Collaborator

Yea just realized this was for syntax only which should be safe.

}

protected boolean evaluateIfElseTagNode(TagNode tagNode, JinjavaInterpreter interpreter) {
if (tagNode.getName().equals(ElseTag.ELSE)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this checking hurting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's necessary anymore. isPositiveIfElseNode is only called once at the top of the if statement and again when an elseif is encoutered.

return Objects.toString(interpreter.resolveELExpression(tagNode.getHelpers(), tagNode.getLineNumber()), "");
public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) {
String result = Objects.toString(interpreter.resolveELExpression(tagNode.getHelpers(), tagNode.getLineNumber()), "");
return interpreter.getContext().isValidationMode() ? result : "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the PrintTag for ValidationMode only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it's a little-known tag.

{% print "hello" %}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems you return result only when isValidationMode()=true. Or, I am missing something.

@hs-lsong
Copy link
Collaborator

hs-lsong commented Jan 2, 2019

Will ForTag be treated differently?

@boulter
Copy link
Contributor Author

boulter commented Jan 2, 2019

Will ForTag be treated differently?

You're right. I need to do this for for.

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.

5 participants