From 70b6533e1f25164f524d34ce34665c9fdf7c1cc4 Mon Sep 17 00:00:00 2001
From: Justin <justin.lee.mills@gmail.com>
Date: Thu, 24 Aug 2023 08:10:55 -0700
Subject: [PATCH] fix(api): When a mutation response code indicates client
 error only call onFailure (#2255)

Co-authored-by: Matt Creaser <mattwcc@amazon.com>
Co-authored-by: gpanshu <91897496+gpanshu@users.noreply.github.com>
---
 .../api/aws/AppSyncGraphQLOperation.java      |  1 +
 .../api/aws/AWSApiPluginTest.java             | 72 +++++++++++++++++++
 2 files changed, 73 insertions(+)

diff --git a/aws-api/src/main/java/com/amplifyframework/api/aws/AppSyncGraphQLOperation.java b/aws-api/src/main/java/com/amplifyframework/api/aws/AppSyncGraphQLOperation.java
index 42c00f8a90..e1cdc9e410 100644
--- a/aws-api/src/main/java/com/amplifyframework/api/aws/AppSyncGraphQLOperation.java
+++ b/aws-api/src/main/java/com/amplifyframework/api/aws/AppSyncGraphQLOperation.java
@@ -147,6 +147,7 @@ public void onResponse(@NonNull Call call, @NonNull Response response) {
                 onFailure.accept(new ApiException
                         .NonRetryableException("OkHttp client request failed.", "Irrecoverable error")
                 );
+                return;
             }
 
             try {
diff --git a/aws-api/src/test/java/com/amplifyframework/api/aws/AWSApiPluginTest.java b/aws-api/src/test/java/com/amplifyframework/api/aws/AWSApiPluginTest.java
index d0214dd373..1a96dd378d 100644
--- a/aws-api/src/test/java/com/amplifyframework/api/aws/AWSApiPluginTest.java
+++ b/aws-api/src/test/java/com/amplifyframework/api/aws/AWSApiPluginTest.java
@@ -36,6 +36,7 @@
 import com.amplifyframework.testmodels.commentsblog.BlogOwner;
 import com.amplifyframework.testutils.Await;
 import com.amplifyframework.testutils.HubAccumulator;
+import com.amplifyframework.testutils.Latch;
 import com.amplifyframework.testutils.Resources;
 import com.amplifyframework.testutils.random.RandomString;
 import com.amplifyframework.util.TypeMaker;
@@ -53,7 +54,9 @@
 import java.lang.reflect.Type;
 import java.util.Arrays;
 import java.util.Map;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import io.reactivex.rxjava3.core.Observable;
 import okhttp3.HttpUrl;
@@ -275,6 +278,75 @@ public void graphQlMutationGetsResponse() throws JSONException, ApiException {
         assertEquals(ApiEndpointStatusChangeEvent.ApiEndpointStatus.REACHABLE, eventData.getCurrentStatus());
     }
 
+    /**
+     * When the server returns a client error code in response to calling
+     * {@link AWSApiPlugin#mutate(GraphQLRequest, Consumer, Consumer)}.
+     * only the onFailure callback should be called.
+     *
+     * @throws InterruptedException if the thread performing the mutation is interrupted.
+     */
+    @Test
+    public void graphQlMutationWithClientErrorResponseCodeShouldNotCallOnResponse() throws InterruptedException {
+        final int CLIENT_ERROR_CODE = 400;
+        webServer.enqueue(new MockResponse().setResponseCode(CLIENT_ERROR_CODE));
+
+        final CountDownLatch latch = new CountDownLatch(2);
+        final AtomicReference<Throwable> unexpectedErrorContainer = new AtomicReference<>();
+        final AtomicReference<GraphQLResponse<BlogOwner>> responseContainer = new AtomicReference<>();
+        final AtomicReference<ApiException> failureContainer = new AtomicReference<>();
+
+        final Consumer<GraphQLResponse<BlogOwner>> onResponse = (response) -> {
+            responseContainer.set(response);
+            latch.countDown();
+        };
+        final Consumer<ApiException> onFailure = (failure) -> {
+            failureContainer.set(failure);
+            latch.countDown();
+        };
+
+        final Thread thread = new Thread(() -> {
+            try {
+                // Try to perform a mutation.
+                final BlogOwner tony = BlogOwner.builder()
+                        .name(RandomString.string())
+                        .build();
+                plugin.mutate(ModelMutation.create(tony), onResponse, onFailure);
+            } catch (Throwable unexpectedFailure) {
+                unexpectedErrorContainer.set(unexpectedFailure);
+                do {
+                    latch.countDown();
+                } while (latch.getCount() > 0);
+            }
+        });
+        thread.setDaemon(true);
+        thread.start();
+
+        try {
+            Latch.await(latch);
+            thread.join();
+        } catch (RuntimeException runtimeException) {
+            // We expect a RuntimeException here.
+            //  That means awaiting the latch timed out and both callbacks were not called.
+        }
+
+        assertNull(
+                "An unexpected error occurred while performing the mutation.",
+                unexpectedErrorContainer.get()
+        );
+        assertTrue(
+                "Latch count == 0; both onFailure and onResponse were called.",
+                latch.getCount() > 0
+        );
+        assertTrue(
+                "Expected client error response code to result in NonRetryableException.",
+                failureContainer.get() instanceof ApiException.NonRetryableException
+        );
+        assertNull(
+                "There was a non-null response but the response code indicates client error.",
+                responseContainer.get()
+        );
+    }
+
     /**
      * Given that only one API was configured in {@link #setup()},
      * the {@link AWSApiPlugin#getSelectedApiName(EndpointType)} should be able to identify