Dynamic validation - #1991
Conversation
| enumSchema(e.asInstanceOf[EnumerationSchema[Enum[?]]]) | ||
| .asInstanceOf[Schema[A]] |
There was a problem hiding this comment.
Casts are pretty risky in Dynamic land. Any way we can make this work without them?
There was a problem hiding this comment.
I don't think we need to limit ourselves to JVM here, this could work on all platforms.
| * behaviour), mirroring the fact that codegen-produced schemas enforce these | ||
| * constraints but dynamically-loaded do not). | ||
| */ | ||
| def loadModel( |
There was a problem hiding this comment.
We also have a platform-agnostic variant that works on our own model of Model - let's add performValidation to it too
| /** | ||
| * Loads a dynamic schema index model from a smithy model. | ||
| * | ||
| * @param performValidation when true, constraint traits (`@length`, `@range`, |
There was a problem hiding this comment.
My first thought seeing this name was that we'd be doing model validations rather than schema validations...
maybe addSchemaValidators or applySchemaRefinements would fit better?
| def reifyConstraints(index: DynamicSchemaIndex): DynamicSchemaIndex = | ||
| new DynamicSchemaIndex { | ||
| def allServices: Iterable[DynamicSchemaIndex.ServiceWrapper] = | ||
| index.allServices |
There was a problem hiding this comment.
- I believe this isn't right: operation inputs/outputs/errors won't have their traits reified. UNLESS the default DSI implementation calls
getSchema(shapeId)- in which case they would be reified - but we can't assume that about theindexinstance here, as it's an implementation detail and not something enforced by a contract.
To be safe, we could convert those services to builders and map each endpoint's input/output/errors, reifying the constraints right there. I think a simpler alternative to this would be embedding the reification process directly in the DynamicCompiler rather than in a "middleware" (here)
|
|
||
| private object ReifyConstraints extends (Schema ~> Schema) { | ||
|
|
||
| private def void[C, A]( |
There was a problem hiding this comment.
I think at this point we should move this to RefinementProvider. It was always a bit of a hack, having it as a more first-class thing would feel better
| private def collection[C[_], B]( | ||
| schema: Schema.CollectionSchema[C, B] | ||
| ): Schema[C[B]] = | ||
| schema.tag match { |
There was a problem hiding this comment.
AFAIK that's not an exhaustive match (CollectionTag is open in 0.19). We should have another case for arbitrary tags.
| case t @ PrimitiveSchema(_, _, tag) => | ||
| tag match { | ||
| case PString => | ||
| t.reifyHint(RefinementProvider.stringLengthConstraint) |
| m.reifyHint( | ||
| RefinementProvider.lengthConstraint[c[k, v]](m.tag.iterator(_).size) | ||
| ) | ||
| case other => other |
There was a problem hiding this comment.
I'd handle the remainder explicitly, like in smithy-playground
| # 0.19.11 | ||
|
|
||
| - codegen: Fix an `IllegalAccessError` (e.g. `class ...IncludeClosures cannot access its abstract superclass ...BackwardCompatHelper`) that could occur during `smithy4sCodegen` when a project dependency pulled a different version of a Smithy library (`smithy-build`, `smithy-model`, etc.) than the one bundled with the codegen plugin. The model-loading `URLClassLoader` used the plugin classloader as its parent, so a duplicate copy of a plugin-provided module on the child loader could split a package across two classloaders and break package-private access. The codegen now drops any dependency already provided by the parent classloader (tracked via the new `BuildInfo.codegenDependencies`) from the child classloader, so Smithy versions no longer need to be aligned between the plugin and the project's dependencies. | ||
| - dynamic: Add `DynamicSchemaIndex.loadModel(model, performValidation: Boolean)` overload. When `performValidation = true`, constraint traits (`@length`, `@range`, `@pattern`, ...) are reified into schema refinements instead of being kept just as hints, matching the behaviour of codegen-produced schemas. |
There was a problem hiding this comment.
.11 is already out, we need a new section :)
aa4cea6 to
3439438
Compare
|
@kubukoz thanks for review, I think all your comments are now addressed |
2 participants
PR Checklist (not all items are relevant to all PRs)