#40: don't call Sphere API if update actions list is empty

src/main/java/com/commercetools/service/ctp/TypeService.java

@@ -5,6 +5,7 @@
 import io.sphere.sdk.types.TypeDraft;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.util.List;
 import java.util.concurrent.CompletionStage;
 
@@ -13,5 +14,5 @@ public interface TypeService {
 
     CompletionStage<Type> createType(@Nonnull TypeDraft typeDraft);
 
-    CompletionStage<Type> updateType(@Nonnull Type type, @Nonnull List<UpdateAction<Type>> updateActions);
+    CompletionStage<Type> updateType(@Nonnull Type type, @Nullable List<UpdateAction<Type>> updateActions);
 }

src/main/java/com/commercetools/service/ctp/impl/BaseSphereService.java

@@ -1,8 +1,17 @@
 package com.commercetools.service.ctp.impl;
 
 import io.sphere.sdk.client.SphereClient;
+import io.sphere.sdk.commands.UpdateAction;
+import io.sphere.sdk.commands.UpdateCommand;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import java.util.List;
+import java.util.concurrent.CompletionStage;
+import java.util.function.BiFunction;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.apache.commons.collections4.CollectionUtils.isEmpty;
 
 abstract public class BaseSphereService {
 
@@ -12,4 +21,28 @@ abstract public class BaseSphereService {
     protected BaseSphereService(@Nonnull SphereClient sphereClient) {
         this.sphereClient = sphereClient;
     }
+
+    /**
+     * Optimize Sphere update actions requests: if {@code updateActions} list is empty (null or doesn't contain values)
+     * — don't send sphere request and return completed stage with the same {@code entity} instance
+     *
+     * @param entity                instance of CTP platform value to update
+     * @param updateActions         list of actions to apply. If null or empty - sphere request won't be sent
+     * @param updateCommandSupplier function to generate Sphere {@link UpdateCommand} based on {@code entity} and
+     *                              {@code updateActions} if the actions list is not empty
+     * @param <T>                   type of CTP platform entity
+     * @return completed future with {@code entity} instance, if {@code updateActions} list doesn't
+     * have values to update. Otherwise - make regular sphere client request
+     * and return completion stage with updated entity instance.
+     */
+    protected final <T extends io.sphere.sdk.models.Resource<T>> CompletionStage<T>
+    returnSameInstanceIfEmptyListOrExecuteCommand(
+            @Nonnull T entity,
+            @Nullable List<? extends UpdateAction<T>> updateActions,
+            @Nonnull BiFunction<T, List<? extends UpdateAction<T>>, UpdateCommand<T>> updateCommandSupplier) {
+
+        return isEmpty(updateActions)
+                ? completedFuture(entity)
+                : sphereClient.execute(updateCommandSupplier.apply(entity, updateActions));
+    }
 }

src/main/java/com/commercetools/service/ctp/impl/PaymentServiceImpl.java

@@ -49,10 +49,7 @@ public CompletionStage<Optional<Payment>> getByPaymentInterfaceNameAndInterfaceI
 
     @Override
     public CompletionStage<Payment> updatePayment(@Nonnull Payment payment, @Nullable List<UpdateAction<Payment>> updateActions) {
-        if (updateActions == null || updateActions.isEmpty()) {
-            return completedFuture(payment);
-        }
-        return sphereClient.execute(PaymentUpdateCommand.of(payment, updateActions));
+        return returnSameInstanceIfEmptyListOrExecuteCommand(payment, updateActions, PaymentUpdateCommand::of);
     }
 
     @Override

src/main/java/com/commercetools/service/ctp/impl/TypeServiceImpl.java

@@ -10,6 +10,7 @@
 import io.sphere.sdk.types.queries.TypeQueryBuilder;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import java.util.List;
 import java.util.concurrent.CompletionStage;
 
@@ -31,8 +32,8 @@ public CompletionStage<Type> createType(@Nonnull TypeDraft typeDraft) {
     }
 
     @Override
-    public CompletionStage<Type> updateType(@Nonnull Type type, @Nonnull List<UpdateAction<Type>> updateActions) {
-        return sphereClient.execute(TypeUpdateCommand.of(type, updateActions));
+    public CompletionStage<Type> updateType(@Nonnull Type type, @Nullable List<UpdateAction<Type>> updateActions) {
+        return returnSameInstanceIfEmptyListOrExecuteCommand(type, updateActions, TypeUpdateCommand::of);
     }
 
 

src/test/java/com/commercetools/service/ctp/impl/BaseSphereServiceTest.java

@@ -0,0 +1,107 @@
+package com.commercetools.service.ctp.impl;
+
+import io.sphere.sdk.client.SphereClient;
+import io.sphere.sdk.commands.UpdateAction;
+import io.sphere.sdk.commands.UpdateCommand;
+import io.sphere.sdk.payments.Payment;
+import io.sphere.sdk.payments.commands.PaymentUpdateCommand;
+import io.sphere.sdk.payments.commands.updateactions.SetInterfaceId;
+import io.sphere.sdk.payments.commands.updateactions.SetStatusInterfaceCode;
+import io.sphere.sdk.payments.commands.updateactions.SetStatusInterfaceText;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import javax.annotation.Nonnull;
+import java.util.List;
+import java.util.function.BiFunction;
+
+import static com.commercetools.testUtil.CompletionStageUtil.executeBlocking;
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyObject;
+import static org.mockito.Mockito.*;
+
+@RunWith(MockitoJUnitRunner.class)
+public class BaseSphereServiceTest {
+
+    @Mock
+    private SphereClient sphereClient;
+
+    @Mock
+    private BaseSphereService baseSphereService;
+
+    @Mock
+    private Payment payment;
+
+    @Mock
+    private Payment paymentUpdated;
+
+    @Before
+    public void setUp() throws Exception {
+        baseSphereService = new TestableBaseSphereService(sphereClient);
+    }
+
+    @Test
+    public void whenEmptyList_returnSameInstanceIfEmptyListOrExecuteCommand_returnsSameInstance() throws Exception {
+        BiFunction<Payment, List<? extends UpdateAction<Payment>>, UpdateCommand<Payment>> throwIfCalled = (a, b) -> {
+            throw new IllegalStateException("command supplier is not expected to be called when list is empty");
+        };
+
+        Payment newPayment = executeBlocking(baseSphereService.returnSameInstanceIfEmptyListOrExecuteCommand(this.payment, emptyList(), throwIfCalled));
+        assertThat(newPayment).isSameAs(payment);
+
+        newPayment = executeBlocking(baseSphereService.returnSameInstanceIfEmptyListOrExecuteCommand(this.payment, null, throwIfCalled));
+        assertThat(newPayment).isSameAs(payment);
+
+        // ensure sphere client is not called at all
+        verify(sphereClient, never()).execute(anyObject());
+    }
+
+    @Test
+    public void whenListHasValuesList_returnSameInstanceIfEmptyListOrExecuteCommand_callsSphereClient() throws Exception {
+        when(sphereClient.execute(any(PaymentUpdateCommand.class))).thenReturn(completedFuture(paymentUpdated));
+
+        // single action
+        Payment newPayment = executeBlocking(baseSphereService.returnSameInstanceIfEmptyListOrExecuteCommand(this.payment,
+                singletonList(SetInterfaceId.of("42")),
+                PaymentUpdateCommand::of));
+        assertThat(newPayment).isSameAs(paymentUpdated);
+
+        ArgumentCaptor<PaymentUpdateCommand> commandCaptor = ArgumentCaptor.forClass(PaymentUpdateCommand.class);
+        verify(sphereClient).execute(commandCaptor.capture());
+        assertThat(commandCaptor.getValue().getUpdateActions()).containsExactly(SetInterfaceId.of("42"));
+
+        // multiple actions
+        newPayment = executeBlocking(baseSphereService.returnSameInstanceIfEmptyListOrExecuteCommand(this.payment,
+                asList(
+                        SetInterfaceId.of("42"),
+                        SetStatusInterfaceText.of("success"),
+                        SetStatusInterfaceCode.of("666")),
+                PaymentUpdateCommand::of));
+        assertThat(newPayment).isSameAs(paymentUpdated);
+
+        commandCaptor = ArgumentCaptor.forClass(PaymentUpdateCommand.class);
+        // capture second call, so expected sphere client call twice and take values.get(1)
+        verify(sphereClient, times(2)).execute(commandCaptor.capture());
+        assertThat(commandCaptor.getAllValues().get(1).getUpdateActions())
+                .containsExactly(
+                        SetInterfaceId.of("42"),
+                        SetStatusInterfaceText.of("success"),
+                        SetStatusInterfaceCode.of("666"));
+    }
+
+    private static class TestableBaseSphereService extends BaseSphereService {
+        TestableBaseSphereService(@Nonnull SphereClient sphereClient) {
+            super(sphereClient);
+        }
+    }
+
+}

src/test/java/com/commercetools/service/ctp/impl/PaymentServiceImplIntegrationTest.java

@@ -25,6 +25,7 @@
 
 import static com.commercetools.testUtil.CompletionStageUtil.executeBlocking;
 import static io.sphere.sdk.models.DefaultCurrencyUnits.EUR;
+import static java.util.Collections.emptyList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.*;
@@ -144,7 +145,7 @@ public void whenUpdateActionsIsNullOrEmpty_shouldNotUpdateAnything() {
                 .build();
         Payment payment = executeBlocking(sphereClient.execute(PaymentCreateCommand.of(draft)));
 
-        Payment paymentAfterUpdate = executeBlocking(paymentService.updatePayment(payment, Collections.emptyList()));
+        Payment paymentAfterUpdate = executeBlocking(paymentService.updatePayment(payment, emptyList()));
         assertThat(paymentAfterUpdate).isEqualTo(payment);
 
         paymentAfterUpdate = executeBlocking(paymentService.updatePayment(payment, null));
@@ -199,4 +200,20 @@ public void throwsExceptionWhenUpdatePaymentByStringIdWhenPaymentIsMissing() {
                 .hasCauseExactlyInstanceOf(CtpServiceException.class)
                 .hasMessageContaining("11111111-1111-1111-1111-111111111111");
     }
+
+    @Test
+    public void whenActionsListIsEmpty_updatePayment_shouldReturnSameInstance() {
+        String paymentKey = "testPayment_sameInstance";
+        Money amountBefore = Money.of(1, EUR);
+        PaymentDraftDsl draft = PaymentDraftBuilder.of(amountBefore)
+                .key(paymentKey)
+                .build();
+        Payment payment = executeBlocking(sphereClient.execute(PaymentCreateCommand.of(draft)));
+
+        Payment updatedPayment = executeBlocking(paymentService.updatePayment(payment, emptyList()));
+        assertThat(updatedPayment).isSameAs(payment);
+
+        updatedPayment = executeBlocking(paymentService.updatePayment(payment, null));
+        assertThat(updatedPayment).isSameAs(payment);
+    }
 }

src/test/java/com/commercetools/service/ctp/impl/TypeServiceImplIntegrationTest.java

@@ -23,6 +23,7 @@
 import static com.commercetools.testUtil.CompletionStageUtil.executeBlocking;
 import static com.commercetools.testUtil.ctpUtil.CleanupTableUtil.cleanupTypes;
 import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singleton;
 import static java.util.Locale.*;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -104,4 +105,17 @@ public void updateType() throws Exception {
         assertThat(updatedType.getName().get(new Locale("uk"))).isEqualTo("test-update-type-uk");
     }
 
+    @Test
+    public void updateTypeWithEmptyList_returnsTheSameInstance() throws Exception {
+        Type testType = executeBlocking(typeService.createType(TypeDraftBuilder
+                .of("test-type-1-key", LocalizedString.of(ENGLISH, "test-updateType-type"), singleton("payment"))
+                .build()));
+
+        Type updatedType = executeBlocking(typeService.updateType(testType, emptyList()));
+        assertThat(updatedType).isSameAs(testType);
+
+        updatedType = executeBlocking(typeService.updateType(testType, null));
+        assertThat(updatedType).isSameAs(testType);
+    }
+
 }