Skip to content

Commit 5dde47b

Browse files
heshamMassoudheshamMassoud
authored
Merge pull request #363 from commercetools/359-empty-list-filter-for-beforeUpdateCallback
#359 add empty check for applyBeforeCallback
2 parents 9d997d9 + 6df67d4 commit 5dde47b

7 files changed

Lines changed: 130 additions & 17 deletions

File tree

‎docs/RELEASE_NOTES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
- **ProductType Sync** - `ProductTypeSyncUtils#buildActions`, `ProductTypeUpdateActionUtils#buildAttributesUpdateActions`
4747
now treat the values of the optional fields `isSearchable`, `inputHint` and `attributeConstraint`
4848
as (`true`, `SingleLine` and `None` respectivley) if they are `null` or not passed. [#354](https://github.com/commercetools/commercetools-sync-java/issues/354)
49+
- **Commons** - Fixed a bug in the `beforeUpdateCallback` which caused the callback to be called even on an empty list of update actions. [#359](https://github.com/commercetools/commercetools-sync-java/issues/359)
4950
5051
- 🛠️ **Enhancements** (1)
5152
- **Commons** - Benchmarks are now run once on every merge to `master` with a lower number of resources for faster benchmarking. [#246](https://github.com/commercetools/commercetools-sync-java/issues/246)

‎src/benchmark/java/com/commercetools/sync/benchmark/BenchmarkUtils.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ final class BenchmarkUtils {
2222
private static final Charset UTF8_CHARSET = StandardCharsets.UTF_8;
2323
private static final String EXECUTION_TIME = "executionTime";
2424
private static final String BRANCH_NAME = ofNullable(System.getenv("TRAVIS_COMMIT"))
25+
.map(commitMessage -> commitMessage.substring(0, 7)) //Use smaller commit sha
2526
.orElse("dev-local");
2627

2728
static final String PRODUCT_SYNC = "productSync";

‎src/main/java/com/commercetools/sync/commons/BaseSyncOptions.java‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -161,24 +161,27 @@ public Function<V, V> getBeforeCreateCallback() {
161161

162162
/**
163163
* Given a {@link List} of {@link UpdateAction}, a new resource draft of type {@code V} and the old existing
164-
* resource of the type {@code U}, this method applies the {@code beforeUpdateCallback} function
165-
* which is set to {@code this} instance of the {@link BaseSyncOptions} and returns the result. If the
166-
* {@code beforeUpdateCallback} is null, this method does nothing to the supplied list of update actions and returns
167-
* the same list. If the result of the callback is null, an empty list is returned.
164+
* resource of the type {@code U}, this method applies the {@code beforeUpdateCallback} function which is set to
165+
* {@code this} instance of the {@link BaseSyncOptions} and returns the result. If the {@code beforeUpdateCallback}
166+
* is null or {@code updateActions} is empty, this method does nothing to the supplied list of {@code updateActions}
167+
* and returns the same list. If the result of the callback is null, an empty list is returned.
168168
*
169169
* @param updateActions the list of update actions to apply the {@code beforeUpdateCallback} function on.
170170
* @param newResourceDraft the new resource draft that is being compared to the old resource.
171171
* @param oldResource the old resource that is being compared to the new draft.
172172
* @return a list of update actions after applying the {@code beforeUpdateCallback} function on. If the
173-
* {@code beforeUpdateCallback} function was null, the supplied list of {@code updateActions} is returned as
174-
* is. If the return of the callback was null, an empty list is returned.
173+
* {@code beforeUpdateCallback} function is null or {@code updateActions} is empty, the supplied list of
174+
* {@code updateActions} is returned as is. If the return of the callback is null, an empty list is
175+
* returned.
175176
*/
176177
@Nonnull
177178
public List<UpdateAction<U>> applyBeforeUpdateCallBack(@Nonnull final List<UpdateAction<U>> updateActions,
178179
@Nonnull final V newResourceDraft,
179180
@Nonnull final U oldResource) {
181+
180182
return ofNullable(beforeUpdateCallback)
181-
.map(callBack -> emptyIfNull(callBack.apply(updateActions, newResourceDraft, oldResource)))
183+
.filter(callback -> !updateActions.isEmpty())
184+
.map(filteredCallback -> emptyIfNull(filteredCallback.apply(updateActions, newResourceDraft, oldResource)))
182185
.orElse(updateActions);
183186
}
184187

@@ -194,7 +197,7 @@ public List<UpdateAction<U>> applyBeforeUpdateCallBack(@Nonnull final List<Updat
194197
*
195198
* @param newResourceDraft the new resource draft that should be created.
196199
* @return an optional containing the resultant resource draft after applying the {@code beforeCreateCallback}
197-
* function on. If the {@code beforeCreateCallback} function was null, the supplied resource draft is
200+
* function on. If the {@code beforeCreateCallback} function is null, the supplied resource draft is
198201
* returned as is, wrapped in an optional.
199202
*/
200203
@Nonnull

‎src/test/java/com/commercetools/sync/categories/CategorySyncOptionsBuilderTest.java‎

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,23 @@
99
import io.sphere.sdk.commands.UpdateAction;
1010
import org.junit.Test;
1111

12-
import java.util.Collections;
1312
import java.util.List;
1413
import java.util.Optional;
1514
import java.util.function.BiConsumer;
1615
import java.util.function.Consumer;
1716
import java.util.function.Function;
1817

1918
import static io.sphere.sdk.models.LocalizedString.ofEnglish;
19+
import static java.util.Collections.emptyList;
2020
import static java.util.Collections.singletonList;
2121
import static org.assertj.core.api.Assertions.assertThat;
22+
import static org.mockito.ArgumentMatchers.any;
2223
import static org.mockito.Mockito.mock;
24+
import static org.mockito.Mockito.never;
25+
import static org.mockito.Mockito.verify;
2326
import static org.mockito.Mockito.when;
2427

28+
2529
public class CategorySyncOptionsBuilderTest {
2630
private static final SphereClient CTP_CLIENT = mock(SphereClient.class);
2731
private CategorySyncOptionsBuilder categorySyncOptionsBuilder = CategorySyncOptionsBuilder.of(CTP_CLIENT);
@@ -47,7 +51,7 @@ public void build_WithClient_ShouldBuildCategorySyncOptions() {
4751
@Test
4852
public void beforeUpdateCallback_WithFilterAsCallback_ShouldSetCallback() {
4953
final TriFunction<List<UpdateAction<Category>>, CategoryDraft, Category, List<UpdateAction<Category>>>
50-
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> Collections.emptyList();
54+
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> emptyList();
5155
categorySyncOptionsBuilder.beforeUpdateCallback(beforeUpdateCallback);
5256

5357
final CategorySyncOptions categorySyncOptions = categorySyncOptionsBuilder.build();
@@ -96,7 +100,7 @@ public void categorySyncOptionsBuilderSetters_ShouldBeCallableAfterBaseSyncOptio
96100
final CategorySyncOptions categorySyncOptions = CategorySyncOptionsBuilder
97101
.of(CTP_CLIENT)
98102
.batchSize(30)
99-
.beforeUpdateCallback((updateActions, newCategory, oldCategory) -> Collections.emptyList())
103+
.beforeUpdateCallback((updateActions, newCategory, oldCategory) -> emptyList())
100104
.beforeCreateCallback(newCategoryDraft -> null)
101105
.build();
102106
assertThat(categorySyncOptions).isNotNull();
@@ -141,7 +145,7 @@ public void applyBeforeUpdateCallBack_WithNullCallback_ShouldReturnIdenticalList
141145
@Test
142146
public void applyBeforeUpdateCallBack_WithCallback_ShouldReturnFilteredList() {
143147
final TriFunction<List<UpdateAction<Category>>, CategoryDraft, Category, List<UpdateAction<Category>>>
144-
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> Collections.emptyList();
148+
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> emptyList();
145149

146150
final CategorySyncOptions categorySyncOptions = CategorySyncOptionsBuilder.of(CTP_CLIENT)
147151
.beforeUpdateCallback(
@@ -174,6 +178,29 @@ public void applyBeforeUpdateCallBack_WithNullReturnCallback_ShouldReturnEmptyLi
174178
assertThat(filteredList).isEmpty();
175179
}
176180

181+
private interface MockTriFunction extends
182+
TriFunction<List<UpdateAction<Category>>, CategoryDraft, Category, List<UpdateAction<Category>>> {
183+
}
184+
185+
@Test
186+
public void applyBeforeUpdateCallBack_WithEmptyUpdateActions_ShouldNotApplyBeforeUpdateCallback() {
187+
final MockTriFunction beforeUpdateCallback = mock(MockTriFunction.class);
188+
189+
final CategorySyncOptions categorySyncOptions =
190+
CategorySyncOptionsBuilder.of(CTP_CLIENT)
191+
.beforeUpdateCallback(beforeUpdateCallback)
192+
.build();
193+
194+
assertThat(categorySyncOptions.getBeforeUpdateCallback()).isNotNull();
195+
196+
final List<UpdateAction<Category>> updateActions = emptyList();
197+
final List<UpdateAction<Category>> filteredList = categorySyncOptions
198+
.applyBeforeUpdateCallBack(updateActions, mock(CategoryDraft.class), mock(Category.class));
199+
200+
assertThat(filteredList).isEmpty();
201+
verify(beforeUpdateCallback, never()).apply(any(), any(), any());
202+
}
203+
177204
@Test
178205
public void applyBeforeCreateCallBack_WithNullCallback_ShouldReturnIdenticalDraft() {
179206
final CategorySyncOptions categorySyncOptions = CategorySyncOptionsBuilder.of(CTP_CLIENT)
@@ -218,7 +245,7 @@ public void applyBeforeCreateCallBack_WithNullReturnCallback_ShouldReturnEmptyLi
218245

219246
final CategoryDraft resourceDraft = mock(CategoryDraft.class);
220247
final Optional<CategoryDraft> filteredDraft = syncOptions.applyBeforeCreateCallBack(resourceDraft);
221-
248+
222249
assertThat(filteredDraft).isEmpty();
223250
}
224251
}

‎src/test/java/com/commercetools/sync/products/ProductSyncOptionsBuilderTest.java‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,12 @@
1919
import static com.commercetools.sync.products.ActionGroup.IMAGES;
2020
import static com.commercetools.sync.products.SyncFilter.ofWhiteList;
2121
import static io.sphere.sdk.models.LocalizedString.ofEnglish;
22+
import static java.util.Collections.emptyList;
2223
import static org.assertj.core.api.Assertions.assertThat;
24+
import static org.mockito.ArgumentMatchers.any;
2325
import static org.mockito.Mockito.mock;
26+
import static org.mockito.Mockito.never;
27+
import static org.mockito.Mockito.verify;
2428
import static org.mockito.Mockito.when;
2529

2630
public class ProductSyncOptionsBuilderTest {
@@ -65,7 +69,7 @@ public void syncFilter_WithSyncFilter_ShouldSetFilter() {
6569
@Test
6670
public void beforeUpdateCallback_WithFilterAsCallback_ShouldSetCallback() {
6771
final TriFunction<List<UpdateAction<Product>>, ProductDraft, Product, List<UpdateAction<Product>>>
68-
beforeUpdateCallback = (updateActions, newProduct, oldProduct) -> Collections.emptyList();
72+
beforeUpdateCallback = (updateActions, newProduct, oldProduct) -> emptyList();
6973
productSyncOptionsBuilder.beforeUpdateCallback(beforeUpdateCallback);
7074

7175
final ProductSyncOptions productSyncOptions = productSyncOptionsBuilder.build();
@@ -114,7 +118,7 @@ public void productSyncOptionsBuilderSetters_ShouldBeCallableAfterBaseSyncOption
114118
.of(CTP_CLIENT)
115119
.batchSize(30)
116120
.beforeCreateCallback((newProduct) -> null)
117-
.beforeUpdateCallback((updateActions, newCategory, oldCategory) -> Collections.emptyList())
121+
.beforeUpdateCallback((updateActions, newCategory, oldCategory) -> emptyList())
118122
.build();
119123
assertThat(productSyncOptions).isNotNull();
120124
}
@@ -173,10 +177,33 @@ public void applyBeforeUpdateCallBack_WithNullReturnCallback_ShouldReturnEmptyLi
173177
assertThat(filteredList).isEmpty();
174178
}
175179

180+
private interface MockTriFunction extends
181+
TriFunction<List<UpdateAction<Product>>, ProductDraft, Product, List<UpdateAction<Product>>> {
182+
}
183+
184+
@Test
185+
public void applyBeforeUpdateCallBack_WithEmptyUpdateActions_ShouldNotApplyBeforeUpdateCallback() {
186+
final MockTriFunction beforeUpdateCallback = mock(MockTriFunction.class);
187+
188+
final ProductSyncOptions productSyncOptions =
189+
ProductSyncOptionsBuilder.of(CTP_CLIENT)
190+
.beforeUpdateCallback(beforeUpdateCallback)
191+
.build();
192+
193+
assertThat(productSyncOptions.getBeforeUpdateCallback()).isNotNull();
194+
195+
final List<UpdateAction<Product>> updateActions = emptyList();
196+
final List<UpdateAction<Product>> filteredList =
197+
productSyncOptions.applyBeforeUpdateCallBack(updateActions, mock(ProductDraft.class), mock(Product.class));
198+
199+
assertThat(filteredList).isEmpty();
200+
verify(beforeUpdateCallback, never()).apply(any(), any(), any());
201+
}
202+
176203
@Test
177204
public void applyBeforeUpdateCallBack_WithCallback_ShouldReturnFilteredList() {
178205
final TriFunction<List<UpdateAction<Product>>, ProductDraft, Product, List<UpdateAction<Product>>>
179-
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> Collections.emptyList();
206+
beforeUpdateCallback = (updateActions, newCategory, oldCategory) -> emptyList();
180207
final ProductSyncOptions productSyncOptions = ProductSyncOptionsBuilder.of(CTP_CLIENT)
181208
.beforeUpdateCallback(
182209
beforeUpdateCallback)

‎src/test/java/com/commercetools/sync/producttypes/ProductTypeSyncOptionsBuilderTest.java‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,13 @@
1616
import java.util.function.Consumer;
1717
import java.util.function.Function;
1818

19+
import static java.util.Collections.emptyList;
1920
import static java.util.Collections.singletonList;
2021
import static org.assertj.core.api.Assertions.assertThat;
22+
import static org.mockito.ArgumentMatchers.any;
2123
import static org.mockito.Mockito.mock;
24+
import static org.mockito.Mockito.never;
25+
import static org.mockito.Mockito.verify;
2226
import static org.mockito.Mockito.when;
2327

2428
public class ProductTypeSyncOptionsBuilderTest {
@@ -160,6 +164,30 @@ public void applyBeforeUpdateCallBack_WithNullReturnCallback_ShouldReturnEmptyLi
160164
assertThat(filteredList).isEmpty();
161165
}
162166

167+
private interface MockTriFunction extends
168+
TriFunction<List<UpdateAction<ProductType>>, ProductTypeDraft, ProductType, List<UpdateAction<ProductType>>> {
169+
}
170+
171+
@Test
172+
public void applyBeforeUpdateCallBack_WithEmptyUpdateActions_ShouldNotApplyBeforeUpdateCallback() {
173+
final MockTriFunction beforeUpdateCallback = mock(MockTriFunction.class);
174+
175+
final ProductTypeSyncOptions productTypeSyncOptions =
176+
ProductTypeSyncOptionsBuilder.of(CTP_CLIENT)
177+
.beforeUpdateCallback(beforeUpdateCallback)
178+
.build();
179+
180+
assertThat(productTypeSyncOptions.getBeforeUpdateCallback()).isNotNull();
181+
182+
final List<UpdateAction<ProductType>> updateActions = emptyList();
183+
final List<UpdateAction<ProductType>> filteredList =
184+
productTypeSyncOptions.applyBeforeUpdateCallBack(updateActions,
185+
mock(ProductTypeDraft.class), mock(ProductType.class));
186+
187+
assertThat(filteredList).isEmpty();
188+
verify(beforeUpdateCallback, never()).apply(any(), any(), any());
189+
}
190+
163191
@Test
164192
public void applyBeforeUpdateCallBack_WithCallback_ShouldReturnFilteredList() {
165193
final TriFunction<List<UpdateAction<ProductType>>,
@@ -195,7 +223,7 @@ public void applyBeforeCreateCallBack_WithCallback_ShouldReturnFilteredDraft() {
195223

196224
final Optional<ProductTypeDraft> filteredDraft =
197225
productTypeSyncOptions.applyBeforeCreateCallBack(resourceDraft);
198-
226+
199227
assertThat(filteredDraft).hasValueSatisfying(productTypeDraft ->
200228
assertThat(productTypeDraft.getKey()).isEqualTo("myKey_filteredKey"));
201229

‎src/test/java/com/commercetools/sync/types/TypeSyncOptionsBuilderTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@
1919
import static java.util.Collections.emptyList;
2020
import static java.util.Collections.singletonList;
2121
import static org.assertj.core.api.Assertions.assertThat;
22+
import static org.mockito.ArgumentMatchers.any;
2223
import static org.mockito.Mockito.mock;
24+
import static org.mockito.Mockito.never;
25+
import static org.mockito.Mockito.verify;
2326
import static org.mockito.Mockito.when;
2427

2528
public class TypeSyncOptionsBuilderTest {
@@ -158,6 +161,29 @@ public void applyBeforeUpdateCallBack_WithNullReturnCallback_ShouldReturnEmptyLi
158161
assertThat(filteredList).isEmpty();
159162
}
160163

164+
private interface MockTriFunction extends
165+
TriFunction<List<UpdateAction<Type>>, TypeDraft, Type, List<UpdateAction<Type>>> {
166+
}
167+
168+
@Test
169+
public void applyBeforeUpdateCallBack_WithEmptyUpdateActions_ShouldNotApplyBeforeUpdateCallback() {
170+
final MockTriFunction beforeUpdateCallback = mock(MockTriFunction.class);
171+
172+
final TypeSyncOptions typeSyncOptions =
173+
TypeSyncOptionsBuilder.of(CTP_CLIENT)
174+
.beforeUpdateCallback(beforeUpdateCallback)
175+
.build();
176+
177+
assertThat(typeSyncOptions.getBeforeUpdateCallback()).isNotNull();
178+
179+
final List<UpdateAction<Type>> updateActions = emptyList();
180+
final List<UpdateAction<Type>> filteredList =
181+
typeSyncOptions.applyBeforeUpdateCallBack(updateActions, mock(TypeDraft.class), mock(Type.class));
182+
183+
assertThat(filteredList).isEmpty();
184+
verify(beforeUpdateCallback, never()).apply(any(), any(), any());
185+
}
186+
161187
@Test
162188
public void applyBeforeUpdateCallBack_WithCallback_ShouldReturnFilteredList() {
163189
final TriFunction<List<UpdateAction<Type>>, TypeDraft, Type, List<UpdateAction<Type>>>

0 commit comments

Comments
 (0)