Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functional binding of querystrings #121

Closed
Alxandr opened this issue Oct 30, 2017 · 20 comments
Closed

Functional binding of querystrings #121

Alxandr opened this issue Oct 30, 2017 · 20 comments
Labels
feature request Request to add new functionality in progress Issue is already being adressed as part of ongoing work
Milestone

Comments

@Alxandr
Copy link

Alxandr commented Oct 30, 2017

I needed some querystring data in a small app I made to try out Giraffe, and the ctx.BindQuery wasn't doing it for me; so I made a functional query string binder based on how Chiron works which allows me to do the following:

type Report =
  { author: string option
    project: string option
    week: int option
    summary: string option
    progress: string list
    comments: string list
    plan: string list }
  
  static member FromQuery (_: Report) =
        fun author project week summary progress comments plan ->
          { author = author
            project = project
            week = week
            summary = summary
            progress = progress
            comments = comments
            plan = plan }
    <!> Query.read "author"
    <*> Query.read "project"
    <*> Query.read "week"
    <*> Query.read "summary"
    <*> Query.read "progress"
    <*> Query.read "comments"
    <*> Query.read "plan"

let reportRoute = route "/report" >=> Query.bind (fun (r: Report) -> text <| sprintf "%A" r)

I was thinking this might be something you'd want to incorporate into Giraffe? Anyways, if people want it, the code is available here: https://gist.github.com/Alxandr/50aef7fbe4806ceb4c2889f1cbde1438

Feel free to use it however you'd like :)

@dustinmoris
Copy link
Member

Hey, that looks quite cool. I'll have a closer look at the code, but I can see how it's useful and think about a possible integration!

@dustinmoris dustinmoris added feature request Request to add new functionality requires review Issue has a potential PR which might resolve it, but requires review labels Nov 2, 2017
@whitebear-gh
Copy link
Contributor

Hi @Alxandr ,

I was trying to use your snippet, but I have some problem after copy/paste your code:
image

Any suggestions?

@Alxandr
Copy link
Author

Alxandr commented Dec 28, 2017

As far as I can see, your issue is indentation. The operators needs to be indented more than static, but less than the factory function.

[Edit]
On a side note, I use 2 space indentation. So it might get messed up if you just copy-paste and have your editor reindent it.

@whitebear-gh
Copy link
Contributor

Thanks for quick replay, unfortunately it's not because of indentation (sorry for misleading screen - I was playing with format). With correct indentation I have the same error:
image

error FS0001: Expecting a type supporting the operator '<!>' but given a function type. You may be missing an argument to a function.

@jonashw
Copy link

jonashw commented Dec 28, 2017

@Alxandr I have discovered that ctx.BindQuery isn't "doing it for me" either. The problems for me where these:

  • reference-type model properties not present in the querystring would end up with null values on my bound model instance
  • value-type model properties not present in the querystring would end up with default values (ie. 0 for int)
  • lack of List / Set support

It appears that @Alxandr 's Query.bind remedies most of my issues. I could be wrong but from the look of it, without a total parsing "match" (ie. all non-option properties are present in the querystring), the handler short-circuits and stops processing the request. This is great in that it avoids ever putting the model type into an invalid state (read: nulls).

  module Query =

    let inline bind (f: ^a -> HttpHandler) : HttpHandler = 
      fun (next : HttpFunc) (ctx : HttpContext) ->
        task {
          match Query.tryParse ctx.Request.QueryString with
          | None   -> return None
          | Some a -> return! f a next ctx
        }

