Fix empty request body issue

[AlaaAttya]

Issue description: Currently, to get the request from context, we do it like fdhttp.Request(ctx) but if you tried to access this request body or params, you'll find it empty. for example, if you try to do the following

request := fdhttp.Request(context)
data, _ := ioutil.ReadAll(request.Body)
fmt.Println(string(empty))

you'll always receive an empty body

Issue details: Currently, we set the request body in the HTTPServe() method https://github.com/foodora/go-ranger/blob/2b6f1bd410f72b191bebe2163f5831ced6d9550e/fdhttp/router.go#L333 and at this point, the request object still has no request body (request body is being set in the request handler)

[Drahflow]

There are actually (at least) two http.Request objects around:

router.StdHandler("POST", "/std", func(w http.ResponseWriter, req *http.Request) {
    ctx := req.Context()
    fmt.Printf("%p <-> %p\n", req, fdhttp.Request(ctx))
}

shows me

0xc0000d8200 <-> 0xc0000d8100

I fear the API of fdhttp is fundamentally broken, there should never have been fdhttp.Request(ctx), as there is no way to have a two-way link between a Context and a Request. When trying to attach the context to "the" request, Request.WithContext will create a new request, and when trying to attach the request to "the" context context.WithValue will create a new context. And golang net/http already gives us Request.Context(), so that direction must exist.

[muharem]

I surprised that it is carrying all those request's data in a context even for those who use std handlers. I would even suggest to use std handlers, I do not see any advantages of using the others instead it brings unnecessary complexity, forces to implement the EndpointFunc rather than standard HandleFunc, and reads data from a stream (for some usecases we even do not need read those data), carries all those data in a context. For the new major version I would prefer to remove that.

[tonyalaribe]

@Drahflow goranger copies the request body into the context so middlewares. So we actually have 2 request bodies. One in the http.Request, and the other in the context. There is some good reason for this, being that middlewares can access this body multiple times. The request body is an io.Reader, so you can only read from it once. But from the context, you can read it multiple times.

But even then, I think this is not a decision a router should be making for every application, since this basically means you have duplicate of the request body lying in memory, whether you use it or not. As a router, fdhttp is really doing too much in my opinion.

This is where we inject the request body into the context: https://github.com/foodora/go-ranger/blob/master/fdhttp/router.go#L159 https://github.com/foodora/go-ranger/blob/master/fdhttp/router.go#L133

Actually, with the request being copied as well, we might have more than 2 copies of the request body

[Drahflow]

There is some good reason for this, being that middlewares can access this body multiple times. The request body is an io.Reader, so you can only read from it once. But from the context, you can read it multiple times.

You can't (or to be precise, it's going to be at EOF on later reads). The code you linked shows a single ioutil.NopCloser is being instantiated from the buffer and re-injected into the request as Body.

I tested this thus:

router.POST("/fd", func(ctx context.Context) (int, interface{}) {
        for i := 0; i < 3; i++ {
                 data := fdhttp.RequestBody(ctx)
                 fmt.Println("%v", data)
                 body, err := ioutil.ReadAll(data)
                 if err != nil {
                         panic(err)
                }
                fmt.Println(body)
       }
        return 200, "Ok."
})

and I see the body printed once, and then two empty bodies being read.

In theory, we could have a re-readable body in the context, but currently we don't. And we probably shouldn't, to avoid forcing body-buffering everywhere.

[tonyalaribe]

My bad, you're right. For some reason, i assumed we were reading the raw bytes into the context and not the buffer. But still, what do you think we should do about the original issue, and how go-ranger handles this data?

[Drahflow]

IMHO:

1. About the original issue (and for now): We don't do anything. If someone needs the body, please use fdhttp.RequestBody(ctx).
2. About the API issue, I propose removing fdhttp.Request(ctx) and adding RequestXXX methods for those members of http.Request still missing. And we change fdhttp.RequestBody to read the entire body, attach the buffered body to the ctx (we WithValue an initially empty pointer to the context early in the routing so we can actually do it in-place) + return a new Reader to that buffer each time it is called.

[awdng]

whats the verdict on this PR?