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

[PART 2] Networking Refactor - Unify REST & GraphQL networking implementation #182

Merged
merged 33 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
3e5785a
WIP - use new RestRequest for analytics
scannillo Aug 9, 2023
dcae37b
Refactor ConfirmPaymentSourceRequest to use custom encoder & ConfirmP…
scannillo Aug 9, 2023
5cd493a
Move new API classes & RESTRequest into own files
scannillo Aug 9, 2023
93f3ba4
Remove AnalyticsEventRequest APIRequest protocol conformance
scannillo Aug 9, 2023
a51ab8b
Add TODO for adding graphQL func
scannillo Aug 9, 2023
e5285f7
Cleanup methods to remove & add TODOs for easier review
scannillo Aug 9, 2023
8e07e5d
Fix linter warnings
scannillo Aug 9, 2023
c83eb48
Remove params from ConfirmPaymentSourceRequest previously used in Vau…
scannillo Aug 9, 2023
55f71c4
Cleanup - make ConfirmPaymentSource CodingKeys private
scannillo Aug 9, 2023
1243524
A
scannillo Aug 9, 2023
63cc3f6
Move HTTPRequest into own swift file
scannillo Aug 9, 2023
0010733
Remove APIRequest conformance from ClientIDRequest in Demo (#178)
scannillo Aug 10, 2023
0493841
WIP - begin adding GraphQL template struct & method to APIClient
scannillo Aug 10, 2023
b5289e3
Move HTTPRequest into own swift file
scannillo Aug 9, 2023
bc2213b
PR Feedback - use encodeIfPresent for optional properties, versus sen…
scannillo Aug 10, 2023
4a43375
Remove Eligibility feature (#180)
KunJeongPark Aug 10, 2023
236ee46
Merge branch 'main' into construct-url-request-in-http
scannillo Aug 10, 2023
c435d0a
PR Feedback - move optional param to end of HTTPRequest property list…
scannillo Aug 10, 2023
94acd26
Merge branch 'construct-url-request-in-http' into refactor-graphql
scannillo Aug 10, 2023
508b845
WIP
scannillo Aug 11, 2023
3a32628
Cleanup - remove vault specific code & cleanup new graphQL implementa…
scannillo Aug 11, 2023
2023a49
Remove unneeded GraphQL files & cleanup comments in HTTPResponseParser
scannillo Aug 11, 2023
bc00f95
Reset temporary testing setup to CheckoutOrdersAPI.swift
scannillo Aug 11, 2023
18de290
Add docstrings to new files
scannillo Aug 11, 2023
2365fc5
Leave TODO for renaming APIClient --> Networking<Something>
scannillo Aug 11, 2023
2d1bcc0
Fixup - add more detail to APIClient rename todo
scannillo Aug 11, 2023
4abe12e
Merge branch 'networking-refactor' into refactor-graphql
scannillo Aug 16, 2023
0695f72
Fixup - reorder & rename constructURL methods in APIClient
scannillo Aug 16, 2023
2d5a36a
PR Feedback - prefer guard over if-let
scannillo Aug 16, 2023
4ef956b
PR Feedback - use custom coding keys for GraphQLErrorResponse
scannillo Aug 16, 2023
d3fe188
PR Feedback - rename constructRestURL -> constructRESTURL method
scannillo Aug 17, 2023
ff3ed9b
PR Feedback - use single line for HTTPRequest() instantiation
scannillo Aug 17, 2023
7afefa2
PR Feedback - consolidate variables in APIClient.fetch(restRequest) w…
scannillo Aug 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions PayPal.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
807BF58F2A2A5D19002F32B3 /* HTTPResponseParser.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807BF58E2A2A5D19002F32B3 /* HTTPResponseParser.swift */; };
807BF5912A2A5D48002F32B3 /* HTTPResponseParser_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807BF5902A2A5D48002F32B3 /* HTTPResponseParser_Tests.swift */; };
807C5E6929102D9800ECECD8 /* AnalyticsEventData.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807C5E6829102D9800ECECD8 /* AnalyticsEventData.swift */; };
807D56AC2A869044009E591D /* GraphQLHTTPPostBody.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807D56AB2A869044009E591D /* GraphQLHTTPPostBody.swift */; };
807D56AE2A869064009E591D /* GraphQLRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807D56AD2A869064009E591D /* GraphQLRequest.swift */; };
807D56B02A869F97009E591D /* GraphQLErrorResponse.swift in Sources */ = {isa = PBXBuildFile; fileRef = 807D56AF2A869F97009E591D /* GraphQLErrorResponse.swift */; };
808EEA81291321FE001B6765 /* AnalyticsEventRequest_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 808EEA80291321FE001B6765 /* AnalyticsEventRequest_Tests.swift */; };
80D0C1382731CC9B00548A3D /* PayPalNativeCheckoutClient.swift in Sources */ = {isa = PBXBuildFile; fileRef = BE7116152723227200165069 /* PayPalNativeCheckoutClient.swift */; };
80DB2F762980795D00CFB86A /* CorePaymentsError.swift in Sources */ = {isa = PBXBuildFile; fileRef = 80DB2F752980795D00CFB86A /* CorePaymentsError.swift */; };
Expand Down Expand Up @@ -134,7 +137,7 @@
CBC16DE029EE2F8200307117 /* PayPalNativePaysheetAction_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBC16DDF29EE2F8200307117 /* PayPalNativePaysheetAction_Tests.swift */; };
CBC16DF629EECCB900307117 /* NativeCheckoutProvider_Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBC16DF529EECCB900307117 /* NativeCheckoutProvider_Tests.swift */; };
CBD6004728D0C24A00C3EFF6 /* MockPayPalDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = CBD6004628D0C24900C3EFF6 /* MockPayPalDelegate.swift */; };
E6022E802857C6BE008B0E27 /* GraphQLQueryResponse.swift in Sources */ = {isa = PBXBuildFile; fileRef = E64623222836A69E008AC8E1 /* GraphQLQueryResponse.swift */; };
E6022E802857C6BE008B0E27 /* GraphQLHTTPResponse.swift in Sources */ = {isa = PBXBuildFile; fileRef = E64623222836A69E008AC8E1 /* GraphQLHTTPResponse.swift */; };
E6022E822857C6BE008B0E27 /* GraphQLError.swift in Sources */ = {isa = PBXBuildFile; fileRef = E64623262836A81E008AC8E1 /* GraphQLError.swift */; };
E6022E832857C6BE008B0E27 /* GraphQLQuery.swift in Sources */ = {isa = PBXBuildFile; fileRef = E646231B28369B9B008AC8E1 /* GraphQLQuery.swift */; };
E6022E842857C6BE008B0E27 /* GraphQLClient.swift in Sources */ = {isa = PBXBuildFile; fileRef = E64623372836AFC1008AC8E1 /* GraphQLClient.swift */; };
Expand Down Expand Up @@ -219,6 +222,9 @@
807BF58E2A2A5D19002F32B3 /* HTTPResponseParser.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HTTPResponseParser.swift; sourceTree = "<group>"; };
807BF5902A2A5D48002F32B3 /* HTTPResponseParser_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HTTPResponseParser_Tests.swift; sourceTree = "<group>"; };
807C5E6829102D9800ECECD8 /* AnalyticsEventData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnalyticsEventData.swift; sourceTree = "<group>"; };
807D56AB2A869044009E591D /* GraphQLHTTPPostBody.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLHTTPPostBody.swift; sourceTree = "<group>"; };
807D56AD2A869064009E591D /* GraphQLRequest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLRequest.swift; sourceTree = "<group>"; };
807D56AF2A869F97009E591D /* GraphQLErrorResponse.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLErrorResponse.swift; sourceTree = "<group>"; };
808EEA80291321FE001B6765 /* AnalyticsEventRequest_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnalyticsEventRequest_Tests.swift; sourceTree = "<group>"; };
80B9F85126B8750000D67843 /* CorePayments.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = CorePayments.framework; sourceTree = BUILT_PRODUCTS_DIR; };
80DB2F752980795D00CFB86A /* CorePaymentsError.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CorePaymentsError.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -314,8 +320,8 @@
CBC16DDF29EE2F8200307117 /* PayPalNativePaysheetAction_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PayPalNativePaysheetAction_Tests.swift; sourceTree = "<group>"; };
CBC16DF529EECCB900307117 /* NativeCheckoutProvider_Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NativeCheckoutProvider_Tests.swift; sourceTree = "<group>"; };
CBD6004628D0C24900C3EFF6 /* MockPayPalDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockPayPalDelegate.swift; sourceTree = "<group>"; };
E64623222836A69E008AC8E1 /* GraphQLHTTPResponse.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLHTTPResponse.swift; sourceTree = "<group>"; };
E646231B28369B9B008AC8E1 /* GraphQLQuery.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLQuery.swift; sourceTree = "<group>"; };
E64623222836A69E008AC8E1 /* GraphQLQueryResponse.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLQueryResponse.swift; sourceTree = "<group>"; };
E64623262836A81E008AC8E1 /* GraphQLError.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLError.swift; sourceTree = "<group>"; };
E64623372836AFC1008AC8E1 /* GraphQLClient.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphQLClient.swift; sourceTree = "<group>"; };
E64763702899B60C00074113 /* MockAPIClient.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockAPIClient.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -712,10 +718,10 @@
E646231928369B71008AC8E1 /* GraphQL */ = {
isa = PBXGroup;
children = (
E64623372836AFC1008AC8E1 /* GraphQLClient.swift */,
E64623262836A81E008AC8E1 /* GraphQLError.swift */,
E646231B28369B9B008AC8E1 /* GraphQLQuery.swift */,
E64623222836A69E008AC8E1 /* GraphQLQueryResponse.swift */,
E64623222836A69E008AC8E1 /* GraphQLHTTPResponse.swift */,
807D56AB2A869044009E591D /* GraphQLHTTPPostBody.swift */,
807D56AD2A869064009E591D /* GraphQLRequest.swift */,
807D56AF2A869F97009E591D /* GraphQLErrorResponse.swift */,
);
path = GraphQL;
sourceTree = "<group>";
Expand Down Expand Up @@ -1251,15 +1257,12 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
807C5E67291027D400ECECD8 /* AnalyticsEventRequest.swift in Sources */,
E6022E802857C6BE008B0E27 /* GraphQLQueryResponse.swift in Sources */,
E6022E802857C6BE008B0E27 /* GraphQLHTTPResponse.swift in Sources */,
80E643832A1EBBD2008FD705 /* HTTPResponse.swift in Sources */,
807C5E6929102D9800ECECD8 /* AnalyticsEventData.swift in Sources */,
E6022E822857C6BE008B0E27 /* GraphQLError.swift in Sources */,
E6022E832857C6BE008B0E27 /* GraphQLQuery.swift in Sources */,
80E237DF2A84434B00FF18CA /* HTTPRequest.swift in Sources */,
E6022E842857C6BE008B0E27 /* GraphQLClient.swift in Sources */,
8021B69029144E6D000FBC54 /* PayPalCoreConstants.swift in Sources */,
807D56B02A869F97009E591D /* GraphQLErrorResponse.swift in Sources */,
80DB2F762980795D00CFB86A /* CorePaymentsError.swift in Sources */,
06CE009926F3D1660000CC46 /* CoreConfig.swift in Sources */,
BEA100EC26EFA7790036A6A5 /* HTTPMethod.swift in Sources */,
Expand All @@ -1273,11 +1276,13 @@
BC04837427B2FC7300FA7B46 /* URLSession+URLSessionProtocol.swift in Sources */,
80E2FDC32A8354AD0045593D /* RESTRequest.swift in Sources */,
3DC42BA927187E8300B71645 /* ErrorResponse.swift in Sources */,
807D56AC2A869044009E591D /* GraphQLHTTPPostBody.swift in Sources */,
804E62822937EBCE004B9FEF /* HTTP.swift in Sources */,
BC04836F27B2FB3600FA7B46 /* URLSessionProtocol.swift in Sources */,
065A4DBC26FCD8090007014A /* CoreSDKError.swift in Sources */,
BEA100E726EF9EDA0036A6A5 /* APIClient.swift in Sources */,
807BF58F2A2A5D19002F32B3 /* HTTPResponseParser.swift in Sources */,
807D56AE2A869064009E591D /* GraphQLRequest.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down
35 changes: 31 additions & 4 deletions Sources/CorePayments/Networking/APIClient.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation

// TODO: - Rename to `NetworkingClient`. Now that we have `<PPaaS>API.swift` classes, ths responsibility of this class really is to coordinate networking. It transforms REST & GraphQL into HTTP requests.
/// :nodoc: This method is exposed for internal PayPal use only. Do not use. It is not covered by Semantic Versioning and may change or be removed at any time.
///
/// `APIClient` is the entry point for each payment method feature to perform API requests. It also offers convenience methods for API requests used across multiple payment methods / modules.
Expand Down Expand Up @@ -29,7 +30,7 @@ public class APIClient {

/// :nodoc:
public func fetch(request: RESTRequest) async throws -> HTTPResponse {
let url = try constructURL(path: request.path, queryParameters: request.queryParameters ?? [:])
let url = try constructRestURL(path: request.path, queryParameters: request.queryParameters ?? [:])

let base64EncodedCredentials = Data(coreConfig.clientID.appending(":").utf8).base64EncodedString()

Expand All @@ -51,12 +52,26 @@ public class APIClient {
return try await http.performRequest(httpRequest)
}

// TODO: - Add GraphQL equivalent request type & function
// public func fetch(request: GraphQLRequest) async throws -> HTTPResponse { }
/// :nodoc:
public func fetch(request: GraphQLRequest) async throws -> HTTPResponse {
let url = try constructGraphQLURL(queryName: request.queryNameForURL)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we need this extra space but up to you:

Suggested change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it intentionally to group postBody and postData together, but down to remove this newline if it's harder to read

let postBody = GraphQLHTTPPostBody(query: request.query, variables: request.variables)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏼

let postData = try JSONEncoder().encode(postBody)

let httpRequest = HTTPRequest(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we single line this is it above our linter line length? May be nice to just group all of the properties together but not sure if this is more readable this way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not violate our line length, but I thought it was a little cleaner. Happy to go either way though!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is fine, was more just curious. If it makes more sense to have things grouped together a particular way that's fine too.

Copy link
Collaborator Author

@scannillo scannillo Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by "group all of these properties together"? You mean single line it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I single-lined both the HTTPRequest() definitions (see commit 7417e15)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean by "group all of these properties together"? You mean single line it?

Ah, sorry was referencing this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That link takes me back to:

If we single line this is it above our linter line length? May be nice to just group all of the properties together but not sure if this is more readable this way.

Do you mean to remove the newlines between these variables?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, idk why GH is linking to that. This comment above is the one I was trying to reference:

I did it intentionally to group postBody and postData together, but down to remove this newline if it's harder to read

Not a blocker though, whatever you feel is best

headers: [.contentType: "application/json"],
method: .post,
url: url,
body: postData
)

return try await http.performRequest(httpRequest)
}

// MARK: - Private Methods

private func constructURL(path: String, queryParameters: [String: String]) throws -> URL {
private func constructRestURL(path: String, queryParameters: [String: String]) throws -> URL {
jaxdesmarais marked this conversation as resolved.
Show resolved Hide resolved
let urlString = coreConfig.environment.baseURL.appendingPathComponent(path)
var urlComponents = URLComponents(url: urlString, resolvingAgainstBaseURL: false)

Expand All @@ -70,4 +85,16 @@ public class APIClient {

return url
}

private func constructGraphQLURL(queryName: String? = nil) throws -> URL {
guard let queryName else {
return coreConfig.environment.graphQLURL
}

if let url = URL(string: coreConfig.environment.graphQLURL.absoluteString + "?" + queryName) {
scannillo marked this conversation as resolved.
Show resolved Hide resolved
return url
} else {
throw CorePaymentsError.urlEncodingFailed
}
}
}
58 changes: 0 additions & 58 deletions Sources/CorePayments/Networking/GraphQL/GraphQLClient.swift

This file was deleted.

10 changes: 0 additions & 10 deletions Sources/CorePayments/Networking/GraphQL/GraphQLError.swift

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import Foundation

/// Used to parse error message details out of GraphQL HTTP response body
struct GraphQLErrorResponse: Decodable {

let error: String
let correlationId: String?
scannillo marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import Foundation

/// The GraphQL query and variable details encoded to be sent in the POST body of a HTTP request
struct GraphQLHTTPPostBody: Encodable {

let query: String
let variables: Data
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/// Used to decode the HTTP reponse body of GraphQL requests
struct GraphQLHTTPResponse<T: Codable>: Codable {

let data: T?
}
7 changes: 0 additions & 7 deletions Sources/CorePayments/Networking/GraphQL/GraphQLQuery.swift

This file was deleted.

This file was deleted.

12 changes: 12 additions & 0 deletions Sources/CorePayments/Networking/GraphQL/GraphQLRequest.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Foundation

/// :nodoc: Values needed to initiate a GraphQL network request
public struct GraphQLRequest {

let query: String
let variables: Data

/// This is non-standard in the GraphQL language, but sometimes required by PayPal's GraphQL API.
/// Some requests are sent to `https://www.api.paypal.com/graphql?<queryNameForURL>`
let queryNameForURL: String?
}
2 changes: 1 addition & 1 deletion Sources/CorePayments/Networking/HTTPRequest.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Foundation

/// :nodoc:
/// :nodoc: Values needed to initiate a HTTP network request
public struct HTTPRequest {

let headers: [HTTPHeader: String]
Expand Down
1 change: 1 addition & 0 deletions Sources/CorePayments/Networking/HTTPResponseParser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public class HTTPResponseParser {
decoder.keyDecodingStrategy = .convertFromSnakeCase
}

// TODO: - Update this func (or file) to handle both GraphQL and REST error parsing
public func parse<T: Decodable>(_ httpResponse: HTTPResponse, as type: T.Type) throws -> T {
guard let data = httpResponse.body else {
throw APIClientError.noResponseDataError
Expand Down