Closes #45 - global handlers for most-specific exceptions & fine-grained logging #494
Conversation
| String.format("Error occurred during processing of rest request: %s", errorData), ex); | ||
|
|
||
| switch (status.series()) { | ||
| case CLIENT_ERROR -> logger.warn( |
There was a problem hiding this comment.
I think this should be an error and not a warn
There was a problem hiding this comment.
I strongly disagree.
Consider a client trying to request an endpoint not existing.
Is this an error? No. Errors are states intended never to be reached.
Contrarily, one could even argue if this is even worth a warning.
Many people do log it informational.
There was a problem hiding this comment.
Well, I strongly disagree about exceptions not logged as errors ;) but your argument is valid. Warning may be fine
| String.format("Exception thrown during processing of rest request: %s", errorData)); | ||
| case SERVER_ERROR -> logger.error( | ||
| String.format("Error occurred during processing of rest request: %s", errorData), ex); | ||
| default -> logger.warn( |
There was a problem hiding this comment.
I think this should be an error and not a warn
There was a problem hiding this comment.
Making this an error would contradict the semantics of the HTTP codes.
It cannot be 4xx nor 5xx because those are checked already.
Therefore it must be either 1xx, 2xx or 3xx, which none of indicate faulty behaviour.
| ErrorCode.of(ERROR_KEY_UNKNOWN_ERROR), HttpStatus.INTERNAL_SERVER_ERROR, ex, req); | ||
| logger.error( | ||
| String.format("Unknown error occurred during processing of rest request: %s", errorData), | ||
| ex); |
There was a problem hiding this comment.
Do we want to log this stacktrace?
There was a problem hiding this comment.
Yes, as it points to us failing to provide a specific handler for an exception, indicating missing thoughts around the implementation.
The question then is, which implementation and where?
The stack trace answers immediately.
Take into perspective that this does not pollute the logs, because we should always handle the most-specific exceptions.
No, semantics shall not change with this PR and all tests passed. |
I don't think manually testing undermines anything :) |
The Quality-Gate fails due to test-coverage.
However, unit-testing a global exception handler seems useless as integration with controllers is key.
Therefore, adding tests for exception handling in controllers acceptance- and integrationtests should be considered.
Thanks for your PR! Please fill out the following list :)