fix: err from next(c) in logger middleware was ignored

[rgci]

In our test suite we have a "throughmiddleware" test helper function sending the handler result through the middleware chain. Unfortunately the logger middleware overwrites the "err" variable after it gets filled by next(c) so that it becomes nil again and is never returned.

This PR fixes this.

[codecov[bot]]

Codecov Report

Merging #1855 (d46b5c2) into master (a4ab482) will decrease coverage by 0.14%. The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #1855      +/-   ##
==========================================
- Coverage   89.57%   89.43%   -0.15%     
==========================================
  Files          31       31              
  Lines        2686     2688       +2     
==========================================
- Hits         2406     2404       -2     
- Misses        180      182       +2     
- Partials      100      102       +2     

Impacted Files	Coverage Δ
middleware/logger.go	83.92% <16.66%> (-3.35%)	:arrow_down:

---

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 3b07058...d46b5c2. Read the comment docs.

[aldas]

Just some remarks:

I'm not really sure what is correct fix. I'm not the author of that middleware but logger middleware is access log. It logs logs requests and (error)responses. It also calls global error handler to log response errors (error log). If we would start to returning errors returned from next(c) now it would mean that global error handler would be called twice. If we would remove calling c.Error and return error it could mean that error could be discarded by some middleware up in chain.

NB: you are missing tests to see if this change works or not.

NB: you could make logger middleware first middleware in chain and not worry about errors being returned or not. c.Error will call error handler anyway.

[aldas]

I think for this to work

			if err = next(c); err != nil {
				c.Error(err)
			}

should be changed to

			err = next(c)

Note to self:

asses would returning an error cause problems

Some historical background related to why c.Error(err) is instead of return err

• https://github.com/labstack/echo/issues/424
• https://github.com/labstack/echo/issues/427
• https://github.com/labstack/echo/commit/b47abdb9ec9b6d38475ed385ebe052affbc14172

[stale[bot]]

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