fix: fail fast when mandatory AsyncApi fields are missing#232
fix: fail fast when mandatory AsyncApi fields are missing#232Varadraj75 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Factory as AsyncApi.asyncApi()
participant AsyncApiObj as AsyncApi instance
participant Checker as checkInitialized
User->>Factory: call asyncApi { ...build... }
Factory->>AsyncApiObj: instantiate AsyncApi and apply build
Factory->>Checker: call validateForSerialization() -> checkInitialized("info","channels")
alt required fields missing
Checker-->>Factory: throw IllegalStateException("Missing required properties: ...")
Factory-->>User: propagate exception
else all required fields present
Checker-->>Factory: return AsyncApi instance
Factory-->>User: return AsyncApi object
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Adds fail-fast validation for mandatory AsyncApi fields (info, channels) to prevent late lateinit crashes and updates tests to assert the new behavior.
Changes:
- Add runtime validation after building
AsyncApivia the DSL factory (AsyncApi.asyncApi { ... }) - Introduce a generic
checkInitializedhelper to detect uninitializedlateinitproperties - Add tests for failure when required fields are missing and success when they are set
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt | Adds post-build validation for required fields and introduces checkInitialized helper; also removes inline from DSL builder methods. |
| kotlin-asyncapi-core/src/test/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApiTest.kt | Adds tests covering fail-fast behavior and successful construction when required fields are set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Show resolved
Hide resolved
kotlin-asyncapi-core/src/test/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApiTest.kt
Show resolved
Hide resolved
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Outdated
Show resolved
Hide resolved
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Outdated
Show resolved
Hide resolved
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Outdated
Show resolved
Hide resolved
Varadraj75
force-pushed
the
fix-mandatory-fields-validation
branch
from
23faf01 to
5031049
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt`:
- Around line 61-76: The top-level extension function checkInitialized should be
made internal to hide it from the public API; update the function declaration
for fun <T> T.checkInitialized(...) to use internal visibility (i.e., internal
fun <T> T.checkInitialized(...)) so it remains usable within the file/module but
is not exported publicly—apply this change to the checkInitialized declaration
in AsyncApi.kt.
- Around line 27-46: The member builder methods info, servers, channels,
components, tags, and externalDocs should be declared inline to match the DSL
pattern and avoid unnecessary lambda allocations; update their signatures (e.g.,
fun info(build: Info.() -> Unit): Info) to be inline fun info(build: Info.() ->
Unit): Info and do the same for servers(ReferencableServersMap.() -> Unit),
channels(ReferencableChannelsMap.() -> Unit), components(Components.() -> Unit),
tags(TagsList.() -> Unit), and externalDocs(ExternalDocumentation.() -> Unit)
while leaving the asyncApi companion function behavior unchanged.
🧹 Nitpick comments (1)
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt (1)
51-58: Validation only runs through the companionasyncApi()entry point.Direct construction via
AsyncApi().apply { … }(as in the existing test on Line 34 of the test file) bypassescheckInitializedentirely. This is a reasonable design choice for a DSL entry-point guard, but worth documenting — users constructingAsyncApi()directly will not get fail-fast validation.Consider adding a KDoc comment on the companion
asyncApifunction noting it is the recommended entry point because it validates mandatory fields.
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Show resolved
Hide resolved
kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
Outdated
Show resolved
Hide resolved
Varadraj75
changed the title
|
Hi @lorenzsimon ,
Would really appreciate a quick review when you have time. Thanks! |
| info { | ||
| title("titleValue") | ||
| version("versionValue") | ||
| } |
There was a problem hiding this comment.
It is actually expected that we build a partial asyncapi spec here. the context module is a core feature of the library which lets users build asnycapi documentation from multiple sources. for example you can define the info in code and extend it with the channels from annotation processors.
My initial idea does not seem to work since we need the ability to build partial asyncapi in order to extend it with context providers.
So our only option is to introduce a separate validation step before we serialise it.
| fun id(value: String): String = | ||
| value.also { id = it } | ||
|
|
||
| inline fun info(build: Info.() -> Unit): Info = |
There was a problem hiding this comment.
why do you remove the inline modifier?
| } | ||
| } | ||
|
|
||
| fun <T> T.checkInitialized( |
There was a problem hiding this comment.
this should be moved to a separate utility file. its not part of the asyncapi dsl model
| TestUtils.assertJsonEquals(expected, actual) | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
After moving the T.checkInitialized to a separate file. You could write a unit test for it, not in the model test.
| } | ||
| } | ||
|
|
||
| fun <T> T.checkInitialized( |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt`:
- Around line 59-60: The DSL entry function asyncApi(build: AsyncApi.() ->
Unit): AsyncApi should enforce validation before returning; after constructing
and applying the build to AsyncApi() (the AsyncApi() instance returned by
AsyncApi().apply(build)), call the instance's validateForSerialization() (or
equivalent validation method on AsyncApi) and let any validation exceptions
propagate (or throw a descriptive exception) so invalid documents fail fast;
update the asyncApi function to perform this validation step on the built
AsyncApi object prior to returning it.
- Around line 49-54: validateForSerialization() is never called, so invalid
AsyncApi instances can be serialized; call validateForSerialization() before
actual serialization to enforce required fields. Update
DefaultAsyncApiSerializer.serialize() to invoke
AsyncApi.validateForSerialization() (or a public wrapper) immediately before
objectMapper.writeValueAsString(this), and/or ensure the asyncApi() DSL
entrypoint triggers the same validation at build/return time so any missing
"info" or "channels" causes an error rather than producing invalid JSON. Ensure
you reference the existing validateForSerialization() method on AsyncApi when
adding the call.
In
`@kotlin-asyncapi-core/src/test/kotlin/com/asyncapi/kotlinasyncapi/util/ValidationUtilsTest.kt`:
- Line 38: Replace the JVM-dependent Kotlin assert() calls in
ValidationUtilsTest (specifically the assertions at the locations checking
exception.message!!.contains(...), currently at lines referenced around the
checks) with JUnit assertions so the tests run reliably; change each assert(...)
to Assertions.assertTrue(...) (or import assertTrue from
org.junit.jupiter.api.Assertions and call assertTrue(...)) for the checks in the
test methods in ValidationUtilsTest.kt (the exception message contains
assertions around requiredProperty) so they execute regardless of JVM assertion
flags.
| internal fun validateForSerialization() { | ||
| checkInitialized( | ||
| "info" to { info }, | ||
| "channels" to { channels } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'validateForSerialization' --type=kotlin -C3Repository: asyncapi/kotlin-asyncapi
Length of output: 1824
🏁 Script executed:
# Look at the full test context
sed -n '45,70p' kotlin-asyncapi-core/src/test/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApiTest.kt
# Search for any serialization implementations or hooks
rg -n 'KSerializer|@Serializable|toJson|serialize' --type=kotlin kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
# Check if validateForSerialization is called elsewhere outside tests
rg -n 'validateForSerialization' --type=kotlin -C2 | grep -v "Test.kt"Repository: asyncapi/kotlin-asyncapi
Length of output: 1458
🏁 Script executed:
# Look for TestUtils.json implementation to see if it calls validateForSerialization
fd -t f -name 'TestUtils.kt' kotlin-asyncapi-core/src/test
# Search for any toJson, json, or serialization methods in AsyncApi
rg -n 'toJson|json|serialize' --type=kotlin kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt
# Check if there's a custom serializer defined
rg -n 'KSerializer|@Serializable' --type=kotlin kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/Repository: asyncapi/kotlin-asyncapi
Length of output: 298
🏁 Script executed:
# Find TestUtils.kt file
fd -t f -name 'TestUtils.kt' kotlin-asyncapi-core/src/test
# Search for json method definition and validateForSerialization calls
rg 'fun json|TestUtils.json' --type=kotlin -A5 -B2Repository: asyncapi/kotlin-asyncapi
Length of output: 48147
🏁 Script executed:
# Look for any custom Jackson serializer for AsyncApi
rg 'AsyncApi' --type=kotlin | grep -i 'serializer\|json\|deserializer\|custom' | head -20
# Check if there's a `@JsonSerialize` annotation on AsyncApi class
sed -n '1,70p' kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.ktRepository: asyncapi/kotlin-asyncapi
Length of output: 5463
🏁 Script executed:
# Find and examine the DefaultAsyncApiSerializer implementation
fd -type f -name 'DefaultAsyncApiSerializer.kt'
# Also check the AsyncApiSerializer interface
rg 'interface AsyncApiSerializer' --type=kotlin -A10Repository: asyncapi/kotlin-asyncapi
Length of output: 1640
🏁 Script executed:
# Read the full DefaultAsyncApiSerializer implementation
cat kotlin-asyncapi-context/src/main/kotlin/com/asyncapi/kotlinasyncapi/context/service/AsyncApiSerializer.ktRepository: asyncapi/kotlin-asyncapi
Length of output: 748
validateForSerialization() is never invoked during actual serialization.
The method is defined but DefaultAsyncApiSerializer.serialize() only calls objectMapper.writeValueAsString(this) without invoking validateForSerialization(). The DSL entry point asyncApi() also does not call it. As a result, users can create and serialize invalid AsyncApi instances (missing required info or channels fields) without any error, making the validation dead code.
🤖 Prompt for AI Agents
In
`@kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt`
around lines 49 - 54, validateForSerialization() is never called, so invalid
AsyncApi instances can be serialized; call validateForSerialization() before
actual serialization to enforce required fields. Update
DefaultAsyncApiSerializer.serialize() to invoke
AsyncApi.validateForSerialization() (or a public wrapper) immediately before
objectMapper.writeValueAsString(this), and/or ensure the asyncApi() DSL
entrypoint triggers the same validation at build/return time so any missing
"info" or "channels" causes an error rather than producing invalid JSON. Ensure
you reference the existing validateForSerialization() method on AsyncApi when
adding the call.
| fun asyncApi(build: AsyncApi.() -> Unit): AsyncApi = | ||
| AsyncApi().apply(build) |
There was a problem hiding this comment.
DSL entry point does not enforce validation.
The asyncApi() function builds and returns an AsyncApi without calling validateForSerialization(). Per the PR objectives, the intent is to "fail fast" so invalid documents can't be created. Consider calling the validation here:
Proposed fix
fun asyncApi(build: AsyncApi.() -> Unit): AsyncApi =
- AsyncApi().apply(build)
+ AsyncApi().apply(build).also { it.validateForSerialization() }🤖 Prompt for AI Agents
In
`@kotlin-asyncapi-core/src/main/kotlin/com/asyncapi/kotlinasyncapi/model/AsyncApi.kt`
around lines 59 - 60, The DSL entry function asyncApi(build: AsyncApi.() ->
Unit): AsyncApi should enforce validation before returning; after constructing
and applying the build to AsyncApi() (the AsyncApi() instance returned by
AsyncApi().apply(build)), call the instance's validateForSerialization() (or
equivalent validation method on AsyncApi) and let any validation exceptions
propagate (or throw a descriptive exception) so invalid documents fail fast;
update the asyncApi function to perform this validation step on the built
AsyncApi object prior to returning it.
| ) | ||
| } | ||
|
|
||
| assert(exception.message!!.contains("Missing required properties: requiredProperty")) |
There was a problem hiding this comment.
assert() may silently pass when JVM assertions are disabled.
Kotlin's assert() is a no-op unless the JVM is started with -ea. If the test runner doesn't enable assertions, lines 38, 52, and 53 will never actually check anything. Use JUnit's assertTrue instead for reliable test behavior.
Proposed fix
- assert(exception.message!!.contains("Missing required properties: requiredProperty"))
+ assertTrue(exception.message!!.contains("Missing required properties: requiredProperty"))And similarly for lines 52–53:
- assert(exception.message!!.contains("requiredProperty"))
- assert(exception.message!!.contains("anotherMissing"))
+ assertTrue(exception.message!!.contains("requiredProperty"))
+ assertTrue(exception.message!!.contains("anotherMissing"))Add the import at the top:
import org.junit.jupiter.api.Assertions.assertDoesNotThrow
import org.junit.jupiter.api.Assertions.assertThrows
+import org.junit.jupiter.api.Assertions.assertTrue📝 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.
| assert(exception.message!!.contains("Missing required properties: requiredProperty")) | |
| assertTrue(exception.message!!.contains("Missing required properties: requiredProperty")) |
🤖 Prompt for AI Agents
In
`@kotlin-asyncapi-core/src/test/kotlin/com/asyncapi/kotlinasyncapi/util/ValidationUtilsTest.kt`
at line 38, Replace the JVM-dependent Kotlin assert() calls in
ValidationUtilsTest (specifically the assertions at the locations checking
exception.message!!.contains(...), currently at lines referenced around the
checks) with JUnit assertions so the tests run reliably; change each assert(...)
to Assertions.assertTrue(...) (or import assertTrue from
org.junit.jupiter.api.Assertions and call assertTrue(...)) for the checks in the
test methods in ValidationUtilsTest.kt (the exception message contains
assertions around requiredProperty) so they execute regardless of JVM assertion
flags.
397446c to
ab65729
Compare
- Remove validation from DSL entry point - Move checkInitialized to internal utility file - Add dedicated utility tests - Introduce validateForSerialization() in AsyncApi
Varadraj75
force-pushed
the
fix-mandatory-fields-validation
branch
from
ab65729 to
9ca7fc0
Compare
|
@lorenzsimon I’ve updated the PR to:
Validation is no longer triggered during DSL construction and is now aligned with the intended serialization lifecycle. I’ve rebased onto the latest Please let me know if this aligns with the expected design. |
2 participants
What
Adds runtime validation to ensure mandatory `AsyncApi` fields (`info` and `channels`) are initialized before an instance can be used.Why
Previously, it was possible to create an invalid
AsyncApiinstance without mandatory fields, which caused a runtime crash later during JSON serialization due to uninitializedlateinitproperties. This change fails fast with a clear and descriptive error instead of allowing an invalid object to propagate.How
IllegalStateExceptionFixes #228
Summary by CodeRabbit