quick-protobuf icon indicating copy to clipboard operation
quick-protobuf copied to clipboard

proto3 optionals ignored

Open q3k opened this issue 3 years ago • 2 comments

Protobuf 3.15 stabilized 'optional' in proto3: https://github.com/protocolbuffers/protobuf/releases/tag/v3.15.0

These, however, behave entirely differently from proto2 optionals. Thus, the following behaviour (effectively ignoring the 'optional' marker) is invalid:

q3k@sizeableunit ~/lolproto $ cat test.proto 
syntax = "proto3";

message Lol {
    bool foo = 1;
    optional bool foo = 2;
}
q3k@sizeableunit ~/lolproto $ pb-rs test.proto 
Found 1 messages, and 0 enums
Writing message Lol
q3k@sizeableunit ~/lolproto $ grep -A 3 'struct Lol' test.rs 
pub struct Lol {
    pub foo: bool,
    pub foo: bool,
}

Instead, quick-protobuf should either a) error out on 'optional' not being supported fully for proto3 b) implement proto3 optionals fully by emitting Option<T> struct members for fields marked as optional, and correctly (un)marshal them from/to the wire format.

q3k avatar Jun 14 '22 12:06 q3k

There is one scenario where this is particularly scary:

service RoleManager {
    rpc EditUser(EditUserRequest) returns (EditUserResponse);
}

message EditUserRequest {
    string username = 1;
    // If set and true: add administrator role ; if set and false: remove administrator role ; if unset: ignore.
    optional bool administrator = 2;
    string real_name = 3;
}

A fully working proto3 client implementation attempting to send { username: "admin", real_name: "Admin", ...Self::default() } will send a wire representation with field 2 being absent, thereby not modifying the administrative role of the user. Quick-protobuf will instead default to administrator: false on the wire, thereby 'accidentally' removing the administrator role from the user.

q3k avatar Jun 14 '22 12:06 q3k

I'm confused about this, same issue here. According to the README this should work?

lamafab avatar Jul 31 '23 17:07 lamafab