Merge pull request #1196 from HubSpot/ignore-parse-errors

Add feature to ignore parsing errors when doing nested interpretation
HubSpot · Sep 12, 2024 · 11926b5 · 11926b5
2 parents 50f8d2f + 447fcfd
commit 11926b5
Show file tree

Hide file tree

Showing 7 changed files with 238 additions and 68 deletions.
diff --git a/src/main/java/com/hubspot/jinjava/interpret/Context.java b/src/main/java/com/hubspot/jinjava/interpret/Context.java
@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
+import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF.TemplateErrorTypeHandlingStrategy;
 import com.hubspot.jinjava.lib.Importable;
 import com.hubspot.jinjava.lib.expression.ExpressionStrategy;
 import com.hubspot.jinjava.lib.exptest.ExpTest;
@@ -769,13 +770,64 @@ public TemporaryValueClosable<Boolean> withDeferLargeObjects(
     return temporaryValueClosable;
   }
 
+  @Deprecated
   public boolean getThrowInterpreterErrors() {
-    return contextConfiguration.isThrowInterpreterErrors();
+    ErrorHandlingStrategy errorHandlingStrategy = getErrorHandlingStrategy();
+    return (
+      errorHandlingStrategy.getFatalErrorStrategy() ==
+      TemplateErrorTypeHandlingStrategy.THROW_EXCEPTION
+    );
   }
 
+  @Deprecated
   public void setThrowInterpreterErrors(boolean throwInterpreterErrors) {
     contextConfiguration =
-      contextConfiguration.withThrowInterpreterErrors(throwInterpreterErrors);
+      contextConfiguration.withErrorHandlingStrategy(
+        ErrorHandlingStrategy
+          .builder()
+          .setFatalErrorStrategy(
+            throwInterpreterErrors
+              ? TemplateErrorTypeHandlingStrategy.THROW_EXCEPTION
+              : TemplateErrorTypeHandlingStrategy.ADD_ERROR
+          )
+          .setNonFatalErrorStrategy(
+            throwInterpreterErrors
+              ? TemplateErrorTypeHandlingStrategy.IGNORE // Deprecated, warnings are ignored when doing eager expression resolving
+              : TemplateErrorTypeHandlingStrategy.ADD_ERROR
+          )
+          .build()
+      );
+  }
+
+  @Deprecated
+  public TemporaryValueClosable<Boolean> withThrowInterpreterErrors() {
+    TemporaryValueClosable<Boolean> temporaryValueClosable = new TemporaryValueClosable<>(
+      getThrowInterpreterErrors(),
+      this::setThrowInterpreterErrors
+    );
+    setThrowInterpreterErrors(true);
+    return temporaryValueClosable;
+  }
+
+  public ErrorHandlingStrategy getErrorHandlingStrategy() {
+    return contextConfiguration.getErrorHandlingStrategy();
+  }
+
+  public void setErrorHandlingStrategy(ErrorHandlingStrategy errorHandlingStrategy) {
+    contextConfiguration =
+      contextConfiguration.withErrorHandlingStrategy(errorHandlingStrategy);
+  }
+
+  public TemporaryValueClosable<ErrorHandlingStrategy> withErrorHandlingStrategy(
+    ErrorHandlingStrategy errorHandlingStrategy
+  ) {
+    TemporaryValueClosable<ErrorHandlingStrategy> temporaryValueClosable =
+      new TemporaryValueClosable<>(
+        getErrorHandlingStrategy(),
+        this::setErrorHandlingStrategy
+      );
+    setErrorHandlingStrategy(errorHandlingStrategy);
+    return temporaryValueClosable;
   }
 
   public boolean isPartialMacroEvaluation() {
@@ -829,10 +881,26 @@ private TemporaryValueClosable(T previousValue, Consumer<T> resetValueConsumer)
       this.resetValueConsumer = resetValueConsumer;
     }
 
+    public static <T> TemporaryValueClosable<T> noOp() {
+      return new NoOpTemporaryValueClosable<>();
+    }
+
     @Override
     public void close() {
       resetValueConsumer.accept(previousValue);
     }
