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.

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

Pull getIssueString logic into CelIssue #385

Merged
merged 1 commit into from
Jul 3, 2024
Merged
Changes from all commits
Commits
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
Pull getIssueString logic into CelIssue
PiperOrigin-RevId: 649196570
  • Loading branch information
l46kok authored and copybara-github committed Jul 3, 2024

Verified

This commit was signed with the committer’s verified signature.
slarse Simon Larsén
commit e85ee166dc090d0d33342fb8eb2b74ac39ed2646
11 changes: 4 additions & 7 deletions checker/src/test/java/dev/cel/checker/CelIssueTest.java
Original file line number Diff line number Diff line change
@@ -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<CelIssue> 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 '<EOF>'"));
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,"
11 changes: 11 additions & 0 deletions common/src/main/java/dev/cel/common/CelIssue.java
Original file line number Diff line number Diff line change
@@ -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<CelIssue> 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.
14 changes: 4 additions & 10 deletions common/src/main/java/dev/cel/common/CelValidationException.java
Original file line number Diff line number Diff line change
@@ -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<CelIssue> errors) {

private static String safeJoinErrorMessage(CelSource source, List<CelIssue> errors) {
if (errors.size() <= MAX_ERRORS_TO_REPORT) {
return JOINER.join(Iterables.transform(errors, error -> error.toDisplayString(source)));
return CelIssue.toDisplayString(errors, source);
}

List<CelIssue> 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. */
8 changes: 2 additions & 6 deletions common/src/main/java/dev/cel/common/CelValidationResult.java
Original file line number Diff line number Diff line change
@@ -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<CelIssue> 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) {
6 changes: 3 additions & 3 deletions policy/src/main/java/dev/cel/policy/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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",
Original file line number Diff line number Diff line change
@@ -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<CelIssue> issues;
private final ArrayList<CelVarDecl> 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) {
Original file line number Diff line number Diff line change
@@ -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();
17 changes: 7 additions & 10 deletions policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java
Original file line number Diff line number Diff line change
@@ -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<CelIssue> getIssues() {
return ctx.getIssues();
}

@Override
7 changes: 3 additions & 4 deletions policy/src/main/java/dev/cel/policy/ParserContext.java
Original file line number Diff line number Diff line change
@@ -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<T> {

void reportError(long id, String message);

String getIssueString(Source source);

boolean hasError();
List<CelIssue> getIssues();

Map<Long, Integer> getIdToOffsetMap();

16 changes: 3 additions & 13 deletions policy/src/main/java/dev/cel/policy/YamlParserContextImpl.java
Original file line number Diff line number Diff line change
@@ -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<Node> {

private static final Joiner JOINER = Joiner.on('\n');

private final ArrayList<CelIssue> issues;
private final HashMap<Long, CelSourceLocation> idToLocationMap;
private final HashMap<Long, Integer> 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<CelIssue> getIssues() {
return issues;
}

@Override
Loading