[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)

[GitBox <gi...@apache.org>]

vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446302765



##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/PlatformDataIntegrityException.java
##########
@@ -30,18 +33,39 @@
 
     public PlatformDataIntegrityException(final String globalisationMessageCode, final String defaultUserMessage,
             final Object... defaultUserMessageArgs) {
+        super(findThrowableCause(defaultUserMessageArgs));
         this.globalisationMessageCode = globalisationMessageCode;
         this.defaultUserMessage = defaultUserMessage;
         this.parameterName = null;
-        this.defaultUserMessageArgs = defaultUserMessageArgs;
+        this.defaultUserMessageArgs = filterThrowableCause(defaultUserMessageArgs);
     }
 
     public PlatformDataIntegrityException(final String globalisationMessageCode, final String defaultUserMessage,
             final String parameterName, final Object... defaultUserMessageArgs) {
+        super(findThrowableCause(defaultUserMessageArgs));
         this.globalisationMessageCode = globalisationMessageCode;
         this.defaultUserMessage = defaultUserMessage;
         this.parameterName = parameterName;
-        this.defaultUserMessageArgs = defaultUserMessageArgs;
+        this.defaultUserMessageArgs = filterThrowableCause(defaultUserMessageArgs);
+    }
+
+    private static Throwable findThrowableCause(Object[] defaultUserMessageArgs) {

Review comment:
       copy pasting (my) `findThrowableCause()` & `filterThrowableCause()` seems like a shame... always try to avoid copy/paste, as much as you can. How about introducing a new e.g. `public abstract class AbstractPlatformException extends RuntimeException` with suitable protected constructors, and these two methods as protected static methods?

##########
File path: fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/exception/ProvisioningCriteriaNotFoundException.java
##########
@@ -19,10 +19,15 @@
 package org.apache.fineract.organisation.provisioning.exception;
 
 import org.apache.fineract.infrastructure.core.exception.AbstractPlatformResourceNotFoundException;
+import org.springframework.dao.EmptyResultDataAccessException;
 
 public class ProvisioningCriteriaNotFoundException extends AbstractPlatformResourceNotFoundException {
 
     public ProvisioningCriteriaNotFoundException(final Long id) {
         super("error.msg.provisioning.criteria.id.invalid", "Provisioning Criteria with identifier " + id + " does not exist", id);
     }
+
+    public ProvisioningCriteriaNotFoundException(Long id, EmptyResultDataAccessException e) {
+        super("error.msg.provisioning.criteria.id.invalid", "Provisioning Criteria with identifier " + id + " does not exist", id);

Review comment:
       you forgot actually propagating the cause here... ;-)
   
   ```suggestion
           super("error.msg.provisioning.criteria.id.invalid", "Provisioning Criteria with identifier " + id + " does not exist", id, e);
   ```

##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/service/LoanProductWritePlatformServiceJpaRepositoryImpl.java
##########
@@ -357,9 +357,8 @@ private void handleDataIntegrityIssues(final JsonCommand command, final Throwabl
             throw new PlatformDataIntegrityException("error.msg.product.loan.duplicate.short.name",
                     "Loan product with short name `" + shortName + "` already exists", "shortName", shortName);
         } else if (realCause.getMessage().contains("Duplicate entry")) {
-            final Object[] args = null;
             throw new PlatformDataIntegrityException("error.msg.product.loan.duplicate.charge",
-                    "Loan product may only have one charge of each type.`", "charges", args);
+                    "Loan product may only have one charge of each type.`", "charges");

Review comment:
       I'm not 100% sure and haven't looked much at this, but could it make sense to:
   
   ```suggestion
                       "Loan product may only have one charge of each type.`", "charges", realCause);
   ```

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/core/exception/PlatformApiDataValidationException.java
##########
@@ -43,6 +43,30 @@ public PlatformApiDataValidationException(final String globalisationMessageCode,
         this.errors = errors;
     }
 
+    public PlatformApiDataValidationException(final String globalisationMessageCode, final String defaultUserMessage,
+            final List<ApiParameterError> errors, final Object... defaultUserMessageArgs) {

Review comment:
       In this case, where there is no `Object... defaultUserMessageArgs` in the original far, you should not introduce it. Just have constructors which take an additional last new argument like `, Throwable cause)` - makes sense?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org