Dogacel / kotlinx-protobuf-gen

Generate kotlinx serializable data classes from protobuf
Apache License 2.0
12 stars 3 forks source link

[FEATURE] - Issue Duplicated Enum Value #14

Closed tamnguyenhuy closed 1 year ago

tamnguyenhuy commented 1 year ago

prefer to https://github.com/protocolbuffers/protobuf/issues/5425 Do you have any options to solve this issue when we gen the kotlin file ?

Dogacel commented 1 year ago

prefer to protocolbuffers/protobuf#5425 Do you have any options to solve this issue when we gen the kotlin file ?

It seems like this restriction comes from the protoc compiler. The generator can handle this case because Kotlin enums are accessed via the full qualifier (i.e., TestEnum.FOO`).

Execution failed for task ':app:generateProto'. protoc: stdout: . stderr: enums.proto:27:3: "FOO" is already defined in "enums". enums.proto:27:3: Note that enum values use C++ scoping rules, meaning that enum values are siblings of their type, not children of it. Therefore, "FOO" must be unique within "enums", not just within "TestEnumDuplicate".

We actually prefix our Enum values with the enum name to overcome this in our codebase. It creates some readability issues as mentioned but unfortunately, this is something we can't adjust.

Dogacel commented 1 year ago

@tamnguyenhuy Having a second thought, I think we can include an option to remove the ENUM_NAME_ prefix from the actual enum names in the generated code. Such as

enum State {
  STATE_UNKNOWN = 0
  STATE_KNOWN = 1
}

will be converted to

@Serializable
public enum class State {
    @ProtoNumber(number = 0)
    UNKNOWN,
    @ProtoNumber(number = 1)
    KNOWN,
}

And this can be toggled via an option

    generateProtoTasks {
        all().forEach {
            it.plugins {
                id("kotlinx-protobuf-gen") {
                    option("remove_enum_prefix") // <-- this
                }
            }
        }
    }

What do you think?

tamnguyenhuy commented 1 year ago

@tamnguyenhuy Having second thoughts, I think we can include an option to remove the ENUMNAME prefix from the actual enum names in the generated code. Such as

It's a good idea but unfortunately, I can't change the enum name because of business rules. So I've added a different package name for each enum. It's work for me.

package ManageState
enum State {
  UNKNOWN = 0
  KNOWN = 1
}

----

package ManageStateOther
enum StateOther {
  UNKNOWN = 0
  KNOWN = 1
}
tamnguyenhuy commented 1 year ago

I have another issue with the proto file.

message Hello {
    enum Type {
        UNKNOWN = 0;
        KNOWN = 1;
    }
    message SubHello {
        message HelloA {
        optional Type type = 1; 
    }
    optional SubHello subHello = 1;
  }
}

It is going to gen to then get an error Data class must have at least one primary constructor parameter

@Serializable
public data class Hello() { // <-- this Data class must have at least one primary constructor parameter
  @Serializable
  public data class SubHello(
    @ProtoNumber(number = 1)
    public val subHello: SubHello? = null,
  ) {
    @Serializable
    public data class HelloA(
      @ProtoNumber(number = 1)
      public val type: Type? = null,
    )
  }

  @Serializable
  public enum class Type {
    @ProtoNumber(number = 0)
    UNKNOWN,
    @ProtoNumber(number = 1)
    KNOWN,
  }
}
Dogacel commented 1 year ago

It's a good idea but unfortunately, I can't change the enum name because of business rules.

I see, this looks like a bug, wanna have a separate ticket for it? You are welcome to work on it too, I would be happy to see you as the first contributor 😃

Probably we should resolve this by removing the data property from classes without any properties.

tamnguyenhuy commented 1 year ago

Probably we should resolve this by removing the data property from classes without any properties.

Okay, @Dogacel. I will fix it asap!