KTOR-9176 KTOR-8997 Improve examples and flow in dependency injection + document configuration options #763
Conversation
vnikolova
changed the title
WalkthroughAdds a new server-di code snippet module with Ktor DI sample (sources, build, tests, resources, HTTP examples) and integrates it into codeSnippets/settings.gradle.kts. Reorganizes and expands dependency-injection documentation into a grouped "Dependency injection" TOC with five dedicated topic files and a condensed overview. 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
| database: | ||
| connectionUrl: postgres://localhost:3037/admin |
There was a problem hiding this comment.
Non-standard PostgreSQL port in sample configuration.
Port 3037 is unusual for PostgreSQL (default is 5432). If this is intentional for demonstration purposes, consider adding a YAML comment to avoid confusing readers who may copy this config.
🤖 Prompt for AI Agents
In `@codeSnippets/snippets/server-di/src/main/resources/application.yaml` around
lines 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.
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.
| get("/paymment/{amount}") { | ||
| call.respondText("Payment for ${call.parameters["amount"]} is pending...") | ||
| } |
There was a problem hiding this comment.
Typo in route path: /paymment → /payment.
Double 'm' in the route path.
Proposed fix
- get("/paymment/{amount}") {
+ get("/payment/{amount}") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get("/paymment/{amount}") { | |
| call.respondText("Payment for ${call.parameters["amount"]} is pending...") | |
| } | |
| get("/payment/{amount}") { | |
| call.respondText("Payment for ${call.parameters["amount"]} is pending...") | |
| } |
🤖 Prompt for AI Agents
In
`@codeSnippets/snippets/server-di/src/main/kotlin/com.example/PaymentService.kt`
around lines 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.
| ```yaml | ||
| ``` | ||
| {src="snippets/server-di/src/main/resources/application.yaml" include-lines="1,4-6,13-14"} | ||
|
|
There was a problem hiding this comment.
Incorrect include-lines — shows payment modules instead of database configuration.
The surrounding text explains that database.connectionUrl is resolved from the configuration, but include-lines="1,4-6,13-14" pulls in the payment-related module entries (lines 13–14 of application.yaml). You likely want lines 15–16 to show the database section:
Proposed fix
-{src="snippets/server-di/src/main/resources/application.yaml" include-lines="1,4-6,13-14"}
+{src="snippets/server-di/src/main/resources/application.yaml" include-lines="1,4-6,15-16"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```yaml | |
| ``` | |
| {src="snippets/server-di/src/main/resources/application.yaml" include-lines="1,4-6,13-14"} |
🤖 Prompt for AI Agents
In `@topics/server-di-dependency-resolution.md` around lines 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.
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.
| @host = http://localhost:8080 | ||
|
|
||
| ### Successful checkout using cookies and amount | ||
| POST http://localhost:8080/checkout?amount=1500 | ||
| Content-Type: application/x-www-form-urlencoded | ||
| Cookie: userId=alice; cartId=cart123 |
There was a problem hiding this comment.
@host variable defined but never used; cartId value inconsistent across requests.
The @host variable on line 1 is never referenced — all requests hardcode http://localhost:8080. Either use {{host}}/checkout... in the request URLs or remove the variable.
Also, the successful checkout (line 6) uses cartId=cart123 while the other two scenarios (lines 13, 20) use cartId=cart-123. Consider making the format consistent to avoid confusing readers of the sample.
Proposed fix
-@host = http://localhost:8080
-
### Successful checkout using cookies and amount
-POST http://localhost:8080/checkout?amount=1500
+POST {{host}}/checkout?amount=1500
Content-Type: application/x-www-form-urlencoded
-Cookie: userId=alice; cartId=cart123
+Cookie: userId=alice; cartId=cart-123
###
### Missing userId cookie
-POST http://localhost:8080/checkout?amount=1500
+POST {{host}}/checkout?amount=1500
Content-Type: application/x-www-form-urlencoded
Cookie: cartId=cart-123
###
### Missing amount query parameter
-POST http://localhost:8080/checkout
+POST {{host}}/checkout
Content-Type: application/x-www-form-urlencoded
Cookie: userId=alice; cartId=cart-123🤖 Prompt for AI Agents
In `@codeSnippets/snippets/server-di/requests.http` around lines 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.
| <def> | ||
| <title><code>Default</code></title> | ||
| Throws an exception when a conflicting dependency is declared | ||
| </def> |
There was a problem hiding this comment.
Missing trailing period for consistency.
The Default policy description on line 90 is missing a period, unlike the OverridePrevious entry on line 94.
Proposed fix
<def>
<title><code>Default</code></title>
-Throws an exception when a conflicting dependency is declared
+Throws an exception when a conflicting dependency is declared.
</def>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <def> | |
| <title><code>Default</code></title> | |
| Throws an exception when a conflicting dependency is declared | |
| </def> | |
| <def> | |
| <title><code>Default</code></title> | |
| Throws an exception when a conflicting dependency is declared. | |
| </def> |
🤖 Prompt for AI Agents
In `@topics/server-di-configuration.md` around lines 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.
| ```kotlin | ||
| fun test() = testApplication { | ||
| application { | ||
| dependencies.provide<MyService> { | ||
| MockService() | ||
| } | ||
| loadServices() | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Clarify what loadServices() represents.
loadServices() on line 23 isn't a standard Ktor function and isn't explained. Readers won't know whether this is a placeholder for their own module-loading logic or a real API. Consider replacing it with a more recognizable pattern (e.g., calling an actual application module function) or adding a brief comment/sentence explaining its purpose.
🤖 Prompt for AI Agents
In `@topics/server-di-testing.md` around lines 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.
1 participant
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"