Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proto3 optionals ignored #219

Open
q3k opened this issue Jun 14, 2022 · 2 comments
Open

proto3 optionals ignored #219

q3k opened this issue Jun 14, 2022 · 2 comments

Comments

@q3k
Copy link

q3k commented Jun 14, 2022

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 struct members for fields marked as optional, and correctly (un)marshal them from/to the wire format.

@q3k
Copy link
Author

q3k commented Jun 14, 2022

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.

@lamafab
Copy link

lamafab commented Jul 31, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants