Validators for collections#1879
Validators for collections#1879msosnicki wants to merge 9 commits intodisneystreaming:series/0.18from
Conversation
| memberRefinements = memberRefinements | ||
| ) | ||
|
|
||
| override def toSchema(a: Schema[A]): Schema[B] = { |
There was a problem hiding this comment.
This is not correct in the current implementation. See for example how ValidatedString changed - the underlyingType contains schema with all refinements attached to it. We also have validator with the same schema information duplicated. Then we have implicit val schema = validator.toSchema(underlyingSchema).
So the resulting schema will have refinements applied twice.
I removed that duplication, but that means that the Validator.toSchema` is just applying the outermost bijection, which in my opinion defeats it's purpose.
|
|
||
| } | ||
|
|
||
| @deprecated |
There was a problem hiding this comment.
todo: add details for all deprecations + update changelog
| def validate(value: A): Either[String, B] | ||
|
|
||
| // todo: deprecate | ||
| def toSchema(a: Schema[A]): Schema[B] |
There was a problem hiding this comment.
I think this method is a bit problematic, especially the more complex scenario there is. For example, see the code generated for ValidatedRefinedListConstrainedMember.
The underlyingType contains the full schema besides bijection, and I haven't changed that.
So doing the schema = validator.toSchema(underlyingSchema) where the validator contains all the schema bindings kind of duplicated. There was a bug present before (see here) that was wrapping schemas in refinements multiple times and after fixing it the validator is just applying the outermost bijection.
| package smithy4s | ||
|
|
||
| sealed trait Validator[A, B] { | ||
| sealed trait Validator[A, B] { self => |
There was a problem hiding this comment.
I tried not to change the current API too much, but given the issues highlighted in the other comments, I think that having a simpler interface, just:
trait Validator[A] {
def validate(a: A): Either[String, A]
}and having a schema visitor Schema ~> Validator that works for any type would be much better. at least at first glance. We would get rid of the duplication of schema bindings that are basically the same in the Schema and Validator members, and this would open up possibility to validate any type, not just the ones that the @validatedNewtype is restricted to.
Not sure how it would fit the 0.18 though
PR Checklist (not all items are relevant to all PRs)