From 362c19f21158d3d7d4b502d903e29f1fa44adc58 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 30 Jun 2021 12:57:41 -0400 Subject: [PATCH 1/3] Better handle spaces within for loop expressions --- .../com/hubspot/jinjava/lib/tag/ForTag.java | 48 ++++++------------- .../jinjava/lib/tag/eager/EagerForTag.java | 11 +++-- .../hubspot/jinjava/lib/tag/ForTagTest.java | 9 ++++ 3 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java index 011cfa7a1..7f6b565a5 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java @@ -43,7 +43,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; -import org.apache.commons.lang3.StringUtils; +import java.util.Optional; /** * {% for a in b|f1:d,c %} @@ -126,12 +126,13 @@ public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) { } public String interpretUnchecked(TagNode tagNode, JinjavaInterpreter interpreter) { - String helpers = getWhitespaceAdjustedHelpers(tagNode.getHelpers()); - List helper = new HelperStringTokenizer(helpers).splitComma(true).allTokens(); + List helperTokens = new HelperStringTokenizer(tagNode.getHelpers()) + .splitComma(true) + .allTokens(); + List loopVars = getLoopVars(helperTokens); + Optional maybeLoopExpr = getLoopExpression(tagNode.getHelpers()); - List loopVars = getLoopVars(helper); - - if (loopVars.size() >= helper.size()) { + if (loopVars.size() >= helperTokens.size() || !maybeLoopExpr.isPresent()) { throw new TemplateSyntaxException( tagNode.getHelpers().trim(), "Tag 'for' expects valid 'in' clause, got: " + tagNode.getHelpers(), @@ -139,11 +140,12 @@ public String interpretUnchecked(TagNode tagNode, JinjavaInterpreter interpreter tagNode.getStartPosition() ); } + String loopExpression = maybeLoopExpr.get(); - String loopExpr = getLoopExpression(helper, loopVars); Object collection; try { - collection = interpreter.resolveELExpression(loopExpr, tagNode.getLineNumber()); + collection = + interpreter.resolveELExpression(loopExpression, tagNode.getLineNumber()); } catch (DeferredParsingException e) { throw new DeferredParsingException( String.format("%s in %s", String.join(", ", loopVars), e.getDeferredEvalResult()) @@ -233,32 +235,12 @@ public String interpretUnchecked(TagNode tagNode, JinjavaInterpreter interpreter } } - public static String getWhitespaceAdjustedHelpers(String helpers) { - /* apdlv72@gmail.com - * Fix for issues with for-loops that contain whitespace in their range, e.g. - * "{% for i in range(1 * 1, 2 * 2) %}" - * This is because HelperStringTokenizer will split the range expressions also - * at white spaces and end up with [i, in, range(1, *, 1, 2, *, 2)]. - * To avoid this, the below fix will remove white space from the expression - * on the right side of the keyword "in". It will do so however only if there - * are no characters in this expression that indicate strings - namely ' and ". - * This avoids messing up expressions like {% for i in ['a ','b'] %} that - * contain spaces in the arguments. - * TODO A somewhat more sophisticated tokenizing/parsing of the for-loop expression. - */ - String[] parts = helpers.split("\\s+in\\s+"); - if (parts.length == 2 && !parts[1].contains("'") && !parts[1].contains("\"")) { - helpers = parts[0] + " in " + parts[1].replace(" ", ""); + public Optional getLoopExpression(String helpers) { + int inIndex = helpers.indexOf(" in "); + if (inIndex > 0) { + return Optional.of(helpers.substring(inIndex + 4).trim()); } - return helpers; - } - - public String getLoopExpression(List helper, List loopVars) { - String loopExpr = StringUtils.join( - helper.subList(loopVars.size() + 1, helper.size()), - "," - ); - return loopExpr; + return Optional.empty(); } public List getLoopVars(List helper) { diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java index 9f8efc7c2..eb3afbfc4 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java @@ -15,6 +15,7 @@ import com.hubspot.jinjava.util.LengthLimitingStringJoiner; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.stream.Collectors; public class EagerForTag extends EagerTagDecorator { @@ -82,13 +83,13 @@ public String eagerInterpret( @Override public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter) { - List helperTokens = new HelperStringTokenizer( - ForTag.getWhitespaceAdjustedHelpers(tagToken.getHelpers()) - ) + List helperTokens = new HelperStringTokenizer(tagToken.getHelpers()) .splitComma(true) .allTokens(); List loopVars = getTag().getLoopVars(helperTokens); - if (loopVars.size() >= helperTokens.size()) { + Optional maybeLoopExpr = getTag().getLoopExpression(tagToken.getHelpers()); + + if (loopVars.size() >= helperTokens.size() || !maybeLoopExpr.isPresent()) { throw new TemplateSyntaxException( tagToken.getHelpers().trim(), "Tag 'for' expects valid 'in' clause, got: " + tagToken.getHelpers(), @@ -97,7 +98,7 @@ public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter ); } - String loopExpression = getTag().getLoopExpression(helperTokens, loopVars); + String loopExpression = maybeLoopExpr.get(); EagerExpressionResult eagerExpressionResult = EagerExpressionResolver.resolveExpression( loopExpression, diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java index fdc32e6db..4d9963d38 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java @@ -279,6 +279,15 @@ public void forLoopShouldCountUsingNamespaceVariable() { assertThat(rendered).isEqualTo("Found item having something: 4"); } + @Test + public void itShouldHandleSpaces() { + String template = + "{% for item in ['foo', 'bar'] %}" + "{{ item }}\n" + "{% endfor %}"; + + String rendered = jinjava.render(template, context); + assertThat(rendered).isEqualTo("foo\nbar\n"); + } + private Node fixture(String name) { try { return new TreeParser( From 21ed8c362a980162ac331fecba5ef9416a9f6ce3 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 30 Jun 2021 13:04:24 -0400 Subject: [PATCH 2/3] Refactor shared code to method --- .../com/hubspot/jinjava/lib/tag/ForTag.java | 44 ++++++++++++------- .../jinjava/lib/tag/eager/EagerForTag.java | 24 +++------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java index 7f6b565a5..a65873a22 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/ForTag.java @@ -34,6 +34,7 @@ import com.hubspot.jinjava.tree.ExpressionNode; import com.hubspot.jinjava.tree.Node; import com.hubspot.jinjava.tree.TagNode; +import com.hubspot.jinjava.tree.parse.TagToken; import com.hubspot.jinjava.util.ForLoop; import com.hubspot.jinjava.util.HelperStringTokenizer; import com.hubspot.jinjava.util.LengthLimitingStringBuilder; @@ -44,6 +45,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Optional; +import org.apache.commons.lang3.tuple.Pair; /** * {% for a in b|f1:d,c %} @@ -126,21 +128,11 @@ public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) { } public String interpretUnchecked(TagNode tagNode, JinjavaInterpreter interpreter) { - List helperTokens = new HelperStringTokenizer(tagNode.getHelpers()) - .splitComma(true) - .allTokens(); - List loopVars = getLoopVars(helperTokens); - Optional maybeLoopExpr = getLoopExpression(tagNode.getHelpers()); - - if (loopVars.size() >= helperTokens.size() || !maybeLoopExpr.isPresent()) { - throw new TemplateSyntaxException( - tagNode.getHelpers().trim(), - "Tag 'for' expects valid 'in' clause, got: " + tagNode.getHelpers(), - tagNode.getLineNumber(), - tagNode.getStartPosition() - ); - } - String loopExpression = maybeLoopExpr.get(); + Pair, String> loopVarsAndExpression = getLoopVarsAndExpression( + (TagToken) tagNode.getMaster() + ); + List loopVars = loopVarsAndExpression.getLeft(); + String loopExpression = loopVarsAndExpression.getRight(); Object collection; try { @@ -235,7 +227,25 @@ public String interpretUnchecked(TagNode tagNode, JinjavaInterpreter interpreter } } - public Optional getLoopExpression(String helpers) { + public Pair, String> getLoopVarsAndExpression(TagToken tagToken) { + List helperTokens = new HelperStringTokenizer(tagToken.getHelpers()) + .splitComma(true) + .allTokens(); + List loopVars = getLoopVars(helperTokens); + Optional maybeLoopExpr = getLoopExpression(tagToken.getHelpers()); + + if (loopVars.size() >= helperTokens.size() || !maybeLoopExpr.isPresent()) { + throw new TemplateSyntaxException( + tagToken.getHelpers().trim(), + "Tag 'for' expects valid 'in' clause, got: " + tagToken.getHelpers(), + tagToken.getLineNumber(), + tagToken.getStartPosition() + ); + } + return Pair.of(loopVars, maybeLoopExpr.get()); + } + + private Optional getLoopExpression(String helpers) { int inIndex = helpers.indexOf(" in "); if (inIndex > 0) { return Optional.of(helpers.substring(inIndex + 4).trim()); @@ -243,7 +253,7 @@ public Optional getLoopExpression(String helpers) { return Optional.empty(); } - public List getLoopVars(List helper) { + private List getLoopVars(List helper) { List loopVars = Lists.newArrayList(); while (loopVars.size() < helper.size()) { String val = helper.get(loopVars.size()); diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java index eb3afbfc4..e0914fd48 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerForTag.java @@ -4,19 +4,17 @@ import com.hubspot.jinjava.interpret.DeferredValueException; import com.hubspot.jinjava.interpret.InterpretException; import com.hubspot.jinjava.interpret.JinjavaInterpreter; -import com.hubspot.jinjava.interpret.TemplateSyntaxException; import com.hubspot.jinjava.lib.tag.ForTag; import com.hubspot.jinjava.tree.TagNode; import com.hubspot.jinjava.tree.parse.TagToken; import com.hubspot.jinjava.util.EagerExpressionResolver; import com.hubspot.jinjava.util.EagerExpressionResolver.EagerExpressionResult; -import com.hubspot.jinjava.util.HelperStringTokenizer; import com.hubspot.jinjava.util.LengthLimitingStringBuilder; import com.hubspot.jinjava.util.LengthLimitingStringJoiner; import java.util.HashSet; import java.util.List; -import java.util.Optional; import java.util.stream.Collectors; +import org.apache.commons.lang3.tuple.Pair; public class EagerForTag extends EagerTagDecorator { @@ -83,22 +81,10 @@ public String eagerInterpret( @Override public String getEagerTagImage(TagToken tagToken, JinjavaInterpreter interpreter) { - List helperTokens = new HelperStringTokenizer(tagToken.getHelpers()) - .splitComma(true) - .allTokens(); - List loopVars = getTag().getLoopVars(helperTokens); - Optional maybeLoopExpr = getTag().getLoopExpression(tagToken.getHelpers()); - - if (loopVars.size() >= helperTokens.size() || !maybeLoopExpr.isPresent()) { - throw new TemplateSyntaxException( - tagToken.getHelpers().trim(), - "Tag 'for' expects valid 'in' clause, got: " + tagToken.getHelpers(), - tagToken.getLineNumber(), - tagToken.getStartPosition() - ); - } - - String loopExpression = maybeLoopExpr.get(); + Pair, String> loopVarsAndExpression = getTag() + .getLoopVarsAndExpression(tagToken); + List loopVars = loopVarsAndExpression.getLeft(); + String loopExpression = loopVarsAndExpression.getRight(); EagerExpressionResult eagerExpressionResult = EagerExpressionResolver.resolveExpression( loopExpression, From 11a7b513830d9f0d7d516eb394845631b25acef4 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 30 Jun 2021 13:17:21 -0400 Subject: [PATCH 3/3] Add test for looping through maps with spaces --- src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java index 4d9963d38..33b430762 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/ForTagTest.java @@ -280,12 +280,14 @@ public void forLoopShouldCountUsingNamespaceVariable() { } @Test - public void itShouldHandleSpaces() { + public void itShouldHandleSpacesInMaps() { String template = - "{% for item in ['foo', 'bar'] %}" + "{{ item }}\n" + "{% endfor %}"; + "{% for item in [{'key': 'foo?'}, {'key': 'bar?'}] %}" + + "{{ item.key }}\n" + + "{% endfor %}"; String rendered = jinjava.render(template, context); - assertThat(rendered).isEqualTo("foo\nbar\n"); + assertThat(rendered).isEqualTo("foo?\nbar?\n"); } private Node fixture(String name) {