Implement @pattern on string shapes (#1998)

* Refactor `ConstrainedStringGenerator` to extract length checking * Extract `TryFrom` rendering into its own function * Extract `ConstraintViolation` definition to separate function * Add pattern check mock for `@pattern` trait * Add `regex` to list of server dependencies * Use real regex check in `check_pattern` * Add `@pattern` to traits that make `string` shapes directly constrained * Remove unsupported validation for `@pattern` on strings * Fix test cases in `ConstraintsTest` * Remove test disallowing `@pattern` on strings * Add first test case for `@pattern` on strings * Fix codegen in `ConstraintViolation::as_validation_exception_field` * Use `OnceCell` to store `@pattern`'s regex * Tidy up `ConstrainedStringGenerator` to work with many constraints * Rename `handleTrait` -> `fromTrait` and make it a static method * Add docs for `ConstraintViolation` variants * Fix unescaped slashes in regexes * Quick hack to get tests compiling * Fix `sensitive_string_display_implementation` test * Use new fn name: `asType` -> `toType` * Refactor `ConstrainedStringGenerator` to not pass in `constraints` * Add `@pattern` test * Use `Lazy` instead of `OnceCell` * Store `&'static str` in `Pattern` error instead of `String` * Move `TraitInfo` to the bottom * Extract branches in `TraitInfo.fromTrait` into separate functions * Add `PatternTrait.validationErrorMessage` * Add more tests for `@pattern` * Add entry to `CHANGELOG.next.toml` * Fix a `@length` + `@pattern` test case * Remove unused binding in `ConstrainedStringGeneratorTest` * Remove calls to `trimIndent` * Use raw Rust strings instead of `escapedPattern` * Require `regex 1.5.5` instead of `1.7.0` * Use `Writable`s in `TraitInfo` * Use single `rust` call for `impl $name` block * Move `supportedStringConstraintTraits` to `Constraints.kt` * Fix error message mentioning wrong `ignoreUnsupportedConstraintTraits` key * Fix usage of `:W` in `rustTemplate` and raw Rust strings * Use shorthand form for `PatternTrait.validationErrorMessage` * Fix protocol tests by adjusting the validation message * Add uses of `@pattern` in `constraints.smithy` model * Fix broken `RestJsonMalformedPatternReDOSString` test * Move `TraitInfo` to its own module and separate string-specific details * Update codegen-core/common-test-models/constraints.smithy Co-authored-by: david-perez <[email protected]> * Fix nits in `constraints.smithy` * Add license to `TraitInfo.kt` * Make `StringTraitInfo.fromTrait` not return a nullable * Remove overzealous formatting * Add some padding to json in `fixRestJsonMalformedPatternReDOSString` * Add `compile_regex` function for `@pattern` strings * Add helpful error message when regexes fail to compile * Add missing coverage in `constraints.smithy` * Fix examples in `constraints.smithy` * Fix failing test * Combine writables in `ConstrainedStringGenerator` * Fix uses of `Writable.join` * Use `expect` over `unwrap_or_else` * Use `once_cell::sync::Lazy` over `once_cell::sync::OnceCell` Co-authored-by: david-perez <[email protected]>
smithy-lang · Nov 23, 2022 · eafbe80 · eafbe80
1 parent 4cc2f95
commit eafbe80
Show file tree

Hide file tree

Showing 12 changed files with 450 additions and 165 deletions.
diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
@@ -242,12 +242,13 @@ message = """
 
 * The `length` trait on `string` shapes.
 * The `length` trait on `map` shapes.
+* The `pattern` trait on `string` shapes.
 
 Upon receiving a request that violates the modeled constraints, the server SDK will reject it with a message indicating why.
 
 Unsupported (constraint trait, target shape) combinations will now fail at code generation time, whereas previously they were just ignored. This is a breaking change to raise awareness in service owners of their server SDKs behaving differently than what was modeled. To continue generating a server SDK with unsupported constraint traits, set `codegenConfig.ignoreUnsupportedConstraints` to `true` in your `smithy-build.json`.
 """
-references = ["smithy-rs#1199", "smithy-rs#1342", "smithy-rs#1401"]
+references = ["smithy-rs#1199", "smithy-rs#1342", "smithy-rs#1401", "smithy-rs#1998"]
 meta = { "breaking" = true, "tada" = true, "bug" = false, "target" = "server" }
 author = "david-perez"
 

diff --git a/codegen-core/common-test-models/constraints.smithy b/codegen-core/common-test-models/constraints.smithy
@@ -23,6 +23,12 @@ service ConstraintsService {
         QueryParamsTargetingMapOfListOfLengthStringOperation,
         QueryParamsTargetingMapOfSetOfLengthStringOperation,
         QueryParamsTargetingMapOfListOfEnumStringOperation,
+
+        QueryParamsTargetingMapOfPatternStringOperation,
+        QueryParamsTargetingMapOfListOfPatternStringOperation,
+        QueryParamsTargetingMapOfLengthPatternStringOperation,
+        QueryParamsTargetingMapOfListOfLengthPatternStringOperation,
+
         HttpPrefixHeadersTargetingLengthMapOperation,
         // TODO(https://github.com/awslabs/smithy-rs/issues/1431)
         // HttpPrefixHeadersTargetingMapOfEnumStringOperation,
@@ -97,6 +103,34 @@ operation QueryParamsTargetingMapOfListOfEnumStringOperation {
     errors: [ValidationException]
 }
 
+@http(uri: "/query-params-targeting-map-of-pattern-string-operation", method: "POST")
+operation QueryParamsTargetingMapOfPatternStringOperation {
+    input: QueryParamsTargetingMapOfPatternStringOperationInputOutput,
+    output: QueryParamsTargetingMapOfPatternStringOperationInputOutput,
+    errors: [ValidationException]
+}
+
+@http(uri: "/query-params-targeting-map-of-list-of-pattern-string-operation", method: "POST")
+operation QueryParamsTargetingMapOfListOfPatternStringOperation {
+    input: QueryParamsTargetingMapOfListOfPatternStringOperationInputOutput,
+    output: QueryParamsTargetingMapOfListOfPatternStringOperationInputOutput,
+    errors: [ValidationException]
+}
+
+@http(uri: "/query-params-targeting-map-of-length-pattern-string", method: "POST")
+operation QueryParamsTargetingMapOfLengthPatternStringOperation {
+    input: QueryParamsTargetingMapOfLengthPatternStringOperationInputOutput,
+    output: QueryParamsTargetingMapOfLengthPatternStringOperationInputOutput,
+    errors: [ValidationException],
+}
+
+@http(uri: "/query-params-targeting-map-of-list-of-length-pattern-string-operation", method: "POST")
+operation QueryParamsTargetingMapOfListOfLengthPatternStringOperation {
+    input: QueryParamsTargetingMapOfListOfLengthPatternStringOperationInputOutput,
+    output: QueryParamsTargetingMapOfListOfLengthPatternStringOperationInputOutput,
+    errors: [ValidationException]
+}
+
 @http(uri: "/http-prefix-headers-targeting-length-map-operation", method: "POST")
 operation HttpPrefixHeadersTargetingLengthMapOperation {
     input: HttpPrefixHeadersTargetingLengthMapOperationInputOutput,
@@ -187,6 +221,26 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
     enumStringListQuery: ListOfEnumString,
 }
 
+structure QueryParamsTargetingMapOfPatternStringOperationInputOutput {
+    @httpQueryParams
+    mapOfPatternString: MapOfPatternString
+}
+
+structure QueryParamsTargetingMapOfListOfPatternStringOperationInputOutput {
+    @httpQueryParams
+    mapOfListOfPatternString: MapOfListOfPatternString
+}
+
+structure QueryParamsTargetingMapOfLengthPatternStringOperationInputOutput {
+    @httpQueryParams
+    mapOfLengthPatternString: MapOfLengthPatternString,
+}
+
+structure QueryParamsTargetingMapOfListOfLengthPatternStringOperationInputOutput {
+    @httpQueryParams
+    mapOfLengthPatternString: MapOfListOfLengthPatternString,
+}
+
 structure HttpPrefixHeadersTargetingLengthMapOperationInputOutput {
     @httpPrefixHeaders("X-Prefix-Headers-LengthMap-")
     lengthMap: ConBMap,
@@ -298,7 +352,21 @@ structure ConA {
     // setOfLengthString: SetOfLengthString,
     mapOfLengthString: MapOfLengthString,
 
-    nonStreamingBlob: NonStreamingBlob
+    nonStreamingBlob: NonStreamingBlob,
+
+    patternString: PatternString,
+    mapOfPatternString: MapOfPatternString,
+    listOfPatternString: ListOfPatternString,
+    // TODO(https://github.com/awslabs/smithy-rs/issues/1401): a `set` shape is
+    //  just a `list` shape with `uniqueItems`, which hasn't been implemented yet.
+    // setOfPatternString: SetOfPatternString,
+
+    lengthLengthPatternString: LengthPatternString,
+    mapOfLengthPatternString: MapOfLengthPatternString,
+    listOfLengthPatternString: ListOfLengthPatternString
+    // TODO(https://github.com/awslabs/smithy-rs/issues/1401): a `set` shape is
+    //  just a `list` shape with `uniqueItems`, which hasn't been implemented yet.
+    // setOfLengthPatternString: SetOfLengthPatternString,
 }
 
 map MapOfLengthString {
@@ -321,6 +389,16 @@ map MapOfListOfEnumString {
     value: ListOfEnumString,
 }
 
+map MapOfListOfPatternString {
+    key: PatternString,
+    value: ListOfPatternString
+}
+
+map MapOfListOfLengthPatternString {
+    key: LengthPatternString,
+    value: ListOfLengthPatternString
+}
+
 map MapOfSetOfLengthString {
     key: LengthString,
     // TODO(https://github.com/awslabs/smithy-rs/issues/1401): a `set` shape is
@@ -346,6 +424,13 @@ string MaxLengthString
 @length(min: 69, max: 69)
 string FixedLengthString
 
+@pattern("[a-d]{5}")
+string PatternString
+
+@pattern("[a-f0-5]*")
+@length(min: 5, max: 10)
+string LengthPatternString
+
 @mediaType("video/quicktime")
 @length(min: 1, max: 69)
 string MediaTypeLengthString
@@ -383,6 +468,14 @@ set SetOfLengthString {
     member: LengthString
 }
 
+set SetOfPatternString {
+    member: PatternString
+}
+
+set SetOfLengthPatternString {
+    member: LengthPatternString
+}
+
 list ListOfLengthString {
     member: LengthString
 }
@@ -391,6 +484,14 @@ list ListOfEnumString {
     member: EnumString
 }
 
+list ListOfPatternString {
+    member: PatternString
+}
+
+list ListOfLengthPatternString {
+    member: LengthPatternString
+}
+
 structure ConB {
     @required
     nice: String,
@@ -443,6 +544,16 @@ list NestedList {
 //     member: String
 // }
 
+map MapOfPatternString {
+    key: PatternString,
+    value: PatternString,
+}
+
+map MapOfLengthPatternString {
+    key: LengthPatternString,
+    value: LengthPatternString,
+}
+
 @length(min: 1, max: 69)
 map ConBMap {
     key: String,

diff --git a/...n-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt b/...n-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt
@@ -21,6 +21,7 @@ import software.amazon.smithy.model.traits.LengthTrait
 import software.amazon.smithy.model.traits.PatternTrait
 import software.amazon.smithy.model.traits.RangeTrait
 import software.amazon.smithy.model.traits.RequiredTrait
+import software.amazon.smithy.model.traits.Trait
 import software.amazon.smithy.model.traits.UniqueItemsTrait
 import software.amazon.smithy.rust.codegen.core.smithy.isOptional
 import software.amazon.smithy.rust.codegen.core.util.UNREACHABLE
@@ -42,6 +43,8 @@ fun Shape.hasConstraintTrait() =
         hasTrait<RangeTrait>() ||
         hasTrait<RequiredTrait>()
 
+val supportedStringConstraintTraits: List<Class<out Trait>> = listOf(LengthTrait::class.java, PatternTrait::class.java)
+
 /**
  * We say a shape is _directly_ constrained if:
  *
@@ -66,7 +69,7 @@ fun Shape.isDirectlyConstrained(symbolProvider: SymbolProvider): Boolean = when
         this.members().map { symbolProvider.toSymbol(it) }.any { !it.isOptional() }
     }
     is MapShape -> this.hasTrait<LengthTrait>()
-    is StringShape -> this.hasTrait<EnumTrait>() || this.hasTrait<LengthTrait>()
+    is StringShape -> this.hasTrait<EnumTrait>() || supportedStringConstraintTraits.any { this.hasTrait(it) }
     else -> false
 }
 
@@ -91,7 +94,7 @@ fun MemberShape.targetCanReachConstrainedShape(model: Model, symbolProvider: Sym
 
 fun Shape.hasPublicConstrainedWrapperTupleType(model: Model, publicConstrainedTypes: Boolean): Boolean = when (this) {
     is MapShape -> publicConstrainedTypes && this.hasTrait<LengthTrait>()
-    is StringShape -> !this.hasTrait<EnumTrait>() && (publicConstrainedTypes && this.hasTrait<LengthTrait>())
+    is StringShape -> !this.hasTrait<EnumTrait>() && (publicConstrainedTypes && supportedStringConstraintTraits.any(this::hasTrait))
     is MemberShape -> model.expectShape(this.target).hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)
     else -> false
 }

diff --git a/...n/software/amazon/smithy/rust/codegen/server/smithy/PatternTraitValidationErrorMessage.kt b/...n/software/amazon/smithy/rust/codegen/server/smithy/PatternTraitValidationErrorMessage.kt
@@ -0,0 +1,12 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.rust.codegen.server.smithy
+
+import software.amazon.smithy.model.traits.PatternTrait
+
+@Suppress("UnusedReceiverParameter")
+fun PatternTrait.validationErrorMessage(): String =
+    "Value {} at '{}' failed to satisfy constraint: Member must satisfy regular expression pattern: {}"
diff --git a/...rc/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt b/...rc/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCargoDependency.kt
@@ -25,6 +25,7 @@ object ServerCargoDependency {
     val PinProjectLite: CargoDependency = CargoDependency("pin-project-lite", CratesIo("0.2"))
     val Tower: CargoDependency = CargoDependency("tower", CratesIo("0.4"))
     val TokioDev: CargoDependency = CargoDependency("tokio", CratesIo("1.8.4"), scope = DependencyScope.Dev)
+    val Regex: CargoDependency = CargoDependency("regex", CratesIo("1.5.5"))
 
     fun SmithyHttpServer(runtimeConfig: RuntimeConfig) = runtimeConfig.runtimeCrate("http-server")
 }

diff --git a/...otlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt b/...otlin/software/amazon/smithy/rust/codegen/server/smithy/ValidateUnsupportedConstraints.kt
@@ -16,7 +16,6 @@ import software.amazon.smithy.model.shapes.ServiceShape
 import software.amazon.smithy.model.shapes.SetShape
 import software.amazon.smithy.model.shapes.Shape
 import software.amazon.smithy.model.shapes.ShapeId
-import software.amazon.smithy.model.shapes.StringShape
 import software.amazon.smithy.model.shapes.UnionShape
 import software.amazon.smithy.model.traits.EnumTrait
 import software.amazon.smithy.model.traits.LengthTrait
@@ -42,7 +41,7 @@ private sealed class UnsupportedConstraintMessageKind {
             This is not supported in the smithy-rs server SDK.
             ${ if (willSupport) "It will be supported in the future." else "" }
             See the tracking issue ($trackingIssue).
-            If you want to go ahead and generate the server SDK ignoring unsupported constraint traits, set the key `ignoreUnsupportedConstraintTraits`
+            If you want to go ahead and generate the server SDK ignoring unsupported constraint traits, set the key `ignoreUnsupportedConstraints`
             inside the `runtimeConfig.codegenConfig` JSON object in your `smithy-build.json` to `true`.
             """.trimIndent().replace("\n", " ")
 
@@ -86,10 +85,6 @@ private sealed class UnsupportedConstraintMessageKind {
                 level,
                 buildMessageShapeHasUnsupportedConstraintTrait(shape, lengthTrait, constraintTraitsUberIssue),
             )
-            is UnsupportedPatternTraitOnStringShape -> LogMessage(
-                level,
-                buildMessageShapeHasUnsupportedConstraintTrait(shape, patternTrait, constraintTraitsUberIssue),
-            )
             is UnsupportedRangeTraitOnShape -> LogMessage(
                 level,
                 buildMessageShapeHasUnsupportedConstraintTrait(shape, rangeTrait, constraintTraitsUberIssue),
@@ -106,7 +101,6 @@ private data class UnsupportedConstraintOnMemberShape(val shape: MemberShape, va
 private data class UnsupportedConstraintOnShapeReachableViaAnEventStream(val shape: Shape, val constraintTrait: Trait) : UnsupportedConstraintMessageKind()
 private data class UnsupportedLengthTraitOnStreamingBlobShape(val shape: BlobShape, val lengthTrait: LengthTrait, val streamingTrait: StreamingTrait) : UnsupportedConstraintMessageKind()
 private data class UnsupportedLengthTraitOnCollectionOrOnBlobShape(val shape: Shape, val lengthTrait: LengthTrait) : UnsupportedConstraintMessageKind()
-private data class UnsupportedPatternTraitOnStringShape(val shape: Shape, val patternTrait: PatternTrait) : UnsupportedConstraintMessageKind()
 private data class UnsupportedRangeTraitOnShape(val shape: Shape, val rangeTrait: RangeTrait) : UnsupportedConstraintMessageKind()
 private data class UnsupportedUniqueItemsTraitOnShape(val shape: Shape, val uniqueItemsTrait: UniqueItemsTrait) : UnsupportedConstraintMessageKind()
 
@@ -146,20 +140,20 @@ fun validateOperationsWithConstrainedInputHaveValidationExceptionAttached(model:
             LogMessage(
                 Level.SEVERE,
                 """
-                Operation ${it.shape.id} takes in input that is constrained 
-                (https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation 
+                Operation ${it.shape.id} takes in input that is constrained
+                (https://awslabs.github.io/smithy/2.0/spec/constraint-traits.html), and as such can fail with a validation
                 exception. You must model this behavior in the operation shape in your model file.
                 """.trimIndent().replace("\n", "") +
                     """
-                        
-                ```smithy
-                use smithy.framework#ValidationException
-                
-                operation ${it.shape.id.name} {
-                    ...
-                    errors: [..., ValidationException] // <-- Add this.
-                }
-                ```
+
+                    ```smithy
+                    use smithy.framework#ValidationException
+
+                    operation ${it.shape.id.name} {
+                        ...
+                        errors: [..., ValidationException] // <-- Add this.
+                    }
+                    ```
                     """.trimIndent(),
             )
         }
@@ -214,17 +208,7 @@ fun validateUnsupportedConstraints(model: Model, service: ServiceShape, codegenC
         .map { UnsupportedLengthTraitOnCollectionOrOnBlobShape(it, it.expectTrait()) }
         .toSet()
 
-    // 5. Pattern trait on string shapes is used. It has not been implemented yet.
-    // TODO(https://github.com/awslabs/smithy-rs/issues/1401)
-    val unsupportedPatternTraitOnStringShapeSet = walker
-        .walkShapes(service)
-        .asSequence()
-        .filterIsInstance<StringShape>()
-        .filterMapShapesToTraits(setOf(PatternTrait::class.java))
-        .map { (shape, patternTrait) -> UnsupportedPatternTraitOnStringShape(shape, patternTrait as PatternTrait) }
-        .toSet()
-
-    // 6. Range trait on any shape is used. It has not been implemented yet.
+    // 5. Range trait on any shape is used. It has not been implemented yet.
     // TODO(https://github.com/awslabs/smithy-rs/issues/1401)
     val unsupportedRangeTraitOnShapeSet = walker
         .walkShapes(service)
@@ -247,7 +231,6 @@ fun validateUnsupportedConstraints(model: Model, service: ServiceShape, codegenC
             unsupportedLengthTraitOnStreamingBlobShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
             unsupportedConstraintOnShapeReachableViaAnEventStreamSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
             unsupportedLengthTraitOnCollectionOrOnBlobShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
-            unsupportedPatternTraitOnStringShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
             unsupportedRangeTraitOnShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) } +
             unsupportedUniqueItemsTraitOnShapeSet.map { it.intoLogMessage(codegenConfig.ignoreUnsupportedConstraints) }