Skip to content

fix: use snakeyaml-engine-kmp for validation #281

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

Merged
merged 2 commits into from
Mar 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ kotlin {
jvmMain {
dependencies {
implementation("com.charleskorn.kaml:kaml:0.73.0")
implementation("it.krzeminski:snakeyaml-engine-kmp:3.1.1")
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package it.krzeminski.githubactionstyping

import it.krzeminski.githubactionstyping.parsing.TypesManifest
import it.krzeminski.githubactionstyping.parsing.ValidationException
import it.krzeminski.githubactionstyping.parsing.parseManifest
import it.krzeminski.githubactionstyping.parsing.parseTypesManifest
import it.krzeminski.githubactionstyping.reporting.appendStatus
Expand All @@ -25,11 +25,14 @@ fun manifestsToReport(manifestAndPath: Pair<String, Path>?, typesManifest: Strin
})
}

val parsedTypesManifest = if (typesManifest.isNotBlank()) {
val parsedTypesManifest =
parseTypesManifest(typesManifest)
} else {
TypesManifest()
}
.onFailure {
if (it is ValidationException) {
return false to it.message
}
throw it
}.getOrThrow()
val parsedManifest = parseManifest(manifestAndPath.first)

val inputsInTypesManifest = parsedTypesManifest.inputs?.keys ?: emptySet()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,44 +1,99 @@
package it.krzeminski.githubactionstyping.parsing

import com.charleskorn.kaml.AnchorsAndAliases
import com.charleskorn.kaml.EmptyYamlDocumentException
import com.charleskorn.kaml.Yaml
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import kotlinx.serialization.decodeFromString

@Serializable
import it.krzeminski.snakeyaml.engine.kmp.api.Load
import it.krzeminski.snakeyaml.engine.kmp.api.LoadSettings
import it.krzeminski.snakeyaml.engine.kmp.schema.CoreSchema

data class TypesManifest(
val inputs: Map<String, ApiItem>? = null,
val outputs: Map<String, ApiItem>? = null,
)

@Serializable
data class ApiItem(
val type: String? = null,
val name: String? = null,
@SerialName("allowed-values")
val allowedValues: List<String>? = null,
val separator: String? = null,
@SerialName("named-values")
val namedValues: Map<String, String>? = null,
@SerialName("list-item")
val namedValues: Map<String, Int>? = null,
val listItem: ApiItem? = null,
)

private val myYaml = Yaml(
configuration = Yaml.default.configuration.copy(
strictMode = false,
anchorsAndAliases = AnchorsAndAliases.Permitted(),
)
)

fun parseTypesManifest(manifestString: String): TypesManifest =
fun parseTypesManifest(manifestString: String): Result<TypesManifest> =
runCatching {
myYaml.decodeFromString<TypesManifest>(manifestString)
}.getOrElse {
if (it is EmptyYamlDocumentException) {
return TypesManifest()
val loadedTypesManifest = Load(
// work-around for https://github.com/krzema12/snakeyaml-engine-kmp/pull/390
LoadSettings.builder().setSchema(CoreSchema()).build()
).loadOne(manifestString)

when (loadedTypesManifest) {
null -> TypesManifest()

is Map<*, *> -> {
TypesManifest(
inputs = loadedTypesManifest.toInputsOrOutputs("inputs"),
outputs = loadedTypesManifest.toInputsOrOutputs("outputs"),
)
}

else -> throw ValidationException("Types file must be a mapping.")
}
}

private fun Map<*, *>.toInputsOrOutputs(key: String): Map<String, ApiItem>? =
when (val inputsOrOutputs = this[key]) {
null -> null

is Map<*, *> -> {
inputsOrOutputs.entries.associate { (key, value) -> key as String to value.toApiItem(key) }
}
throw it

else -> throw ValidationException("$key must be a mapping.")
}

private fun Any?.toApiItem(key: String): ApiItem =
when (this) {
null -> ApiItem()

is Map<*, *> -> {
ApiItem(
type = get("type")?.let {
"$it"
},
name = get("name")?.let {
"$it"
},
allowedValues = get("allowed-values")?.let {
if (it !is List<*>) {
throw ValidationException("allowed-values must be a sequence.")
}
it.map {
"$it"
}
},
separator = get("separator")?.let {
if (it !is String) {
throw ValidationException("separator must be string.")
}
it
},
namedValues = get("named-values")?.let {
if (it !is Map<*, *>) {
throw ValidationException("named-values must be a mapping.")
}
it.mapKeys { (key, _) -> key as String }.mapValues { (_, value) ->
if (value !is Int) {
throw ValidationException("Named values must be integer.")
}
value
}
},
listItem = toApiItem("list-item"),
)
}

else -> throw ValidationException("$key must be a mapping.")
}

private fun Map<*, *>.toApiItem(key: String): ApiItem? = get(key)?.toApiItem(key)

internal class ValidationException(override val message: String) : Exception(message)
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ fun ApiItem.validateInteger(): ItemValidationResult {
if (this.namedValues?.keys?.any { it.isBlank() } == true) {
return ItemValidationResult.Invalid("Named value names must not be empty.")
}
if (this.namedValues?.values?.any { it.toIntOrNull() == null } == true) {
return ItemValidationResult.Invalid("Named values must be integer.")
}
if (this.name != null && this.name.isBlank()) {
return ItemValidationResult.Invalid("Name must not be empty.")
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package it.krzeminski.githubactionstyping

import com.charleskorn.kaml.IncorrectTypeException
import com.charleskorn.kaml.InvalidPropertyValueException
import io.kotest.assertions.assertSoftly
import io.kotest.assertions.withClue
import io.kotest.matchers.should
import io.kotest.matchers.shouldBe
import io.kotest.matchers.types.beOfType
import it.krzeminski.githubactionstyping.parsing.ValidationException
import it.krzeminski.githubactionstyping.parsing.parseTypesManifest
import it.krzeminski.githubactionstyping.reporting.toPlaintextReport
import it.krzeminski.githubactionstyping.validation.ItemValidationResult
Expand All @@ -18,7 +17,7 @@ import java.io.File
*/
class LogicConsistencyTest : UseCaseTest() {
override suspend fun testValid(typing: File) {
val validationResult = parseTypesManifest(typing.readText()).validate(typing.toPath().fileName)
val validationResult = parseTypesManifest(typing.readText()).getOrThrow().validate(typing.toPath().fileName)
withClue(validationResult.toPlaintextReport()) {
validationResult.overallResult shouldBe ItemValidationResult.Valid
}
Expand All @@ -31,15 +30,11 @@ class LogicConsistencyTest : UseCaseTest() {
.trimMargin("#")
.trim()

val parsedTypesManifest = runCatching {
parseTypesManifest(typesManifest).validate(typing.toPath().fileName)
}
if (parsedTypesManifest.isFailure &&
((parsedTypesManifest.exceptionOrNull() is InvalidPropertyValueException) || (parsedTypesManifest.exceptionOrNull() is IncorrectTypeException))
) {
val parsedTypesManifest = parseTypesManifest(typesManifest)
if (parsedTypesManifest.isFailure && parsedTypesManifest.exceptionOrNull() is ValidationException) {
parsedTypesManifest.exceptionOrNull()!!.message shouldBe expectedValidationError
} else {
val validationResult = parsedTypesManifest.getOrThrow()
val validationResult = parsedTypesManifest.getOrThrow().validate(typing.toPath().fileName)
assertSoftly {
validationResult.overallResult should beOfType<ItemValidationResult.Invalid>()
validationResult
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ class ManifestValidationTest : FunSpec({
"integer-input" to ApiItem(type = "integer"),
"integer-with-named-values-input" to ApiItem(
type = "integer",
namedValues = mapOf("foo" to "1", "bar" to "2")
namedValues = mapOf("foo" to 1, "bar" to 2)
),
"integer-with-named-values-and-custom-item-name-input" to ApiItem(
type = "integer",
name = "SomeItemName",
namedValues = mapOf("foo" to "1", "bar" to "2")
namedValues = mapOf("foo" to 1, "bar" to 2)
),
"float-input" to ApiItem(type = "float"),
),
Expand Down Expand Up @@ -481,11 +481,11 @@ class ManifestValidationTest : FunSpec({
// given
val manifest = TypesManifest(
inputs = mapOf(
"string-input" to ApiItem(type = "string", namedValues = mapOf("foo" to "1")),
"boolean-input" to ApiItem(type = "boolean", namedValues = mapOf("foo" to "1")),
"float-input" to ApiItem(type = "float", namedValues = mapOf("foo" to "1")),
"list-input" to ApiItem(type = "list", separator = ",", listItem = ApiItem(type = "string"), namedValues = mapOf("foo" to "1")),
"enum-input" to ApiItem(type = "enum", allowedValues = listOf("foo", "bar"), namedValues = mapOf("foo" to "1")),
"string-input" to ApiItem(type = "string", namedValues = mapOf("foo" to 1)),
"boolean-input" to ApiItem(type = "boolean", namedValues = mapOf("foo" to 1)),
"float-input" to ApiItem(type = "float", namedValues = mapOf("foo" to 1)),
"list-input" to ApiItem(type = "list", separator = ",", listItem = ApiItem(type = "string"), namedValues = mapOf("foo" to 1)),
"enum-input" to ApiItem(type = "enum", allowedValues = listOf("foo", "bar"), namedValues = mapOf("foo" to 1)),
),
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ inputs:

# Expected validation error
#
#Value for 'inputs' is invalid: Value for 'granted-scopes' is invalid: Value for 'list-item' is invalid: Value for 'allowed-values' is invalid: Expected a list, but got a scalar value
#allowed-values must be a sequence.
#
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ inputs:

# Expected validation error
#
#Value for 'inputs' is invalid: Value for 'permissions' is invalid: Value for 'allowed-values' is invalid: Expected a list, but got a scalar value
#allowed-values must be a sequence.
#
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,9 @@ inputs:
type: integer
name: AllowedValues
named-values:
foo: bar
foo: '0'

# Expected validation error
#
#For action with manifest at 'inputs_integer_list_item_with_non_integer_named_value.yml':
#Result:
#\x1B[31m❌ INVALID: Some typing is invalid.\x1B[0m
#
#Inputs:
#• list-of-integer:
# \x1B[31m❌ INVALID: List item type: Named values must be integer.\x1B[0m
#
#Outputs:
#None.
Comment on lines -15 to -24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only remaining remark I have for this change is a regression in the level of details for this single case ("named values must be integer"). With the proposed change, the user doesn't know which YAML and which input/output is affected.

However in practice, the lack of details is present in many other cases. I appreciate the fact that checking this type is done together with checking other types, so this PR brings some kind of consistency to the behavior and code structure, even if it introduces a regression in one.

I'm fine with merging it as is, but ideally we should include details about the affected YAML file and input/output in all cases (just articulating it, not saying you should do it). We should rethink if parsing and validation should really be done separately - maybe it's an artificial distinction here, and it's easier to make it in one go?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe it can be combined.
I basically kept the changes minimal by replacing the parsing part, only doing some slight improvements on the way and loosing that one type of clarity.
But it can probably be rewritten to provide better errors for all cases, yeah.

#Named values must be integer.
#
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ inputs:

# Expected validation error
#
#Value for 'inputs' is invalid: Value for 'list-of-integer' is invalid: Value for 'list-item' is invalid: Value for 'named-values' is invalid: Expected a map, but got a list
#named-values must be a mapping.
#
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,9 @@ inputs:
type: integer
name: AllowedValues
named-values:
foo: bar
foo: '0'

# Expected validation error
#
#For action with manifest at 'inputs_integer_with_non_integer_named_value.yml':
#Result:
#\x1B[31m❌ INVALID: Some typing is invalid.\x1B[0m
#
#Inputs:
#• retries:
# \x1B[31m❌ INVALID: Named values must be integer.\x1B[0m
#
#Outputs:
#None.
#Named values must be integer.
#
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@ inputs:

# Expected validation error
#
#Value for 'inputs' is invalid: Value for 'retries' is invalid: Value for 'named-values' is invalid: Expected a map, but got a list
#named-values must be a mapping.
#
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ inputs:
#
#Inputs:
#• permissions:
# \x1B[31m❌ INVALID: Unknown type: '0x0'.\x1B[0m
# \x1B[31m❌ INVALID: Unknown type: '0'.\x1B[0m
#
#Outputs:
#None.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ outputs:

# Expected validation error
#
#Value for 'outputs' is invalid: Value for 'granted-scopes' is invalid: Value for 'list-item' is invalid: Value for 'allowed-values' is invalid: Expected a list, but got a scalar value
#allowed-values must be a sequence.
#
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ outputs:

# Expected validation error
#
#Value for 'outputs' is invalid: Value for 'permissions' is invalid: Value for 'allowed-values' is invalid: Expected a list, but got a scalar value
#allowed-values must be a sequence.
#
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,9 @@ outputs:
type: integer
name: AllowedValues
named-values:
foo: bar
foo: '0'

# Expected validation error
#
#For action with manifest at 'outputs_integer_list_item_with_non_integer_named_value.yml':
#Result:
#\x1B[31m❌ INVALID: Some typing is invalid.\x1B[0m
#
#Inputs:
#None.
#
#Outputs:
#• list-of-integer:
# \x1B[31m❌ INVALID: List item type: Named values must be integer.\x1B[0m
#Named values must be integer.
#
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ outputs:

# Expected validation error
#
#Value for 'outputs' is invalid: Value for 'list-of-integer' is invalid: Value for 'list-item' is invalid: Value for 'named-values' is invalid: Expected a map, but got a list
#named-values must be a mapping.
#
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,9 @@ outputs:
type: integer
name: AllowedValues
named-values:
foo: bar
foo: '0'

# Expected validation error
#
#For action with manifest at 'outputs_integer_with_non_integer_named_value.yml':
#Result:
#\x1B[31m❌ INVALID: Some typing is invalid.\x1B[0m
#
#Inputs:
#None.
#
#Outputs:
#• retries:
# \x1B[31m❌ INVALID: Named values must be integer.\x1B[0m
#Named values must be integer.
#
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ outputs:

# Expected validation error
#
#Value for 'outputs' is invalid: Value for 'retries' is invalid: Value for 'named-values' is invalid: Expected a map, but got a list
#named-values must be a mapping.
#
Loading
Loading