Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING: add ContextFlowable too demand GetDefaultStatusCode #376

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dylanhitt
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

dylanhitt commented Jan 29, 2025

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 😄 😅 😆

@EwenQuim EwenQuim added this to the v0.19 milestone Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants