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

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