[Draft] Refactoring Validator

[geirolz]

No description provided.

[adamw]

Thanks for your work! I like the general direction and some changes in particular, though I would also keep some aspects of the original design.

[adamw]

I think both designs are equivalent (abstract show method in the parent trait / show as a pattern-match), so I'm wondering what's the rationale here for the change?

[geirolz]

The idea was to keep the definition of validator in just one place. As you can see in this design when you define a Validator you can specify the error and the show(that contains a symbolic representation of the validator).

I see the show as a property of the validator and not just an optional thing, this is why I centralized it.
If this doesn't make sense for you I can use pattern matching np :)

[adamw]

If validator wasn't a sealed trait I would definitely agree :) But since it is ... well I guess both designs are equivalent so I can go with either.

[adamw]

The goal here was to separate the strucutre of the validators from rendering them as a string value. What's the benefit of moving this logic inside ValidatorResult trait?

[geirolz]

Given the proposed design, all these methods are not needed now, t is just needed a method to simply reduce messages, given these small methods I thought to move them inside ValidatorResult.
Maybe these can be helpful for clients when they want to test their custom validators.

I don't have a strong opinion on this, If you prefer a can move those 2 methods inside DecodeFailureHadler 👍

[adamw]

I'm not sold on using a value class here. I like representing FieldNames as a value, though. Maybe it would be enough (and marginally less type safe) to simply have a case class FieldPath(path: List[FieldName]), with a .toString method rendering it in the dot-notation?

[adamw]

Or rather .toStringPath as it needs to return an Option[String]

[geirolz]

