Merge pull request #621 from HubSpot/eager-java-like-output

Output pyish versions of objects using legacy override flag
HubSpot · Mar 15, 2021 · 77a18af · 77a18af
2 parents 8cee1f9 + 9ad34b5
commit 77a18af
Show file tree

Hide file tree

Showing 12 changed files with 62 additions and 49 deletions.
diff --git 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<Context.Library, Set<String>> 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
@@ -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
@@ -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/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
@@ -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<PrintTag> {
@@ -78,7 +77,12 @@ public static String interpretExpression(
         (
           includeExpressionResult
             ? wrapInRawIfNeeded(
-              WhitespaceUtils.unquote(resolvedExpression.getResult()),
+              interpreter.getAsString(
+                interpreter.resolveELExpression(
+                  resolvedExpression.getResult(),
+                  interpreter.getLineNumber()
+                )
+              ),
               interpreter
             )
             : ""

diff --git 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<String> 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<String> 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<String> 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,18 +318,16 @@ 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;
             }
           }
         } catch (TemplateSyntaxException ignored) {}
 
         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/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
@@ -67,7 +67,9 @@ public Optional<LocationResolver> 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
@@ -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
@@ -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,6 +30,9 @@ public void eagerSetup() {
           .newBuilder()
           .withMaxOutputSize(MAX_OUTPUT_SIZE)
           .withExecutionMode(EagerExecutionMode.instance())
+          .withLegacyOverrides(
+            LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
+          )
           .build()
       );
 
@@ -64,14 +68,4 @@ public void itLimitsLength() {
     assertThat(interpreter.getErrors().get(0).getReason())
       .isEqualTo(ErrorReason.OUTPUT_TOO_BIG);
   }
-
-  /** This is broken in normal Jinjava as <code>hey</code> does not get output in quotes.
-   * It works in Eager Jinjava as <code>hey</code> 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
@@ -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,6 +40,9 @@ public void eagerSetup() {
         JinjavaConfig
           .newBuilder()
           .withExecutionMode(EagerExecutionMode.instance())
+          .withLegacyOverrides(
+            LegacyOverrides.newBuilder().withUsePyishObjectMapper(true).build()
+          )
           .build()
       );
     Tag tag = EagerTagFactory

diff --git 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());
   }