tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.97k stars 513 forks source link

Are proto2 extensions supported? #100

Open im-0 opened 6 years ago

im-0 commented 6 years ago

https://developers.google.com/protocol-buffers/docs/proto#extensions

I have existing proto2 protocol definition which uses extensions. And I'm unable to understand how to work with such protocol using prost. Am I looking into wrong direction or this feature is not supported?

im-0 commented 6 years ago

I found a workaround. This protocol definition:

syntax = "proto2";

message Header {
        required uint64 msg_id = 1;
        extensions 100 to max;
}

message FooMessage {
        required string some_parameter = 1;
        extend Header {
                optional FooMessage msg_foo = 101;
        }
}

message BarMessage {
        required string another_parameter = 1;
        extend Header {
                optional BarMessage msg_bar = 102;
        }
}

can be rewritten as this:

syntax = "proto2";

message FooMessage {
        required string some_parameter = 1;
}

message BarMessage {
        required string another_parameter = 1;
}

message Header {
        required uint64 msg_id = 1;

        optional FooMessage msg_foo = 101;
        optional BarMessage msg_bar = 102;
}

This does not change the binary format, so both definitions are equal. And the latter one works out of the box with prost.

danburkert commented 6 years ago

Glad you found a workaround. Extensions aren't supported, since I haven't personally needed them, and they are more-or-less deprecated. I'm not against adding support in prost, but obviously it will be low down on the priority list. If you want to hack on it then please feel free.

im-0 commented 6 years ago

If you want to hack on it then please feel free.

I already have a sufficient workaround and it is very unlikely that I will need this feature again. So, you may close this issue if you want.

kamalmarhubi commented 6 years ago

Keep it open :-)

Custom options are implemented as extensions, even in proto3: https://developers.google.com/protocol-buffers/docs/proto3#custom_options

Seems conceivable that prost could allow customizing generated code via options.

Xorlev commented 4 years ago

I find myself in need of extensions for the reason mentioned above. :)

I'm working on a Rust port of https://github.com/Xorlev/grpc-jersey using tonic and tower-web, but I'll need to parse options:

service TestService {
    rpc TestMethod (TestRequest) returns (TestResponse) {
        option (google.api.http).get = "/users/{string}/{uint32}/{nested_type.field_f64}";
    }
}

If you're still open to the idea @danburkert I'll start up a draft PR. I suspect it should be reasonably similar to Java protobufs + prost-build would gain an ExtensionRegistry argument such that the parsed MethodDescriptorProto would have an extensions field populated.

It wouldn't be a bad time to drag #117 along for the ride either, but they can be separate.

tiziano88 commented 4 years ago

I'm also interested in this.

@Xorlev I think it would require changing the generated code to carry around unknown fields, so that they may be accessed by looking them up via extension number. Or are you thinking of something else?

Xorlev commented 4 years ago

In the Java version of protos, extension registries are declared during deserialization, and those fields are eagerly extracted out. If an extension isn't recognized, it goes to unknown fields. This has the advantage of checking validity at deserialization time rather than extension access time.

They're mechanisms that work well together but don't necessarily need to both be implemented.

On Wed, Mar 4, 2020, 11:24 Tiziano Santoro notifications@github.com wrote:

I'm also interested in this.

@Xorlev https://github.com/Xorlev I think it would require changing the generated code to carry around unknown fields, so that they may be accessed by looking them up via extension number. Or are you thinking of something else?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/danburkert/prost/issues/100?email_source=notifications&email_token=AACVDSQ24BROZKSISY4OZCLRF3BHTA5CNFSM4FARIA3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN2MALA#issuecomment-594853932, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVDSTOOBC5UWOQJG4JF53RF3BHTANCNFSM4FARIA3A .

amilkov3 commented 4 years ago

@Xorlev did you manage to start working on this? we're porting a lot of gRPC stuff to Rust at work and need this so just wanted to make sure we aren't duplicating work. happy to help as well

lenaschimmel commented 4 years ago

Proto2 Extensions are also required by the GTFS (General Transit Feed Specification) realtime spec. For testing purposes, I will try to use the workaround proposed by @im-0, but it would be great if we could follow the official way for our gtfs-rt extension for our project Dystonse.

imalsogreg commented 3 years ago

@Xorlev I'd love to understand your proposed solution better, so I could try to implement it. We need the google.http.api extension at work.

One thing in particular I didn't understand was this:

In the Java version of protos, extension registries are declared during deserialization

What deserialization are you referring to? The deserialization of the literal value on the right-hand-side of option(my_option) = { foo: "bar"};