goadesign / goa

šŸŒŸ Goa: Elevate Go API development! šŸš€ Streamlined design, automatic code generation, and seamless HTTP/gRPC support. āœØ
https://goa.design
MIT License
5.54k stars 556 forks source link

Allow implementing `io.WriterTo` for `SkipResponseBodyEncodeDecode` responses. #3522

Closed duckbrain closed 3 weeks ago

duckbrain commented 2 months ago

When a server uses SkipResponseBodyEncodeDecode, the response must be of the type io.ReadCloser. In most cases I've come across, an io.Writer would be more ergonomic to use. I often find myself needing to use io.Pipe to create an io.Reader I can write to. It's made worse by the fact that it's abstracting the http.ResponseWriter and pushing it through a bufio.Reader, so there's lots of extra copying going on.

I propose to have the generated code check if the io.ReadCloser implements io.WriterTo, and if it does, call that instead.

Additionally, it'd be nice to have a helper in the goa package to implement the interface from an io.WriterTo, to make it easier to switch to, eg: If used as the response to a handler, the Read would never be called, and the pipe would never be created.

func SkipResponseWriter(wt io.WriterTo) io.ReadCloser {
    return writerToReaderAdapter{wt, nil}
}

type writerToReaderAdapter struct {
    io.WriterTo
    pr *io.PipeReader
}

func (a *writerToReaderAdapter) initPipe() {
    if a.pr != nil {
        return
    }
    r, w := io.Pipe()
    go func() {
        _, err := wt.WriteTo(w)
        w.CloseWithError(err)
    }()
    a.pr = r
}

func (a *writerToReaderAdapter) Read(b []byte) (n int, err error) {
    a.initPipe()
    return a.pr.Read(b)
}

func (a *writerToReaderAdapter) Close() error {
    a.initPipe()
    return a.pr.Close()
}

As an alternate suggestion, you could have some way in the DSL that changes the expected type.

raphael commented 2 months ago

That makes sense, a couple of thoughts: We'd have to make sure the documentation makes it clear that the generated code supports this alternative. Maybe the generated comment could indicate that this is the case. In the high level implementation above should we use a sync.Once to initialize the pipe in a way that is goroutine safe? We also might want to update the upload_download example to illustrate the use of SkipResponseWriter.

duckbrain commented 1 month ago

I went to update the upload_download example, then I realized it already will use this new code path, since it's returning an *os.File for the io.ReadCloser implementation. :smile:

I could contrive an alternative example that reads a file line by line and streams JSON for each line with line length, etc.

raphael commented 3 weeks ago

Oh that makes sense! nothing to do then, don't think it's worth coming up with a contrive example - thank you for looking into it!