more SpotBugs related / inspired code clean up (see FINERACT-702)

based upon code review of changes made by @awasum in previous commit:

 - fix clear contains() GC_UNRELATED_TYPES bug in Loan

 - fix clear equals() EC_UNRELATED_TYPES bug in Loan

 - fix more equals() & hashCode() in...
    * Charge
    * TrialBalance
    * LoanProductBorrowerCycleVariations
    * LoanProductProvisioningEntry

 - fix AccountingConstants to avoid using @FindBugsSuppressWarnings

 - fix SelfLoansDataValidator instead of @FindBugsSuppressWarnings

 - minor clean up in build.gradle (spotbugs has replaced findbugs)

PS: This change originally included also fixing the missing hashCode()
    and equals() in ShareAccountCharge, but that turned out to be non
    trivial, see FINERACT-827, and is thus not yet fixed by this.
This commit is contained in:
Michael Vorburger
2020-01-13 08:41:16 +01:00
parent 7420545d8f
commit 6ca1a678cb
14 changed files with 124 additions and 133 deletions
-6
View File
@@ -64,7 +64,6 @@ apply plugin: 'openjpa'
apply plugin: "com.github.spotbugs"
// apply plugin: 'pmd'
// apply plugin: 'findbugs'
dependencyManagement {
imports {
@@ -341,11 +340,6 @@ compileJava{
/*
pmd {
sourceSets = [sourceSets.main]
ignoreFailures = true
}
findbugs {
ignoreFailures = true
sourceSets = [sourceSets.main]
}
*/
@@ -21,8 +21,6 @@ package org.apache.fineract.accounting.common;
import org.apache.fineract.accounting.financialactivityaccount.data.FinancialActivityData;
import org.apache.fineract.accounting.glaccount.domain.GLAccountType;
import net.sf.ehcache.util.FindBugsSuppressWarnings;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@@ -245,7 +243,15 @@ public class AccountingConstants {
private final Integer value;
private final String code;
private final GLAccountType mappedGLAccountType;
private static List<FinancialActivityData> financialActivities;
static {
financialActivities = new ArrayList<>();
for (final FINANCIAL_ACTIVITY type : FINANCIAL_ACTIVITY.values()) {
FinancialActivityData financialActivityData = convertToFinancialActivityData(type);
financialActivities.add(financialActivityData);
}
}
private FINANCIAL_ACTIVITY(final Integer value, final String code, final GLAccountType mappedGLAccountType) {
this.value = value;
@@ -291,16 +297,7 @@ public class AccountingConstants {
return convertToFinancialActivityData(type);
}
// TODO Fix this..
@FindBugsSuppressWarnings("LI_LAZY_INIT_UPDATE_STATIC")
public static List<FinancialActivityData> getAllFinancialActivities() {
if (financialActivities == null) {
financialActivities = new ArrayList<>();
for (final FINANCIAL_ACTIVITY type : FINANCIAL_ACTIVITY.values()) {
FinancialActivityData financialActivityData = convertToFinancialActivityData(type);
financialActivities.add(financialActivityData);
}
}
return financialActivities;
}
@@ -67,7 +67,6 @@ public class TrialBalance extends AbstractPersistableCustom<Long> {
}
protected TrialBalance() {
}
public Long getOfficeId() {
@@ -97,16 +96,17 @@ public class TrialBalance extends AbstractPersistableCustom<Long> {
@Override
public boolean equals(Object obj) {
if (!obj.getClass().equals(getClass())) return false;
TrialBalance trialBalance = (TrialBalance) obj;
return trialBalance.getOfficeId().equals(this.getOfficeId())
&& trialBalance.getGlAccountId().equals(this.getGlAccountId())
&& trialBalance.getEntryDate().equals(this.getEntryDate())
&& trialBalance.getTransactionDate().equals(this.getTransactionDate());
TrialBalance other = (TrialBalance) obj;
return Objects.equals(other.officeId, officeId)
&& Objects.equals(other.glAccountId, glAccountId)
&& Objects.equals(other.amount, amount)
&& Objects.equals(other.entryDate, entryDate)
&& Objects.equals(other.transactionDate, transactionDate)
&& Objects.equals(other.closingBalance, closingBalance);
}
@Override
public int hashCode() {
// TODO return Objects.hash(officeId, glAccountId, amount, entryDate, transactionDate, closingBalance);
return Objects.hash(officeId, glAccountId, entryDate, transactionDate);
return Objects.hash(officeId, glAccountId, amount, entryDate, transactionDate, closingBalance);
}
}
@@ -74,8 +74,8 @@ public class LoanProductProvisioningEntry extends AbstractPersistableCustom<Long
private GLAccount expenseAccount;
protected LoanProductProvisioningEntry() {
}
public LoanProductProvisioningEntry(final LoanProduct loanProduct, final Office office, final String currencyCode,
final ProvisioningCategory provisioningCategory, final Long overdueInDays, final BigDecimal reservedAmount,
final GLAccount liabilityAccount, final GLAccount expenseAccount, Long criteriaId) {
@@ -95,41 +95,50 @@ public class LoanProductProvisioningEntry extends AbstractPersistableCustom<Long
}
public BigDecimal getReservedAmount() {
return this.reservedAmount ;
return this.reservedAmount;
}
public void addReservedAmount(BigDecimal value) {
this.reservedAmount = this.reservedAmount.add(value) ;
this.reservedAmount = this.reservedAmount.add(value);
}
public Office getOffice() {
return this.office ;
}
return this.office;
}
public GLAccount getLiabilityAccount() {
return this.liabilityAccount;
}
public String getCurrencyCode() {
return this.currencyCode;
}
public GLAccount getExpenseAccount() {
return this.expenseAccount;
}
// TODO Note that this domain class does equals() & hashCode() on getId() for @JoinColumn attributes, which not all other classes do...
public GLAccount getLiabilityAccount() {
return this.liabilityAccount ;
}
public String getCurrencyCode() {
return this.currencyCode ;
}
public GLAccount getExpenseAccount() {
return this.expenseAccount ;
}
@Override
public boolean equals(Object obj) {
if (!obj.getClass().equals(getClass())) return false;
LoanProductProvisioningEntry other = (LoanProductProvisioningEntry) obj;
return Objects.equals(other.loanProduct.getId(), this.loanProduct.getId())
&& Objects.equals(other.provisioningCategory.getId(), this.provisioningCategory.getId())
return Objects.equals(other.entry.getId(), this.entry.getId())
&& Objects.equals(other.criteriaId, this.criteriaId)
&& Objects.equals(other.office.getId(), this.office.getId())
&& Objects.equals(other.getCurrencyCode(), this.getCurrencyCode());
&& Objects.equals(other.currencyCode, this.currencyCode)
&& Objects.equals(other.loanProduct.getId(), this.loanProduct.getId())
&& Objects.equals(other.provisioningCategory.getId(), this.provisioningCategory.getId())
&& Objects.equals(other.overdueInDays, this.overdueInDays)
&& Objects.equals(other.reservedAmount, this.reservedAmount)
&& Objects.equals(other.liabilityAccount.getId(), this.liabilityAccount.getId())
&& Objects.equals(other.expenseAccount.getId(), this.expenseAccount.getId());
}
@Override
public int hashCode() {
// TODO equals() must match: return Objects.hash(entry, criteriaId, office, currencyCode, loanProduct, provisioningCategory, overdueInDays, reservedAmount, liabilityAccount, expenseAccount);
return Objects.hash(loanProduct.getId(), provisioningCategory.getId(), office.getId(), getCurrencyCode());
// NOT return Objects.hash(entry, criteriaId, office, currencyCode, loanProduct, provisioningCategory, overdueInDays, reservedAmount, liabilityAccount, expenseAccount);
// to remain consistent with the implementation in equals(), also use getId() here.
return Objects.hash(entry.getId(), criteriaId, office.getId(), currencyCode, loanProduct.getId(), provisioningCategory.getId(), overdueInDays, reservedAmount, liabilityAccount.getId(), expenseAccount.getId());
}
}
@@ -22,6 +22,7 @@ import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import org.springframework.context.ConfigurableApplicationContext;
@@ -38,7 +39,7 @@ public abstract class ApplicationExitUtil {
System.out.println("\nHit Enter to quit...");
// NOTE: In Eclipse, System.console() is not available.. so:
// (@see https://bugs.eclipse.org/bugs/show_bug.cgi?id=122429)
BufferedReader d = new BufferedReader(new InputStreamReader(System.in, Charset.defaultCharset()));
BufferedReader d = new BufferedReader(new InputStreamReader(System.in, StandardCharsets.UTF_8));
d.readLine();
ctx.stop();
@@ -35,36 +35,20 @@ public abstract class AbstractPersistableCustom<PK extends Serializable> impleme
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
/*
* (non-Javadoc)
*
* @see org.springframework.data.domain.Persistable#getId()
*/
@Override
public Long getId() {
return id;
}
/**
* Sets the id of the entity.
*
* @param id the id to set
*/
protected void setId(final Long id) {
this.id = id;
}
/*
* (non-Javadoc)
*
* @see org.springframework.data.domain.Persistable#isNew()
*/
@Override
public boolean isNew() {
return null == this.id;
}
//We have removed toString(), hashCode() and equals() methods. By adding them end up issues with OpenJPA
// We have removed toString(), hashCode() and equals() methods here, because by adding them here, we end up with issues on OpenJPA.
}
@@ -40,6 +40,7 @@ public class MoneyHelper {
private ConfigurationDomainService configurationDomainService;
@PostConstruct
// This is a hack, but fixing this is not trivial, because some @Entity domain classes use this helper
@FindBugsSuppressWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD")
public void someFunction () {
staticConfigurationDomainService = configurationDomainService;
@@ -112,7 +112,6 @@ public class Charge extends AbstractPersistableCustom<Long> {
private TaxGroup taxGroup;
public static Charge fromJson(final JsonCommand command, final GLAccount account, final TaxGroup taxGroup) {
final String name = command.stringValueOfParameterNamed("name");
final BigDecimal amount = command.bigDecimalValueOfParameterNamed("amount");
final String currencyCode = command.stringValueOfParameterNamed("currencyCode");
@@ -138,7 +137,6 @@ public class Charge extends AbstractPersistableCustom<Long> {
}
protected Charge() {
//
}
private Charge(final String name, final BigDecimal amount, final String currencyCode, final ChargeAppliesTo chargeAppliesTo,
@@ -288,7 +286,6 @@ public class Charge extends AbstractPersistableCustom<Long> {
}
public Map<String, Object> update(final JsonCommand command) {
final Map<String, Object> actualChanges = new LinkedHashMap<>(7);
final String localeAsInput = command.locale();
@@ -507,7 +504,6 @@ public class Charge extends AbstractPersistableCustom<Long> {
}
public ChargeData toData() {
final EnumOptionData chargeTimeType = ChargeEnumerations.chargeTimeType(this.chargeTimeType);
final EnumOptionData chargeAppliesTo = ChargeEnumerations.chargeAppliesTo(this.chargeAppliesTo);
final EnumOptionData chargeCalculationType = ChargeEnumerations.chargeCalculationType(this.chargeCalculation);
@@ -605,26 +601,25 @@ public class Charge extends AbstractPersistableCustom<Long> {
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof Charge)) return false;
Charge charge = (Charge) o;
return Objects.equals(name, charge.name) &&
Objects.equals(amount, charge.amount) &&
Objects.equals(currencyCode, charge.currencyCode) &&
Objects.equals(chargeAppliesTo, charge.chargeAppliesTo) &&
Objects.equals(chargeTimeType, charge.chargeTimeType) &&
Objects.equals(chargeCalculation, charge.chargeCalculation) &&
Objects.equals(chargePaymentMode, charge.chargePaymentMode) &&
Objects.equals(feeOnDay, charge.feeOnDay) &&
Objects.equals(feeInterval, charge.feeInterval) &&
Objects.equals(feeOnMonth, charge.feeOnMonth) &&
Objects.equals(penalty, charge.penalty) &&
Objects.equals(active, charge.active) &&
Objects.equals(deleted, charge.deleted) &&
Objects.equals(minCap, charge.minCap) &&
Objects.equals(maxCap, charge.maxCap) &&
Objects.equals(feeFrequency, charge.feeFrequency) &&
Objects.equals(account, charge.account) &&
Objects.equals(taxGroup, charge.taxGroup);
Charge other= (Charge) o;
return Objects.equals(name, other.name) &&
Objects.equals(amount, other.amount) &&
Objects.equals(currencyCode, other.currencyCode) &&
Objects.equals(chargeAppliesTo, other.chargeAppliesTo) &&
Objects.equals(chargeTimeType, other.chargeTimeType) &&
Objects.equals(chargeCalculation, other.chargeCalculation) &&
Objects.equals(chargePaymentMode, other.chargePaymentMode) &&
Objects.equals(feeOnDay, other.feeOnDay) &&
Objects.equals(feeInterval, other.feeInterval) &&
Objects.equals(feeOnMonth, other.feeOnMonth) &&
penalty == other.penalty &&
active == other.active &&
deleted == other.deleted &&
Objects.equals(minCap, other.minCap) &&
Objects.equals(maxCap, other.maxCap) &&
Objects.equals(feeFrequency, other.feeFrequency) &&
Objects.equals(account, other.account) &&
Objects.equals(taxGroup, other.taxGroup);
}
@Override
@@ -734,15 +734,12 @@ public class Loan extends AbstractPersistableCustom<Long> {
updateLoanSummaryDerivedFields();
}
// TODO Fix this correctly...
// See this review: https://github.com/apache/fineract/pull/670/files/0409af3903d350afe43ef4837e4d915ccbe14285#r357920211
@FindBugsSuppressWarnings("GC_UNRELATED_TYPES")
private void removeOrModifyTransactionAssociatedWithLoanChargeIfDueAtDisbursement(final LoanCharge loanCharge) {
if (loanCharge.isDueAtDisbursement()) {
LoanTransaction transactionToRemove = null;
List<LoanTransaction> transactions = getLoanTransactions() ;
for (final LoanTransaction transaction : transactions) {
if (transaction.isRepaymentAtDisbursement() && transaction.getLoanChargesPaid().contains(loanCharge)) {
if (transaction.isRepaymentAtDisbursement() && doesLoanChargePaidByContainLoanCharge(transaction.getLoanChargesPaid(), loanCharge)) {
final MonetaryCurrency currency = loanCurrency();
final Money chargeAmount = Money.of(currency, loanCharge.amount());
@@ -764,6 +761,15 @@ public class Loan extends AbstractPersistableCustom<Long> {
}
}
private boolean doesLoanChargePaidByContainLoanCharge(Set<LoanChargePaidBy> loanChargePaidBys, LoanCharge loanCharge) {
for (LoanChargePaidBy loanChargePaidBy : loanChargePaidBys) {
if (loanChargePaidBy.getLoanCharge().equals(loanCharge)) {
return true;
}
}
return false;
}
public Map<String, Object> updateLoanCharge(final LoanCharge loanCharge, final JsonCommand command) {
final Map<String, Object> actualChanges = new LinkedHashMap<>(3);
@@ -2719,8 +2725,6 @@ public class Loan extends AbstractPersistableCustom<Long> {
return chargesPayment;
}
// TODO FIX THIS
@FindBugsSuppressWarnings("EC_UNRELATED_TYPES") // see this review: https://github.com/apache/fineract/pull/670/files/0409af3903d350afe43ef4837e4d915ccbe14285#r357920364
public Map<String, Object> undoDisbursal(final ScheduleGeneratorDTO scheduleGeneratorDTO, final List<Long> existingTransactionIds,
final List<Long> existingReversedTransactionIds, AppUser currentUser) {
@@ -2741,7 +2745,7 @@ public class Loan extends AbstractPersistableCustom<Long> {
this.actualDisbursementDate = null;
this.disbursedBy = null;
boolean isDisbursedAmountChanged =
!this.approvedPrincipal.equals(this.loanRepaymentScheduleDetail.getPrincipal());
!this.approvedPrincipal.equals(this.loanRepaymentScheduleDetail.getPrincipal().getAmount());
this.loanRepaymentScheduleDetail.setPrincipal(this.approvedPrincipal);
if (this.loanProduct.isMultiDisburseLoan()) {
for (final LoanDisbursementDetails details : this.disbursementDetails) {
@@ -796,4 +796,6 @@ public class LoanTransaction extends AbstractPersistableCustom<Long> {
&& !(this.isDisbursement() || this.isAccrual() || this.isRepaymentAtDisbursement() || this.isNonMonetaryTransaction() || this
.isIncomePosting());
}
// TODO missing hashCode(), equals(Object obj), but probably OK as long as this is never stored in a Collection.
}
@@ -56,7 +56,6 @@ public class LoanProductBorrowerCycleVariations extends AbstractPersistableCusto
private BigDecimal defaultValue;
protected LoanProductBorrowerCycleVariations() {
}
public LoanProductBorrowerCycleVariations(final Integer borrowerCycleNumber, final Integer paramType, final Integer valueConditionType,
@@ -83,30 +82,19 @@ public class LoanProductBorrowerCycleVariations extends AbstractPersistableCusto
@Override
public boolean equals(final Object obj) {
final LoanProductBorrowerCycleVariations borrowerCycleVariations = (LoanProductBorrowerCycleVariations) obj;
boolean minValequal = false;
if (borrowerCycleVariations.minValue == null && this.minValue == null) {
minValequal = true;
} else if (borrowerCycleVariations.minValue != null && this.minValue != null) {
minValequal = borrowerCycleVariations.minValue.equals(this.minValue);
}
boolean maxValequal = false;
if (borrowerCycleVariations.maxValue == null && this.maxValue == null) {
maxValequal = true;
} else if (borrowerCycleVariations.maxValue != null && this.maxValue != null) {
maxValequal = borrowerCycleVariations.maxValue.equals(this.maxValue);
}
if (borrowerCycleVariations.borrowerCycleNumber.equals(this.borrowerCycleNumber)
&& borrowerCycleVariations.defaultValue.equals(this.defaultValue) && minValequal && maxValequal
&& borrowerCycleVariations.valueConditionType.equals(this.valueConditionType)
&& borrowerCycleVariations.paramType.equals(this.paramType)) { return true; }
return false;
final LoanProductBorrowerCycleVariations other = (LoanProductBorrowerCycleVariations) obj;
return Objects.equals(loanProduct, other.loanProduct)
&& Objects.equals(borrowerCycleNumber, other.borrowerCycleNumber)
&& Objects.equals(paramType, other.paramType)
&& Objects.equals(valueConditionType, other.valueConditionType)
&& Objects.equals(minValue, other.minValue)
&& Objects.equals(maxValue, other.maxValue)
&& Objects.equals(defaultValue, other.defaultValue);
}
@Override
public int hashCode() {
return Objects.hash(borrowerCycleNumber, minValue, maxValue);
return Objects.hash(loanProduct, borrowerCycleNumber, paramType, valueConditionType, minValue, maxValue, defaultValue);
}
public void copy(final LoanProductBorrowerCycleVariations borrowerCycleVariations) {
@@ -132,5 +120,4 @@ public class LoanProductBorrowerCycleVariations extends AbstractPersistableCusto
public BigDecimal getDefaultValue() {
return this.defaultValue;
}
}
@@ -18,8 +18,6 @@
*/
package org.apache.fineract.portfolio.loanproduct.domain;
import org.apache.commons.lang.builder.EqualsBuilder;
import org.apache.commons.lang.builder.HashCodeBuilder;
import org.apache.fineract.infrastructure.core.api.JsonCommand;
import org.apache.fineract.infrastructure.core.domain.AbstractPersistableCustom;
import org.apache.fineract.portfolio.loanproduct.LoanProductConstants;
@@ -40,8 +40,6 @@ import org.springframework.stereotype.Component;
import com.google.gson.JsonElement;
import net.sf.ehcache.util.FindBugsSuppressWarnings;
@Component
public class SelfLoansDataValidator {
private static final Set<String> allowedAssociationParameters = new HashSet<>(
@@ -85,9 +83,6 @@ public class SelfLoansDataValidator {
}
// TODO fix this!
@FindBugsSuppressWarnings("EC_UNRELATED_TYPES")
// See review here: https://github.com/apache/fineract/pull/670
public HashMap<String, Object> validateLoanApplication(final String json){
if (StringUtils.isBlank(json)) { throw new InvalidJsonException(); }
@@ -98,7 +93,7 @@ public class SelfLoansDataValidator {
final String loanTypeParameterName = "loanType";
final String loanTypeStr = this.fromApiJsonHelper.extractStringNamed(loanTypeParameterName, element);
baseDataValidator.reset().parameter(loanTypeParameterName).value(loanTypeStr).notNull().equals("individual");
baseDataValidator.reset().parameter(loanTypeParameterName).value(loanTypeStr).notNull().isOneOfTheseStringValues("individual");
final String clientIdParameterName = "clientId";
final String clientId = this.fromApiJsonHelper.extractStringNamed(clientIdParameterName, element);
@@ -110,12 +105,8 @@ public class SelfLoansDataValidator {
retAttr.put("clientId", Long.parseLong(clientId));
return retAttr;
}
// TODO fix this!
@FindBugsSuppressWarnings("EC_UNRELATED_TYPES")
// See review here: https://github.com/apache/fineract/pull/670/files/0409af3903d350afe43ef4837e4d915ccbe14285#r357920937
public HashMap<String, Object> validateModifyLoanApplication(final String json) {
final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource("loan");
@@ -125,7 +116,7 @@ public class SelfLoansDataValidator {
final String loanTypeParameterName = "loanType";
if(this.fromApiJsonHelper.parameterExists(loanTypeParameterName, element)){
final String loanTypeStr = this.fromApiJsonHelper.extractStringNamed(loanTypeParameterName, element);
baseDataValidator.reset().parameter(loanTypeParameterName).value(loanTypeStr).notNull().equals("individual");
baseDataValidator.reset().parameter(loanTypeParameterName).value(loanTypeStr).notNull().isOneOfTheseStringValues("individual");
}
final String clientIdParameterName = "clientId";
@@ -159,6 +150,4 @@ public class SelfLoansDataValidator {
unsupportedParams.add("template");
}
}
}
@@ -0,0 +1,30 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.fineract.accounting.common;
import org.junit.Test;
import org.springframework.util.Assert;
public class AccountingConstantsTest {
@Test
public void testGetAllFinancialActivities() {
Assert.notEmpty(AccountingConstants.FINANCIAL_ACTIVITY.getAllFinancialActivities(), "static initialization of collection of all enums is broken");
}
}