Add Configuration Loader #1353

richherrera · 2024-07-02T23:24:46Z

Summary of changes

Refactor the existing logic by creating a ConfigurationLoader object, moving all the logic from the fetchOrReturnRemoteConfiguration method to this new object. The public method (fetchOrReturnRemoteConfiguration) remain unchanged to avoid requiring modifications from the clients. This will help us maintain similarities with Android and isolate the logic, allowing us to better implement a mechanism to avoid multiple calls to the v1/configuration endpoint.

Checklist

Added a changelog entry

Authors

@richherrera

Sources/BraintreeCore/BTAPIClient.swift

scannillo · 2024-07-02T23:54:10Z

Sources/BraintreeCore/ConfigurationLoader.swift

+    init(http: BTHTTP) {
+        self.http = http
+    }
+
+    deinit {
+        http.session.finishTasksAndInvalidate()
+    }
+
+    func getConfig(_ authorization: ClientAuthorization, completion: @escaping (BTConfiguration?, Error?) -> Void) {


Just a few more MARKs we usually use, but open to if folks don't find these useful

Suggested change

init(http: BTHTTP) {

self.http = http

}

deinit {

http.session.finishTasksAndInvalidate()

}

func getConfig(_ authorization: ClientAuthorization, completion: @escaping (BTConfiguration?, Error?) -> Void) {

// MARK: - Intitializer

init(http: BTHTTP) {

self.http = http

}

deinit {

http.session.finishTasksAndInvalidate()

}

// MARK: - Internal Methods

func getConfig(_ authorization: ClientAuthorization, completion: @escaping (BTConfiguration?, Error?) -> Void) {

I like them, added: 418b39d

scannillo · 2024-07-02T23:54:42Z

Sources/BraintreeCore/ConfigurationLoader.swift

+    }
+
+    func getConfig(_ authorization: ClientAuthorization, completion: @escaping (BTConfiguration?, Error?) -> Void) {
+        // Fetches or returns the configuration and caches the response in the GET BTHTTP call if successful


Nit - this should probably be a docstring instead of a comment

Thats a good idea, updated: 418b39d

scannillo · 2024-07-02T23:56:12Z

CHANGELOG.md

+## unreleased
+* BraintreeCore
+  * Add `ConfigurationLoader` class
+


IMO this doesn't need a CHANGELOG entry since it's not merchant facing and isn't really a functional change that we'll want a reference to in the future. What do you think?

Agree, updated: 646a1ef

scannillo · 2024-07-02T23:56:42Z

Sources/BraintreeCore/BTAPIClient.swift

@@ -22,6 +22,8 @@ import Foundation
    var graphQLHTTP: BTGraphQLHTTP?
    var payPalHTTP: BTHTTP?



Suggested change

scannillo

👏 Some minor comments, but this looks great!

This reverts commit fcc1931.

KunJeongPark · 2024-07-03T19:20:04Z

Sources/BraintreeCore/BTAPIClient.swift

            } catch {
                return nil
            }
        }

+        let btHttp = BTHTTP(authorization: self.authorization)
+        http = btHttp
+        configurationLoader = ConfigurationLoader(http: btHttp)


I am thinking about the fact that ConfigurationCache is a singleton and thought of ConfigurationLoader as being one as well. In a weird scenario that there are multiple instances of BTAPIClient. wdyt?

This is something we want to do on Android too. Making ConfigurationLoader a singleton makes the most sense for our architecture as well.

Something that happened in MXO iOS was that almost everything was a singleton. At least in iOS, we need to define the architecture. I think most of the code was a migration from Objective-C to Swift (IMO there are several areas of opportunity that we could refactor.). For now, I think we can proceed this way and discuss whether it would be necessary to convert it to a singleton in the future. Like any design pattern, it has its pros and cons. I'm not entirely a fan because it violates the SRP, and we have to think of a creative way to mock

@richherrera I'm 100% onboard with that concern. The one thing I noticed with MXO though is that the interface was static e.g. ConfigurationLoader.load() . If we go for ConfigurationLoader.shared.load() we'll preserve the flexibility needed for mocking if we inject the singleton reference in the constructor (we do this quite a bit in Android with getInstance() accessors).

Yes, passing it in a protocol with default value in initializer makes testing possible.

So, do you think having two singletons (Cache and Loader) is necessary?

Necessary, not sure, but I think there might be some weirdness in possibly having multiple instances of BTAPIClient and multiple instances of Loader, single Cache.

@sshropshire yeah DIP rules, i like that, something I’d like to know is how we could use the same instance of BTHTTP in both BTAPIClient and ConfigurationLoader. Are there two different instances in Android (for BraintreeClient and ConfigurationLoader)?

I think for as long as there can be multiple instances of BTAPIClient, we should address those issues.

I guess with singleton ConfigurationLoader, you'd need to keep track of BTHTTP instance creation in ConfigurationLoader by authorization or something and check with the getConfiguration function.

I think maybe considering multiple instances of BTAPIClient and configurationLoader, we might need to synchronize ConfigurationCache functions as well some time.

I think we have discussed, for future, making sure that there is only one instance of BTAPIClient.

Great work on this Rich.

Are there two different instances in Android (for BraintreeClient and ConfigurationLoader)?

@richherrera On Android we have a single BraintreeHttpClient instance that's shared between BraintreeClient and ConfigurationLoader. That would be equivalent to BTAPIClient and ConfigurationLoader sharing a BTHTTP instance on iOS.

KunJeongPark · 2024-07-03T19:26:38Z

UnitTests/BraintreeCoreTests/ConfigurationLoader_Tests.swift

+            XCTAssertEqual(error as NSError, mockError)
+        }
+    }
+}


