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

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! :)