Fix odd behavior in Stripe.retrievePossibleBrands #6977

tillh-stripe · 2023-07-07T17:59:48Z

Summary

This pull request fixes some odd behavior that affects stripe.retrievePossibleBrands().

In RemoteCardAccountRangeSource, we stored any response from the backend in our persistent storage. This is problematic for two reasons: Firstly, the backend returns empty ranges for (seemingly?) valid BINs. Secondly, we don’t filter out invalid responses due to network or parsing errors. In both cases, we’d store an empty response on the device, which we’d continue to serve to callers into eternity, as we never clear the local storage. Now, we will fall back to the static ranges if the response from the local or remote data sources are empty.

Also, the conditions under which we called onCardMetadataMissingRange() was wrong, leading us to send way too many events.

Motivation

Correctly working card brand functionality in preparation for CBC.

Testing

Added tests
Modified tests
Manually verified

Screenshots

Before	After
before screenshot	after screenshot

Changelog

[FIXED] Fixed an issue where Stripe.retrievePossibleBrands() returned incorrect results.

tillh-stripe · 2023-07-07T18:00:27Z

payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt


-            val accountRanges =
-                stripeRepository.getCardMetadata(bin, requestOptions)?.accountRanges.orEmpty()
-            cardAccountRangeStore.save(bin, accountRanges)


We no longer store any response, as these could include null responses due to network errors. Instead, the repository will decide whether to store a result.

tillh-stripe · 2023-07-07T18:07:22Z

payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt

-                    accountRanges
+            if (!accountRanges.isNullOrEmpty()) {
+                val hasMatch = accountRanges.any { it.binRange.matches(cardNumber) }
+                if (!hasMatch && cardNumber.isValidLuhn) {


Re-adding the hasMatch check that was removed in https://github.com/stripe/stripe-android/pull/6335/files#diff-d6f7381dc56080adb96df46f224f39adff555b81bdff2952944a80530f1d9e03L39.

cc @jameswoo-stripe @davidme-stripe @wooj-stripe: I’m not fully sure if this implementation is correct. It seems more correct than before, but iOS does have some additional logic that I don’t understand. Any thoughts?

Sorry, I don't remember why I removed that piece of logic :( It does seem odd that it was removed!

tillh-stripe · 2023-07-07T18:08:27Z

payments-core/src/test/java/com/stripe/android/cards/CardAccountRangeServiceTest.kt

@@ -77,7 +75,7 @@ class CardAccountRangeServiceTest {
            } else {
                never()
            }
-        ).getAccountRange(cardNumber)
+        ).getAccountRanges(cardNumber)


The implementation changed, so the test needs to change 😕

tillh-stripe · 2023-07-07T18:10:30Z

payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt

+        if (!ranges.isNullOrEmpty()) {
+            store.save(bin, ranges)
+        }
+
+        return ranges?.takeIf { it.isNotEmpty() } ?: staticSource.getAccountRanges(cardNumber)


We only save the response if we actually get a non-empty one. The backend service returns an empty list for some valid BINs (🤷) and I assume we don’t want to serve those but would rather fall back to the static ranges.

I do think that the remove backend service is the source of truth, we probably should not rely on our static list. I think as long as the response is successful, we should use that data.

This is inconsistent with what we’re doing in PaymentSheet then, where we take the static ranges into account (cc @davidme-stripe and @wooj-stripe for help)

The BIN data is often wrong, even in cases where it doesn't return an empty response. (See RUN_MOBILESDK-2135 for more discussion on this.)

On iOS in the card field, we only fetch it for UnionPay BINs as they could be 19 digit cards (81/62). For this possibleCardBrands API, we actually maintain a separate cache, as we don't want to pollute the card text field's cache with bad data. Would that explain the difference in behavior you're seeing?

@davidme-stripe We seem to have the same logic on Android in CardAccountRangeService — see shouldQueryRepository, which only returns true for unknown brands and UnionPay. That service is used in the card field and PaymentSheet, but not for possibleCardBrands. Should it be?

@jameswoo-stripe We already take the static ranges into account right now, but only on the very first call for a given prefix. Afterwards, the fact that we store things on the device makes us return an empty list instead of results from the static ranges.

