Conversation
When collecting dependencies for types in cel-java, for some reason, only public imports are considered. Because of this, if a message type from another file is referenced within an expression, the type will not be resolved, leading to strange, unexpected errors. In most cases, things will work purely by accident, obscuring the broken resolution behavior. We should try to follow this up with an upstream fix as soon as possible.
ghost
requested review from
timostamm
left a comment
timostamm
left a comment
There was a problem hiding this comment.
Looks like registering a Protobuf type with ProtoTypeRegistry will register all types declared in the containing file (including public imports, which make imports appear as local declarations).
Seems like it might be a deliberate choice? Either way, registering all referenced types regardless of file boundaries is the behavior I'd expect in the context of protovalidate.
| private void collectDependencies(Set<Descriptor> dependencyTypes, Descriptor desc) { | ||
| dependencyTypes.add(desc); | ||
| for (FieldDescriptor field : desc.getFields()) { | ||
| if (field.getJavaType() != FieldDescriptor.JavaType.MESSAGE) { |
There was a problem hiding this comment.
Just for other readers: This includes edition delimited / proto2 group encoding, and excludes map entries.
See:
https://github.com/protocolbuffers/protobuf/blob/v31.1/java/core/src/main/java/com/google/protobuf/Descriptors.java#L1299
https://github.com/protocolbuffers/protobuf/blob/v31.1/java/core/src/main/java/com/google/protobuf/Descriptors.java#L1657
There was a problem hiding this comment.
Hmmm, now that you mention it though, I think I probably Do have to cover the map entry case. I'm not sure why I didn't do that last night, maybe I was yet again making the mistake of thinking it's fine since it's a message on the wire. (I keep doing that even though I know it doesn't look that way in the reflection API.)
There was a problem hiding this comment.
We don't care about the synthetic message for map entries, but we do care about the message M in map<int32, M>.
There was a problem hiding this comment.
Yeah, exactly. However, guess what? I just debugged this thoroughly and... this code actually does traverse the synthetic map entry and therefore the value message in maps. I don't really know why, but I'm going to just add the tests showing that it works (validated by stepping through to make sure it's really not working by accident) and just call it for now since this is a rather urgent fix. At least in Java protobuf, the map fields actually do have type MESSAGE.
2 participants
When collecting dependencies for types in cel-java, for some reason, only public imports are considered. Because of this, if a message type from another file is referenced within an expression, the type will not be resolved, leading to strange, unexpected errors. In most cases, things will work purely by accident, obscuring the broken resolution behavior.
We should try to follow this up with an upstream fix as soon as possible.