From 94068036050031ad63cd30903011273d86735d9a Mon Sep 17 00:00:00 2001 From: oluexpert99 Date: Wed, 24 Jun 2026 00:27:18 +0100 Subject: [PATCH] FINERACT-2657: Fix NPE updating a provisioning criteria by matching definitions on categoryId - ProvisioningCriteria.update matched incoming definitions by the surrogate id, but the public UPDATE payload keys each definition only by categoryId, so data.getId() was null and update() threw a NullPointerException (HTTP 500) on every provisioning-criteria update. - Match the existing definition by categoryId (its natural key, unique per criteria) and reject an unknown categoryId with a clean validation error instead of the NPE; add a getCategoryId() accessor on ProvisioningCriteriaDefinition. - Add ProvisioningCriteriaTest covering the match-by-categoryId update path and the unknown-category validation path. Signed-off-by: oluexpert99 --- .../domain/ProvisioningCriteria.java | 16 ++- .../ProvisioningCriteriaDefinition.java | 4 + .../domain/ProvisioningCriteriaTest.java | 98 +++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 fineract-provider/src/test/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaTest.java diff --git a/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteria.java b/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteria.java index d01ffc90aef..8fff9e67d5c 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteria.java +++ b/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteria.java @@ -26,6 +26,7 @@ import jakarta.persistence.Table; import jakarta.persistence.UniqueConstraint; import java.time.LocalDateTime; +import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; @@ -33,7 +34,9 @@ import java.util.Set; import org.apache.fineract.accounting.glaccount.domain.GLAccount; import org.apache.fineract.infrastructure.core.api.JsonCommand; +import org.apache.fineract.infrastructure.core.data.ApiParameterError; import org.apache.fineract.infrastructure.core.domain.AbstractAuditableCustom; +import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException; import org.apache.fineract.organisation.provisioning.constants.ProvisioningCriteriaConstants; import org.apache.fineract.organisation.provisioning.data.ProvisioningCriteriaDefinitionData; import org.apache.fineract.portfolio.loanproduct.domain.LoanProduct; @@ -116,10 +119,19 @@ public Map update(JsonCommand command, List loanPro public void update(ProvisioningCriteriaDefinitionData data, GLAccount liability, GLAccount expense) { for (ProvisioningCriteriaDefinition def : provisioningCriteriaDefinition) { - if (data.getId().equals(def.getId())) { + // The public UPDATE payload keys each definition by categoryId (assumed unique per criteria; not + // DB-enforced), not by the surrogate id, which it never carries. Match on categoryId so the + // always-null data.getId() is no longer dereferenced (which threw a NullPointerException / HTTP 500 + // on every update). + if (data.getCategoryId() != null && data.getCategoryId().equals(def.getCategoryId())) { def.update(data.getMinAge(), data.getMaxAge(), data.getProvisioningPercentage(), liability, expense); - break; + return; } } + final List errors = new ArrayList<>(); + errors.add(ApiParameterError.parameterError("error.msg.provisioningcriteria.definition.category.not.found", + "Provisioning criteria has no definition for the given category", ProvisioningCriteriaConstants.JSON_CATEOGRYID_PARAM, + data.getCategoryId())); + throw new PlatformApiDataValidationException(errors); } } diff --git a/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaDefinition.java b/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaDefinition.java index de5c3ac14c5..7a95f3ffca3 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaDefinition.java +++ b/fineract-provider/src/main/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaDefinition.java @@ -79,6 +79,10 @@ public static ProvisioningCriteriaDefinition newPrivisioningCriteria(Provisionin liabilityAccount, expenseAccount); } + public Long getCategoryId() { + return this.provisioningCategory == null ? null : this.provisioningCategory.getId(); + } + public void update(Long minAge, Long maxAge, BigDecimal percentage, GLAccount lia, GLAccount exp) { this.minimumAge = minAge; this.maximumAge = maxAge; diff --git a/fineract-provider/src/test/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaTest.java b/fineract-provider/src/test/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaTest.java new file mode 100644 index 00000000000..91faadaaf76 --- /dev/null +++ b/fineract-provider/src/test/java/org/apache/fineract/organisation/provisioning/domain/ProvisioningCriteriaTest.java @@ -0,0 +1,98 @@ +/** + * 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.organisation.provisioning.domain; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.math.BigDecimal; +import java.util.Set; +import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException; +import org.apache.fineract.organisation.provisioning.data.ProvisioningCriteriaDefinitionData; +import org.junit.jupiter.api.Test; + +/** + * Regression coverage for + * {@link ProvisioningCriteria#update(ProvisioningCriteriaDefinitionData, org.apache.fineract.accounting.glaccount.domain.GLAccount, org.apache.fineract.accounting.glaccount.domain.GLAccount)}. + * + *

+ * The public UPDATE payload keys each definition only by {@code categoryId}; it never carries the surrogate {@code id}. + * The method used to match definitions by {@code data.getId().equals(def.getId())}, so {@code data.getId()} was null + * and every update threw a {@link NullPointerException} (HTTP 500). These tests assert it now matches by + * {@code categoryId} and rejects an unknown category with a clean validation exception instead. + */ +class ProvisioningCriteriaTest { + + @Test + void update_matchesExistingDefinitionByCategoryId() { + final ProvisioningCriteriaDefinition def = mock(ProvisioningCriteriaDefinition.class); + when(def.getCategoryId()).thenReturn(7L); + + final ProvisioningCriteria criteria = new ProvisioningCriteria(); + criteria.setProvisioningCriteriaDefinitions(Set.of(def)); + + final ProvisioningCriteriaDefinitionData data = new ProvisioningCriteriaDefinitionData().setCategoryId(7L).setMinAge(0L) + .setMaxAge(30L).setProvisioningPercentage(BigDecimal.valueOf(25)); + + criteria.update(data, null, null); + + verify(def, times(1)).update(0L, 30L, BigDecimal.valueOf(25), null, null); + } + + @Test + void update_withMultipleDefinitions_updatesOnlyTheMatchingCategory() { + final ProvisioningCriteriaDefinition matched = mock(ProvisioningCriteriaDefinition.class); + when(matched.getCategoryId()).thenReturn(2L); + final ProvisioningCriteriaDefinition other = mock(ProvisioningCriteriaDefinition.class); + when(other.getCategoryId()).thenReturn(5L); + + final ProvisioningCriteria criteria = new ProvisioningCriteria(); + criteria.setProvisioningCriteriaDefinitions(Set.of(matched, other)); + + final ProvisioningCriteriaDefinitionData data = new ProvisioningCriteriaDefinitionData().setCategoryId(2L).setMinAge(31L) + .setMaxAge(60L).setProvisioningPercentage(BigDecimal.valueOf(50)); + + criteria.update(data, null, null); + + verify(matched, times(1)).update(31L, 60L, BigDecimal.valueOf(50), null, null); + verify(other, never()).update(any(), any(), any(), any(), any()); + } + + @Test + void update_withUnknownCategoryId_throwsValidationExceptionNotNpe() { + final ProvisioningCriteriaDefinition def = mock(ProvisioningCriteriaDefinition.class); + when(def.getCategoryId()).thenReturn(7L); + + final ProvisioningCriteria criteria = new ProvisioningCriteria(); + criteria.setProvisioningCriteriaDefinitions(Set.of(def)); + + // categoryId 99 is not present on the criteria, and the data carries no surrogate id (as the real payload + // does). + final ProvisioningCriteriaDefinitionData data = new ProvisioningCriteriaDefinitionData().setCategoryId(99L).setMinAge(0L) + .setMaxAge(30L).setProvisioningPercentage(BigDecimal.valueOf(25)); + + assertThrows(PlatformApiDataValidationException.class, () -> criteria.update(data, null, null)); + verify(def, never()).update(any(), any(), any(), any(), any()); + } +}