Exception mapping #24
Conversation
GuentherJulian
requested review from
TugbaDalmaz,
lilliCao,
EduardKrieger,
SchettlerKoehler and
hohwille
There was a problem hiding this comment.
@GuentherJulian thanks for this PR. 👍
By some accident we got two PRs for the same story (see #25).
After both reviews this PR is closer to what Malte, Matus and I had in mind.
Can you please have a look at the review feedback and start rework/discussion.
| } | ||
| if (exception instanceof WebApplicationException) { |
There was a problem hiding this comment.
else?
also we should add support for JAX-RS Exceptions (that we have been lacking in devon4j) with e.g. NotFoundException procuding 404.
There was a problem hiding this comment.
The Quarkus RESTEasy extension provides a mapper for NotFoundException out of the box, that produces a 404 response (https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy/runtime/src/main/java/io/quarkus/resteasy/runtime/NotFoundExceptionMapper.java).
Should we anyway implement and use an own mapper for it?
There was a problem hiding this comment.
If that already plays together with out exception mapper, it is fully sufficient. But then you could add a nice inline comment to the code giving this valuable information including that link so people can easily understand what is going on. I also was not aware of that so thanks for making this clear.
There was a problem hiding this comment.
But comming back to my original concern:
How about permission denied? How about validation failures (from BV)?
We do not want to report this as 500 (internal server error)!
This is IMHO only complete if we have all these cases covered.
Ideally we even add a JUnit that is testing all these scenarios to make them transparent and to document our features and behaviour.
There was a problem hiding this comment.
Since the Quarkus ExceptionMapper implementations do not return any further information (only status code) I decided to create own implementations. So now we have exception mapper for NotFoundException, UnauthorizedException and ValidationException + our ApplicationBusinessException. Other exceptions are handled as WebApplicationException or as general Exception with status 500
@hohwille is this what you had in mind?
src/main/java/com/devonfw/quarkus/general/service/exception/RestServiceExceptionFacade.java
Outdated
Show resolved
Hide resolved
src/main/java/com/devonfw/quarkus/productmanagement/logic/UcFindProductImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/devonfw/quarkus/productmanagement/logic/UcManageProductImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/devonfw/quarkus/productmanagement/service/v1/ProductRestService.java
Outdated
Show resolved
Hide resolved
|
@hohwille thanks for your review comments. I have changed some parts accordingly. Can you please take again a look and check my comments? |
...n/java/com/devonfw/quarkus/general/service/exception/mapper/UnauthorizedExceptionMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/devonfw/quarkus/general/service/exception/mapper/NotFoundExceptionMapper.java
Outdated
Show resolved
Hide resolved
src/main/java/com/devonfw/quarkus/general/service/exception/mapper/AbstractExceptionMapper.java
Outdated
Show resolved
Hide resolved
|
@GuentherJulian please check for the conflicts. thanks |
# Conflicts: # src/main/java/com/devonfw/quarkus/general/rest/json/CustomObjectMapper.java # src/main/java/com/devonfw/quarkus/general/rest/json/PageSerializer.java # src/main/java/com/devonfw/quarkus/productmanagement/logic/UcFindProductImpl.java # src/main/java/com/devonfw/quarkus/productmanagement/logic/UcManageProduct.java # src/main/java/com/devonfw/quarkus/productmanagement/logic/UcManageProductImpl.java # src/main/java/com/devonfw/quarkus/productmanagement/rest/v1/ProductRestService.java # src/main/java/com/devonfw/quarkus/productmanagement/rest/v1/model/NewProductDto.java # src/main/resources/db/migration/V001__Create_schema.sql # src/main/resources/db/migration/V002__Master_data.sql # src/test/java/com/devonfw/demoquarkus/rest/v1/ProductRestServiceTest.java
@maybeec Conflicts resolved. |
|
@hohwille ,waiting for your approval |
Added
RestServiceExceptionFacadeas described in issue #12 for exception handling. Business exceptions are thrown throughApplicationBusinessExceptionand return status 400 (bad request). Other runtime exceptions will result in a response with status 500 (internal server error).Also did some refactoring of the rest service and the database migration script.