Skip to content

FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() fails silently when loanScheduleType is null or has an invalid value #5288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wkigenyi wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() fails silently when loanScheduleType is null or has an invalid value #5288

wkigenyi wants to merge 1 commit into from
+11 −4

Conversation

@wkigenyi
Copy link
Contributor

@wkigenyi wkigenyi commented Jan 5, 2026

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

@wkigenyi wkigenyi changed the title FINERACT-2432 FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() throws NullPointerException when loanScheduleType is null Jan 5, 2026
adamsaghy
adamsaghy reviewed Jan 8, 2026
View reviewed changes
...n/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java Outdated
private void validateLoanScheduleType(final String transactionProcessingStrategyCode, final DataValidatorBuilder baseDataValidator,
final JsonElement element) {
final String loanScheduleType = this.fromApiJsonHelper.extractStringNamed(LoanProductConstants.LOAN_SCHEDULE_TYPE, element);
if (loanScheduleType == null) {
Copy link
Contributor

@adamsaghy adamsaghy Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having null value is not correct and it shall fail the validation and throw error!

Copy link
Contributor Author

@wkigenyi wkigenyi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sole purpose of the PR is to check if the value null was provided instead of leaving it out entirely or using undefined (remember it is not a required field), and if this is the case the validate function does not proceed (returns immediately). Is an integration test necessary?

Copy link
Contributor

@adamsaghy adamsaghy Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its required and mandatory field or it should be.

Copy link
Contributor Author

@wkigenyi wkigenyi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the validation and the API docs loanScheduleType is not among the mandatory fields but it has a default value that is used in case it is not specified when creating a loan product ie "CUMULATIVE". I have added some tests to take care of this. I have also address a closely related issue where the loanScheduleType has an invalid value.

Copy link
Contributor

@adamsaghy adamsaghy Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (this.fromApiJsonHelper.parameterExists(LoanProductConstants.LOAN_SCHEDULE_TYPE, element)) {
            validateLoanScheduleType(transactionProcessingStrategyCode, baseDataValidator, element);
        }

This is how it starts. If any value was provided for "loanScheduleType", it CANNOT be Null!

Copy link
Contributor

@adamsaghy adamsaghy Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the rest of the changes can be removed.

Copy link
Contributor Author

@wkigenyi wkigenyi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. This is clean.

Copy link
Contributor Author

@wkigenyi wkigenyi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks clean but it does not sort out the issue. If loanScheduleType is NULL and it is ignored then we shall have a NullPointer issue here if (! LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType))) because execution did not stop even after finding the the value is null.

Copy link
Contributor

@adamsaghy adamsaghy Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, I guess we can keep this part

if (loanScheduleType == null || baseDataValidator.hasError()) {
            return;
        }

Copy link
Contributor Author

@wkigenyi wkigenyi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@wkigenyi wkigenyi changed the title FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() throws NullPointerException when loanScheduleType is null FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() throws NullPointerException when loanScheduleType is null or has an invalid value Jan 9, 2026
@wkigenyi wkigenyi changed the title FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() throws NullPointerException when loanScheduleType is null or has an invalid value FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() fails silently when loanScheduleType is null or has an invalid value Jan 9, 2026
adamsaghy
adamsaghy reviewed Jan 9, 2026
View reviewed changes
...n/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java Outdated
Comment on lines 2803 to 2817
if (!LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType))
if (loanScheduleType == null || baseDataValidator.hasError()) {
return;
}

LoanScheduleType scheduleType;
try {
scheduleType = LoanScheduleType.valueOf(loanScheduleType);
} catch (IllegalArgumentException e) {
throw new GeneralPlatformDomainRuleException("error.msg.invalid.loan.schedule.type", "Invalid loan schedule type");
}

if (!LoanScheduleType.PROGRESSIVE.equals(scheduleType)
Copy link
Contributor

@adamsaghy adamsaghy Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed.
If the value is NULL just throw new PlatformApiDataValidationException...

Something like this:

Index: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java
--- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java	(revision dff889be29032cd75f18d2d418f28989f53c3cf5)
+++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java	(date 1767962780072)
@@ -2797,8 +2797,9 @@
     private void validateLoanScheduleType(final String transactionProcessingStrategyCode, final DataValidatorBuilder baseDataValidator,
             final JsonElement element) {
         final String loanScheduleType = this.fromApiJsonHelper.extractStringNamed(LoanProductConstants.LOAN_SCHEDULE_TYPE, element);
-        baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType)
-                .isOneOfEnumValues(LoanScheduleType.class);
+        baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType).notNull();
+        baseDataValidator.throwValidationErrors();
+        baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType).isOneOfEnumValues(LoanScheduleType.class);
 
         if (!LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType))
                 && AdvancedPaymentScheduleTransactionProcessor.ADVANCED_PAYMENT_ALLOCATION_STRATEGY

adamsaghy
adamsaghy requested changes Jan 9, 2026
View reviewed changes
Copy link
Contributor

@adamsaghy adamsaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly see my review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamsaghy adamsaghy adamsaghy requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wkigenyi @adamsaghy