You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/06/17 19:58:42 UTC
[GitHub] [fineract] thesmallstar opened a new pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle
thesmallstar opened a new pull request #1079:
URL: https://github.com/apache/fineract/pull/1079
According to the logging guidelines by @vorburger I am quoting
> When catching exceptions, either rethrow them, or log them. Either way, include the root cause by using catch (SomeException e) and then either throw AnotherException("..details..", e) or LOG.error("...context...", e).
The AvoidHidingCauseException required that an exception that caused the exception "SomeException" must be thrown and not "AnotherException" as in guidelines.
How should I follow on this?
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-647979952
@thesmallstar no not with `throw x.initcause(y);` but by adding a `, Throwable cause` argument to `RoleNotFoundException` constructor etc.. would it help if I explained it better by raising a first small PR to illustrate what we mean here, which you can then apply everywhere?
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-650264163
@vorburger @xurror can you please review this?
I have some points:-
1. I used suppresswarning at two places( I thought the current approach is correct ).
2. Some Predefined exceptions were of the form:
throw new PredefinedException(x.messege()); // x was lost exception
this is converted to
throw new PredefinedException(x); //x is throwable at the exception accepts it,
This was a a lot of manual work btw :P
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446529329
##########
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:
nice catch! thanks
----------------------------------------------------------------
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
[GitHub] [fineract] xurror commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446533514
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
super("error.msg.email.configuration.name.not.found", "Email configuration with name " + name + " does not exist", name);
Review comment:
Just curious but wouldn't it be better to add the 3rd param here instead of duplicating the method? cause I can see the 2param method will just be history and unused.
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-645610477
Thanks for this Draft. I'll have a closer look at the build failure some time in the coming week, and propose how to address it. (Work on something else in the meantime.) Note to self - see https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/AvoidHidingCauseExceptionCheck.java
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r448555858
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
super("error.msg.email.configuration.name.not.found", "Email configuration with name " + name + " does not exist", name);
Review comment:
@vorburger @xurror making a new PR for this cleanup.
----------------------------------------------------------------
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
[GitHub] [fineract] xurror commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446532048
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
Review comment:
Just curious but wouldn't it be better to add the 3rd param here instead of duplicating the method? cause I can see the 2param method will just be history and unused.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446529577
##########
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:
okay! noted.
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447258956
##########
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:
It's OK, although arguable now it's perhaps more of a... `PlatformExceptionUtil` than an `AbstractPlatformException`.. I'll merge it anyway, as this isn't super important, and perhaps I'll follow-up with a suggested improvement.
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-647013479
> I'll have a closer look at the build failure some time in the coming week, and propose how to
So most of this seems to be related to Fineract's concept of having its own "business" exceptions (like `RoleNotFoundException` and all those other many subclasses of `AbstractPlatformResourceNotFoundException`), and their constructors not accepting any `Throwable cause` argument.
If we do want to introduce this quality check (which I think we should, in order to prevent wrong exception handling for OTHER types of exception than those mentioned above), then I think the easiest way, and most "consistent", would be to simple have even those "business" exceptions accept a `cause` (unrelated to whether they really "need" it or not; I don't really see any "harm"). @ptuomola does this sound reasonable to you?
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-647179155
@vorburger @ptuomola
So I will be doing this:
try{
//intead of throw new Y();
new X()
throw x.initcause(y);
}catch(x){
//rethrow or do something.
}
Problem:
What if Y is coming from some library function like EmptyResultDataAccessException which comes from JDBCtemplate.queryForObject(), in that case I could not figure out a way to do this. Can you add anything to this?(I hope I am making some sense here)
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger merged pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1079:
URL: https://github.com/apache/fineract/pull/1079
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446529515
##########
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:
Also adding the same in rest of the throws in the same function.
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by 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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447254959
##########
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:
Ok now.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar edited a comment on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-647179155
@vorburger @ptuomola
So I will be doing this:
```
try{
//intead of throw new Y();
new X()
throw x.initcause(y);
}catch(x){
//rethrow or do something.
}
```
Problem:
What if Y is coming from some library function like EmptyResultDataAccessException which comes from JDBCtemplate.queryForObject(), in that case I could not figure out a way to do this. Can you add anything to this?(I hope I am making some sense here)
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447258248
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
super("error.msg.email.configuration.name.not.found", "Email configuration with name " + name + " does not exist", name);
Review comment:
@thesmallstar @xurror the real question is really more if there are any (legitimate) remaining usages of the original constructor, without the cause. It seems like this was only used in `EmailConfigurationReadPlatformServiceImpl`, which was changed to use the new constructor, right? Then I actually agree that it would have been nicer and better to just add an additional parameter here... this isn't important enough to justify not finally merging this, so I'll probably go ahead, but I'll leave this open in case you'd like to clean this up in a new PR? (And perhaps in other exceptions where a cause was added - if there is no usage without a cause, it's better to simplify - and force future users to provide a cause.)
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447300913
##########
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:
@thesmallstar I did this, and much more in #1125
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447256294
##########
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:
great, thanks; I've seen it - resolving
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446529856
##########
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:
Agreed! I did not understand do we really need a constructor though(or you mean something else here), an abstract class without any attributes and two static methods will do IMO.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar edited a comment on pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar edited a comment on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-650264163
@vorburger @xurror can you please review this?
I have some points:-
1. I used suppresswarning at two places( I thought the current approach is correct ).
2. Some Predefined exceptions were of the form:
throw new PredefinedException(x.messege()); // x was lost exception
this is converted to
throw new PredefinedException(x); //x is throwable at the exception accepts it,
This was a a lot of manual work btw :P
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446534931
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
super("error.msg.email.configuration.name.not.found", "Email configuration with name " + name + " does not exist", name);
Review comment:
Nope, we cannot change the original implementation. If the cause of the exception is the exception itself, we need this constructor to be used.
----------------------------------------------------------------
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
[GitHub] [fineract] vorburger commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-648430879
> raising a first small PR to illustrate what we mean here, which you can then apply everywhere?
==> #1107
----------------------------------------------------------------
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
[GitHub] [fineract] xurror commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446532048
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
Review comment:
Just curious but wouldn't it be better to add the 3rd param here instead of duplicating the method?
----------------------------------------------------------------
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
[GitHub] [fineract] ptuomola commented on pull request #1079: [WIP] Added AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#issuecomment-647148907
@vorburger Makes sense to me - and who knows, there may be cases where having access to the original cause may help with debugging or troubleshooting. At very least, I can't see why it would cause any harm.
----------------------------------------------------------------
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
[GitHub] [fineract] xurror commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446532048
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
Review comment:
Just curious but wouldn't it be better to add the 3rd param here instead of duplicating the method? cause I can see the 2param method will just be history and unused.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446535152
##########
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:
@vorburger made the change I am unsure though if is this is what you meant.
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r447397311
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/exception/EmailConfigurationNotFoundException.java
##########
@@ -28,4 +29,9 @@
public EmailConfigurationNotFoundException(final String name) {
super("error.msg.email.configuration.name.not.found", "Email configuration with name " + name + " does not exist", name);
Review comment:
Agreed! I will update this
----------------------------------------------------------------
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
[GitHub] [fineract] thesmallstar commented on a change in pull request #1079: Added and Enforced AvoidHidingCauseException Checkstyle (FINERACT-942)
Posted by GitBox <gi...@apache.org>.
thesmallstar commented on a change in pull request #1079:
URL: https://github.com/apache/fineract/pull/1079#discussion_r446529417
##########
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:
Yes! makes sense :D
----------------------------------------------------------------
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