Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error Refactor Spike #300

Closed
wants to merge 8 commits into from
Closed

Error Refactor Spike #300

wants to merge 8 commits into from

Conversation

KunJeongPark
Copy link
Collaborator

@KunJeongPark KunJeongPark commented Nov 18, 2024

Reason for changes

  • This PR is a spike, point of discussion for refactor of our Error types.

Problem Statement

Our SDK currently returns a single error type, CoreSDKError, which records:
• Domain: A String indicating the origin of the error (e.g., CardClientErrorDomain, PayPalClientErrorDomain).
• Code: An Int value classifying specific errors.
• Error Description: A string for the error.

This design is similar to NSError, which also provides domain, code, and localizedDescription.

Each module defines its own enum (e.g., CardErrorNetworkingClientError) with:

  1. Enum cases representing specific error types.
  2. Static constants that create CoreSDKError under the hood.
    CardClientErrors_diagram
    PayPalClientErrors_diagram

For example:
CardClient defines its own errors, including those from 3DS verification.
• Errors propagate up from networking layers as CoreSDKError with names reflecting their module and level of origin, but merchants must use domain and code checks to handle specific cases.

Challenges with the Current Design

  1. Error Case Identification (v.1.5.0, last major release and in main) :
    Merchants must rely on the domain and code properties to identify specific errors, which can lead to verbose and error-prone code:
if let error {
    if error.domain == "CardClientErrorDomain" && error.code == 3 {
        // Handle cancellation
    }
}

  1. Awkward Helper Functions (current beta release branch):
    To simplify cancellation handling in the beta release, I introduced helper methods like:
if PayPalError.isCheckoutCanceled(error) {
    // Handle cancellation
} else {
    // Handle other errors
}

While functional, this approach is not intuitive or consistent with Swift’s idiomatic error handling.

  1. Limited Extensibility:
    Introducing new modules or errors requires updating CoreSDKError and potentially modifying helper methods, increasing maintenance complexity.

  2. Merchant Experience:
    The current design does not align well with how merchants typically handle errors. Instead of looking at centralized domains and codes, merchants prefer to interact with module-specific error types directly.

Proposed Alternatives

  1. Module-Specific Errors (e.g., Braintree’s Approach)
    • Each module defines its own error type (e.g., CardErrorPayPalError), conforming to ErrorLocalizedError, and Equatable.
    • Completion handlers return a general Error type, allowing merchants to switch on module-specific errors:
if let error {
    switch error {
    case BTPayPalError.canceled:
        // Handle PayPal cancellation
    case BTAPIClientError.notAuthorized:
        // Handle API client errors
    default:
        // Handle all other errors
    }
}

Advantages:
• Clear and modular error handling.
• Merchants can switch directly on error types without relying on domains and codes.

Disadvantages:
• Merchants must cast errors to module-specific types when needed.

  1. Status Enum with Results and Errors
    • Use Status enum with possibly CoreSDKError:
enum Status<ResultType> {
    case succeeded(ResultType)
    case canceled // with or without Error returned
    case failed(Error)
}

• Completion handlers return Status, allowing merchants to handle success, cancellation, and failure in a unified manner:

switch status {
case .succeeded(let result):
    print("Success: \(result)")
case .canceled:
    print("Operation canceled.")
case .failed(let error):
    print("Error: \(error.localizedDescription)")
}

Advantages:
• Simplifies error handling by combining results and statuses.

Disadvantages:
• It may not be a pattern merchants are used to and some flows such as card payment without 3DS don't have cancel path.
• Merchants still need to cast Error for specific cases.

  1. NSError-Style General Errors (which is what we currently have)
    • Adopt a design similar to NSError, where all errors are generic with domain, code, and localizedDescription.

Advantages:
• Familiar to many developers.
• Reduces complexity by avoiding module-specific error types.

Disadvantages:
• Does not leverage Swift’s strong typing.
• Error handling becomes verbose and less intuitive.

Recommendation

I recommend adopting module-specific error types (Alternative 1), similar to Braintree’s approach:
• Each module manages its own errors (CardError, PayPalError, NetworkingErrors that conform to Equatable protocol).
• Completion handlers return a generic Error type for compatibility.
• Merchants can handle errors using switch statements or type casting.

I think keeping errors as they are is also a viable option as well.
It is a pattern that developers are used to and I understand reason for this implementation.

Cancel separate handling may be not needed by majority of merchants and helper function
should be adequate for merchants if they want to do so. They can also write their own helper
functions to discern the error codes from our internal module specific enum types.

My final thoughts are to release our current v2_beta branch as beta1 and gage merchant feedback for changes.
There are already significant changes from delegation pattern to completion handler and async/await pattern in beta1.

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

* Remove MXO

* remove Package.resolved file containing incompatible pinstorage version

* CHANGELOG entry

* Jax feedback and docStrings correction

* Remove PatchOrder for MXO, unused files in demo app

* Remove more classes used exclusively for MXO
* PayPal remove delgation pattern for completion handler function for checkout

* lint error for paypal start

* completion and analytics into notify functions

* paypal vault completion, unit tests

* remove PayPal delegates, references

* Card vault to completion, unit tests

* Card approve to completion, unit tests

* docstrings for approve completion param

* wrap paypal functions in async await

* async await wrapper for CardClient functions

* remove CardDelegate and references

* fix error in notifyCheckoutFailure

* Make error names and messages payPal caps consistent

* changelog entires

* Steven PR feedback: typo in Chagelog
* Simplify cancel errors

* CHANGELOG for the cancel errors

* Steven PR feedback: change back CardClientError.canceled to .threeDSecureCanceled

* CHANGELOG update
* http performRequest returns NetworkingClientErrors

* CardClient helper function for threeDSCancel, demo app cancel, demo app minor fixes

* PayPalClient cancel helper functions and demo app changes

* Steven PR feedback: move static helper functions to error enums

* Rename CardClientError -> CardError, PayPalWebCheckoutError -> PayPalError

* Steven PR feedback: return CoreSDKError in merchant completion handler

* CHANGELOG and analytics typo and fix wrong code in graphql error
* v2 migration guide

* just cocoapods or SPM

* fix typos

* minor spacing changes

* Update with simplified cancel errors

* Steven PR feedback - diff to render green/red

* include removal of delegate methods in delete block

* update with cardClient threeDSecureCanceled error

* change to threeDSecureCanceled in migration steps

* add comment highlighting cancellation errors

* typo fix

* clarify separating cancel cases in errors

* revert cancel handling instructions

* add changes for cancellation helper methods

* fix typo in PayPalError.isCheckoutCanceled

* Steven PR feedback
* Changelog: move additive changes from breaking section

* remove duplicate line for PayPal cancel errors
@KunJeongPark KunJeongPark requested a review from a team as a code owner November 18, 2024 14:53
Base automatically changed from v2_beta to main November 19, 2024 20:44
@KunJeongPark
Copy link
Collaborator Author

Closing this, can be used as reference.
Note: we have merged in PR that makes CoreSDKError equatable
#303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant