open-toast / protokt

Protobuf compiler and runtime for Kotlin
Apache License 2.0
154 stars 14 forks source link

Grpc service code generated from proto not using `java_multiple_files` does not compile #285

Open jjagged opened 2 hours ago

jjagged commented 2 hours ago

Code generated from a gRPC proto that does not specify option java_multiple_files = true; doesn't compile, when generating the gRPC Kotlin stubs. See broken example project here.

When using

protokt {
    generate {      
        grpcKotlinStubs = true
    }
}

to generate code for a proto file hello_world_file.proto

service Greeter {
  rpc sayHello (HelloRequest) returns (HelloReply) {} // Lower case initial 's' 
}

message HelloRequest {
  string message = 1;
}

message HelloReply {
  string message = 1;
}

the generated GreeterGrpcKt object will contain method descriptors with the outer class name as part of the parameter types.

public object GreeterGrpcKt {
  public const val SERVICE_NAME: String = GreeterGrpc.SERVICE_NAME

  @JvmStatic
  public val serviceDescriptor: ServiceDescriptor
    get() = getServiceDescriptor()

  public val sayHelloMethod:
      MethodDescriptor<HelloWorldFile.HelloRequest, HelloWorldFile.HelloReply>
    @JvmStatic
    get() = GreeterGrpc.getSayHelloMethod()

The easy solution is of course to just add the java_multiple_files option in our protos, but I think protokt should work in case the option is not set as well. What do you think?

andrewparmet commented 2 hours ago

This appears to be another case of us delegating the gRPC Kotlin generation to grpc-kotlin causing issues. I'd again think we should provide behavior that matches their expectations. In this case it's a bit nasty since we'd be generating Kotlin code dependent on Java conventions, but having java_multiple_files set is (I think) generally considered best practice these days. In any case, this should be a relatively straightforward fix.

Related in concept to #284.

jjagged commented 1 hour ago

👍 All our protos have been built using ScalaPB previously, so we haven't been following java best practices that well 😄