`Purchases.customerInfo()`: added overload with a new `CacheFetchPolicy` by NachoSoto · Pull Request #1608 · RevenueCat/purchases-ios

NachoSoto · 2022-05-24T22:02:49Z

Fixes CF-19.
This allows specifying CustomerInfoManager fetching behavior, both as a public API for users, and also internally.
Some future uses of CustomerInfoManager might want to ensure that we don't rely on cached data, for example.

Thanks to the APITester we know that the existing methods still work, this is purely an additive change.

This is the core of the change:

/// Specifies the behavior for a caching API.
@objc(RCCacheFetchPolicy)
public enum CacheFetchPolicy: Int {
    /// Returns values from the cache, or throws an error if not available.
    case fromCacheOnly

    /// Always fetch the most up-to-date data.
    case fetchCurrent

    /// Returns the cached data if available and not stale, or fetches up-to-date data.
    case notStaleCachedOrFetched

    /// Default behavior: returns the cached data if available (even if stale), or fetches up-to-date data.
    case cachedOrFetched

    /// Default ``CacheFetchPolicy`` behavior.
    public static let `default`: Self = .cachedOrFetched
}

The new CustomerInfoManagerGetCustomerInfoTests check all the different combinations of fetching behavior.
I've taken some of the existing tests and refactored them to be async, as well as added a bunch more tests.

Note:
There is also one big hidden bug fix:

Purchases.getCustomerInfo now calls BackendError.asPurchasesError, so we don't end up returning an internal error type.
This was missed because we always return generic Error types, so we have no way to know at compile time that we only return ErrorCodes.

NachoSoto · 2022-05-24T22:50:15Z

Feedback welcome on these names.

NachoSoto · 2022-05-24T22:52:56Z

I think I covered all scenarios, but let me know if I missed any or if it'd be worth adding any more.

NachoSoto · 2022-05-24T22:54:59Z

I changed all these calls to explicitly pass the previous behavior. But now that we have this feature, it's possible that we might want to customize it and use a different one.

NachoSoto · 2022-05-24T22:55:27Z

The new error infrastructure makes this so easy 🎉

aboedo · 2022-05-31T13:32:03Z

looking great so far! and I ❤️ having the options. I left a comment regarding the current behavior

NachoSoto · 2022-05-31T18:06:17Z

⚠️ 🚨 This was wrong! We were returning a BackendErrorCode directly.

NachoSoto · 2022-05-31T18:07:25Z

This behavior makes a lot more sense. I wonder if we should change some calls to use this instead now.

I agree. Let's run it by the support team, they might have thoughts based on customer feedback.

I think we can merge this PR as is (with the current behavior), then ask the support team (and broader team) for feedback regarding the proposed change, then if folks agree make a release that makes this the default.
We'd have to also make a purchases-android equivalent release so that we don't have inconsistent behavior in the hybrids.

Hmm I have a concern about this fetch policy... If the cache is stale and we try to fetch while offline with this cache policy, we won't use the cached version even if it's stale right? I guess we should add a completion on the fetch so if the fetch fails, to use the cache if it exists even if it's stale. Thoughts?

🤔 If I called something that said "not stale" I'd expect it to never give me something potentially stale, so if it can't fetch, I think an error might be more appropriate than returning a stale cache for this one

Well, I can see your reasoning... I think this is ok as long as this option is not the default... But if we wanted to make this the default as was mentioned on this comment, we probably want to be more lenient and allow to use the stale data... I'm sure there are a ton of people that would use the apps while offline that would cause this to error out even if we had a cached value that the customer can potentially use.

that's a valid point. and also an argument for a "best effort" case, where if the cache is stale, we try to fetch, but if there are any problems we return the cached value.
@NachoSoto thoughts?

Well, I can see your reasoning... I think this is ok as long as this option is not the default... But if we wanted to make this the default as was mentioned on this comment, we probably want to be more lenient and allow to use the stale data...

Totally agreed.

So we want this to be notStaleCachedOrFetchedUnlessThatFailsInWhichCaseStaleCache. I think this is pointing to the fact that really there's a lot of different combinations, and we can't offer them all. So maybe the right answer is to make sure this is documented correctly (could add a note to this policy).

To create that behavior users could do it themselves. I think it's best if we provide the primitives that users can compose around:

