Parsing JSON in a request body with Go

gistfile1.go

package main

import (
	"encoding/json"
	"io/ioutil"
	"log"
	"net/http"
)

type Message struct {
	Id   int64  `json:"id"`
	Name string `json:"name"`
}

// curl localhost:8000 -d '{"name":"Hello"}'
func Cleaner(w http.ResponseWriter, r *http.Request) {
	// Read body
	b, err := ioutil.ReadAll(r.Body)
	defer r.Body.Close()
	if err != nil {
		http.Error(w, err.Error(), 500)
		return
	}

	// Unmarshal
	var msg Message
	err = json.Unmarshal(b, &msg)
	if err != nil {
		http.Error(w, err.Error(), 500)
		return
	}

	output, err := json.Marshal(msg)
	if err != nil {
		http.Error(w, err.Error(), 500)
		return
	}
	w.Header().Set("content-type", "application/json")
	w.Write(output)
}

func main() {
	http.HandleFunc("/", Cleaner)
	address := ":8000"
	log.Println("Starting server on address", address)
	err := http.ListenAndServe(address, nil)
	if err != nil {
		panic(err)
	}
}

use decoder instead of unmarshal

func main() {
    http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
        var u User
        if r.Body == nil {
            http.Error(w, "Please send a request body", 400)
            return
        }
        err := json.NewDecoder(r.Body).Decode(&u)
        if err != nil {
            http.Error(w, err.Error(), 400)
            return
        }
        fmt.Println(u.Id)
    })
    log.Fatal(http.ListenAndServe(":8080", nil))
}

Yes you are right @ikrauchanka

thanks @ikrauchanka & @aodin

Actually, I think you are wrong @ikrauchanka https://ahmet.im/blog/golang-json-decoder-pitfalls/

@kwiesmueller: If you look at that blog post you linked, it mentions that the issue in question was addressed in Go 1.7.

If you use decoder, the server won't care if you are sending invalid json strings that don't match your interface

@eoconnor I read the blog post: the third issue was fixed, but the first 2 points in the blog post still seem valid.

Also, unrelated, but r.Body.Close() might return an error but it's ignored in the gist.

Thx!!! This helped me a lot~😊

So helpful!

As @tzachshabtay pointed out, there's an unhandled error for the deferred r.Body.Close(). This can be fixed by replacing the defer on line 19 with:

defer func() {
    err := r.Body.Close()

    if err != nil {
        http.Error(w, err.Error(), 500)
        return
    }
}()

For those of you who are worried about the "missing" error handling of defer r.Body.Close(): the actual string or byte buffer of the inbound request body is wrapped in a nopCloser, which has a Close method that always returns nil: https://golang.org/src/io/ioutil/ioutil.go#L110

The wrapping occurs in the following type switch: https://golang.org/src/net/http/request.go#L874

So this error can be safely ignored.

If I am incorrect, and you have code that produces an error from this method, please let me know and I will edit this comment.

But you should also tell the Go team because the standard library ignores this error everywhere as well:
https://golang.org/src/net/http/request.go#L1388
https://golang.org/src/net/http/response.go#L337

@aodin Ah, good point. It's probably fine as-is then. 👍 Ignore me!

@aodin @aaron-goulet-bfg I would argue that it's still wrong. You're not supposed to rely on the inner implementation, you're supposed to rely on the contract, and the contract says there could be an error. The inner implementation can change at any time to return an error and it wouldn't be a breaking change as it complies with the contract so it can happen even in a minor version. If that happens, it can break your code in production.

The fact the standard library is doing it too, is, imo, very unfortunate and sets a bad example to the community.

I understand your point, tzachshabtay, but I would add that the "contract" you're interpreting from the type signature is far less concrete than you'd expect. Take this classic error with net/http:

resp, err := http.Get(url)
defer resp.Body.Close()
if err != nil {
    return err
}

This code will compile, and if url is valid, will run without error. But if url is invalid, the code will panic with a runtime error because resp is nil.

What's the fix? Knowing that resp may be nil, the defensive programmer would likely check if it is nil before writing calling defer.

But that's not the best practice established by the standard library, which instead says to check the error before using the response, and if there is an error, the body's Close method does not need to be called. This is even true for a single case where both resp and err can return non-nil!

Thankfully, go vet will catch any instance of the response being used before the error is checked. But it only cares if you checked for the error, not if you successfully handled it! So go vet won't care if you continue to use the nil response post-check and cause a panic once again.

Knowing this, you could write super defensive code that checks for all possible nil returns, and it wouldn't be "wrong", but it would add plenty of "unnecessary" code that diverges from the standard library. And then during a code review another programmer will take issue with the amount of non-standard defensive coding, and you'll respond with a concern about "future-proofing", and then your office has erupted in a civil war, and you would never had guessed that a single defer line was your Fort Sumter.

In brief, there's no substitute for knowing the codebase, but the Go team could do two things to make this particular issue better:

1. Specifically declare in godocs that the error returned from response.Body.Close() can be ignored
2. And if 1) ever changes, create a go vet rule that says to check this error

Nice work man. really appreciate it

Thank you! Great discussion here everyone.

@aodin , thank you for the original post, and great writing for your response to handling the defer resp.Body.Close(). I found it to be enlightening in more ways that just an analysis of Go.

@ikrauchanka , thank you for noting to use decoder instead of unmarshal.

@tzachshabtay , thank you for being honest and sticking to your guns about your stance on the un-handled errors. Without that then there wouldn't have been as many chances to learn.

Have a great day peoples! :)