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

[C0mbatwombat]

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]

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

[ikhoon]

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]

I've compiled a proto file with protoc 3.25.1 and json_name was empty if it is not set.

armeria/dependencies.toml

Line 119 in 683bd74

protobuf = "3.25.1"

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

[mbgit2]

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]

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 json_name 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]

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.

[mbgit2]

Issue: explicitly need to set json_name in proto file to enable trancoding.

I've made a minimal repo that reproduces the issue. Note it is build using bazel, but with all previously specified versions as per this issue.

syntax = "proto3";

package hello.service.v1;

import "hello/v1/hello.proto";
import "google/api/annotations.proto";

option java_package = "com.example.myproject.service.hello.v1";
option java_multiple_files = true;


service HelloService {
  rpc Hello (hello.v1.HelloRequest) returns (hello.v1.HelloReply){
    option (google.api.http) = {
      get: "/v1/hello/{name_input}"
    };
  }
}

message HelloRequest {
  string name_input = 1;// [json_name="name_input"];
}

message HelloReply {
  string message = 1;
}

This proto compiled to java and then used by armeria:1.26 with transcoding results in the following service printing "Hello, mbgit2!". However with all the same in armeria:1.30 it prints "Hello, !". The incoming HelloRequest is empty, unless we specify the json_name, see the commented line in the proto file.

public class HelloService extends HelloServiceGrpc.HelloServiceImplBase {

    @Override
    public void hello(HelloRequest req, StreamObserver<HelloReply> responseObserver) {
        HelloReply reply = HelloReply.newBuilder()
                .setMessage("Hello, " + req.getNameInput() + '!')
                .build();
        responseObserver.onNext(reply);
        responseObserver.onCompleted();
    }
}

    ServerBuilder sb = Server.builder();
    GrpcServiceBuilder grpcBuilder = GrpcService.builder();
    grpcBuilder.enableHttpJsonTranscoding(true);
    sb.service(grpcBuilder.addService(new HelloService()).build());

We would really like to have the 1.26 behavior such that it is not necessary to put the json_name everywhere explicitly. Thanks for your time.

[mbgit2]

Any update would be much appreciated, would this be taken into the next version?

[ikhoon]

I managed to reproduce your repo that uses Bazel. In fact, I didn't catch the exact difference between Gradle and Bazel. Anyway, I will send a patch that will be included in the next minor version.

[C0mbatwombat]

That's good news, thanks! I understand this is a very edge-case situation, but it would help us stay updated :-)

[ikhoon]

I think this issue will be fixed by #6082. Would you like to review if the workaround resolves your problems?

[C0mbatwombat]

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.

I was wondering, with your solution it is not possible to use the json_name field anymore at all, whereas the workaround used by protobuf does allow it to be set. Was there a reason not to go for that approach? Don't get me wrong, I'm happy that you looked at this annoying edge-case, and I don't use json_name anywhere (yet), so it's currently fine for me.

[ikhoon]

I was wondering, with your solution it is not possible to use the json_name field anymore at al

Good point. I updated the PR to allow the json name field as a lower priority.
I believe that your code base will work without any modification. It will look up the json name fields first and then check the original fields. The first search does not matter since most users may have an empty or a small number of json names.

armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.java

Lines 44 to 48 in 3acdb3b

	/**
	* Evaluates json_name first, then original field.
	*/
	private static final EnumSet<HttpJsonTranscodingQueryParamMatchRule> DEFAULT_QUERY_PARAM_MATCH_RULES =
	EnumSet.of(JSON_NAME, ORIGINAL_FIELD);

To get an optimized code path for your case to look up the original fields first, you can change the order of queryParamMatchRules.

 HttpJsonTranscodingOptions
  .builder()
  // Use only original fields
  .queryParamMatchRules(HttpJsonTranscodingQueryParamMatchRule.ORIGINAL_FIELD)
  .build()

 HttpJsonTranscodingOptions
  .builder()
  // Check the original fields first and then use json names 
  .queryParamMatchRules(HttpJsonTranscodingQueryParamMatchRule.ORIGINAL_FIELD, JSON_NAME)
  .build()

[C0mbatwombat]

Perfect, thank you!