+
+    private static class NoOpTemporaryValueClosable<T> extends TemporaryValueClosable<T> {
+
+      private NoOpTemporaryValueClosable() {
+        super(null, null);
+      }
+
+      @Override
+      public void close() {
+        // No-op
+      }
+    }
   }
 
   public Node getCurrentNode() {

diff --git a/src/main/java/com/hubspot/jinjava/interpret/ContextConfigurationIF.java b/src/main/java/com/hubspot/jinjava/interpret/ContextConfigurationIF.java
@@ -34,17 +34,51 @@ default boolean isDeferLargeObjects() {
   }
 
   @Default
-  default boolean isThrowInterpreterErrors() {
+  default boolean isPartialMacroEvaluation() {
     return false;
   }
 
   @Default
-  default boolean isPartialMacroEvaluation() {
+  default boolean isUnwrapRawOverride() {
     return false;
   }
 
   @Default
-  default boolean isUnwrapRawOverride() {
-    return false;
+  default ErrorHandlingStrategy getErrorHandlingStrategy() {
+    return ErrorHandlingStrategy.of();
+  }
+
+  @Immutable(singleton = true)
+  @HubSpotImmutableStyle
+  interface ErrorHandlingStrategyIF {
+    @Default
+    default TemplateErrorTypeHandlingStrategy getFatalErrorStrategy() {
+      return TemplateErrorTypeHandlingStrategy.ADD_ERROR;
+    }
+
+    @Default
+    default TemplateErrorTypeHandlingStrategy getNonFatalErrorStrategy() {
+      return TemplateErrorTypeHandlingStrategy.ADD_ERROR;
+    }
+
+    enum TemplateErrorTypeHandlingStrategy {
+      IGNORE,
+      ADD_ERROR,
+      THROW_EXCEPTION,
+    }
+
+    static ErrorHandlingStrategy throwAll() {
+      return ErrorHandlingStrategy
+        .of()
+        .withFatalErrorStrategy(TemplateErrorTypeHandlingStrategy.THROW_EXCEPTION)
+        .withNonFatalErrorStrategy(TemplateErrorTypeHandlingStrategy.THROW_EXCEPTION);
+    }
+
+    static ErrorHandlingStrategy ignoreAll() {
+      return ErrorHandlingStrategy
+        .of()
+        .withFatalErrorStrategy(TemplateErrorTypeHandlingStrategy.IGNORE)
+        .withNonFatalErrorStrategy(TemplateErrorTypeHandlingStrategy.IGNORE);
+    }
   }
 }
diff --git a/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java b/src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java
@@ -29,6 +29,9 @@
 import com.hubspot.jinjava.el.ExpressionResolver;
 import com.hubspot.jinjava.el.ext.DeferredParsingException;
 import com.hubspot.jinjava.el.ext.ExtendedParser;
+import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable;
+import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF;
+import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF.TemplateErrorTypeHandlingStrategy;
 import com.hubspot.jinjava.interpret.TemplateError.ErrorItem;
 import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
 import com.hubspot.jinjava.interpret.TemplateError.ErrorType;
@@ -83,6 +86,8 @@ public class JinjavaInterpreter implements PyishSerializable {
 
   public static final String OUTPUT_UNDEFINED_VARIABLES_ERROR =
     "OUTPUT_UNDEFINED_VARIABLES_ERROR";
+  public static final String IGNORE_NESTED_INTERPRETATION_PARSE_ERRORS =
+    "IGNORE_NESTED_INTERPRETATION_PARSE_ERRORS";
   private final Multimap<String, BlockInfo> blocks = ArrayListMultimap.create();
   private final LinkedList<Node> extendParentRoots = new LinkedList<>();
   private final Map<String, RevertibleObject> revertibleObjects = new HashMap<>();
@@ -265,13 +270,30 @@ public String renderFlat(String template, long renderLimit) {
         return template;
       } else {
         context.setRenderDepth(depth + 1);
-        return render(parse(template), false, renderLimit);
+        Node parsedNode;
+        try (
+          TemporaryValueClosable<ErrorHandlingStrategy> c = ignoreParseErrorsIfActivated()
+        ) {
+          parsedNode = parse(template);
+        }
+        return render(parsedNode, false, renderLimit);
       }
     } finally {
       context.setRenderDepth(depth);
     }
   }
 
+  private TemporaryValueClosable<ErrorHandlingStrategy> ignoreParseErrorsIfActivated() {
+    return config
+        .getFeatures()
+        .getActivationStrategy(
+          JinjavaInterpreter.IGNORE_NESTED_INTERPRETATION_PARSE_ERRORS
+        )
+        .isActive(context)
+      ? context.withErrorHandlingStrategy(ErrorHandlingStrategyIF.ignoreAll())
+      : TemporaryValueClosable.noOp();
+  }
+
   /**
    * Parse the given string into a root Node, and then renders it processing extend parents.
    *
@@ -812,46 +834,51 @@ public void addError(TemplateError templateError) {
     if (templateError == null) {
       return;
     }
-
-    if (context.getThrowInterpreterErrors()) {
-      if (templateError.getSeverity() == ErrorType.FATAL) {
-        // Throw fatal errors when locating deferred words.
+    ErrorHandlingStrategy errorHandlingStrategy = context.getErrorHandlingStrategy();
+    TemplateErrorTypeHandlingStrategy errorTypeHandlingStrategy =
+      templateError.getSeverity() == ErrorType.FATAL
+        ? errorHandlingStrategy.getFatalErrorStrategy()
+        : errorHandlingStrategy.getNonFatalErrorStrategy();
+    switch (errorTypeHandlingStrategy) {
+      case IGNORE:
+        return;
+      case THROW_EXCEPTION:
         throw new TemplateSyntaxException(
           this,
           templateError.getFieldName(),
           templateError.getMessage()
         );
-      } else {
-        // Hide warning errors when locating deferred words.
-        return;
-      }
-    }
-    // fix line numbers not matching up with source template
-    if (!context.getCurrentPathStack().isEmpty()) {
-      if (
-        !templateError.getSourceTemplate().isPresent() &&
-        context.getCurrentPathStack().peek().isPresent()
-      ) {
-        templateError.setMessage(
-          getWrappedErrorMessage(
-            context.getCurrentPathStack().peek().get(),
-            templateError
-          )
-        );
-        templateError.setSourceTemplate(context.getCurrentPathStack().peek().get());
-      }
-      templateError.setStartPosition(context.getCurrentPathStack().getTopStartPosition());
-      templateError.setLineno(context.getCurrentPathStack().getTopLineNumber());
-    }
+      case ADD_ERROR:
+      default: // Checkstyle
+        // fix line numbers not matching up with source template
+        if (!context.getCurrentPathStack().isEmpty()) {
+          if (
+            !templateError.getSourceTemplate().isPresent() &&
+            context.getCurrentPathStack().peek().isPresent()
+          ) {
+            templateError.setMessage(
+              getWrappedErrorMessage(
+                context.getCurrentPathStack().peek().get(),
+                templateError
+              )
+            );
+            templateError.setSourceTemplate(context.getCurrentPathStack().peek().get());
+          }
+          templateError.setStartPosition(
+            context.getCurrentPathStack().getTopStartPosition()
+          );
+          templateError.setLineno(context.getCurrentPathStack().getTopLineNumber());
+        }
 
-    // Limit the number of errors and filter duplicates
-    if (errors.size() < MAX_ERROR_SIZE) {
-      templateError = templateError.withScopeDepth(scopeDepth);
-      int errorCode = templateError.hashCode();
-      if (!errorSet.contains(errorCode)) {
-        this.errors.add(templateError);
-        this.errorSet.add(errorCode);
-      }
+        // Limit the number of errors and filter duplicates
+        if (errors.size() < MAX_ERROR_SIZE) {
+          templateError = templateError.withScopeDepth(scopeDepth);
+          int errorCode = templateError.hashCode();
+          if (!errorSet.contains(errorCode)) {
+            this.errors.add(templateError);
+            this.errorSet.add(errorCode);
+          }
+        }
     }
   }
 

diff --git a/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java b/src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java
@@ -2,8 +2,11 @@
 
 import com.google.common.annotations.Beta;
 import com.hubspot.jinjava.JinjavaConfig;
+import com.hubspot.jinjava.interpret.Context.TemporaryValueClosable;
+import com.hubspot.jinjava.interpret.ContextConfigurationIF.ErrorHandlingStrategyIF;
+import com.hubspot.jinjava.interpret.ErrorHandlingStrategy;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
-import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
+import com.hubspot.jinjava.interpret.TemplateSyntaxException;
 import com.hubspot.jinjava.lib.filter.EscapeFilter;
 import com.hubspot.jinjava.lib.tag.eager.DeferredToken;
 import com.hubspot.jinjava.lib.tag.eager.EagerExecutionResult;
@@ -14,7 +17,6 @@
 import com.hubspot.jinjava.util.EagerReconstructionUtils;
 import com.hubspot.jinjava.util.Logging;
 import com.hubspot.jinjava.util.PrefixToPreserveState;
-import java.util.Objects;
 import org.apache.commons.lang3.StringUtils;
 
 @Beta
@@ -103,17 +105,20 @@ public static String postProcessResult(
         StringUtils.contains(result, master.getSymbols().getExpressionStartWithTag()))
     ) {
       if (interpreter.getConfig().isNestedInterpretationEnabled()) {
-        long errorSizeStart = getParsingErrorsCount(interpreter);
-
-        interpreter.parse(result);
-
-        if (getParsingErrorsCount(interpreter) == errorSizeStart) {
+        try {
+          try (
+            TemporaryValueClosable<ErrorHandlingStrategy> c = interpreter
+              .getContext()
+              .withErrorHandlingStrategy(ErrorHandlingStrategyIF.throwAll())
+          ) {
+            interpreter.parse(result);
+          }
           try {
             result = interpreter.renderFlat(result);
           } catch (Exception e) {
             Logging.ENGINE_LOG.warn("Error rendering variable node result", e);
           }
-        }
+        } catch (TemplateSyntaxException ignored) {}
       } else {
         result = EagerReconstructionUtils.wrapInRawIfNeeded(result, interpreter);
       }
@@ -125,18 +130,6 @@ public static String postProcessResult(
     return result;
   }
 
-  private static long getParsingErrorsCount(JinjavaInterpreter interpreter) {
-    return interpreter
-      .getErrors()
-      .stream()
-      .filter(Objects::nonNull)
-      .filter(error ->
-        "Unclosed comment".equals(error.getMessage()) ||
-        error.getReason() == ErrorReason.DISABLED
-      )
-      .count();
-  }
-
   private static String wrapInExpression(String output, JinjavaInterpreter interpreter) {
     JinjavaConfig config = interpreter.getConfig();
     return String.format(