-
Notifications
You must be signed in to change notification settings - Fork 14
feature: POC enforce-not-null-strategy #199
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
feature: POC enforce-not-null-strategy #199
Conversation
|
Thank you very much, I like the idea 👍 I will have a look on the code in the next days |
|
Hello @mcarleio |
mcarleio
left a comment
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.
Sorry for the delay. I think there are only minor changes to do.
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/EnforceNotNullStrategy.kt
Show resolved
Hide resolved
processor/src/testFixtures/kotlin/io/mcarle/konvert/processor/KonverterITest.kt
Outdated
Show resolved
Hide resolved
converter/src/test/kotlin/io/mcarle/konvert/converter/SameTypeConverterITest.kt
Outdated
Show resolved
Hide resolved
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/EnforceNotNullStrategy.kt
Outdated
Show resolved
Hide resolved
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/KonvertOptions.kt
Outdated
Show resolved
Hide resolved
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/KonvertOptions.kt
Show resolved
Hide resolved
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/extensions.kt
Outdated
Show resolved
Hide resolved
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/AbstractTypeConverter.kt
Outdated
Show resolved
Hide resolved
| * generates `requireNotNull(expression) { "Value for '<expression>' must not be null" }` | ||
| */ | ||
| protected fun applyNotNullEnforcementIfNeeded( | ||
| fieldName: String, |
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.
In most cases a fieldName should work, but I think there are places, where there is no fieldName 🤔 If I am not mistaken, the IterableToX and MapToX have special cases.
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.
I slightly adjusted the implementation so it works uniformly for all cases.
|
@mcarleio Let me know if you think more tests are needed, or if the current coverage is sufficient. |
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.
Pull request overview
This PR introduces a configurable strategy for enforcing non-null mapping in Konvert. Currently, Konvert uses the !! assertion operator which throws raw NullPointerExceptions. The new konvert.enforce-not-null-strategy option allows choosing between the existing ASSERTION_OPERATOR (default) and a new REQUIRE_NOT_NULL strategy that generates requireNotNull() calls with descriptive error messages. This change is backward-compatible and initially implemented in SameTypeConverter as proof-of-concept.
Key changes:
- Added
EnforceNotNullStrategyenum withASSERTION_OPERATORandREQUIRE_NOT_NULLoptions - Introduced
ENFORCE_NOT_NULL_STRATEGY_OPTIONconfiguration option with defaultASSERTION_OPERATOR - Refactored converters to use
applyNotNullEnforcementIfNeeded()instead of string concatenation
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/EnforceNotNullStrategy.kt |
Defines the new strategy enum with documentation |
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/KonvertOptions.kt |
Declares the configuration option |
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/extensions.kt |
Implements configuration accessor with parsing logic |
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/AbstractTypeConverter.kt |
Implements strategy logic in applyNotNullEnforcementIfNeeded() method |
converter/src/main/kotlin/io/mcarle/konvert/converter/SameTypeConverter.kt |
Applies new enforcement method |
| Multiple converter files | Refactor to use new enforcement approach |
processor/src/main/kotlin/io/mcarle/konvert/processor/codegen/MappingCodeGenerator.kt |
Updates top-level mapping generation to support new strategy |
processor/src/test/kotlin/io/mcarle/konvert/processor/codegen/MappingCodeGeneratorITest.kt |
Adds integration tests for both strategies |
converter/src/test/kotlin/io/mcarle/konvert/converter/SameTypeConverterITest.kt |
Adds parameterized tests for REQUIRE_NOT_NULL strategy |
converter/src/test/kotlin/io/mcarle/konvert/converter/utils/ConverterITest.kt |
Extends test infrastructure to support options parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
converter-api/src/main/kotlin/io/mcarle/konvert/converter/api/config/EnforceNotNullStrategy.kt
Show resolved
Hide resolved
| ?.let { "Value for '$it' must not be null" } | ||
| ?: "Value must not be null" |
Copilot
AI
Dec 4, 2025
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.
The error message construction doesn't match the test expectation. Test line 84 expects the format 'it.test0' must not be null but this produces Value for 'it.test0' must not be null. Consider using just \"$it must not be null\" to match test expectations and provide more concise error messages.
| ?.let { "Value for '$it' must not be null" } | |
| ?: "Value must not be null" | |
| ?.let { "'$it' must not be null" } | |
| ?: "must not be null" |
|
|
||
| EnforceNotNullStrategy.REQUIRE_NOT_NULL -> { | ||
| val requireCode = | ||
| "return·requireNotNull(${context.paramName})·{·\"${context.paramName}·must·not·be·null\"·}·.let·{\n⇥%L%L⇤\n}" |
Copilot
AI
Dec 4, 2025
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.
The error message format here (\"${context.paramName}·must·not·be·null\") is inconsistent with the format in AbstractTypeConverter.applyNotNullEnforcementIfNeeded (\"Value for '$it' must not be null\"). These should use the same message format for consistency across the codebase.
| "return·requireNotNull(${context.paramName})·{·\"${context.paramName}·must·not·be·null\"·}·.let·{\n⇥%L%L⇤\n}" | |
| "return·requireNotNull(${context.paramName})·{·\"Value·for·'${context.paramName}'·must·not·be·null\"·}·.let·{\n⇥%L%L⇤\n}" |
|
@mcarleio do you want us to answer to AI comments? |
|
No, sorry, I am quite busy at the moment and do not find enough time for Konvert. Pre holiday stress... I just wanted to try out what an AI identifies, so that I get a better feeling. Of course you can have a look and think about what the AI identified, but my plan was, that I would validate and if I think it is important, then tag the creator of the PR on that (probably with some comment from myself). I really like it that you want to contribute! I am really sorry for the poor experience so far, but currently I have (too) little time for Konvert. |
|
Hello @mcarleio |
|
I hope to find time by end of the week. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
- Coverage 89.97% 89.92% -0.05%
==========================================
Files 81 82 +1
Lines 2754 2850 +96
Branches 389 397 +8
==========================================
+ Hits 2478 2563 +85
- Misses 112 118 +6
- Partials 164 169 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Sorry to let you wait that long. Nevertheless, today I finally had a look on the code, and it looks good 🥳 Thank you for the effort and the contribution! I decided to directly merge it, so that we can proceed here in the upcoming days. I have some local changes (started over a month ago...), which I try to finalize before the next release, but not sure how much time I will find. |
Konvert currently enforces non-nullability using
!!, which results in raw NPEs without context.Introducing a strategy-based approach allows producing clearer, debuggable errors via
requireNotNull(...).New configuration:
Strategies:
ASSERTION_OPERATOR (current and default strategy)
REQUIRE_NOT_NULL
The default remains ASSERTION_OPERATOR, so this is fully backward-compatible and introduces no breaking changes for existing users.
This PR implements the strategy only in SameTypeConverter as a PoC.
If the idea is accepted, I can extend it across all converters and generation paths.
Let me know what you think!