dropbox / pb-jelly

A protobuf code generation framework for the Rust language developed at Dropbox.
Apache License 2.0
610 stars 25 forks source link

Example honoring required and optional #125

Closed kootenpv closed 2 years ago

kootenpv commented 2 years ago

It is very confusing to me.

Is there no way I could initialize Command where ts is not Option?

Code (proto 2)

message Command {
    required uint64 ts = 1;
    required CommandType command = 2;
    optional uint64 trace_id = 3;
    optional Service service = 4;
}
ddeville commented 2 years ago

If you put (rust.nullable_field)=false on the field it will make it the type itself rather than an Option of the type.

kootenpv commented 2 years ago

@ddeville thanks!

Though does that mean when it is optional then it is again nullable?

kootenpv commented 2 years ago

Actually don't understand how to make (rust.nullable_field)=false work, e.g.

required uint64 ts = 1 [(rust.nullable_field)=false];

I added the extensions.proto file but I'm getting an error that this option is not known

nipunn1313 commented 2 years ago

Hiya, you have a couple questions here.

  • Command::default() allows creating an object with everything missing
  • Command { ts: 1 } complains that I will have to pass all values.
  • I have to always pass a Some (regardless required or optional) for each field (this is actually the real issue)

Requirement to pass all values is by design. This helps the developer audit the code after adding a new field. You have to do something like

Command { ts: 1, command: None, trace_id: None, service: None }

If you really want the behavior you're describing, you can do

let mut c = Command::default();
c.ts = 1;

Is there no way I could initialize Command where ts is not Option?

(rust.nullable_field)=false is the answer here. We don't have an example yet (see #29). But here's an example - There is an example here in pbtest. https://github.com/dropbox/pb-jelly/blob/47eb5a98a74db4ea5d0c835d0ddeda1fc6910f57/pb-test/proto/packages/pbtest/pbtest3.proto#L137

I added the extensions.proto file but I'm getting an error that this option is not known

Oops. It's rust.nullable_field. You can see the source here. https://github.com/dropbox/pb-jelly/blob/47eb5a98a74db4ea5d0c835d0ddeda1fc6910f57/pb-test/proto/includes/rust/extensions.proto#L39

nipunn1313 commented 2 years ago

Added an example with #126 - hopefully that helps!

kootenpv commented 2 years ago

Thanks a lot for the example!

It seems really ugly to have to put this on every field in proto, but at least it is possible

nipunn1313 commented 2 years ago
message Inner {}
message Outer {
   Inner i = 1;
}

proto (on the wire) allows for message fields to be nil. On the wire, Outer { i: None } and Outer { i: Some(Inner {})} are distinct and distinguishable. In proto2, primitive fields also have this distinction, but in proto3 they do not.

nullable_field=true is a sensible default because it maintains this distinction. With nullable_field=false - when deserializing off the wire, both cases end up deserializing to Outer { i: Inner {}} - so you're losing information.

The decision to do this (in our opinion) is a field-by-field decision. This was inspired directly by gogoproto.nullable (https://pkg.go.dev/github.com/gogo/protobuf/gogoproto)