diff --git a/src/integration-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java b/src/integration-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java index 6b5bde1c4c..b2e6e96cd6 100644 --- a/src/integration-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java +++ b/src/integration-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 index 34c42b3bc1..cb9290247c 100644 --- 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 void collectReferencedKeyFromReference( - @Nullable final Reference reference, @Nonnull final Consumer 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 index 6a53054dda..8ebe697e06 100644 --- 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 index 91b97b9a19..d66d767c59 100644 --- 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 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 index d4895a7cd0..231d40d89a 100644 --- 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 index 02bda39621..88b84f613e 100644 --- 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> 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 index be89082e74..6465214e6c 100644 --- 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 index 90d65359ad..0339030b16 100644 --- 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 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 index d41808e79b..eca0e10127 100644 --- a/src/test/java/com/commercetools/sync/products/utils/ProductTransformUtilsTest.java +++ b/src/test/java/com/commercetools/sync/products/utils/ProductTransformUtilsTest.java @@ -398,6 +398,17 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu final ApiHttpResponse productTypesResponse = TestUtils.mockGraphQLResponse(jsonStringProductTypes); + String jsonStringTaxCategory = + "{ \"taxCategories\": {\"results\":[{\"id\":\"ebbe95fb-2282-4f9a-8747-fbe440e02dc0\"," + + "\"key\":\"null\"}]}}"; + final ApiHttpResponse taxCategoryResponse = + TestUtils.mockGraphQLResponse(jsonStringTaxCategory); + + String jsonStringState = + "{ \"states\": {\"results\":[{\"id\": " + null + "," + "\"key\": " + null + "}]}}"; + final ApiHttpResponse stateResponse = + TestUtils.mockGraphQLResponse(jsonStringState); + final ByProjectKeyGraphqlPost byProjectKeyGraphQlPost = mock(ByProjectKeyGraphqlPost.class); when(sourceClient.graphql()).thenReturn(mock()); @@ -405,6 +416,8 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu .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 index f091736bdc..dc809c313b 100644 --- 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)