tillh-stripe · 2023-07-07T18:12:00Z

payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeStore.kt

+        private const val VERSION = 2
+        private const val PREF_FILE = "InMemoryCardAccountRangeSource.Store.$VERSION"


Updating the key here so that we can ”force-refresh”. I’m doing this because we might already be in a corrupted state for existing users.

github-actions · 2023-07-07T18:12:16Z

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: V1, V2)
NEW: paymentsheet-example-release-pr.apk (signature: V1, V2)

          │          compressed           │         uncompressed         
          ├───────────┬───────────┬───────┼──────────┬──────────┬────────
 APK      │ old       │ new       │ diff  │ old      │ new      │ diff   
──────────┼───────────┼───────────┼───────┼──────────┼──────────┼────────
      dex │   3.4 MiB │   3.4 MiB │ -73 B │  7.4 MiB │  7.4 MiB │ -300 B 
     arsc │   2.1 MiB │   2.1 MiB │   0 B │  2.1 MiB │  2.1 MiB │    0 B 
 manifest │   4.9 KiB │   4.9 KiB │   0 B │ 23.8 KiB │ 23.8 KiB │    0 B 
      res │ 868.7 KiB │ 868.7 KiB │   0 B │  1.3 MiB │  1.3 MiB │    0 B 
   native │   2.6 MiB │   2.6 MiB │   0 B │    6 MiB │    6 MiB │    0 B 
    asset │     3 MiB │     3 MiB │  +1 B │    3 MiB │    3 MiB │   +1 B 
    other │ 199.8 KiB │ 199.8 KiB │  +2 B │  447 KiB │  447 KiB │    0 B 
──────────┼───────────┼───────────┼───────┼──────────┼──────────┼────────
    total │  12.1 MiB │  12.1 MiB │ -70 B │ 20.3 MiB │ 20.3 MiB │ -299 B 

 DEX     │ old   │ new   │ diff           
─────────┼───────┼───────┼────────────────
   files │     1 │     1 │  0             
 strings │ 36364 │ 36364 │  0 (+3 -3)     
   types │ 12093 │ 12092 │ -1 (+0 -1)     
 classes │ 10196 │ 10195 │ -1 (+0 -1)     
 methods │ 53630 │ 53627 │ -3 (+101 -104) 
  fields │ 33753 │ 33749 │ -4 (+82 -86)   

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  291 │  291 │  0   
 entries │ 6924 │ 6924 │  0

APK

    compressed    │    uncompressed    │                               
──────────┬───────┼───────────┬────────┤                               
 size     │ diff  │ size      │ diff   │ path                          
──────────┼───────┼───────────┼────────┼───────────────────────────────
  3.4 MiB │ -73 B │   7.4 MiB │ -300 B │ ∆ classes.dex                 
 62.7 KiB │  +4 B │ 140.9 KiB │    0 B │ ∆ META-INF/CERT.SF            
 48.5 KiB │  -2 B │ 140.9 KiB │    0 B │ ∆ META-INF/MANIFEST.MF        
  6.4 KiB │  +1 B │   6.3 KiB │   +1 B │ ∆ assets/dexopt/baseline.prof 
──────────┼───────┼───────────┼────────┼───────────────────────────────
  3.5 MiB │ -70 B │   7.7 MiB │ -299 B │ (total)

DEX

STRINGS:

   old   │ new   │ diff      
  ───────┼───────┼───────────
   36364 │ 36364 │ 0 (+3 -3) 
  + InMemoryCardAccountRangeSource.Store.2
  + accountRanges
  + ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:28b26f3,r8-mode:full,version:8.0.46}
  
  - InMemoryCardAccountRangeSource.Store
  - Llc/s0;
  - ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:f84dafc,r8-mode:full,version:8.0.46}
  

TYPES:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   12093 │ 12092 │ -1 (+0 -1) 
  - Llc/s0;
  

METHODS:

   old   │ new   │ diff           
  ───────┼───────┼────────────────
   53630 │ 53627 │ -3 (+101 -104) 
  + b9.b <init>(e, Context, r0, y, Boolean, a, a)
  + be.f <init>(r0, a, d, h, Set)
  + be.j <init>(r0, a, h)
  + com.google.crypto.tink.shaded.protobuf.q <init>(q0)
  + fa.t0 <init>(Application, Stack, z, Resources, l, d, r0, i, f, boolean, a)
  + ge.d <init>(q0, a, e, x)
  + jc.u <init>(q0)
  + l9.d1 <init>(Context, o0, q0, boolean, h, int)
  + lc.a0 <init>(q0, d)
  + lc.b0 <init>(q0, d)
  + lc.c0 <init>(q0, d)
  + lc.d0 <init>(q0, d)
  + lc.d c(Context, r0, c, f, boolean, h, h, Map, a, Set, boolean) → a
  + lc.e0 <init>(q0, d)
  + lc.f0 <init>(q0, d)
  + lc.g0 <init>(q0, d)
  + lc.g <init>(q0, d)
  + lc.h0 <init>(q0, d)
  + lc.i0 <init>(q0, d)
  + lc.j0 <init>(q0, d)
  + lc.k0 <init>(q0, d)
  + lc.k <init>(q0, d)
  + lc.l0 <init>(q0, d)
  + lc.l <init>(q0, d)
  + lc.m0 <init>(q0, d)
  + lc.m <init>(q0, d)
  + lc.n0 <init>(q0, d)
  + lc.n <init>(q0, Set, int)
  + lc.o0 <init>(q0, d)
  + lc.o <init>(q0, d)
  + lc.p0 <init>(q0, d)
  + lc.p <init>(q0, int)
  + lc.q0 <clinit>()
  + lc.q0 <init>(Context, a, h, Set, f, c, d)
  + lc.q0 <init>(Context, a, d, h, Set, c, f, Set, int)
  + lc.q0 A(o, i, d) → Object
  + lc.q0 B(String, i, d) → Object
  + lc.q0 C(String, Set, i, d) → Object
  + lc.q0 D(a1, i, d) → Object
  + lc.q0 E(a1, i, d) → Object
  + lc.q0 F(String, i, List, d) → Object
  + lc.q0 G(String, i, List, d) → Object
  + lc.q0 H(String, i, List, d) → Object
  + lc.q0 I(r4, i, d) → Object
  + lc.q0 a(String, String, String, i, List, d) → Object
  + lc.q0 b(String, String, String, i, List, d) → Object
  + lc.q0 c(String, Set, String, i, d) → Object
  + lc.q0 d(Set) → String
  + lc.q0 e(i, String, String, d) → Object
  + lc.q0 f(i, String, String, d) → Object
  + lc.q0 g(String, i, d) → Object
  + lc.q0 h(o, i, List, d) → Object
  + lc.q0 i(o, i, List, d) → Object
  + lc.q0 j(p, i, List, d) → Object
  + lc.q0 k(String, String, String, String, Locale, String, int, i, d) → Object
  + lc.q0 l(String, List) → LinkedHashMap
  + lc.q0 m(j0, i, d) → Object
  + lc.q0 n(String, w, i, d) → Object
  + lc.q0 o(String, k0, i, d) → Object
  + lc.q0 p(l3, i, d) → Object
  + lc.q0 q(String, k0, i, d) → Object
  + lc.q0 r(Set, String, i, d) → Object
  + lc.q0 s(j, b, a, d) → Object
  + lc.q0 t(PaymentAnalyticsEvent)
  + lc.q0 u()
  + lc.q0 v(a, i, d) → Object
  + lc.q0 w(i, d) → Object
  + lc.q0 x(g1, Set, i, d) → Object
  + lc.q0 y(j, a, d) → Object
  + lc.q0 z(Map, l3, n4) → Map
  + lc.q <init>(q0, d)
  + lc.r <init>(q0, d)
  + lc.s <init>(q0, d)
  + lc.t <init>(q0, d)
  + lc.u <init>(q0, d)
  + lc.v <init>(q0, d)
  + lc.w <init>(q0, d)
  + lc.x <init>(q0, d)
  + lc.y <init>(q0, d)
  + lc.z <init>(q0, d)
  + mc.h <init>(Context, w0, r0, d, h)
  + mc.l <init>(Context, a, r0, d, h)
  + mc.p <init>(Context, a, r0, d, h)
  + pc.c <init>(q0)
  + pc.g <init>(q0)
  + r.n a() → q0
  + sb.f0 <init>(b0, y, z, e, boolean, Context, c, Set, a, a, boolean, h, f, c, r0)
  + sb.q0 <init>(p, i, j0, r0, q, b, k1)
  + sb.u <init>(p, i, m, q0, d1, q, b, k1)
  + t9.y <init>(q0, i, u, n, f)
  + tc.d <init>(r0, c, f, y, d, h)
  + tc.w <init>(n, r0, c, f, a, a1, j, l0, h, k1, boolean)
  + ub.b <init>(r0)
  + uc.b <init>(h, e, Context, r0, c, f, Boolean, h, h, Map, String, a, Set, Boolean)
  + uc.c <init>(d, e, Context, Boolean, h, h, r0, f, a, a, Set)
  + wc.q <init>(boolean, r0, g, a, a, Map, a, a, n, f, h, k1, boolean)
  + wc.v <init>(a, a, e, Integer, Context, boolean, h, h, r0, f, Set)
  + xc.l <init>(Context, r0, boolean, a, a)
  + yc.k t1(r0, String, i, d) → Object
  + yf.f1 <init>(Application, String, q0)
  + zb.g <init>(a, a, r0, a, h, Locale)
  
  - b9.b <init>(e, Context, s0, y, Boolean, a, a)
  - be.f <init>(s0, a, d, h, Set)
  - be.j <init>(s0, a, h)
  - com.google.crypto.tink.shaded.protobuf.q <init>(r0)
  -
