enhancement(codecs)!: Use DescriptorPool from ProtobufDeserializer #901
Conversation
nb-mouse
changed the title
nb-mouse
force-pushed
the
protobuf-descriptor-pool
branch
from
1420ff8 to
a3ed24f
Compare
src/protobuf/parse.rs
Outdated
| let mut obj_map = ObjectMap::new(); | ||
| for field_desc in v.descriptor().fields() { | ||
| if v.has_field(&field_desc) { | ||
| let field_value = v.get_field(&field_desc); | ||
| let out = proto_to_value(field_value.as_ref(), Some(&field_desc))?; | ||
| let out = proto_to_value(descriptor_pool, field_value.as_ref(), Some(&field_desc))?; |
There was a problem hiding this comment.
There is a possible improvement if fields in descriptor is ordered and type_url comes before value field then logic from above can be moved here.
| @@ -99,6 +130,7 @@ pub(crate) fn parse_proto(descriptor: &MessageDescriptor, value: Value) -> Resol | |||
| let dynamic_message = DynamicMessage::decode(descriptor.clone(), bytes) | |||
| .map_err(|error| format!("Error parsing protobuf: {:?}", error))?; | |||
| Ok(proto_to_value( | |||
| None, | |||
There was a problem hiding this comment.
If we declare a singleton method to retrieve global configuration with loaded descriptor pool, we can use it here (when specified)
|
Hi @nb-mouse, I was unavailable the past few days. I will take a look shortly. |
| if let Some(descriptor_pool) = descriptor_pool { | ||
| if let Some(type_url_cow) = v.get_field_by_name("type_url") { | ||
| if let Some(value_cow) = v.get_field_by_name("value") { | ||
| if let prost_reflect::Value::String(type_url) = &*type_url_cow { | ||
| if let prost_reflect::Value::Bytes(value) = &*value_cow { | ||
| let type_name = type_url.trim_start_matches("type.googleapis.com/"); | ||
| match get_message_descriptor_from_pool(descriptor_pool, type_name) { |
There was a problem hiding this comment.
This is a lot of nesting, can you explain the motivation and the use case for this? I am also worried about the hardcoded values in here.
There was a problem hiding this comment.
This was a naive implementation for checking the presence of fields.
The FieldDescriptor actually have a type_name. If we can check if the given field of this type, we can decode it straight away:
field {
name: "anydata"
number: 3
label: LABEL_OPTIONAL
type: TYPE_MESSAGE
type_name: ".google.protobuf.Any"
json_name: "anydata"
}
This change allows to recursively handle
google.protobuf.Anymessages from a shared DescriptorPool loaded viadecoding.protobuf.desc_fileat best effort.