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

Generator does not respect java_multiple_files option #6

Closed
kohenkatz opened this issue Feb 24, 2023 · 3 comments
Closed

Generator does not respect java_multiple_files option #6

kohenkatz opened this issue Feb 24, 2023 · 3 comments
Assignees

Comments

@kohenkatz
Copy link
Contributor

The java_multiple_files option is described like this in the protobuf documentation:

... For each .proto file input, the compiler creates a wrapper .java file containing a Java class which represents the .proto file itself.

If the .proto file contains a line like the following:

option java_multiple_files = true;

Then the compiler will also create separate .java files for each of the classes/enums which it will generate for each top-level message, enumeration, and service declared in the .proto file.

Otherwise (i.e. when the java_multiple_files option is disabled; which is the default), the aforementioned wrapper class will also be used as an outer class, and the generated classes/enums for each top-level message, enumeration, and service declared in the .proto file will all be nested within the outer wrapper class. Thus the compiler will only generate a single .java file for the entire .proto file.

The grpc-java and grpc-kotlin libraries both honor this option. For the given .proto file, the following outputs are produced by grpc-java:

syntax = "proto3";

package com.example;

option java_package = "com.example";

service EventService {
    rpc Start (StartRequest) returns (StartResponse);
}

message StartRequest { }

message StartResponse { }

With java_multiple_files = false

public suspend fun start(request: com.example.EventsServiceProto.StartRequest, headers: Metadata = Metadata()): com.example.EventsServiceProto.StartResponse = unaryRpc(
  channel,
  EventServiceGrpc.getStartMethod(),
  request,
  callOptions,
  headers
)

With java_multiple_files = true

public suspend fun start(request: com.example.StartRequest, headers: Metadata = Metadata()): com.example.StartResponse = unaryRpc(
  channel,
  EventServiceGrpc.getStartMethod(),
  request,
  callOptions,
  headers
)

However, protoc-gen-connect-kotlin generates identical output irrespective of the value of java_multiple_files:

public override suspend fun start(request: StartRequest, headers: Headers): ResponseMessage<StartResponse> = client.unary(
  request,
  headers,
  MethodSpec(
  "live.v1.EventService/Start",
    com.example.StartRequest::class,
    com.example.StartResponse::class,
  ),
)

This means that the Request and Response classes are not found when trying to compile the application.

The connect-kotlin generator probably should check this option and generate compatible code.

@kohenkatz
Copy link
Contributor Author

I have not fully tested this, but my quick tests seem to work. I was thinking about opening a Pull Request, but since I copied part of this implementation directly from grpc-kotlin, I'm not sure if you want to reimplement it for licensing reasons.

In fun generate(...):

for (file in request.protoFileList) {
    val packageName = if (file.options.javaPackage.isNullOrBlank()) file.`package` else file.options.javaPackage
    val nestedClassName = file.getOuterClassName()
    for (messageType in file.messageTypeList) {
        compiledTypes.put(messageType.name, packageName+nestedClassName)
    }
    for (enumType in file.enumTypeList) {
        compiledTypes.put(enumType.name, packageName+nestedClassName)
    }
}
// Based on https://github.com/grpc/grpc-kotlin/blob/ae2d5f7b514b45cf944296d91dd14c22a9a17c51/compiler/src/main/java/io/grpc/kotlin/generator/protoc/DescriptorUtil.kt#L119
private fun DescriptorProtos.FileDescriptorProto.getOuterClassName(): String {
    if (options.javaMultipleFiles) {
        return ""
    }

    if (options.hasJavaOuterClassname()) {
        return ".${options.javaOuterClassname}"
    }

    val fileName = name.substringAfterLast('/').removeSuffix(".proto")

    val defaultName = fileName.replace("-", "_").underscoresToCamel()

    val foundDuplicate = messageTypeList.any { it.name == defaultName } ||
            enumTypeList.any { it.name == defaultName } ||
            serviceList.any { it.name == defaultName }

    return if (foundDuplicate) {
        ".${defaultName}OuterClass"
    } else {
        ".$defaultName"
    }
}

// Copied from https://github.com/grpc/grpc-kotlin/blob/ae2d5f7b514b45cf944296d91dd14c22a9a17c51/compiler/src/main/java/io/grpc/kotlin/generator/protoc/DescriptorUtil.kt#L139
private fun String.underscoresToCamel(): String {
    val builder = StringBuilder()
    var capNextLetter = true
    for ((i, ch) in this.withIndex()) {
        if (ch in 'a'..'z') {
            builder.append(
                if (capNextLetter) ch.uppercaseChar() else ch
            )
            capNextLetter = false
        } else if (ch in 'A'..'Z') {
            builder.append(
                if (i == 0 && !capNextLetter) ch.uppercaseChar() else ch
            )
            capNextLetter = false
        } else if (ch in '0'..'9') {
            builder.append(ch)
            capNextLetter = true
        } else {
            capNextLetter = true
        }
    }
    return builder.toString()
}

@kohenkatz
Copy link
Contributor Author

Hmm, it looks like you might already have fixed this in #5 while I was working through my testing in the comment above.

@buildbreaker buildbreaker self-assigned this Feb 24, 2023
@buildbreaker
Copy link
Contributor

Hey @kohenkatz our (recent release)[https://github.com/bufbuild/connect-kotlin/releases/tag/v0.1.2] should have a fix for your issue. If you observe any issues, don't be afraid to reach back out :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants