Add support for constraints #201
Conversation
| } | ||
|
|
||
| trait Validator[T] { outer => | ||
| def validate(t: T): Boolean |
There was a problem hiding this comment.
Would it be more useful for this to return a cats Validated or ValidatedNel? That way the Validate decoder can expose specific errors.
There was a problem hiding this comment.
yes we should probably return some more information. Validated would be good though it would mean adding cats as a dependency. So I'm not sure about that yet :)
There was a problem hiding this comment.
Couldn't we just copy Validated from cats?
There was a problem hiding this comment.
maybe let's start with a simple ValidationError(msg: String)?
| override def validate(t: T): Boolean = constraints.forall(_.check(t)) | ||
| } | ||
|
|
||
| trait Constraint[T] { |
There was a problem hiding this comment.
Will this eventually be sealed, or users be able to define their own custom constraints? If it's custom, I think there will need to be a 2nd method defining how to express the constraint in the OpenAPI model.
There was a problem hiding this comment.
I think we should allow users to define their custom constraints. Initially we might only support a well-defined subset for openapi (that's a closed set anyway), and discard any custom constraints
There was a problem hiding this comment.
For the first version lets make it sealed and we will think about allowing users to define their custom constraints latter.
| } | ||
|
|
||
| trait Constraint[T] { | ||
| def check(t: T): Boolean |
There was a problem hiding this comment.
Same question as above, should this return a Validated?
| case TSchema.SInteger => | ||
| Right(OSchema(SchemaType.Integer)) | ||
| case (TSchema.SInteger, ValueValidator(constraints)) => | ||
| Right(OSchema(SchemaType.Integer).copy(minimum = constraints.collectFirst { case Constraint.Minimum(v) => v })) |
There was a problem hiding this comment.
This pattern feels like it might be tough to scale as the number of constraints grows. Would it be better to add an OSchema => OSchema method onto Constraint so the custom logic for each constraint is located in a single place?
docs/openapi-docs/src/main/scala/tapir/docs/openapi/schema/ObjectSchemasForEndpoints.scala
Outdated
Show resolved
Hide resolved
As a result it fixes failing serverTests. This change introduces also a bug when calling magnolia fallback method from another macro Because of that we add instances for validator of commonly used types to workaround that problem
|
|
||
| private[tapir] val validator: Validator[T] = Validator.passing | ||
|
|
||
| def decodeAndValidate(f: F): DecodeResult[T] = { |
There was a problem hiding this comment.
now we have three methods: decode, safeDecode and decodeAndValidate. This might be confusing. Maybe we could rename decode to rawDecode or sth like that, and keep only decode which would (1) validate (2) catch exceptions (3) call rawDecode? That would also collapse the Validate and Decode traits into one.
I don't think there's any use-case in calling these methods separately
There was a problem hiding this comment.
Agree, but I at the same time I like the separation of decode and validate. Also decode/encode creates nice couple.
How about renaming decodeAndValidate to safeDecode, leaving Decode::safeDecode (will be called using super.safeDecode) and also leaving Codec::decode without change?
Btw: This is already implemented so you can see how it looks like
There was a problem hiding this comment.
Separation as private methods: sure. But no need to introduce public traits/methods, which can only lead to confusion.
For me safeDecode doesn't say anything about validation, only exceptions. So I'd either go with decode - meaning the whole process (raw decode, exceptions, validations), or sth with "validation" in the name
| def forArray: Validator[Array[T]] = (t: Array[T]) => { | ||
| t.forall(outer.validate) | ||
| } | ||
| def forSet: Validator[Set[T]] = (t: Set[T]) => { |
There was a problem hiding this comment.
maybe we can have a single ElementValidator which takes an Iterable? The logic is all the same here.
There was a problem hiding this comment.
I've just added support for collections. There were two problems:
- I had to create two implementations - one for array and the other one for iterable
- There is an ugly cast inside each one, types should be ok, but I couldn't omit it.
# Conflicts: # docs/openapi-docs/src/main/scala/tapir/docs/openapi/schema/ObjectSchemasForEndpoints.scala # docs/openapi-docs/src/test/scala/tapir/docs/openapi/VerifyYamlTest.scala # openapi/openapi-model/src/main/scala/tapir/openapi/OpenAPI.scala
ghostbuster91
changed the title
|
Released in 0.10.0 |
This refers to #48