You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/03/17 11:26:12 UTC
Re: [PR] FINERACT-2256: New command processing - Product Mix (org.apache.fineract.portfolio.loanproduct.productmix) [fineract]
Copilot commented on code in PR #5622:
URL: https://github.com/apache/fineract/pull/5622#discussion_r2946113718
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/productmix/service/ProductMixWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -52,109 +51,75 @@
public class ProductMixWritePlatformServiceJpaRepositoryImpl implements ProductMixWritePlatformService {
private static final Logger LOG = LoggerFactory.getLogger(ProductMixWritePlatformServiceJpaRepositoryImpl.class);
+
private final PlatformSecurityContext context;
- private final ProductMixDataValidator fromApiJsonDeserializer;
+ private final ProductMixDataValidator productMixDataValidator;
private final ProductMixRepository productMixRepository;
private final LoanProductRepository productRepository;
@Autowired
public ProductMixWritePlatformServiceJpaRepositoryImpl(final PlatformSecurityContext context,
- final ProductMixDataValidator fromApiJsonDeserializer, final ProductMixRepository productMixRepository,
+ final ProductMixDataValidator productMixDataValidator, final ProductMixRepository productMixRepository,
final LoanProductRepository productRepository) {
this.context = context;
- this.fromApiJsonDeserializer = fromApiJsonDeserializer;
+ this.productMixDataValidator = productMixDataValidator;
this.productMixRepository = productMixRepository;
this.productRepository = productRepository;
}
@Transactional
@Override
- public CommandProcessingResult createProductMix(final Long productId, final JsonCommand command) {
-
+ public CommandProcessingResult createProductMix(final ProductMixCommandRequest request) {
try {
-
this.context.authenticatedUser();
- this.fromApiJsonDeserializer.validateForCreate(command.json());
+ final Long productId = request.productId();
+ final Set<Long> restrictedIds = Set.copyOf(request.restrictedProducts());
- final Set<String> restrictedIds = new HashSet<>(Arrays.asList(command.arrayValueOfParameterNamed("restrictedProducts")));
+ this.productMixDataValidator.validateRestrictedProductsForCreate(request.restrictedProducts());
- // remove the existed restriction if it is not exists in
- // restrictedIds.
final List<Long> removedRestrictions = updateRestrictionsForProduct(productId, restrictedIds);
final Map<Long, LoanProduct> restrictedProductsAsMap = getRestrictedProducts(restrictedIds);
final List<ProductMix> productMixes = new ArrayList<>();
createNewProductMix(restrictedProductsAsMap, productId, productMixes);
-
this.productMixRepository.saveAll(productMixes);
final Map<String, Object> changes = new LinkedHashMap<>();
- changes.put("restrictedProductsForMix", restrictedProductsAsMap.keySet());
+ changes.put("restrictedProductsForMix", new ArrayList<>(restrictedProductsAsMap.keySet()));
changes.put("removedProductsForMix", removedRestrictions);
Review Comment:
The "removedProductsForMix" change set is populated from updateRestrictionsForProduct(), but that helper currently collects ProductMix entity IDs (productMix.getId()) rather than restricted product IDs. Update/delete paths report removed restricted product IDs, so this is inconsistent and can confuse API consumers; consider returning restrictedProductId values here as well.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/productmix/data/ProductMixDeleteRequest.java:
##########
@@ -0,0 +1,12 @@
+package org.apache.fineract.portfolio.loanproduct.productmix.data;
+
+import java.io.Serial;
+import java.io.Serializable;
+import lombok.Builder;
Review Comment:
This new source file is missing the standard Apache Software Foundation license header block present in other Java files in this repository. Add the ASF header at the top of the file to comply with project requirements.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/productmix/service/ProductMixWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -52,109 +51,75 @@
public class ProductMixWritePlatformServiceJpaRepositoryImpl implements ProductMixWritePlatformService {
private static final Logger LOG = LoggerFactory.getLogger(ProductMixWritePlatformServiceJpaRepositoryImpl.class);
+
private final PlatformSecurityContext context;
- private final ProductMixDataValidator fromApiJsonDeserializer;
+ private final ProductMixDataValidator productMixDataValidator;
private final ProductMixRepository productMixRepository;
private final LoanProductRepository productRepository;
@Autowired
public ProductMixWritePlatformServiceJpaRepositoryImpl(final PlatformSecurityContext context,
- final ProductMixDataValidator fromApiJsonDeserializer, final ProductMixRepository productMixRepository,
+ final ProductMixDataValidator productMixDataValidator, final ProductMixRepository productMixRepository,
final LoanProductRepository productRepository) {
this.context = context;
- this.fromApiJsonDeserializer = fromApiJsonDeserializer;
+ this.productMixDataValidator = productMixDataValidator;
this.productMixRepository = productMixRepository;
this.productRepository = productRepository;
}
@Transactional
@Override
- public CommandProcessingResult createProductMix(final Long productId, final JsonCommand command) {
-
+ public CommandProcessingResult createProductMix(final ProductMixCommandRequest request) {
try {
-
this.context.authenticatedUser();
- this.fromApiJsonDeserializer.validateForCreate(command.json());
+ final Long productId = request.productId();
+ final Set<Long> restrictedIds = Set.copyOf(request.restrictedProducts());
- final Set<String> restrictedIds = new HashSet<>(Arrays.asList(command.arrayValueOfParameterNamed("restrictedProducts")));
+ this.productMixDataValidator.validateRestrictedProductsForCreate(request.restrictedProducts());
Review Comment:
In createProductMix(), Set.copyOf(request.restrictedProducts()) is evaluated before any validation. If restrictedProducts is null, this will throw a NullPointerException instead of returning a proper validation error. Validate the request first (or defensively treat null as an empty list) before building the Set.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/productmix/service/ProductMixWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -52,109 +51,75 @@
public class ProductMixWritePlatformServiceJpaRepositoryImpl implements ProductMixWritePlatformService {
private static final Logger LOG = LoggerFactory.getLogger(ProductMixWritePlatformServiceJpaRepositoryImpl.class);
+
private final PlatformSecurityContext context;
- private final ProductMixDataValidator fromApiJsonDeserializer;
+ private final ProductMixDataValidator productMixDataValidator;
private final ProductMixRepository productMixRepository;
private final LoanProductRepository productRepository;
@Autowired
public ProductMixWritePlatformServiceJpaRepositoryImpl(final PlatformSecurityContext context,
- final ProductMixDataValidator fromApiJsonDeserializer, final ProductMixRepository productMixRepository,
+ final ProductMixDataValidator productMixDataValidator, final ProductMixRepository productMixRepository,
final LoanProductRepository productRepository) {
this.context = context;
- this.fromApiJsonDeserializer = fromApiJsonDeserializer;
+ this.productMixDataValidator = productMixDataValidator;
this.productMixRepository = productMixRepository;
this.productRepository = productRepository;
}
@Transactional
@Override
- public CommandProcessingResult createProductMix(final Long productId, final JsonCommand command) {
-
+ public CommandProcessingResult createProductMix(final ProductMixCommandRequest request) {
try {
-
this.context.authenticatedUser();
- this.fromApiJsonDeserializer.validateForCreate(command.json());
+ final Long productId = request.productId();
+ final Set<Long> restrictedIds = Set.copyOf(request.restrictedProducts());
- final Set<String> restrictedIds = new HashSet<>(Arrays.asList(command.arrayValueOfParameterNamed("restrictedProducts")));
+ this.productMixDataValidator.validateRestrictedProductsForCreate(request.restrictedProducts());
- // remove the existed restriction if it is not exists in
- // restrictedIds.
final List<Long> removedRestrictions = updateRestrictionsForProduct(productId, restrictedIds);
final Map<Long, LoanProduct> restrictedProductsAsMap = getRestrictedProducts(restrictedIds);
final List<ProductMix> productMixes = new ArrayList<>();
createNewProductMix(restrictedProductsAsMap, productId, productMixes);
-
this.productMixRepository.saveAll(productMixes);
final Map<String, Object> changes = new LinkedHashMap<>();
- changes.put("restrictedProductsForMix", restrictedProductsAsMap.keySet());
+ changes.put("restrictedProductsForMix", new ArrayList<>(restrictedProductsAsMap.keySet()));
changes.put("removedProductsForMix", removedRestrictions);
- return new CommandProcessingResultBuilder().withProductId(productId).with(changes).withCommandId(command.commandId()).build();
- } catch (final JpaSystemException | DataIntegrityViolationException dve) {
-
- handleDataIntegrityIssues(dve);
- return CommandProcessingResult.empty();
- }
- }
-
- private List<Long> updateRestrictionsForProduct(final Long productId, final Set<String> restrictedIds) {
-
- final List<Long> removedRestrictions = new ArrayList<>();
- final List<ProductMix> mixesToRemove = new ArrayList<>();
-
- final List<ProductMix> existedProductMixes = this.productMixRepository.findRestrictedProducts(productId);
- for (final ProductMix productMix : existedProductMixes) {
- if (!restrictedIds.contains(productMix.getProductId().toString())) {
- mixesToRemove.add(productMix);
- removedRestrictions.add(productMix.getId());
- }
- }
- if (!CollectionUtils.isEmpty(mixesToRemove)) {
- this.productMixRepository.deleteAll(mixesToRemove);
- }
- return removedRestrictions;
- }
-
- private void createNewProductMix(final Map<Long, LoanProduct> restrictedProductsAsMap, final Long productId,
- final List<ProductMix> productMixes) {
- final LoanProduct productMixInstance = findByProductIdIfProvided(productId);
- for (final LoanProduct restrictedProduct : restrictedProductsAsMap.values()) {
- final ProductMix productMix = ProductMix.createNew(productMixInstance, restrictedProduct);
- productMixes.add(productMix);
+ return new CommandProcessingResultBuilder().withProductId(productId).with(changes).build();
+ } catch (final JpaSystemException | DataIntegrityViolationException dve) {
+ throw handleDataIntegrityIssues(dve);
}
}
+ @Transactional
@Override
- public CommandProcessingResult updateProductMix(final Long productId, final JsonCommand command) {
-
+ public CommandProcessingResult updateProductMix(final ProductMixCommandRequest request) {
try {
this.context.authenticatedUser();
- this.fromApiJsonDeserializer.validateForUpdate(command.json());
+
+ final Long productId = request.productId();
final Map<String, Object> changes = new LinkedHashMap<>();
final List<ProductMix> existedProductMixes = new ArrayList<>(this.productMixRepository.findByProductId(productId));
if (CollectionUtils.isEmpty(existedProductMixes)) {
throw new ProductMixNotFoundException(productId);
}
- final Set<String> restrictedIds = new HashSet<>(Arrays.asList(command.arrayValueOfParameterNamed("restrictedProducts")));
- // updating with empty array means deleting the existed records.
- if (restrictedIds.isEmpty()) {
+ this.productMixDataValidator.validateRestrictedProductsForUpdate(request.restrictedProducts());
+
+ if (CollectionUtils.isEmpty(request.restrictedProducts())) {
final List<Long> removedRestrictedProductIds = this.productMixRepository.findRestrictedProductIdsByProductId(productId);
this.productMixRepository.deleteAll(existedProductMixes);
Review Comment:
updateProductMix() treats a null restrictedProducts the same as an empty list (CollectionUtils.isEmpty), which will delete all existing product-mix rows. If the intention is that only an explicit empty array triggers deletion, reject null via validation (and/or use request.restrictedProducts().isEmpty() after a not-null check) so missing/omitted restrictedProducts cannot wipe all restrictions.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/productmix/serialization/ProductMixDataValidator.java:
##########
@@ -18,66 +18,50 @@
*/
package org.apache.fineract.portfolio.loanproduct.productmix.serialization;
-import com.google.gson.JsonElement;
-import com.google.gson.reflect.TypeToken;
-import java.lang.reflect.Type;
import java.util.ArrayList;
-import java.util.HashSet;
import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import org.apache.commons.lang3.StringUtils;
import org.apache.fineract.infrastructure.core.data.ApiParameterError;
import org.apache.fineract.infrastructure.core.data.DataValidatorBuilder;
-import org.apache.fineract.infrastructure.core.exception.InvalidJsonException;
import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidationException;
-import org.apache.fineract.infrastructure.core.serialization.FromJsonHelper;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
+import org.springframework.util.CollectionUtils;
@Component
public final class ProductMixDataValidator {
- public static final String RESTRICTED_PRODUCTS = "restrictedProducts";
- /**
- * The parameters supported for this command.
- */
- private static final Set<String> SUPPORTED_PARAMETERS = new HashSet<>(List.of(RESTRICTED_PRODUCTS));
public static final String PRODUCTMIX = "productmix";
+ public static final String RESTRICTED_PRODUCTS = "restrictedProducts";
public static final String RESTRICTED_PRODUCT = "restrictedProduct";
- private final FromJsonHelper fromApiJsonHelper;
+ public void validateRestrictedProductsForCreate(final List<Long> restrictedProducts) {
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+ final DataValidatorBuilder baseDataValidator = new DataValidatorBuilder(dataValidationErrors).resource(PRODUCTMIX);
- @Autowired
- public ProductMixDataValidator(final FromJsonHelper fromApiJsonHelper) {
- this.fromApiJsonHelper = fromApiJsonHelper;
- }
+ baseDataValidator.reset().parameter(RESTRICTED_PRODUCTS).value(restrictedProducts).notNull();
- public void validateForCreate(final String json) {
- if (StringUtils.isBlank(json)) {
- throw new InvalidJsonException();
+ if (CollectionUtils.isEmpty(restrictedProducts)) {
Review Comment:
validateRestrictedProductsForCreate() calls notNull() and then uses CollectionUtils.isEmpty(), which is also true for null. This can produce two validation errors (null + empty) for the same input. Consider separating the null vs empty checks so only the most appropriate validation message is emitted.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org