-
Notifications
You must be signed in to change notification settings - Fork 7
fix: fail fast when mandatory AsyncApi fields are missing #232
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import com.asyncapi.kotlinasyncapi.model.channel.ReferencableChannelsMap | |
| import com.asyncapi.kotlinasyncapi.model.component.Components | ||
| import com.asyncapi.kotlinasyncapi.model.info.Info | ||
| import com.asyncapi.kotlinasyncapi.model.server.ReferencableServersMap | ||
| import com.asyncapi.kotlinasyncapi.util.checkInitialized | ||
|
|
||
| @Target(AnnotationTarget.CLASS, AnnotationTarget.TYPEALIAS) | ||
| @DslMarker | ||
|
|
@@ -24,31 +25,38 @@ class AsyncApi { | |
| fun id(value: String): String = | ||
| value.also { id = it } | ||
|
|
||
| inline fun info(build: Info.() -> Unit): Info = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you remove the inline modifier? |
||
| fun info(build: Info.() -> Unit): Info = | ||
| Info().apply(build).also { info = it } | ||
|
|
||
| inline fun servers(build: ReferencableServersMap.() -> Unit): ReferencableServersMap = | ||
| fun servers(build: ReferencableServersMap.() -> Unit): ReferencableServersMap = | ||
| ReferencableServersMap().apply(build).also { servers = it } | ||
|
|
||
| inline fun channels(build: ReferencableChannelsMap.() -> Unit): ReferencableChannelsMap = | ||
| fun channels(build: ReferencableChannelsMap.() -> Unit): ReferencableChannelsMap = | ||
| ReferencableChannelsMap().apply(build).also { channels = it } | ||
Varadraj75 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| fun defaultContentType(value: String): String = | ||
| value.also { defaultContentType = it } | ||
|
|
||
| inline fun components(build: Components.() -> Unit): Components = | ||
| fun components(build: Components.() -> Unit): Components = | ||
| Components().apply(build).also { components = it } | ||
|
|
||
| inline fun tags(build: TagsList.() -> Unit): TagsList = | ||
| fun tags(build: TagsList.() -> Unit): TagsList = | ||
| TagsList().apply(build).also { tags = it } | ||
|
|
||
| inline fun externalDocs(build: ExternalDocumentation.() -> Unit): ExternalDocumentation = | ||
| fun externalDocs(build: ExternalDocumentation.() -> Unit): ExternalDocumentation = | ||
| ExternalDocumentation().apply(build).also { externalDocs = it } | ||
Varadraj75 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| internal fun validateForSerialization() { | ||
| checkInitialized( | ||
| "info" to { info }, | ||
| "channels" to { channels } | ||
| ) | ||
| } | ||
|
Comment on lines
+49
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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
The method is defined but 🤖 Prompt for AI Agents |
||
|
|
||
| companion object { | ||
| const val VERSION = "2.4.0" | ||
|
|
||
| inline fun asyncApi(build: AsyncApi.() -> Unit): AsyncApi = | ||
| fun asyncApi(build: AsyncApi.() -> Unit): AsyncApi = | ||
| AsyncApi().apply(build) | ||
|
Comment on lines
+59
to
60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DSL entry point does not enforce validation. The Proposed fix fun asyncApi(build: AsyncApi.() -> Unit): AsyncApi =
- AsyncApi().apply(build)
+ AsyncApi().apply(build).also { it.validateForSerialization() }🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package com.asyncapi.kotlinasyncapi.util | ||
|
|
||
| internal fun <T> T.checkInitialized( | ||
| vararg checks: Pair<String, T.() -> Unit> | ||
| ): T = also { | ||
| val missing = checks.mapNotNull { (name, check) -> | ||
| try { | ||
| it.check() | ||
| null | ||
| } catch (_: UninitializedPropertyAccessException) { | ||
| name | ||
| } | ||
| } | ||
|
|
||
| check(missing.isEmpty()) { | ||
| "Missing required properties: ${missing.joinToString()}" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| package com.asyncapi.kotlinasyncapi.model | ||
|
|
||
| import com.asyncapi.kotlinasyncapi.model.info.Info | ||
| import io.mockk.clearConstructorMockk | ||
| import io.mockk.every | ||
| import io.mockk.mockkConstructor | ||
| import org.junit.jupiter.api.AfterEach | ||
| import org.junit.jupiter.api.Test | ||
| import com.asyncapi.kotlinasyncapi.model.info.Info | ||
| import org.junit.jupiter.api.Assertions.assertTrue | ||
| import org.junit.jupiter.api.Assertions.assertNotNull | ||
| import org.junit.jupiter.api.Assertions.assertThrows | ||
|
|
||
| internal class AsyncApiTest { | ||
|
|
||
|
|
@@ -43,4 +46,31 @@ internal class AsyncApiTest { | |
|
|
||
| TestUtils.assertJsonEquals(expected, actual) | ||
| } | ||
|
|
||
| @Test | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After moving the T.checkInitialized to a separate file. You could write a unit test for it, not in the model test. |
||
| fun `should fail fast when mandatory fields are missing`() { | ||
| val api = AsyncApi.asyncApi { | ||
| } | ||
|
|
||
| val exception = assertThrows(IllegalStateException::class.java) { | ||
| api.validateForSerialization() | ||
| } | ||
|
|
||
| assertTrue(exception.message!!.contains("Missing required properties")) | ||
| assertTrue(exception.message!!.contains("info")) | ||
| assertTrue(exception.message!!.contains("channels")) | ||
Varadraj75 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @Test | ||
| fun `should succeed when mandatory fields are initialized`() { | ||
| val api = AsyncApi.asyncApi { | ||
| info { | ||
| title = "Test API" | ||
| version = "1.0.0" | ||
| } | ||
| channels { } | ||
| } | ||
|
|
||
| assertNotNull(api) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||
| package com.asyncapi.kotlinasyncapi.util | ||||||
|
|
||||||
| import org.junit.jupiter.api.Assertions.assertDoesNotThrow | ||||||
| import org.junit.jupiter.api.Assertions.assertThrows | ||||||
| import org.junit.jupiter.api.Test | ||||||
| import java.lang.IllegalStateException | ||||||
|
|
||||||
| internal class ValidationUtilsTest { | ||||||
|
|
||||||
| class TestClass { | ||||||
| lateinit var requiredProperty: String | ||||||
| var optionalProperty: String? = null | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `checkInitialized should pass when required property is initialized`() { | ||||||
| val instance = TestClass().apply { | ||||||
| requiredProperty = "value" | ||||||
| } | ||||||
|
|
||||||
| assertDoesNotThrow { | ||||||
| instance.checkInitialized( | ||||||
| "requiredProperty" to { requiredProperty } | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `checkInitialized should fail when required property is not initialized`() { | ||||||
| val instance = TestClass() | ||||||
|
|
||||||
| val exception = assertThrows(IllegalStateException::class.java) { | ||||||
| instance.checkInitialized( | ||||||
| "requiredProperty" to { requiredProperty } | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| assert(exception.message!!.contains("Missing required properties: requiredProperty")) | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Kotlin's 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
Suggested change
🤖 Prompt for AI Agents |
||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `checkInitialized should handle multiple checks`() { | ||||||
| val instance = TestClass() | ||||||
|
|
||||||
| val exception = assertThrows(IllegalStateException::class.java) { | ||||||
| instance.checkInitialized( | ||||||
| "requiredProperty" to { requiredProperty }, | ||||||
| "anotherMissing" to { throw UninitializedPropertyAccessException() } | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| assert(exception.message!!.contains("requiredProperty")) | ||||||
| assert(exception.message!!.contains("anotherMissing")) | ||||||
| } | ||||||
| } | ||||||
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.
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.