[Feature] Support evaluating dynamic messages with dynamic descriptors

[Kmotiko]

Hi,

First of all, I would like to apologize for that the issue #333 I posted was incorrect.
I have once again confirmed my program behavior, so let me ask a question again.

I tried to execute cel expression against a map in DynamicMessage built with a Descriptor which created from FileDescriptorSet at run time, and then that java process resulted into dev.cel.runtime.CelEvaluationException.
The exception message is like bellow.

 Unexpected exception thrown: dev.cel.runtime.CelEvaluationException: evaluation error at <input>:8: class com.google.protobuf.DynamicMessage cannot be cast to class com.google.protobuf.MapEntry (com.google.protobuf.DynamicMessage and com.google.protobuf.MapEntry are in unnamed module of loader 'app')

My question is that are there any way to execute a cel expression to map in DynamicMessage built with a Descriptor created at run time or are there any plan to support that case in cel-java.

Description

It seems that cel-java try to convert input DynamicMessage to a generated Message, probably a Message class generated from protobuf definition, with DynamicProto::maybeAdaptDynamicMessage but when build a Descriptor from FileDescriptorSet and use it to construct DynamicMessage to pass to eval method's argument, cel-java can not load a generated Message and so use DynamicMessage::Builder as maybeBuilder that is local variable in maybeAdaptDynamicMessge.
I'm not familiar with the DynamicMessage implementation, but from the behavior it appears that DynamicMessage::Builder treats the map field as a list of DynamicMessages, then the map field in Message returned by maybeAdaptDynamicMessage will also be the list of DynamicMessages and cause exception of class cast.

If use a generated Message's Builder to build Message or use generated Message's Descriptor to build DynamicMessage and pass it to eval method as argument, cel-java will be able to acquire a generated Message's Builder at DynamicProto::maybeAdaptDynamicMessage method and it treat map field as Map, so Exception will not happen.

To Reproduce

To reproduce, create descriptor and execute cel expression with following steps.

1. define protobuf message and convert it to FileDescriptorSet format using protoc as preparation
2. load the FileDescriptorSet and build a Descriptor from it in java code
3. build CelRuntime::Program using the Descriptor created in step 2
4. create DynamicMessage::Builder with the Descriptor which generated in step 2, and parse json to DynamicMessage
5. call CelRuntime.Program::eval and pass DynamicMessage built at step4
6. Exception will be raised

To do that, I write test code and put here.

[l46kok]

Hi,

First it'd be helpful for me to understand the larger context of what you're trying to achieve here. From the test code, it seems like you're trying to deal with JSON data. The canonical way of achieving that is to leverage google.protobuf.Value (or google.protobuf.Struct). There will be a section added for this in the codelabs in the near future (currently being reviewed: #349). I think this alone may make this question moot.

To answer the question -- the reason why you're running into this specific issue is because two semantically equal descriptors that differ in binary are present in the runtime: https://github.com/google/cel-java/blob/main/common/src/main/java/dev/cel/common/internal/DefaultInstanceMessageFactory.java#L78-L82. When constructing a default protobuf message, we rely on java reflection to lookup the descriptor to construct a default instance with but we try to ensure we're using the right descriptor for this work. The fix here is to not load the same file descriptor when you already have it available in memory (I realize you're probably doing this to reproduce the issue, but this work shouldn't be necessary). You should see your test case pass if you just pass in Test.getDescriptor().getFile()) in directly rather than reading it from the file.

As an aside -- I noticed that you're enabling CelValue and optional syntax in CelOptions. Keep in mind that 1) CelValue is currently experimental, as noted in the option, so you may run into some rough edges (but it is something we want to make the default for the runtime in the future) and 2) the preferred way of enabling optionals is through adding the library to your environment.

[Kmotiko]

@l46kok
Thank you for your reply.

At first, sorry for not explaining enough my situation.
Let me explain again. What I want do is following.

1. schemas written with protocol buffer is managed outside of java process
2. java program receive json value as a target value of cel expression and dinamically load a Descriptor for the value at runtime from schemas describe in step 1
3. build a DynamicMessage from the json value and CelRuntime::Program using the Descriptor loaded at step 2.
4. call eval method of the CelRuntime::Program and pass the DynamicMessage built at step2 as argument.

In step 3, input value’s schema is defined by a Descriptor and so I parse the value and convert it to DynamicMessage with the Descriptor.

In my use case, runtime process dynamically load Descriptors and does not include generated Messages, so can not load a Descriptor from a generated Message like Test.getDescriptor(). In the repository for reproduction, I added generated Messages under the directory src/main/java/org/example/proto to write the test code for the normal case, but it is not necessary to describe my problem situation. Also, in my test code I made unit test named CelForDynamicMessageTest not depends on src/main/java/org/example/proto, where generated messages located in, by src/test/java/org/example/cel/dynamicmessage/BUILD.bazel setting, so it was intended to be that test process does not load generated Messages at runtime and resulted to same situation described above. However, these settings were difficult to understand and it made confusing. I apologize for this. I simplified my test code and push it here.

