API for unknown fields

[andrewhickman]

Currently DynamicMessage stores unknown fields internally and roundtrips them through the byte representation, but there is no API to inspect them.

[124C41p]

Would it be possible without much effort to add functions for getting/setting all unknown fields at once as a single Vec<u8> (at least temporarily unless you come up with something more general)?

Background: I am currently in the progress of creating an extension module for python-betterproto based on prost-reflect for improving (de-)serialization performance (see this PR). For that purpose, I need to be able to convert between your DynamicMessage and their betterproto.Message in both directions (including unknown fields).

[andrewhickman]

I had hoped prost would gain support for unknown fields that we could re-use here.

However if there's a real-world need for it, I'm happy to expose some new APIs for this. Would the APIs in #52 be sufficient for your usecase?

[124C41p]

Regarding the level of detail this is perfect (even more than I need). However, I'll also need a function to set the unknown fields for conversion of betterproto.Message into DynamicMessage (maybe .push_unknown(&mut self, ...)?). Also (as mentioned in #50) it would be really nice to have a destructive iterator like pub fn take_unknown_fields(&mut self) -> impl Iterator<Item = UnknownField> to avoid cloning on conversion.

Thanks a lot for starting this PR!

[andrewhickman]

What format are the unknown fields from betterproto.Message in? If they are just bytes, then I think DynamicMessage::merge or DynamicMessage::merge_field should be suffcient to add them into the message.

A push_unknown function would need to do something in the error case when a field with the given number exists, which makes me hesitate to add it.

[124C41p]

Yes, I see. You are technically right. But when converting back and forth (DynamicMessage -> betterproto.Message -> DynamicMessage) this would mean a bit of unnecessary (de-)serialization, as I would have to call something like msg.merge(unknown_field.encode_field(num)) for each unknown field. Wouldn't it be nicer to have a function DynamicField::set_unknown_field(&mut self, number: u32, wire_type: WireType, buf: &mut B) -> Result<(), DecodeError> with the same semantics but less overhead?

Anyway, it probably isn't actually that much of a deal, and I am very grateful that you are making this possible in the first place. So please go ahead as you feel most comfortable with.

[124C41p]

If I am not mistaken, the current API fn take_unknown(&mut self) -> impl Iterator<Item = UnknownField> does not return the field numbers of the unknown fields, does it? This would be required for conversion.

Added later: Sorry, my bad. I was looking at the main branch..

[andrewhickman]

If I am not mistaken, the current API fn take_unknown(&mut self) -> impl Iterator<Item = UnknownField> does not return the field numbers of the unknown fields, does it? This would be required for conversion.

UnknownField includes the number, there is a getter function for it.

[andrewhickman]

Yes, I see. You are technically right. But when converting back and forth (DynamicMessage -> betterproto.Message -> DynamicMessage) this would mean a bit of unnecessary (de-)serialization, as I would have to call something like msg.merge(unknown_field.encode_field(num)) for each unknown field. Wouldn't it be nicer to have a function DynamicField::set_unknown_field(&mut self, number: u32, wire_type: WireType, buf: &mut B) -> Result<(), DecodeError> with the same semantics but less overhead?

Anyway, it probably isn't actually that much of a deal, and I am very grateful that you are making this possible in the first place. So please go ahead as you feel most comfortable with.

I've created an issue for a more extensive unknown fields API: #56 . I do agree that a proper setter function would be nicer, but I'd like to put some more thought into the API design.

[andrewhickman]

Version 0.11.5 is released with the new APIs