Add internal server error handler and tests#8
Merged
Conversation
Contributor
Author
eawoods
approved these changes
Apr 21, 2020
eawoods
left a comment
There was a problem hiding this comment.
Builds well and it is easy to find the new error logs in surefire-reports.
One test fails with two errors in ProgramControllerUnitTest:
-------------------------------------------------------------------------------
Test set: org.breedinginsight.api.v1.controller.ProgramControllerUnitTest
-------------------------------------------------------------------------------
Tests run: 13, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.576 sec <<< FAILURE! - in org.breedinginsight.api.v1.controller.ProgramControllerUnitTest
getProgramsAllDataAccessException Time elapsed: 0.01 sec <<< ERROR!
org.jooq.exception.DataAccessException: TEST
at org.breedinginsight.api.v1.controller.ProgramControllerUnitTest.getProgramsAllDataAccessException(ProgramControllerUnitTest.java:68)
getProgramsSingleDataAccessException Time elapsed: 0.003 sec <<< ERROR!
org.jooq.exception.DataAccessException: TEST
at org.breedinginsight.api.v1.controller.ProgramControllerUnitTest.getProgramsSingleDataAccessException(ProgramControllerUnitTest.java:61)
According to Chris this is a known error that is fixed in a subsequent PR. Approving, to move on.
timparsons
approved these changes
Apr 21, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This feature creates an ID for internal server errors and prints them to the logs so that we are able to debug the errors later.
Design Concepts
I used the micronaut custom error handling ability to catch all unhandled exceptions that are returned from the controllers. When we receive the unhandled exception, a UUID is created and logged with stack trace of the error. An internal server error response with the error UUID is then returned.
https://guides.micronaut.io/micronaut-error-handling/guide/index.html
Exceptions that are thrown in the services and handled in the controllers are not affected. By handling all unhandled exceptions, we can remove the custom
DataAccessExceptionhandling that we were doing on all controller code and also expand the types errors we are able to trace.Only getAll() and getById() in the ProgramController have been modified to account for the new error handling so far. There is another card to convert the existing controllers over to the new error handling method.
https://trello.com/c/D5WB7q1J/183-inf-69-update-controllers-to-throw-custom-web-exception-to-ensure-logging-is-consistent
Once this pull request is merged, the other controller methods will be converted.
Review Considerations
Note: This feature breaks two DataAcessException unit tests
getProgramsAllDataAccessExceptionandgetProgramsSingleDataAccessException. This is because the getById and getAll functions were converted to the new error handling code as an example. These tests will be removed in INF-69.