...✂

jameswoo-stripe · 2023-07-07T19:11:58Z

payments-core/src/test/java/com/stripe/android/cards/RemoteCardAccountRangeSourceTest.kt

@@ -24,14 +24,12 @@ import kotlin.test.Test

 @RunWith(RobolectricTestRunner::class)
 internal class RemoteCardAccountRangeSourceTest {
-    private val cardAccountRangeStore = mock<CardAccountRangeStore>()


Should we have a test that does check if the data was saved?

jameswoo-stripe · 2023-07-07T19:13:12Z

payments-core/src/main/java/com/stripe/android/cards/DefaultCardAccountRangeRepository.kt

+        if (!ranges.isNullOrEmpty()) {
+            store.save(bin, ranges)
+        }
+
+        return ranges?.takeIf { it.isNotEmpty() } ?: staticSource.getAccountRanges(cardNumber)


I do think that the remove backend service is the source of truth, we probably should not rely on our static list. I think as long as the response is successful, we should use that data.

jameswoo-stripe · 2023-07-07T19:16:27Z

payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt

-                    accountRanges
+            if (!accountRanges.isNullOrEmpty()) {
+                val hasMatch = accountRanges.any { it.binRange.matches(cardNumber) }
+                if (!hasMatch && cardNumber.isValidLuhn) {


Sorry, I don't remember why I removed that piece of logic :( It does seem odd that it was removed!

tillh-stripe · 2023-07-11T15:45:01Z

@jameswoo-stripe I reduced the diff so that my changes are more clearly visible.

jaynewstrom-stripe

Do we have tests to verify we're using the stored account ranges?

payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt

Store an empty response to avoid queries, but fall back to static ranges if the stored/fetched response is empty.

* Fix odd behavior in Stripe.possibleCardBrands * Update tests * Reduce queries for card account ranges Store an empty response to avoid queries, but fall back to static ranges if the stored/fetched response is empty. * Fix test * Update remote source test for new behavior * Make minor tweaks * Add changelog entry * Add more tests * Remove method that’s now unused

tillh-stripe commented Jul 7, 2023

View reviewed changes

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from ee863ed to 6f5c77a Compare July 7, 2023 18:20

jameswoo-stripe reviewed Jul 7, 2023

View reviewed changes

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from 6f5c77a to e9e8972 Compare July 11, 2023 15:44

tillh-stripe changed the title ~~Fix odd behaviors in Stripe.possibleCardBrands~~ Fix odd behavior in Stripe.possibleCardBrands Jul 14, 2023

tillh-stripe changed the title ~~Fix odd behavior in Stripe.possibleCardBrands~~ Fix odd behavior in Stripe.retrievePossibleBrands Jul 14, 2023

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from 79a91c1 to 8a1554c Compare July 18, 2023 20:36

tillh-stripe marked this pull request as ready for review July 18, 2023 20:42

tillh-stripe requested review from a team as code owners July 18, 2023 20:42

tillh-stripe requested review from jameswoo-stripe, davidme-stripe and jaynewstrom-stripe July 18, 2023 20:42

jameswoo-stripe previously approved these changes Jul 18, 2023

View reviewed changes

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from 7554fa8 to 1d24b54 Compare July 18, 2023 23:02

jaynewstrom-stripe reviewed Jul 19, 2023

View reviewed changes

payments-core/src/main/java/com/stripe/android/cards/RemoteCardAccountRangeSource.kt Show resolved Hide resolved

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from 1d24b54 to 9eea0bd Compare July 19, 2023 18:25

tillh-stripe dismissed jameswoo-stripe’s stale review via 208051a July 19, 2023 18:44

jaynewstrom-stripe previously approved these changes Jul 19, 2023

View reviewed changes

tillh-stripe added 5 commits July 19, 2023 15:05

Fix odd behavior in Stripe.possibleCardBrands

50349b5

Update tests

548a136

Reduce queries for card account ranges

947bfda

Store an empty response to avoid queries, but fall back to static ranges if the stored/fetched response is empty.

Fix test

3c4bfb2

Update remote source test for new behavior

79eb92d

tillh-stripe added 4 commits July 19, 2023 15:05

Make minor tweaks

5b12cf2

Add changelog entry

133af8b

Add more tests

892888f

Remove method that’s now unused

d9722cb

tillh-stripe dismissed jaynewstrom-stripe’s stale review via d9722cb July 19, 2023 19:05

tillh-stripe force-pushed the tillh/possible-card-brand-fixes branch from 208051a to d9722cb Compare July 19, 2023 19:05

jaynewstrom-stripe approved these changes Jul 19, 2023

View reviewed changes

tillh-stripe merged commit 35c436e into master Jul 19, 2023

tillh-stripe deleted the tillh/possible-card-brand-fixes branch July 19, 2023 19:36

stripe deleted a comment from nosleep71 Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix odd behavior in Stripe.retrievePossibleBrands #6977

Fix odd behavior in Stripe.retrievePossibleBrands #6977

tillh-stripe commented Jul 7, 2023 •

edited

Loading

tillh-stripe Jul 7, 2023

tillh-stripe Jul 7, 2023

jameswoo-stripe Jul 7, 2023

tillh-stripe Jul 7, 2023

tillh-stripe Jul 7, 2023

jameswoo-stripe Jul 7, 2023

tillh-stripe Jul 10, 2023

davidme-stripe Jul 10, 2023

tillh-stripe Jul 11, 2023

tillh-stripe Jul 7, 2023

github-actions bot commented Jul 7, 2023 •

edited

Loading

jameswoo-stripe Jul 7, 2023

jameswoo-stripe Jul 7, 2023

jameswoo-stripe Jul 7, 2023

tillh-stripe commented Jul 11, 2023

jaynewstrom-stripe left a comment

		private const val VERSION = 2
		private const val PREF_FILE = "InMemoryCardAccountRangeSource.Store.$VERSION"

Fix odd behavior in Stripe.retrievePossibleBrands #6977

Fix odd behavior in Stripe.retrievePossibleBrands #6977

Conversation

tillh-stripe commented Jul 7, 2023 • edited Loading

Summary

Motivation

Testing

Screenshots

Changelog

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

github-actions bot commented Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillh-stripe commented Jul 11, 2023

jaynewstrom-stripe left a comment

Choose a reason for hiding this comment

tillh-stripe commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023 •

edited

Loading