Add serde support for google.protobuf.Any
#18
Conversation
|
Would you be able to rebase this on to current main 🙏 |
|
|
||
| const NAME: &str = "google.protobuf.Any"; | ||
|
|
||
| let value = <crate::Value as prost::Message>::decode(self.value.clone()).map_err(|_| { |
There was a problem hiding this comment.
Shouldn't this perform a match on self.type and decode to the corresponding well known type if it matches? This seems to be assuming all google.protobuf.Any payloads are encoded as google.protobuf.Value?
There was a problem hiding this comment.
decode to the corresponding well known type if it matches?
I'm not sure that I follow. If we only decoded matching "well-known" values, then this type can't be used for most projects. For example, in the project I want to use this library in, we're using the data-plane-api which uses google.protobuf.Any to store typed configs, those aren't going to be known to this project, so that makes decoding those impossible.
This seems to be assuming all google.protobuf.Any payloads are encoded as google.protobuf.Value?
Almost correct, it assumes that any payload can be decoded as a google.protobuf.Value, which to my understanding (which could be wrong), matches any protobuf value.
There was a problem hiding this comment.
If we only decoded matching "well-known" values, then this type can't be used for most projects.
Yes, this is a major limitation of protobuf.Any and by extension protobuf as a whole. A payload cannot be decoded without knowledge of the schema, in fact the wire format has no concept of the name of a field at all see here.
so that makes decoding those impossible.
If the client understands the types it can decode the value field to the concrete message type, we do this in IOx for operations.
If you wish to handle any protobuf payload, you will need some channel to get the message descriptors (e.g. gRPC reflection) and then a client library that can use these descriptors to decode the messages, but I'm not sure of a Rust implementation of protobuf reflection...
which to my understanding (which could be wrong), matches any protobuf value.
The wire encoding of a protobuf message is tightly coupled to its schema, whilst any message with the same layout of fields and types could theoretically be decoded as protobuf.Value, it would only be correct to decode to a payload that was an actual encoded protobuf.Value.
Edit: I also found this with some googling that might be useful
| }; | ||
| } | ||
| AnyField::Unknown(key) => { | ||
| if map.contains_key(&key) { |
There was a problem hiding this comment.
Surely this should be an error?
There was a problem hiding this comment.
What do you mean? It does return an error?
There was a problem hiding this comment.
I meant any unknown field should be an error, regardless of if it already exists in the map?
| return Err(serde::de::Error::duplicate_field(VALUE_FIELD)); | ||
| } | ||
|
|
||
| value = if let Ok(bytes) = |
There was a problem hiding this comment.
I think this should be inspecting the type_url to work out what the value payload is and how to treat it, I think this assumes that anything that is valid base64 is a bytes payload?
I think you're mixing concepts here, just because a protobuf message doesn't describe the schema of that generated message, doesn't mean that the protobuf wire format itself isn't self-describing. You can decode a Protobuf message without knowing the schema, as detailed in "Message structure" in your link. It's just less useful. I've worked with actual non-self-describing formats, and protobuf isn't one of them. That being said, it does seem like |
This PR adds serde support for
google.protobuf.Any. This one is a bit more complex thangoogle.protobuf.Valuebecause in order to support the inline layout for objects we need to have an additional marshalling step, where we transcode the value into agoogle.protobuf.Valuein order to be able to determine what the type is, and how to encode/decode it.The only alternative to this approach that I'm aware of, would be to change
Anyto accept a generic parameterT, but that would defeat the purposes of it being dynamic.closes #2
depends on #16