In my use case conditions, DefaultInstanceMessageFactory will not be able to load defaultInstance and it will be null: https://github.com/google/cel-java/blob/v0.5.1/common/src/main/java/dev/cel/common/internal/DefaultInstanceMessageFactory.java#L231-L234, https://github.com/google/cel-java/blob/v0.5.1/common/src/main/java/dev/cel/common/internal/DefaultInstanceMessageFactory.java#L75-L77.
As a result, DynamicProto::maybeAdaptDynamicMessage will use DynamicMessage::Builder as maybeBuilder: https://github.com/google/cel-java/blob/v0.5.1/common/src/main/java/dev/cel/common/internal/DynamicProto.java#L104-L115. That is probably why, a map field remain as a list of DynamicMessages and eval method ends with cast error.

Sorry if I’m wrong, but It looks that a map field in DynamicMessage constructed using preveious described steps is consists of list of DynamicMessage which contains key and value fields, so to cast it to map, it may be necessary to execute some kind of conversion process, for example to take a key and value from each element of the list and convert it to an MapEntry. Because of these reasons, I asked that if there is a way to execute the cel expression in my use case or if cel-java plan to support this situation.

Thank you also for your advise about CelOptions. I tried several settings, and remaining enableOptionalSyntax option but it is not necessary. Regarding CelValue, if I set enableCelValue to false, result of cel expression is like this and looks same as when set it true.
Unexpected exception thrown: dev.cel.runtime.CelEvaluationException: evaluation error at <input>:eight: class com.google.protobuf.DynamicMessage cannot be cast to class com.google.protobuf.MapEntry (com.google.protobuf.DynamicMessage and com.google.protobuf.MapEntry are in unnamed module of loader ‘app’)
When disable CelValue, the exception is looks raise around here: https://github.com/google/cel-java/blob/v0.5.1/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java#L216-L220.
With set enableCelValeu to true, exception was happen here: https://github.com/google/cel-java/blob/v0.5.1/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java#L205-L207.

[l46kok]

Oh I follow now, thanks for elaborating. CEL for the most part expects non-dynamic descriptors as constructing protobuf messages (structs) in expressions is baked into the specification (example: Test {} would return Test.getDefaultInstance()). We only support dynamic messages to the extent where a concrete descriptor is present in the environment. So to support your case of mapping a dynamic message into the fields of dynamic descriptor would also mean we have to be able to somehow construct a default instance from it, and I don't really see a great way of achieving this. One area we could improve on is providing a more descriptive error message to make this clear.

One other thought I had is you could technically override the manner of how to construct the messages via setTypeFactory in the runtime builder by using the fully qualified message name. But if you don't have the generated messages to begin with, I don't think this will really help. I thought I'd mention it just in case.

[l46kok]

Thinking about this further -- what I've mentioned regarding CEL-Java expecting concrete descriptors has been traditionally true, but we might be able to extend the support to dynamic descriptors as well. In discussing with the team, support for fully dynamic messages and descriptors may already be there for golang and C++ stacks via protobuf reflection. The biggest challenge would be to ensure that the usual CEL semantics is fully supported in CEL-Java. We'll explore this further and see what can be done.

[Kmotiko]

Thanks for your explanation for the specification and suggesting ways to support my situation.

CEL for the most part expects non-dynamic descriptors as constructing protobuf messages (structs) in expressions is baked into the specification (example: Test {} would return Test.getDefaultInstance()).

I may not fully understand or misunderstand the cel-spec. If it's alright, let me ask about this comment. Does this mean cel-spec define that to execute a cel expression against a message, a runtime should load generated messages and should be able to call getDefaultInstance method correspoinding to that message?

With dynamically loaded Descriptors, I tried to run cel expressions which access to various types of fields, for example enum, repeated, and map, and those were success in case except map. Therefore it would be very appreciated if cel-java support to run the expression for a map in a DynamicMessage with Descriptors loaded at runtime.

[l46kok]

Ah I meant to say CEL-Java on that, my mistake. CEL-Spec only specifies that an expression M {} must return an empty message. For CEL-Java, that historically meant ConcreteType.getDefaultInstance(). In cases where dynamic messages are provided as an input, we make all possible attempts to convert it into a concrete type as long as we have the descriptor with generated message available for it.

I think your observations about dynamic message is correct -- I think it should behave the same except for map (and only for cases where JSON is parsed into dynamic message, because it converts the json map into a dynamic message to preserve ordering post-serialization). I just need to verify the behavior against few other things such as function dispatch before we can canonically support this case.

[Kmotiko]

Thank you for your response. I understood what you are saying about cel-java specification.
Also, I really appriciate for your handling to support the situation I reported.