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

Request: bind single-case unions or use json parsing for bindQuery #518

Open
reinux opened this issue Jul 10, 2022 · 2 comments
Open

Request: bind single-case unions or use json parsing for bindQuery #518

reinux opened this issue Jul 10, 2022 · 2 comments

Comments

@reinux
Copy link

reinux commented Jul 10, 2022

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.)

@Banashek
Copy link
Contributor

Created a sample and debugged into it to see what's going on.
It looks like in the union case, the parser is attempted to find the raw value in the list of union names.

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:

image

The highlighted line 121 looks to be doing an Array.tryFind over the cases (viewable in the bottom right debugger variables pane), but instead of trying to match the Union name "UserId", it's attempting to match against the raw value of 123


Thoughts

After 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: ?size=small

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.

@Banashek
Copy link
Contributor

Banashek commented Jul 10, 2022

Just more notes on looking into this:

image

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 UnionCaseInfo.DeclaredFields[0] or something like UnionCaseInfo.GetFields ref

image

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.

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

No branches or pull requests

2 participants