Skip to content

Commit

Permalink
Bump cel-spec + googleapi submodules
Browse files Browse the repository at this point in the history
Bump cel-spec submodule to [v0.13.0](https://github.com/google/cel-spec/releases/tag/v0.13.0), googleapi submodule to
[f2d78630d2c1d5e20041dfff963e093de9298e4d](googleapis/googleapis@f2d7863).

This was necessary due to recent build errors caused by missing (old) artifacts for the conformance bazel build.

The submodule bumps bring a bunch of new conformance tests as well, which unveiled that some functionality that was not present in CEL-Go when CEL-Java was created, is required by the CEl-Spec.

Summary of changes:
* Bugfix in `o.p.c.checker.Standard` that fixes a type resolution error for equals and not-equals functions, when both parameters need to be up-casted.
* Changes in many types (in `o.p.c.common.types.*T` classes) to automatically convert between numeric and null types.
  * equal and compare with a null and a non-null type no longer fail, but return `False`
  * equal and compare between different numeric types no longer fail, but return "the right" result
* Fix retrieval of milliseconds from `Duration` - must only return the milliseconds within the second
* Un-ignore a bunch of conformance tests that pass fine now

A few conformance tests have been added to `InterpreterTest`, but the majority of tests is left in the conformance tests.
  • Loading branch information
snazy committed Nov 17, 2023
1 parent f244ee9 commit a46e549
Show file tree
Hide file tree
Showing 38 changed files with 825 additions and 234 deletions.
7 changes: 5 additions & 2 deletions conformance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ The CEL-spec conformance test suite is written in Go and uses the bazel build to

Required tools:
* [Bazel build tool](https://bazel.build/)

On Ubuntu run `apt-get install bazel-bootstrap`

See [Bazel web site](https://bazel.build/install) for installation instructions.
Do **not** use the `bazel-bootstrap` package on Ubuntu, because it comes with
pre-compiled classes that are built with Java 17 or newer, preventing it from
running bazel using older Java versions.
* gcc

On Ubuntu run `apt-get install gcc`
Expand Down
9 changes: 8 additions & 1 deletion conformance/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ plugins {

apply<ProtobufPlugin>()

sourceSets.main { java.srcDir(layout.buildDirectory.dir("generated/source/proto/main/java")) }
sourceSets.main {
java.srcDir(layout.buildDirectory.dir("generated/source/proto/main/java"))
java.srcDir(layout.buildDirectory.dir("generated/source/proto/main/grpc"))
}

dependencies {
implementation(project(":cel-core"))
Expand Down Expand Up @@ -58,6 +61,10 @@ configure<ProtobufExtension> {
// Download from repositories
artifact = "com.google.protobuf:protoc:${libs.versions.protobuf.get()}"
}
plugins {
this.create("grpc") { artifact = "io.grpc:protoc-gen-grpc-java:${libs.versions.grpc.get()}" }
}
generateProtoTasks { all().configureEach { this.plugins.create("grpc") {} } }
}

// The protobuf-plugin should ideally do this
Expand Down
24 changes: 15 additions & 9 deletions conformance/run-conformance-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,30 +46,33 @@ cel_java_skips=(
# nested protobuf-object-structure, which gets rejected during gRPC/protobuf request
# deserialization. Just skip those tests.
"--skip_test=parse/nest/message_literal"
"--skip_test=parse/repeat/index"
# Proto equality specialties don't seem to be in effect for Java
"--skip_test=comparisons/eq_wrapper/eq_proto_nan_equal"
"--skip_test=comparisons/ne_literal/ne_proto_nan_not_equal"

# TODO Actual known issux to fix, a protobuf Any returned via this test is wrapped twice (Any in Any).
# TODO Actual known issue to fix, a protobuf Any returned via this test is wrapped twice (Any in Any).
"--skip_test=dynamic/any/var"
)

cel_go_skips=(
"--skip_test=comparisons/eq_literal/not_eq_list_false_vs_types,not_eq_map_false_vs_types"
"--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error"
"--skip_test=comparisons/eq_literal/not_eq_list_false_vs_types,not_eq_map_false_vs_types"
"--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error"
"--skip_test=dynamic/int32/field_assign_proto2_range,field_assign_proto3_range"
"--skip_test=dynamic/uint32/field_assign_proto2_range,field_assign_proto3_range"
"--skip_test=dynamic/float/field_assign_proto2_range,field_assign_proto3_range"
"--skip_test=dynamic/value_null/literal_unset,field_read_proto2_unset,field_read_proto3_unset"
"--skip_test=enums/legacy_proto2/assign_standalone_int_too_big,assign_standalone_int_too_neg"
"--skip_test=enums/legacy_proto3/assign_standalone_int_too_big,assign_standalone_int_too_neg"
"--skip_test=enums/strong_proto2"
"--skip_test=enums/strong_proto3"
"--skip_test=fields/qualified_identifier_resolution/map_key_float,map_key_null,map_value_repeat_key"
# This conformance test is invalid nowadays
"--skip_test=fields/qualified_identifier_resolution/map_key_float"
# Unclear why the 'to_json_string' is expected to return a string, unlike the preceding to_json_number test.
"--skip_test=wrappers/uint64/to_json_string"
# TODO implement proper "toJson" at some point
"--skip_test=wrappers/field_mask/to_json"
"--skip_test=wrappers/timestamp/to_json"
"--skip_test=wrappers/empty/to_json"
)

test_files=(
"tests/simple/testdata/plumbing.textproto"
"tests/simple/testdata/basic.textproto"
"tests/simple/testdata/comparisons.textproto"
"tests/simple/testdata/conversions.textproto"
Expand All @@ -83,11 +86,14 @@ test_files=(
"tests/simple/testdata/macros.textproto"
"tests/simple/testdata/namespace.textproto"
"tests/simple/testdata/parse.textproto"
"tests/simple/testdata/plumbing.textproto"
"tests/simple/testdata/proto2.textproto"
"tests/simple/testdata/proto3.textproto"
"tests/simple/testdata/string.textproto"
# TODO "tests/simple/testdata/string_ext.textproto"
"tests/simple/testdata/timestamps.textproto"
"tests/simple/testdata/unknowns.textproto"
"tests/simple/testdata/wrappers.textproto"
)

bazel-bin/tests/simple/simple_test_/simple_test \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,20 @@
import static org.projectnessie.cel.common.types.UnknownT.isUnknown;
import static org.projectnessie.cel.common.types.UnknownT.unknownOf;

import com.google.api.expr.v1alpha1.CheckRequest;
import com.google.api.expr.v1alpha1.CheckResponse;
import com.google.api.expr.v1alpha1.ConformanceServiceGrpc.ConformanceServiceImplBase;
import com.google.api.expr.conformance.v1alpha1.CheckRequest;
import com.google.api.expr.conformance.v1alpha1.CheckResponse;
import com.google.api.expr.conformance.v1alpha1.ConformanceServiceGrpc.ConformanceServiceImplBase;
import com.google.api.expr.conformance.v1alpha1.EvalRequest;
import com.google.api.expr.conformance.v1alpha1.EvalResponse;
import com.google.api.expr.conformance.v1alpha1.IssueDetails;
import com.google.api.expr.conformance.v1alpha1.ParseRequest;
import com.google.api.expr.conformance.v1alpha1.ParseResponse;
import com.google.api.expr.conformance.v1alpha1.SourcePosition;
import com.google.api.expr.v1alpha1.ErrorSet;
import com.google.api.expr.v1alpha1.EvalRequest;
import com.google.api.expr.v1alpha1.EvalResponse;
import com.google.api.expr.v1alpha1.ExprValue;
import com.google.api.expr.v1alpha1.IssueDetails;
import com.google.api.expr.v1alpha1.ListValue;
import com.google.api.expr.v1alpha1.MapValue;
import com.google.api.expr.v1alpha1.MapValue.Entry;
import com.google.api.expr.v1alpha1.ParseRequest;
import com.google.api.expr.v1alpha1.ParseResponse;
import com.google.api.expr.v1alpha1.SourcePosition;
import com.google.api.expr.v1alpha1.UnknownSet;
import com.google.api.expr.v1alpha1.Value;
import com.google.protobuf.Any;
Expand Down
1 change: 1 addition & 0 deletions conformance/src/main/proto/google/api/expr/conformance
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,22 @@
import static org.projectnessie.cel.TestExpr.ExprLiteral;
import static org.projectnessie.cel.Util.mapOf;

import com.google.api.expr.v1alpha1.CheckRequest;
import com.google.api.expr.v1alpha1.CheckResponse;
import com.google.api.expr.conformance.v1alpha1.CheckRequest;
import com.google.api.expr.conformance.v1alpha1.CheckResponse;
import com.google.api.expr.conformance.v1alpha1.ConformanceServiceGrpc;
import com.google.api.expr.conformance.v1alpha1.ConformanceServiceGrpc.ConformanceServiceBlockingStub;
import com.google.api.expr.conformance.v1alpha1.EvalRequest;
import com.google.api.expr.conformance.v1alpha1.EvalResponse;
import com.google.api.expr.conformance.v1alpha1.ParseRequest;
import com.google.api.expr.conformance.v1alpha1.ParseResponse;
import com.google.api.expr.v1alpha1.CheckedExpr;
import com.google.api.expr.v1alpha1.ConformanceServiceGrpc;
import com.google.api.expr.v1alpha1.ConformanceServiceGrpc.ConformanceServiceBlockingStub;
import com.google.api.expr.v1alpha1.Constant;
import com.google.api.expr.v1alpha1.Constant.ConstantKindCase;
import com.google.api.expr.v1alpha1.EvalRequest;
import com.google.api.expr.v1alpha1.EvalResponse;
import com.google.api.expr.v1alpha1.Expr;
import com.google.api.expr.v1alpha1.Expr.Call;
import com.google.api.expr.v1alpha1.Expr.ExprKindCase;
import com.google.api.expr.v1alpha1.ExprValue;
import com.google.api.expr.v1alpha1.ExprValue.KindCase;
import com.google.api.expr.v1alpha1.ParseRequest;
import com.google.api.expr.v1alpha1.ParseResponse;
import com.google.api.expr.v1alpha1.ParsedExpr;
import com.google.api.expr.v1alpha1.SourceInfo;
import com.google.api.expr.v1alpha1.Type;
Expand Down
1 change: 1 addition & 0 deletions core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ dependencies {

testImplementation(platform(libs.junit.bom))
testImplementation(libs.bundles.junit.testing)
testImplementation(libs.protobuf.java)
testRuntimeOnly(libs.junit.jupiter.engine)

jmhImplementation(libs.jmh.core)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,13 @@ Overloads.GreaterEqualsBytes, asList(Decls.Bytes, Decls.Bytes), Decls.Bool),
Decls.newFunction(
Operator.Equals.id,
Decls.newParameterizedOverload(
Overloads.Equals, asList(paramA, paramA), Decls.Bool, typeParamAList)));
Overloads.Equals, asList(paramA, paramB), Decls.Bool, typeParamABList)));

idents.add(
Decls.newFunction(
Operator.NotEquals.id,
Decls.newParameterizedOverload(
Overloads.NotEquals, asList(paramA, paramA), Decls.Bool, typeParamAList)));
Overloads.NotEquals, asList(paramA, paramB), Decls.Bool, typeParamABList)));

