FINERACT-839: NPE when creating a Loan for a client fix

[adamsaghy]

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 https://github.com/apache/fineract/blob/develop/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

[awasum]

Make sure this passes Spotbugs checks on your local machine.

Lets wait for Travis to build.

[awasum]

On your local machine, run ./gradlew clean check build and make sure checkstyle and spotbugs rules are not voilated.

[awasum]

Then squash your commits into one when done.

[adamsaghy]

The problem is the SpotBug is a little bit too restrictive:
Bad practice Warnings
Code Warning
NP org.apache.fineract.portfolio.loanproduct.data.LoanProductData.isCompoundingToBePostedAsTransaction() has Boolean return type and returns explicit null
Details
NP_BOOLEAN_RETURN_NULL: Method with Boolean return type returns explicit null
A method that returns either Boolean.TRUE, Boolean.FALSE or null is an accident waiting to happen. This method can be invoked as though it returned a value of type boolean, and the compiler will insert automatic unboxing of the Boolean value. If a null value is returned, this will result in a NullPointerException.

Before the spotBug the business logic required to return null...as it is a valid option from business perspective.

Do you have any advice?

[awasum]

The problem is the SpotBug is a little bit too restrictive:
Bad practice Warnings
Code Warning
NP org.apache.fineract.portfolio.loanproduct.data.LoanProductData.isCompoundingToBePostedAsTransaction() has Boolean return type and returns explicit null
Details
NP_BOOLEAN_RETURN_NULL: Method with Boolean return type returns explicit null
A method that returns either Boolean.TRUE, Boolean.FALSE or null is an accident waiting to happen. This method can be invoked as though it returned a value of type boolean, and the compiler will insert automatic unboxing of the Boolean value. If a null value is returned, this will result in a NullPointerException.

Before the spotBug the business logic required to return null...as it is a valid option from business perspective.

Do you have any advice?

Maybe we add a spotbugs exception to omit checks of this type..Or just for this case in the code base.

@vorburger some help please.

[awasum]

What is wron with instead of returning Null we instead do return Boolean.FALSE;, will this still cause issues?

[adamsaghy]

From business perspective it is different to return FALSE or NULL. I reckon there was a consideration to choose Boolean as object to make it able to return NULL.

A warning would be better.

[awasum]

From business perspective it is different to return FALSE or NULL. I reckon there was a consideration to choose Boolean as object to make it able to return NULL.

A warning would be better.

Ok...In your PR..Suppress spotbugs for the methods having the error..Just above it, place the statement: @FindBugsSuppressWarnings("NP_BOOLEAN_RETURN_NULL") and spotbug check will ignore it.

[awasum]

Have u functionally tested this and the problem has gone away? @adamsaghy . I dont have the right environment to test. If everything is fine on your end..let me know to merge this since every spotbugs and checkstyle test is also passing.

Thanks for your patience.

[adamsaghy]

I did a quick test as now I was able to open a Loan to the client but havent checked all the iteration.
Basically it is a revert back to the original business logic , which we had before the introduced spotbugs changes.

[awasum]

I did a quick test as now I was able to open a Loan to the client but havent checked all the iteration.
Basically it is a revert back to the original business logic , which we had before the introduced spotbugs changes.

Please...check everything and let us know if this is Ok to be merged.

[adamsaghy]

As it worked for 3 years before the spotbugs commit, and we just reverted back to the original behaviour, in my opinion it is safe to merge. ;)

[awasum]

Send more PRs as you identify bugs. Thanks

[vorburger]

What is wron with instead of returning Null we instead do return Boolean.FALSE;, will this still cause issues?

That's what I would have suggested as well... have commented in FINERACT-839.

@adamsaghy are you motivated to raise a follow-up PR for that?