pimbrouwers / Validus

An extensible F# validation library.
Apache License 2.0
144 stars 10 forks source link

Implemented chain Validator operation and added operator #11

Closed xperiandri closed 2 years ago

xperiandri commented 2 years ago

Also cleaned up operators available in FsToolkit.ErrorHandling

pimbrouwers commented 2 years ago

Interesting idea.

I had naively just assumed that you would always want to execute all validators everytime. Can you describe a use case for this approach, where the combinatory method would not work?

xperiandri commented 2 years ago

@pimbrouwers you can see the test I have written. It describe the scenario well

xperiandri commented 2 years ago

I cannot figure out why the second Age validator not executed https://github.com/pimbrouwers/Validus/blob/e4c2f632f525c914be2b92ea939d9cda8f3000b2/test/Validus.Tests/ValidationResultTests.fs#L157

pimbrouwers commented 2 years ago

I cannot figure out why the second Age validator not executed

https://github.com/pimbrouwers/Validus/blob/e4c2f632f525c914be2b92ea939d9cda8f3000b2/test/Validus.Tests/ValidationResultTests.fs#L157

Let me look into that for you!

pimbrouwers commented 2 years ago

Had a go at this tonight, implemented Validator.chain and added an operator <-> for it. Below is the code from the tests. Let me know your thoughts!

https://github.com/pimbrouwers/Validus/blob/5bd9fa0450ecf1e9f508a76f2c992e49012eb115/test/Validus.Tests/ValidationResultTests.fs#L52-L102

xperiandri commented 2 years ago

I thought about different operators and minus does not represent the sense of the operation. I thought about <|> like first or second. But it does not represent the sense of the operation too. I thought about <&> as logical and which will not execute the right part if the left part is false. But in that case <+> should be <|>

xperiandri commented 2 years ago

So that fish and back fish look the most appropriate for me. As the operation is similar to bind but with 2 input parameters

pimbrouwers commented 2 years ago

I thought about different operators and minus does not represent the sense of the operation. I thought about <|> like first or second. But it does not represent the sense of the operation too. I thought about <&> as logical and which will not execute the right part if the left part is false. But in that case <+> should be <|>

I like the pipe actually. These are totally scope-specific, I am not to fussed about matching any type of FP norm here. The <|> to me sort of symbolized a "guard" which is effectively what this is.

My only remaining qualm is the need for bracketing, but I don't believe this is avoidable.

 Validators.Default.String.notEmpty <|> 
    (Validators.Default.String.pattern "^\S" 
     <+> Validators.Default.String.pattern "\S$") 
xperiandri commented 2 years ago

@abelbraaksma what do you think?

abelbraaksma commented 2 years ago

My only remaining qualm is the need for bracketing, but I don't believe this is avoidable.

You could solve that by using a different operator that has higher precedence (or lower, depending on what you think the preferred evaluation is). Unfortunately, in F# (unless they changed this recently) the precedence is calculated from the first character in the operator only. This means: < in this case, which both operators share.

As the operation is similar to bind but with 2 input parameters

W.r.t. the Kleisli operators, I agree they're most similar, but people may confuse them for actual Kleisli operators.

This all gets quite close to validation as used in parsing. You can get some ideas there. I've use FParsec a lot and the operations you're adding here seem similar. As a result, I wouldn't use <|> unless you really mean it to mean choice: pick the first that validates, but I don't think that's the intention.

When I read this:

 Validators.Default.String.notEmpty <|>   // thought: must not be empty, OR
    (Validators.Default.String.pattern "^\S"   // must not start with string, AND
     <+> Validators.Default.String.pattern "\S$")   // must not end with string

Don't you mean the opposite? If it is not empty, it is already "true" and the rest does not need to be validated. But if it is empty, the other patterns can never be true.

Basically what I'm saying: be careful using the choice operator <|> for this. In the end you can choose whatever, but ideally it should be as close as possible to what people may already be accustomed to.

abelbraaksma commented 2 years ago

Thinking about this a little more, let's simplify the model:

type Validator<'a, 'b> = string -> 'a -> Result<'b, ValidationErrors>

Now, we can define functions (if both succeed, continue):

val chainPickLeft: Validator<'a, 'b> -> Validator<'b, 'c> -> Validator<'a, 'b>
val chainPickRight: Validator<'a, 'b> -> Validator<'b, 'c> -> Validator<'a, 'c>
val chainPickBoth: Validator<'a, 'b> -> Validator<'b, 'c> -> Validator<'a * 'b, c>

Now it makes more sense to define the operators to mimic normal function composition, i.e. >>:

let (>>.) = chainPickRight
let (.>>) = chainPickLeft
let (.>>.) = chainPickBoth

