Make Balance and Capture use sync endpoints #288

devinmorgan · 2024-07-11T19:15:10Z

What

Swaps out polling usage in CapturePaymentRepository and CheckBalanceRepository with sync endpoints by bumping the API-VERSION header from default -> 2024-01-08.

Sources of complexity in this PR:

Since Rosetta uses the same OkHttpClientBuilder as our model-fetching GET requests use, it was necessary to modify our OkHttp facilities to support specifying a custom API-VERSION and using "default" as the API-VERSION header when left unspecified. Since BT uses its own network facilities, specifying API-VERSION: 2024-01-08 was less involved.
BT started returning gson representations of JSON instead of ferrying the JSON as a string along so we had to navigate this gracefully

Why

We've wanted to do this for almost a quarter now in Android. It's already done in iOS and JS

Test Plan

✅ I've added unit tests for this change.
❌ You can manually test the changes in this PR if you want, but I'll be doing a designated bug bash on Friday, July 26th at 10:45 am Eastern.

Demo

CI tests pass!

How

Must be merged after previous PRs in the stack

devinmorgan · 2024-07-11T19:15:26Z

Update sdk + sample app to use single error #295
Drop no-longer-used polling and SQS message code #289
Make Balance and Capture use sync endpoints #288 👈
Organize ForageApiResponse.Failure logic #290
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @devinmorgan and the rest of your teammates on Graphite

devinmorgan · 2024-07-11T19:16:58Z

...rc/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BasisTheoryPinSubmitter.kt

@@ -49,7 +49,7 @@ internal class BasisTheoryPinSubmitter(
        val bt = buildVaultProvider()

        val proxyRequest: ProxyRequest = ProxyRequest().apply {
-            headers = vaultProxyRequest.headers
+            headers = vaultProxyRequest.headers + (ForageConstants.Headers.API_VERSION to "2024-01-08")


Acknowledging that this is ugly. I have plans to clean this up in the future

devinmorgan · 2024-07-24T10:35:29Z

...in/java/com/joinforage/forage/android/core/services/forageapi/network/OkHttpClientBuilder.kt

-                        // If the API_VERSION header has already been appended, don't override it!
-                        chain.request().headers[ForageConstants.Headers.API_VERSION]?.let {
-                            this
+                    .apply {
+                        if (chain.request().headers[ForageConstants.Headers.API_VERSION] == null) {
+                            addHeader(ForageConstants.Headers.API_VERSION, apiVersion)
                        }
-                            // Otherwise, set the default API_VERSION header
-                            ?: addHeader(
-                                ForageConstants.Headers.API_VERSION,
-                                "default"
-                            )


Opportunity to make the code easier to understand

devinmorgan · 2024-07-24T10:38:58Z

...in/java/com/joinforage/forage/android/core/services/forageapi/network/OkHttpClientBuilder.kt

@@ -19,7 +19,8 @@ internal object OkHttpClientBuilder {
        sessionToken: String,
        merchantId: String? = null,
        idempotencyKey: String? = null,
-        traceId: String? = null
+        traceId: String? = null,
+        apiVersion: String = "default"


provideOkHttpClient get's used by a lot of callees. Declaring the apiVersion with a param that defaults to default will decouple these callers from needing to work directly with OkHttpClient.addHeader and the ForageConstants.Headers. Less coupling makes for more maintainable code!

devinmorgan · 2024-07-24T10:40:53Z

.../com/joinforage/forage/android/core/services/forageapi/network/error/payload/ErrorPayload.kt

@@ -44,7 +44,7 @@ internal abstract class ErrorPayload(
                    return RosettaErrorResponse(jsonErrorResponse)
                }
                else -> {
-                    throw UnexpectedResponseError(jsonErrorResponse.toString())
+                    throw UnknownForageFailureResponse(jsonErrorResponse.toString())


The new name more precisely describes that this exception is thrown when we there is a response that we believe is Forage-defined that we are unable to parse

devinmorgan · 2024-07-24T11:34:17Z

...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt

-        return try {
-            // In AbstractVaultSubmitter we track the forageError
-            // and the forage status code of the UnknownErrorApiResponse
-            // via the VaultProxyResponseMonitor. This keeps us informed
-            // of *when* erroroneous BT response happen. Unfortunately,
-            // we do not currently track the specifics of the proxy_error
-            // that that BT returned to us.
-            // TODO: log the specific error that BT responds with when
-            //  resRegExp.containsProxyError == true
-            if (resRegExp.containsProxyError) UnknownErrorApiResponse else null
-        } catch (_: Exception) {
-            null
-        }


The try catch block is not necessary here because regexp matching won't throw so I remove it in this PR. To elaborate on why it won't throw:

if the regexp don't find a match they return null and the compiler enforces that we use null-safety accessors (?.).

if the regexp does match, .get(0) always refers to the entire match and .get(1) refers to the first matched group

Point being that there won't ever be an index-out-of-bounds scenario by doing .get(1) or any other error from not finding a regexp match. I added unit tests in BTResponseParserText.kt and BTFailureResponseRegExp.kt to confirm

A learning I took from implementing these changes in iOS and JS is that we should really force all error scenarios through the BT codepath to ensure we are happy with the parsing. The error scenarios are:

EBT network error code (eg. ebt_error_55)

Forage API error (eg. invalid_merchant_header)

BT error (eg. IDK COULD BE ANYTHING)

Great point! I'll incorporate this into the bug bash on Friday. Thanks for surfacing this!!

devinmorgan · 2024-07-24T11:38:31Z

...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt

-        return try {
-            ForageApiResponse.Failure(resRegExp.statusCode, resRegExp.bodyText)
-        } catch (_: Exception) {
+    private fun parseForageError(resRegExp: BtFailureResponseRegExp): ForageApiResponse.Failure? =
+        if (resRegExp.bodyText == null ||
+            resRegExp.statusCode == null ||
+            // if there's a proxy_error, don't try to parse as a Forage error
+            resRegExp.containsProxyError


I replace the try/catch with an additional clause in the if-statement that confirms resRegExp.containsProxyError == false before attempting to parse as a ForageError. It is true that attempting to parse an error that is not a Forage error with ForageApiResponse.Failure(..) will throw an exception. If the error is not a proxy_error, then it can only be a ForageError and if we are unable to parse the FroageError, that is an exceptional circumstance and we need to throw.

NOTE: the callee of BTResponseParser will catch the exception, log it, and gracefully return UnknownServerError

devinmorgan · 2024-07-24T11:43:19Z

...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt

+            val jsonResponse = when {
+                (result == null) -> JSONObject()
+                (result == "") -> JSONObject()
+                (result is Map<*, *>) -> JSONObject(result)


For whatever reason, using API-VERSION: 2024-01-08 caused BT to start returning a Gson representation of the JSON response. While this is a bad oversight of BT to leak a third-party dependency in their public API, fortunately, the Gson representation can be cast as a Map<*, *> and the built-int JSONObject constructor correctly parses.

devinmorgan · 2024-07-24T11:44:11Z

...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt

+                (result == null) -> JSONObject()
+                (result == "") -> JSONObject()


These are to gracefully handle the deferred pin-submission flows since they technically have an empty body and were throwing with the new API-VERSION: 2024-01-08

devinmorgan · 2024-07-24T11:45:10Z

...rc/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BasisTheoryPinSubmitter.kt

+        return try {
+            vaultToForageResponse(BTResponseParser(vaultResponse))
+        } catch (e: UnknownBTSuccessResponse) {
+            logger.e("Unknown success response from BasisTheory Vault.", e)
+            UnknownErrorApiResponse
+        } catch (e: UnknownForageFailureResponse) {
+            logger.e("Unknown error from Payments API.", e)
+            UnknownErrorApiResponse
+        } catch (e: Exception) {
+            logger.e("Request to Basis Theory failed.", e)
+            UnknownErrorApiResponse
+        }


and this is where we catch the various exceptions that could arise from parsing BT responses

devinmorgan · 2024-07-24T11:47:07Z

...oid/src/test/java/com/joinforage/forage/android/network/data/CapturePaymentRepositoryTest.kt

remove tests that tested whether polling was explicitly used or not, as polling no longer happens with sync endpoints. This is a separate endeavor from dropping all the polling code, which is done in the next PR

devinmorgan · 2024-07-24T11:47:54Z

forage-android/src/test/java/com/joinforage/forage/android/vault/BasisTheoryPinSubmitterTest.kt

@@ -52,7 +52,7 @@ class BasisTheoryPinSubmitterTest() : MockServerSuite() {
        mockBasisTheoryTextElement = mock(TextElement::class.java)

        // ensure we don't make any live requests!
-        mockBasisTheoryResponse(Result.success("success"))
+        mockBasisTheoryResponse(Result.success(mapOf("mySuccessfulKey" to "mySuccessfulValue")))


Updating because BT starts returning Gson representation of JSON instead of just the JSON string

.../src/main/java/com/joinforage/forage/android/core/services/vault/CapturePaymentRepository.kt

...id/src/main/java/com/joinforage/forage/android/core/services/vault/CheckBalanceRepository.kt

dleis612 · 2024-07-24T14:26:42Z

...droid/src/main/java/com/joinforage/forage/android/ecom/services/vault/bt/BTResponseParser.kt

-        return try {
-            // In AbstractVaultSubmitter we track the forageError
-            // and the forage status code of the UnknownErrorApiResponse
-            // via the VaultProxyResponseMonitor. This keeps us informed
-            // of *when* erroroneous BT response happen. Unfortunately,
-            // we do not currently track the specifics of the proxy_error
-            // that that BT returned to us.
-            // TODO: log the specific error that BT responds with when
-            //  resRegExp.containsProxyError == true
-            if (resRegExp.containsProxyError) UnknownErrorApiResponse else null
-        } catch (_: Exception) {
-            null
-        }


A learning I took from implementing these changes in iOS and JS is that we should really force all error scenarios through the BT codepath to ensure we are happy with the parsing. The error scenarios are:

EBT network error code (eg. ebt_error_55)

Forage API error (eg. invalid_merchant_header)

BT error (eg. IDK COULD BE ANYTHING)

Note that we are not deleting any polling code here - we're just inserting the sync endpoints into the flow. We are only deleting test cases testing internal details of how pin-submission works as SQS messages are simply not involved in the flow with sync-endpoints Signed-off-by: Devin Morgan <[email protected]>

BT returns different shaped responses with API-VESION: 2024-01-08. To ensure reliability, it's time to add unit test for these facillities.

Signed-off-by: Devin Morgan <[email protected]>

We have the ability now to differentiate between a failure to parse what should be a ForageError and a failure to parse what should be a successful response from BT. Signed-off-by: Devin Morgan <[email protected]>

It took me a very long time to understand why we go back and forth between ForageApiResponse.Success and EbtBalance so I'm adding this comments to clarify for other developers Signed-off-by: Devin Morgan <[email protected]>

Signed-off-by: Devin Morgan <[email protected]>

devinmorgan · 2024-07-24T20:41:31Z

Merge activity

Jul 24, 4:41 PM EDT: @devinmorgan merged this pull request with Graphite.

* Insert sync endpoints in BT and Rosetta Note that we are not deleting any polling code here - we're just inserting the sync endpoints into the flow. We are only deleting test cases testing internal details of how pin-submission works as SQS messages are simply not involved in the flow with sync-endpoints Signed-off-by: Devin Morgan <[email protected]> * Add BT parsing tests BT returns different shaped responses with API-VESION: 2024-01-08. To ensure reliability, it's time to add unit test for these facillities. * Apply spotless Signed-off-by: Devin Morgan <[email protected]> * Catch and log BT response parsing exceptions We have the ability now to differentiate between a failure to parse what should be a ForageError and a failure to parse what should be a successful response from BT. Signed-off-by: Devin Morgan <[email protected]> * Add comments for balance type back-and-forth It took me a very long time to understand why we go back and forth between ForageApiResponse.Success and EbtBalance so I'm adding this comments to clarify for other developers Signed-off-by: Devin Morgan <[email protected]> * Re-drop setting API-VERSION: default in AbstractVaultSubmitter Signed-off-by: Devin Morgan <[email protected]> * Get rid of unnecessary when statements and logs Signed-off-by: Devin Morgan <[email protected]> * Fix typo Signed-off-by: Devin Morgan <[email protected]> --------- Signed-off-by: Devin Morgan <[email protected]>

devinmorgan mentioned this pull request Jul 11, 2024

Drop no-longer-used polling and SQS message code #289

Merged

devinmorgan commented Jul 11, 2024

View reviewed changes

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch 3 times, most recently from 596fa5c to a93d1f2 Compare July 15, 2024 18:35

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from a93d1f2 to 5a2a00f Compare July 22, 2024 16:45

devinmorgan changed the base branch from main to devin/organize-forage-api-response-failures July 22, 2024 16:45

devinmorgan mentioned this pull request Jul 22, 2024

Organize ForageApiResponse.Failure logic #290

Merged

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from 5a2a00f to 72c3b74 Compare July 22, 2024 17:08

devinmorgan force-pushed the devin/organize-forage-api-response-failures branch from 8a3a0ba to f8e4aa9 Compare July 23, 2024 13:02

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from 72c3b74 to 8869b6f Compare July 23, 2024 13:02

devinmorgan mentioned this pull request Jul 23, 2024

Update sdk + sample app to use single error #295

Merged

devinmorgan commented Jul 24, 2024

View reviewed changes

devinmorgan force-pushed the devin/organize-forage-api-response-failures branch from f8e4aa9 to c57c042 Compare July 24, 2024 11:25

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from 8869b6f to ae3f7cb Compare July 24, 2024 11:25

devinmorgan commented Jul 24, 2024

View reviewed changes

devinmorgan marked this pull request as ready for review July 24, 2024 12:03

devinmorgan requested review from owenkim, ForageRob, dleis612 and djoksimo as code owners July 24, 2024 12:03

devinmorgan requested a review from evanfreeze as a code owner July 24, 2024 12:03

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch 2 times, most recently from d0a288c to 5c38867 Compare July 24, 2024 13:19

dleis612 requested changes Jul 24, 2024

View reviewed changes

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from f46d91d to 1f1d6d9 Compare July 24, 2024 19:52

devinmorgan requested a review from dleis612 July 24, 2024 20:20

devinmorgan added 8 commits July 24, 2024 20:27

Add BT parsing tests

cef47b5

BT returns different shaped responses with API-VESION: 2024-01-08. To ensure reliability, it's time to add unit test for these facillities.

Apply spotless

8ce4372

Signed-off-by: Devin Morgan <[email protected]>

Catch and log BT response parsing exceptions

7ec5052

We have the ability now to differentiate between a failure to parse what should be a ForageError and a failure to parse what should be a successful response from BT. Signed-off-by: Devin Morgan <[email protected]>

Add comments for balance type back-and-forth

f8da0f5

It took me a very long time to understand why we go back and forth between ForageApiResponse.Success and EbtBalance so I'm adding this comments to clarify for other developers Signed-off-by: Devin Morgan <[email protected]>

Re-drop setting API-VERSION: default in AbstractVaultSubmitter

c2ffe5d

Signed-off-by: Devin Morgan <[email protected]>

Get rid of unnecessary when statements and logs

a8e4db7

Signed-off-by: Devin Morgan <[email protected]>

Fix typo

e135910

Signed-off-by: Devin Morgan <[email protected]>

devinmorgan force-pushed the devin/insert-sync-endpoints-leave-polling-code-in branch from 1f1d6d9 to e135910 Compare July 24, 2024 20:27

devinmorgan changed the base branch from devin/organize-forage-api-response-failures to main July 24, 2024 20:27

dleis612 approved these changes Jul 24, 2024

View reviewed changes

devinmorgan merged commit dba38d7 into main Jul 24, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Balance and Capture use sync endpoints #288

Make Balance and Capture use sync endpoints #288

devinmorgan commented Jul 11, 2024 •

edited

Loading

devinmorgan commented Jul 11, 2024 •

edited

Loading

devinmorgan Jul 11, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024 •

edited

Loading

dleis612 Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024 •

edited

Loading

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

devinmorgan Jul 24, 2024

dleis612 Jul 24, 2024

devinmorgan commented Jul 24, 2024

		(result == null) -> JSONObject()
		(result == "") -> JSONObject()

Make Balance and Capture use sync endpoints #288

Make Balance and Capture use sync endpoints #288

Conversation

devinmorgan commented Jul 11, 2024 • edited Loading

What

Why

Test Plan

Demo

How

devinmorgan commented Jul 11, 2024 • edited Loading

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

devinmorgan Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinmorgan Jul 24, 2024 • edited Loading

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

devinmorgan commented Jul 24, 2024

Merge activity

devinmorgan commented Jul 11, 2024 •

edited

Loading

devinmorgan commented Jul 11, 2024 •

edited

Loading

devinmorgan Jul 24, 2024 •

edited

Loading

devinmorgan Jul 24, 2024 •

edited

Loading