Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature to ignore parsing errors when doing nested interpretation #1196

Merged
merged 5 commits into from
Sep 12, 2024
Merged

Add feature to ignore parsing errors when doing nested interpretation #1196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
72d938f
Add feature to ignore parse errors when doing nested interpretation
jasmith-hs Sep 3, 2024
09cbfba
Test that errors aren't added when parse errors are ignored
jasmith-hs Sep 3, 2024
4e144d8
Test that parse errors are ignored when doing nested interpretation
jasmith-hs Sep 3, 2024
f5975fa
Replace isIgnoreParseErrors and isThrowInterpreterErrors with
jasmith-hs Sep 3, 2024
447fcfd
Add default branch for checkstyle
jasmith-hs Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 70 additions & 2 deletions src/main/java/com/hubspot/jinjava/interpret/Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
42 changes: 38 additions & 4 deletions src/main/java/com/hubspot/jinjava/interpret/ContextConfigurationIF.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
97 changes: 62 additions & 35 deletions src/main/java/com/hubspot/jinjava/interpret/JinjavaInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<>();
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);
}
}
}
}

Expand Down
33 changes: 13 additions & 20 deletions src/main/java/com/hubspot/jinjava/lib/expression/EagerExpressionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
}
Expand All @@ -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(
Expand Down
Loading
Loading