-
Notifications
You must be signed in to change notification settings - Fork 266
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
Request: bind single-case unions or use json parsing for bindQuery #518
Comments
Created a sample and debugged into it to see what's going on. Test Sample code:type UserId = UserId of int
// Uses Type alias
[<CLIMutable>]
type TestUser =
{ Id : UserId
Name : string }
// Doesn't use type alias
[<CLIMutable>]
type NamedWithId =
{ Id : int
Name : string }
let testUserToString (u : TestUser) =
printfn $"%A{u}"
$"{u.Name} has an Id of {string u.Id}"
let americanEnglish = CultureInfo.CreateSpecificCulture("en-US")
// Handler for type alias user
let bindAndPrintUser =
tryBindQuery<TestUser>
RequestErrors.BAD_REQUEST
(Some americanEnglish)
(fun tu -> Successful.OK <| testUserToString tu)
// Handler for non-type alias model
let bindAndPrintNamed : HttpHandler =
tryBindQuery<NamedWithId>
RequestErrors.BAD_REQUEST
(Some americanEnglish)
(fun ni -> Successful.OK <| $"Non-typed id: {ni.Id} with name {ni.Name}")
let endpoints : Endpoint list =
[
GET [
route "/ping" (text "pong")
route "/" (htmlView <| Views.index { Text = "foo" })
route "/typeduser" bindAndPrintUser
route "/untypednamed" bindAndPrintNamed ]]
Input prompting execution:➜ curl "http://localhost:5000/typeduser?id=123&name=foo"
"The value '123' is not a valid case for type GiraffePlayground.App+UserId." ➜ Debugging:The highlighted line ThoughtsAfter looking at this, it appears that perhaps there could be a branching point added for single-case unions that uses slightly different logic. My assumption is that the current union logic is looking for something akin to the modelbinding tests where it can match something like the following: Definition: type Size =
| Big
| Small QueryString: relying completely on the string value of the union case. I'm not sure how it can be done, but I believe that the fix would be to add in substitution of type abbreviations for their existing-type counterparts during the validation here, and additionally bind appropriately. I haven't done too much with type abbreviations or reflection in general, so happy to hear other thoughts. |
Just more notes on looking into this: For some reason my brain mistakenly thought this was a case of type abbreviations when really it's a single case union. It does appear that the value is still accessible via the fields info either with Seems like it might be a way to map these types into their underlying representations. I don't want to go implementing anything since even though I've done a bit of work with F#, I've successfully stayed away from reflection for quite a while. Happy to learn and contribute however. |
I'm using single-case unions extensively to constrain ID types in my backend, e.g.
type UserId = UserId of int
.It would be really cool if
bindQuery
could recognize this and automatically wrap single-case unions of primitive types so that we don't need to make an additional type for the query binding which is the same thing just with raw types.(In the strictest sense, it does make sense to decouple the HTTP API from the internal representation, but I find that it makes sense to cut down on bureaucracy for simple CRUD endpoints.)
The text was updated successfully, but these errors were encountered: