300 type sync review

config/checkstyle/checkstyle.xml

@@ -72,7 +72,8 @@
             <property name="excludes" value="com.commercetools.sync.products.utils.productvariantupdateactionutils.prices.PriceDraftFixtures,
             com.commercetools.sync.products.utils.productvariantupdateactionutils.prices.PriceFixtures,
             com.commercetools.sync.commons.utils.LocalizedEnumValueFixtures,
-            com.commercetools.sync.commons.utils.PlainEnumValueFixtures"/>
+            com.commercetools.sync.commons.utils.PlainEnumValueFixtures,
+            com.commercetools.sync.types.utils.FieldDefinitionFixtures"/>
             <property name="allowClassImports" value="false"/>
             <property name="allowStaticMemberImports" value="false"/>
         </module>

docs/README.md

@@ -30,7 +30,7 @@ Currently this library supports synchronising the following entities in commerce
  - [InventoryEntries](usage/INVENTORY_SYNC.md)
  - [ProductTypes](usage/PRODUCT_TYPE_SYNC.md)
  - [Types](usage/TYPE_SYNC.md)
- 
+
 ![commercetools-java-sync-final 001](https://user-images.githubusercontent.com/9512131/31230702-0f2255a6-a9e5-11e7-9412-04ed52641dde.png)
 
 

docs/RELEASE_NOTES.md

@@ -41,11 +41,6 @@
     - **Type Sync** - Support for syncing types. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300) For more info how to use it please refer to [Type usage doc](/docs/usage/TYPE_SYNC.md). 
     - **Type Sync** - Exposed `TypeSyncUtils#buildActions` which calculates all needed update actions after comparing a `Type` and a `TypeDraft`. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
     - **Type Sync** - Exposed `TypeUpdateActionUtils` which contains utils for calculating needed update actions after comparing individual fields of a `Type` and a `TypeDraft`. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
-    - **Type Sync** - Exposed `LocalizedEnumValueUpdateActionUtils` which contains utils for calculating needed update actions after comparing two lists of `LocalizedEnumValue`s. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
-    - **Type Sync** - Exposed `PlainEnumValueUpdateActionUtils` which contains utils for calculating needed update actions after comparing two lists of `EnumValue`s. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
-    - **Type Sync** - Exposed `FieldDefinitionsUpdateActionUtils` which contains utils for calculating needed update actions after comparing a list of `FieldDefinition`s and a list of `FieldDefinition`s. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
-    - **Type Sync** - Exposed `FieldDefinitionUpdateActionUtils` which contains utils for calculating needed update actions after comparing a `FieldDefinition` and a `FieldDefinition`. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
-    - **Commons** - Exposed `EnumValuesUpdateActionUtils` which contains generic utilities for calculating needed update actions after comparing two lists of `EnumValue`s or `LocalizedEnumValue`s. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
 
 - 📋 **Documentation** (4)
     - **Commons** - Added the documentation github pages. https://commercetools.github.io/commercetools-sync-java 
