Skip to content

Commit

Permalink
Disallow @uniqueItems-constrained list shapes that reach a map shape (
Browse files Browse the repository at this point in the history
smithy-lang#2375)

* Disallow `@uniqueItems`-constrained list shapes that reach a map shape

The server SDK codegen generates Rust code that does not compile when a
`@uniqueItems`-constrained list shape reaches a map shape, essentially
because `std::collections::HashMap` does not implement
`std::hash::Hash`.

A ticket with the Smithy team was opened in smithy-lang/smithy#1567 to
disallow such models.

This commit makes it so that codegen aborts with an informative message
when such models are encountered, instead of going ahead and producing
code that does not compile.

This commit also slightly adjusts the error messages produced when
unsupported constraint traits are found.

* ./gradlew ktlintFormat
  • Loading branch information
david-perez committed Feb 17, 2023
1 parent b9f8090 commit 198c500
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 5 deletions.
Expand Up @@ -10,7 +10,9 @@ import software.amazon.smithy.model.shapes.BlobShape
import software.amazon.smithy.model.shapes.ByteShape
import software.amazon.smithy.model.shapes.EnumShape
import software.amazon.smithy.model.shapes.IntegerShape
import software.amazon.smithy.model.shapes.ListShape
import software.amazon.smithy.model.shapes.LongShape
import software.amazon.smithy.model.shapes.MapShape
import software.amazon.smithy.model.shapes.MemberShape
import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.shapes.ServiceShape
Expand All @@ -36,13 +38,17 @@ private sealed class UnsupportedConstraintMessageKind {
private val constraintTraitsUberIssue = "https://github.com/awslabs/smithy-rs/issues/1401"

fun intoLogMessage(ignoreUnsupportedConstraints: Boolean): LogMessage {
fun buildMessage(intro: String, willSupport: Boolean, trackingIssue: String, canBeIgnored: Boolean = true): String {
fun buildMessage(intro: String, willSupport: Boolean, trackingIssue: String? = null, canBeIgnored: Boolean = true): String {
var msg = """
$intro
$intro
This is not supported in the smithy-rs server SDK."""
if (willSupport) {
msg += """
It will be supported in the future. See the tracking issue ($trackingIssue)."""
It will be supported in the future."""
}
if (trackingIssue != null) {
msg += """
For more information, and to report if you're affected by this, please use the tracking issue: $trackingIssue."""
}
if (canBeIgnored) {
msg += """
Expand Down Expand Up @@ -106,6 +112,19 @@ private sealed class UnsupportedConstraintMessageKind {
level,
buildMessageShapeHasUnsupportedConstraintTrait(shape, uniqueItemsTrait, constraintTraitsUberIssue),
)

is UnsupportedMapShapeReachableFromUniqueItemsList -> LogMessage(
Level.SEVERE,
buildMessage(
"""
The map shape `${mapShape.id}` is reachable from the list shape `${listShape.id}`, which has the
`@uniqueItems` trait attached.
""".trimIndent().replace("\n", " "),
willSupport = false,
trackingIssue = "https://github.com/awslabs/smithy/issues/1567",
canBeIgnored = false,
),
)
}
}
}
Expand All @@ -129,6 +148,12 @@ private data class UnsupportedRangeTraitOnShape(val shape: Shape, val rangeTrait
private data class UnsupportedUniqueItemsTraitOnShape(val shape: Shape, val uniqueItemsTrait: UniqueItemsTrait) :
UnsupportedConstraintMessageKind()

private data class UnsupportedMapShapeReachableFromUniqueItemsList(
val listShape: ListShape,
val uniqueItemsTrait: UniqueItemsTrait,
val mapShape: MapShape,
) : UnsupportedConstraintMessageKind()

data class LogMessage(val level: Level, val message: String)
data class ValidationResult(val shouldAbort: Boolean, val messages: List<LogMessage>) :
Throwable(message = messages.joinToString("\n") { it.message })
Expand Down Expand Up @@ -246,11 +271,29 @@ fun validateUnsupportedConstraints(
.map { (shape, rangeTrait) -> UnsupportedRangeTraitOnShape(shape, rangeTrait as RangeTrait) }
.toSet()

// 5. `@uniqueItems` cannot reach a map shape.
// See https://github.com/awslabs/smithy/issues/1567.
val mapShapeReachableFromUniqueItemsListShapeSet = walker
.walkShapes(service)
.asSequence()
.filterMapShapesToTraits(setOf(UniqueItemsTrait::class.java))
.flatMap { (listShape, uniqueItemsTrait) ->
walker.walkShapes(listShape).filterIsInstance<MapShape>().map { mapShape ->
UnsupportedMapShapeReachableFromUniqueItemsList(
listShape as ListShape,
uniqueItemsTrait as UniqueItemsTrait,
mapShape,
)
}
}
.toSet()

val messages =
unsupportedConstraintOnMemberShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
unsupportedLengthTraitOnStreamingBlobShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
unsupportedConstraintShapeReachableViaAnEventStreamSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
unsupportedRangeTraitOnShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) }
unsupportedRangeTraitOnShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
mapShapeReachableFromUniqueItemsListShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) }

return ValidationResult(shouldAbort = messages.any { it.level == Level.SEVERE }, messages)
}
Expand Down
Expand Up @@ -27,7 +27,6 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest {
namespace test
service TestService {
version: "123",
operations: [TestOperation]
}
Expand Down Expand Up @@ -186,6 +185,47 @@ internal class ValidateUnsupportedConstraintsAreNotUsedTest {
}
}

private val mapShapeReachableFromUniqueItemsListShapeModel =
"""
$baseModel
structure TestInputOutput {
uniqueItemsList: UniqueItemsList
}
@uniqueItems
list UniqueItemsList {
member: Map
}
map Map {
key: String
value: String
}
""".asSmithyModel()

@Test
fun `it should detect when a map shape is reachable from a uniqueItems list shape`() {
val validationResult = validateModel(mapShapeReachableFromUniqueItemsListShapeModel)

validationResult.messages shouldHaveSize 1
validationResult.shouldAbort shouldBe true
validationResult.messages[0].message shouldContain """
The map shape `test#Map` is reachable from the list shape `test#UniqueItemsList`, which has the
`@uniqueItems` trait attached.
""".trimIndent().replace("\n", " ")
}

@Test
fun `it should abort when a map shape is reachable from a uniqueItems list shape, despite opting into ignoreUnsupportedConstraintTraits`() {
val validationResult = validateModel(
mapShapeReachableFromUniqueItemsListShapeModel,
ServerCodegenConfig().copy(ignoreUnsupportedConstraints = true),
)

validationResult.shouldAbort shouldBe true
}

@Test
fun `it should abort when constraint traits in event streams are used, despite opting into ignoreUnsupportedConstraintTraits`() {
val validationResult = validateModel(
Expand Down

0 comments on commit 198c500

Please sign in to comment.