Authv2 / Networking improvements

[federicocappelli]

Task/Issue URL: https://app.asana.com/0/1205842942115003/1209170372758735/f
iOS PR: duckduckgo/iOS#3820
macOS PR: duckduckgo/macos-browser#3746
What kind of version bump will this require?: Major (APIRequestV2 init can now return nil, TestUtils library has been replaced)

CC: @miasma13

Description:

Networking v2 Improvements

• Introduction of RetryPolicy
• Authentication support improved with refresh callback in case of 401 error
• Equatable conformance
• Hashable Conformance
• APIRequestV2Error Equatable conformance
• New Oauth framework inside Networking v2
• Malicious site protection APIClient updated to Networking v2
• A lot of utilities, moks and improvements added to NetworkingTestingUtils

Additional changes include:

• DecodableHelper expanded and renamed CodableHelper
• Date extension with utilities + unit tests
• Rationalisation of the package "Testing Utils" modules, now every library has its XYZTestingUtils module, I removed the generic TestingUtils

Steps to test this PR:

• malicious site protection API works as expected

---

Internal references:

Software Engineering Expectations
Technical Design Template

[mallexxx]

I think it should crash in case we can‘t form a request as it would indicate a critical code flaw. Or at least assert if there are valid cases when a request cannot be created from a url (which I believe there‘s no).

[federicocappelli]

I agree that it isn't realistically possible to create a invalid request via an invalid URL, but... URLComponents.init result is optional so I'm not keen to force unwrap anything. I haven't yet discovered a way to make URLComponents.init fail but I can't demonstrate that doesn't exist.
I've added an assert, but crashing in production is not an option.

[alessandroboron]

@federicocappelli we would catch it during development though? I understand the concern but I think it will make the client simpler. What do we do if a request is nil?

[federicocappelli]

I added an assert to catch this during development.
I don't think force unwrapping is an option because this is a library that will, hopefully, be used in future by other objectives and I can't safeguard the usage.
A practical example is: what if somebody creates a URL dynamically at runtime, maybe adding query params based on user input and builds it incorrectly? Unit tests would probably miss that and would crash in production.

[alessandroboron]

@federicocappelli I was doing the iOS PR and I can see many places in the future where we have to guard against a nil network request and I’m not really a fan of it as it will make the client more complex. You have a fair point about dynamically generated URLs. But what if make the NetworkService responsible of checking if the network request is well formed? What I mean is that we could move this code in the NetworkService:

// Generate URL request
        guard var urlComps = URLComponents(url: url, resolvingAgainstBaseURL: true) else {
            assertionFailure("Malformed URL: \(url)")
            return nil
        }
        if let queryItems {
            // we append both the query items already added to the URL and the new passed as parameters
            let originalQI = urlComps.queryItems ?? []
            urlComps.queryItems = originalQI + queryItems.toURLQueryItems(allowedReservedCharacters: allowedQueryReservedCharacters)
        }
        guard let finalURL = urlComps.url else { return nil }
        var request = URLRequest(url: finalURL, timeoutInterval: timeoutInterval)
        request.allHTTPHeaderFields = headers?.httpHeaders
        request.httpMethod = method.rawValue
        request.httpBody = body
        if let cachePolicy = cachePolicy {
            request.cachePolicy = cachePolicy
        }

and if the request is malformed we just throw an error? E.g. invalidURL or we add another case if we need to.

In this way we get best of both worlds. The client stay simple by not handling nil network requests and we gracefully handle malformed network requests. In the APIService we would have

public func fetch(request: APIRequestV2) async throws -> APIResponseV2 {
   …
   let urlRequest = try makeURLRequest(v2Request: request)
   let (data, response) = try await fetch(for: urlRequest)   
   …
}   

This is also easy to unit test. if the request is malformed it should throw the error we expect, if it is well-formed it should ask URLSession to execute the URLRequest.

@mallexxx what do you also think about this approach?

[federicocappelli]

I'm sorry @alessandroboron but I disagree.

1. With this approach you are mixing responsibilities. It's the init responsibility to build a request, not the fetch(...)
2. Why is it a problem to check for a failed init with a guard? This is the standard Apple approach, see URLComponents! With your approach, URLComponents should build an invalid URL and then fail during a URLSession data session? I think this is an anti-pattern and shouldn't be used just for avoiding a guard.
3. The APIRequest init has valid reasons to fail and we shouldn't hide this just for convenience, the URL in the init is an unsanitised input and can be invalid for many reasons, we shouldn't assume anything and we shouldn't delegate the check to anybody else if not the init.
4. Disliking an optional init is a personal preference and I don't think this should block the PR.

[alessandroboron]

I didn’t mean to block the PR, but discuss approaches to keep the client simple. I don’t think your approach is wrong so I’m comfortable approving the PR.

Replying to your comments here about my reasoning:

1. Init would indeed build an APIRequestV2, just not a URLRequest.

2. "With your approach, URLComponents should build an invalid URL and then fail during a URLSession data session”.
   No as the URLRequest will be initialised when the request is processed by the APIService so if the URLComponent was invalid it would throw an error.

[alessandroboron]

@federicocappelli that was a massive effort so good job! I left some comments, let me know what you think.

[alessandroboron]

Sorry I thought I posted this comment but it didn’t.

I think structs are perfect fits to represent network requests as network requests are a piece of data that should not change once they are created.

My advice is to keep the requests stateless and have the APIService keeping track of the retryCount.

We can do that by changing the below to:

public func fetch(request: APIRequestV2) async throws -> APIResponseV2 {
   try async performFetch(request: request) 
}

private func performFetch(request: APIRequestV2, authRefreshRetryCount: Int = 0, failureRetryCount: Int = 0) async throws -> APIResponseV2 {

   if responseHTTPStatus == .unauthorized,
           request.isAuthenticated == true,
           authRefreshRetryCount == 0,
           let authorizationRefresherCallback {
              // Ask to refresh the token
               let refreshedToken = try await authorizationRefresherCallback(request)
               request.updateAuthorizationHeader(refreshedToken)
               
               return try await performFetch(request: request, authRefreshRetryCount: authRefreshRetryCount + 1, failureRetryCount: failureRetryCount)
           }
           
           // It's a failure and the request must be retried
        if  let retryPolicy = request.retryPolicy,
            responseHTTPStatus.isFailure,
            responseHTTPStatus != .unauthorized, // No retries needed is unuathorised
            failureRetryCount < retryPolicy.maxRetries {
           
            if retryPolicy.delay > 0 {
                try? await Task.sleep(interval: retryPolicy.delay)
            }

            // Try again
            return try await performFetch(request: request, authRefreshRetryCount: authRefreshRetryCount, failureRetryCount: failureRetryCount + 1)
        }
        
        // Continue handling the response...
} 

I haven’t tested the code but it’s to give the idea. Also we can maybe change authRefreshRetryCount to a bool if we re-try only once e.g.

private func performFetch(request: APIRequestV2, hasAlreadyTriedToRefreshAuthToken: Bool = false, failureRetryCount: Int = 0) async throws -> APIResponseV2

Maybe another improvement we could make, but not mandatory, is to avoid making an API call if the token is already/about to expire.

At the moment we execute the request and if it is expired we refresh the token and retry.

We know the expiry date of the token so we could check if the token.expiryDate <= now + and we refresh before making the API call. But yeah this is just an idea.

[federicocappelli]

• suggested changes implemented, now APIRequestV2 is a struct with a single mutating function
• We already check if the token is expired before using but: 1) in some cases, especially with a slow connection, the 401 can happen 2) this is a generic API, we can't guarantee that the token source behaves correctly. Best case scenario this feature is never used.