search

line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.83k stars 921 forks source link

JSON transcoding has backwards incompatible change in 1.27.0, due to json_name field always populated by protoc #5890

Open C0mbatwombat opened 2 months ago

C0mbatwombat commented 2 months ago

Hi,

In #5193 functionality was added so that if a user sets the json_name field in the .proto file explicitly, it is used to resolve the path & query params when doing transcoding. This makes perfect sense, if it weren't for a bug in protoc: the proto compiler always populates the json_name field since 3.1.0.

This means that to retain our existing behaviour (using the original field which might contain underscores) we have to add json_name = <field_name> to all our proto definitions.

The Protobuf code base also acknowledges it as a bug and has a workaround to deal with it.

I suppose Armeria needs to use the same workaround as the protobuf codebase: if the json_name that is set is the same as the default calculated one, assume it was not set explicitly by the user.

Thanks!

mbgit2 commented 1 month ago

Any update on this? I'm currently stuck on 1.26.4 due to this.

ikhoon commented 1 month ago

I suppose Armeria needs to use the same workaround as the protobuf codebase: if the json_name that is set is the same as the default calculated one, assume it was not set explicitly by the user.

It makes sense. We may provide a new option to ignore json_name. Let us handle it in the next version.

ikhoon commented 1 month ago

I've compiled a proto file with protoc 3.25.1 and json_name was empty if it is not set. https://github.com/line/armeria/blob/683bd74b91eec18a1a5007c4ac1013327e7b5a62/dependencies.toml#L119

image

Was that bug fixed in recent versions or am I missing something to reproduce the problem?

mbgit2 commented 4 weeks ago

For us it doesn't seem fixed. We have a custom interceptor, but this now gets a complete empty (ReqT message) argument when using transcoding.

berendjan commented 4 weeks ago

Hi @ikhoon, we're also encountering the problem but only when using plugins. As noted in the code base: _"json_name option is not allowed on extension fields. Note that the jsonname field in FieldDescriptorProto is always populated by protoc when it sends descriptor data to plugins"

If you add a plugin you will see the filled in json_name. In our setup we use https://github.com/bufbuild/protoc-gen-validate leading to the same problem

Thanks!

ikhoon commented 3 days ago

I failed to reproduce the problem by configuring https://github.com/bufbuild/protoc-gen-validate. Would you provide a minimal reproducible example? The example should be useful for preventing regression and ensuring compatibility.