Add nullability support for server parameter delegation#5486
Conversation
|
I'm a bit unsure how reliable |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGeneric type constraints for Parameters delegated accessors were relaxed to allow nullable result types; implementation now returns null for absent parameters when the target type is nullable and preserves existing exceptions for non-nullable types and conversion failures. ChangesParameters nullable handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
bjhham
left a comment
There was a problem hiding this comment.
Looks like an easy win 👍
I'm pretty sure isMarkedNullable works cross-platform on reified KTypes.
I don't know why we didn't just implement it that way in the first place...
|
FYI logged an issue to handle property delegation in the compiler plugin: |
|
@osipxd I updated the PR to move the nullability check in the actual implementation. This way we now also support nullable types for
Could you review again? Thank you for the careful review and pointing it out! 🙏 We're still getting a better outcome thanks to it! |
…getOrFailImpl`. This way both `getOrFail` and `getValue` now support nullable types.
1056631 to
2973653
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ktor-server/ktor-server-core/common/src/io/ktor/server/util/Parameters.kt`:
- Around line 53-54: Update the KDoc for getOrFail<R> to reflect its new
nullable-missing behavior: state that when R is non-nullable a missing parameter
throws MissingRequestParameterException, but when R is a nullable type the
function returns null for missing values; also keep the existing note that
ParameterConversionException is thrown when conversion from String to R fails.
Modify the documentation on the getOrFail<R> declaration to mention the
conditional behavior based on R's nullability and include both exception and
null-return cases for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb869399-a278-451d-b6c4-7876eda331cb
📒 Files selected for processing (3)
ktor-server/ktor-server-core/api/ktor-server-core.klib.apiktor-server/ktor-server-core/common/src/io/ktor/server/util/Parameters.ktktor-server/ktor-server-core/common/test/io/ktor/server/http/ParametersTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- ktor-server/ktor-server-core/common/test/io/ktor/server/http/ParametersTest.kt
- ktor-server/ktor-server-core/api/ktor-server-core.klib.api
| * @throws MissingRequestParameterException if no values associated with this [name] | ||
| * @throws ParameterConversionException when conversion from String to [R] fails |
There was a problem hiding this comment.
Update generic getOrFail KDoc to include nullable-missing behavior.
getOrFail<R> now returns null for missing values when R is nullable, but this KDoc still says missing always throws. Please align docs with the new contract.
As per coding guidelines: "Keep KDoc documentation correct when behavior/signatures change".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ktor-server/ktor-server-core/common/src/io/ktor/server/util/Parameters.kt`
around lines 53 - 54, Update the KDoc for getOrFail<R> to reflect its new
nullable-missing behavior: state that when R is non-nullable a missing parameter
throws MissingRequestParameterException, but when R is a nullable type the
function returns null for missing values; also keep the existing note that
ParameterConversionException is thrown when conversion from String to R fails.
Modify the documentation on the getOrFail<R> declaration to mention the
conditional behavior based on R's nullability and include both exception and
null-return cases for clarity.
Subsystem
Server, core
Motivation
Currently it is not possible to use nullable values with parameter delegation.
Solution
This PR add support by removing the
: Anyconstraint and relying ontypeInfo.kotlinType?.isMarkedNullableto check if null is allowed or not.