Skip to content

Commit ac20aca

Browse files
ahalberkampJudeNiroshanahmetoz
authored
366: validate asset keys (#509)
* Validate asset keys before syncing (#366) * 366:Fix checkstyle issues. * 366:Add new tests. * 366:Fix checkstyle issues. * Fix test. * 366:Fix checkstyle issues. * Update src/main/java/com/commercetools/sync/commons/utils/AssetsUpdateActionUtils.java Co-Authored-By: Jude Niroshan <jude.niroshan11@gmail.com> * Update src/test/java/com/commercetools/sync/categories/utils/categoryupdateactionutils/BuildAssetsUpdateActionsTest.java Co-Authored-By: Jude Niroshan <jude.niroshan11@gmail.com> * Update src/test/java/com/commercetools/sync/categories/utils/categoryupdateactionutils/BuildAssetsUpdateActionsTest.java Co-Authored-By: Jude Niroshan <jude.niroshan11@gmail.com> * Update src/main/java/com/commercetools/sync/commons/utils/AssetsUpdateActionUtils.java Co-Authored-By: Ahmet Öz <bilmuhahmet@gmail.com> * Update src/main/java/com/commercetools/sync/commons/utils/AssetsUpdateActionUtils.java Co-Authored-By: Ahmet Öz <bilmuhahmet@gmail.com> * 366:Add missing word. * 366:fix typo. * 366:Remove unneeded while line. * 366:Format code. * 366:Improve readability. * 366:Improve readability. * 366:Add final. * 366:Fix checkstyle issues. * 366:Fix tests. Co-authored-by: Andreas Halberkamp <> Co-authored-by: Jude Niroshan <jude.niroshan11@gmail.com> Co-authored-by: Ahmet Öz <bilmuhahmet@gmail.com>
1 parent 7baae24 commit ac20aca

6 files changed

Lines changed: 206 additions & 46 deletions

File tree

‎src/main/java/com/commercetools/sync/categories/utils/CategoryUpdateActionUtils.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ public static List<UpdateAction<Category>> buildAssetsUpdateActions(
251251
return AssetsUpdateActionUtils.buildAssetsUpdateActions(
252252
oldCategory.getAssets(),
253253
newCategory.getAssets(),
254-
new CategoryAssetActionFactory(syncOptions));
254+
new CategoryAssetActionFactory(syncOptions), syncOptions);
255255
} catch (final BuildUpdateActionException exception) {
256256
syncOptions.applyErrorCallback(format("Failed to build update actions for the assets "
257257
+ "of the category with the key '%s'. Reason: %s", oldCategory.getKey(), exception), exception);

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

Lines changed: 47 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
11
package com.commercetools.sync.commons.utils;
22

3+
import com.commercetools.sync.commons.BaseSyncOptions;
34
import com.commercetools.sync.commons.exceptions.BuildUpdateActionException;
45
import com.commercetools.sync.commons.exceptions.DuplicateKeyException;
56
import com.commercetools.sync.commons.helpers.AssetActionFactory;
67
import io.sphere.sdk.commands.UpdateAction;
78
import io.sphere.sdk.models.Asset;
89
import io.sphere.sdk.models.AssetDraft;
910

11+
1012
import javax.annotation.Nonnull;
1113
import javax.annotation.Nullable;
1214
import java.util.ArrayList;
1315
import java.util.Collection;
16+
import java.util.HashMap;
1417
import java.util.HashSet;
1518
import java.util.List;
1619
import java.util.Map;
@@ -21,14 +24,21 @@
2124

2225
import static com.commercetools.sync.commons.utils.CommonTypeUpdateActionUtils.buildUpdateAction;
2326
import static com.commercetools.sync.commons.utils.OptionalUtils.filterEmptyOptionals;
27+
import static java.lang.String.format;
2428
import static java.util.Collections.singletonList;
2529
import static java.util.Optional.ofNullable;
2630
import static java.util.stream.Collectors.toCollection;
2731
import static java.util.stream.Collectors.toList;
2832
import static java.util.stream.Collectors.toMap;
33+
import static org.apache.commons.lang3.StringUtils.isNotBlank;
34+
2935

3036
public final class AssetsUpdateActionUtils {
3137

38+
public static final String ASSET_KEY_NOT_SET = "Asset with %s has no defined key. Keys are required for "
39+
+ "asset matching.";
40+
41+
3242
/**
3343
* Compares a list of {@link Asset}s with a list of {@link AssetDraft}s. The method serves as a generic
3444
* implementation for assets syncing. The method takes in functions for building the required update actions (
@@ -42,6 +52,8 @@ public final class AssetsUpdateActionUtils {
4252
* @param newAssetDrafts the new list of asset drafts.
4353
* @param assetActionFactory factory responsible for building asset update actions.
4454
* @param <T> the type of the resource the asset update actions are built for.
55+
* @param syncOptions responsible for supplying the sync options to the sync utility method.
56+
* It is used for triggering the warn callback within the utility
4557
* @return a list of asset update actions on the resource of type T if the list of assets is not identical.
4658
* Otherwise, if the assets are identical, an empty list is returned.
4759
* @throws BuildUpdateActionException in case there are asset drafts with duplicate keys.
@@ -50,11 +62,13 @@ public final class AssetsUpdateActionUtils {
5062
public static <T> List<UpdateAction<T>> buildAssetsUpdateActions(
5163
@Nonnull final List<Asset> oldAssets,
5264
@Nullable final List<AssetDraft> newAssetDrafts,
53-
@Nonnull final AssetActionFactory<T> assetActionFactory)
65+
@Nonnull final AssetActionFactory<T> assetActionFactory,
66+
@Nonnull final BaseSyncOptions syncOptions)
5467
throws BuildUpdateActionException {
5568

5669
if (newAssetDrafts != null) {
57-
return buildAssetsUpdateActionsWithNewAssetDrafts(oldAssets, newAssetDrafts, assetActionFactory);
70+
return buildAssetsUpdateActionsWithNewAssetDrafts(oldAssets, newAssetDrafts, assetActionFactory,
71+
syncOptions);
5872
} else {
5973
return oldAssets.stream()
6074
.map(Asset::getKey)
@@ -73,6 +87,8 @@ public static <T> List<UpdateAction<T>> buildAssetsUpdateActions(
7387
* @param newAssetDrafts the new list of asset drafts.
7488
* @param assetActionFactory factory responsible for building asset update actions.
7589
* @param <T> the type of the resource the asset update actions are built for.
90+
* @param syncOptions responsible for supplying the sync options to the sync utility method.
91+
* It is used for triggering the warn callback within the utility
7692
* @return a list of asset update actions on the resource of type T if the list of assets is not identical.
7793
* Otherwise, if the assets are identical, an empty list is returned.
7894
* @throws BuildUpdateActionException in case there are asset drafts with duplicate keys.
@@ -81,25 +97,41 @@ public static <T> List<UpdateAction<T>> buildAssetsUpdateActions(
8197
private static <T> List<UpdateAction<T>> buildAssetsUpdateActionsWithNewAssetDrafts(
8298
@Nonnull final List<Asset> oldAssets,
8399
@Nonnull final List<AssetDraft> newAssetDrafts,
84-
@Nonnull final AssetActionFactory<T> assetActionFactory)
100+
@Nonnull final AssetActionFactory<T> assetActionFactory,
101+
@Nonnull final BaseSyncOptions syncOptions)
85102
throws BuildUpdateActionException {
86103

87104
// Asset set that has only the keys of the assets which should be removed, this is used in the method
88105
// #buildChangeAssetOrderUpdateAction in order to compare the state of the asset lists after the remove actions
89106
// have already been applied.
90107
final HashSet<String> removedAssetKeys = new HashSet<>();
91108

92-
final Map<String, Asset> oldAssetsKeyMap = oldAssets.stream().collect(toMap(Asset::getKey, asset -> asset));
109+
final Map<String, Asset> oldAssetsKeyMap = new HashMap<>();
93110

94-
final Map<String, AssetDraft> newAssetDraftsKeyMap;
111+
oldAssets.forEach(asset -> {
112+
String assetKey = asset.getKey();
113+
if (isNotBlank(assetKey)) {
114+
oldAssetsKeyMap.put(assetKey, asset);
115+
} else {
116+
syncOptions.applyWarningCallback(format(ASSET_KEY_NOT_SET, "id: " + asset.getId()));
117+
}
118+
});
119+
final Map<String, AssetDraft> newAssetDraftsKeyMap = new HashMap<>();
95120
try {
96-
newAssetDraftsKeyMap =
97-
newAssetDrafts.stream().collect(
98-
toMap(AssetDraft::getKey, assetDraft -> assetDraft, (assetDraftA, assetDraftB) -> {
121+
newAssetDrafts.forEach(newAsset -> {
122+
String assetKey = newAsset.getKey();
123+
if (isNotBlank(assetKey)) {
124+
newAssetDraftsKeyMap.merge(assetKey, newAsset, (assetDraftA, assetDraftB) -> {
99125
throw new DuplicateKeyException("Supplied asset drafts have duplicate keys. Asset keys are"
100126
+ " expected to be unique inside their container (a product variant or a category).");
101127
}
102-
));
128+
);
129+
130+
} else {
131+
syncOptions.applyWarningCallback(format(ASSET_KEY_NOT_SET, "name: " + newAsset.getName()));
132+
}
133+
});
134+
103135
} catch (final DuplicateKeyException exception) {
104136
throw new BuildUpdateActionException(exception);
105137
}
@@ -149,6 +181,7 @@ private static <T> List<UpdateAction<T>> buildRemoveAssetOrAssetUpdateActions(
149181
// actions.
150182
return oldAssets
151183
.stream()
184+
.filter(asset -> isNotBlank(asset.getKey()))
152185
.map(oldAsset -> {
153186
final String oldAssetKey = oldAsset.getKey();
154187
final AssetDraft matchingNewAssetDraft = newAssetDraftsKeyMap.get(oldAssetKey);
@@ -186,15 +219,18 @@ private static <T> Optional<UpdateAction<T>> buildChangeAssetOrderUpdateAction(
186219
@Nonnull final AssetActionFactory<T> assetActionFactory) {
187220

188221
final Map<String, String> oldAssetKeyToIdMap = oldAssets.stream()
222+
.filter(asset -> isNotBlank(asset.getKey()))
189223
.collect(toMap(Asset::getKey, Asset::getId));
190224

191225
final List<String> newOrder = newAssetDrafts.stream()
226+
.filter(asset -> isNotBlank(asset.getKey()))
192227
.map(AssetDraft::getKey)
193228
.map(oldAssetKeyToIdMap::get)
194229
.filter(Objects::nonNull)
195230
.collect(toList());
196231

197232
final List<String> oldOrder = oldAssets.stream()
233+
.filter(asset -> isNotBlank(asset.getKey()))
198234
.filter(asset -> !removedAssetKeys.contains(asset.getKey()))
199235
.map(Asset::getId)
200236
.collect(toList());
@@ -225,7 +261,8 @@ private static <T> List<UpdateAction<T>> buildAddAssetUpdateActions(
225261
IntStream.range(0, newAssetDrafts.size())
226262
.mapToObj(assetDraftIndex ->
227263
ofNullable(newAssetDrafts.get(assetDraftIndex))
228-
.filter(assetDraft -> !oldAssetsKeyMap.containsKey(assetDraft.getKey()))
264+
.filter(assetDraft -> isNotBlank(assetDraft.getKey())
265+
&& !oldAssetsKeyMap.containsKey(assetDraft.getKey()))
229266
.map(assetDraft -> assetActionFactory.buildAddAssetAction(assetDraft, assetDraftIndex))
230267
)
231268
.collect(toCollection(ArrayList::new));

‎src/main/java/com/commercetools/sync/products/utils/ProductVariantUpdateActionUtils.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,7 @@ public static List<UpdateAction<Product>> buildProductVariantAssetsUpdateActions
253253
return buildAssetsUpdateActions(
254254
oldProductVariant.getAssets(),
255255
newProductVariant.getAssets(),
256-
new ProductAssetActionFactory(oldProductVariant.getId(), syncOptions));
257-
256+
new ProductAssetActionFactory(oldProductVariant.getId(), syncOptions), syncOptions);
258257
} catch (final BuildUpdateActionException exception) {
259258
syncOptions.applyErrorCallback(format("Failed to build update actions for the assets "
260259
+ "of the product variant with the sku '%s'. Reason: %s", oldProductVariant.getSku(), exception),

0 commit comments

Comments
 (0)