Great job on these tests! Very thorough!

richherrera added 8 commits June 27, 2024 10:58

Add ConfigurationLoader class

8200b88

Add ConfigurationLoader logic

13c929c

Merge branch 'main' into configuration-loader-object

57a8c24

Merge branch 'main' into configuration-loader-object

a2b8165

Move Configuration tests from APIClient to ConfigurationLoaderTests

480c5ea

Add MockConfigurationLoader

070c611

Update BTAPIClient to use ConfigurationLoader & Update UTs

ff1524c

Update CHANGELOG

fcc1931

richherrera requested a review from a team as a code owner July 2, 2024 23:24

Update UTs names

8a45508

scannillo reviewed Jul 2, 2024

View reviewed changes

Sources/BraintreeCore/BTAPIClient.swift Show resolved Hide resolved

scannillo reviewed Jul 2, 2024

View reviewed changes

scannillo approved these changes Jul 2, 2024

View reviewed changes

richherrera added 3 commits July 3, 2024 10:14

Add async method on mocked APIClient

33f914a

PR feedback: Add marks, add docstring

418b39d

Revert "Update CHANGELOG"

646a1ef

This reverts commit fcc1931.

KunJeongPark reviewed Jul 3, 2024

View reviewed changes

KunJeongPark approved these changes Jul 5, 2024

View reviewed changes

richherrera merged commit d93bd06 into main Jul 8, 2024
6 of 7 checks passed

richherrera deleted the configuration-loader-object branch July 8, 2024 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Configuration Loader #1353

Add Configuration Loader #1353

richherrera commented Jul 2, 2024

scannillo Jul 2, 2024

richherrera Jul 3, 2024

scannillo Jul 2, 2024

richherrera Jul 3, 2024

scannillo Jul 2, 2024

richherrera Jul 3, 2024

scannillo Jul 2, 2024

scannillo left a comment

KunJeongPark Jul 3, 2024

sshropshire Jul 3, 2024

richherrera Jul 3, 2024 •

edited

Loading

sshropshire Jul 3, 2024

KunJeongPark Jul 3, 2024 •

edited

Loading

richherrera Jul 3, 2024

KunJeongPark Jul 3, 2024 •

edited

Loading

richherrera Jul 3, 2024 •

edited

Loading

KunJeongPark Jul 5, 2024 •

edited

Loading

sshropshire Jul 5, 2024

KunJeongPark Jul 3, 2024

		@@ -22,6 +22,8 @@ import Foundation
		var graphQLHTTP: BTGraphQLHTTP?
		var payPalHTTP: BTHTTP?

Add Configuration Loader #1353

Add Configuration Loader #1353

Conversation

richherrera commented Jul 2, 2024

Summary of changes

Checklist

Authors

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

scannillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richherrera Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunJeongPark Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KunJeongPark Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

richherrera Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

KunJeongPark Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richherrera Jul 3, 2024 •

edited

Loading

KunJeongPark Jul 3, 2024 •

edited

Loading

KunJeongPark Jul 3, 2024 •

edited

Loading

richherrera Jul 3, 2024 •

edited

Loading

KunJeongPark Jul 5, 2024 •

edited

Loading