binding.Field.Required=true is ignored on JSON input

[mark-kubacki]

binding.Field.Required is ignored if the corresponding type implements the Binder interface. Bind is not run for absent input either.

Example:

type EmailAddress string

func (e *EmailAddress) Bind(strVals []string, errs binding.Errors) binding.Errors {
    // [snip]
    *e = EmailAddress(strVals[0])
    return errs
}

type SignupForm struct {
    Email       EmailAddress
    InviteCode1 string
}

func (cf *SignupForm) FieldMap() binding.FieldMap {
    return binding.FieldMap{
        &cf.Email: binding.Field{
            Form:     "email",
            Required: true,
        },
        &cf.InviteCode1: binding.Field{
            Form:     "invite-code-1",
            Required: true,
        },
    }
}

// ----
func signupHandler(resp http.ResponseWriter, req *http.Request) {
    myForm := new(SignupForm)
    errs := binding.Bind(req, myForm)
    if errs.Handle(resp) {
        return
    }
}

Test:

curl --data "message=Yadda" http://127.0.0.1/signup

Expected:

[{"fieldNames":["email"],"classification":"RequiredError","message":"Required"},
{"fieldNames":["invite-code-1"],"classification":"RequiredError","message":"Required"}]

Actual result:

[{"fieldNames":["invite-code-1"],"classification":"RequiredError","message":"Required"}]

[mholt]

Your fix for this is awesome and elegant. I want to keep it, but I think we just broke the enforcement of required fields for JSON payloads (see my comment in the associated PR).

I'm going to to try to find time to actually write some tests to prevent this in the future, but I wonder if there's an equally elegant way to enforce required fields with JSON deserialization.

[mark-kubacki]

Go's convention for JSON is using structs with pointers. If they are, after unmarshaling, nil then they are missing from JSON input.

type ContactForm struct {
    User struct {
        ID *int
    }
    Email   *string
    Message *string
}

Although that has to be done once, I don't know how much of a chore it would be copying structs provided by the user and switching to pointers in the process. Or if doing that at all is a good idea, because it works without pointers as you have clearly demonstrated for form-urlencoded data.

And there is json.NewDecoder, which decodes into map[string]interface{} instead of (see bindForm) map[string][]string. If we united the latter data structures we would obtain reflection-free JSON decoding — which would be great.

[mark-kubacki]

I have been wrong: json.NewDecoder utilizes reflection.

My thinking is that we don't need to decode the entire nested JSON input. We just need enough to fill our given struct. And if required fields are missing, we don't have to do even that.

Here's what I currently play with:

package main

import (
        "bytes"
        "encoding/json"
        "fmt"
        "os"
)

func main() {
        dec := json.NewDecoder(os.Stdin)
        p, _ := flatDecode(dec)
        for k := range p {
                fmt.Println(k)
        }
}

func flatDecode(dec *json.Decoder) (map[string]json.RawMessage, error) {
        var v map[string]json.RawMessage
        if err := dec.Decode(&v); err != nil {
                return v, err
        }
        keysToDelete := make([]string, 0, len(v))
        for k := range v {
                if v[k][0] == '{' {
                        keysToDelete = append(keysToDelete, k)
                        subStruct, err := flatDecode(json.NewDecoder(bytes.NewReader(v[k])))
                        if err != nil { // cannot happen anymore
                                return v, err
                        }
                        for sk := range subStruct {
                                v[k+"."+sk] = subStruct[sk]
                        }
                }
        }
        for _, k := range keysToDelete {
                delete(v, k)
        }
        return v, nil
}

echo "package main" >> utils.go
grep -A 120 -F 'func unquoteBytes' /usr/lib/go/src/pkg/encoding/json/decode.go >> utils.go
goimports -w=true utils.go

echo '{"autor": {"name": "mark", "id": 4}, "alter": 12}' | go run json-decode-test.go

alter
autor.name
autor.id

json.RawMessage is just []byte, which we can pass to json.Unmarshal or, even faster, to unquoteBytes if needed.

[mark-kubacki]

Matt, I am waiting for your feedback here. If you agree that pipeing JSON input as map[string][]string through bindForm, and by that consolidating processing of form-urlencoded as well as JSON, is the way to go I will implement that for binding tomorrow.

[mholt]

Yeah, I know, sorry, I'm traveling all day. I'll give it a detailed look soon though.

[mholt]

Hey @wmark, I finally got a chance to give it a shot. I like where this is going! How hard would it be to implement? Do you have a plan or would you like me to take a stab at it?

I'm unclear as to whether json struct tags could still be used; my gut is telling me no. Is that correct?

One more thing: what do you think about a way to toggle which method of JSON deserialization is used? In other words, if users wanted to use the standard JSON deserializer like we had before, they can (but it won't check for required fields), otherwise it defaults to this custom one (which would enforce required fields).

[mark-kubacki]

Welcome back @mholt — I can implement the already outlined concept within an hour or two.

If a developer wants de-serialization (unmarshaling) of a form f the Go way he/she can already call json.Unmarshal() and f.Validate().

And you're right, we won't use json struct tags. Golang or ffjson will need them for marshaling, though.

---

Golang needs tag "required" for unmarshaling!

[mark-kubacki]

Done. Please see

• mark-kubacki@66f260c
• mark-kubacki@906ffb8

I'd love your input on both. I will cherry-pick the changes you like to branch for-mholt as usual. Tests would be awesome — I didn't write any in Go because calls to binding are already covered by my integration tests written in Python.

curl and JSON:

curl -v -H "Content-Type: application/json" -d '{"email":"w@h.de", "invite-code-1":"a"}' http://127.0.0.1:3000/signup

[mholt]

Awesome!

I just got back from a camping trip and have barely had a chance to look these over.

You'll notice I commented on the first one; definitely a great improvement. The second one is also exciting; but it's a little scary since it uses unexported code from the standard lib. You're right about tests; I started this package as an experiment and I didn't think about tests until I started to see it working. (I'll get around to them eventually.)

The recursive decoding is a nice touch. Relatively simple and adds a lot more power.

Anyway, I haven't had a chance to try either commit yet. For now, I trust your integration tests and curl commands. I'll work on revising the README related to JSON deserialization, and try the changes myself. In the mean time, if there's anything else you want to do to clean it up for a pull request, you can do that now to prepare for it.

[mark-kubacki]

I have scheduled the cleanup for tomorrow morning CEST.