Skip to content

Instantly share code, notes, and snippets.

@aodin
Last active March 23, 2024 20:24
Show Gist options
  • Save aodin/9493190 to your computer and use it in GitHub Desktop.
Save aodin/9493190 to your computer and use it in GitHub Desktop.
Parsing JSON in a request body with 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)
}
}
@ikrauchanka
Copy link

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

@ram-rana-16
Copy link

Yes you are right @ikrauchanka

@casoetan
Copy link

thanks @ikrauchanka & @aodin

@kwiesmueller
Copy link

@eoconnor
Copy link

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

@CelesteComet
Copy link

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

@tzachshabtay
Copy link

@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.

@Lweiis
Copy link

Lweiis commented Aug 30, 2019

Thx!!! This helped me a lot~😊

@FridaKemp
Copy link

So helpful!

@aaron-goulet-bfg
Copy link

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

@aodin
Copy link
Author

aodin commented Oct 10, 2019

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

@aaron-goulet-bfg
Copy link

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

@tzachshabtay
Copy link

@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.

@aodin
Copy link
Author

aodin commented Oct 12, 2019

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

@Ammar022
Copy link

Nice work man. really appreciate it

@DaHogie
Copy link

DaHogie commented Nov 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment