search

rcrowley / go-tigertonic

A Go framework for building JSON web services inspired by Dropwizard
Other
997 stars 79 forks source link

Allow Marshaled to respond with arbitrary streams. #93

Closed emil2k closed 8 years ago

emil2k commented 8 years ago

If the return type of a Marshaled handler interface implements the io.Reader interface the stream will be written directly to the requestor. A 'Content-Type' header is required to be specified in the response headers and the 'Accept' header for these particular requests can be anything.

Additionally, if the return type of the Marshaled handler implements the io.Closer interface the stream will be automatically closed after it is flushed to the requestor.

@wadey

willfaught commented 8 years ago

Shouldn't the response Content-Type header value match one of the Accept request header values?

How are errors handled in the response?

Perhaps it would be better to make a separate middleware for this. Marshaled is well understood to be all about marshaling to and from JSON. This complicates that contract. Would it work to make something like a Stream middleware that returns an io.Reader (that might optionally implement io.Closer)?

emil2k commented 8 years ago

Shouldn't the response Content-Type header value match one of the Accept request header values?

Ideally yes, I can take a look at how to do that.

How are errors handled in the response?

Returns a plain text error if it doesn't accept JSON.

Perhaps it would be better to make a separate middleware for this. Marshaled is well understood to be all about marshaling to and from JSON. This complicates that contract.

I don't particularly think it complicates the contract much, but I also talked this over with @wadey before implementing.

Would it work to make something like a Stream middleware that returns an io.Reader (that might optionally implement io.Closer)?

A Stream middleware for this use case ( JSON request and say an image response ) would still have to marshal the requests from JSON and do a lot of the validations around the handler functions so their would be a lot of code duplication.

willfaught commented 8 years ago

Ideally yes, I can take a look at how to do that.

It would be useful to panic if Content-Type doesn't match Accept (programmer error).

A Stream middleware for this use case ( JSON request and say an image response ) would still have to marshal the requests from JSON and do a lot of the validations around the handler functions so their would be a lot of code duplication.

Makes sense.

emil2k commented 8 years ago

@willfaught I implemented (with a test) the content-type checking for a stream response, in case of mismatch it returns a 406.

willfaught commented 8 years ago

@emil2k Nice idea, makes sense! I haven't looked at the code changes; I'll leave that to @wadey.

wadey commented 8 years ago

I just want to double check that this might not break any existing code (code that might have relied on the JSON parsing but also implements io.Reader on the struct).

emil2k commented 8 years ago

@wadey I had a test just for that case: https://github.com/rcrowley/go-tigertonic/pull/93/files#diff-a3a0ddbe848119f0afb81a48380dde05R388

mihasya commented 8 years ago

A TEST??? :eyes:

Sounds like @wadey has been following this more closely. I just read through the comments - my only feedback would have been to implement this as a wrapper around Marshaled but that probably wouldn't work given the nature of the change and that is confirmed by where the changes are happening in the code.

I'm good with this.

wadey commented 8 years ago

@emil2k ah thanks, I missed that test! I am going to go ahead and merge then, looks good :+1: