BREAKING: add ContextFlowable too demand GetDefaultStatusCode

[dylanhitt]

Relates to #375

Please read the research done in #375.

Looking for some feedback. I looked at how echo/gin deal with this and this is pretty much it. You must wait until you set a Content-Type before you can call WriteHeader as WriteHeader sends data to the client to prepare for serialization of whatever payload we're calling.

The PR removes the need to implement a separate SetDefaultStatusCode from all adaptors and centralized how we set our status code getting rid of some redundant code. I did take a shortcut just get it "working" by just wrapping up the code in our Send method. I think it's best we update our Sender functions to adhere to this signature: type Sender func(http.ResponseWriter, *http.Request, int, any) error. Assuming the rest of the idea his fine. Again I'm not really sure there is another way to do this while maintaining the current fuego.Server Serialization pattern.

Side note: There is technically no way in fuego.Server to set a StatusCode on a response for say like 204 or 201 without using the DefaultStatusCode thing. Right now you can call ctx.SetStatus, but again that's the bug. Once you call WriteHeader the Headers are written to the client. So say if OutTransform fails or some other part of our flow fails you'll only ever get what was Set by status first. Other frameworks go as far as even giving warning to their callers, like from echo:

// WriteHeader sends an HTTP response header with status code. If WriteHeader is
// not called explicitly, the first call to Write will trigger an implicit
// WriteHeader(http.StatusOK). Thus explicit calls to WriteHeader are mainly
// used to send error codes.
func (r *Response) WriteHeader(code int) {
	if r.Committed {
		r.echo.Logger.Warn("response already committed")
		return
	}
	r.Status = code
	for _, fn := range r.beforeFuncs {
		fn()
	}
	r.Writer.WriteHeader(r.Status)
	r.Committed = true
}

// Write writes the data to the connection as part of an HTTP reply.
func (r *Response) Write(b []byte) (n int, err error) {
	if !r.Committed {
		if r.Status == 0 {
			r.Status = http.StatusOK
		}
		r.WriteHeader(r.Status)
	}
	n, err = r.Writer.Write(b)
	r.Size += int64(n)
	for _, fn := range r.afterFuncs {
		fn()
	}
	return
}

if WriteHeader is called twice when using echo it will actually warn with "response already committed"

Sorry for all the words. This was pretty great learning experience

[dylanhitt]

Also this made realize that we have some bugs in SendJSON, SendXML, and SendYaml. We'll need to set the WriteHeader after we've ensured the response has been written properly. I will need to add some more tests to figure this out later.

[dylanhitt]

Okay, so we cannot support setting DefaultStatusCodes and actually setting them unless we wrap http.ResponseWriter. So currently if we set a default status code in say SendJson either with what is on main now or in the code in this PR and we fail to Encode the json it will always return the DefaultStatusCode. So say we set it to 201. The client will only ever get a 201 even if they can't Encode the json say it's like a function or channel or recursive.

The issue is that once WriteHeader is called it cannot be changed. So in our case our only option is to let the Senders just send. So it's not really even possible to change the status code of any successful response in fuego. In order to do this we have to wrap the http.ResponseWriter and Implement it's interface on our Response object. Here is how echo does it.

func (r *Response) WriteHeader(code int) {
	if r.Committed {
		r.echo.Logger.Warn("response already committed")
		return
	}
	r.Status = code
	for _, fn := range r.beforeFuncs {
		fn()
	}
	r.Writer.WriteHeader(r.Status)
	r.Committed = true
}

// Write writes the data to the connection as part of an HTTP reply.
func (r *Response) Write(b []byte) (n int, err error) {
	if !r.Committed {
		if r.Status == 0 {
			r.Status = http.StatusOK
		}
		r.WriteHeader(r.Status)
	}
	n, err = r.Writer.Write(b)
	r.Size += int64(n)
	for _, fn := range r.afterFuncs {
		fn()
	}
	return
}

mainly the Write is what is important here. As they set the Status if it's not been set then write the header. The reason why they can do this and how it differs from ours is that most Encode funcs validate their response then just can .Write on whatever io.Writer they're given. In our case the std libs which looks like:

