Skip to content

Commit 2b1b8e9

Browse files
salander85salander85
authored
Merge pull request #1102 from commercetools/fix-npe-in-validation-and-transformation
Fix npe in validation and transformation
2 parents 0c985d1 + aa29202 commit 2b1b8e9

10 files changed

Lines changed: 76 additions & 42 deletions

File tree

‎src/integration-test/java/com/commercetools/sync/integration/services/impl/ProductServiceImplIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,8 @@ void cacheKeysToIds_WithAlreadyCachedKeys_ShouldNotMakeRequestsAndReturnCurrentC
259259
spyProductService.cacheKeysToIds(singleton(product.getKey())).toCompletableFuture().join();
260260
assertThat(cache).hasSize(2);
261261
assertThat(cache).containsKeys(product.getKey(), product2.getKey());
262+
cache = spyProductService.cacheKeysToIds(singleton(null)).toCompletableFuture().join();
263+
assertThat(cache).hasSize(2);
262264

263265
// verify only 1 request was made to fetch id the first time, but not second time since it's
264266
// already in cache.

‎src/main/java/com/commercetools/sync/commons/helpers/BaseBatchValidator.java‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import static org.apache.commons.lang3.StringUtils.isBlank;
44

55
import com.commercetools.api.models.common.AssetDraft;
6-
import com.commercetools.api.models.common.Reference;
76
import com.commercetools.api.models.common.ResourceIdentifier;
87
import com.commercetools.api.models.customer.CustomerDraft;
98
import com.commercetools.api.models.type.CustomFieldsDraft;
@@ -70,14 +69,6 @@ protected void collectReferencedKeysFromAssetDrafts(
7069
}
7170
}
7271

