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

acavailhez-stripe · 2019-01-23T20:09:55Z

onKeyUpdate was called outside a try clause which is not ideal
The error messages where not incredibly useful, made them a tad better

mshafrir-stripe · 2019-01-23T20:46:19Z

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

@@ -216,7 +216,7 @@ String getType() {
    protected static <TEphemeralKey extends AbstractEphemeralKey> TEphemeralKey
    fromString(@Nullable String rawJson, Class ephemeralKeyClass) throws JSONException {
        if (rawJson == null) {
-            return null;
+            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass+ " null raw key");


How about "Attempted to instantiate " + ephemeralKeyClass + " with null key JSON"? Can you also add a test? Does it need to be ephemeralKeyClass.getSimpleName()?

getSimpleName makes the name a tad shorter, since the error message is quite long.

I'll change the text

A test is not trivial because of the was TestEphemeralKeyProvider is build, but I'll re-try

all done by the way

mshafrir-stripe · 2019-01-23T20:49:51Z

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

+                null,
+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, times(0)).onKeyUpdate(


you can do never() instead of times(0)

mshafrir-stripe · 2019-01-23T20:51:49Z

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

+    public void triggerCorrectErrorOnInvalidRawKey() {
+
+        mTestEphemeralKeyProvider.setNextRawEphemeralKey("Not_a_JSON");
+        EphemeralKeyManager<CustomerEphemeralKey> keyManager = new EphemeralKeyManager(


new EphemeralKeyManager<>(...)

mshafrir-stripe · 2019-01-23T20:52:12Z

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

+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, times(0)).onKeyUpdate(
+                (CustomerEphemeralKey) isNull(), (String) isNull(), (Map<String, Object>) isNull());


I think this is better:

any(CustomerEphemeralKey.class), anyString(), ArgumentMatchers.<String, Object>anyMap()

So I initially tried that, but it fails to error when onKeyUpdate is called with nulls

don't we expect it not to be called though? so I would assume "never with any" makes more logical sense than "never with nulls"

Ah yes, but if I change to the any and re-introduce the bug where onKeyUpdate is called, the test still passes

mshafrir-stripe · 2019-01-23T20:52:32Z

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

+                null,
+                CustomerEphemeralKey.class);
+
+        verify(mKeyManagerListener, times(0)).onKeyUpdate(


mshafrir-stripe · 2019-01-23T20:53:53Z

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

+        if (key == null) {
+            mListener.onKeyError(HttpURLConnection.HTTP_INTERNAL_ERROR,
+                    "EphemeralKeyUpdateListener.onKeyUpdate was called " +
+                            "with a null value");


should we return; after this line?

ah yes, good catch

mshafrir-stripe · 2019-01-23T20:55:51Z

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

-                            + e.getLocalizedMessage());
+                    "EphemeralKeyUpdateListener.onKeyUpdate was passed " +
+                            "a value that could not be JSON parsed: ["
+                            + e.getLocalizedMessage() + "] the raw body from Stripe's response" +


I think "the raw body..." should be a new sentence, so "]. The raw body...

Also below.

mshafrir-stripe · 2019-01-23T20:57:41Z

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

        } catch (InvocationTargetException e) {
-            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass, e);
+            if (e.getTargetException() != null){
+                throw new IllegalArgumentException("Improperly formatted JSON for ephemeral key " +


I think we can remove the e.getTargetException().getMessage() part from the message because we're passing e.getTargetException() as an argument.

hm, in an ideal world yes, but the interface of onKeyError only accepts a String, and that's what we're passing to the dev.

So without explicitely dropping the message in the String, we risk losing it as it is surfaced, if that makes sense

mshafrir-stripe · 2019-01-23T21:00:07Z

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

+                            "a value that could not be JSON parsed: ["
+                            + e.getLocalizedMessage() + "] the raw body from Stripe's response" +
+                            " should be passed");
+        } catch (Exception e) {


did you add this for safety?

Yeah, if any other exception happens, the error will just be silenced, which was the case for a wrongly formatted JSON (the JSON is parsed, but then the CustomerEphemeralKey cannot be instanciated)

acavailhez-stripe · 2019-01-23T21:14:03Z

r? @mshafrir-stripe thanks for the review!
Still 3 open discussions

mshafrir-stripe · 2019-01-23T21:17:28Z

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

        } catch (InvocationTargetException e) {
-            throw new IllegalArgumentException("Exception instantiating " + ephemeralKeyClass, e);
+            if (e.getTargetException() != null){


space before {

mshafrir-stripe · 2019-01-23T21:27:41Z

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

        try {
            mEphemeralKey = AbstractEphemeralKey.fromString(key, mEphemeralKeyClass);
+            mListener.onKeyUpdate(mEphemeralKey, actionString, arguments);
        } catch (JSONException e) {


do we still need to explicitly catch JSONException if we have the catch all below?

I think it helps the message be a tad more explicit

acavailhez-stripe added 3 commits January 23, 2019 12:09

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

ce3d47a

Fix lint

6dbff0a

more explicit variable naming

263e12e

mshafrir-stripe requested changes Jan 23, 2019

View reviewed changes

stripe-ci assigned acavailhez-stripe Jan 23, 2019

mshafrir-stripe reviewed Jan 23, 2019

View reviewed changes

acavailhez-stripe added 4 commits January 23, 2019 13:05

Better handling of null ephemeral key case

384cbe7

Better naming still

e5f8387

Review comments and bug fix

2e50eca

Better texts

332b2eb

mshafrir-stripe requested changes Jan 23, 2019

View reviewed changes

More explicit error message

8c287d8

mshafrir-stripe approved these changes Jan 23, 2019

View reviewed changes

stripe-ci added the approved label Jan 23, 2019

acavailhez-stripe merged commit ac547fc into master Jan 24, 2019

acavailhez-stripe deleted the acavailhez/ephemeral-key-fix branch January 24, 2019 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

acavailhez-stripe commented Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

acavailhez-stripe commented Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

mshafrir-stripe Jan 23, 2019

acavailhez-stripe Jan 23, 2019

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

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

Conversation

acavailhez-stripe commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acavailhez-stripe commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment