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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
