Sacred http.Handler interface and http.HandlerFunc signature

[learnjin]

As Missy is a http middleware I really think we should not deviate from the standard interface... but embrace it. So instead of

func(w *ResponseWriter, r *http.Request)

use the http.HandlerFunc signature of:

func(w http.ResponseWriter, r *http.Request)

And make the Missy response writer a http.ResponseWriter by implementing:

func (w *ResponseWriter) Header() http.Header{}
func (w *ResponseWriter) Write(p []byte) (int, error){}
func (w *ResponseWriter) WriteHeader(code int) {}

[pbrzostowski]

Yea, i though the same. But I think that the idea was to get status code with own ResponseWriter that holds it (have a look the standard ResponseWriter is an interface, that existing types implements. So as far as I know there is no need to use a ponter to this interface because it is already behind a pointer anyway (I red about this on internet some time ago somewhere).

We could not have own solution but how we can get the status code ? original ResponseWriter does not expose it and it's is used i.e. in missy prometheus on finalizing middlewhere (Prometheus.OnRequestFinished(r.Method, r.URL.Path, w.Status, timer.durationMillis()))

[pbrzostowski]

What about not StatusOK from response? Why this is hardcoded?

[learnjin]

It's not hardcoded, just the initial value. Have a look at service.go where I do a w.WriteHeader(code).

Having said that: There should be a test that verifies that the code is actually updated.

[levrik]

why setting a value there at all? It's really confusing.

[learnjin]

@levrik Look at the standard library. Here's what's httptest is using:

func NewRecorder() *ResponseRecorder {
	return &ResponseRecorder{
		HeaderMap: make(http.Header),
		Body:      new(bytes.Buffer),
		Code:      200,
	}
}

I am not a Gopher yet, but from my limited understanding Go likes to set sane default values. I think 200 is a sane default value, but I am totally open for your suggestions and experiences with Go.

[levrik]

@learnjin mhhh I see your point. But your example creates a new struct instance with defaults. I think our usecase differs a bit. Let's talk about that on Monday 🙂

[slot]

Hi guys,

we have our own response Writer for the following reasons:

• Status code (same as @pbrzostowski mentioned above) is not exposed by the interface of http.ResponseWriter
• I added handy methods to our response Writer like Marshal() or Error()
• I wanted to force the dev-user of missy to use our writer. If we allow any http.ResponseWriter you can break missy code with that.

Possible solutions would be that we stick to the original http.ResponseWriter interface and that we get the status from somwhere else. But that means we must hook into the http server much deeper. I don't know if we want that? But we can try.

Another solution could be that we provide a wrapper func that will upgrade a http.ResponeWriter implementation to a missy Response Writer so you can use other/foreign handlers as well.

[learnjin]

Hi there. I do get the Status. I am happy to go into details, but it's in the code :)

In fact, It's very similar to the httptest.ResponseWriter which does almost the same ....

[learnjin]

rec := ResponseWriter{w, http.StatusOK}
h(&rec,r) // rec.Status holds the status now

[levrik]

@learnjin but now it always is a 200. We should be able to change that.

[slot]

OK @learnjin I'm going with your implementation but then we have to move to marshal function out of the response writer and inside of e.g. the service package. It then has the signature
service.Marshal(w http.ResponseWriter, r *http.Request, v interface{})
it will then look at the Accept Header of the Request and marshall accordingly and write the marshalled string to the writer.

[learnjin]

@levrik see comment above. Maybe I am missing something.

[levrik]

@slot Mhhh. I don't like the idea of putting the marshalling into it's own package. I thought that this XML/JSON thing should be something that missy supports out of the box. We would break this principle with that change.

[pbrzostowski]

Maybe we can implement Marshaling as a HandlerFunc and put it at the end of a chain? Service that implement missy could use context to pass the data. In that way we can support marshaling within missy out of the box but with context convention.

But... I'm not sure if we should utilize context for such a way.

[slot]

closed in favor to #16