Introduced type-safe `PurchasesError` and fixed some incorrect returned error types #1879

NachoSoto · 2022-08-31T19:04:51Z

This is the last change from what was started with #1437. That PR introduced a few private error types, but for the errors returned in the public APIs we were still passing around Errors everywhere, which didn't provide any compile-time guarantees.

Goals:

Ensure that public APIs only return ErrorCodes wrapped with the additional userInfo, and not accidentally return other types like SKError, BackendError, ErrorCode directly, etc.
Guarantee type-safety when passing errors around within our internal implementations instead of assuming certain thrown errors are of a particular type.

Changes:

Created PurchasesError for wrapping an ErrorCode with the additional userInfo.
Changed returned errors from Error to NSError. This is a change in the API, but the new type is a more specific Error so it shouldn't lead to any changes in client apps.
Because PurchasesError isn't implicitly convertible to NSError, this ensures that we don't return those values directly, and instead use PurchasesError.asPublicError.
Changed ErrorCodeConvertible to a now more precise PurchasesErrorConvertible.

Thanks to this new type-safety this PR fixes at least 3 bugs where we were returning private errors instead of ErrorCodes.

Testing:

Thanks to #1871 we know that the returned errors are still convertible to ErrorCode so users can switch over them.

NachoSoto · 2022-08-31T19:08:41Z

