-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KTOR-8602 Fix handling of optional module parameters #4959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes update parameter resolution logic in the server engine to prioritize dependency injection before throwing exceptions for "Application"-typed parameters. In tests, module tracking is refactored from a single attribute to a set of loaded module names, with new extension functions and assertions reflecting this approach. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-server/ktor-server-core/jvm/test/io/ktor/tests/hosts/EmbeddedServerReloadingTests.kt (1)
9-28: Review import additions for necessity and compliance.The new imports generally look appropriate for the added functionality. However, line 18 imports
addLoadedModulefrom the same class's companion object, which may be unnecessary since it's accessible within the same class scope.Consider removing the self-import:
-import io.ktor.tests.hosts.EmbeddedServerReloadingTests.Companion.addLoadedModule
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/internal/CallableUtils.kt(2 hunks)ktor-server/ktor-server-core/jvm/test/io/ktor/tests/hosts/EmbeddedServerReloadingTests.kt(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-server/ktor-server-core/jvm/src/io/ktor/server/engine/internal/CallableUtils.kt
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.kt`: Include copyright header in new files. Use star imports for io.ktor.* packages (configured in .editorconfig). Max line length: 120 characters. Indent with 4 spaces.
**/*.kt: Include copyright header in new files.
Use star imports for io.ktor.* packages (configured in .editorconfig).
Max line length: 120 characters.
Indent with 4 spaces.
ktor-server/ktor-server-core/jvm/test/io/ktor/tests/hosts/EmbeddedServerReloadingTests.kt
`**/test/**`: Tests are organized by platform in module's test directories.
**/test/**: Tests are organized by platform in module's test directories.
ktor-server/ktor-server-core/jvm/test/io/ktor/tests/hosts/EmbeddedServerReloadingTests.kt
🔇 Additional comments (3)
ktor-server/ktor-server-core/jvm/test/io/ktor/tests/hosts/EmbeddedServerReloadingTests.kt (3)
399-402: Excellent test coverage for different default parameter types.The test now properly validates the handling of multiple module functions with different types of default parameters (Boolean, function type, and inline value class), which directly addresses the PR objectives for KTOR-8602 and KTOR-8608.
Also applies to: 415-422
529-536: Well-designed module tracking infrastructure.The new module tracking approach using a set is more flexible than the previous single-attribute method and properly supports testing multiple module functions. The use of
computeIfAbsentensures thread-safe initialization of the mutable set.
677-690: Comprehensive testing of problematic default parameter types.These module functions excellently test the specific issues mentioned in the PR objectives:
defaultArgBoolean: Standard Boolean default parameterdefaultArgContainingApplicationWord: Function type withApplicationreceiver (KTOR-8602)defaultArgInline: Inline value class default parameter (KTOR-8608)The
InstanceIdinline value class is properly defined and demonstrates the inline default argument handling fix.
e68b729 to
5b7e709
Compare
Subsystem
Server
Motivation
KTOR-8602 Module parameter type Application.() -> kotlin.Unit is not supported in 3.2.0
KTOR-8608 Ktor fails to boot with default jvminline argument
Solution