This is inspired by the FParsec link I mentioned earlier.. Likewise, you could add a choice-validator, if you so wish (pick the first if it succeeds, and ignore the second. If the first fails, validate the second, if true, the whole is true, and throw away the first fail, unless they both fail, in which case you can keep the first or both):

val choice: Validator<'a, 'b> -> Validator<'a, 'b> -> Validator<'a, 'b>
let (<|>) = choice

Now the example becomes:

 Validators.Default.String.notEmpty 
 >>. Validators.Default.String.pattern "^\S" 
 >>. Validators.Default.String.pattern "\S$"

Other example:

Validators.Default.pattern "foo"     // returns DU.Foo
<|> Validators.Default.pattern "bar"  // OR returns DU.Bar
<|> Validators.Default.pattern "zed"  // OR returns DU.Zed

Other example:

// This will return the integer value
validateStringIsNumber  // string -> int
.>> validateNumberIsPositive  // int -> int
.>> validateNumberIsForAdult  // int -> Adult of int

// This will return the DU.IsAdult type 
validateStringIsNumber  // string -> int
>>. validateNumberIsPositive  // int -> int
>>. validateNumberIsForAdult  // int -> DU.IsAdult

// This will return both as a tuple, i.e. (42, IsAdult)
validateStringIsNumber  // string -> int
>>. validateNumberIsPositive  // int -> int
.>>. validateNumberIsForAdult  // int -> Adult of int

I think the current version for a single chain is too restrictive. It's an excellent idea, but we need to consider that this is a library used by a wider public, and fixing chain now for only one of the combinations is probably not a good idea, we should allow flexibility by users. Also, this pattern would simplify the modeling of the types you're dealing with when chaining validations with map & bind.

I don't know enough about Validus to know whether this makes any sense, I got this from a few discussions and I should probably try it out myself to give a more informed opinion.

pimbrouwers commented 2 years ago

Thanks for all of this great feedback @abelbraaksma. Really appreciate it, especially considering your lack of experience, I felt it was very well informed. I've enjoyed this general discourse around the shape of the library.

I have some thoughts.

My guiding principle when creating libraries is to create something that is easily understood. I believe fundamentally that with F# and ML in general, you can do a lot with a little, and that holds especially true for FP knowledge.

When I first created this, I was reluctant to add symbology (operators) at all, for this very reason. I can sense the topic developing an undercurrent, which gives me pause. Additionally, I see the operator side of this as only a small value add. In practice, at least I've found across several large projects, you most often use either one of the built-in primitive checks or a bespoke validator using Validator.create.

So, I have a radical thought. We ditch them altogether. If we have a desire to support those looking to combine these functions, we favor some new structure(s) and plain "English" named functions.

Idea 1

A fluent builder. I am not married to ValidationBuilder as a name. But I do very much like this approach as I believe it is immediately understood.