// Algebra.

Expand Down
10 changes: 7 additions & 3 deletions core/src/main/java/org/projectnessie/cel/common/types/BoolT.java
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,14 @@ public Val convertToType(Type typeVal) {
/** Equal implements the ref.Val interface method. */
@Override
public Val equal(Val other) {
if (!(other instanceof BoolT)) {
return noSuchOverload(this, "equal", other);
switch (other.type().typeEnum()) {
case Bool:
return Types.boolOf(b == ((BoolT) other).b);
case Null:
return False;
default:
return noSuchOverload(this, "equal", other);
}
return Types.boolOf(b == ((BoolT) other).b);
}

/** Negate implements the traits.Negater interface method. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.projectnessie.cel.common.types;

import static org.projectnessie.cel.common.types.BoolT.False;
import static org.projectnessie.cel.common.types.Err.newErr;
import static org.projectnessie.cel.common.types.Err.newTypeConversionError;
import static org.projectnessie.cel.common.types.Err.noSuchOverload;
Expand Down Expand Up @@ -162,10 +163,14 @@ public Val convertToType(Type typeValue) {
/** Equal implements the ref.Val interface method. */
@Override
public Val equal(Val other) {
if (!(other instanceof BytesT)) {
return noSuchOverload(this, "equal", other);
switch (other.type().typeEnum()) {
case Bytes:
return boolOf(Arrays.equals(b, ((BytesT) other).b));
case Null:
return False;
default:
return noSuchOverload(this, "equal", other);
}
return boolOf(Arrays.equals(b, ((BytesT) other).b));
}

/** Size implements the traits.Sizer interface method. */
Expand Down
88 changes: 59 additions & 29 deletions core/src/main/java/org/projectnessie/cel/common/types/DoubleT.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package org.projectnessie.cel.common.types;

import static org.projectnessie.cel.common.types.BoolT.False;
import static org.projectnessie.cel.common.types.Err.newTypeConversionError;
import static org.projectnessie.cel.common.types.Err.noSuchOverload;
import static org.projectnessie.cel.common.types.Err.rangeError;
Expand All @@ -31,7 +32,6 @@
import com.google.protobuf.Value;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.Objects;
import org.projectnessie.cel.common.types.ref.BaseVal;
import org.projectnessie.cel.common.types.ref.Type;
import org.projectnessie.cel.common.types.ref.TypeEnum;
Expand Down Expand Up @@ -68,6 +68,11 @@ private DoubleT(double d) {
this.d = d;
}

@Override
public double doubleValue() {
return d;
}

/** Add implements traits.Adder.Add. */
@Override
public Val add(Val other) {
Expand All @@ -77,20 +82,6 @@ public Val add(Val other) {
return doubleOf(d + ((DoubleT) other).d);
}

/** Compare implements traits.Comparer.Compare. */
@Override
public Val compare(Val other) {
if (!(other instanceof DoubleT)) {
return noSuchOverload(this, "compare", other);
}
double od = ((DoubleT) other).d;
if (d == od) {
// work around for special case of -0.0d == 0.0d (IEEE 754)
return IntZero;
}
return intOfCompare(Double.compare(d, od));
}

/** ConvertToNative implements ref.Val.ConvertToNative. */
@SuppressWarnings("unchecked")
@Override
Expand Down Expand Up @@ -141,17 +132,23 @@ public Val convertToType(Type typeValue) {
// (see https://github.com/google/cel-spec/blob/master/doc/langdef.md#numeric-values)
switch (typeValue.typeEnum()) {
case Int:
if (!Double.isFinite(d)) {
return rangeError(d, "int");
}
long r = (long) d; // ?? Math.round(d);
if (r == Long.MIN_VALUE || r == Long.MAX_VALUE) {
return rangeError(d, "int");
}
return intOf(r);
case Uint:
if (!Double.isFinite(d)) {
return rangeError(d, "uint");
}
// hack to support uint64
BigDecimal dec = new BigDecimal(d);
BigInteger bi = dec.toBigInteger();
if (d < 0 || bi.compareTo(MAX_UINT64) > 0) {
return rangeError(d, "int");
return rangeError(d, "uint");
}
return uintOf(bi.longValue());
case Double:
Expand All @@ -173,14 +170,51 @@ public Val divide(Val other) {
return doubleOf(d / ((DoubleT) other).d);
}

/** Compare implements traits.Comparer.Compare. */
@Override
public Val compare(Val other) {
switch (other.type().typeEnum()) {
case Uint:
case Int:
case Double:
Val converted = other.convertToType(type());
if (converted.type().typeEnum() == TypeEnum.Err) {
return converted;
}
double od = ((DoubleT) converted).d;
if (d == od) {
// work around for special case of -0.0d == 0.0d (IEEE 754)
return IntZero;
}
return intOfCompare(Double.compare(d, od));
default:
return noSuchOverload(this, "compare", other);
}
}

/** Equal implements ref.Val.Equal. */
@Override
public Val equal(Val other) {
if (!(other instanceof DoubleT)) {
return noSuchOverload(this, "equal", other);
switch (other.type().typeEnum()) {
case Uint:
case Int:
case Double:
case String:
Val converted = other.convertToType(type());
if (converted.type().typeEnum() == TypeEnum.Err) {
return converted;
}
double o = ((DoubleT) converted).d;
// TODO: Handle NaNs properly.
return boolOf(d == o);
case Null:
case Bytes:
case List:
case Map:
return False;
default:
return noSuchOverload(this, "equal", other);
}
/** TODO: Handle NaNs properly. */
return boolOf(d == ((DoubleT) other).d);
}

/** Multiply implements traits.Multiplier.Multiply. */
Expand Down Expand Up @@ -224,20 +258,16 @@ public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
if (!(o instanceof Val)) {
return false;
}
DoubleT doubleT = (DoubleT) o;
double od = ((DoubleT) o).d;
if (d == od) {
// work around for special case of -0.0d == 0.0d (IEEE 754)
return true;
}
return Double.compare(doubleT.d, d) == 0;
// Defer to CEL's equal functionality to allow heterogeneous numeric map keys
return equal((Val) o).booleanValue();
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), d);
// Used to allow heterogeneous numeric map keys
return (int) d;
}
}
Loading

0 comments on commit a46e549

Please sign in to comment.