`minprotobuf` broken for out-of-order fields

[arnetheduck]

`minprotobuf` broken for out-of-order fields

[zah]

I think my previous message was misunderstood. I meant that the option will be gained by FieldReadersTable which is a generic mechanism defined in the common nim-serialization library that will be shared by both Json and ProtoBuf. But thanks for sharing these considerations.

[zah]

This will be solved by status-im/nim-serialization#20, because the reading code will be modelled after our json deserializer, which can handle fields appearing in arbitrary order:

https://github.com/status-im/nim-json-serialization/blob/master/json_serialization/reader.nim#L221-L239

To clarify the loop above, nim-serialization builds a RTTI table of "field reader" functions. When you encounter a key-value pair, you use the key to find a function that will deserialize the value into the correct field of the object.

[arnetheduck]

Sure, though I can, at a glance, tell what minprotobuf is doing wrong and how to fix it.

nim-serialization? not so - it looks very clever - but let's not model it too closely after json:

in json duplicate keys are a bit of an open question whereas in pb, the wire encoding for lists and scalars is the same, the difference being that the last seen value is used for scalars - the way it's usually implemented is that it just keeps overwriting the scalar. you could say that it's a feature of pb that you can seamlessly upgrade a scalar to a list and the wire encoding stays the same - some protocols use this for forwards/backwards compatibility schemes.

in protobuf, there are two levels of "structured" decoding - the wire level and the protobuf level - the wire level is useful to decode separately because there are use cases for stopping there and not doing the "protobuf" level - the former is about identifying parsing the stream into keys and values of "approximate" type while the latter is about further refining the value into a more specific type. depending on the application, one or the other level may be appropriate.

protobuf is pretty explicit about the fact that unknown keys should be at least ignored or sometimes kept as unknown generic blobs, in case the message is meant to be reencoded - thus it would go against the spirit of protobuf for the parser to require that all fields are present, which I think that json code does? I mean it does something like raiseUnexpectedField - but only if totalSerializedFields > 0 which looks a bit odd.

[zah]

When I say "modelled after", don't take that too literally. It will be custom code responsible for exposing the peculiarities of ProtoBuf (just like in the json readers you can directly interact with the Json lexer).

protobuf is pretty explicit about the fact that unknown keys should be at least ignored or sometimes kept as unknown generic blobs, in case the message is meant to be reencoded - thus it would go against the spirit of protobuf for the parser to require that all fields are present

The point about unrecognized fields is an interesting one, will consider it during the design. Regarding requiring all fields, this will become a feature of the FieldReadersTable. It will have an option to populate a bitmask indicating which fields were encountered, so the readers can decide which policy is more appropriate.

[arnetheduck]

the policy of protobuf is that required fields are a mistake in applications that use protobuf - indeed, in protobuf 3, all fields are optional and the required specifier is gone. We could do worse than take a hint from google that has plenty of experience in how protobuf is best used.

https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules

Notice the big warning about required - it's hard to upgrade a distributed system already, and required fields make it worse.

Protobuf is generally use for network protocols (like libp2p) which benefit tremendously from having clients write fallback behaviour deliberately and up front, before the first modifications to the protobuf are necessary - the people using protobuf should always be aware that this will happen eventually. We should certainly not be changing this policy, except maybe as an option that comes with an equally explicit warning, or we will be doing the users of the library a disservice - required is not an expected use case of protobuf.

[arnetheduck]

this was fixed