type ValidatorBuilder<'a>(validator : Validator<'a>) =
    member x.andCheck(v : Validator<'a>) = ValidatorBuilder(Validator.merge validator v)
    member x.thenCheck(v : Validator<'a>) = ValidatorBuilder(Validator.compose validator v)
    member _.build() = validator

let customValidator : Validator<int> =
    ValidatorBuilder(Check.Int.greaterThan 0)
        .andCheck(Check.Int.lessThan 100)
        .thenCheck(Check.Int.notEquals 90)
        .build()

Idea 2

A discriminated union, with a runner. Not as big of a fan of this one, but it is also easily understood.

type ValidatorOps<'a> =
    | ThenCheck of Validator<'a>
    | AndCheck of Validator<'a>

module Validator =
    let combine (head : Validator<'a>) (ops : ValidatorOps<'a> list) : Validator<'a> =
        List.fold (fun validator op ->
            match op with
            | ThenCheck v -> Validator.compose validator v
            | AndCheck v -> Validator.merge validator v) head ops

let customValidator : Validator<int> = 
    Validator.combine (Check.Int.greaterThan 0) [
        andCheck (Check.Int.lessThan 100)
        thenCheck (Check.Int.notEquals 90) ]
abelbraaksma commented 2 years ago

I think it is a good idea to add a fluent interface, many people like the "ease of discovery" of that, plus it makes it simpler to use with C# (add CompiledName attribute for the PascalCase version of the names if you want to go that route).

I took a moment to work with your library, just to understand it a little better. It's a great, yet simple-to-use library and I like it. Just to bring home my previous point, I wanted to implement the combinators.

And yes, I do understand if you "want to ditch the operators altogether". Though, to be fair, you already hide them in Operators, which is what other libs do as well, so people can opt-in to it.

They're also trivial to build locally, of course. Here's what I wanted to suggest (which may have been clear already, but anyway):

#r "nuget: Validus, 3.0.1"

open System
open Validus
open Validus.Operators

type AgeGroup = Adult | Child | Senior

//type ValidationErrors = string list

type Validator<'a, 'r> = string -> 'a -> Result<'r, ValidationErrors>
let map (f: Validator<'a, 'b>) (g: 'b -> 'c): Validator<'a, 'c> = fun a b -> Result.map g (f a b)
let bind (f: Validator<'a, 'b>) g: Validator<'a, 'b> = fun a b -> Result.bind g (f a b)
let compose (v1: Validator<'a, 'a>) (v2: Validator<'a, 'a>): Validator<'a, 'a> = Validator.compose v1 v2
let kleisli (v1: Validator<'a, 'b>) (v2: Validator<'b, 'c>): Validator<'a, 'c> = fun x y -> Result.bind (v2 x) (v1 x y)
let pickLeft (v1: Validator<'a, 'b>) (v2: Validator<'b, 'c>): Validator<'a, 'b> =
    fun x y ->
        match v1 x y with
        | Ok v -> 
            match v2 x v with
            | Ok _ -> Ok v
            | Error e -> Error e
        | Error e -> Error e

// pickRight behaves the same as Kleisli for validators
let pickRight (v1: Validator<'a, 'b>) (v2: Validator<'b, 'c>): Validator<'a, 'c> =
    fun x y ->
        match v1 x y with
        | Ok v -> 
            match v2 x v with
            | Ok w -> Ok w
            | Error e -> Error e
        | Error e -> Error e

// pickRight behaves the same as Kleisli for validators
let pickBoth (v1: Validator<'a, 'b>) (v2: Validator<'b, 'c>): Validator<'a, 'b * 'c> =
    fun x y ->
        match v1 x y with
        | Ok v -> 
            match v2 x v with
            | Ok w -> Ok(v, w)
            | Error e -> Error e
        | Error e -> Error e

let (<!>) f g = map f g
let (>>=) f g = bind f g
let (<>=>) v1 v2 = compose v1 v2
let (>=>) v1 v2 = kleisli v1 v2
let (.>>) v1 v2 = pickLeft v1 v2
let (>>.) v1 v2 = pickLeft v1 v2
let (.>>.) v1 v2 = pickBoth v1 v2

Gives:

val (<!>) : f: Validator<'a,'b> -> g: ('b -> 'c) -> Validator<'a,'c>
val (>>=) : f: Validator<'a,'b> -> g: ('b -> Result<'b,ValidationErrors>) -> Validator<'a,'b>
val (<>=>) : v1: Validator<'a,'a> -> v2: Validator<'a,'a> -> Validator<'a,'a>
val (>=>) : v1: Validator<'a,'b> -> v2: Validator<'b,'c> -> Validator<'a,'c>
val (.>>) : v1: Validator<'a,'b> -> v2: Validator<'b,'c> -> Validator<'a,'b>
val (>>.) : v1: Validator<'a,'b> -> v2: Validator<'b,'c> -> Validator<'a,'b>
val (.>>.) : v1: Validator<'a,'b> -> v2: Validator<'b,'c> -> Validator<'a,('b * 'c)>

Turns out that in practice, these are very powerful builders. I've written out a few of them, as too terse language may be hard to understand. Btw, you can ignore <>=>, I don't know what to choose for compose, as it is not monadic in any way. Maybe just <+> again...

But that discussion is moot if we're gonna ditch the operators. I'm totally OK with that. Plus the above snippet may help others. You may even add it as an fsx file and link it from the readme.md to guide people that like composable operators.

abelbraaksma commented 2 years ago

Forgot to add the code for the choice operator <|>, which is extremely useful when dealing with DUs:

// picks the left and stops, or if that fails, picks the right. If both fail, combines the errors
let choice (v1: Validator<'a, 'b>) (v2: Validator<'a, 'b>): Validator<'a, 'b> =
    fun x y ->
        match v1 x y with
        | Ok v -> Ok v
        | Error e1 -> 
            match v2 x y with
            | Ok v -> Ok v
            | Error e2 -> 
                ValidationErrors.merge e1 e2
                |> Error

let (<|>) v1 v2 = choice v1 v2
abelbraaksma commented 2 years ago

To see that in action, it looks like this:

type AgeGroup = Adult of int | Child | Senior
module ValidateStuff = 
    // let's try with these operators to get rid of the parentheses:
    let ( *|* ) f g = map f g
    let ( *| ) f x = map f (fun _ -> x)

    let ageValidator = 
        Check.Int.between 0 18          *| Child
        <|> Check.Int.greaterThan 65    *| Senior
        <|> Check.Int.between 18 65     *|* Adult
        <|> Check.Int.between 18 65     *|* fun x -> Adult x
        <=< Check.String.pattern @"\d+" *|* Int32.Parse
pimbrouwers commented 2 years ago

Nice! Looks forward to seeing your PR for the Operators module!

xperiandri commented 2 years ago

Wow! Looks amazing!

pimbrouwers commented 2 years ago

Addressed in #20. Thank you both for everything.