Merge pull request #1102 from commercetools/fix-npe-in-validation-and…

…-transformation Fix npe in validation and transformation
commercetools · Oct 23, 2023 · 2b1b8e9 · 2b1b8e9
2 parents 0c985d1 + aa29202
commit 2b1b8e9
Show file tree

Hide file tree

Showing 10 changed files with 76 additions and 42 deletions.
diff --git a/...tion-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java b/...tion-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java
@@ -259,6 +259,8 @@ void cacheKeysToIds_WithAlreadyCachedKeys_ShouldNotMakeRequestsAndReturnCurrentC
         spyProductService.cacheKeysToIds(singleton(product.getKey())).toCompletableFuture().join();
     assertThat(cache).hasSize(2);
     assertThat(cache).containsKeys(product.getKey(), product2.getKey());
+    cache = spyProductService.cacheKeysToIds(singleton(null)).toCompletableFuture().join();
+    assertThat(cache).hasSize(2);
 
     // verify only 1 request was made to fetch id the first time, but not second time since it's
     // already in cache.

diff --git a/src/main/java/com/commercetools/sync/commons/helpers/BaseBatchValidator.java b/src/main/java/com/commercetools/sync/commons/helpers/BaseBatchValidator.java
@@ -3,7 +3,6 @@
 import static org.apache.commons.lang3.StringUtils.isBlank;
 
 import com.commercetools.api.models.common.AssetDraft;
-import com.commercetools.api.models.common.Reference;
 import com.commercetools.api.models.common.ResourceIdentifier;
 import com.commercetools.api.models.customer.CustomerDraft;
 import com.commercetools.api.models.type.CustomFieldsDraft;
@@ -70,14 +69,6 @@ protected void collectReferencedKeysFromAssetDrafts(
     }
   }
 
