diff --git a/checker/src/test/java/dev/cel/checker/CelIssueTest.java b/checker/src/test/java/dev/cel/checker/CelIssueTest.java index e0d34fd0..a200a5fd 100644 --- a/checker/src/test/java/dev/cel/checker/CelIssueTest.java +++ b/checker/src/test/java/dev/cel/checker/CelIssueTest.java @@ -16,9 +16,7 @@ import static com.google.common.truth.Truth.assertThat; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import dev.cel.common.CelIssue; import dev.cel.common.CelSource; import org.junit.Test; @@ -28,8 +26,6 @@ @RunWith(JUnit4.class) public final class CelIssueTest { - private static final Joiner JOINER = Joiner.on('\n'); - @Test public void toDisplayString_narrow() throws Exception { CelSource source = @@ -38,7 +34,7 @@ public void toDisplayString_narrow() throws Exception { ImmutableList.of( CelIssue.formatError(1, 1, "No such field"), CelIssue.formatError(2, 20, "Syntax error, missing paren")); - assertThat(JOINER.join(Iterables.transform(issues, error -> error.toDisplayString(source)))) + assertThat(CelIssue.toDisplayString(issues, source)) .isEqualTo( "ERROR: issues-test:1:2: No such field\n" + " | a.b\n" @@ -53,7 +49,7 @@ public void toDisplayString_wideAndNarrow() throws Exception { CelSource source = CelSource.newBuilder("你好吗\n我b很好\n").setDescription("issues-test").build(); ImmutableList issues = ImmutableList.of(CelIssue.formatError(2, 3, "Unexpected character '好'")); - assertThat(JOINER.join(Iterables.transform(issues, error -> error.toDisplayString(source)))) + assertThat(CelIssue.toDisplayString(issues, source)) .isEqualTo("ERROR: issues-test:2:4: Unexpected character '好'\n" + " | 我b很好\n" + " | ...^"); } @@ -73,7 +69,8 @@ public void toDisplayString_emojis() throws Exception { + " IDENTIFIER}"), CelIssue.formatError(1, 35, "Syntax error: token recognition error at: '😁'"), CelIssue.formatError(1, 36, "Syntax error: missing IDENTIFIER at ''")); - assertThat(JOINER.join(Iterables.transform(issues, error -> error.toDisplayString(source)))) + + assertThat(CelIssue.toDisplayString(issues, source)) .isEqualTo( "ERROR: issues-test:1:33: Syntax error: extraneous input 'in' expecting {'[', '{'," + " '(', '.', '-', '!', 'true', 'false', 'null', NUM_FLOAT, NUM_INT, NUM_UINT," diff --git a/common/src/main/java/dev/cel/common/CelIssue.java b/common/src/main/java/dev/cel/common/CelIssue.java index 1e3cdb58..2f507d5a 100644 --- a/common/src/main/java/dev/cel/common/CelIssue.java +++ b/common/src/main/java/dev/cel/common/CelIssue.java @@ -14,10 +14,14 @@ package dev.cel.common; +import static com.google.common.collect.ImmutableList.toImmutableList; + import com.google.auto.value.AutoValue; +import com.google.common.base.Joiner; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; import dev.cel.common.internal.SafeStringFormatter; +import java.util.Collection; import java.util.Optional; import java.util.PrimitiveIterator; @@ -28,6 +32,7 @@ @Immutable @SuppressWarnings("UnicodeEscape") // Suppressed to distinguish half-width and full-width chars. public abstract class CelIssue { + private static final Joiner JOINER = Joiner.on('\n'); /** Severity of a CelIssue. */ public enum Severity { @@ -73,6 +78,12 @@ public static CelIssue formatError(int line, int column, String message) { private static final char WIDE_DOT = '\uff0e'; private static final char WIDE_HAT = '\uff3e'; + /** Returns a human-readable error with all issues joined in a single string. */ + public static String toDisplayString(Collection issues, Source source) { + return JOINER.join( + issues.stream().map(iss -> iss.toDisplayString(source)).collect(toImmutableList())); + } + /** Returns a string representing this error that is suitable for displaying to humans. */ public String toDisplayString(Source source) { // Based onhttps://github.com/google/cel-go/blob/v0.5.1/common/error.go#L42. diff --git a/common/src/main/java/dev/cel/common/CelValidationException.java b/common/src/main/java/dev/cel/common/CelValidationException.java index ef136f8a..522706e8 100644 --- a/common/src/main/java/dev/cel/common/CelValidationException.java +++ b/common/src/main/java/dev/cel/common/CelValidationException.java @@ -15,15 +15,12 @@ package dev.cel.common; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import java.util.List; /** Base class for all checked exceptions explicitly thrown by the library during parsing. */ public final class CelValidationException extends CelException { - private static final Joiner JOINER = Joiner.on('\n'); // Truncates all errors beyond this limit in the message. private static final int MAX_ERRORS_TO_REPORT = 1000; @@ -46,17 +43,14 @@ public CelValidationException(CelSource source, List errors) { private static String safeJoinErrorMessage(CelSource source, List errors) { if (errors.size() <= MAX_ERRORS_TO_REPORT) { - return JOINER.join(Iterables.transform(errors, error -> error.toDisplayString(source))); + return CelIssue.toDisplayString(errors, source); } List truncatedErrors = errors.subList(0, MAX_ERRORS_TO_REPORT); - StringBuilder sb = new StringBuilder(); - JOINER.appendTo( - sb, Iterables.transform(truncatedErrors, error -> error.toDisplayString(source))); - sb.append( - String.format("%n...and %d more errors (truncated)", errors.size() - MAX_ERRORS_TO_REPORT)); - return sb.toString(); + return CelIssue.toDisplayString(truncatedErrors, source) + + String.format( + "%n...and %d more errors (truncated)", errors.size() - MAX_ERRORS_TO_REPORT); } /** Returns the {@link CelSource} that was being validated. */ diff --git a/common/src/main/java/dev/cel/common/CelValidationResult.java b/common/src/main/java/dev/cel/common/CelValidationResult.java index 3e52bc8e..61152c49 100644 --- a/common/src/main/java/dev/cel/common/CelValidationResult.java +++ b/common/src/main/java/dev/cel/common/CelValidationResult.java @@ -17,9 +17,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.Comparator.comparing; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.Immutable; import com.google.errorprone.annotations.InlineMe; @@ -33,8 +31,6 @@ @Immutable public final class CelValidationResult { - private static final Joiner JOINER = Joiner.on('\n'); - @SuppressWarnings("Immutable") private final @Nullable Throwable failure; @@ -115,7 +111,7 @@ public ImmutableList getAllIssues() { /** Convert all issues to a human-readable string. */ public String getIssueString() { - return JOINER.join(Iterables.transform(issues, iss -> iss.toDisplayString(source))); + return CelIssue.toDisplayString(issues, source); } /** @@ -131,7 +127,7 @@ public String getDebugString() { /** Convert the {@code CelIssue}s with {@code ERROR} severity to an error string. */ public String getErrorString() { - return JOINER.join(Iterables.transform(getErrors(), error -> error.toDisplayString(source))); + return CelIssue.toDisplayString(getErrors(), source); } private static boolean issueIsError(CelIssue iss) { diff --git a/policy/src/main/java/dev/cel/policy/BUILD.bazel b/policy/src/main/java/dev/cel/policy/BUILD.bazel index 407ce2f4..c13bcf0c 100644 --- a/policy/src/main/java/dev/cel/policy/BUILD.bazel +++ b/policy/src/main/java/dev/cel/policy/BUILD.bazel @@ -109,7 +109,7 @@ java_library( ":source", ":validation_exception", ":value_string", - "//common:source", + "//common:compiler_common", "//common/internal", "@maven//:com_google_guava_guava", "@maven//:org_yaml_snakeyaml", @@ -217,6 +217,7 @@ java_library( ":parser_context", ":source", ":validation_exception", + "//common:compiler_common", "//common/internal", "@maven//:com_google_guava_guava", "@maven//:org_yaml_snakeyaml", @@ -265,7 +266,7 @@ java_library( deps = [ ":policy", ":value_string", - "//common:source", + "//common:compiler_common", ], ) @@ -302,7 +303,6 @@ java_library( ":value_string", "//common", "//common:compiler_common", - "//common:source", "//common/internal", "@maven//:com_google_guava_guava", "@maven//:org_yaml_snakeyaml", diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java b/policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java index 4fb81f42..c2f1516d 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java @@ -15,9 +15,7 @@ package dev.cel.policy; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.collect.ImmutableList.toImmutableList; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.CanIgnoreReturnValue; import dev.cel.bundle.Cel; @@ -145,7 +143,6 @@ private static CelAbstractSyntaxTree newErrorAst() { } private static final class CompilerContext { - private static final Joiner JOINER = Joiner.on('\n'); private final ArrayList issues; private final ArrayList newVariableDeclarations; private final CelPolicySource celPolicySource; @@ -170,10 +167,7 @@ private boolean hasError() { } private String getIssueString() { - return JOINER.join( - issues.stream() - .map(iss -> iss.toDisplayString(celPolicySource)) - .collect(toImmutableList())); + return CelIssue.toDisplayString(issues, celPolicySource); } private CompilerContext(CelPolicySource celPolicySource) { diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyYamlConfigParser.java b/policy/src/main/java/dev/cel/policy/CelPolicyYamlConfigParser.java index 01dfc77b..d8955fe2 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicyYamlConfigParser.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicyYamlConfigParser.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import dev.cel.common.CelIssue; import dev.cel.common.internal.CelCodePointArray; import dev.cel.policy.CelPolicyConfig.ExtensionConfig; import dev.cel.policy.CelPolicyConfig.FunctionDecl; @@ -339,8 +340,9 @@ private CelPolicyConfig parseYaml(String source, String description) .setPositionsMap(ctx.getIdToOffsetMap()) .build(); - if (ctx.hasError()) { - throw new CelPolicyValidationException(ctx.getIssueString(configSource)); + if (!ctx.getIssues().isEmpty()) { + throw new CelPolicyValidationException( + CelIssue.toDisplayString(ctx.getIssues(), configSource)); } return policyConfig.setConfigSource(configSource).build(); diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java index 5d36185a..0972451f 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java @@ -20,13 +20,14 @@ import static dev.cel.policy.YamlHelper.assertYamlType; import com.google.common.collect.ImmutableSet; -import dev.cel.common.Source; +import dev.cel.common.CelIssue; import dev.cel.common.internal.CelCodePointArray; import dev.cel.policy.CelPolicy.Match; import dev.cel.policy.CelPolicy.Match.Result; import dev.cel.policy.CelPolicy.Variable; import dev.cel.policy.ParserContext.PolicyParserContext; import dev.cel.policy.YamlHelper.YamlNodeType; +import java.util.List; import java.util.Map; import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; @@ -76,8 +77,9 @@ private CelPolicy parseYaml() throws CelPolicyValidationException { CelPolicy celPolicy = parsePolicy(this, node); - if (ctx.hasError()) { - throw new CelPolicyValidationException(ctx.getIssueString(celPolicy.policySource())); + if (!ctx.getIssues().isEmpty()) { + throw new CelPolicyValidationException( + CelIssue.toDisplayString(ctx.getIssues(), celPolicy.policySource())); } return celPolicy; @@ -296,13 +298,8 @@ public void reportError(long id, String message) { } @Override - public String getIssueString(Source source) { - return ctx.getIssueString(source); - } - - @Override - public boolean hasError() { - return ctx.hasError(); + public List getIssues() { + return ctx.getIssues(); } @Override diff --git a/policy/src/main/java/dev/cel/policy/ParserContext.java b/policy/src/main/java/dev/cel/policy/ParserContext.java index 385a022c..13b81641 100644 --- a/policy/src/main/java/dev/cel/policy/ParserContext.java +++ b/policy/src/main/java/dev/cel/policy/ParserContext.java @@ -14,10 +14,11 @@ package dev.cel.policy; -import dev.cel.common.Source; +import dev.cel.common.CelIssue; import dev.cel.policy.CelPolicy.Match; import dev.cel.policy.CelPolicy.Rule; import dev.cel.policy.CelPolicy.Variable; +import java.util.List; import java.util.Map; /** @@ -40,9 +41,7 @@ public interface ParserContext { void reportError(long id, String message); - String getIssueString(Source source); - - boolean hasError(); + List getIssues(); Map getIdToOffsetMap(); diff --git a/policy/src/main/java/dev/cel/policy/YamlParserContextImpl.java b/policy/src/main/java/dev/cel/policy/YamlParserContextImpl.java index 613aca53..d84b9f83 100644 --- a/policy/src/main/java/dev/cel/policy/YamlParserContextImpl.java +++ b/policy/src/main/java/dev/cel/policy/YamlParserContextImpl.java @@ -14,18 +14,16 @@ package dev.cel.policy; -import static com.google.common.collect.ImmutableList.toImmutableList; import static dev.cel.policy.YamlHelper.ERROR; import static dev.cel.policy.YamlHelper.assertYamlType; -import com.google.common.base.Joiner; import dev.cel.common.CelIssue; import dev.cel.common.CelSourceLocation; -import dev.cel.common.Source; import dev.cel.common.internal.CelCodePointArray; import dev.cel.policy.YamlHelper.YamlNodeType; import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.yaml.snakeyaml.DumperOptions; import org.yaml.snakeyaml.nodes.Node; @@ -34,8 +32,6 @@ /** Package-private class to assist with storing policy parsing context. */ final class YamlParserContextImpl implements ParserContext { - private static final Joiner JOINER = Joiner.on('\n'); - private final ArrayList issues; private final HashMap idToLocationMap; private final HashMap idToOffsetMap; @@ -48,14 +44,8 @@ public void reportError(long id, String message) { } @Override - public String getIssueString(Source source) { - return JOINER.join( - issues.stream().map(iss -> iss.toDisplayString(source)).collect(toImmutableList())); - } - - @Override - public boolean hasError() { - return !issues.isEmpty(); + public List getIssues() { + return issues; } @Override