I also really like this approach (ie. Chiron's approach: https://github.com/xyncro/chiron) because it eliminates the need for reflection, favoring instead an explicit deserialization interface that is decoupled from the type's CLI representation.

@gerardtoconnor
Copy link
Member

This does not look nice/simple to me, you have had to state each property name 3x, one time being loosely by string that is not verified and susceptible to run time error, as well as importing the whole Chiron library and using funky operators.

Rather then bloat the base with this it might be worth trying to review underlying issues first:

the ctx.BindQuery wasn't doing it for me

@jonashw thanks for providing the following feedback, offers us a starting point:

  • reference-type model properties not present in the querystring would end up with null values on my bound model instance
    
  • value-type model properties not present in the querystring would end up with default values (ie. 0 for int)
    
  • lack of List / Set support
    

It is not too difficult to expand BindQueryString to handle Option/List better so that user simply defines the binding type and will populate as expected without need for manual FromQuery definition each time with Chiron.

Naturally users free to use Chiron approach but I would rather see BindQueryString fixed up to work for these scenarios then introduce unneeded abstraction.

@jonashw
Copy link

jonashw commented Dec 28, 2017

I've gone ahead and added support for Lists/Sets in my own custom version of BindQueryString. I have not written automated tests for it but it works in my sample app. Required a little refactoring of the existing code to accommodate multi-values.

type QueryStringValueType = 
    | OptionType of Type 
    | List of Type 
    | SetType of Type 
    | Other

type HttpContext with
    member this.TryGetQueryValue (key : string) =
        let q = this.Request.Query
        match q.TryGetValue key with
        | true, value -> 
            Some value
        | _ -> None

    member this.BindQueryStringJW<'T>(?cultureInfo : CultureInfo) =
        let obj     = Activator.CreateInstance<'T>()
        let culture = defaultArg cultureInfo CultureInfo.InvariantCulture
        let props   = obj.GetType().GetProperties(BindingFlags.Instance ||| BindingFlags.Public)
        props
        |> Seq.iter (fun p ->
            let queryValueType =
                if not (p.PropertyType.GetTypeInfo().IsGenericType)
                then Other
                else 
                    let genericType = p.PropertyType.GetGenericTypeDefinition() 
                    let genericArg = p.PropertyType.GetGenericArguments().[0]
                    if genericType = typedefof<Option<_>> 
                    then OptionType genericArg
                    elif genericType = typedefof<List<_>>
                    then List genericArg
                    elif genericType = typedefof<Set<_>>
                    then SetType genericArg
                    else Other

            let propertyType =
                let pt = 
                    match queryValueType with 
                    | Other -> p.PropertyType
                    | List t -> t
                    | OptionType t -> t
                    | SetType t -> t
                if pt.GetTypeInfo().IsValueType 
                then (typedefof<Nullable<_>>).MakeGenericType([|pt|])
                else pt

            let converter = TypeDescriptor.GetConverter propertyType

            let value = 
                match queryValueType, (this.TryGetQueryValue p.Name) with
                | Other, None -> null
                | Other, Some v -> converter.ConvertFromString(null, culture, v.ToString())
                | List _, None ->
                    FSharpValue.MakeUnion(FSharpType.GetUnionCases(p.PropertyType).[0], [||])
                | List _, Some v -> 
                    let consCase, empty = 
                        let cases = FSharpType.GetUnionCases(p.PropertyType)
                        cases.[1], FSharpValue.MakeUnion(cases.[0], [||])
                    if v.Count = 0
                    then empty
                    else Array.foldBack 
                          (fun item list -> FSharpValue.MakeUnion(consCase, [|item :> obj ; list|]))
                          (v.ToArray())
                          empty
                | SetType genericType, None -> 
                    let listType = (typedefof<List<_>>).MakeGenericType(genericType)
                    let emptyList = FSharpValue.MakeUnion(FSharpType.GetUnionCases(listType).[0], [||])
                    Activator.CreateInstance(p.PropertyType, [|emptyList|])
                | SetType _, Some v ->
                    let values = v.ToArray() |> Array.toList
                    Activator.CreateInstance(p.PropertyType, values)
                | OptionType _, None ->
                    FSharpValue.MakeUnion(FSharpType.GetUnionCases(p.PropertyType).[0], [||])
                | OptionType _, Some v ->
                    let cases = FSharpType.GetUnionCases(p.PropertyType)
                    let v = converter.ConvertFromString(null, culture, v.ToString())
                    if isNull v 
                    then FSharpValue.MakeUnion(cases.[0], [||])
                    else FSharpValue.MakeUnion(cases.[1], [|v|])
            p.SetValue(obj, value, null))
        obj

@jonashw
Copy link

jonashw commented Dec 28, 2017

Note that the above does not address the null problem though it has at least made the presence of null explicit.

let value = 
  match queryValueType, (this.TryGetQueryValue p.Name) with
  | Other, None -> null
  ...

@Alxandr
Copy link
Author

Alxandr commented Dec 29, 2017

@gerardtoconnor You don't have to like it, nor do you have to use it. I posted this here because some people might like it, and it fits well with the functional paradigm of programming web services (in my opinion). But since it was just a single file, I didn't want to make it a library that nobody ever found. But to respond to a bit of your feedback:

you have had to state each property name 3x

This is sort of true, but also not necessary. If you have a function of type string -> int -> Person, then you can easily do factoryFunc <!> Query.read "name" <*> Query.read "age". I tend to write the factories in place to make it more readable (in my opinion).

one time being loosely by string that is not verified and susceptible to run time error

This is the case for all query parsing. Even if you base your query names from property names (using reflection, or any other way), there is no strong coupling that what is sent from the client actually matches with what your model expects. You get no added safety by using reflection for this.

as well as importing the whole Chiron library

Chiron is not a dependency. You don't import it at all.

using funky operators

You don't have to use a single "funky" operator. For instance, you can write it like this instead:

type Person = 
  { name: string
    age: int }

  static member FromQuery (_: Person) =
    query {
      let! name = Query.read "name"
      let! age = Query.read "age"
      return { name = name; age = age } }

@whitebear-gh the best I can guess based on the little information I have to go off is that you're lacking open Giraffe.Query.Operators. You have to explicitly open the operators module to get the operators in scope.

@whitebear-gh
Copy link
Contributor

@Alxandr thanks a lot!
Adding open Giraffe.Query.Operators helped, and I'm looking forward to use it in my project.
Thanks for helping noob like me :)

