Integers are overflowed silently

[arp242]

Right now this library silently overflows integers. For example, filling in 300 for an uint8 results in 44, since the maximum size is 256 and 300 - 256 = 44.

Personally I think this behaviour is rather surprising, and would expect an error from this library.

Thoughts?

[arp242]

Thinking about this some more, handling it in formam is the only feasible way of adding a check for this at all as far as I can tell. The only other way is writing form parsing code to check integer values before formam decodes it.

[emilgpa]

I haven’t not much time to create a sample right now. do you have a sample to reproduce? Definitely, Formam should returns to error (or panic if it's is not possible)

[arp242]

Here's a simple test case:

diff --git i/formam_test.go w/formam_test.go
index 470acfa..f2657c9 100644
--- i/formam_test.go
+++ w/formam_test.go
@@ -870,3 +870,22 @@ func TestPanic(t *testing.T) {
        }

 }
+
+// #25
+func TestOverflow(t *testing.T) {
+       s := struct {
+               N uint8
+       }{}
+
+       vals := url.Values{
+               "N": []string{"300"},
+       }
+
+       dec := formam.NewDecoder(&formam.DecoderOptions{})
+       err := dec.Decode(vals, &s)
+       if err != nil {
+               t.Error(err)
+       }
+
+       t.Fatalf("s.N is %d", s.N) // 44
+}

Output:

[~/code/formam](master)% go test
--- FAIL: TestOverflow (0.00s)
    formam_test.go:890: s.N is 44
FAIL

At first I thought the user could detect this, so I created an issue to discus possible options first, but then I realized it's not so easy and that returning an error is really the only sane thing to do (as far as I can tell?)

I'll take a look at fixing it later, after the errcode PR is done (otherwise it'll conflict, which means more work).

[emilgpa]

Ok, thanks you. I will see some possible solution later too.

[emilgpa]

Here there is a possible solution: https://stackoverflow.com/a/26275382 using OverflowInt and OverflowUint methods. I hope implement it when have free time when I arrive at home.

[arp242]

We can pass dec.curr.Type().Bits() to strconv.ParseInt(), I already made a patch for that yesterday: https://github.com/arp242/formam/commit/a8074a77eaafc37c58fbe9fecc8c5782722241a1#diff-5435d3638a3d29bff71400c6c11bff1aR425

The problem I kind of ran in to is that:

1. Creating nice-looking errors is rather hard (general problem, not strictly related to this PR)

2. Ideally I'd like to add a OverflowSetMax option which sets it to the maximum value (e.g. 255 on 300) and returns an error, but that's not something that's easy to do either. Returning an error aborts all parsing.

   The use case for this is the ability to display a "warning: value for Foo is truncated to 255", rather than silently setting it to the max value or displaying a hard error.

I need to think about how to best fix those.

[emilgpa]

Why you see bad aborts all parsing? I think that would be the right case and leave it to the developer the solution (in your example, the developer should to change int8 to int32).

[arp242]

The use case(s) I'm thinking of are user errors, rather than developer errors.

For example, a user can fill in 9999 in a form which gets parsed to uint8. The bit size is correct here, as this value never needs to be higher than 255. It's the user who just provided a value that's out of range.

Right now, using:

err := formam.Decode(r.Form, &args)
if err != nil {
    return err
}

Results in a rather ugly error:

formam: field=N; path=N: could not parse number: strconv.ParseUint: parsing "300": value out of range

You can improve on that by type asserting the err, getting the code and other information, and formatting a new error string. But it's rather cumbersome, and essentially every user of formam will have to write a helper function to present nice errors for their users.

The same applies to some other user errors, such as filling in asd instead of a number:

formam: field=N; path=N: could not parse number: strconv.ParseUint: parsing "asd": invalid syntax

[emilgpa]

@arp242 Finally, you will fix this error with your branch? It's possible that you haven't time. If it is, don't worry, I could solve it this weekend.

[arp242]

Yeah, I just wanted to also do the errors, but don't have time for that now. I made a PR to fix the overflow.

[emilgpa]

ok, perfect. Thanks you, @arp242