pimbrouwers / Falco

A toolkit for building fast and functional-first web applications using F#.
https://www.falcoframework.com
Apache License 2.0
530 stars 38 forks source link

Cannot parse 0 or 1 as numeric values from URL query #129

Open JulesNP opened 2 days ago

JulesNP commented 2 days ago

Using RequestData to extract an integer or other numeric value from a query parameter currently fails if the query value is set to 0 or 1.

Example to reproduce:

open Falco
open Falco.Routing
open Microsoft.AspNetCore.Builder

let app = WebApplication.Create()

let endpoints = [
    get "/" (fun ctx ->
        let q = Request.getQuery ctx
        let num = q.TryGetInt "test"
        Response.ofPlainText $"%A{num}" ctx)
]

app.UseFalco(endpoints).Run()

Using the query parameter ?test=2 correctly responds with Some 2, but using ?test=1 incorrectly shows None as the response.

pimbrouwers commented 2 days ago

Hi JulesNp,

Thanks for reaching out, I appreciate the detailed issue.

What a strange quirk! This took some fiddling around, but I got to the root of it. Basically distilled down to being overly eager in what was considered a boolean (i.e., "yes", "no", "1", "0" etc.). Ultimately this lead me to discover there was a bug in the test verifying this behaviour.

Anyway, thank you for the feedback. I've made some tweaks, added a new test and pushed a new beta version.

JulesNP commented 2 days ago

Thanks for the quick update!

It looks like there's still a few other edge cases related to the boolean parsing through, as I've discovered some additional quirks when reading a query as a string type:

open Falco
open Falco.Routing
open Microsoft.AspNetCore.Builder

let app = WebApplication.Create()

let endpoints = [
    get "/" (fun ctx ->
        let q = Request.getQuery ctx
        let text = q.TryGetString "test"
        Response.ofPlainText $"%A{text}" ctx)
]

app.UseFalco(endpoints).Run()

Passing ?test=on returns Some "true" instead of Some "on".

pimbrouwers commented 1 day ago

Thanks Jules, I appreciate the additional investigation.

I added some additional tests for boolean parsing, they should have been there sooner really. I was trying to be DRY and share logic between RequestValue (the parsing phase) and RequestData (the consumption phase). But my thinking on this has changed slightly since I wrote the original code. This situation has shown me that we need to be conservative in our type assumptions during parsing, and generous during reading (i.e., when the consumer is telling you exactly what they believe the value should be).

I think I've covered everything in those latest changes. I feel silly I didn't catch these the first time around. So thank you for providing additional eyes on this.

I just pushed 5.0.0-beta4 which should address the remaining edge cases. Let me know what you find!