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

Support query string binding for Option<'T> #51

Closed
diegobfernandez opened this issue May 23, 2017 · 5 comments
Closed

Support query string binding for Option<'T> #51

diegobfernandez opened this issue May 23, 2017 · 5 comments
Labels
bug Reproducible bug

Comments

@diegobfernandez
Copy link
Contributor

When binding from a query string to a record with a field of type int option this exception happens.
Haven't try with form/json/xml binding.

System.AggregateException: One or more errors occurred. ---> System.NotSupportedException: TypeConverter cannot convert from System.String.
   at System.ComponentModel.TypeConverter.GetConvertFromException(Object value)
   at System.ComponentModel.TypeConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value)
   at Giraffe.HttpContextExtensions.HttpContext-BindQueryString@86-1.Invoke(PropertyInfo p)
   at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc`2 action, IEnumerable`1 source)
   at Giraffe.HttpContextExtensions.HttpContext-BindQueryString@82.Invoke(Unit unitVar)
   at [email protected](AsyncParams`1 args)
   --- End of inner exception stack trace ---
---> (Inner Exception #0) System.NotSupportedException: TypeConverter cannot convert from System.String.
   at System.ComponentModel.TypeConverter.GetConvertFromException(Object value)
   at System.ComponentModel.TypeConverter.ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, Object value)
   at Giraffe.HttpContextExtensions.HttpContext-BindQueryString@86-1.Invoke(PropertyInfo p)
   at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc`2 action, IEnumerable`1 source)
   at Giraffe.HttpContextExtensions.HttpContext-BindQueryString@82.Invoke(Unit unitVar)
   at [email protected](AsyncParams`1 args)<---
@nojaf
Copy link
Contributor

nojaf commented May 23, 2017

This is actually a Json.NET thing, see https://gist.github.com/isaacabraham/ba679f285bfd15d2f53e.
Might be interesting to include these extra converters by default in Giraffe.
@dustinmoris thoughts?

@diegobfernandez diegobfernandez changed the title Support model binding for Option<'T> Support query string binding for Option<'T> May 23, 2017
@diegobfernandez
Copy link
Contributor Author

I meant Query String binding :)
This is the model I'm trying to bind to:

[<CLIMutable>]
type Query =
    { pattern: string
      page: int
      perPage: int
      userId: string
      dataViewId: int option }

and the code to do the binding:

let! query = ctx.BindQueryString<Query>()

That line gives me the exception.
I can give it a try and submit a PR if it seems doable.

I don't currently use JSON payload on my requests, althoug that link provide a lot of insights for future needs, I think for now it should be mentioned on docs and after people feedback may be enabled by default.

@nojaf
Copy link
Contributor

nojaf commented May 23, 2017

Oh, my bad.

@dustinmoris
Copy link
Member

Hey guys, yes I think we could add a case for options to the binding of the query string.

@dustinmoris dustinmoris added the bug Reproducible bug label May 30, 2017
@diegobfernandez
Copy link
Contributor Author

I'll do a PR for this soon, just finishing the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Reproducible bug
Projects
None yet
Development

No branches or pull requests

3 participants