Add kxrpc grpc plugin #224
Conversation
WalkthroughThis change introduces initial support for a gRPC plugin for Ktor using kotlinx-rpc. It adds version properties, a manifest file, a proto definition, generated stubs, a sample service implementation, application integration, documentation, version mapping, and a test verifying end-to-end gRPC functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/inside_app.kt (1)
5-9: Consider using a more descriptive function name.The function name
install()is generic and could be confusing since Ktor already has a built-ininstallfunction for features/plugins. Consider a more specific name likeinstallGrpcSample()orconfigureGrpcService().-fun Application.install() { +fun Application.installGrpcSample() { grpc { registerService<SampleService> { SampleServiceImpl() } } }The gRPC service registration logic itself looks correct for kotlinx-rpc integration.
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/SampleServiceImpl.kt (1)
1-5: Consider renaming the parameter for clarity.The parameter name
namefollowed by property accessname.namecould be confusing. Consider using a more descriptive parameter name.- override suspend fun greeting(name: ClientGreeting): ServerGreeting { - return ServerGreeting { content = "Hello, ${name.name}!" } + override suspend fun greeting(request: ClientGreeting): ServerGreeting { + return ServerGreeting { content = "Hello, ${request.name}!" } }The service implementation logic is correct and follows the expected pattern for kotlinx-rpc services.
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/test.kt (1)
12-17: Consider using dynamic port allocation to avoid conflicts.The hard-coded port 8081 could cause test failures if the port is already in use. Consider using a dynamic port or
testApplication's built-in port management.- application { - grpc(8081) { - registerService<SampleService> { SampleServiceImpl() } - } - } + application { + grpc { + registerService<SampleService> { SampleServiceImpl() } + } + }Then retrieve the actual port from the application for the client connection.
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/manifest.ktor.yaml (2)
8-10: Address the Amper support limitation.The TODO comment indicates that Amper support is disabled pending fixes. Consider documenting this limitation more prominently for users who might expect Amper support.
Would you like me to help create a more detailed explanation of the Amper limitation in the documentation or add it to a known issues section?
29-38: Clarify proto source set structure intention.The commented sections suggest uncertainty about the proto file structure. This aligns with the PR objective noting that "proto source sets do not currently work." Consider either removing these comments or converting them to a clear TODO with an issue reference.
-# I need src/main/proto structure -# -#mainSourceSet: -# - proto/service.proto - -# Or Maybe? -#src: -# - main: -# - proto: -# - proto/service.proto +# TODO: Add support for proto source sets (see issue reference) +# Planned structure: +# mainSourceSet: +# - proto/service.protoplugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/stub.kt (2)
3-9: Consider using immutable data classes for better design.The current implementation uses mutable properties, which can lead to unexpected side effects. Consider making the data classes immutable for better thread safety and predictability.
-class ClientGreeting { - var name: String = "" - - companion object { - operator fun invoke(body: ClientGreeting.() -> Unit): ClientGreeting = ClientGreeting().apply(body) - } -} +data class ClientGreeting( + val name: String = "" +) { + companion object { + operator fun invoke(body: ClientGreetingBuilder.() -> Unit): ClientGreeting { + return ClientGreetingBuilder().apply(body).build() + } + } +} + +class ClientGreetingBuilder { + var name: String = "" + fun build() = ClientGreeting(name) +}
11-17: Apply the same immutability improvement to ServerGreeting.Similar to
ClientGreeting,ServerGreetingshould also be immutable for consistency and better design practices.-class ServerGreeting { - var content: String = "" - - companion object { - operator fun invoke(body: ServerGreeting.() -> Unit): ServerGreeting = ServerGreeting().apply(body) - } -} +data class ServerGreeting( + val content: String = "" +) { + companion object { + operator fun invoke(body: ServerGreetingBuilder.() -> Unit): ServerGreeting { + return ServerGreetingBuilder().apply(body).build() + } + } +} + +class ServerGreetingBuilder { + var content: String = "" + fun build() = ServerGreeting(content) +}plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md (1)
34-34: Fix redundant phrasing as flagged by static analysis.The phrase "alongside with" is redundant. Use either "alongside" or "with".
-The `SampleService` interface will generated for you alongside with other types and helper declarations. +The `SampleService` interface will be generated for you alongside other types and helper declarations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
plugins/gradle.properties(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/SampleServiceImpl.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/inside_app.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/manifest.ktor.yaml(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/stub.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/test.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/versions.ktor.yaml(1 hunks)
🧰 Additional context used
🪛 Buf (1.55.1)
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto
3-3: Files with package "io.ktor" must be within a directory "io/ktor" relative to root but were in directory "plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto".
(PACKAGE_DIRECTORY_MATCH)
🪛 LanguageTool
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md
[style] ~34-~34: This phrase is redundant. Consider writing “alongside” or “with”.
Context: ...rvice` interface will generated for you alongside with other types and helper declarations. ...
(ALONGSIDE_WITH)
🔇 Additional comments (7)
plugins/gradle.properties (1)
6-6: LGTM! Version property added for the new gRPC plugin.The version format
0.10.0-grpc-121suggests this might be a pre-release or specialized build. Ensure this version is stable and available in the intended repository.plugins/server/org.jetbrains/kotlinx-rpc-grpc/versions.ktor.yaml (1)
1-4: LGTM — gRPC plugin dependency versions verified
- The version mapping for Ktor
"[3.0,)"correctly targets Ktor 3.0+org.jetbrains.kotlinx:kotlinx-rpc-grpc-ktor-server:$kotlinx-rpc-grpcproperly references the Gradle propertyio.grpc:grpc-netty:1.73.0andio.grpc:grpc-kotlin-stub:1.4.1were confirmed on Maven CentralNo further changes required.
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto (2)
3-3: Consider the package directory structure.The static analysis tool correctly identifies that package
io.ktorshould typically be placed in directoryio/ktorrelative to the proto root. While this might be acceptable for the plugin registry structure, it could cause issues with standard Protocol Buffer tooling and code generation.Consider whether this directory structure aligns with the project's proto conventions and tooling requirements.
5-15: LGTM! Proto definition is well-structured.The Protocol Buffers definition correctly uses proto3 syntax and defines appropriate message types and service contract. The field numbering and RPC method signature are properly structured for gRPC communication.
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/stub.kt (1)
19-21: LGTM for the service interface design.The
SampleServiceinterface correctly uses a suspend function, which is appropriate for gRPC operations that are inherently asynchronous.plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md (2)
5-11: LGTM for the comprehensive disclaimer.The disclaimer appropriately sets expectations about the dev preview status and encourages community feedback through multiple channels.
69-69: Unable to locate client cleanup methods – please verify consistencyI wasn’t able to find any
close(),shutdown()orawaitTermination()methods on the generated gRPC client in the repository. Before we update the docs, please manually confirm how yourgreeterClient(…)(or other RPC client factory) actually exposes its shutdown API:
- Verify the return type of your
greeterClient()(or equivalent) function:
• If it implementsAutoCloseable/Closeable, usingclient.close()in the docs is correct.
• If it returns a raw gRPC channel (e.g.,ManagedChannel), you’ll need to useclient.shutdown()followed byclient.awaitTermination(...).- Once you’ve confirmed the real API, update
documentation.md(line 69) to match the client’s actual cleanup methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto (1)
9-11: Field numbering gap is harmless but unconventional
contentstarts at field #2, leaving #1 unused. While legal, keeping numbering dense avoids confusion for future schema evolution.- string content = 2; + string content = 1;If #1 is intentionally reserved, add an explicit
reserved 1;statement.plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md (1)
34-34: Fix grammar and remove redundant phrasing-The `SampleService` interface will generated for you alongside with other types and helper declarations. +The `SampleService` interface will be generated for you alongside other types and helper declarations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
plugins/gradle.properties(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/SampleServiceImpl.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/inside_app.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/manifest.ktor.yaml(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/stub.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/test.kt(1 hunks)plugins/server/org.jetbrains/kotlinx-rpc-grpc/versions.ktor.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/stub.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/SampleServiceImpl.kt
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/inside_app.kt
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/versions.ktor.yaml
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/test.kt
- plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/manifest.ktor.yaml
- plugins/gradle.properties
🧰 Additional context used
🪛 LanguageTool
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/documentation.md
[style] ~34-~34: This phrase is redundant. Consider writing “alongside” or “with”.
Context: ...rvice` interface will generated for you alongside with other types and helper declarations. ...
(ALONGSIDE_WITH)
🪛 Buf (1.55.1)
plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto/service.proto
3-3: Files with package "io.ktor" must be within a directory "io/ktor" relative to root but were in directory "plugins/server/org.jetbrains/kotlinx-rpc-grpc/3.0/proto".
(PACKAGE_DIRECTORY_MATCH)
| sources: | ||
| - SampleServiceImpl.kt | ||
|
|
||
| # I need src/main/proto structure | ||
| # | ||
| #mainSourceSet: | ||
| # - proto/service.proto | ||
|
|
||
| # Or Maybe? | ||
| #src: | ||
| # - main: | ||
| # - proto: |
There was a problem hiding this comment.
I'm updating it so you can just write:
sources:
- SampleServiceImpl.kt
- service.proto
And it'll move the proto file into its own source folder.
There was a problem hiding this comment.
@bjhham is this solution much easier than to make custom source mapping?
There was a problem hiding this comment.
Yes, custom source mappings would need a bit of extra work to ensure they play nice with the gradle vs. amper source folder layout logic. The new project templating engine I'm working on will have support for all that though.
One thing that doesn't work: proto source sets