fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Date Fri, 26 Jun 2020 17:07:21 GMT

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



Mime
View raw message