From 4e96d71b88e21d767eee7b7965b3b5dba6faf70a Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Tue, 31 Oct 2023 11:07:08 -0400 Subject: [PATCH 1/3] Revert "Merge pull request #1124 from HubSpot/legacy-text-merging" This reverts commit 3b99a0a9673a30ebfe685af86b88919d81722e99, reversing changes made to 264ac2ba00bc5612d1374b054122a1f70ff198c1. --- .../com/hubspot/jinjava/LegacyOverrides.java | 26 +----------------- .../com/hubspot/jinjava/tree/TreeParser.java | 13 +-------- .../hubspot/jinjava/tree/parse/TextToken.java | 7 ----- .../hubspot/jinjava/tree/TreeParserTest.java | 27 ------------------- 4 files changed, 2 insertions(+), 71 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/LegacyOverrides.java b/src/main/java/com/hubspot/jinjava/LegacyOverrides.java index 5ac395397..5ff01314f 100644 --- a/src/main/java/com/hubspot/jinjava/LegacyOverrides.java +++ b/src/main/java/com/hubspot/jinjava/LegacyOverrides.java @@ -3,20 +3,9 @@ /** * This class allows Jinjava to be configured to override legacy behaviour. * LegacyOverrides.NONE signifies that none of the legacy functionality will be overridden. - * LegacyOverrides.ALL signifies that all new functionality will be used; avoid legacy "bugs". */ public class LegacyOverrides { public static final LegacyOverrides NONE = new LegacyOverrides.Builder().build(); - public static final LegacyOverrides ALL = new LegacyOverrides.Builder() - .withEvaluateMapKeys(true) - .withIterateOverMapKeys(true) - .withUsePyishObjectMapper(true) - .withUseSnakeCasePropertyNaming(true) - .withWhitespaceRequiredWithinTokens(true) - .withUseNaturalOperatorPrecedence(true) - .withParseWhitespaceControlStrictly(true) - .withAllowAdjacentTextNodes(true) - .build(); private final boolean evaluateMapKeys; private final boolean iterateOverMapKeys; private final boolean usePyishObjectMapper; @@ -24,7 +13,6 @@ public class LegacyOverrides { private final boolean whitespaceRequiredWithinTokens; private final boolean useNaturalOperatorPrecedence; private final boolean parseWhitespaceControlStrictly; - private final boolean allowAdjacentTextNodes; private LegacyOverrides(Builder builder) { evaluateMapKeys = builder.evaluateMapKeys; @@ -34,7 +22,6 @@ private LegacyOverrides(Builder builder) { whitespaceRequiredWithinTokens = builder.whitespaceRequiredWithinTokens; useNaturalOperatorPrecedence = builder.useNaturalOperatorPrecedence; parseWhitespaceControlStrictly = builder.parseWhitespaceControlStrictly; - allowAdjacentTextNodes = builder.allowAdjacentTextNodes; } public static Builder newBuilder() { @@ -69,10 +56,6 @@ public boolean isParseWhitespaceControlStrictly() { return parseWhitespaceControlStrictly; } - public boolean isAllowAdjacentTextNodes() { - return allowAdjacentTextNodes; - } - public static class Builder { private boolean evaluateMapKeys = false; private boolean iterateOverMapKeys = false; @@ -81,7 +64,6 @@ public static class Builder { private boolean whitespaceRequiredWithinTokens = false; private boolean useNaturalOperatorPrecedence = false; private boolean parseWhitespaceControlStrictly = false; - private boolean allowAdjacentTextNodes = false; private Builder() {} @@ -101,8 +83,7 @@ public static Builder from(LegacyOverrides legacyOverrides) { .withUseNaturalOperatorPrecedence(legacyOverrides.useNaturalOperatorPrecedence) .withParseWhitespaceControlStrictly( legacyOverrides.parseWhitespaceControlStrictly - ) - .withAllowAdjacentTextNodes(legacyOverrides.allowAdjacentTextNodes); + ); } public Builder withEvaluateMapKeys(boolean evaluateMapKeys) { @@ -145,10 +126,5 @@ public Builder withParseWhitespaceControlStrictly( this.parseWhitespaceControlStrictly = parseWhitespaceControlStrictly; return this; } - - public Builder withAllowAdjacentTextNodes(boolean allowAdjacentTextNodes) { - this.allowAdjacentTextNodes = allowAdjacentTextNodes; - return this; - } } } diff --git a/src/main/java/com/hubspot/jinjava/tree/TreeParser.java b/src/main/java/com/hubspot/jinjava/tree/TreeParser.java index 409b3db1f..bc6f62557 100644 --- a/src/main/java/com/hubspot/jinjava/tree/TreeParser.java +++ b/src/main/java/com/hubspot/jinjava/tree/TreeParser.java @@ -62,18 +62,7 @@ public Node buildTree() { Node node = nextNode(); if (node != null) { - if ( - node instanceof TextNode && - getLastSibling() instanceof TextNode && - !interpreter.getConfig().getLegacyOverrides().isAllowAdjacentTextNodes() - ) { - // merge adjacent text nodes so whitespace control properly applies - ((TextToken) getLastSibling().getMaster()).mergeImageAndContent( - (TextToken) node.getMaster() - ); - } else { - parent.getChildren().add(node); - } + parent.getChildren().add(node); } } diff --git a/src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java b/src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java index edfbb4d3e..dc3d64b42 100644 --- a/src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java +++ b/src/main/java/com/hubspot/jinjava/tree/parse/TextToken.java @@ -29,13 +29,6 @@ public TextToken( super(image, lineNumber, startPosition, symbols); } - public void mergeImageAndContent(TextToken otherToken) { - String thisOutput = output(); - String otherTokenOutput = otherToken.output(); - this.image = thisOutput + otherTokenOutput; - this.content = image; - } - @Override public int getType() { return getSymbols().getFixed(); diff --git a/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java b/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java index 7a665abc8..b31e82cc3 100644 --- a/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java +++ b/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java @@ -6,7 +6,6 @@ import com.hubspot.jinjava.BaseInterpretingTest; import com.hubspot.jinjava.Jinjava; import com.hubspot.jinjava.JinjavaConfig; -import com.hubspot.jinjava.LegacyOverrides; import com.hubspot.jinjava.interpret.TemplateError.ErrorType; import java.nio.charset.StandardCharsets; import org.junit.Test; @@ -232,13 +231,6 @@ public void itTrimsNotes() { assertThat(interpreter.render(tree)).isEqualTo("AB"); } - @Test - public void itMergesTextNodesWhileRespectingTrim() { - String expression = "{% print 'A' -%}\n{#- note -#}\nB\n{%- print 'C' %}"; - final Node tree = new TreeParser(interpreter, expression).buildTree(); - assertThat(interpreter.render(tree)).isEqualTo("ABC"); - } - @Test public void itTrimsExpressions() { String expression = "A\n{{- 'B' -}}\nC"; @@ -246,25 +238,6 @@ public void itTrimsExpressions() { assertThat(interpreter.render(tree)).isEqualTo("ABC"); } - @Test - public void itDoesNotMergeAdjacentTextNodesWhenLegacyOverrideIsApplied() { - String expression = "A\n{%- if true -%}\n{# comment #}\nB{% endif %}"; - final Node tree = new TreeParser(interpreter, expression).buildTree(); - assertThat(interpreter.render(tree)).isEqualTo("AB"); - interpreter = - new Jinjava( - JinjavaConfig - .newBuilder() - .withLegacyOverrides( - LegacyOverrides.newBuilder().withAllowAdjacentTextNodes(true).build() - ) - .build() - ) - .newInterpreter(); - final Node overriddenTree = new TreeParser(interpreter, expression).buildTree(); - assertThat(interpreter.render(overriddenTree)).isEqualTo("A\nB"); - } - Node parse(String fixture) { try { return new TreeParser( From c0992b76cdefa8385ca9bdcbb155a484ca720715 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Tue, 31 Oct 2023 11:07:20 -0400 Subject: [PATCH 2/3] Revert "Merge pull request #1123 from HubSpot/prevent-accidental-expressions" This reverts commit 264ac2ba00bc5612d1374b054122a1f70ff198c1, reversing changes made to 7a60dca9580a929c7fd515429d5f20094202ac4e. --- .../jinjava/tree/output/OutputList.java | 69 ------------------- .../tree/parse/TokenScannerSymbols.java | 6 -- .../interpret/JinjavaInterpreterTest.java | 53 -------------- 3 files changed, 128 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/tree/output/OutputList.java b/src/main/java/com/hubspot/jinjava/tree/output/OutputList.java index 3ab3ec9f3..738e9062c 100644 --- a/src/main/java/com/hubspot/jinjava/tree/output/OutputList.java +++ b/src/main/java/com/hubspot/jinjava/tree/output/OutputList.java @@ -3,14 +3,11 @@ import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.interpret.OutputTooBigException; import com.hubspot.jinjava.interpret.TemplateError; -import com.hubspot.jinjava.tree.parse.TokenScannerSymbols; import com.hubspot.jinjava.util.LengthLimitingStringBuilder; import java.util.LinkedList; import java.util.List; public class OutputList { - public static final String PREVENT_ACCIDENTAL_EXPRESSIONS = - "PREVENT_ACCIDENTAL_EXPRESSIONS"; private final List nodes = new LinkedList<>(); private final List blocks = new LinkedList<>(); private final long maxOutputSize; @@ -51,72 +48,6 @@ public List getBlocks() { public String getValue() { LengthLimitingStringBuilder val = new LengthLimitingStringBuilder(maxOutputSize); - return JinjavaInterpreter - .getCurrentMaybe() - .map(JinjavaInterpreter::getConfig) - .filter( - config -> - config - .getFeatures() - .getActivationStrategy(PREVENT_ACCIDENTAL_EXPRESSIONS) - .isActive(null) - ) - .map( - config -> joinNodesWithoutAddingExpressions(val, config.getTokenScannerSymbols()) - ) - .orElseGet(() -> joinNodes(val)); - } - - private String joinNodesWithoutAddingExpressions( - LengthLimitingStringBuilder val, - TokenScannerSymbols tokenScannerSymbols - ) { - String separator = getWhitespaceSeparator(tokenScannerSymbols); - String prev = null; - String cur; - for (OutputNode node : nodes) { - try { - cur = node.getValue(); - if ( - prev != null && - prev.length() > 0 && - prev.charAt(prev.length() - 1) == tokenScannerSymbols.getExprStartChar() - ) { - if ( - cur.length() > 0 && - TokenScannerSymbols.isNoteTagOrExprChar(tokenScannerSymbols, cur.charAt(0)) - ) { - val.append(separator); - } - } - prev = cur; - val.append(node.getValue()); - } catch (OutputTooBigException e) { - JinjavaInterpreter - .getCurrent() - .addError(TemplateError.fromOutputTooBigException(e)); - return val.toString(); - } - } - - return val.toString(); - } - - private static String getWhitespaceSeparator(TokenScannerSymbols tokenScannerSymbols) { - @SuppressWarnings("StringBufferReplaceableByString") - String separator = new StringBuilder() - .append('\n') - .append(tokenScannerSymbols.getPrefixChar()) - .append(tokenScannerSymbols.getNoteChar()) - .append(tokenScannerSymbols.getTrimChar()) - .append(' ') - .append(tokenScannerSymbols.getNoteChar()) - .append(tokenScannerSymbols.getExprEndChar()) - .toString(); - return separator; - } - - private String joinNodes(LengthLimitingStringBuilder val) { for (OutputNode node : nodes) { try { val.append(node.getValue()); diff --git a/src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java b/src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java index 5f561d198..ed499556b 100644 --- a/src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java +++ b/src/main/java/com/hubspot/jinjava/tree/parse/TokenScannerSymbols.java @@ -122,10 +122,4 @@ public String getClosingComment() { } return closingComment; } - - public static boolean isNoteTagOrExprChar(TokenScannerSymbols symbols, char c) { - return ( - c == symbols.getNote() || c == symbols.getTag() || c == symbols.getExprStartChar() - ); - } } diff --git a/src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java b/src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java index 16e3d990b..6b6b7733b 100644 --- a/src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java +++ b/src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java @@ -8,19 +8,15 @@ import com.google.common.collect.Lists; import com.hubspot.jinjava.Jinjava; import com.hubspot.jinjava.JinjavaConfig; -import com.hubspot.jinjava.features.FeatureConfig; -import com.hubspot.jinjava.features.FeatureStrategies; import com.hubspot.jinjava.interpret.JinjavaInterpreter.InterpreterScopeClosable; import com.hubspot.jinjava.interpret.TemplateError.ErrorItem; import com.hubspot.jinjava.interpret.TemplateError.ErrorReason; import com.hubspot.jinjava.interpret.TemplateError.ErrorType; -import com.hubspot.jinjava.mode.EagerExecutionMode; import com.hubspot.jinjava.mode.PreserveRawExecutionMode; import com.hubspot.jinjava.objects.date.FormattedDate; import com.hubspot.jinjava.objects.date.StrftimeFormatter; import com.hubspot.jinjava.tree.TextNode; import com.hubspot.jinjava.tree.output.BlockInfo; -import com.hubspot.jinjava.tree.output.OutputList; import com.hubspot.jinjava.tree.parse.TextToken; import com.hubspot.jinjava.tree.parse.TokenScannerSymbols; import java.time.ZoneId; @@ -507,53 +503,4 @@ public void itFiltersDuplicateErrors() { assertThat(interpreter.getErrors()).containsExactly(error1, error2); } - - @Test - public void itPreventsAccidentalExpressions() { - String makeExpression = "if (true) {\n{%- print deferred -%}\n}"; - String makeTag = "if (true) {\n{%- print '% print 123 %' -%}\n}"; - String makeNote = "if (true) {\n{%- print '# note #' -%}\n}"; - jinjava.getGlobalContext().put("deferred", DeferredValue.instance()); - - JinjavaInterpreter normalInterpreter = new JinjavaInterpreter( - jinjava, - jinjava.getGlobalContext(), - JinjavaConfig.newBuilder().withExecutionMode(EagerExecutionMode.instance()).build() - ); - JinjavaInterpreter preventingInterpreter = new JinjavaInterpreter( - jinjava, - jinjava.getGlobalContext(), - JinjavaConfig - .newBuilder() - .withFeatureConfig( - FeatureConfig - .newBuilder() - .add(OutputList.PREVENT_ACCIDENTAL_EXPRESSIONS, FeatureStrategies.ACTIVE) - .build() - ) - .withExecutionMode(EagerExecutionMode.instance()) - .build() - ); - JinjavaInterpreter.pushCurrent(normalInterpreter); - try { - assertThat(normalInterpreter.render(makeExpression)) - .isEqualTo("if (true) {{% print deferred %}}"); - assertThat(normalInterpreter.render(makeTag)) - .isEqualTo("if (true) {% print 123 %}"); - assertThat(normalInterpreter.render(makeNote)).isEqualTo("if (true) {# note #}"); - } finally { - JinjavaInterpreter.popCurrent(); - } - JinjavaInterpreter.pushCurrent(preventingInterpreter); - try { - assertThat(preventingInterpreter.render(makeExpression)) - .isEqualTo("if (true) {\n" + "{#- #}{% print deferred %}}"); - assertThat(preventingInterpreter.render(makeTag)) - .isEqualTo("if (true) {\n" + "{#- #}% print 123 %}"); - assertThat(preventingInterpreter.render(makeNote)) - .isEqualTo("if (true) {\n" + "{#- #}# note #}"); - } finally { - JinjavaInterpreter.popCurrent(); - } - } } From ca98de925868e072234f03a512e0c891ed41426a Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Tue, 31 Oct 2023 11:07:32 -0400 Subject: [PATCH 3/3] Revert "Merge pull request #1122 from HubSpot/note-trim" This reverts commit 7a60dca9580a929c7fd515429d5f20094202ac4e, reversing changes made to 23695ab534b517842d0225dffdd9df9f632f7c3d. --- .../com/hubspot/jinjava/tree/TreeParser.java | 58 ++++++++++++------- .../hubspot/jinjava/tree/parse/NoteToken.java | 5 -- .../com/hubspot/jinjava/tree/parse/Token.java | 5 ++ .../hubspot/jinjava/tree/TreeParserTest.java | 14 ----- 4 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/tree/TreeParser.java b/src/main/java/com/hubspot/jinjava/tree/TreeParser.java index bc6f62557..07fbbb142 100644 --- a/src/main/java/com/hubspot/jinjava/tree/TreeParser.java +++ b/src/main/java/com/hubspot/jinjava/tree/TreeParser.java @@ -62,7 +62,12 @@ public Node buildTree() { Node node = nextNode(); if (node != null) { - parent.getChildren().add(node); + if (node instanceof TextNode && getLastSibling() instanceof TextNode) { + // merge adjacent text nodes so whitespace control properly applies + getLastSibling().getMaster().mergeImageAndContent(node.getMaster()); + } else { + parent.getChildren().add(node); + } } } @@ -91,12 +96,6 @@ public Node buildTree() { private Node nextNode() { Token token = scanner.next(); - if (token.isLeftTrim()) { - final Node lastSibling = getLastSibling(); - if (lastSibling instanceof TextNode) { - lastSibling.getMaster().setRightTrim(true); - } - } if (token.getType() == symbols.getFixed()) { if (token instanceof UnclosedToken) { @@ -171,7 +170,7 @@ private Node text(TextToken textToken) { final Node lastSibling = getLastSibling(); // if last sibling was a tag and has rightTrimAfterEnd, strip whitespace - if (lastSibling != null && isRightTrim(lastSibling)) { + if (lastSibling instanceof TagNode && isRightTrim((TagNode) lastSibling)) { textToken.setLeftTrim(true); } @@ -187,21 +186,18 @@ private Node text(TextToken textToken) { return n; } - private boolean isRightTrim(Node lastSibling) { - if (lastSibling instanceof TagNode) { - return ( - ((TagNode) lastSibling).getEndName() == null || - ( - ((TagNode) lastSibling).getTag() instanceof FlexibleTag && - !((FlexibleTag) ((TagNode) lastSibling).getTag()).hasEndTag( - (TagToken) lastSibling.getMaster() - ) - ) + private boolean isRightTrim(TagNode lastSibling) { + return ( + lastSibling.getEndName() == null || + ( + lastSibling.getTag() instanceof FlexibleTag && + !((FlexibleTag) lastSibling.getTag()).hasEndTag( + (TagToken) lastSibling.getMaster() + ) ) - ? lastSibling.getMaster().isRightTrim() - : lastSibling.getMaster().isRightTrimAfterEnd(); - } - return lastSibling.getMaster().isRightTrim(); + ) + ? lastSibling.getMaster().isRightTrim() + : lastSibling.getMaster().isRightTrimAfterEnd(); } private Node expression(ExpressionToken expressionToken) { @@ -246,6 +242,14 @@ private Node tag(TagToken tagToken) { if (tag instanceof EndTag) { endTag(tag, tagToken); return null; + } else { + // if a tag has left trim, mark the last sibling to trim right whitespace + if (tagToken.isLeftTrim()) { + final Node lastSibling = getLastSibling(); + if (lastSibling instanceof TextNode) { + lastSibling.getMaster().setRightTrim(true); + } + } } TagNode node = new TagNode(tag, tagToken, symbols); @@ -264,6 +268,16 @@ private Node tag(TagToken tagToken) { } private void endTag(Tag tag, TagToken tagToken) { + final Node lastSibling = getLastSibling(); + + if ( + parent instanceof TagNode && + tagToken.isLeftTrim() && + lastSibling instanceof TextNode + ) { + lastSibling.getMaster().setRightTrim(true); + } + if (parent.getMaster() != null) { // root node parent.getMaster().setRightTrimAfterEnd(tagToken.isRightTrim()); } diff --git a/src/main/java/com/hubspot/jinjava/tree/parse/NoteToken.java b/src/main/java/com/hubspot/jinjava/tree/parse/NoteToken.java index c57f8e124..fe98c4dda 100644 --- a/src/main/java/com/hubspot/jinjava/tree/parse/NoteToken.java +++ b/src/main/java/com/hubspot/jinjava/tree/parse/NoteToken.java @@ -15,8 +15,6 @@ **********************************************************************/ package com.hubspot.jinjava.tree.parse; -import org.apache.commons.lang3.StringUtils; - public class NoteToken extends Token { private static final long serialVersionUID = -3859011447900311329L; @@ -39,9 +37,6 @@ public int getType() { */ @Override protected void parse() { - if (StringUtils.isNotEmpty(image)) { - handleTrim(image.substring(2, image.length() - 2)); - } content = ""; } diff --git a/src/main/java/com/hubspot/jinjava/tree/parse/Token.java b/src/main/java/com/hubspot/jinjava/tree/parse/Token.java index 487987626..dda5bd63b 100644 --- a/src/main/java/com/hubspot/jinjava/tree/parse/Token.java +++ b/src/main/java/com/hubspot/jinjava/tree/parse/Token.java @@ -53,6 +53,11 @@ public String getImage() { return image; } + public void mergeImageAndContent(Token otherToken) { + this.image = image + otherToken.image; + this.content = content + otherToken.content; + } + public int getLineNumber() { return lineNumber; } diff --git a/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java b/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java index b31e82cc3..6ba9065ca 100644 --- a/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java +++ b/src/test/java/com/hubspot/jinjava/tree/TreeParserTest.java @@ -224,20 +224,6 @@ public void itWarnsTwiceAgainstUnclosedBlockTag() { assertThat(interpreter.getErrors().get(1).getLineno()).isEqualTo(1); } - @Test - public void itTrimsNotes() { - String expression = "A\n{#- note -#}\nB"; - final Node tree = new TreeParser(interpreter, expression).buildTree(); - assertThat(interpreter.render(tree)).isEqualTo("AB"); - } - - @Test - public void itTrimsExpressions() { - String expression = "A\n{{- 'B' -}}\nC"; - final Node tree = new TreeParser(interpreter, expression).buildTree(); - assertThat(interpreter.render(tree)).isEqualTo("ABC"); - } - Node parse(String fixture) { try { return new TreeParser(