ogen icon indicating copy to clipboard operation
ogen copied to clipboard

feat: Response types that return binary strings should be generated as io.ReadCloser

Open urandom opened this issue 2 years ago • 1 comments

Description

Currently, a response that returns a binary strings will generate the following type:

type FooOKSomeContentType struct {
	Data io.Reader
}

This type seems to force us to convert our own io.ReadClosers to something like bytes.Reader before assigning it to the Data field, because we have to Close our readers before releasing control. This unnecessarily copies the data around, instead of sending it directly to the client through the response itself.

If the type was generated as an io.ReadCloser, we could directly assign our own readClosers, and the code would make sure to Close after invoking io.Copy between the ResponseWriter and the response itself.

References

https://swagger.io/docs/specification/data-models/data-types/#file

urandom avatar Sep 04 '23 14:09 urandom

~To clarify the current situation, ogen allows response streaming by using io.Reader for reading the response body, but there is no way for the client to terminate the request early. Is that correct? This functionality would be useful for dealing with server-side events ("text/event-stream" responses).~

Update: Looking at the generated code, it seems that ogen calls io.ReadAll on the response body before exposing it as an io.Reader, which is really just a bytes.Reader. This means that you can't actually make streaming requests right now. It would be nice to have a way of getting direct access to the the underlying http.Response.Body.

mxk avatar Apr 15 '24 14:04 mxk

It seems that ogen calls io.ReadAll on the response body

This doesn't seem to be the case anymore?

rubenv avatar Jan 21 '25 13:01 rubenv

BTW, I think this can be implemented in a backwards compatible way (not break user code).

func encodeSomethingResponse(response SomethingOK, w http.ResponseWriter) error {
	w.Header().Set("Content-Type", "application/octet-stream")
	w.WriteHeader(200)

	writer := w
	if _, err := io.Copy(writer, response); err != nil {
		return errors.Wrap(err, "write")
	}

	return nil
}

If we add to that something along the lines of:

if c, ok := response.Data.(io.Closer); ok {
    if err := c.Close(); err != nil {
        return errors.Wrap(err, "close")
    }
}

This will do the expected and handle closing as well.

Would a PR for that be accepted?

rubenv avatar Jan 21 '25 13:01 rubenv

I believe what @rubenv suggested is what the latest version of ogen does already. So the issue could be closed.

ironsmile avatar Jul 23 '25 10:07 ironsmile