let customerInfo: CustomerInfo? do { customerInfo = try await purchases.customerInfo(fetchPolicy: .notStaleCachedOrFetched) } catch { customerInfo = try? await purchases.customerInfo(fetchPolicy: .fromCacheOnly) }

What do you guys think?

Yeah, I think with the current options, users have a lot of flexibility, so as long as it's well documented, customers should be able to do what they need. However, as I mentioned, I wouldn't change the default to notStaleCachedOrFetched since I think that can potentially cause a bunch of issues for users.

We can talk about what the default behavior should be if we think the current behavior (cachedOrFetched) is causing issues

I added a warning to the docstring: #1708

NachoSoto · 2022-05-31T18:17:20Z

All the existing tests are left unchanged, meaning the existing default behavior is still the same.

aboedo

great work on this!

aboedo · 2022-05-31T18:44:16Z

I agree. Let's run it by the support team, they might have thoughts based on customer feedback.

I think we can merge this PR as is (with the current behavior), then ask the support team (and broader team) for feedback regarding the proposed change, then if folks agree make a release that makes this the default.
We'd have to also make a purchases-android equivalent release so that we don't have inconsistent behavior in the hybrids.

aboedo · 2022-05-31T18:44:59Z

aboedo · 2022-05-31T18:47:35Z

the only thing I'd add is to have a case for "returns error if cache was stale and new cache can't be fetched" in case of network errors

Good idea, added for both .cachedOrFetched and .notStaleCachedOrFetched.

@objc

Fixes [CF-19]. This allows specifying `CustomerInfoManager` fetching behavior, both as a public API for users, and also internally. Some future uses of `CustomerInfoManager` might want to ensure that we don't rely on cached data, for example. Thanks to the `APITester` we know that the existing methods still work, this is purely an additive change. This is the core of the change: ```swift /// Specifies the behavior that a caching API should have. @objc(RCCacheFetchPolicy) public enum CacheFetchPolicy: Int { /// Returns values from the cache, or throws an error if not available. case fromCacheOnly /// Always fetch the most up-to-date data. case fetchCurrent /// Default behavior: retruns the cache data if available and not stale, or fetches up-to-date data. case cachedOrFetched /// Default ``CacheFetchPolicy`` behavior. public static let `default`: Self = .cachedOrFetched } ``` The new `CustomerInfoManagerGetCustomerInfoTests` check all the different combinations of fetching behavior. I've taken some of the existing tests and refactored them to be `async`, as well as added a bunch more tests. There is also one big hidden bug fix: `Purchases.getCustomerInfo` now calls `BackendError.asPurchasesError`, so we don't end up returning an internal error type. This was missed because we always return generic `Error` types, so we have no way to know at compile time that we only return `ErrorCode`s.

…on `CacheFetchPolicy.cachedOrFetched`

@NachoSoto

### New Features * `Purchases.customerInfo()`: added overload with a new `CacheFetchPolicy` (#1608) via NachoSoto (@NachoSoto) * `Storefront`: added `sk1CurrentStorefront` for Objective-C (#1614) via NachoSoto (@NachoSoto) ### Bug Fixes * Fix for not being able to read receipts on watchOS (#1625) via Patrick Busch (@patrickbusch) ### Other Changes * Added tests for `PurchasesOrchestrator` invoking `listenForTransactions` only if SK2 is enabled (#1618) via NachoSoto (@NachoSoto) * `PurchasesOrchestrator`: removed `lazy` hack for properties with `@available` (#1596) via NachoSoto (@NachoSoto) * `PurchasesOrchestrator.purchase(sk2Product:promotionalOffer:)`: simplified implementation with new operator (#1602) via NachoSoto (@NachoSoto)

tonidero · 2022-06-13T16:36:34Z

