Fix bug calling onKeyUpdate when should not, and better error messages (

#780)

* Fix bug calling onKeyUpdate when should not, and better error messages

* Fix lint

* more explicit variable naming

* Better handling of null ephemeral key case

* Better naming still

* Review comments and bug fix

* Better texts

* More explicit error message

stripe/src/main/java/com/stripe/android/AbstractEphemeralKey.java

@@ -216,7 +216,8 @@ String getType() {
     protected static <TEphemeralKey extends AbstractEphemeralKey> TEphemeralKey
     fromString(@Nullable String rawJson, Class ephemeralKeyClass) throws JSONException {
         if (rawJson == null) {
-            return null;
+            throw new IllegalArgumentException("Attempted to instantiate " +
+                    ephemeralKeyClass.getSimpleName() + " with null raw key");
         }
         JSONObject object = new JSONObject(rawJson);
         return fromJson(object, ephemeralKeyClass);
@@ -226,21 +227,32 @@ String getType() {
     protected static <TEphemeralKey extends AbstractEphemeralKey> TEphemeralKey
     fromJson(@Nullable JSONObject jsonObject, Class ephemeralKeyClass) {
         if (jsonObject == null) {
-            return null;
+            throw new IllegalArgumentException("Exception instantiating " +
+                    ephemeralKeyClass.getSimpleName() +
+                    " null JSON");
         }
 
         try {
             return (TEphemeralKey)
                     ephemeralKeyClass.getConstructor(JSONObject.class).newInstance(jsonObject);
         } catch (InstantiationException e) {
-            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass, e);
+            throw new IllegalArgumentException("Exception instantiating " +
+                    ephemeralKeyClass.getSimpleName(), e);
         } catch (IllegalAccessException e) {
-            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass, e);
+            throw new IllegalArgumentException("Exception instantiating " +
+                    ephemeralKeyClass.getSimpleName(), e);
         } catch (InvocationTargetException e) {
-            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass, e);
+            if (e.getTargetException() != null) {
+                throw new IllegalArgumentException("Improperly formatted JSON for ephemeral key " +
+                        ephemeralKeyClass.getSimpleName() +
+                        " - " + e.getTargetException().getMessage(),
+                        e.getTargetException());
+            }
+            throw new IllegalArgumentException("Improperly formatted JSON for ephemeral key " +
+                    ephemeralKeyClass.getSimpleName(), e);
         } catch (NoSuchMethodException e) {
             throw new IllegalArgumentException("Class " +
-                    ephemeralKeyClass +
+                    ephemeralKeyClass.getSimpleName() +
                     " does not have an accessible (JSONObject) constructor", e);
         }
     }

stripe/src/main/java/com/stripe/android/EphemeralKeyManager.java

@@ -54,18 +54,35 @@ TEphemeralKey getEphemeralKey() {
         return mEphemeralKey;
     }
 
+    @SuppressWarnings("checkstyle:IllegalCatch")
     private void updateKey(
             @NonNull String key,
             @Nullable String actionString,
             @Nullable Map<String, Object> arguments) {
+        // Key is coming from the user, so even if it's @NonNull annotated we
+        // want to double check it
+        if (key == null) {
+            mListener.onKeyError(HttpURLConnection.HTTP_INTERNAL_ERROR,
+                    "EphemeralKeyUpdateListener.onKeyUpdate was called " +
+                            "with a null value");
+            return;
+        }
         try {
             mEphemeralKey = AbstractEphemeralKey.fromString(key, mEphemeralKeyClass);
+            mListener.onKeyUpdate(mEphemeralKey, actionString, arguments);
         } catch (JSONException e) {
             mListener.onKeyError(HttpURLConnection.HTTP_INTERNAL_ERROR,
-                    "The JSON from the key could not be parsed: "
-                            + e.getLocalizedMessage());
+                    "EphemeralKeyUpdateListener.onKeyUpdate was passed " +
+                            "a value that could not be JSON parsed: ["
+                            + e.getLocalizedMessage() + "]. The raw body from Stripe's response" +
+                            " should be passed");
+        } catch (Exception e) {
+            mListener.onKeyError(HttpURLConnection.HTTP_INTERNAL_ERROR,
+                    "EphemeralKeyUpdateListener.onKeyUpdate was passed " +
+                            "a JSON String that was invalid: ["
+                            + e.getLocalizedMessage() + "]. The raw body from Stripe's response" +
+                            " should be passed");
         }
-        mListener.onKeyUpdate(mEphemeralKey, actionString, arguments);
     }
 
     private void updateKeyError(int errorCode, @Nullable String errorMessage) {

stripe/src/main/java/com/stripe/android/EphemeralKeyUpdateListener.java

@@ -11,9 +11,9 @@ public interface EphemeralKeyUpdateListener {
     /**
      * Called when a key update request from your server comes back successfully.
      *
-     * @param rawKey the raw String returned from Stripe's servers
+     * @param stripeResponseJson the raw JSON String returned from Stripe's servers
      */
-    void onKeyUpdate(@NonNull String rawKey);
+    void onKeyUpdate(@NonNull String stripeResponseJson);
 
     /**
      * Called when a key update request from your server comes back with an error.

stripe/src/test/java/com/stripe/android/EphemeralKeyManagerTest.java

@@ -13,6 +13,7 @@
 import org.mockito.MockitoAnnotations;
 import org.robolectric.RobolectricTestRunner;
 
+import java.net.HttpURLConnection;
 import java.util.Calendar;
 import java.util.HashMap;
 import java.util.Map;
@@ -25,6 +26,7 @@
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -250,4 +252,67 @@ public void updateKeyIfNecessary_whenReturnsError_setsExistingKeyToNull() {
         verifyNoMoreInteractions(mKeyManagerListener);
         assertNull(keyManager.getEphemeralKey());
     }
+
+    @Test
+    public void triggerCorrectErrorOnInvalidRawKey() {
+
+        mTestEphemeralKeyProvider.setNextRawEphemeralKey("Not_a_JSON");
+        EphemeralKeyManager<CustomerEphemeralKey> keyManager = new EphemeralKeyManager<>(
+                mTestEphemeralKeyProvider,
+                mKeyManagerListener,
+                TEST_SECONDS_BUFFER,
+                null,
+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, never()).onKeyUpdate(
+                (CustomerEphemeralKey) isNull(), (String) isNull(), (Map<String, Object>) isNull());
+        verify(mKeyManagerListener, times(1)).onKeyError(
+                HttpURLConnection.HTTP_INTERNAL_ERROR,
+                "EphemeralKeyUpdateListener.onKeyUpdate was passed a value that " +
+                        "could not be JSON parsed: [Value Not_a_JSON of type java.lang.String " +
+                        "cannot be converted to JSONObject]. The raw body from Stripe's " +
+                        "response should be passed");
+        assertNull(keyManager.getEphemeralKey());
+    }
+
+    @Test
+    public void triggerCorrectErrorOnInvalidJsonKey() {
+
+        mTestEphemeralKeyProvider.setNextRawEphemeralKey("{}");
+        EphemeralKeyManager<CustomerEphemeralKey> keyManager = new EphemeralKeyManager<>(
+                mTestEphemeralKeyProvider,
+                mKeyManagerListener,
+                TEST_SECONDS_BUFFER,
+                null,
+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, never()).onKeyUpdate(
+                (CustomerEphemeralKey) isNull(), (String) isNull(), (Map<String, Object>) isNull());
+        verify(mKeyManagerListener, times(1)).onKeyError(
+                HttpURLConnection.HTTP_INTERNAL_ERROR,
+                "EphemeralKeyUpdateListener.onKeyUpdate was passed a JSON String " +
+                        "that was invalid: [Improperly formatted JSON for ephemeral " +
+                        "key CustomerEphemeralKey - No value for created]. The raw body " +
+                        "from Stripe's response should be passed");
+        assertNull(keyManager.getEphemeralKey());
+    }
+
+    @Test
+    public void triggerCorrectErrorOnNullKey() {
+
+        mTestEphemeralKeyProvider.setNextRawEphemeralKey(null);
+        EphemeralKeyManager<CustomerEphemeralKey> keyManager = new EphemeralKeyManager<>(
+                mTestEphemeralKeyProvider,
+                mKeyManagerListener,
+                TEST_SECONDS_BUFFER,
+                null,
+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, never()).onKeyUpdate(
+                (CustomerEphemeralKey) isNull(), (String) isNull(), (Map<String, Object>) isNull());
+        verify(mKeyManagerListener, times(1)).onKeyError(
+                HttpURLConnection.HTTP_INTERNAL_ERROR,
+                "EphemeralKeyUpdateListener.onKeyUpdate was called with a null value");
+        assertNull(keyManager.getEphemeralKey());
+    }
 }

stripe/src/test/java/com/stripe/android/testharness/TestEphemeralKeyProvider.java

@@ -27,6 +27,9 @@ public void createEphemeralKey(
             keyUpdateListener.onKeyUpdate(mRawEphemeralKey);
         } else if (mErrorCode != INVALID_ERROR_CODE) {
             keyUpdateListener.onKeyUpdateFailure(mErrorCode, mErrorMessage);
+        } else {
+            // Useful to test edge cases
+            keyUpdateListener.onKeyUpdate(null);
         }
     }