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

gRPC: Inconsistency in generated method name #9987

Closed
djnalluri opened this issue Jun 14, 2020 · 12 comments · Fixed by #29339
Closed

gRPC: Inconsistency in generated method name #9987

djnalluri opened this issue Jun 14, 2020 · 12 comments · Fixed by #29339
Assignees
Labels
area/grpc gRPC kind/bug Something isn't working
Milestone

Comments

@djnalluri
Copy link
Contributor

djnalluri commented Jun 14, 2020

Describe the bug
The generated method names in quarkus-grpc-protoc-plugin do not match those generated by grpc-java for all valid input. This creates compile errors when the generated Mutiny code tries to delegate to grpc-java's implementation.

Expected behavior
A gRPC method named 'THIS_FAILS' should generate a Java method named 'tHISFAILS'.

Actual behavior
quarkus-grpc-protoc-plugin generates 'tHIS_FAILS' and attempts to delegate to the same name in grpc-java which does not exist.

To Reproduce
Steps to reproduce the behavior:

  1. Follow the Getting Started with gRPC guide here: https://quarkus.io/guides/grpc-getting-started
  2. Replace helloworld.proto with the following:
syntax = "proto3";

option java_multiple_files = true;
option java_package = "io.quarkus.example";
option java_outer_classname = "HelloWorldProto";

package helloworld;

// The greeting service definition.
service Greeter {
    // Sends a greeting
    rpc SayHello (HelloRequest) returns (HelloReply) {}
    rpc THIS_FAILS (HelloRequest) returns (HelloReply) {}
}

// The request message containing the user's name.
message HelloRequest {
    string name = 1;
}

// The response message containing the greetings
message HelloReply {
    string message = 1;
}
  1. Attempt compilation.

Environment:

  • Output of uname -a or ver: Linux <hostname> 5.3.0-59-generic #53~18.04.1-Ubuntu SMP Thu Jun 4 14:58:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
  • Output of java -version: OpenJDK Runtime Environment GraalVM CE 20.0.0 (build 11.0.6+9-jvmci-20.0-b02)
  • Quarkus version or git rev: 1.5.1.Final
  • Build tool (ie. output of mvnw --version or gradlew --version):
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /home/<user>/.sdkman/candidates/maven/current
Java version: 11.0.6, vendor: Oracle Corporation, runtime: /home/<user>/.sdkman/candidates/java/20.0.0.r11-grl
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.3.0-59-generic", arch: "amd64", family: "unix"

Additional context
grpc-java's algorithm for converting gRPC method name to Java method name can be found here: https://github.com/grpc/grpc-java/blob/40b815058f1209e66086f4426c407875f47fe400/compiler/src/java_plugin/cpp/java_generator.cpp#L115

The location where the method name is being assigned in Quarkus is here:

methodContext.methodName = lowerCaseFirst(methodProto.getName());

@djnalluri djnalluri added the kind/bug Something isn't working label Jun 14, 2020
@gsmet
Copy link
Member

gsmet commented Jun 15, 2020

/cc @michalszynkiewicz @cescoffier

@gsmet gsmet added the area/grpc gRPC label Jun 15, 2020
@cescoffier
Copy link
Member

The template has been provided by @pmlopes - Paulo, are you aware of this?

@gsmet
Copy link
Member

gsmet commented Jun 24, 2020

@pmlopes @michalszynkiewicz @cescoffier looks like something we would like for 1.6, no?

@cescoffier
Copy link
Member

@gsmet not sure, it's a bit tricky. gRPC convention and java convention are not exactly the same. We have followed the java convention.

@mkouba
Copy link
Contributor

mkouba commented Oct 6, 2021

For the record, I've just tried to reproduce the problem in the main branch and got several compilation errors:

[ERROR] /opt/source/protean/quarkus-quickstarts/grpc-plain-text-quickstart/target/generated-sources/grpc/examples/GreeterGrpc.java:[281,28] variable METHODID_SAY_HELLO is already defined in class examples.GreeterGrpc
[ERROR] /opt/source/protean/quarkus-quickstarts/grpc-plain-text-quickstart/target/generated-sources/grpc/examples/GreeterGrpc.java:[304,9] duplicate case label
[ERROR] /opt/source/protean/quarkus-quickstarts/grpc-plain-text-quickstart/target/generated-sources/grpc/examples/MutinyGreeterGrpc.java:[53,74] invalid method reference
  cannot find symbol
    symbol:   method sAY_HELLO()
    location: class examples.GreeterGrpc.GreeterStub

The first two come from the examples.GreeterGrpc which is generated by grpc-java. In other words, even if we change the method name the reproducer app would not compile.

@djnalluri
Copy link
Contributor Author

The extra compiler errors you're seeing appear to be due to added static final fields since the issue was filed. The two methods in the reproducer are so similarly named that these generated fields overlap. Removing or renaming them to not be so close will leave only the errors described in this issue. I have updated the reproducer to remove the overlap.

@cescoffier
Copy link
Member

Unfortunately, there is no much we can do without breaking the assumption made by other Java gRPC libraries (following the same convention)

@cescoffier cescoffier added the triage/wontfix This will not be worked on label Nov 7, 2022
@djnalluri
Copy link
Contributor Author

What other gRPC libraries is the project trying to align with? It's been a while since I dove into this code. I'm assuming Vert.x gRPC at a minimum but any others?

@cescoffier
Copy link
Member

cescoffier commented Nov 13, 2022

It seems that both Vert.x and Micronaut uses that convention.
Also, updating now would be a big breaking change for all the code generated with the current convention.

We may be able to fix it, but need to use a flag to enable this new mapping. Wondering if it worth it (it's going to make the code a lot more complicated).

@cescoffier cescoffier self-assigned this Nov 14, 2022
@djnalluri
Copy link
Contributor Author

I should mention that this may no longer be blocking my use case so I'm fine with closing this issue as part of triage.

@cescoffier
Copy link
Member

ah ah ah!

@mkouba fixed this issue already (#27183)

Just adding a few tests to verify!

@cescoffier
Copy link
Member

Here are the tests: #29339

@cescoffier cescoffier removed the triage/wontfix This will not be worked on label Nov 17, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc gRPC kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants