servicetalk-grpc-protoc name conflict fixes #2157
Conversation
Motivation: servicetalk-grpc-protoc attempts to detect naming conflicts and appends numbers to the end of the class names to avoid the conflict. However in the event `java_outer_classname` isn't specified and the service type name conflicts with the file name a conflict is incorrectly detected which leads to forcing a `0` at the end of the service name. Detection of implicit class name isn't consistent with protobuf-java which may result in not generating code in the same file when `java_multiple_files = false`. Types (messages, enums) that start with a lower case value would fail to compile due to CLassName.bestGuess restrictions. Potential User Changes: If you are in this scenario you must regenerate code and your class names will change, requiring code changes.
Scottmitch
force-pushed
the
grpc_protoc_conflict
branch
from
2f8e1b0 to
b630be6
Compare
| } | ||
|
|
||
| // todo(scott): make first char lowercase | ||
| // https://github.com/protocolbuffers/protobuf/issues/9653 |
There was a problem hiding this comment.
once protocolbuffers/protobuf#9653 is fixed, these tests can be updated to validate behavior.
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-protoc/src/main/java/io/servicetalk/grpc/protoc/FileDescriptor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code changes LGTM.
If you are in this scenario you must regenerate code and your class names will change, requiring code changes.
IIUC the risk is minimal: (1) not many users are in this situation; (2) if they are, they likely already use java_outer_classname; (3) if they don't, they can use java_outer_classname before upgrading to the next version to avoid any code changes as part of the version bump. Right?
Trying to see if it's ok to merge into 0.42.x or defer until 0.43
servicetalk-grpc-protoc/src/test/proto/test_conflict_enum.proto
Outdated
Show resolved
Hide resolved
servicetalk-grpc-protoc/src/test/proto/test_conflict_enum.proto
Outdated
Show resolved
Hide resolved
|
@idelpivnitskiy - yes I agree with your assessment that the risk is minimal. Relying on default behavior is not well defined when the name isn't specified and you can even see that protobuf compiler has some uncovered bugs in this department. |
Motivation:
servicetalk-grpc-protoc attempts to detect naming conflicts
and appends numbers to the end of the class names to avoid
the conflict. However in the event
java_outer_classnameisn'tspecified and the service type name conflicts with the file name
a conflict is incorrectly detected which leads to forcing a
0at the end of the service name.
Detection of implicit class name isn't consistent with
protobuf-java which may result in not generating code in the
same file when
java_multiple_files = false.Types (messages, enums) that start with a lower case value
would fail to compile due to CLassName.bestGuess restrictions.
Potential User Changes:
If you are in this scenario you must regenerate code and your
class names will change, requiring code changes.