5 ms·
A couple of subtly wrong points in the article. Firstly gRPC payload can be anything, needs not be an encoded protocol buffer. Secondly there’s not a whole lot
by shereadsthenews 7y ago
A couple of subtly wrong points in the article. Firstly gRPC payload can be anything, needs not be an encoded protocol buffer. Secondly there’s not a whole lot of “validation” going on in the protobuf codec. Basically any fundamentally correct buffer encoded as message A will decode successfully as message B for any B. If there are unknown fields they are silently consumed. If there are missing fields they are given the default values and there is no “required” in proto3. So there is significantly less safety, and significantly more flexibility, in gRPC than people generally realize.
- duality 7y ago"Basically any fundamentally correct buffer encoded as message A will decode successfully as message B for any B." This is incorrect. I suspect you're overextending proto3's treatment of unknown fields to include discarding incorrectly typed fields too. If A has field 1 types as an int, and B has field 1 typed as a string, an A message with field 1 set will not parse as a B message. However, if the A message has no fields set, or sets a field number unknown to B, that could parse successfully with "leftover" unknown fields.
- shereadsthenews 7y agoOk but these messages are isomorphic on the wire: message enc { int foo = 1; SomeMessage bar = 2; } message dec { bool should_explode = 1; string why = 2; } You can successfully decode the latter from an encoding of the former.
- dweis 7y agoMinor nit, but not necessarily. For basically all values of SomeMessage, dec should fail to parse due to improperly encoded UTF8 data for field 2 (modulo some proto2 vs. proto3 and language binding implementation differences). Change field 2 to a bytes field instead of a string field and then yes.
- shereadsthenews 7y agoTrue but UTF8 enforcements was quite absent in all implementations until proto3, and the empty string would be a special case.
- shereadsthenews 7y agoI should mention that I consider this a feature not a bug. The isomorphism permits an endpoint to use ‘bytes submessage_i_dont_need_to_decode’ to cheaply handle nested message structures that need to be preserved but not inspected, such as in a proxy application.
- keymone 7y agoBool will decode from int’s encoding??
- dweis 7y agoI don't think this is the case, or at least, I'd expect it to be a bug. Protocol Buffers should generally be non-destructive of the underlying data. That means even if it encounters the wrong wire type for a field, it should simply retain that value in the unknown field set rather than discard it.
- kentonv 7y ago> If A has field 1 types as an int, and B has field 1 typed as a string, an A message with field 1 set will not parse as a B message. In the C++ reference implementation, which I wrote, this is not true. The field 1 with the wrong wire type would be treated as an unknown field, not an error. It's possible that implementations in other languages have different behavior, but that would be a bug. The C++ implementation is considered the reference implementation that all others should follow. However, shereadsthenews' assertion is not quite right either. Specifically, a string field and a sub-message field both use the same wire type; essentially, the message is encoded into a byte string. So if message A has field 1 type string, containing some bytes that aren't a protobuf, and message B has field 1 type sub-message, then you'll get a parse error. But it is indeed quite common that one message type parses successfully as another unrelated type.
- therein 7y ago> In the C++ reference implementation, which I wrote, this is not true. The field 1 with the wrong wire type would be treated as an unknown field, not an error. Yeah I was about to say, protobuf C++ implementation will definitely treat it as an unknown field. I just had it do that a few days ago. :)
- ntenenz 7y ago`required` was removed due to the challenges it introduces in designing backwards-compatible API changes[1]. [1] https://github.com/protocolbuffers/protobuf/issues/2497#issuecomment-267422550 https://github.com/protocolbuffers/protobuf/issues/2497#issu...
- docker_up 7y ago"Required" fields that are no longer required, and "optional" fields that are no longer optional are basically 6 of one and half a dozen of another. I'm personally strongly in the "required" camp because at least the interface makes an attempt at giving clues to a user as to what fields are important. If everything is optional, there's no information being passed as to what is important anymore.