+        completion: ((Result<CustomerInfo, BackendError>) -> Void)?
+    ) {
+        switch fetchPolicy {
+        case .fromCacheOnly:


I'm working on the android side of this, and one thing I'm wondering is, do we want to fetch new data even with this fetch policy (if stale)? I think that might be better so we update as early as possible, but I could also understand devs thinking that the cache will not be updated in this case... Though if that's the case we should make sure we handle all other possible places where the cache can be updated. cc @NachoSoto @aboedo

The docs for CachePolicy might help illustrate the desired behavior here.
For this specific case, if there's nothing in cache, we'd return an error right away no fetching. If there's something in cache, it gets returned even if stale. FWIW I think most apps will not want to use this behavior, and will opt for any of the others

Happy to clarify further if needed!

I agree that this won't be used much by devs so it's not that big of a deal. The docs don't specify whether this would initiate a fetch, and it doesn't either for some other policies, like the cacheOrFetched policy, it says: Default behavior: returns the cached data if available (even if stale), or fetches up-to-date data., it says or but it can be an and if the cache is stale (I'm probably nitpicking here).

For now I will follow iOS behavior on this, but I'm more concerned about this other question: #1608 (comment) @aboedo

That is certainly valid feedback. We should update the docs to make it 100% unambiguous, i.e.: specify for each one:

If cache is available and valid, ...

If cache is available and stale, ...

If cache is not available, ...

Where the second part specifies whether an error or value is returned, whether it'll trigger a fetch, and whether it'll wait for the fetch before returning or it'll trigger something in the delegate instead.

Maybe we could specify that in the Android implementation and copy the text back to this one?

…o docstring See conversation in #1608 (comment)

#1708) See conversation in #1608 (comment)

NachoSoto requested a review from a team May 24, 2022 22:02

NachoSoto force-pushed the customer-info-cache-option branch 4 times, most recently from ace4e4d to 673df33 Compare May 24, 2022 22:42

NachoSoto changed the title ~~[WIP] Introduced CacheFetchPolicy to allow specifying CustomerInfoManager fetching behavior~~ Purchases.customerInfo(): added overload with a new CacheFetchPolicy May 24, 2022

NachoSoto marked this pull request as ready for review May 24, 2022 22:43

NachoSoto force-pushed the customer-info-cache-option branch 2 times, most recently from a588661 to 28dad09 Compare May 24, 2022 22:54

NachoSoto commented May 24, 2022

View reviewed changes

NachoSoto force-pushed the customer-info-cache-option branch 2 times, most recently from 65a091e to ff25c58 Compare May 25, 2022 14:46

aboedo requested changes May 31, 2022

View reviewed changes

Comment thread Sources/Purchasing/CacheFetchPolicy.swift Outdated

NachoSoto force-pushed the customer-info-cache-option branch 3 times, most recently from 0013e41 to d640269 Compare May 31, 2022 18:04

NachoSoto commented May 31, 2022

View reviewed changes

NachoSoto requested a review from aboedo May 31, 2022 18:06

NachoSoto commented May 31, 2022

View reviewed changes

aboedo approved these changes May 31, 2022

View reviewed changes

NachoSoto added 4 commits May 31, 2022 12:56

Using Self.appUserID

f9bd225

Added new CacheFetchPolicy.notStaleCachedOrFetched and updated doc …

12ea8c5

…on `CacheFetchPolicy.cachedOrFetched`

Added 2 more tests

5c10239

NachoSoto force-pushed the customer-info-cache-option branch from d640269 to 12ea8c5 Compare May 31, 2022 20:03

NachoSoto merged commit c531814 into main May 31, 2022

NachoSoto deleted the customer-info-cache-option branch May 31, 2022 20:04

NachoSoto mentioned this pull request Jun 2, 2022

Release/4.5.0 #1643

Merged

aboedo mentioned this pull request Jun 3, 2022

Release: 4.5.1 #1653

Closed

tonidero reviewed Jun 13, 2022

View reviewed changes

NachoSoto mentioned this pull request Jun 16, 2022

CacheFetchPolicy.notStaleCachedOrFetched: added warning to docstring #1708

Merged

NachoSoto added a commit that referenced this pull request Jun 16, 2022

[skip ci] CacheFetchPolicy.notStaleCachedOrFetched: added warning t…

dce9ed5

…o docstring See conversation in #1608 (comment)

NachoSoto added a commit that referenced this pull request Jun 16, 2022

CacheFetchPolicy.notStaleCachedOrFetched: added warning to docstring (

2fdefe6

#1708) See conversation in #1608 (comment)

Uh oh!

Conversation

NachoSoto commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aboedo commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboedo Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NachoSoto commented May 24, 2022 •

edited

Loading

NachoSoto Jun 14, 2022 •

edited

Loading

aboedo Jun 13, 2022 •

edited

Loading