Conversation
vnikolova
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
vnikolova
changed the title
![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
🤖 Fix all issues with AI agents
In `@codeSnippets/snippets/server-di/src/main/kotlin/com.example/Logging.kt`:
- Line 7: There is an extra leading space before the class declaration; remove
the leading space so the declaration "class Logger(private val out: PrintStream)
{" aligns with the file's top-level indentation and other type declarations
(i.e., adjust the "class Logger" line to start at column 1 with no leading
spaces).
In `@codeSnippets/snippets/server-di/src/main/resources/application.yaml`:
- Around line 13-14: The sample database connectionUrl uses a non-standard
PostgreSQL port (postgres://localhost:3037/admin); either change the port to the
standard 5432 in the database.connectionUrl value or add a YAML comment next to
database.connectionUrl explaining that 3037 is intentionally non-standard for
demo purposes so readers aren't confused when copying the config.
In `@topics/server-di-configuration.md`:
- Line 64: Add a trailing period to the sentence "It will not resolve as
`Collection?`, because that combination is not included in the expression" so it
ends with a period; update the text in topics/server-di-configuration.md (the
sentence containing `Collection?` in that paragraph) to be punctuated
consistently.
🧹 Nitpick comments (6)
codeSnippets/snippets/server-di/build.gradle.kts (1)
1-3:kotlin_versionis declared but unused.The
kotlin_versionproperty is read from the project but never referenced in this build script. Consider removing it to avoid confusion.Proposed fix
val ktor_version: String by project -val kotlin_version: String by project val logback_version: String by projecttopics/server-di-resource-lifecycle-management.md (2)
39-46: Undeclared typesCloserandCustomCloserin the snippet may confuse readers.Unlike the other snippets which use recognizable names (
DatabaseConnection,ResourceManager),CloserandCustomCloserappear without context. Consider using more descriptive names (e.g.,ResourceHandler/CustomResourceHandler) or adding a brief comment indicating what they represent.
11-21: Indentation uses 2 spaces instead of the Kotlin convention of 4 spaces.The inline Kotlin snippets use 2-space indentation. Kotlin's official coding conventions recommend 4 spaces. This applies to the other snippets in this file as well.
topics/server-di-dependency-registration.md (1)
84-92: Tabs block only contains a YAML tab — consider adding HOCON.The
<tabs>element has only anapplication.yamltab. If Ktor's file-based DI configuration also supportsapplication.conf(HOCON), adding that tab would be consistent with other Ktor documentation pages that show both formats side by side.topics/server-di-testing.md (1)
9-17: Consider referencing the actual sample project test instead of (or in addition to) inline pseudo-code.The PR includes a concrete test in
GreetingTest.ktwithFakeGreetingService,FakeUserRepository, etc. Referencing it here (using{src=...}) would give readers a working, runnable example rather than genericMyService/MockServicepseudo-code, and would keep the documentation consistent with the approach used in the registration topic.topics/server-di-configuration.md (1)
79-93:IgnoreConflictsdescription only mentions test behavior, not the general policy.The descriptions for
DefaultandOverridePreviousexplain what the policy does, butIgnoreConflictsonly explains its default usage in tests. Consider adding a brief description of the general behavior (e.g., "Keeps the first registered dependency and silently ignores subsequent conflicting registrations") before mentioning the test-environment default.
codeSnippets/snippets/server-di/src/main/kotlin/com.example/Logging.kt
Outdated
Show resolved
Hide resolved
codeSnippets/snippets/server-di/src/main/resources/application.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt`:
- Around line 58-60: The route path in the GET handler is misspelled as
"/paymment/{amount}"; update the route string used in the get(...) call inside
PaymentService (the GET handler that calls call.respondText with
call.parameters["amount"]) to "/payment/{amount}" and verify any tests or
callers referencing "/paymment" are updated to the corrected "/payment" path.
In `@topics/server-di-dependency-resolution.md`:
- Around line 66-69: The included snippet uses incorrect include-lines for
src/main/resources/application.yaml so the example shows payment modules instead
of the database config; update the include-lines from "1,4-6,13-14" to the lines
that contain the database section (the lines with the database.connectionUrl and
database keys, e.g., change to include the lines covering the database block
such as 15-16) so the YAML excerpt demonstrates database.connectionUrl
resolution correctly.
🧹 Nitpick comments (2)
codeSnippets/snippets/server-di/src/main/resources/application.yaml (1)
23-25: Sample secret in configuration is fine for demo, but consider adding a comment.The
clientKeyvalue"super-secret-client-key"is clearly a placeholder, which is appropriate for a sample project. A brief YAML comment (e.g.,# Replace with an environment variable in production) could reinforce best practices for readers who copy-paste this config.codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt (1)
28-31: User-supplied values in redirect URL are not URL-encoded.
userId,cartId, andamountare interpolated directly into the redirect URL. While this is a sample project, it could mislead readers. Consider usingURLBuilderorencodeURLParameter()for the query parameters to demonstrate best practices.
codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt
Outdated
Show resolved
Hide resolved
vnikolova
force-pushed
the
vnikolova/di-updates
branch
from
d9b7ca0 to
6fece44
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@codeSnippets/snippets/server-di/requests.http`:
- Around line 1-6: The `@host` variable is defined but unused and the cartId
values are inconsistent; update the HTTP requests to use the host variable
(replace hardcoded "http://localhost:8080" with "{{host}}" in each request URL)
or delete the `@host` line if you prefer hardcoding, and make all Cookie headers
use a consistent cartId format (e.g., change "cart123" to "cart-123" in the
successful checkout request) so the Cookie header and all request URLs (the POST
/checkout requests) are consistent.
In `@topics/server-di-configuration.md`:
- Around line 88-91: The sentence for the <code>Default</code> policy title is
missing a trailing period; update the description text for the
<code>Default</code> entry to add a period at the end so it matches the
punctuation style of the <code>OverridePrevious</code> entry and other entries.
In `@topics/server-di-testing.md`:
- Around line 17-26: Clarify that loadServices() is a placeholder for the app's
module/bootstrap function by updating the example to call or comment that it
should call your real module initialization (e.g., the function that registers
routes/services) instead of a Ktor API; reference loadServices(),
testApplication, application, dependencies.provide<MyService>, and MockService
so readers know to replace loadServices() with their actual application module
bootstrap or leave a short inline comment explaining that loadServices()
represents the project's module-loading/DI registration step.
🧹 Nitpick comments (6)
codeSnippets/snippets/server-di/src/main/kotlin/com.example/AsyncDependencies.kt (1)
10-13: Consider noting the provide-before-resolve ordering requirement.
installEvents()callsresolve()which will fail ifloadEventsConnection()(theprovidecall) hasn't been invoked first. Since this is a documentation snippet, ensure the accompanying topic text or the calling code makes the required ordering clear to readers. If it's already addressed in the docs, feel free to disregard.topics/server-di-configuration.md (1)
96-100:IgnoreConflictsdescription reads more like a usage note than a definition.The other two policy entries (
Default,OverridePrevious) describe what the policy does. TheIgnoreConflictsentry instead describes where it's used by default. Consider leading with the behavior (e.g., "Silently keeps the first registration and ignores subsequent conflicting ones.") and then adding the note about test-engine defaults as supplementary detail.topics/server-di-testing.md (1)
13-41: Consider referencing the sample project's test for a more complete example.The testing page is quite minimal. The PR adds
GreetingTest.ktin theserver-disample project which demonstrates a full test flow with fakes and endpoint assertions. Referencing or including that snippet would give readers a concrete, runnable example alongside the conceptual snippets here.codeSnippets/snippets/server-di/build.gradle.kts (1)
19-29: Inconsistent string interpolation style.Some dependency declarations use bare
$ktor_version(e.g., lines 20–22) while others use${ktor_version}(lines 23, 26). Both are functionally identical, but picking one style improves readability.codeSnippets/snippets/server-di/src/test/kotlin/GreetingTest.kt (1)
23-29: Consider making the fake greeting include thenameparameter for a more realistic example.Since this is a documentation sample, it could be more instructive if
FakeGreetingService.greetused thenameparameter (e.g.,"Fake greeting, $name"), and the assertion verified"Fake greeting, Test". This would better demonstrate that the route correctly passes path parameters through to the service.codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt (1)
10-12: Consider separatingApplicationCallfrom thePaymentProcessorinterface.Passing
ApplicationCallinto a domain-level interface couples payment processing to Ktor's HTTP layer. For a documentation sample that aims to showcase DI best practices, having the processor return a result object and letting the route handler translate it to an HTTP response would better demonstrate the decoupling benefits of DI.That said, this is a sample project and the current approach keeps things concise, so this is a minor point.
zibet27
left a comment
zibet27
left a comment
There was a problem hiding this comment.
Lgtm
Check code rabbit comments
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt (1)
54-54: Consider validating thatamountis positive.The
toLongOrNull()check ensures valid numeric input, but negative or zero amounts would still be processed. For sample code demonstrating payment handling, adding a positivity check would reinforce best practices.💡 Optional validation enhancement
- val amount = call.request.queryParameters["amount"]?.toLongOrNull() ?: return@post call.respondText("Amount missing", status = HttpStatusCode.BadRequest) + val amount = call.request.queryParameters["amount"]?.toLongOrNull()?.takeIf { it > 0 } + ?: return@post call.respondText("Invalid amount", status = HttpStatusCode.BadRequest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt` at line 54, The current parsing of query parameter "amount" accepts non-positive values; update the handler that parses the amount (variable amount) in PaymentService.kt to validate amount > 0 after toLongOrNull(), and if amount is null or <= 0 return@post call.respondText with a BadRequest status and a clear message like "Amount must be a positive number" so negative or zero amounts are rejected before further processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeSnippets/snippets/server-di/src/test/kotlin/PaymentTest.kt`:
- Around line 14-19: The test creates two MockPaymentProcessor instances so the
DI registration via provide<PaymentProcessor>("external") is unused; fix by
using the same instance for both or by letting DI resolve the dependency: create
one val mock = MockPaymentProcessor() and pass that to
provide<PaymentProcessor>("external") and to paymentsHandling(mock), or
alternatively remove the direct paymentsHandling(...) call parameter so
paymentsHandling() resolves PaymentProcessor from DI instead; update references
to MockPaymentProcessor, provide<PaymentProcessor>("external"), and
paymentsHandling accordingly.
---
Nitpick comments:
In
`@codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt`:
- Line 54: The current parsing of query parameter "amount" accepts non-positive
values; update the handler that parses the amount (variable amount) in
PaymentService.kt to validate amount > 0 after toLongOrNull(), and if amount is
null or <= 0 return@post call.respondText with a BadRequest status and a clear
message like "Amount must be a positive number" so negative or zero amounts are
rejected before further processing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
codeSnippets/snippets/server-di/requests.httpcodeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.ktcodeSnippets/snippets/server-di/src/main/resources/application.yamlcodeSnippets/snippets/server-di/src/test/kotlin/PaymentTest.kttopics/server-di-configuration.mdtopics/server-di-dependency-resolution.mdtopics/server-di-testing.md
🚧 Files skipped from review as they are similar to previous changes (2)
- topics/server-di-testing.md
- codeSnippets/snippets/server-di/requests.http
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Ktor dependency injection documentation from a single monolithic page into a well-organized multi-page structure, adds comprehensive code examples through a new server-di sample project, and expands coverage of DI-related configuration options.
Changes:
- Split the dependency injection documentation into six focused topics: Overview, Configuration, Dependency Registration, Dependency Resolution, Resource Lifecycle Management, and Testing
- Created a new
server-disample project with working code examples demonstrating DI features including named dependencies, async resolution, resource cleanup, and testing patterns - Added documentation for DI configuration options (keyMapping and conflictPolicy) that were previously undocumented
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| topics/server-dependency-injection.md | Refactored from comprehensive guide to focused overview with links to detailed sub-topics |
| topics/server-di-configuration.md | New page documenting DI plugin configuration options for key mapping and conflict policies |
| topics/server-di-dependency-registration.md | New page covering all dependency registration methods (lambdas, constructors, classes, functions, named dependencies) |
| topics/server-di-dependency-resolution.md | New page explaining dependency resolution including property delegation, direct resolution, async resolution, and module injection |
| topics/server-di-resource-lifecycle-management.md | New page documenting AutoCloseable support, custom cleanup logic, and scoped cleanup |
| topics/server-di-testing.md | New page covering testing utilities including dependency overriding and configuration loading |
| ktor.tree | Updated table of contents to reflect new hierarchical documentation structure |
| codeSnippets/snippets/server-di/* | New sample project demonstrating DI features with services, repositories, payment handling, and tests |
| codeSnippets/settings.gradle.kts | Registered the new server-di sample project |
Comments suppressed due to low confidence (6)
codeSnippets/snippets/server-di/src/test/kotlin/PaymentTest.kt:19
- The test both registers the mock processor in the DI container (line 17) and passes it directly as a parameter to
paymentsHandling(line 19). Since the function is being called with an explicit argument, the DI registration is redundant and potentially confusing. Either callpaymentsHandling()without arguments to test DI injection, or remove the DI registration if testing with direct dependency passing.
dependencies {
provide<PaymentProcessor>("external") { mockProcessor }
}
paymentsHandling(mockProcessor)
codeSnippets/snippets/server-di/src/main/kotlin/com.example/Logging.kt:8
- The function has inconsistent indentation with an extra leading space. The line should start at the same indentation level as the rest of the class members.
fun log(message: String) {
codeSnippets/snippets/server-di/build.gradle.kts:23
- Variable reference style is inconsistent. Line 23 uses
${ktor_version}with braces while other lines (20-22, 27) use$ktor_versionwithout braces. For consistency, use the same style throughout (preferably without braces when the variable name is unambiguous).
implementation("io.ktor:ktor-server-config-yaml:${ktor_version}")
codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt:39
- The
providefunction is called with a name parameter but without a type parameter. According to the named dependency registration pattern shown elsewhere in the documentation, when providing a named dependency, you should specify the type parameter. This should beprovide<PaymentProcessor>("external") { ... }to match the pattern and ensure proper type-safe resolution.
provide("external") { CreditCardPaymentProvider(baseUrl, clientKey) { it.reversed() } }
codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt:54
- This line is extremely long and difficult to read. Consider breaking it into multiple lines for better readability. The null-check and early return could be formatted across multiple lines to improve maintainability.
val amount = call.request.queryParameters["amount"]?.toLongOrNull() ?: return@post call.respondText("Amount missing", status = HttpStatusCode.BadRequest)
topics/server-dependency-injection.md:21
- The hyphen character in "built‑in" appears to be a non-breaking hyphen (U+2011) instead of a regular hyphen-minus (U+002D). This is inconsistent with other hyphenated terms like "type-safe" used elsewhere in the documentation. For consistency, use a regular hyphen: "built-in".
Ktor provides a built‑in DI plugin that lets you register services and configuration objects once and access them
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description:
server-diand reference code from it where applicablePreview on staging
Related issues:
KTOR-8997 Documentation for DI: Allow file configuration
KTOR-9176 Improve examples and flow in Dependency Injection
KTOR-9240 Document named dependency registration
KTOR-9294 Provide examples for nested dependencies in "Dependency Injection"