-
Notifications
You must be signed in to change notification settings - Fork 17
Upgrade CEL from 0.10.1 to 0.11.0 #345
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
Conversation
Update to the latest CEL version, which required changing to use internal CEL null/bytes types to fix an overloaded method and the conformance tests. While `evaluateCanonicalTypesToNativeValues` isn't the default yet, according to the release notes it will be the default soon so switched everything to use it.
| api(libs.protobuf.java) | ||
| implementation(libs.cel) { | ||
| // https://github.com/google/cel-java/issues/748 | ||
| exclude(group = "com.google.protobuf", module = "protobuf-javalite") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream issue is fixed in this release.
| BytesIn invalid = BytesIn.newBuilder().setVal(ByteString.copyFromUtf8("bar")).build(); | ||
| ValidationResult validate = validator.validate(invalid); | ||
| assertThat(validate.isSuccess()).isTrue(); | ||
| assertThat(validate.getViolations()).isEmpty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just showing expected true, got false isn't great for debugging. Made it use isEmpty so it will print the violations.
| } | ||
| for (int i = 0; i < param.size(); i++) { | ||
| if (param.byteAt(i) != receiver.byteAt(i)) { | ||
| byte[] paramBytes = param.toByteArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there was a byteAt method on CelByteString to avoid having to allocate a byte array to access a single byte.
| .addCompilerLibraries(validateLibrary) | ||
| .addRuntimeLibraries(validateLibrary) | ||
| .setOptions( | ||
| CelOptions.DEFAULT.toBuilder().evaluateCanonicalTypesToNativeValues(true).build()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This opts in to the new behavior which uses CEL types instead of Protobuf types for NullValue / ByteString. This is apparently coming soon as the default, so since we had to update our overloads to use the new types just opted in to migrate everything to use them.
|
|
||
| @Override | ||
| public String toString() { | ||
| return proto.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made debugging more painful - added a toString which will dump the contents of the proto Validation message.
Update to the latest CEL version, which required changing to use internal CEL null/bytes types to fix an overloaded method and the conformance tests.
While
evaluateCanonicalTypesToNativeValuesisn't the default yet, according to the release notes it will be the default soon so switched everything to use it.