Fix start callback to be invoked on failure. (#402)

Co-authored-by: Mayank Sanganeria <[email protected]>

Author: hborisoff

AndroidSDKCore/src/main/java/com/leanplum/Leanplum.java

@@ -704,6 +704,12 @@ public void response(JSONObject response) {
         handleStartResponse(response);
       }
     });
+    request.onError(new RequestOld.ErrorCallback() {
+      @Override
+      public void error(Exception e) {
+        handleStartResponse(null);
+      }
+    });
 
     if (isBackground) {
       request.sendEventually();
@@ -725,9 +731,10 @@ private static void handleStartResponse(final JSONObject response) {
         // Load the variables that were stored on the device from the last session.
         VarCache.loadDiffs();
 
-        triggerStartResponse(false);
       } catch (Throwable t) {
         Util.handleException(t);
+      } finally {
+        triggerStartResponse(success);
       }
     } else {
       try {
@@ -781,7 +788,6 @@ private static void handleStartResponse(final JSONObject response) {
         applyContentInResponse(response, true);
 
         VarCache.saveUserAttributes();
-        triggerStartResponse(true);
 
         if (response.optBoolean(Constants.Keys.SYNC_INBOX, false)) {
           LeanplumInbox.getInstance().downloadMessages();
@@ -892,6 +898,8 @@ public void run() {
         startHeartbeat();
       } catch (Throwable t) {
         Util.handleException(t);
+      } finally {
+        triggerStartResponse(success);
       }
     }
   }

AndroidSDKCore/src/main/java/com/leanplum/internal/RequestOld.java

@@ -361,7 +361,7 @@ public interface NoPendingDownloadsCallback {
    * @param responseBody JSONObject with response body from server.
    * @param error Exception.
    */
-  protected void parseResponseBody(JSONObject responseBody, Exception error) {
+  protected void triggerCallbackManager(JSONObject responseBody, Exception error) {
     synchronized (RequestOld.class) {
       if (responseBody == null && error != null) {
         // Invoke potential error callbacks for all events of this request.
@@ -520,7 +520,7 @@ private void sendRequests() {
 
         if (statusCode >= 200 && statusCode <= 299) {
           // Parse response body and trigger callbacks
-          parseResponseBody(responseBody, null);
+          triggerCallbackManager(responseBody, null);
 
           // Clear localErrors list.
           localErrors.clear();
@@ -534,15 +534,16 @@ private void sendRequests() {
           Exception errorException = new Exception("HTTP error " + statusCode);
           if (statusCode != -1 && statusCode != 408 && !(statusCode >= 500 && statusCode <= 599)) {
             deleteSentRequests(unsentRequests.size());
-            parseResponseBody(responseBody, errorException);
           }
+          triggerCallbackManager(responseBody, errorException);
         }
       } catch (JSONException e) {
         Log.e("Error parsing JSON response: " + e.toString() + "\n" + Log.getStackTraceString(e));
         deleteSentRequests(unsentRequests.size());
-        parseResponseBody(null, e);
+        triggerCallbackManager(null, e);
       } catch (Exception e) {
         Log.e("Unable to send request: " + e.toString() + "\n" + Log.getStackTraceString(e));
+        triggerCallbackManager(null, e);
       } finally {
         if (op != null) {
           op.disconnect();

AndroidSDKTests/src/test/java/com/leanplum/LeanplumTest.java

@@ -48,11 +48,17 @@
 import com.leanplum.models.GeofenceEventType;
 import com.leanplum.models.MessageArchiveData;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import javax.net.ssl.HttpsURLConnection;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.junit.Test;
 import org.mockito.Mockito;
+import org.powermock.api.mockito.PowerMockito;
 import org.robolectric.RuntimeEnvironment;
 
 import java.lang.reflect.Method;
@@ -81,6 +87,7 @@
 import static org.mockito.Matchers.anyDouble;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.when;
@@ -161,6 +168,97 @@ public void onResponse(boolean success) {
     assertTrue(Leanplum.hasStarted());
   }
 
+  /**
+   * Test to verify that callbacks are invoked when Exception is thrown after https request.
+   */
+  @Test
+  public void testStartFailWithException() throws Exception {
+    // Seed response to throw exception.
+    ResponseHelper.seedJsonResponseException();
+
+    // Expected request params.
+    final HashMap<String, Object> expectedRequestParams = CollectionUtil.newHashMap(
+        "city", "(detect)",
+        "country", "(detect)",
+        "location", "(detect)",
+        "region", "(detect)",
+        "locale", "en_US"
+    );
+
+    // Validate request.
+    RequestHelper.addRequestHandler(new RequestHelper.RequestHandler() {
+      @Override
+      public void onRequest(String httpMethod, String apiMethod, Map<String, Object> params) {
+        assertEquals(Constants.Methods.START, apiMethod);
+        assertTrue(params.keySet().containsAll(expectedRequestParams.keySet()));
+        assertTrue(params.values().containsAll(expectedRequestParams.values()));
+      }
+    });
+
+    class TestStartCallback extends StartCallback {
+      private boolean onResponseCalled;
+      private boolean startSuccessfull;
+      @Override
+      public void onResponse(boolean success) {
+        onResponseCalled = true;
+        startSuccessfull = success;
+      }
+    }
+
+    TestStartCallback callback = new TestStartCallback();
+    Leanplum.start(mContext, callback);
+
+    assertTrue(Leanplum.hasStarted());
+    assertTrue(callback.onResponseCalled);
+    assertFalse(callback.startSuccessfull);
+  }
+
+  /**
+   * Test to verify that callbacks are invoked when responseCode is 500 after https request.
+   */
+  @Test
+  public void testStartFailWithInternalServerError() throws Exception {
+    prepareHttpsURLConnection(500);
+
+    ResponseHelper.seedJsonResponseNull();
+
+    // Expected request params.
+    final HashMap<String, Object> expectedRequestParams = CollectionUtil.newHashMap(
+        "city", "(detect)",
+        "country", "(detect)",
+        "location", "(detect)",
+        "region", "(detect)",
+        "locale", "en_US"
+    );
+
+    // Validate request.
+    RequestHelper.addRequestHandler(new RequestHelper.RequestHandler() {
+      @Override
+      public void onRequest(String httpMethod, String apiMethod, Map<String, Object> params) {
+        assertEquals(Constants.Methods.START, apiMethod);
+        assertTrue(params.keySet().containsAll(expectedRequestParams.keySet()));
+        assertTrue(params.values().containsAll(expectedRequestParams.values()));
+      }
+    });
+
+    class TestStartCallback extends StartCallback {
+      private boolean onResponseCalled;
+      private boolean startSuccessfull;
+      @Override
+      public void onResponse(boolean success) {
+        onResponseCalled = true;
+        startSuccessfull = success;
+      }
+    }
+
+    TestStartCallback callback = new TestStartCallback();
+    Leanplum.start(mContext, callback);
+
+    assertTrue(Leanplum.hasStarted());
+    assertTrue(callback.onResponseCalled);
+    assertFalse(callback.startSuccessfull);
+  }
+
   @Test
   public void testStartWithUserAttributes() {
     ResponseHelper.seedResponse("/responses/simple_start_response.json");

AndroidSDKTests/src/test/java/com/leanplum/__setup/AbstractTest.java

@@ -150,21 +150,9 @@ public void before() throws Exception {
     // Setup the sdk.
     LeanplumTestHelper.setUp();
 
-    // Mock url connection to work offline.
-    URL mockedURL = mock(URL.class);
-    HttpsURLConnection httpsURLConnection = mock(HttpsURLConnection.class);
-
     // To be able to run tests offline and not depend on a server we have to mock URLConnection to
     // return proper status code.
-    whenNew(URL.class).withParameterTypes(String.class).withArguments(anyString())
-        .thenReturn(mockedURL);
-    when(mockedURL.openConnection()).thenReturn(httpsURLConnection);
-    when(httpsURLConnection.getOutputStream()).thenReturn(new ByteArrayOutputStream());
-    when(httpsURLConnection.getResponseCode()).thenReturn(200);
-    // We are just seeding a random file as a InputStream of a mocked httpConnection which will be
-    // used in FileManager tests other tests depends on Util.getResponse() to mock response.
-    when(httpsURLConnection.getInputStream()).thenReturn(ResponseHelper
-        .seedInputStream("/responses/simple_start_response.json"));
+    prepareHttpsURLConnection(200);
 
     stopLeanplumExceptionHandling();
 
@@ -202,6 +190,22 @@ protected void resumeLeanplumExceptionHandling() throws Exception {
     PowerMockito.doNothing().when(Util.class, "handleException", any(Throwable.class));
   }
 
+  protected void prepareHttpsURLConnection(int responseCode) throws Exception {
+    // Mock url connection to work offline.
+    URL mockedURL = mock(URL.class);
+    HttpsURLConnection httpsURLConnection = mock(HttpsURLConnection.class);
+
+    whenNew(URL.class).withParameterTypes(String.class).withArguments(anyString())
+        .thenReturn(mockedURL);
+    when(mockedURL.openConnection()).thenReturn(httpsURLConnection);
+    when(httpsURLConnection.getOutputStream()).thenReturn(new ByteArrayOutputStream());
+    when(httpsURLConnection.getResponseCode()).thenReturn(responseCode);
+    // We are just seeding a random file as a InputStream of a mocked httpConnection which will be
+    // used in FileManager tests other tests depends on Util.getResponse() to mock response.
+    when(httpsURLConnection.getInputStream()).thenReturn(ResponseHelper
+        .seedInputStream("/responses/simple_start_response.json"));
+  }
+
   @After
   public void after() {
     LeanplumTestHelper.tearDown();

AndroidSDKTests/src/test/java/com/leanplum/_whitebox/utilities/RequestHelper.java

@@ -76,7 +76,7 @@ public void sendEventually() {
   }
 
   @Override
-  protected void parseResponseBody(JSONObject responseBody, Exception error) {
+  protected void triggerCallbackManager(JSONObject responseBody, Exception error) {
     try {
       // attach the uuid we generated
       JSONArray jsonArray = responseBody.getJSONArray("response");
@@ -87,7 +87,7 @@ protected void parseResponseBody(JSONObject responseBody, Exception error) {
     } catch (Exception e) {
       // ignore
     }
-    super.parseResponseBody(responseBody, error);
+    super.triggerCallbackManager(responseBody, error);
   }
 
   /**

AndroidSDKTests/src/test/java/com/leanplum/_whitebox/utilities/ResponseHelper.java

@@ -24,11 +24,13 @@
 import com.leanplum.internal.Util;
 
 import java.io.BufferedReader;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 
 import static org.mockito.Matchers.anyObject;
 import static org.powermock.api.mockito.PowerMockito.doReturn;
+import static org.powermock.api.mockito.PowerMockito.doThrow;
 
 /**
  * @author Milos Jakovljevic
@@ -79,13 +81,26 @@ public static void seedResponse(String filename) {
    * Seeds the Null response to Util.getResponse method.
    *
    */
-
-
   public static void seedResponseNull() {
     try {
       doReturn(false).when(Util.class, Util.isConnected());
     } catch (Exception e) {
       Log.e("ResponseHelper", "Unable to seed response from file: ");
     }
   }
+
+  /**
+   * Seeds Null response to Util.getJsonResponse method.
+   */
+  public static void seedJsonResponseNull() throws Exception{
+    doReturn(null).when(Util.class, "getJsonResponse", anyObject());
+  }
+
+  /**
+   * Seed response to throw an exception.
+   * Used for testing if request callbacks are invoked in case of exception.
+   */
+  public static void seedJsonResponseException() throws Exception {
+    doThrow(new IOException()).when(Util.class, "getJsonResponse", anyObject());
+  }
 }