-
Notifications
You must be signed in to change notification settings - Fork 16
Introduce MessageReflector to enable alternative protobuf runtimes #202
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
base: main
Are you sure you want to change the base?
Introduce MessageReflector to enable alternative protobuf runtimes #202
Conversation
|
Hey! We recently switched the CEL implementation, can you rebase. Also can you add a test target that runs the test against an alternate protobuf runtime? |
|
I'll rebase, no problem. As far as a test case, I had implemented the validation in protokt's repository itself. Would you like me to move that implementation here? Or pull in the protokt runtime as a test dependency? I'm open to whatever strategy you all want here but don't want to do the scaffolding work if you decide to structure it differently. For now, this PR just includes what we'd need to implement protovalidate within protokt (which does support it right now, but only through conversion to DynamicMessage). |
|
@srikrsna-buf I've rebased this PR on the current main branch and it passes all conformance tests. Working on porting that support to protokt so I can point to a working commit like in the PR description with this implementation. |
|
@srikrsna-buf Okay, I've updated the link to the protokt PR, which contains instructions to run the conformance tests there (after publishing this branch to Maven local). The new implementation still fails some conformance tests due to differences in value conversion for the new CEL implementation but I'm slowly bringing that number down. |
|
Hey, just checking in on this. Aside from the conflict, is this ready for review? |
|
@chrispine yes, thank you! Open to alternative API suggestions as well - this is just one way to get interoperation with other runtimes to work. |
| public Object celValue() { | ||
| return value.getMessage(); | ||
| } |
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 library needs to support the protobuf runtime, otherwise this will never be recognized as the protobuf message.
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.
I'm not sure I understand what you mean here - when the protovalidate runtime (this project) requests the value of this MessageValue object using this method, it's intending to pass the value into the CEL runtime.
Looking at the usage today in CelPrograms:
CelVariableResolver bindings = Variable.newThisVariable(val.value(Object.class));where the return value of value(Object.class) is just this.value, which is a protobuf Message.
After this PR, this becomes:
CelVariableResolver bindings = Variable.newThisVariable(val.celValue());which is a more specific request than "cast this to an Object" - it's "please give me this value as it can be interpreted by CEL." The answer happens to be the same, as the new implementation returns value.getMessage(), which is the same protobuf Message value as before.
In the specific case of protokt, I have not implemented custom support for CEL to read protokt's protokt.v1.Message format, so when this is requested from a protokt message we'll wrap the message in a DynamicMessage that CEL can read. It's an area we can optimize further later by providing reflection support for CEL.
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.
In the specific case of protokt, I have not implemented custom support for CEL to read protokt's
protokt.v1.Messageformat, so when this is requested from a protokt message we'll wrap the message in a DynamicMessage that CEL can read. It's an area we can optimize further later by providing reflection support for CEL.
This is what I was referring to, unless the CEL library itself supports alternate runtimes, I am not sure how we can fully support this.
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.
Right now, when asked to evaluate a Message for CEL, the protokt runtime converts it to a protobuf-java DynamicMessage. This is the same thing it does today for the root message object to enable protovalidate today, but is suboptimal since we can access basic fields (i.e., those used in validations evaluated from this library instead of within CEL) without converting the entire message to a DynamicMessage.
The plan was: If we benchmark and find that full message conversion is common and a bottleneck, we'll build native CEL support for protokt's Message interface as an extension in CEL.
Rehash of #132 since that PR ended up with some bad merge conflicts.
See #80
See open-toast/protokt#402 for what the implementation looks like given this PR