From 03e7fc67c313798a71c64c2b3b541ac70c89891f Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Mon, 15 Mar 2021 10:12:17 -0400 Subject: [PATCH 1/4] With eager execution, only output pyish versions of objects if usePyishObjectMapper is true --- .../lib/expression/EagerExpressionStrategy.java | 10 +++++----- .../jinjava/lib/tag/eager/EagerPrintTag.java | 16 +++++++--------- .../jinjava/lib/tag/eager/EagerDoTagTest.java | 11 +---------- .../lib/tag/eager/EagerImportTagTest.java | 1 + 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java b/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java index 8756b59ae..0dad31b52 100644 --- a/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java +++ b/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java @@ -11,7 +11,6 @@ import com.hubspot.jinjava.tree.parse.ExpressionToken; import com.hubspot.jinjava.util.ChunkResolver; import com.hubspot.jinjava.util.Logging; -import com.hubspot.jinjava.util.WhitespaceUtils; import org.apache.commons.lang3.StringUtils; public class EagerExpressionStrategy implements ExpressionStrategy { @@ -48,11 +47,12 @@ private EagerStringResult eagerResolveExpression( : "" ); if (chunkResolver.getDeferredWords().isEmpty()) { - String fullyResolved = chunkResolver.resolveChunk( - resolvedExpression.getResult(), - "" + String result = interpreter.getAsString( + interpreter.resolveELExpression( + resolvedExpression.getResult(), + interpreter.getLineNumber() + ) ); - String result = WhitespaceUtils.unquoteAndUnescape(fullyResolved); if ( !StringUtils.equals(result, master.getImage()) && ( diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java index e3a3842e3..53f00de8d 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java @@ -6,7 +6,6 @@ import com.hubspot.jinjava.tree.parse.TagToken; import com.hubspot.jinjava.util.ChunkResolver; import com.hubspot.jinjava.util.LengthLimitingStringJoiner; -import com.hubspot.jinjava.util.WhitespaceUtils; import org.apache.commons.lang3.StringUtils; public class EagerPrintTag extends EagerStateChangingTag { @@ -72,17 +71,16 @@ public static String interpretExpression( : "" ); if (chunkResolver.getDeferredWords().isEmpty()) { + String result = interpreter.getAsString( + interpreter.resolveELExpression( + resolvedExpression.getResult(), + interpreter.getLineNumber() + ) + ); // Possible macro/set tag in front of this one. return ( prefixToPreserveState.toString() + - ( - includeExpressionResult - ? wrapInRawIfNeeded( - WhitespaceUtils.unquote(resolvedExpression.getResult()), - interpreter - ) - : "" - ) + (includeExpressionResult ? wrapInRawIfNeeded(result, interpreter) : "") ); } prefixToPreserveState.append( diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java index e2f10fcae..cdb62f4c0 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java @@ -29,6 +29,7 @@ public void eagerSetup() { .newBuilder() .withMaxOutputSize(MAX_OUTPUT_SIZE) .withExecutionMode(EagerExecutionMode.instance()) + .withUsePyishObjectMapper(true) .build() ); @@ -64,14 +65,4 @@ public void itLimitsLength() { assertThat(interpreter.getErrors().get(0).getReason()) .isEqualTo(ErrorReason.OUTPUT_TOO_BIG); } - - /** This is broken in normal Jinjava as hey does not get output in quotes. - * It works in Eager Jinjava as hey is quoted properly. - */ - @Test - @Override - public void itResolvesExpressions() { - String template = "{% set output = [] %}{% do output.append('hey') %}{{ output }}"; - assertThat(interpreter.render(template)).isEqualTo("['hey']"); - } } diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java index 6674206ba..c3559ed20 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -39,6 +39,7 @@ public void eagerSetup() { JinjavaConfig .newBuilder() .withExecutionMode(EagerExecutionMode.instance()) + .withUsePyishObjectMapper(true) .build() ); Tag tag = EagerTagFactory From cef80fbe32ae90a209c8b0ec929b7ed320e741a6 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Mon, 15 Mar 2021 10:28:43 -0400 Subject: [PATCH 2/4] Refactor usePyishObjectMapper into LegacyOverrides --- .../java/com/hubspot/jinjava/JinjavaConfig.java | 14 +------------- .../java/com/hubspot/jinjava/LegacyOverrides.java | 15 ++++++++++++++- .../jinjava/interpret/JinjavaInterpreter.java | 2 +- .../java/com/hubspot/jinjava/BaseJinjavaTest.java | 9 ++++++++- src/test/java/com/hubspot/jinjava/EagerTest.java | 8 ++++++-- .../expression/EagerExpressionStrategyTest.java | 5 ++++- .../jinjava/lib/tag/eager/EagerDoTagTest.java | 5 ++++- .../jinjava/lib/tag/eager/EagerImportTagTest.java | 5 ++++- 8 files changed, 42 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java index ff356aa96..d62880df8 100644 --- a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java +++ b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java @@ -51,7 +51,6 @@ public class JinjavaConfig { private final Map> disabled; private final boolean failOnUnknownTokens; private final boolean nestedInterpretationEnabled; - private final boolean usePyishObjectMapper; private final RandomNumberGeneratorStrategy randomNumberGenerator; private final boolean validationMode; private final long maxStringLength; @@ -103,7 +102,6 @@ private JinjavaConfig(Builder builder) { failOnUnknownTokens = builder.failOnUnknownTokens; maxOutputSize = builder.maxOutputSize; nestedInterpretationEnabled = builder.nestedInterpretationEnabled; - usePyishObjectMapper = builder.usePyishObjectMapper; randomNumberGenerator = builder.randomNumberGeneratorStrategy; validationMode = builder.validationMode; maxStringLength = builder.maxStringLength; @@ -176,10 +174,6 @@ public boolean isNestedInterpretationEnabled() { return nestedInterpretationEnabled; } - public boolean isUsePyishObjectMapper() { - return usePyishObjectMapper; - } - public boolean isValidationMode() { return validationMode; } @@ -235,7 +229,6 @@ public static class Builder { private int maxMacroRecursionDepth; private boolean failOnUnknownTokens; private boolean nestedInterpretationEnabled = true; - private boolean usePyishObjectMapper = false; private RandomNumberGeneratorStrategy randomNumberGeneratorStrategy = RandomNumberGeneratorStrategy.THREAD_LOCAL; private boolean validationMode = false; @@ -330,11 +323,6 @@ public Builder withNestedInterpretationEnabled(boolean nestedInterpretationEnabl return this; } - public Builder withUsePyishObjectMapper(boolean usePyishObjectMapper) { - this.usePyishObjectMapper = usePyishObjectMapper; - return this; - } - public Builder withValidationMode(boolean validationMode) { this.validationMode = validationMode; return this; @@ -366,7 +354,7 @@ public Builder withTokenScannerSymbols(TokenScannerSymbols tokenScannerSymbols) } /** - * @deprecated Replaced by {@link LegacyOverrides.Builder#withIterateOverMapKeys(boolean)} ()} + * @deprecated Replaced by {@link LegacyOverrides.Builder#withIterateOverMapKeys(boolean)}} */ @Deprecated public Builder withIterateOverMapKeys(boolean iterateOverMapKeys) { diff --git a/src/main/java/com/hubspot/jinjava/LegacyOverrides.java b/src/main/java/com/hubspot/jinjava/LegacyOverrides.java index ebd7a8e23..80684d81a 100644 --- a/src/main/java/com/hubspot/jinjava/LegacyOverrides.java +++ b/src/main/java/com/hubspot/jinjava/LegacyOverrides.java @@ -8,10 +8,12 @@ public class LegacyOverrides { public static final LegacyOverrides NONE = new LegacyOverrides.Builder().build(); private final boolean evaluateMapKeys; private final boolean iterateOverMapKeys; + private final boolean usePyishObjectMapper; private LegacyOverrides(Builder builder) { evaluateMapKeys = builder.evaluateMapKeys; iterateOverMapKeys = builder.iterateOverMapKeys; + usePyishObjectMapper = builder.usePyishObjectMapper; } public static Builder newBuilder() { @@ -26,9 +28,14 @@ public boolean isIterateOverMapKeys() { return iterateOverMapKeys; } + public boolean isUsePyishObjectMapper() { + return usePyishObjectMapper; + } + public static class Builder { private boolean evaluateMapKeys = false; private boolean iterateOverMapKeys = false; + private boolean usePyishObjectMapper = false; private Builder() {} @@ -39,7 +46,8 @@ public LegacyOverrides build() { public static Builder from(LegacyOverrides legacyOverrides) { return new Builder() .withEvaluateMapKeys(legacyOverrides.evaluateMapKeys) - .withIterateOverMapKeys(legacyOverrides.iterateOverMapKeys); + .withIterateOverMapKeys(legacyOverrides.iterateOverMapKeys) + .withUsePyishObjectMapper(legacyOverrides.usePyishObjectMapper); } public Builder withEvaluateMapKeys(boolean evaluateMapKeys) { @@ -51,5 +59,10 @@ public Builder withIterateOverMapKeys(boolean iterateOverMapKeys) { this.iterateOverMapKeys = iterateOverMapKeys; return this; } + + public Builder withUsePyishObjectMapper(boolean usePyishObjectMapper) { + this.usePyishObjectMapper = usePyishObjectMapper; + return this; + } } } diff --git a/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java b/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java index 1a38b6a90..fb718fd61 100644 --- a/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java +++ b/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java @@ -479,7 +479,7 @@ public String resolveString(String variable, int lineNumber, int startPosition) } public String getAsString(Object object) { - if (config.isUsePyishObjectMapper()) { + if (config.getLegacyOverrides().isUsePyishObjectMapper()) { return PyishObjectMapper.getAsUnquotedPyishString(object); } return Objects.toString(object, ""); diff --git a/src/test/java/com/hubspot/jinjava/BaseJinjavaTest.java b/src/test/java/com/hubspot/jinjava/BaseJinjavaTest.java index 9d27b3b66..86fd55ef7 100644 --- a/src/test/java/com/hubspot/jinjava/BaseJinjavaTest.java +++ b/src/test/java/com/hubspot/jinjava/BaseJinjavaTest.java @@ -8,6 +8,13 @@ public abstract class BaseJinjavaTest { @Before public void baseSetup() { jinjava = - new Jinjava(JinjavaConfig.newBuilder().withUsePyishObjectMapper(true).build()); + new Jinjava( + JinjavaConfig + .newBuilder() + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) + .build() + ); } } diff --git a/src/test/java/com/hubspot/jinjava/EagerTest.java b/src/test/java/com/hubspot/jinjava/EagerTest.java index b2ba8ca92..19a9b3c11 100644 --- a/src/test/java/com/hubspot/jinjava/EagerTest.java +++ b/src/test/java/com/hubspot/jinjava/EagerTest.java @@ -67,7 +67,9 @@ public Optional getLocationResolver() { .withRandomNumberGeneratorStrategy(RandomNumberGeneratorStrategy.DEFERRED) .withExecutionMode(EagerExecutionMode.instance()) .withNestedInterpretationEnabled(true) - .withUsePyishObjectMapper(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) .withMaxMacroRecursionDepth(5) .withEnableRecursiveMacroCalls(true) .build(); @@ -712,7 +714,9 @@ public void itWrapsCertainOutputInRaw() { .withRandomNumberGeneratorStrategy(RandomNumberGeneratorStrategy.DEFERRED) .withExecutionMode(EagerExecutionMode.instance()) .withNestedInterpretationEnabled(false) - .withUsePyishObjectMapper(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) .build(); JinjavaInterpreter parentInterpreter = new JinjavaInterpreter( jinjava, diff --git a/src/test/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategyTest.java b/src/test/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategyTest.java index ddcc0a294..951120eb5 100644 --- a/src/test/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategyTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategyTest.java @@ -3,6 +3,7 @@ import static org.assertj.core.api.Assertions.assertThat; import com.hubspot.jinjava.JinjavaConfig; +import com.hubspot.jinjava.LegacyOverrides; import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.mode.EagerExecutionMode; @@ -44,7 +45,9 @@ public void itPreservesRawTags() { JinjavaConfig .newBuilder() .withNestedInterpretationEnabled(false) - .withUsePyishObjectMapper(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) .withExecutionMode(EagerExecutionMode.instance()) .build() ); diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java index cdb62f4c0..437b271dd 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerDoTagTest.java @@ -4,6 +4,7 @@ import com.hubspot.jinjava.ExpectedNodeInterpreter; import com.hubspot.jinjava.JinjavaConfig; +import com.hubspot.jinjava.LegacyOverrides; import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.JinjavaInterpreter; import com.hubspot.jinjava.interpret.TemplateError.ErrorReason; @@ -29,7 +30,9 @@ public void eagerSetup() { .newBuilder() .withMaxOutputSize(MAX_OUTPUT_SIZE) .withExecutionMode(EagerExecutionMode.instance()) - .withUsePyishObjectMapper(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) .build() ); diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java index c3559ed20..1cc166ffe 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/eager/EagerImportTagTest.java @@ -4,6 +4,7 @@ import com.google.common.io.Resources; import com.hubspot.jinjava.JinjavaConfig; +import com.hubspot.jinjava.LegacyOverrides; import com.hubspot.jinjava.interpret.Context; import com.hubspot.jinjava.interpret.DeferredValue; import com.hubspot.jinjava.interpret.JinjavaInterpreter; @@ -39,7 +40,9 @@ public void eagerSetup() { JinjavaConfig .newBuilder() .withExecutionMode(EagerExecutionMode.instance()) - .withUsePyishObjectMapper(true) + .withLegacyOverrides( + LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build() + ) .build() ); Tag tag = EagerTagFactory From 47d47bace2207513bf7b79308c9bc2df6887740c Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Mon, 15 Mar 2021 10:35:19 -0400 Subject: [PATCH 3/4] Refactor ChunkResolver.resolveChunk() as a private method --- .../hubspot/jinjava/util/ChunkResolver.java | 20 ++++++++----------- .../jinjava/util/ChunkResolverTest.java | 2 +- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/util/ChunkResolver.java b/src/main/java/com/hubspot/jinjava/util/ChunkResolver.java index 8e9028253..9ddf7a010 100644 --- a/src/main/java/com/hubspot/jinjava/util/ChunkResolver.java +++ b/src/main/java/com/hubspot/jinjava/util/ChunkResolver.java @@ -180,7 +180,7 @@ private List getChunk(Character chunkLevelMarker) { } else if (CHUNK_LEVEL_MARKER_MAP.containsKey(c)) { setPrevChar(c); tokenBuilder.append(c); - tokenBuilder.append(resolveChunk(String.join("", getChunk(c)), JINJAVA_NULL)); + tokenBuilder.append(resolveChunk(String.join("", getChunk(c)))); tokenBuilder.append(prevChar); continue; } else if (isTokenSplitter(c)) { @@ -201,7 +201,7 @@ private List getChunk(Character chunkLevelMarker) { } tokenBuilder = new StringBuilder(); if (isMiniChunkSplitter(c)) { - chunks.add(resolveChunk(miniChunkBuilder.toString(), JINJAVA_NULL)); + chunks.add(resolveChunk(miniChunkBuilder.toString())); chunks.add(String.valueOf(c)); miniChunkBuilder = new StringBuilder(); } else { @@ -221,7 +221,7 @@ private List getChunk(Character chunkLevelMarker) { tokenBuilder.append(c); } miniChunkBuilder.append(resolveToken(tokenBuilder.toString())); - chunks.add(resolveChunk(miniChunkBuilder.toString(), JINJAVA_NULL)); + chunks.add(resolveChunk(miniChunkBuilder.toString())); return chunks; } @@ -290,9 +290,7 @@ private String resolveToken(String token) { // val is still null } } - if (val == null || !isResolvableObject(val)) { - resolvedToken = token; - } else { + if (val != null && isResolvableObject(val)) { resolvedToken = PyishObjectMapper.getAsPyishString(val); } } @@ -303,7 +301,7 @@ private String resolveToken(String token) { } // Try resolving the chunk/mini chunk as an ELExpression - public String resolveChunk(String chunk, String nullDefault) { + private String resolveChunk(String chunk) { if (StringUtils.isBlank(chunk)) { return chunk; } @@ -320,7 +318,7 @@ public String resolveChunk(String chunk, String nullDefault) { ); if (val != null) { // If this isn't the final call, don't prematurely resolve complex objects. - if (JINJAVA_NULL.equals(nullDefault) && !isResolvableObject(val)) { + if (!isResolvableObject(val)) { return chunk; } } @@ -328,10 +326,8 @@ public String resolveChunk(String chunk, String nullDefault) { Object val = interpreter.resolveELExpression(chunk, token.getLineNumber()); if (val == null) { - resolvedChunk = nullDefault; - } else if (JINJAVA_NULL.equals(nullDefault) && !isResolvableObject(val)) { - resolvedChunk = chunk; - } else { + resolvedChunk = ChunkResolver.JINJAVA_NULL; + } else if (isResolvableObject(val)) { resolvedChunk = PyishObjectMapper.getAsPyishString(val); } } diff --git a/src/test/java/com/hubspot/jinjava/util/ChunkResolverTest.java b/src/test/java/com/hubspot/jinjava/util/ChunkResolverTest.java index 41c57f108..ec957c722 100644 --- a/src/test/java/com/hubspot/jinjava/util/ChunkResolverTest.java +++ b/src/test/java/com/hubspot/jinjava/util/ChunkResolverTest.java @@ -288,7 +288,7 @@ public void itSerializesDateProperly() { // don't prematurely resolve date because of datetime functions. assertThat(WhitespaceUtils.unquoteAndUnescape(chunkResolver.resolveChunks())) .isEqualTo("date"); - assertThat(WhitespaceUtils.unquoteAndUnescape(chunkResolver.resolveChunk("date", ""))) + assertThat(WhitespaceUtils.unquoteAndUnescape(interpreter.resolveString("date", -1))) .isEqualTo(date.toString()); } From 9ad34b56a4ef37280f21f366eb330f6f33b9585a Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Mon, 15 Mar 2021 10:37:43 -0400 Subject: [PATCH 4/4] Inline variable to save computation when evaluating a DoTag --- .../jinjava/lib/tag/eager/EagerPrintTag.java | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java index 53f00de8d..b263f4f81 100644 --- a/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java +++ b/src/main/java/com/hubspot/jinjava/lib/tag/eager/EagerPrintTag.java @@ -71,16 +71,22 @@ public static String interpretExpression( : "" ); if (chunkResolver.getDeferredWords().isEmpty()) { - String result = interpreter.getAsString( - interpreter.resolveELExpression( - resolvedExpression.getResult(), - interpreter.getLineNumber() - ) - ); // Possible macro/set tag in front of this one. return ( prefixToPreserveState.toString() + - (includeExpressionResult ? wrapInRawIfNeeded(result, interpreter) : "") + ( + includeExpressionResult + ? wrapInRawIfNeeded( + interpreter.getAsString( + interpreter.resolveELExpression( + resolvedExpression.getResult(), + interpreter.getLineNumber() + ) + ), + interpreter + ) + : "" + ) ); } prefixToPreserveState.append(