Add a validator for integration with custom complex validations #659
Conversation
| @@ -139,6 +140,14 @@ object Validator extends ValidatorMagnoliaDerivation with ValidatorEnumMacro { | |||
| } | |||
| } | |||
|
|
|||
| case class CustomCaseClass[T](doValidate: T => List[InvalidField[_]]) extends Single[T] { | |||
There was a problem hiding this comment.
We already have Product validator which can validate arbitrary case classes.
The problem we are trying to solve is that with customValidator currently we cannot return more than one ValidationError.
To do this we should move message out of constructor.
But that isn't so simple because of how the end message for user is built. In ServerDefaults we do pattern matching on validator's type to provide user-friendly message:
/**
* Default messages when the decode failure is due to a validation error.
*/
object ValidationMessages {
/**
* Default message describing why a value is invalid.
* @param valueName Name of the validated value to be used in error messages
*/
def invalidValueMessage[T](ve: ValidationError[T], valueName: String): String =
ve.validator match {
....
I think that this could be addressed by splitting ValidationError into ValidationErrorCustom and ValidationErrorPrimitive, so that ValidationErrorCustom wouldn't need to have validator: Validator.Primitive field. For ValidationErrorPrimitive we could reuse the current logic, and for ValidationErrorCustom we would just concat the error messages.
WDYT?
There was a problem hiding this comment.
But Validator.Custom extends Validator.Primitive, so that shouldn't be a problem?
There was a problem hiding this comment.
It is because we want to have a list of error messages inside ValidationError rather than reference to Validator.Custom
There was a problem hiding this comment.
ahh yes I get it. Hm, maybe ValidationError should contain an optional message, and if it's not specified, the default one is used?
There was a problem hiding this comment.
Yes, it is an option, but I would rather prefer to create another type which more clearly express developers' intentions.
There was a problem hiding this comment.
ok, so I don't think we want to have a list of error messages inside ValidationError, but rather a list of validation errors, each with its own message. Validating a single value can lead to multiple errors on multiple (nested) values - as in any complex case class.
So the basic method of a validator needs to remain T => List[ValidationError]. Now as you write, we can have two subclasses of that, one binding the error with a specific validator (as currently - DefaultValidationError?), and the other one with a custom message, or maybe rather: custom constraint description (that's being validated), e.g. CustomValidationError. That should cover the use-cases I guess.
| @@ -139,6 +140,14 @@ object Validator extends ValidatorMagnoliaDerivation with ValidatorEnumMacro { | |||
| } | |||
| } | |||
|
|
|||
| case class CustomCaseClass[T](doValidate: T => List[InvalidField[_]]) extends Single[T] { | |||
There was a problem hiding this comment.
maybe we can simply change the Custom validator so that it accepts a doValidate: T= > List[InvalidValue] function?
There was a problem hiding this comment.
Wouldn't that require executing doValidate function twice - one for validation and second time for calculating error message?
There was a problem hiding this comment.
yeah, we need to design it so that we avoid calling it twice :)
ghostbuster91
force-pushed
the
feature/custom-validator
branch
from
46570b5
to
723da5a
Compare
ghostbuster91
force-pushed
the
feature/custom-validator
branch
from
723da5a
to
8f89a37
Compare
| @@ -236,7 +232,7 @@ object Validator extends ValidatorMagnoliaDerivation with ValidatorEnumMacro { | |||
| case MaxLength(value) => Some(s"length<=$value") | |||
| case MinSize(value) => Some(s"size>=$value") | |||
| case MaxSize(value) => Some(s"size<=$value") | |||
| case Custom(_, message) => Some(message) | |||
| case Custom(_) => Some("custom message") //TODO? | |||
There was a problem hiding this comment.
Maybe we could add additional optional property - description which will be used for that purpose?
|
Released in 0.17.0-M1, thanks! |
No description provided.