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/05/20 13:23:39 UTC
[GitHub] [fineract] xurror opened a new pull request #915: FINERACT-986
xurror opened a new pull request #915:
URL: https://github.com/apache/fineract/pull/915
Update spotbugs to v4.2.0
## Description
Describe the changes made and why they were made. Ignore if these details are present on the associated Jira ticket
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide
----------------------------------------------------------------
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 edited a comment on pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror edited a comment on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631899383
I tried forcing the function to be able to accept a null parameter, but that just won't do:
``` org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge since first historized release unspecified
In class org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge
In method org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge.percentageOf(BigDecimal, BigDecimal)
Parameter value
At SavingsAccountCharge.java:[lines 540-548]
value must be non-null but is marked as nullable
This parameter is always used in a way that requires it to be non-null, but the parameter is explicitly annotated as being Nullable. Either the use of the parameter or the annotation is wrong.
This got me thinking, why would the default value for a decimal be `null` instead of zero. It's a glitch that has worked well so far, probably because we never stumble on the crash. The method is vouched to fail if a null value is passed through.
----------------------------------------------------------------
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] awasum commented on a change in pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428064991
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
Hoping this is not check for null somewhere in the code base?
----------------------------------------------------------------
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 pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631899383
I tried forcing the function to be able to accept a null parameter, but that just won't do:
` org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge since first historized release unspecified
In class org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge
In method org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge.percentageOf(BigDecimal, BigDecimal)
Parameter value
At SavingsAccountCharge.java:[lines 540-548]
value must be non-null but is marked as nullable
This parameter is always used in a way that requires it to be non-null, but the parameter is explicitly annotated as being Nullable. Either the use of the parameter or the annotation is wrong.`
This got me thinking, why would the default value for a decimal be `null` instead of zero. It's a glitch that has worked well so far, probably because we never stumble on the crash. The method is vouched to fail if a null value is passed through.
----------------------------------------------------------------
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 #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428069678
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
Can't tell for sure, Just know it's a nullable field and as null, spotbugs throws an error when the value is passed to `update.percentageOf(BigDecimal, BigDecimal)`. So I just replaced that particular instance where I needed it to be something other than null
----------------------------------------------------------------
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 pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631508619
@vorburger @awasum LGTY?
----------------------------------------------------------------
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] awasum commented on a change in pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
awasum commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428064991
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
Hoping this is not checked for null somewhere in the code base?
----------------------------------------------------------------
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] maektwain commented on a change in pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
maektwain commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428231333
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
Yes true, might be just initializing it with null and then populating with real values... Could be like that
----------------------------------------------------------------
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 #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428228209
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
Hmm, This is troublesome. I thought this was the most subtle way of going around this. I don't think multiplication by null is possible. This is actually some faulty method but I think I'll see how to make spotsbug ignore this just to stay on the safe side
----------------------------------------------------------------
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 pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631897808
> Of you have to use Suppress (it would be better to avoid it), then please create a JIRA and add a comment like this: @SuppressWarning("...") // TODO FINERACT-123
> […](#)
Suppress is not helping here. Unfortunately.
----------------------------------------------------------------
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 #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631654748
Of you have to use Suppress (it would be better to avoid it), then please
create a JIRA and add a comment like this: @SuppressWarning("...") // TODO
FINERACT-123
>
----------------------------------------------------------------
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 pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631935016
Had some other concern, passing a ZERO to the function and performing a ZERO multiplication means the method returns a ZERO. I fear what a bunch of ZEROs everywhere may do to the core system functions.
----------------------------------------------------------------
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 edited a comment on pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror edited a comment on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631899383
I tried forcing the function to be able to accept a null parameter, but that just won't do:
```
org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge since first historized release unspecified
In class org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge
In method org.apache.fineract.portfolio.savings.domain.SavingsAccountCharge.percentageOf(BigDecimal, BigDecimal)
Parameter value
At SavingsAccountCharge.java:[lines 540-548]
value must be non-null but is marked as nullable
This parameter is always used in a way that requires it to be non-null, but the parameter is explicitly annotated as being Nullable. Either the use of the parameter or the annotation is wrong.
```
This got me thinking, why would the default value for a decimal be `null` instead of zero. It's a glitch that has worked well so far, probably because we never stumble on the crash. The method is vouched to fail if a null value is passed through.
----------------------------------------------------------------
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 pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631939730
I just created https://issues.apache.org/jira/browse/FINERACT-987 for 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] xurror commented on pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631959120
@awasum, @vorburger, @maektwain can we conclude this thread on these changes?
----------------------------------------------------------------
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] maektwain commented on pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
maektwain commented on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631923153
The method is updating the values, in the savings account charge now the null value was there to initialise it , might be the case that it was checking for null values somewhere else while applying the charges and then based on that computing from zero.
This could be interesting to note that we can start to remove all the null values where it should be a bigDecimal as zero functionally .
What are your thoughts ?
----------------------------------------------------------------
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] maektwain commented on a change in pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
maektwain commented on a change in pull request #915:
URL: https://github.com/apache/fineract/pull/915#discussion_r428187942
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountCharge.java
##########
@@ -474,7 +475,7 @@ public void update(final BigDecimal amount, final LocalDate dueDate, final Month
break;
case PERCENT_OF_AMOUNT:
this.percentage = newValue;
- this.amountPercentageAppliedTo = null;
+ this.amountPercentageAppliedTo = BigDecimal.ZERO;
Review comment:
I think we cannot have this zero, since it would affect the calculation, there is a reason why they were using null, Zero means there is some value provided ie zero, did you test it though?
----------------------------------------------------------------
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 edited a comment on pull request #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
xurror edited a comment on pull request #915:
URL: https://github.com/apache/fineract/pull/915#issuecomment-631899383
----------------------------------------------------------------
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 #915: FINERACT-986: Update spotbugs to v4.2.0
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #915:
URL: https://github.com/apache/fineract/pull/915
----------------------------------------------------------------
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