I say yes and no and the same time :)
I say yes because makes sense to have a toStringPath method and avoid the value class where reading the value.
But I say no(or at least I'd like to discuss it) because maybe it is less clear when you use it as parameter

Here for example If we remove FieldPath we have an Endo of String, since this will be available to clients to build custom errors for custom validators maybe is better to keep the value class to make the signature more clear (?)

case class ValidationError(f: FieldPath => String, path: FieldNames = FieldNames.empty)

[adamw]

Hm well I suppose this depends on how the other comments get resolved. Now that I understand more of your design, I agree that using a value class makes sense here, and FieldPath => String is definitely better than String => String.

But (there's always a but ;) ), as I mentioned in other comments, I would prefer to build the validation error messages externally - not as part of ValidationResult/Errors functionality. I think such a design makes more sense at it looses precision/information later, but I can also try to get convinced otherwise :)

[adamw]

(and when we build error messages externally, I think there would be no f function which could cause confusion)

[adamw]

I think I'd prefer to retain the validator that caused the error as part of the ValidationError - currently in the new design we are loosing this information, in favour of constructing the error description earlier. I'd defer creating the string to a later time, and

[geirolz]

This was one of my biggest doubts about this refactoring.
In the end, I've used this design for those reasons:

• The Validator info is not used actually (ideally it's great to have it but actually is useless and introduce to many compromises)
• The early creation of the error message allows more flexibility to custom validator
• Better responsibility reparation, ValidationResult is agnostic, and potentially reusable for custom validators( I mean ValidationError here)

Adding the Validator info to ValidationError is trivial but introduce some problems for the custom validators, check this example

case class Custom[T](doValidate: T => ValidationResult[T], showMessage: Option[String] = None) extends 
  Validator[T] {
    override def show: Option[String] = showMessage.orElse(Some("custom"))
    override def apply(t: T): ValidationResult[T] = doValidate(t)
  }

Here you don't have a Validator instance to create the error because you are creating it. For sure we can create an ad-hoc error for custom validators but I'm not sure if this is so important to justify all this design effort.

Maybe we can derive this information that is needed in some other ways, I mean, if you have a Validator instance ad you run it you have both, Validator and ValidationResult.

[adamw]

Ok thanks, let me try to unpack this so that we're on the same page :)

Not sure what you mean by validator info not being used - it was being used in ValdiatorMessages to create the appropriate message?

I do get the point with the custom validator - so far the message structure was fixed (unless you overwrote the DecodeFailureHandler) to s"expected $valueName to pass custom validation: ${c.message}, but was '${ve.invalidValue}'", and you'd like to customise this?

Maybe we could have both with a structure like this:

case class ValidationError[T](v: Validator[T], invalidValue: T, path: FieldPath, customMessage: Option[String])`

Then, we retain all of the information, and given a ValdiationError we can create a nice error message - using the default structure, unless there's a custom message defined, which entirely replaces the default one (which is a change from what we have now). What do you think?

[geirolz]

Not sure what you mean by validator info not being used - it was being used in ValdiatorMessages to create the appropriate message?

Yes exactly, but just to do pattern matching and create the right error message. But If now we create the error message when we are defining the validator this is no needed

---

Maybe we could have both with a structure like this

The problem is that you don't have a Vallidator instance when you are defining it, this problem exists only for custom validators

Given this

case class Custom[T](doValidate: T => ValidationResult[T], showMessage: Option[String] = None) extends Validator[T]
case class ValidationError[T](v: Validator[T], invalidValue: T, path: FieldPath, customMessage: Option[String])

We ha some problems doing this, moreover we are duplicating the invalidValue(both inside Invalid and ValidationError)

val myCustomVal : Validator[Int] = Custom[Int](myNumb match {
   case 1 => ValidationResult.Valid(myNumb)
   case _ => ValidationResult.Invalid(myNumb, List(
      ValidationError(???, myNumb, ???, Some("Number must be 1"))
  ))
}

[geirolz]

Another proposal, instead of delegating the creation of the full error message to each Validator why we don't just use a String for describing the cause of the error and create the full error message just changing a little bit the template

Like this

case class ValidationError(cause: String, path: FieldNames = FieldNames.empty) {

  def prependPath(f: FieldName): ValidationError =
    copy(path = path.prependPath(f))

  def message: String = s"Validation failed at `${path.asPath.getOrElse(FieldPath("value"))}` because: $cause"
}

So the previous example becomes

val myCustomVal : Validator[Int] = Custom[Int](myNumb match {
   case 1 => ValidationResult.Valid(myNumb)
   case _ => ValidationResult.Invalid(myNumb, List(
      ValidationError(s"$myNumb is not 1")
  ))
}

myCustomVal
   .validate(0)
   .invalid
   .get
   .description // Validation failed for `value` because: 0 is not 1

While some standards validator becomes

case class Min[T](value: T, exclusive: Boolean)(implicit val valueIsNumeric: Numeric[T]) extends Primitive[T] {
    override def show: Option[String] = Some(s"${if (exclusive) ">" else ">="}$value")
    override def apply(t: T): ValidationResult[T] =
      ValidationResult.when(valueIsNumeric.gt(t, value) || (!exclusive && valueIsNumeric.equiv(t, value)))(
        ifTrue = Valid(t),
        ifFalse = Invalid(
          value = t,
          errors = List(ValidationError(s"$t is not be greater than ${if (exclusive) "" else "or equal to "}$value"))
        )
      )
  }

[geirolz]

We just need the ValidationResult and not the Validator instance to build the error message. If the results contain the reason(as Circe do) we can build the error message in DecodeFailureHandler just using the result of the validation

for example:

case class ValidationError(expectedValue: T, reason: String)

//DecodeFailureHandler
val validationResult: ValidationResult[T] = ???
val errors: Option[List[String]] = validationResult.invalid.map(invalid => invalid.errors.map(e => {
  s"Validation failed at `${e.path.asPath.getOrElse(FieldPath("value"))}` because: ${e.cause}, expected ${e.expectedValue}"
  })
)

[adamw]

Well but you can't e.g. customise the message for Min specifically. Let's say I'd like to change expected $valueName to be greater than to polish or some other exotic language ;)

In the above code, only the general template which is used to present validation errors is customisable. I think that anything that ends up being sent to the user as a string should be customisable, or at leat we should try :)

[geirolz]

Ok i got you! and what do you think I we have the possibility to recover the validator ? In this way we can re-map the error message

//Validator
def recoverWith(f: Invalid[T] => ValidationResult[T]): Validator[T]

//ValidationResult.Invalid
def withError(error: ValidationError): Invalid[T]

Min(1, false).recoverWith(_.withError(ValidationError("Must be non empty!")))

[adamw]

Hm I suppose to answer that we have to consider where you'd prefer to customize the error messages: centrally in DecodeFailureHandler, or individually per-validator usage.

I guess there are use-cases for both, and as I think we already noted in the PR, we do need a way to customise the error message entirely. But (excuse me for my stubbornness) I think that central configuration is more often useful - so that you don't have to always remember to use a customised-error-message-Min-validator, you just customize it once when the message is generated.

[geirolz]

Ok makes sense, let's use the 'Validator' instance 👍 I'll update the PR as soon as possibile

[adamw]

Closing in favor of #2108. @geirolz if you'd like some more changes to the design please comment there or open new issues :)