@@ -60,7 +55,7 @@
     - **Product Sync** - Reference keys are not validated if they are in UUID format anymore. [#166](https://github.com/commercetools/commercetools-sync-java/issues/166)
     - **Category Sync** - Reference keys are not validated if they are in UUID format anymore. [#166](https://github.com/commercetools/commercetools-sync-java/issues/166)
     - **Inventory Sync** - Reference keys are not validated if they are in UUID format anymore. [#166](https://github.com/commercetools/commercetools-sync-java/issues/166)
-    - **ProductType Sync** - Added `ProductTypeSyncBenchmark` to benchmark the product type sync, to be able to compare the performance of the sync with the future releases. [#301](https://github.com/commercetools/commercetools-sync-java/issues/301)
+    - **ProductType Sync** - Added benchmarks for the `productType` sync to be able to compare the performance of the sync with the future releases. [#301](https://github.com/commercetools/commercetools-sync-java/issues/301)
     - **Commons** - Bumped commercetools-jvm-sdk to version [1.37.0](http://commercetools.github.io/commercetools-jvm-sdk/apidocs/io/sphere/sdk/meta/ReleaseNotes.html#v1_37_0).
     - **Commons** - Bumped `mockito` to 2.23.4.
     - **Commons** - Bumped `com.adarshr.test-logger` to 1.6.0.
@@ -71,7 +66,7 @@
     - **Commons** - Bumped `com.adarshr.test-logger` to 1.6.0.
     - **Commons** - Bumped `org.ajoberstar.grgit` to 3.0.0.
     - **Commons** - Bumped gradle to version [gradle-5.0](https://docs.gradle.org/5.0/release-notes.html)
-    - **Type Sync** - Added `TypeSyncBenchmark` to benchmark the type sync, to be able to compare the performance of the sync with the future releases. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
+    - **Type Sync** - Added benchmarks for the `type` sync to be able to compare the performance of the sync with the future releases. [#300](https://github.com/commercetools/commercetools-sync-java/issues/300)
 
 - 🚧 **Breaking Changes** (11) 
     - **Product Sync** - `allowUuid` option is now removed. [#166](https://github.com/commercetools/commercetools-sync-java/issues/166) 

docs/usage/PRODUCT_TYPE_SYNC.md

@@ -12,6 +12,8 @@ against a [ProductTypeDraft](https://docs.commercetools.com/http-api-projects-pr
   - [Sync list of product type drafts](#sync-list-of-product-type-drafts)
     - [Prerequisites](#prerequisites)
     - [Running the sync](#running-the-sync)
+    - [Important to Note](#important-to-note)
+    - [More examples of how to use the sync](#more-examples-of-how-to-use-the-sync)
   - [Build all update actions](#build-all-update-actions)
   - [Build particular update action(s)](#build-particular-update-actions)
 - [Caveats](#caveats)
@@ -64,8 +66,15 @@ __Note__ The statistics object contains the processing time of the last batch on
 
  1. The sync processing time should not take into account the time between supplying batches to the sync.
  2. It is not known by the sync which batch is going to be the last one supplied.
+
+#### Important to Note
+
+1. If two matching `attributeDefinition`s (old and new) on the matching `productType`s (old and new) have a different `AttributeType`, the sync will
+**remove** the existing `attributeDefinition` and then **add** a new `attributeDefinition` with the new `AttributeType`.
+
+2. The `attributeDefinition` for which the `AttributeType` is not defined (`null`) will not be synced. 
 
-##### More examples of how to use the sync
+#### More examples of how to use the sync
 
  1. [Sync from another CTP project as a source](https://github.com/commercetools/commercetools-sync-java/tree/master/src/integration-test/java/com/commercetools/sync/integration/ctpprojectsource/producttypes/ProductTypeSyncIT.java).
  2. [Sync from an external source](https://github.com/commercetools/commercetools-sync-java/tree/master/src/integration-test/java/com/commercetools/sync/integration/externalsource/producttypes/ProductTypeSyncIT.java).

docs/usage/TYPE_SYNC.md

@@ -12,6 +12,8 @@ against a [TypeDraft](https://docs.commercetools.com/http-api-projects-types.htm
   - [Sync list of type drafts](#sync-list-of-type-drafts)
     - [Prerequisites](#prerequisites)
     - [Running the sync](#running-the-sync)
+    - [Important to Note](#important-to-note)
+    - [More examples of how to use the sync](#more-examples-of-how-to-use-the-sync)
   - [Build all update actions](#build-all-update-actions)
   - [Build particular update action(s)](#build-particular-update-actions)
 - [Caveats](#caveats)
@@ -62,7 +64,13 @@ __Note__ The statistics object contains the processing time of the last batch on
  1. The sync processing time should not take into account the time between supplying batches to the sync.
  2. It is not known by the sync which batch is going to be the last one supplied.
 
-##### More examples of how to use the sync
+#### Important to Note
+1. If two matching `fieldDefinition`s (old and new) on the matching `type`s (old and new) have a different `FieldType`, the sync will
+**remove** the existing `fieldDefinition` and then **add** a new `fieldDefinition` with the new `FieldType`.
+
+2. The `fieldDefinition` for which the `fieldType` is not defined (`null`) will not be synced.
+
+#### More examples of how to use the sync
 
  1. [Sync from another CTP project as a source](https://github.com/commercetools/commercetools-sync-java/tree/master/src/integration-test/java/com/commercetools/sync/integration/ctpprojectsource/types/TypeSyncIT.java).
  2. [Sync from an external source](https://github.com/commercetools/commercetools-sync-java/tree/master/src/integration-test/java/com/commercetools/sync/integration/externalsource/types/TypeSyncIT.java).

src/benchmark/java/com/commercetools/sync/benchmark/ProductSyncBenchmark.java

@@ -158,7 +158,7 @@ public void sync_ExistingProducts_ShouldUpdateProducts() throws IOException {
 
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_SYNC, CREATES_ONLY, totalTime);
+        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_SYNC, UPDATES_ONLY, totalTime);
         assertThat(diff)
             .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.", diff,
                 THRESHOLD))
@@ -222,7 +222,7 @@ public void sync_WithSomeExistingProducts_ShouldSyncProducts() throws IOExceptio
 
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_SYNC, CREATES_ONLY, totalTime);
+        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_SYNC, CREATES_AND_UPDATES, totalTime);
         assertThat(diff)
             .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.", diff,
                 THRESHOLD))

src/benchmark/java/com/commercetools/sync/benchmark/ProductTypeSyncBenchmark.java

@@ -1,6 +1,5 @@
 package com.commercetools.sync.benchmark;
 
-import com.commercetools.sync.commons.asserts.statistics.AssertionsForStatistics;
 import com.commercetools.sync.commons.utils.SyncSolutionInfo;
 import com.commercetools.sync.producttypes.ProductTypeSync;
 import com.commercetools.sync.producttypes.ProductTypeSyncOptions;
@@ -36,6 +35,7 @@
 import static com.commercetools.sync.benchmark.BenchmarkUtils.UPDATES_ONLY;
 import static com.commercetools.sync.benchmark.BenchmarkUtils.calculateDiff;
 import static com.commercetools.sync.benchmark.BenchmarkUtils.saveNewResult;
+import static com.commercetools.sync.commons.asserts.statistics.AssertionsForStatistics.assertThat;
 import static com.commercetools.sync.integration.commons.utils.ProductTypeITUtils.ATTRIBUTE_DEFINITION_DRAFT_1;
 import static com.commercetools.sync.integration.commons.utils.ProductTypeITUtils.deleteProductTypes;
 import static com.commercetools.sync.integration.commons.utils.SphereClientUtils.CTP_TARGET_CLIENT;
@@ -111,9 +111,7 @@ public void sync_NewProductTypes_ShouldCreateProductTypes() throws IOException {
         assertThat(totalNumberOfProductTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
 
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, NUMBER_OF_RESOURCE_UNDER_TEST, 0, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, NUMBER_OF_RESOURCE_UNDER_TEST, 0, 0);
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();
         assertThat(warningCallBackMessages).isEmpty();
@@ -144,7 +142,7 @@ public void sync_ExistingProductTypes_ShouldUpdateProductTypes() throws IOExcept
         final long totalTime = System.currentTimeMillis() - beforeSyncTime;
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_TYPE_SYNC, CREATES_ONLY, totalTime);
+        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_TYPE_SYNC, UPDATES_ONLY, totalTime);
         assertThat(diff)
                 .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.",
                         diff, THRESHOLD))
@@ -172,9 +170,7 @@ public void sync_ExistingProductTypes_ShouldUpdateProductTypes() throws IOExcept
         assertThat(totalNumberOfProductTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
         // Assert statistics
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, 0, NUMBER_OF_RESOURCE_UNDER_TEST, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, 0, NUMBER_OF_RESOURCE_UNDER_TEST, 0);
 
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();
@@ -207,7 +203,9 @@ public void sync_WithSomeExistingProductTypes_ShouldSyncProductTypes() throws IO
         final long totalTime = System.currentTimeMillis() - beforeSyncTime;
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_TYPE_SYNC, CREATES_ONLY, totalTime);
+        final double diff =
+            calculateDiff(SyncSolutionInfo.LIB_VERSION, PRODUCT_TYPE_SYNC, CREATES_AND_UPDATES, totalTime);
+
         assertThat(diff)
                 .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.",
                         diff, THRESHOLD))
@@ -235,9 +233,7 @@ public void sync_WithSomeExistingProductTypes_ShouldSyncProductTypes() throws IO
         assertThat(totalNumberOfProductTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
         // Assert statistics
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, halfNumberOfDrafts, halfNumberOfDrafts, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, halfNumberOfDrafts, halfNumberOfDrafts, 0);
 
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();

src/benchmark/java/com/commercetools/sync/benchmark/TypeSyncBenchmark.java

@@ -1,6 +1,5 @@
 package com.commercetools.sync.benchmark;
 
-import com.commercetools.sync.commons.asserts.statistics.AssertionsForStatistics;
 import com.commercetools.sync.commons.utils.SyncSolutionInfo;
 import com.commercetools.sync.types.TypeSync;
 import com.commercetools.sync.types.TypeSyncOptions;
@@ -38,7 +37,7 @@
 import static com.commercetools.sync.benchmark.BenchmarkUtils.UPDATES_ONLY;
 import static com.commercetools.sync.benchmark.BenchmarkUtils.calculateDiff;
 import static com.commercetools.sync.benchmark.BenchmarkUtils.saveNewResult;
-import static com.commercetools.sync.integration.commons.utils.ITUtils.deleteTypesFromTargetAndSource;
+import static com.commercetools.sync.commons.asserts.statistics.AssertionsForStatistics.assertThat;
 import static com.commercetools.sync.integration.commons.utils.SphereClientUtils.CTP_TARGET_CLIENT;
 import static com.commercetools.sync.integration.commons.utils.TypeITUtils.FIELD_DEFINITION_1;
 import static com.commercetools.sync.integration.commons.utils.TypeITUtils.FIELD_DEFINITION_NAME_1;
@@ -57,7 +56,7 @@ public class TypeSyncBenchmark {
 
     @BeforeClass
     public static void setup() {
-        deleteTypesFromTargetAndSource();
+        deleteTypes(CTP_TARGET_CLIENT);
     }
 
     @AfterClass
@@ -120,9 +119,7 @@ public void sync_NewTypes_ShouldCreateTypes() throws IOException {
         assertThat(totalNumberOfTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
 
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, NUMBER_OF_RESOURCE_UNDER_TEST, 0, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, NUMBER_OF_RESOURCE_UNDER_TEST, 0, 0);
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();
         assertThat(warningCallBackMessages).isEmpty();
@@ -152,7 +149,7 @@ public void sync_ExistingTypes_ShouldUpdateTypes() throws IOException {
         final long totalTime = System.currentTimeMillis() - beforeSyncTime;
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, TYPE_SYNC, CREATES_ONLY, totalTime);
+        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, TYPE_SYNC, UPDATES_ONLY, totalTime);
         assertThat(diff)
                 .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.",
                         diff, THRESHOLD))
@@ -180,9 +177,7 @@ public void sync_ExistingTypes_ShouldUpdateTypes() throws IOException {
         assertThat(totalNumberOfTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
         // Assert statistics
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, 0, NUMBER_OF_RESOURCE_UNDER_TEST, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, 0, NUMBER_OF_RESOURCE_UNDER_TEST, 0);
 
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();
@@ -215,23 +210,23 @@ public void sync_WithSomeExistingTypes_ShouldSyncTypes() throws IOException {
         final long totalTime = System.currentTimeMillis() - beforeSyncTime;
 
         // Calculate time taken for benchmark and assert it lies within threshold
-        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, TYPE_SYNC, CREATES_ONLY, totalTime);
+        final double diff = calculateDiff(SyncSolutionInfo.LIB_VERSION, TYPE_SYNC, CREATES_AND_UPDATES, totalTime);
         assertThat(diff)
                 .withFailMessage(format("Diff of benchmark '%e' is longer than expected threshold of '%d'.",
                         diff, THRESHOLD))
                 .isLessThanOrEqualTo(THRESHOLD);
 
         // Assert actual state of CTP project (number of updated types)
-        final CompletableFuture<Integer> totalNumberOfUpdatedTypesWithOldFielDefinitionName =
+        final CompletableFuture<Integer> totalNumberOfUpdatedTypesWithOldFieldDefinitionName =
                 CTP_TARGET_CLIENT.execute(TypeQuery.of()
                                                    .withPredicates(p -> p.fieldDefinitions().name().is(
                                                            FIELD_DEFINITION_NAME_1 + "_old")))
                                  .thenApply(PagedQueryResult::getTotal)
                                  .thenApply(Long::intValue)
                                  .toCompletableFuture();
 
-        executeBlocking(totalNumberOfUpdatedTypesWithOldFielDefinitionName);
-        assertThat(totalNumberOfUpdatedTypesWithOldFielDefinitionName).isCompletedWithValue(0);
+        executeBlocking(totalNumberOfUpdatedTypesWithOldFieldDefinitionName);
+        assertThat(totalNumberOfUpdatedTypesWithOldFieldDefinitionName).isCompletedWithValue(0);
 
         // Assert actual state of CTP project (total number of existing types)
         final CompletableFuture<Integer> totalNumberOfTypes =
@@ -243,9 +238,7 @@ public void sync_WithSomeExistingTypes_ShouldSyncTypes() throws IOException {
         assertThat(totalNumberOfTypes).isCompletedWithValue(NUMBER_OF_RESOURCE_UNDER_TEST);
 
         // Assert statistics
-        AssertionsForStatistics
-                .assertThat(syncStatistics)
-                .hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, halfNumberOfDrafts, halfNumberOfDrafts, 0);
+        assertThat(syncStatistics).hasValues(NUMBER_OF_RESOURCE_UNDER_TEST, halfNumberOfDrafts, halfNumberOfDrafts, 0);
 
         assertThat(errorCallBackExceptions).isEmpty();
         assertThat(errorCallBackMessages).isEmpty();