73-
protected <T> void collectReferencedKeyFromReference(
74-
@Nullable final Reference reference, @Nonnull final Consumer<String> keyInReferenceSupplier) {
75-
76-
if (reference != null && !isBlank(reference.getId())) {
77-
keyInReferenceSupplier.accept(reference.getId());
78-
}
79-
}
80-
8172
protected void handleError(@Nonnull final SyncException syncException) {
8273
this.syncOptions.applyErrorCallback(syncException);
8374
this.syncStatistics.incrementFailed();

‎src/main/java/com/commercetools/sync/commons/utils/CustomValueConverter.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@
44
import com.commercetools.api.models.type.CustomFieldsDraft;
55
import com.fasterxml.jackson.databind.JsonNode;
66
import com.fasterxml.jackson.databind.ObjectMapper;
7+
import com.fasterxml.jackson.databind.node.JsonNodeType;
78
import io.vrap.rmf.base.client.utils.json.JsonUtils;
89
import java.util.Objects;
910
import javax.annotation.Nullable;
11+
import org.apache.commons.lang3.StringUtils;
1012

1113
public final class CustomValueConverter {
1214

@@ -27,4 +29,17 @@ public static JsonNode convertCustomValueObjDataToJsonNode(@Nullable final Objec
2729
final JsonNode jsonNode = objectMapper.convertValue(data, JsonNode.class);
2830
return jsonNode;
2931
}
32+
33+
/**
34+
* Takes a value of type JsonNode and checks if it's a valid string value.
35+
*
36+
* @param node - a jsonNode which might contain text
37+
* @return true if the given node is not null, not blank and does not contain a "null" value.
38+
*/
39+
public static boolean isValidTextNode(@Nullable JsonNode node) {
40+
return node != null
41+
&& JsonNodeType.STRING.equals(node.getNodeType())
42+
&& !StringUtils.isBlank(node.asText())
43+
&& !"null".equals(node.asText());
44+
}
3045
}

‎src/main/java/com/commercetools/sync/products/helpers/ProductBatchValidator.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.commercetools.sync.products.helpers;
22

3+
import static com.commercetools.sync.commons.utils.CustomValueConverter.isValidTextNode;
34
import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_ID_FIELD;
45
import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.isReferenceOfType;
56
import static java.lang.String.format;
@@ -285,7 +286,9 @@ private static Set<String> getReferencedKeysWithReferenceTypeId(
285286

286287
return allAttributeReferences.stream()
287288
.filter(reference -> isReferenceOfType(reference, referenceTypeId))
288-
.map(reference -> reference.get(REFERENCE_ID_FIELD).asText())
289+
.map(reference -> reference.get(REFERENCE_ID_FIELD))
290+
.filter(field -> isValidTextNode(field))
291+
.map(field -> field.asText())
289292
.filter(Objects::nonNull)
290293
.collect(Collectors.toSet());
291294
}

‎src/main/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidator.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ private static String getProductTypeKey(@Nonnull final AttributeNestedType neste
167167
throws InvalidReferenceException {
168168

169169
final String key = nestedAttributeType.getTypeReference().getId();
170-
if (isBlank(key)) {
170+
if (isBlank(key) || "null".equals(key)) {
171171
throw new InvalidReferenceException(BLANK_ID_VALUE_ON_REFERENCE);
172172
}
173173
return key;

‎src/main/java/com/commercetools/sync/services/impl/BaseService.java‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import com.commercetools.api.models.ResourcePagedQueryResponse;
1212
import com.commercetools.api.models.graph_ql.GraphQLRequest;
1313
import com.commercetools.api.models.graph_ql.GraphQLRequestBuilder;
14+
import com.commercetools.api.models.graph_ql.GraphQLResponse;
1415
import com.commercetools.api.models.graph_ql.GraphQLVariablesMapBuilder;
1516
import com.commercetools.sync.commons.BaseSyncOptions;
1617
import com.commercetools.sync.commons.exceptions.SyncException;
@@ -26,13 +27,7 @@
2627
import io.vrap.rmf.base.client.Draft;
2728
import io.vrap.rmf.base.client.error.NotFoundException;
2829
import io.vrap.rmf.base.client.utils.json.JsonUtils;
29-
import java.util.Collections;
30-
import java.util.HashSet;
31-
import java.util.Iterator;
32-
import java.util.List;
33-
import java.util.Map;
34-
import java.util.Optional;
35-
import java.util.Set;
30+
import java.util.*;
3631
import java.util.concurrent.CompletableFuture;
3732
import java.util.concurrent.CompletionException;
3833
import java.util.concurrent.CompletionStage;
@@ -145,10 +140,10 @@ public CompletionStage<Map<String, String>> cacheKeysToIdsUsingGraphQl(
145140
.thenApply(
146141
graphQlResults -> {
147142
graphQlResults.stream()
148-
.map(r -> r.getBody().getData())
149-
// todo: set limit to -1, the payload will have errors object but what to do with
150-
// it ?
151-
// .filter(Objects::nonNull)
143+
.map(ApiHttpResponse::getBody)
144+
.filter(Objects::nonNull)
145+
.map(GraphQLResponse::getData)
146+
.filter(Objects::nonNull)
152147
.forEach(
153148
data -> {
154149
ObjectMapper objectMapper = JsonUtils.getConfiguredObjectMapper();

‎src/main/java/com/commercetools/sync/services/impl/BaseTransformServiceImpl.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.commercetools.sync.services.impl;
22

3+
import static com.commercetools.sync.commons.utils.CustomValueConverter.isValidTextNode;
34
import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_ID_FIELD;
45
import static com.commercetools.sync.commons.utils.ResourceIdentifierUtils.REFERENCE_TYPE_ID_FIELD;
56
import static java.lang.String.format;
@@ -34,7 +35,6 @@
3435
import java.util.stream.Collectors;
3536
import javax.annotation.Nonnull;
3637
import javax.annotation.Nullable;
37-
import org.apache.commons.lang3.StringUtils;
3838
import org.apache.commons.text.StringEscapeUtils;
3939

4040
public abstract class BaseTransformServiceImpl {
@@ -224,12 +224,9 @@ protected void cacheResourceReferenceKeys(
224224
}
225225

226226
private void fillReferenceIdToKeyCache(@Nullable JsonNode id, @Nullable JsonNode key) {
227-
if (id != null && !StringUtils.isBlank(id.asText())) {
227+
if (isValidTextNode(id)) {
228228
final String idValue = id.asText();
229-
final String keyValue =
230-
key != null && !StringUtils.isBlank(key.asText())
231-
? key.asText()
232-
: KEY_IS_NOT_SET_PLACE_HOLDER;
229+
final String keyValue = isValidTextNode(key) ? key.asText() : KEY_IS_NOT_SET_PLACE_HOLDER;
233230
referenceIdToKeyCache.add(idValue, keyValue);
234231
}
235232
}

‎src/test/java/com/commercetools/sync/products/helpers/ProductBatchValidatorTest.java‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,22 +16,18 @@
1616
import com.commercetools.api.models.common.PriceDraftBuilder;
1717
import com.commercetools.api.models.custom_object.CustomObjectReference;
1818
import com.commercetools.api.models.customer_group.CustomerGroupResourceIdentifierBuilder;
19-
import com.commercetools.api.models.product.Attribute;
20-
import com.commercetools.api.models.product.AttributeBuilder;
21-
import com.commercetools.api.models.product.ProductDraft;
22-
import com.commercetools.api.models.product.ProductVariantDraft;
23-
import com.commercetools.api.models.product.ProductVariantDraftBuilder;
19+
import com.commercetools.api.models.product.*;
2420
import com.commercetools.api.models.state.StateResourceIdentifierBuilder;
2521
import com.commercetools.api.models.tax_category.TaxCategoryResourceIdentifierBuilder;
2622
import com.commercetools.api.models.type.CustomFieldsDraftBuilder;
2723
import com.commercetools.api.models.type.FieldContainerBuilder;
2824
import com.commercetools.api.models.type.TypeResourceIdentifierBuilder;
25+
import com.commercetools.sync.commons.utils.ResourceIdentifierUtils;
2926
import com.commercetools.sync.customobjects.helpers.CustomObjectCompositeIdentifier;
3027
import com.commercetools.sync.products.ProductSyncMockUtils;
3128
import com.commercetools.sync.products.ProductSyncOptions;
3229
import com.commercetools.sync.products.ProductSyncOptionsBuilder;
3330
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
34-
import com.fasterxml.jackson.databind.node.NullNode;
3531
import com.fasterxml.jackson.databind.node.ObjectNode;
3632
import java.math.BigDecimal;
3733
import java.util.ArrayList;
@@ -142,10 +138,29 @@ void getReferencedProductKeys_WithProductRefAsValue_ShouldReturnKeyInSet() {
142138
}
143139

144140
@Test
145-
void getProductKeyFromReference_WithNullJsonNode_ShouldReturnEmptyOpt() {
146-
final NullNode nullNode = JsonNodeFactory.instance.nullNode();
141+
void getProductKeyFromReference_WithNullId_ShouldReturnEmptyOpt() {
142+
final ObjectNode referenceValue = JsonNodeFactory.instance.objectNode();
143+
referenceValue.put(ResourceIdentifierUtils.REFERENCE_TYPE_ID_FIELD, ProductReference.PRODUCT);
144+
referenceValue.set(
145+
ResourceIdentifierUtils.REFERENCE_ID_FIELD, JsonNodeFactory.instance.nullNode());
147146
final Attribute productReferenceAttribute =
148-
AttributeBuilder.of().name("foo").value(nullNode).build();
147+
AttributeBuilder.of().name("foo").value(referenceValue).build();
148+
149+
final ProductVariantDraft productVariantDraft =
150+
ProductVariantDraftBuilder.of().attributes(productReferenceAttribute).build();
151+
152+
final Set<String> result = ProductBatchValidator.getReferencedProductKeys(productVariantDraft);
153+
154+
assertThat(result).isEmpty();
155+
}
156+
157+
@Test
158+
void getProductKeyFromReference_WithNullStringIdValue_ShouldReturnEmptyOpt() {
159+
final Attribute productReferenceAttribute =
160+
AttributeBuilder.of()
161+
.name("foo")
162+
.value(ProductSyncMockUtils.getProductReferenceWithId("null"))
163+
.build();
149164

150165
final ProductVariantDraft productVariantDraft =
151166
ProductVariantDraftBuilder.of().attributes(productReferenceAttribute).build();

‎src/test/java/com/commercetools/sync/products/utils/ProductTransformUtilsTest.java‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -398,13 +398,26 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu
398398
final ApiHttpResponse<GraphQLResponse> productTypesResponse =
399399
TestUtils.mockGraphQLResponse(jsonStringProductTypes);
400400

401+
String jsonStringTaxCategory =
402+
"{ \"taxCategories\": {\"results\":[{\"id\":\"ebbe95fb-2282-4f9a-8747-fbe440e02dc0\","
403+
+ "\"key\":\"null\"}]}}";
404+
final ApiHttpResponse<GraphQLResponse> taxCategoryResponse =
405+
TestUtils.mockGraphQLResponse(jsonStringTaxCategory);
406+
407+
String jsonStringState =
408+
"{ \"states\": {\"results\":[{\"id\": " + null + "," + "\"key\": " + null + "}]}}";
409+
final ApiHttpResponse<GraphQLResponse> stateResponse =
410+
TestUtils.mockGraphQLResponse(jsonStringState);
411+
401412
final ByProjectKeyGraphqlPost byProjectKeyGraphQlPost = mock(ByProjectKeyGraphqlPost.class);
402413

403414
when(sourceClient.graphql()).thenReturn(mock());
404415
when(sourceClient.graphql().post(any(GraphQLRequest.class)))
405416
.thenReturn(byProjectKeyGraphQlPost);
406417
when(byProjectKeyGraphQlPost.execute())
407418
.thenReturn(CompletableFuture.completedFuture(productTypesResponse))
419+
.thenReturn(CompletableFuture.completedFuture(taxCategoryResponse))
420+
.thenReturn(CompletableFuture.completedFuture(stateResponse))
408421
.thenReturn(CompletableFuture.completedFuture(mock(ApiHttpResponse.class)));
409422

410423
mockAttributeCustomObjectReference(sourceClient);
@@ -424,9 +437,12 @@ void transform_ProductReferences_ShouldReplaceReferencesIdsWithKeysAndMapToProdu
424437

425438
assertThat(productKey1)
426439
.hasValueSatisfying(
427-
productDraft ->
428-
assertThat(productDraft.getProductType().getKey())
429-
.isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER));
440+
productDraft -> {
441+
assertThat(productDraft.getProductType().getKey())
442+
.isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER);
443+
assertThat(productDraft.getTaxCategory().getKey())
444+
.isEqualTo(BaseTransformServiceImpl.KEY_IS_NOT_SET_PLACE_HOLDER);
445+
});
430446
}
431447

432448
@Test

‎src/test/java/com/commercetools/sync/producttypes/helpers/ProductTypeBatchValidatorTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ void validateAndCollectReferencedKeys_WithEmptyDraft_ShouldHaveEmptyResult() {
264264
attributeTypeBuilder
265265
.nestedBuilder()
266266
.typeReference(
267-
productTypeReferenceBuilder -> productTypeReferenceBuilder.id("")))
267+
productTypeReferenceBuilder -> productTypeReferenceBuilder.id("null")))
268268
.name("invalidNested")
269269
.label(ofEnglish("koko"))
270270
.isRequired(true)
@@ -282,7 +282,7 @@ void validateAndCollectReferencedKeys_WithEmptyDraft_ShouldHaveEmptyResult() {
282282
.nestedBuilder()
283283
.typeReference(
284284
productTypeReferenceBuilder ->
285-
productTypeReferenceBuilder.id(""))))
285+
productTypeReferenceBuilder.id("null"))))
286286
.name("setOfInvalidNested")
287287
.label(ofEnglish("koko"))
288288
.isRequired(true)

0 commit comments

Comments
 (0)