func (w *response) Write(data []byte) (n int, err error) {
	return w.write(len(data), data, "")
}

// either dataB or dataS is non-zero.
func (w *response) write(lenData int, dataB []byte, dataS string) (n int, err error) {
	if w.conn.hijacked() {
		if lenData > 0 {
			caller := relevantCaller()
			w.conn.server.logf("http: response.Write on hijacked connection from %s (%s:%d)", caller.Function, path.Base(caller.File), caller.Line)
		}
		return 0, ErrHijacked
	}

	if w.canWriteContinue.Load() {
		// Body reader wants to write 100 Continue but hasn't yet. Tell it not to.
		w.disableWriteContinue()
	}

	if !w.wroteHeader {
		w.WriteHeader(StatusOK)
	}
	if lenData == 0 {
		return 0, nil
	}
	if !w.bodyAllowed() {
		return 0, ErrBodyNotAllowed
	}

	w.written += int64(lenData) // ignoring errors, for errorKludge
	if w.contentLength != -1 && w.written > w.contentLength {
		return 0, ErrContentLength
	}
	if dataB != nil {
		return w.w.Write(dataB)
	} else {
		return w.w.WriteString(dataS)
	}
}

Only assumes 200. How other libs get around is by wrapping Response like echo above and overriding Write as Write is called later in Encode for instance the Json encoder:

func (enc *Encoder) Encode(v any) error {
	if enc.err != nil {
		return enc.err
	}

	e := newEncodeState()
	defer encodeStatePool.Put(e)

	err := e.marshal(v, encOpts{escapeHTML: enc.escapeHTML})
	if err != nil {
		return err
	}

	// Terminate each value with a newline.
	// This makes the output look a little nicer
	// when debugging, and some kind of space
	// is required if the encoded value was a number,
	// so that the reader knows there aren't more
	// digits coming.
	e.WriteByte('\n')

	b := e.Bytes()
	if enc.indentPrefix != "" || enc.indentValue != "" {
		enc.indentBuf, err = appendIndent(enc.indentBuf[:0], b, enc.indentPrefix, enc.indentValue)
		if err != nil {
			return err
		}
		b = enc.indentBuf
	}
	if _, err = enc.w.Write(b); err != nil {
		enc.err = err
	}
	return err
}

This is true for both xml and yaml in our case

I know Write is returning an error here but most of the errors that could occur in the std libs response.Write are handled by us further up.

So a few options:

1. Don't provide default error codes for successful responses (this seems like absolute no go honestly)
2. Accept that returning the default status code, even though we've had unmarshalling issue, is acceptable
3. Wrap ResponseWriter and implement Write/WriteHeader/Header
4. Maybe we could reinitialize or close ResponseWriter somehow when encode errors, but I haven't reasoned my way through that at all yet.
5. Perform the validation ourselves somehow so we can be more sure
6. Someone else come up with something cause I can't figure out another way of doing this 😄 😅 😆

[dylanhitt]

@EwenQuim I'm going to leave this open.

We definitely need to address this before v1, but between some of the other things in this repo right now and given that we'll need to have a bit of a discussion to decide an implementation. I'll just let this hang here if that's okay.

[EwenQuim]

OK no problem, thank you for this investigation already, this is good work and I'm sure we'll build on this :)

[ccoVeille]

From discussion that is happening/happened here, and on the following discussion

• http.Flusher is not always present in http.ResponseWriter #439 (comment)

If a breaking change is added, it might be interesting to consider using http.ReponseController instead of http.ResponseWriter

As it might help to solve some issues, and if there is a breaking change here, let's use it to add something else at the same time

TBD further of course

[ccoVeille]

I looked closely at the PR, I understand what you did and why you did it.

I went through the documentation, articles, code, and comments you posted.

It's definitely not easy. Now, I wonder whether it cannot be done differently, without a breaking change

I'm not saying it's impossible, but I hope I could find something else.

Let me process all this.

[dylanhitt]

Reminder to myself. If we end up wrapping ResponseWriter/ResponseController this will change the fuego Server Send. As it would need to adhere to our wrapped response.