@dustinmoris
Copy link
Member

dustinmoris commented Feb 12, 2018

So I was reading all the feedback from this issue and I am working on improving BindQueryString (and other methods such as BindFormAsync which I believe should match the behaviour of BindQueryString and routeBind from the http handlers).

I was thinking to do the following:

  • The current BindQueryString method will remain as is in order to not break existing functionality and also because it would match MVC's behaviour where a model can be bound successfully from a partial data set
  • A new method called TryBindQueryString or an overload of BindQueryString with a new argument called strictBinding (true/false) will be made available

Behaviour of new "strict" model binding of query string:

  • If a value from the model was missing in the query string then TryBindQueryString should return None
  • Unless the missing value from the model is an Option<'T> type, which would be an exception. If all non Option<'T> values could be resolved from the query string then the TryBindQueryString function will still return Some 'T and the missing option values will be set to None of course.
  • The improved binding function will also be able to bind discriminated union types*

*) For now it will be able to bind simple union types:

// Will work:
type PaymentMethod =
    | Credit
    | Debit
    | Cash

// Will not work:
type PaymentMethod =
    | Credit of CreditCard
    | Debit of DebitCard
    | Cash

I have two questions for everyone involved here:

  1. Does all of this sound like it would fix your current shortcomings of BindQueryString?
  2. How does list data look in a query string which you want BindQueryString to work with?
  3. Should a list of normal value types and string values work or also a list of options, unions etc? What would that look like, I would need an example query string which I can use for testing.

Thanks everyone for the help and patience!

@Alxandr
Copy link
Author

Alxandr commented Feb 12, 2018

I would still use my helper, because I really don't like the imperative API of BindQueryString. Also don't really like how it's reflection based. Another point to add. If you want a MVC-like BindQueryString and BindForm, you might really want to consider how you deal with missing values and validation. There is a reason MVC has ModelState and uses [<Required>]. It fits really well with that API.

@dustinmoris
Copy link
Member

dustinmoris commented Feb 12, 2018

@Alxandr Thanks for the reply! What you say regarding the ModelState this is a good point. To be honest I don't like the attribute driven validation, because these attributes don't mean anything in other parts of the code where there is no API to evaluate them. The F# way of making something required is much better, just declare a property normally and make optional properties explicit with Option<'T>. This will make the model being validated the same way in every part of an application and doesn't require a special API to check for some attributes, so maybe I'll just implement one version of BindQueryString which will behave entirely FSharpy.

Need to think a bit more about it...

I don't think I understand what you mean by "don't like the imperative API of BindQueryString" though? What is imperative about calling a function which will map a string value (= query string) to a record type? This is not much different than other F# APIs like different serializers, the FSharp.Data API, etc., right?

