DefaultHTTPErrorHandler should use errors.As() instead of type assertion on HTTPError

[danielbprice]

• Helps consumers who want to wrap HTTPError, and other use cases
• Added testing for HEAD requests which produce errors

[codecov]

Codecov Report

Merging #2228 (5379984) into master (a9879ff) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2228      +/-   ##
==========================================
+ Coverage   92.34%   92.44%   +0.09%     
==========================================
  Files          37       37              
  Lines        3123     3123              
==========================================
+ Hits         2884     2887       +3     
+ Misses        150      149       -1     
+ Partials       89       87       -2     

Impacted Files	Coverage Δ
echo.go	96.15% <100.00%> (+0.96%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9879ff...5379984. Read the comment docs.

[danielbprice]

For what it's worth, I think this change is much scarier than the previous one I submitted, in #2227, but the overall thrust is the same. Totally understand if this seems too risky, maybe it could go into v5 instead (I don't really know).

If you have additional testing I should do, or ideas of how I could raise confidence, let me know.

[aldas]

I think this would be OK if that he.Internal check would be included. Currently this PR is removing that "feature" so if you have set something to internal it would be used (not to break users that are relying on that).

[danielbprice]

Ok, I think I misunderstood what that code was trying to do. I'll take another lap.

[danielbprice]

Just so I understand: the desired behavior is that if an HTTPError has another HTTPError inside of it, then we want to use the inner HTTPError as the "result" of the HTTP call and that error sets the Status code? This case is sadly not covered by the current test suite:

It would be so helpful to have a sense of how this might occur. Does it have to do with Middleware somehow?

The modern thing would be to call "As" again, searching down the chain, but maybe it's only about the directly nested error? I would think that if this was a concern, the right thing would be to loop to find the innermost HTTPError.

This behavior seems odd to me and not what I guess I would expect, but I can preserve it. The commit which introduced this doesn't give any real hints that I can see: ecc01d2

If we could understand this better, I could write some test cases for it as well.

[danielbprice]

Here's a short test which shows how strange this behavior is:

import (
        "fmt"
        "net/http"
        "net/http/httptest"

        "github.com/labstack/echo/v4"
        "github.com/labstack/echo/v4/middleware"
)

func request(method, path string, e *echo.Echo) (int, string) {
        req := httptest.NewRequest(method, path, nil)
        rec := httptest.NewRecorder()
        e.ServeHTTP(rec, req)
        return rec.Code, rec.Body.String()
}

func main() {
        e := echo.New()
        e.Use(middleware.Logger())
        e.Debug = true

        e.GET("/http-error-in-http-error", func(c echo.Context) error {
                err := echo.NewHTTPError(http.StatusForbidden)
                // Put bad Gateway inside of this error, now it's going to be used.
                err.SetInternal(echo.NewHTTPError(http.StatusBadGateway))
                fmt.Printf("Server: Returning Error: %s\n", err.Error())
                return err
        })

        code, body := request(http.MethodGet, "/http-error-in-http-error", e)
        fmt.Printf("Client: Code=%d, Body=%s\n", code, body)
}

If you run it:

Server: Returning Error: code=403, message=Forbidden, internal=code=502, message=Bad Gateway
{"time":"2022-07-21T21:13:35.644020527Z","id":"","remote_ip":"192.0.2.1","host":"example.com","method":"GET","uri":"/http-error-in-http-error","user_agent":"","status":502,"error":"code=403, message=Forbidden, internal=code=502, message=Bad Gateway","latency":67600,"latency_human":"67.6µs","bytes_in":0,"bytes_out":113}
Client: Code=502, Body={
  "error": "code=403, message=Forbidden, internal=code=502, message=Bad Gateway",
  "message": "Bad Gateway"
}

What seems so confusing to me is that this isn't documented anywhere, and code like HTTPError.Error() doesn't follow this protocol.

Would you accept a PR to drop this behavior in V5?

[danielbprice]

Argh. I see that this code has also changed in v5 to some degree; let me know, I can try to sync up with that more closely.

echo/echo.go

Lines 314 to 322 in 7402266

	he := &HTTPError{
	Code: http.StatusInternalServerError,
	Message: http.StatusText(http.StatusInternalServerError),
	}
	if errors.As(err, &he) {
	if he.Internal != nil { // max 2 levels of checks even if internal could have also internal
	errors.As(he.Internal, &he)
	}
	}