andrewhickman / prost-reflect

A protobuf library extending prost with reflection support and dynamic messages.
https://crates.io/crates/prost-reflect
Apache License 2.0
86 stars 19 forks source link

Make DynamicMessage deconstructable #50

Closed 124C41p closed 1 year ago

124C41p commented 1 year ago

There are two categories of public functions on DynamicMessage for accessing their field values: get_field_by_* and get_field_by_*_mut. However, I am missing functions for deconstructing a Message, i.e. for taking ownership of a field value and setting the field to None (like Option::take). Do you mind adding functions take_field_by_*(&mut self, ...) -> Option<Value>?

I am also missing iterators for the fields of a DynamicMessage (.iter(), .iter_mut(), .into_iter()). At the moment, I am doing the following for iterating over all known fields of a message which have been set:

let msg: DynamicMessage = ...;
for field in msg.descriptor().fields() {
    let field_name = field.name();
    if msg.has_field_by_name(field_name) {
        if let Some(field_value) = msg.get_field_by_name(field_name) {
            ...
        }
    }

This however is certainly not as efficient as it could be.

andrewhickman commented 1 year ago

I think the take_field_by methods definitely make sense,

The iteration APIs feel sensible as well, what do you think of these signatures?

/// Iterator over all fields for which `has_field` is true.
fn fields(&self) -> impl Iterator<Item = (FieldDescriptor, &'_ Value>);
/// Iterator over all extensions for which `has_extension` is true.
fn extensions(&self) -> impl Iterator<Item = (ExtensionDescriptor, &'_ Value>);

In future, there could be a variant of fields which includes proto3 fields set to their default value (useful for implementing the JSON serialization with skip_default_fields set to false, for example). However I don't think that one is any more efficient than an implementation using existing APIs.

I've implemented the new APIs in #55

124C41p commented 1 year ago

Looks really good to me! Do you mind also adding those destructive iterators?

fn take_fields(&mut self) -> impl Iterator<Item = (FieldDescriptor, Value>)>;
fn take_extensions(&mut self) -> impl Iterator<Item = (ExtensionDescriptor, Value>)>;

(for completeness you would probably also want to add fields_mut and extensions_mut in this case)

Thanks a lot!

andrewhickman commented 1 year ago

I've added the extra methods to #52, if you think they look good I'll publish a new version tomorrow