Sources/Purchasing/Purchases/Purchases.swift

        self.identityManager.logIn(appUserID: appUserID) { result in
            self.operationDispatcher.dispatchOnMainThread {
-                completion(result.value?.info, result.value?.created ?? false, result.error)
+                completion(result.value?.info, result.value?.created ?? false, result.error?.asPurchasesError)


⚠️ This was a bug, we were returning BackendErrors! Caught by the compiler now.

NachoSoto · 2022-08-31T19:44:20Z

Yess I got this all working with type-safe public errors 🤪

NachoSoto · 2022-08-31T20:37:51Z

Sources/Purchasing/Purchases/PurchasesOrchestrator.swift

@@ -391,7 +391,7 @@ final class PurchasesOrchestrator {
                ))

                DispatchQueue.main.async {
-                    completion(nil, nil, error, false)
+                    completion(nil, nil, ErrorUtils.purchasesError(withUntypedError: error).asPublicError, false)


This is needed because of untyped throws in Swift, which doesn't allow us to guarantee type safety here.
However this is pretty well covered in tests.

tonidero

It looks good, like the extra security this provides us 😄 . Just a couple of questions.

Sources/Error Handling/ErrorUtils.swift

tonidero · 2022-09-02T13:12:08Z

Sources/Error Handling/PurchasesError.swift

+import Foundation
+
+/// An error returned by a `RevenueCat` public API.
+public typealias PublicError = NSError


Hmm I wonder if we should rename this... PublicError from the point of view of SDK users doesn't make that much sense... Maybe RCError? PurchasesError is already used... Can't think of other names at the moment 😅

Yeah I didn't love it either. It's just a transparent alias but I agree it could be better. Maybe ExposedError, ReturnedError?

Hmm I guess I don't love any of the current options but I have a slight preference for ExposedError. Also, not sure if we should namespace the errors to RC? As in RCExposedError?

We don't need to namespace in Swift (since it's already namespaces by the module name RevenueCat. And this typealias is transparent in Objective-C, meaning APIs see NSError instead.

NachoSoto · 2022-08-31T21:34:25Z

Sources/Purchasing/ProductsManager.swift

        _ = Task<Void, Never> {
            do {
                let products = try await self.sk2StoreProducts(withIdentifiers: identifiers)
                Logger.debug(Strings.storeKit.store_product_request_finished)
                completion(.success(Set(products)))
            } catch {
                Logger.debug(Strings.storeKit.store_products_request_failed(error: error))
-                completion(.failure(error))
+                completion(.failure(ErrorUtils.storeProblemError(error: error)))


⚠️ This was another bug, we were returning a private error type instead of an ErrorCode.

Sources/Error Handling/ErrorUtils.swift

NachoSoto · 2022-09-02T16:16:48Z

Sources/Error Handling/PurchasesError.swift

+import Foundation
+
+/// An error returned by a `RevenueCat` public API.
+public typealias PublicError = NSError


Yeah I didn't love it either. It's just a transparent alias but I agree it could be better. Maybe ExposedError, ReturnedError?

… returned error types This is the last change from what was started with #1437. That PR introduced a few private error types, but for the errors returned in the public `APIs` we were still passing around `Error`s everywhere, which didn't provide any compile-time guarantees. - Ensure that public APIs *only* return `ErrorCode`s wrapped with the additional `userInfo`, and not accidentally return other types like `SKError`, `BackendError`, `ErrorCode` directly, etc. - Guarantee type-safety when passing errors around within our internal implementations instead of assuming certain thrown errors are of a particular type. - Created `PurchasesError` for wrapping an `ErrorCode` with the additional `userInfo`. - Changed returned errors from `Error` to `NSError`. This is a change in the API, but the new type is a more specific `Error` so it shouldn't lead to any changes in client apps. - Because `PurchasesError` isn't implicitly convertible to `NSError`, this ensures that we don't return those values directly, and instead use `PurchasesError.asPublicError`. - Changed `ErrorCodeConvertible` to a now more precise `PurchasesErrorConvertible`. Thanks to this new type-safety this PR fixes at least 2 bugs where we were returning private errors instead of `ErrorCode`s. Thanks to #1871 we know that the returned errors are still convertible to `ErrorCode` so users can `switch` over them.

tonidero

There is the question of that rename but the PR looks great!

@NachoSoto

**This is an automatic release.** ### Bugfixes * `watchOS`: fixed crash when ran on single-target apps with Xcode 14 and before `watchOS 9.0` (#1895) via NachoSoto (@NachoSoto) * `CustomerInfoManager`/`OfferingsManager`: improved display of underlying errors (#1888) via NachoSoto (@NachoSoto) * `Offering`: improved confusing log for `PackageType.custom` (#1884) via NachoSoto (@NachoSoto) * `PurchasesOrchestrator`: don't log warning if `allowSharingAppStoreAccount` setting was never explicitly set (#1885) via NachoSoto (@NachoSoto) * Introduced type-safe `PurchasesError` and fixed some incorrect returned error types (#1879) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: fixed thread-unsafe implementation (#1878) via NachoSoto (@NachoSoto) ### New Features * Disable SK1's `StoreKitWrapper` if SK2 is enabled and available (#1882) via NachoSoto (@NachoSoto) * `Sendable` support (#1795) via NachoSoto (@NachoSoto) ### Other Changes * Renamed `StoreKitWrapper` to `StoreKit1Wrapper` (#1886) via NachoSoto (@NachoSoto) * Enabled `DEAD_CODE_STRIPPING` (#1887) via NachoSoto (@NachoSoto) * `HTTPClient`: added `X-Client-Bundle-ID` and logged on SDK initialization (#1883) via NachoSoto (@NachoSoto) * add link to SDK reference (#1872) via Andy Boedo (@aboedo) * Added `StoreKit2Setting.shouldOnlyUseStoreKit2` (#1881) via NachoSoto (@NachoSoto) * Introduced `TestLogHandler` to simplify how we test logged messages (#1858) via NachoSoto (@NachoSoto) * `Integration Tests`: added test for purchasing `StoreProduct` instead of `Package` (#1875) via NachoSoto (@NachoSoto) * `ErrorUtils`: added test to verify that returned errors can be converted to `ErrorCode` (#1871) via NachoSoto (@NachoSoto)

This was missed in #1879. Without this, we might have been forwarding some private errors instead of ensuring that we only sent `CodeError`s

…'d as `ErrorCode` This is a key part of how we document error handling in the SDK, so it's important that it's covered in a test. The second test also verifies that this same error contains all the metadata. This is a follow up to #1879.

This wasn't required, likely since the introduction of #1879, which added `matchError` as a much better way to check errors. I noticed this when adding a new `case` with an associated value that wasn't `Equatable`. Tiny change but should make compilation faster, and reduce the binary size.

NachoSoto added the refactor label Aug 31, 2022

NachoSoto commented Aug 31, 2022

View reviewed changes

NachoSoto force-pushed the public-error branch from abc278f to b75891e Compare August 31, 2022 19:41

NachoSoto force-pushed the public-error branch 7 times, most recently from dda0acc to f0a66f8 Compare August 31, 2022 20:31

NachoSoto added the pr:fix A bug fix label Aug 31, 2022

NachoSoto changed the title ~~[WIP] Type-safe errors~~ [WIP] Type-safe errors. Fixed some incorrect returned error types Aug 31, 2022

NachoSoto commented Aug 31, 2022

View reviewed changes

NachoSoto changed the title ~~[WIP] Type-safe errors. Fixed some incorrect returned error types~~ Introduced type-safe PurchasesError errors and fixed some incorrect returned error types Aug 31, 2022

NachoSoto force-pushed the public-error branch 2 times, most recently from fb6d963 to 557bc8a Compare August 31, 2022 21:38

NachoSoto marked this pull request as ready for review August 31, 2022 21:39

NachoSoto requested a review from a team August 31, 2022 21:41

NachoSoto force-pushed the public-error branch 3 times, most recently from 9573724 to e94f7ee Compare August 31, 2022 21:54

NachoSoto changed the title ~~Introduced type-safe PurchasesError errors and fixed some incorrect returned error types~~ Introduced type-safe PurchasesError and fixed some incorrect returned error types Sep 1, 2022

tonidero reviewed Sep 2, 2022

View reviewed changes

NachoSoto mentioned this pull request Sep 4, 2022

Disable SK1's StoreKitWrapper if SK2 is enabled and available #1882

Merged

3 tasks

NachoSoto commented Sep 4, 2022

View reviewed changes

NachoSoto force-pushed the public-error branch from e94f7ee to abc5cd9 Compare September 4, 2022 21:47

tonidero approved these changes Sep 5, 2022

View reviewed changes

NachoSoto merged commit d4ea390 into main Sep 5, 2022

NachoSoto deleted the public-error branch September 5, 2022 20:46

revenuecat-ops mentioned this pull request Sep 7, 2022

[AUTOMATIC] Release/4.12.0 #1890

Merged

NachoSoto added a commit that referenced this pull request Sep 20, 2022

Purchases.beginRefundRequest: ensured errors are PublicError

083fa9b

This was missed in #1879. Without this, we might have been forwarding some private errors instead of ensuring that we only sent `CodeError`s

NachoSoto mentioned this pull request Sep 20, 2022

Purchases.beginRefundRequest: ensured errors are PublicError #1913

Merged

NachoSoto added a commit that referenced this pull request Sep 21, 2022

Purchases.beginRefundRequest: ensured errors are PublicError (#1913)

12cf786

This was missed in #1879. Without this, we might have been forwarding some private errors instead of ensuring that we only sent `CodeError`s

NachoSoto mentioned this pull request Sep 21, 2022

ErrorUtils: added test to verify that PublicErrors can be catch'd as ErrorCode #1924

Merged

NachoSoto mentioned this pull request May 24, 2023

BackendError/NetworkError: removed Equatable conformances #2535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced type-safe `PurchasesError` and fixed some incorrect returned error types #1879

Introduced type-safe `PurchasesError` and fixed some incorrect returned error types #1879

NachoSoto commented Aug 31, 2022 •

edited

Loading

NachoSoto Aug 31, 2022 •

edited

Loading

NachoSoto commented Aug 31, 2022

NachoSoto Aug 31, 2022

tonidero left a comment

tonidero Sep 2, 2022

NachoSoto Sep 2, 2022

tonidero Sep 5, 2022

NachoSoto Sep 5, 2022

NachoSoto Aug 31, 2022

NachoSoto Sep 2, 2022

tonidero left a comment

Introduced type-safe PurchasesError and fixed some incorrect returned error types #1879

Introduced type-safe PurchasesError and fixed some incorrect returned error types #1879

Conversation

NachoSoto commented Aug 31, 2022 • edited Loading

Goals:

Changes:

Testing:

NachoSoto Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

NachoSoto commented Aug 31, 2022

NachoSoto Aug 31, 2022

Choose a reason for hiding this comment

tonidero left a comment

Choose a reason for hiding this comment

tonidero Sep 2, 2022

Choose a reason for hiding this comment

NachoSoto Sep 2, 2022

Choose a reason for hiding this comment

tonidero Sep 5, 2022

Choose a reason for hiding this comment

NachoSoto Sep 5, 2022

Choose a reason for hiding this comment

NachoSoto Aug 31, 2022

Choose a reason for hiding this comment

NachoSoto Sep 2, 2022

Choose a reason for hiding this comment

tonidero left a comment

Choose a reason for hiding this comment

Introduced type-safe `PurchasesError` and fixed some incorrect returned error types #1879

Introduced type-safe `PurchasesError` and fixed some incorrect returned error types #1879

NachoSoto commented Aug 31, 2022 •

edited

Loading

NachoSoto Aug 31, 2022 •

edited

Loading