@Alxandr
Copy link
Author

Alxandr commented Feb 12, 2018

Hmm. Imperative might have been the wrong name. What I mainly don't like is the fact that you need to have the ctx available. With my example, you still stay in the realm of "WebParts" and functions (not using methods).

let reportRoute = 
      route "/report" 
  >=> Query.bind (fun (r: Report) -> text <| sprintf "%A" r)

@Alxandr
Copy link
Author

Alxandr commented Feb 12, 2018

Also; you can bake validation into the types themselves. The static FromQuery method can contain validation, ensuring that any values that successfully deserialize are valid.

@dustinmoris
Copy link
Member

dustinmoris commented Feb 12, 2018

Ah ok I see... yeah that is quite nice... hmm thanks for helping me understand the issue better!

Since you seem to be very responsive right now could you quickly provide me an example how a client would pass a list in a query string?

I am trying to address this as well:

lack of List / Set support

Thank you for the help so far!

@Alxandr
Copy link
Author

Alxandr commented Feb 12, 2018

There are multiple ways to pass lists using query strings. And arguments on which ones are the best 😛 . As far as I know MVC uses user[]=foo&user[]=bar to create lists/arrays. This is one of the most explicit ways (though not my personal favorite).

Ways:

user=foo&user=bar:

Advantages:

  • Clean and simple, nice URLs.

Disadvantages:

  • user=foo = list? or scalar? Note that I think explicit typing removes this issue.
  • order is not explicit, and may be lost.
  • cannot do list of lists.

user[]=foo&user[]=bar:

Advantages:

  • Explicitly lists. user[]=foo will always be a list of length 1.
  • Support for hierarchial data (see comment bellow).

Disadvantages:

  • Ugly (IMHO).
  • order is not explicit, and may be lost.

user[0]=foo&user[1]=bar:

Advantages:

  • Really friggin explicit. Nothing is left to imagination.
  • Support for hierarchial data (see comment bellow).

Disadvantages:

  • Really ugly.
  • Can have holes in indexes. How is that treated? List of options?

Hierarchical data

Using one of the explicit formats, we could actually turn structured keys into Json DU from Chiron (which might be cool, and a nice way of going about things). This would allow you to parse arbitrary hierarchical data from x-www-form-urlencoded easily and reuse the JSON parsing from Chiron.

@lambdakris
Copy link
Contributor

Something to consider regarding lists/arrays in query strings is compatibility with frontend libraries. For example, many JavaScript libraries follow the first convention users=user1&users=user2&users=user3:

@dustinmoris
Copy link
Member

Awesome, thank you for all the information!

@dustinmoris dustinmoris added in progress Issue is already being adressed as part of ongoing work and removed requires review Issue has a potential PR which might resolve it, but requires review labels Feb 13, 2018
@dustinmoris dustinmoris added this to the 1.1.0 milestone Feb 13, 2018
dustinmoris added a commit that referenced this issue Feb 14, 2018
Improved model binding which addresses several issues. Related to #206 and #121.
@dustinmoris
Copy link
Member

Hi,

I have implemented an improved model binding API based on the feedback provided here.

There is a new TryBindQueryString<'T> extension method, but also a new tryBindQuery<'T> http handler for a more functional way of binding a model from a query string. The new tryBindQuery<'T> http handler will only bind a model if all non-optional data was provided and in the correct format, otherwise it will invoke an error handler which is configurable (via currying).

The new implementation continues to use reflection as I prefer that model over having to do it manually in a member method of the record type. I've also thought about how to allow a user to specify additional business logic validation and added a new model validation API.

Overall I think this addresses all issues raised in this ticket. A run through of the new features can be found in this blog post.

Nevertheless I have added a reference to @Alxandr Chrion based implementation in the documentation (see at the bottom of this section) in case users prefer to implement their own query string binding functions.

Also in terms of binding lists from a query string I just used ASP.NET Core's default implementation, which supports both, the ?user=0&user=1 format and the ?user[]=0&user[]=1 format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request to add new functionality in progress Issue is already being adressed as part of ongoing work
Projects
None yet
Development

No branches or pull requests

6 participants