-  protected <T> void collectReferencedKeyFromReference(
-      @Nullable final Reference reference, @Nonnull final Consumer<String> keyInReferenceSupplier) {
-
-    if (reference != null && !isBlank(reference.getId())) {
-      keyInReferenceSupplier.accept(reference.getId());
-    }
-  }
-
   protected void handleError(@Nonnull final SyncException syncException) {
     this.syncOptions.applyErrorCallback(syncException);
     this.syncStatistics.incrementFailed();

diff --git a/src/main/java/com/commercetools/sync/commons/utils/CustomValueConverter.java b/src/main/java/com/commercetools/sync/commons/utils/CustomValueConverter.java
@@ -4,9 +4,11 @@
 import com.commercetools.api.models.type.CustomFieldsDraft;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.JsonNodeType;
 import io.vrap.rmf.base.client.utils.json.JsonUtils;
 import java.util.Objects;
 import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
 
 public final class CustomValueConverter {
 
@@ -27,4 +29,17 @@ public static JsonNode convertCustomValueObjDataToJsonNode(@Nullable final Objec
     final JsonNode jsonNode = objectMapper.convertValue(data, JsonNode.class);
     return jsonNode;
   }
+
+  /**
+   * Takes a value of type JsonNode and checks if it's a valid string value.
+   *
+   * @param node - a jsonNode which might contain text
+   * @return true if the given node is not null, not blank and does not contain a "null" value.
+   */
+  public static boolean isValidTextNode(@Nullable JsonNode node) {
+    return node != null
+        && JsonNodeType.STRING.equals(node.getNodeType())
+        && !StringUtils.isBlank(node.asText())
+        && !"null".equals(node.asText());
+  }
 }
diff --git a/src/main/java/com/commercetools/sync/products/helpers/ProductBatchValidator.java b/src/main/java/com/commercetools/sync/products/helpers/ProductBatchValidator.java
@@ -1,5 +1,6 @@
 package com.commercetools.sync.products.helpers;
 
+import static com.commercetools.sync.commons.utils.CustomValueConverter.isValidTextNode;
 import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_ID_FIELD;
 import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.isReferenceOfType;
 import static java.lang.String.format;
@@ -285,7 +286,9 @@ private static Set<String> getReferencedKeysWithReferenceTypeId(
 
     return allAttributeReferences.stream()
         .filter(reference -> isReferenceOfType(reference, referenceTypeId))
-        .map(reference -> reference.get(REFERENCE_ID_FIELD).asText())
+        .map(reference -> reference.get(REFERENCE_ID_FIELD))
+        .filter(field -> isValidTextNode(field))
+        .map(field -> field.asText())
         .filter(Objects::nonNull)
         .collect(Collectors.toSet());
   }

diff --git a/src/main/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidator.java b/src/main/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidator.java
@@ -167,7 +167,7 @@ private static String getProductTypeKey(@Nonnull final AttributeNestedType neste
       throws InvalidReferenceException {
 
     final String key = nestedAttributeType.getTypeReference().getId();
-    if (isBlank(key)) {
+    if (isBlank(key) || "null".equals(key)) {
       throw new InvalidReferenceException(BLANK_ID_VALUE_ON_REFERENCE);
     }
     return key;

diff --git a/src/main/java/com/commercetools/sync/services/impl/BaseService.java b/src/main/java/com/commercetools/sync/services/impl/BaseService.java
@@ -11,6 +11,7 @@
 import com.commercetools.api.models.ResourcePagedQueryResponse;
 import com.commercetools.api.models.graph_ql.GraphQLRequest;
 import com.commercetools.api.models.graph_ql.GraphQLRequestBuilder;
+import com.commercetools.api.models.graph_ql.GraphQLResponse;
 import com.commercetools.api.models.graph_ql.GraphQLVariablesMapBuilder;
 import com.commercetools.sync.commons.BaseSyncOptions;
 import com.commercetools.sync.commons.exceptions.SyncException;
@@ -26,13 +27,7 @@
 import io.vrap.rmf.base.client.Draft;
 import io.vrap.rmf.base.client.error.NotFoundException;
 import io.vrap.rmf.base.client.utils.json.JsonUtils;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
 import java.util.concurrent.CompletionStage;
@@ -145,10 +140,10 @@ public CompletionStage<Map<String, String>> cacheKeysToIdsUsingGraphQl(
         .thenApply(
             graphQlResults -> {
               graphQlResults.stream()
-                  .map(r -> r.getBody().getData())
-                  // todo: set limit to -1, the payload will have errors object but what to do with
-                  // it ?
-                  //                  .filter(Objects::nonNull)
+                  .map(ApiHttpResponse::getBody)
+                  .filter(Objects::nonNull)
+                  .map(GraphQLResponse::getData)
+                  .filter(Objects::nonNull)
                   .forEach(
                       data -> {
                         ObjectMapper objectMapper = JsonUtils.getConfiguredObjectMapper();

diff --git a/src/main/java/com/commercetools/sync/services/impl/BaseTransformServiceImpl.java b/src/main/java/com/commercetools/sync/services/impl/BaseTransformServiceImpl.java
@@ -1,5 +1,6 @@
 package com.commercetools.sync.services.impl;
 
+import static com.commercetools.sync.commons.utils.CustomValueConverter.isValidTextNode;
 import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_ID_FIELD;
 import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_TYPE_ID_FIELD;
 import static java.lang.String.format;
@@ -34,7 +35,6 @@
 import java.util.stream.Collectors;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.text.StringEscapeUtils;
 
 public abstract class BaseTransformServiceImpl {
@@ -224,12 +224,9 @@ protected void cacheResourceReferenceKeys(
   }
 
   private void fillReferenceIdToKeyCache(@Nullable JsonNode id, @Nullable JsonNode key) {
-    if (id != null && !StringUtils.isBlank(id.asText())) {
+    if (isValidTextNode(id)) {
       final String idValue = id.asText();
-      final String keyValue =
-          key != null && !StringUtils.isBlank(key.asText())
-              ? key.asText()
-              : KEY_IS_NOT_SET_PLACE_HOLDER;
+      final String keyValue = isValidTextNode(key) ? key.asText() : KEY_IS_NOT_SET_PLACE_HOLDER;
       referenceIdToKeyCache.add(idValue, keyValue);
     }
   }

diff --git a/src/test/java/com/commercetools/sync/products/helpers/ProductBatchValidatorTest.java b/src/test/java/com/commercetools/sync/products/helpers/ProductBatchValidatorTest.java
@@ -16,22 +16,18 @@
 import com.commercetools.api.models.common.PriceDraftBuilder;
 import com.commercetools.api.models.custom_object.CustomObjectReference;
 import com.commercetools.api.models.customer_group.CustomerGroupResourceIdentifierBuilder;
-import com.commercetools.api.models.product.Attribute;
-import com.commercetools.api.models.product.AttributeBuilder;
-import com.commercetools.api.models.product.ProductDraft;
-import com.commercetools.api.models.product.ProductVariantDraft;
-import com.commercetools.api.models.product.ProductVariantDraftBuilder;
+import com.commercetools.api.models.product.*;
 import com.commercetools.api.models.state.StateResourceIdentifierBuilder;
 import com.commercetools.api.models.tax_category.TaxCategoryResourceIdentifierBuilder;
 import com.commercetools.api.models.type.CustomFieldsDraftBuilder;
 import com.commercetools.api.models.type.FieldContainerBuilder;
 import com.commercetools.api.models.type.TypeResourceIdentifierBuilder;
+import com.commercetools.sync.commons.utils.ResourceIdentifierUtils;
 import com.commercetools.sync.customobjects.helpers.CustomObjectCompositeIdentifier;
 import com.commercetools.sync.products.ProductSyncMockUtils;
 import com.commercetools.sync.products.ProductSyncOptions;
 import com.commercetools.sync.products.ProductSyncOptionsBuilder;
 import com.fasterxml.jackson.databind.node.JsonNodeFactory;
-import com.fasterxml.jackson.databind.node.NullNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import java.math.BigDecimal;
 import java.util.ArrayList;
@@ -142,10 +138,29 @@ void getReferencedProductKeys_WithProductRefAsValue_ShouldReturnKeyInSet() {
   }
 
   @Test
-  void getProductKeyFromReference_WithNullJsonNode_ShouldReturnEmptyOpt() {
-    final NullNode nullNode = JsonNodeFactory.instance.nullNode();
+  void getProductKeyFromReference_WithNullId_ShouldReturnEmptyOpt() {
+    final ObjectNode referenceValue = JsonNodeFactory.instance.objectNode();
+    referenceValue.put(ResourceIdentifierUtils.REFERENCE_TYPE_ID_FIELD, ProductReference.PRODUCT);
+    referenceValue.set(
+        ResourceIdentifierUtils.REFERENCE_ID_FIELD, JsonNodeFactory.instance.nullNode());
     final Attribute productReferenceAttribute =
-        AttributeBuilder.of().name("foo").value(nullNode).build();
+        AttributeBuilder.of().name("foo").value(referenceValue).build();
+
+    final ProductVariantDraft productVariantDraft =
+        ProductVariantDraftBuilder.of().attributes(productReferenceAttribute).build();
+
+    final Set<String> result = ProductBatchValidator.getReferencedProductKeys(productVariantDraft);
+
+    assertThat(result).isEmpty();
+  }
+
+  @Test
+  void getProductKeyFromReference_WithNullStringIdValue_ShouldReturnEmptyOpt() {
+    final Attribute productReferenceAttribute =
+        AttributeBuilder.of()
+            .name("foo")
+            .value(ProductSyncMockUtils.getProductReferenceWithId("null"))
+            .build();
 
     final ProductVariantDraft productVariantDraft =
         ProductVariantDraftBuilder.of().attributes(productReferenceAttribute).build();

diff --git a/src/test/java/com/commercetools/sync/products/utils/ProductTransformUtilsTest.java b/src/test/java/com/commercetools/sync/products/utils/ProductTransformUtilsTest.java
@@ -398,13 +398,26 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu
     final ApiHttpResponse<GraphQLResponse> productTypesResponse =
         TestUtils.mockGraphQLResponse(jsonStringProductTypes);
 
+    String jsonStringTaxCategory =
+        "{ \"taxCategories\": {\"results\":[{\"id\":\"ebbe95fb-2282-4f9a-8747-fbe440e02dc0\","
+            + "\"key\":\"null\"}]}}";
+    final ApiHttpResponse<GraphQLResponse> taxCategoryResponse =
+        TestUtils.mockGraphQLResponse(jsonStringTaxCategory);
+
+    String jsonStringState =
+        "{ \"states\": {\"results\":[{\"id\": " + null + "," + "\"key\": " + null + "}]}}";
+    final ApiHttpResponse<GraphQLResponse> stateResponse =
+        TestUtils.mockGraphQLResponse(jsonStringState);
+
     final ByProjectKeyGraphqlPost byProjectKeyGraphQlPost = mock(ByProjectKeyGraphqlPost.class);
 
     when(sourceClient.graphql()).thenReturn(mock());
     when(sourceClient.graphql().post(any(GraphQLRequest.class)))
         .thenReturn(byProjectKeyGraphQlPost);
     when(byProjectKeyGraphQlPost.execute())
         .thenReturn(CompletableFuture.completedFuture(productTypesResponse))
+        .thenReturn(CompletableFuture.completedFuture(taxCategoryResponse))
+        .thenReturn(CompletableFuture.completedFuture(stateResponse))
         .thenReturn(CompletableFuture.completedFuture(mock(ApiHttpResponse.class)));
 
     mockAttributeCustomObjectReference(sourceClient);
@@ -424,9 +437,12 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu
 
     assertThat(productKey1)
         .hasValueSatisfying(
-            productDraft ->
-                assertThat(productDraft.getProductType().getKey())
-                    .isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER));
+            productDraft -> {
+              assertThat(productDraft.getProductType().getKey())
+                  .isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER);
+              assertThat(productDraft.getTaxCategory().getKey())
+                  .isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER);
+            });
   }
 
   @Test

diff --git a/src/test/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidatorTest.java b/src/test/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidatorTest.java
@@ -264,7 +264,7 @@ void validateAndCollectReferencedKeys_WithEmptyDraft_ShouldHaveEmptyResult() {
                     attributeTypeBuilder
                         .nestedBuilder()
                         .typeReference(
-                            productTypeReferenceBuilder -> productTypeReferenceBuilder.id("")))
+                            productTypeReferenceBuilder -> productTypeReferenceBuilder.id("null")))
             .name("invalidNested")
             .label(ofEnglish("koko"))
             .isRequired(true)
@@ -282,7 +282,7 @@ void validateAndCollectReferencedKeys_WithEmptyDraft_ShouldHaveEmptyResult() {
                                     .nestedBuilder()
                                     .typeReference(
                                         productTypeReferenceBuilder ->
-                                            productTypeReferenceBuilder.id(""))))
+                                            productTypeReferenceBuilder.id("null"))))
             .name("setOfInvalidNested")
             .label(ofEnglish("koko"))
             .isRequired(true)