Semantics of DefaultHTTPErrorHandler seem wrong when HTTPError.Message is customized && e.Debug

[danielbprice]

Issue Description

While trying to write a custom HTTP error handler, I discovered a relatively new and seemingly incorrect semantic in the DefaultHTTPErrorHandler. I am still using echo v3, and the old semantic seems to be sensible, while the new one does not:

In the old semantic (I am on v3.3.9), the code does this:

        if he, ok := err.(*HTTPError); ok {
		code = he.Code
		msg = he.Message
		if he.Internal != nil {
			err = fmt.Errorf("%v, %v", err, he.Internal)
		}
	} else if e.Debug {
		msg = err.Error()
	} else {
		msg = http.StatusText(code)
	}
	if _, ok := msg.(string); ok {
		msg = Map{"message": msg}
	}

That is to say, if the error is an HTTPError, then we use its Code and Message. If the error is some other type of error, then in debug mode, we use err.Error(). This seems reasonable. It's useful to rember that HTTPError.Message is of type interface{}. And so it is reasonable to pass it anything that could be marshalled to JSON.

In the new code, (I am testing with v4.1.13), introduced by commit ed51400 the logic is different:

	he, ok := err.(*HTTPError)
	if ok {
		if he.Internal != nil {
			if herr, ok := he.Internal.(*HTTPError); ok {
				he = herr
			}
		}
	} else {
		he = &HTTPError{
			Code:    http.StatusInternalServerError,
			Message: http.StatusText(http.StatusInternalServerError),
		}
	}

	// Issue #1426
	code := he.Code
	message := he.Message
	if e.Debug {  // <-------------------- unconditional overwrite of message
		message = err.Error()
	} else if m, ok := message.(string); ok {
		message = Map{"message": m}
	}

This means that toggling on Debug ALWAYS overwrites message (which may be a map full of an arbitrary set of things). This seems to run counter to what a developer would reasonably expect.

Checklist

• Dependencies installed
• No typos
• Searched existing issues and docs

Expected behaviour

e.Debug should not modify the semantics of error messages which have a developer-supplied payload

Actual behaviour

e.Debug now wipes out customized error Message payload

Steps to reproduce

Run server below. Hit endpoint with e.g. curl http://localhost:9999

Working code to debug

package main

import (
  "github.com/labstack/echo/v4"
)

func main() {
  // Echo instance
  e := echo.New()
  e.Debug = true

  // Routes
  e.GET("/", hello)

  // Start server
  e.Logger.Fatal(e.Start(":9999"))
}

// Handler
func hello(c echo.Context) error {
  myError := make(map[string]string)
  myError["error"] = "Bad thing happened"
  myError["reason"] = "no one knows why"
  return echo.NewHTTPError(500, myError)
}

In the above code, if e.Debug is set to False, then curl against this endpoint produces:

{"error":"Bad thing happened","reason":"no one knows why"}

Which seems to be the expected result. But when debug=True, it produces:

"code=500, message=map[error:Bad thing happened reason:no one knows why], internal=\u003cnil\u003e"

Which seems like an unexpected result-- the presence of Debug is changing the error semantics of API. This means that the Debug server can't be used with clients which expect to interpret specific fields from API errors.

Version/commit

4.1.13

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielbprice]

Is an issue really stale if no one has read it or responded? Seems suboptimal.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[danielbprice]

I'm willing to work on a PR for this, if the team can acknowledge that they agree with my statement that this is a problem.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lammel]

That looks like a bug. Sorry for overlooking this issue.

As we have no "production" mode identification, e.Debug is used to decide if the error should be returned.
Still it should not everwrite the whole message. Adding custom information to an Error is useful.

@danielbprice Yes, a PR would be welcome!

[danielbprice]

I will see what I can do.

[lammel]

PR #1666 created now. Open for review, will be merged in the next view days.

[lammel]

Should be fixed already with #1666 .