[BUG] Recorded body overwritten by error status code handler

[feliciegodineau]

When the request is being recorded, the body is overwritten before sending the response by the error status code handler. This doesn't happen when the request is not recorded.

package main

import (
	"fmt"

	"github.com/kataras/iris/v12"
)

func main() {
	app := iris.New()
	app.Handle("POST", "/fail", func(ctx iris.Context) {
		ctx.Record()
		ctx.StatusCode(500)
		if _, err := ctx.Recorder().Write([]byte("My error message")); err != nil {
			panic(err)
		}
		ctx.Next()
	})
	app.Run(iris.Addr(":8080"))
}

Response:
Internal Server Error

This can be hotfix by setting the option DisableAutoFireStatusCode to true
But, I think that the request should have the same behaviour when it's being recorded and when it's not.

package main

import (
	"fmt"

	"github.com/kataras/iris/v12"
)

func main() {
	app := iris.New()
	app.Handle("POST", "/fail", func(ctx iris.Context) {
		// ctx.Record()
		ctx.StatusCode(500)
		if _, err := ctx.Recorder().Write([]byte("My error message")); err != nil {
			panic(err)
		}
		ctx.Next()
	})
	app.Run(iris.Addr(":8080"))
}

Response:
My error message

[kataras]

Hello, actually that was a feature rather than a bug, there is a test case for that as well:

iris/core/router/status_test.go

Lines 38 to 43 in 02699ed

	app.Get("/406", func(ctx context.Context) {
	ctx.Record()
	ctx.WriteString("this should not be sent, only status text will be sent")
	ctx.WriteString("the handler can handle 'rollback' of the text when error code fired because of the recorder")
	ctx.StatusCode(iris.StatusNotAcceptable)
	})

The same thing happens to a gzip response writer as well, the code:

iris/core/router/handler.go

Lines 445 to 454 in 02699ed

	if statusCodeSuccessful(w.StatusCode()) { // if not an error status code
	w.WriteHeader(statusCode) // then set it manually here, otherwise it should be set via ctx.StatusCode(...)
	}
	// reset if previous content and it's recorder, keep the status code.
	w.ClearHeaders()
	w.ResetBody()
	} else if w, ok := ctx.ResponseWriter().(*context.GzipResponseWriter); ok {
	// reset and disable the gzip in order to be an expected form of http error result
	w.ResetBody()
	w.Disable()

Why?

1. Imagine that you set a body in a handler and in the next handler you have an error that should clear the previous response and set the error's handler one.
2. The developer can also call the app.FireErrorCode directly, so it should override any previous set body or header.
3. Transactions, which they should "rollback"/reset the whole response on case of an error (transactions feature uses the response recorder too)

That's why the DisableAutoFireStatusCode was introduced as well. However, give me time to think over this, we can change this behavior by a new configuration option (in order to not break existing behavior and not need to disable auto fire status code in all handlers).

Note: you can use the ctx. as you used to, no need to call the ctx.Recorder().XXX functions, e.g. your code should be like that:

package main

import "github.com/kataras/iris/v12"

func main() {
	app := iris.New()
	app.Post("/fail", func(ctx iris.Context) {
		ctx.Record()
		ctx.StatusCode(iris.StatusInternalServerError) // 500 status code
		ctx.WriteString("My error message")
		// ctx.Next() // also you don't need that.
	})
	app.Listen(":8080")
}

Update

OK. The default behavior is now changed to respect any body and headers set on recorder and gzip writer on error codes. The behavior is now aligned with the common response writer as you correctly noted it should. However, it brings a breaking change which can be fixed by the new Configuration.ResetOnFireErrorCode field (defaults to false), if set to true then the router will behave as it used to so far (pre v12.2.0).

Thank you @feliciegodineau