open-toast / protokt

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

Grpc service code generated from lower case RPC name does not compile #284

Open jjagged opened 10 hours ago

jjagged commented 10 hours ago

Hello!

It seems that code generated from a gRPC proto that contains an RPC name with lower case 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

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

message HelloRequest {
  string message = 1;
}

message HelloReply {
  string message = 1;
}

the generated GreeterGrpc object will get the function

  @JvmStatic
  public fun getsayHelloMethod(): MethodDescriptor<protokt.v1.HelloRequest, protokt.v1.HelloReply> = _sayHelloMethod

which preserves the lower case 's'.

However, the GreeterGrpcKt class will reference the function with uppercase 'S'

/**
 * Holder for Kotlin coroutine-based client and server APIs for Greeter.
 */
public object GreeterGrpcKt {
  public const val SERVICE_NAME: String = GreeterGrpc.SERVICE_NAME

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

and this doesn't compile. I know that lowercase rpc names are not best practice, but it's still valid proto syntax, so protokt should support it, right?

Having looked around the code, a simple solution might be to add a capitalizseFirstChar() and to modify ServiceGenerator.grpcImplementations():

    private fun grpcImplementations(): List<TypeSpec> =
        if (supportedPlugin()) {
            val getMethodFunctions =
                s.methods.map { method ->
                    buildFunSpec("get" + method.name.capitalizeFirstChar() + "Method") {
                        ...
                    }
                }
            }
            ...

private fun String.capitalizeFirstChar() =
    replaceFirstChar { it.uppercaseChar() }              

What do you think?

andrewparmet commented 10 hours ago

gRPC Kotlin code is generated by grpc-kotlin so that capitalization is done by them. We're not matching it and it looks like we probably should. My guess is they follow the same logic as grpc-java, but I'd want to see if grpc-java also does this capitalization.

Good find.

jjagged commented 10 hours ago

Btw, this does compile in grpc-kotlin, but we're looking at protokt as a possible improvement