-
Notifications
You must be signed in to change notification settings - Fork 3
Expand celType to account for Any
#255
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
packages/cel/src/type.ts
Outdated
| /** | ||
| * Returns true if v satisfies type t. | ||
| * | ||
| * For lists and maps |
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.
Need to complete sentence or drop the line :)
I would prefer that we explain how DYN behaves here, even if "satisfies" gives an indication.
| if (typeName === ValueSchema.typeName) { | ||
| const value = anyUnpack(v.message, ValueSchema); | ||
| return value ? valueToType(value) : CelScalar.NULL; | ||
| } |
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.
For a google.protobuf.Any containing a message other than google.protobuf.Value, celTypeOfMessage will still yield an object type google.protobuf.Any. Shouldn't we unwrap as well? We'll need an overload to function objectType that doesn't require a DescMessage.
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.
Yep missed that in a refactor, done in 46d561e
| function celTypeOfMessage(v: ReflectMessage): CelType { | ||
| let typeName = v.desc.typeName; | ||
| if (isReflectMessageAny(v)) { | ||
| typeName = typeUrlToName(v.message.typeUrl); |
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.
We'll error for an empty google.protobuf.Any.
I am not sure what the correct behavior is - this isn't explicitly specified.
For now, let's add a test to capture the behavior. We should have at least a few tests for function celType, including the behavior for Any we're fixing here and including the edge case behavior for an empty Any.
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.
Isn't it the same as unwrapping an Any? This is consistent with other functions like equal
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.
The CEL spec says:
All google.protobuf.Any typed fields are unpacked before comparison, unless the type_url cannot be resolved, in which case the comparison falls back to byte equality.
equals from @bufbuild/protobuf follows that, and considers two empty Any messages equal.
bufbuild/protobuf-es#1338
Today
celTypeis only used in thetypeCEL function and error messages. It is also exported. If an any were to be passed tocelTypeit will report it as agoogle.protobuf.Anywhich is different from whattypereturns. This change modifiescelTypeto account for Any.