Add method stream type to generated code #172
Conversation
It is useful to have the stream type in the generated code (similar to other Connect implementations). In the future we may use this information to optimize stream handling - for example, avoiding the pipe/duplex/oneshot for server streaming calls to enable Connect over HTTP/1.1. Update the generator to include the stream type in the generated code. Additionally, move the HTTP method to the HTTPRequest class - mutating a method spec to determine which HTTP method to use seems wrong. We should keep MethodSpec to contain only fields set from the generator.
| */ | ||
| class MethodSpec<Input : Any, Output : Any>( | ||
| val path: String, | ||
| val requestClass: KClass<Input>, | ||
| val responseClass: KClass<Output>, | ||
| val idempotency: Idempotency = Idempotency.UNKNOWN, | ||
| val method: String = Method.POST_METHOD, |
There was a problem hiding this comment.
This did not make sense to live on the MethodSpec - it should have lived on the HTTPRequest. Now the MethodSpec type only contains fields assigned by the code generator.
| @@ -216,7 +216,7 @@ class ProtocolClient( | |||
| isComplete = true | |||
| when (streamResult.code) { | |||
| Code.OK -> channel.close() | |||
| else -> channel.close(streamResult.connectException() ?: ConnectException(code = streamResult.code)) | |||
| else -> channel.close(streamResult.connectException() ?: ConnectException(code = streamResult.code, exception = streamResult.cause)) | |||
There was a problem hiding this comment.
There were a few places where we were missing specifying the ConnectException's underlying cause (if set).
protoc-gen-connect-kotlin/src/main/kotlin/com/connectrpc/protocgen/connect/Generator.kt
Outdated
Show resolved
Hide resolved
library/src/test/kotlin/com/connectrpc/impl/BiDirectionalStreamTest.kt
Outdated
Show resolved
Hide resolved
| val idempotency: Idempotency = Idempotency.UNKNOWN, | ||
| val method: String = Method.POST_METHOD, | ||
| val streamType: StreamType = StreamType.UNKNOWN, |
There was a problem hiding this comment.
This unknown value seems a little scary. How will it impact things? Is it necessary for backwards-compatibility with older generated code? What could go wrong if older code is paired with this newer runtime -- would it mean that the GET HTTP verb is never used and interceptors would see the wrong value for stream type? I think it might be safer to make this a non-optional parameter with no default and remove the UNKNOWN sentinel enum entry. Users must re-generate their code when they update to newer version of the library.
There was a problem hiding this comment.
I went back and forth on this. Having UNKNOWN also matches connect-go and grpc-java. connect-swift doesn't have an unknown type.
It was less about the compatibility with existing generated code as this is still pre-v1 and it should be expected that consumers keep the runtime libraries in sync with the generated code. I'll make the change to make it required.
There was a problem hiding this comment.
I know connect-go has this because there's no real support for enums in Go, and it's just good practice for the default/zero value to be a sentinel of this sort. But in kotlin and java, it's much less clear why you'd include one. With grpc-java, it's likely because that supports server-side, too, and if you want to create an "unknown method handler", it's most correct for it to report to handler/interceptor code that the stream type is unknown.
It is useful to have the stream type in the generated code (similar to other Connect implementations). In the future we may use this information to optimize stream handling - for example, avoiding the pipe/duplex/oneshot for server streaming calls to enable Connect over HTTP/1.1. Update the generator to include the stream type in the generated code.
Additionally, move the HTTP method to the HTTPRequest class - mutating a method spec to determine which HTTP method to use seems wrong. We should keep MethodSpec to contain only fields set from the generator.