ensureConnection - fixed rejection handling #2316

Mkolakoglu · 2023-03-16T12:40:12Z

Hi, I was trying to create a scenario that we can not establish BillingClient connection. To do that, I simply changed initConnection as below:

fun initConnection(promise: Promise) {
         Log.i(TAG, "Google Play Services are not available on this device")
         promise.safeReject(PromiseUtils.E_NOT_PREPARED, "Google Play Services are not available on this device")
         return
}

After observing that scenario, I noticed that ensureConnection is not parsing the rejection parameters properly and it's only logging this info in the logcat, which is quite dangerous because it's happening silently and probably none of us is aware of this issue so far.

When it's rejected, there is only 1 item in it array and here is the output of it[0]:

I tried to fix the parameter handling and rejected the case when the parameters are incorrect instead of just logging it. So that, we will be aware of if anything goes wrong.

andresesfm · 2023-03-16T17:05:37Z

@Mkolakoglu Can you please add a unit test for the case you are trying to fix? It's not clear to me what this is trying to solve

andresesfm · 2023-03-16T17:08:51Z

Please note that there's coverage on that method. So, it failing must be captured by the Unit Tests. Thank you for your contribution

Mkolakoglu · 2023-03-17T10:30:45Z

android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt

                        Log.i(TAG, "Incorrect parameter in resolve")
                    }
                },
                {
-                    if (it.size > 1 && it[0] is String && it[1] is String) {


When this happens, it has only item, so it.size > 1 is becoming false and it's jumping to else statement.

Please see the output of it[0] below:

Mkolakoglu · 2023-03-17T10:35:40Z

android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt

-                            it[0] as String,
-                            it[1] as String
+                            errorCode,
+                            errorMessage
                        )
                    } else {


In this block, Promise is not neither rejected nor resolved. So, when this block is called, any RNIap function is awaiting infitinitely. We have to reject the promise in this case

Mkolakoglu · 2023-03-17T10:37:33Z

android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt

+                    var errorCode: String? = null
+                    var errorMessage: String? = null
+                    if (it.isNotEmpty() && it[0] is WritableNativeMap) {
+                        val errorMap = it[0] as WritableNativeMap


Parsing code and message fields from it[0]. So that, we can use them in promise.safeReject().

Mkolakoglu · 2023-03-17T10:37:57Z

android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt

@@ -69,16 +69,26 @@ class RNIapModule(
                            promise.safeReject(PromiseUtils.E_NOT_PREPARED, "Unable to auto-initialize connection")
                        }
                    } else {


Same here: https://github.com/dooboolab/react-native-iap/pull/2316/files?diff=split&w=0#r1140057338

Mkolakoglu · 2023-03-17T10:39:56Z

@Mkolakoglu Can you please add a unit test for the case you are trying to fix? It's not clear to me what this is trying to solve

Hi @andresesfm, sorry I can not do that atm because I am super busy atm. I can not even build the project on my machine and it will probably take some to fix the issues. Instead, I added comments on some lines, hope it makes more clear it for you.

andresesfm · 2023-03-17T16:19:24Z

@Mkolakoglu thank you for the comments. I'm not understanding the edge case that makes you believe there's an issue. I have the following tests that pass: https://github.com/dooboolab/react-native-iap/blob/12.8.5/android/src/testPlay/java/com/dooboolab/RNIap/RNIapModuleTest.kt#L102 that test the scenario that you describe. What am I missing?

Mkolakoglu · 2023-03-20T15:33:41Z

@Mkolakoglu thank you for the comments. I'm not understanding the edge case that makes you believe there's an issue. I have the following tests that pass: https://github.com/dooboolab/react-native-iap/blob/12.8.5/android/src/testPlay/java/com/dooboolab/RNIap/RNIapModuleTest.kt#L102 that test the scenario that you describe. What am I missing?

Please correct me if I am wrong. That test seems to be testing only initConnection fn. However, I am claiming that ensureConnection is not handling the failure cases properly.

Let me describe how to create a failure scenario for ensureConnection fn. So that you can reproduce the issue on your machine.

Change initConnection fn as below (just to reproduce the issue):

fun initConnection(promise: Promise) {
         Log.i(TAG, "Google Play Services are not available on this device")
         promise.safeReject(PromiseUtils.E_NOT_PREPARED, "Google Play Services are not available on this device")
         return
}

So that, when ensureConnection calls initConnection, it will be always rejected.

Call any RNIap fn. E.g RNIap.getSubscriptions. It will trigger getItemsByType and since you didn't init the connection yet, it will trigger ensureConnection.
initConnection will reject ensureConnection. When this happens, please debug this line: https://github.com/dooboolab/react-native-iap/blob/bc650a3a650c8d85790012a52efa02cf4ec73cf6/android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt#L76

You will see that, it array has only 1 item. So, it.size > 1 is always false. It will always go to this block: https://github.com/dooboolab/react-native-iap/blob/bc650a3a650c8d85790012a52efa02cf4ec73cf6/android/src/play/java/com/dooboolab/RNIap/RNIapModule.kt#L81

andresesfm · 2023-03-23T00:22:35Z

@Mkolakoglu I ran the example of safeReject('Code1', 'Hello'). Here is the breakpoint:

As you can see there are 2 elements in the array, the first one is the code and the second one is the message. I don't see under what circumstances this could be an ArrayMap. The only place where initConnection rejects is this: promise.safeReject(PromiseUtils.E_NOT_PREPARED, "Google Play Services are not available on this device") which has exactly two parameters.

Sorry if I'm not understanding correctly. Are you thinking about adding a new rejection inside initConnection that has a different number of parameters? Please let me know so we can continue the conversation. As a quick test you can run the unit tests in debug mode and set a breakpoint as shown in the screenshot

Mkolakoglu · 2023-03-24T13:05:15Z

@andresesfm

Could you please share the unit test you wrote to get that output? It has been a while since I developed in Kotlin and it will be a little time consuming for me to write a new unit test.

Sorry if I'm not understanding correctly. Are you thinking about adding a new rejection inside initConnection that has a different number of parameters?

No, I don't mean that. I am saying that the current one is not properly handling the rejections 😂 Please see the output of the it array when initConnection safeReject the promise:

You will also see the same result if you would follow the reproducable steps I shared above.

Mkolakoglu · 2023-03-24T14:01:12Z

@andresesfm,

I was digging deeper and found that PromiseImpl is changing all the rejections to WritableNativeMap.

This is the reject fn we are using: https://github.com/facebook/react-native/blob/91a3a548e237cf9991105d10e2457422874d1e60/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java#L69

As you can see, it's triggering the reject fn on this line: https://github.com/facebook/react-native/blob/91a3a548e237cf9991105d10e2457422874d1e60/ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java#L168

Which is converting the error info to WritableNativeMap.

Mkolakoglu · 2023-04-03T15:22:37Z

Kind reminder @andresesfm

andresesfm · 2023-04-05T01:27:57Z

Thank you for the reminder. I haven't had a chance to recreate the unit test. I'll do it this week

andresesfm · 2023-04-06T02:06:02Z

@Mkolakoglu can you please take a look at the merge conflicts. Let's get this merged

Mkolakoglu · 2023-04-06T10:30:51Z

@Mkolakoglu can you please take a look at the merge conflicts. Let's get this merged

@andresesfm Done ✅

Also keep the old check for completeness

andresesfm · 2023-04-06T18:18:52Z

I added the old check back as a fallback. if does not interfere with your change

fix type

Mkolakoglu added 2 commits March 16, 2023 13:12

ensureConnection - fixed rejection handling

e2f05c0

ensureConnection - improve rejection handling

563f50b

Mkolakoglu requested a review from andresesfm March 16, 2023 12:40

Mkolakoglu added the 🤖 android Related to android label Mar 16, 2023

Mkolakoglu commented Mar 17, 2023

View reviewed changes

andresesfm added 👣 waiting for response Need feedback to continue ℹ needs more info Needs more detailed information to move forward labels Mar 17, 2023

Mkolakoglu removed 👣 waiting for response Need feedback to continue ℹ needs more info Needs more detailed information to move forward labels Mar 22, 2023

andresesfm added the 👣 waiting for response Need feedback to continue label Mar 23, 2023

Mkolakoglu removed the 👣 waiting for response Need feedback to continue label Mar 24, 2023

Mkolakoglu added 2 commits April 6, 2023 12:08

Merge branch 'main' into fix-ensureConnection-rejection-handling

e1cc2f2

lint

901e7a0

Update RNIapModule.kt

b13e553

Also keep the old check for completeness

Update RNIapModule.kt

77756b1

fix type

andresesfm merged commit 2f0403a into dooboolab-community:main Apr 6, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensureConnection - fixed rejection handling #2316

ensureConnection - fixed rejection handling #2316

Mkolakoglu commented Mar 16, 2023 •

edited

andresesfm commented Mar 16, 2023

andresesfm commented Mar 16, 2023

Mkolakoglu Mar 17, 2023

Mkolakoglu Mar 17, 2023

Mkolakoglu Mar 17, 2023

Mkolakoglu Mar 17, 2023

Mkolakoglu commented Mar 17, 2023

andresesfm commented Mar 17, 2023

Mkolakoglu commented Mar 20, 2023 •

edited

andresesfm commented Mar 23, 2023

Mkolakoglu commented Mar 24, 2023

Mkolakoglu commented Mar 24, 2023 •

edited

Mkolakoglu commented Apr 3, 2023

andresesfm commented Apr 5, 2023

andresesfm commented Apr 6, 2023

Mkolakoglu commented Apr 6, 2023

andresesfm commented Apr 6, 2023

ensureConnection - fixed rejection handling #2316

ensureConnection - fixed rejection handling #2316

Conversation

Mkolakoglu commented Mar 16, 2023 • edited

andresesfm commented Mar 16, 2023

andresesfm commented Mar 16, 2023

Mkolakoglu Mar 17, 2023

Choose a reason for hiding this comment

Mkolakoglu Mar 17, 2023

Choose a reason for hiding this comment

Mkolakoglu Mar 17, 2023

Choose a reason for hiding this comment

Mkolakoglu Mar 17, 2023

Choose a reason for hiding this comment

Mkolakoglu commented Mar 17, 2023

andresesfm commented Mar 17, 2023

Mkolakoglu commented Mar 20, 2023 • edited

andresesfm commented Mar 23, 2023

Mkolakoglu commented Mar 24, 2023

Mkolakoglu commented Mar 24, 2023 • edited

Mkolakoglu commented Apr 3, 2023

andresesfm commented Apr 5, 2023

andresesfm commented Apr 6, 2023

Mkolakoglu commented Apr 6, 2023

andresesfm commented Apr 6, 2023

Mkolakoglu commented Mar 16, 2023 •

edited

Mkolakoglu commented Mar 20, 2023 •

edited

Mkolakoglu commented Mar 24, 2023 •

edited