RestExceptionFacade changes #25

isandesh1986 · 2021-10-07T05:19:37Z

Changes to implement RestExceptionFacade - first commit.

matus-m

My 5 cents: Our goal with Devon Quarkus is to embrace simplicity, efficiency and pragmatism to help developers write production-ready cloud native microservices in short time. With that in mind, we really should not include in the project an exception handling model, that requires so much abstraction and special case.

Custom exceptions should only be added if they add some value.
Same for tests - there is no reason to introduce several layers of abstract classes, if they provide no value.
Do we really need to add mmm-util-validation as I dont see how it is used in the project?
Maybe some parts of this could be extracted into an advanced documentation guide on how to integration mmm exception utils with Quarkus

Not related to this particular MR: as this is supposed to be our reference project, we should really focus on good practices and recommendations for lets say CRUD backend service with Restful API. As this has again fallen back to Interface+Impl in logic layer(completely unnecessary) and the provided API is not following Restful approaches. We also have two ,,subpackages" in the project - where the general really should not be here - thats a library code, which either should be replaced by some devon lib or tkit-quarkus-jpa)

matus-m · 2021-10-12T06:19:12Z

src/main/java/com/devonfw/quarkus/productmanagement/service/v1/ProductRestService.java

@@ -113,9 +114,13 @@ public ProductDto createNewProduct(NewProductDto dto) {
  @APIResponse(responseCode = "404", description = "Product not found"), @APIResponse(responseCode = "500") })
  @Operation(operationId = "getProductById", description = "Returns Product with given id")
  @GET
-  @Path("{id}")
+  @Path("/id/{id}")


{id} was the more correct restful URL, /id/{id} would imply that we want to load an id subresource of product

But looking at the rest of the endpoints in this class .... this is really a poor example to put into reference project, if we want to encourage Restful API microservice design. This should be refactored.

matus-m · 2021-10-12T06:22:43Z

src/main/java/com/devonfw/quarkus/exceptionutils/RestServiceExceptionFacade.java

+    return null;
+  }
+
+  private Throwable getRollbackCause(Throwable exception) {


there is no logic in this code, that is somehow related to transactions or rollback thereof. This should be removed entirely.

matus-m · 2021-10-12T06:32:16Z

src/main/java/com/devonfw/quarkus/exceptionutils/RestServiceExceptionFacade.java

+ */
+@Provider
+@Priority(value = 1)
+public class RestServiceExceptionFacade implements ExceptionMapper<Throwable> {


IMO this whole mapper is needlessly complicated, handling several special case exceptions(NlsRuntimeE,NlsThrowable, ConstrainetE), only to return a generic error response anyhow.

A single Microservice usually wants to define a single Error Response type (let say similar to the manually constructed json map in createJsonErrorResponseMessage) so we can simply:

1 define a DTO for our error response (say ErrorResponseDTO)
2 define a generic exception mapper ( for throwable, catch-all scenario if we have no more specific exception mapper in place) that simply converts the throwable into generic instance of ErrorResponseDTO and status 500
3 define a specific ExceptionMapper for each type of error we are interested in: e.g. validation exception, business exception where again we in the end return our error response DTO.

matus-m · 2021-10-12T06:33:25Z

src/main/java/com/devonfw/quarkus/exceptionutils/RestServiceExceptionFacade.java

+  protected String createJsonErrorResponseMessage(String message, String code, UUID uuid,
+      Map<String, List<String>> errorsMap) {
+
+    Map<String, Object> jsonMap = new HashMap<>();


why bother manually constructing an instance of map, and then marshall it into JSON, when we can use a typesafe DTO that we simply set as response entity

matus-m · 2021-10-12T06:35:51Z

src/main/java/com/devonfw/quarkus/exceptionutils/RestServiceExceptionFacade.java

+  /**
+   * @param mapper the mapper to set
+   */
+  @Inject


injecting via setters is not standard. Just use the preferred approach in Quarkus - inject into fields.

matus-m · 2021-10-12T06:43:47Z

src/main/java/com/devonfw/quarkus/exceptionutils/ProductNotFoundException.java

+  /**
+   * The constructor.
+   */
+  public ProductNotFoundException(String message) {


this is not a very good example case for custom exception. In most cases, the fact that some resource was not found is not an error case and should be handled by a standard http 404 response.

matus-m · 2021-10-12T06:44:45Z

src/main/java/com/devonfw/quarkus/exceptionutils/ServiceConstants.java

+  public static final String URL_FOLDER_SERVICES = "services";
+
+  /** The services URL path. */
+  public static final String URL_PATH_SERVICES = "/" + URL_FOLDER_SERVICES;


where are all these constants used, why are they here?

matus-m · 2021-10-12T06:47:31Z

src/test/java/com/devonfw/quarkus/exceptionutils/BaseTest.java

@@ -0,0 +1,64 @@
+package com.devonfw.quarkus.exceptionutils;


Why create all these abstract classes if we have no need for them. Our actual tests RestServiceExceptionFacadeTest has four! layers above it for no good reason. There is no need to extend any JUnit classes, not do we need to add complicated setup/teardown

matus-m · 2021-10-12T06:51:17Z

src/test/java/com/devonfw/quarkus/exceptionutils/TagModuleTest.java

+ * This is the meta Annotation JUnit5 {@link org.junit.jupiter.api.Tag} for a Module Test
+ *
+ */
+@Tag("module")


not used anywhere....

matus-m · 2021-10-12T06:52:17Z

src/test/java/com/devonfw/quarkus/exceptionutils/ModuleTest.java

@@ -0,0 +1,16 @@
+package com.devonfw.quarkus.exceptionutils;
+
+import org.junit.experimental.categories.Category;


categories are removed in junit5. If you need to group tests(you dont at the moment) use @Tag https://junit.org/junit5/docs/current/user-guide/#writing-tests-tagging-and-filtering

isandesh1986 · 2021-10-12T09:51:30Z

We have reused most of the code from devon4j.
since the intention was to come up with a common approach between existing devon4j and quarkus.
But I agree that this needs a huge refactoring inorder to align the quarkus way.
I will start looking at these and come up with a different approach. Meanwhile, @hohwille -Would you please have a look and
suggest too?

hohwille · 2021-10-14T07:42:50Z

@yntelectual thanks for your review feedback. I agree and cite from the according story #12

It also depends on net.sf.mmm.util.* (esp. NlsThrowable) ...
We can also create our own solution if we want to avoid such dependency.

IMHO our sample app should not have any mmm dependency.
However, I see it a most common use-case that an app needs to throw an exception that is directly send to the end-user while other exceptions are treated as technical exceptions that are "hidden" following OWASP sensitive data exposure best-practice. My PoV is that we create a reasonable use-case for such example and create a reusable generic solution via exception facade. However, we can omitt I18N, UUID, etc. support here in this sample.

Update: We just discussed and are fine with stripping I18N and but we should keep our "contract" we defined for REST communication to keep a reasonable standard that REST is so far lacking:
https://github.com/devonfw/devon4j/blob/master/documentation/guide-rest.asciidoc#rest-exception-handling

hohwille · 2021-10-14T07:50:59Z

Somehow this PR is a kind of duplicate of PR #24 that is already more aligned with what we want to archieve.

hohwille · 2021-10-14T08:19:21Z

@isandesh1986 thanks for your PR 👍 I do not know why and how but by accident we got two PRs for the same story (see also #24). I am very sorry, and actually when you strictly follow our process (https://github.com/devonfw/.github/blob/master/CONTRIBUTING.asciidoc#issues) this should be prevented.
As we have to focus on one solution, I will close this PR in favour of #24.

isandesh1986 added 2 commits October 7, 2021 10:48

RestExceptionFacade first commit.

76046b5

Cleanup of unwanted loggers

17b8eef

matus-m reviewed Oct 12, 2021

View reviewed changes

hohwille linked an issue Oct 14, 2021 that may be closed by this pull request

Extend sample app with RestExceptionFacade #12

Open

hohwille closed this Oct 14, 2021

isandesh1986 deleted the RestExceptionFacade_changes branch October 27, 2021 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RestExceptionFacade changes #25

RestExceptionFacade changes #25

isandesh1986 commented Oct 7, 2021

matus-m left a comment

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

matus-m Oct 12, 2021

isandesh1986 commented Oct 12, 2021

hohwille commented Oct 14, 2021 •

edited

Loading

hohwille commented Oct 14, 2021

hohwille commented Oct 14, 2021

		@@ -0,0 +1,16 @@
		package com.devonfw.quarkus.exceptionutils;

		import org.junit.experimental.categories.Category;

RestExceptionFacade changes #25

RestExceptionFacade changes #25

Conversation

isandesh1986 commented Oct 7, 2021

matus-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isandesh1986 commented Oct 12, 2021

hohwille commented Oct 14, 2021 • edited Loading

hohwille commented Oct 14, 2021

hohwille commented Oct 14, 2021

hohwille commented Oct 14, 2021 •

edited

Loading