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

Request Adapter State #3504

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Request Adapter State #3504

merged 4 commits into from
Oct 19, 2021

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Oct 10, 2021

This PR adds a new RequestAdapterState struct, a new RequestAdapter protocol API, and a default protocol extension for the new API for backwards compatibility.

Goals ⚽

To allow client libraries to match the URLRequest in a RequestAdapter with the Request object they created.

I'm currently trying to match a RequestOptions type in my client library to the URLRequest in RequestAdapter. I have the Request as well, but I don't have a way to match the URLRequest to the parent Request.

struct RequestOptions {
    let authenticate: Bool
}

class MyInterceptor: RequestInterceptor {
    private var requests: [UUID: RequestOptions] = [:]

    init() {}

    func adapt(_ urlRequest: URLRequest, for session: Session, completion: @escaping (Result<URLRequest, Error>) -> Void) {
        let options = requests[requestID] // I don't have a requestID!

        if options?.authenticate {
            var urlRequest = urlRequest
            urlRequest.setValue("Bearer: 1234", forHTTPHeaderField: "Authorization")

            completion(.success(urlRequest))
        } else {
            completion(.success(urlRequest))
        }
    }

    func append(_ request: Request, with options: RequestOptions) {
        requests[request.id] = options
    }
}

let interceptor = MyInterceptor()
let options = RequestOptions(authenticate: true)
let urlRequest = URLRequest(url: URL(string: "https://httpbin.org/get")!)

let request = AF.request(urlRequest, interceptor: interceptor).response { response in
    print(response)
}

interceptor.append(request, with: options)

This example is drastically simplified, but it demonstrates the core issue. I don't have a way to match the RequestOptions against the URLRequest. Please ignore the thread safety issues with the example above. I have those sorted in my client library.

Instead, I'd like to propose the addition of the new adapt API:

func adapt(_ urlRequest: URLRequest, 
           using state: RequestAdapterState, 
           completion: @escaping (Result<URLRequest, Error>) -> Void)

This new API will give you access to the original Request.id which you can then use in your client library to do anything you need. In my example above, I'll now be able to use state.requestID as the requestID in that example to authenticate the URLRequest with the Authorization header.

Implementation Details 🚧

The new RequestAdapterState struct is a new approach to protocol design in Alamofire when we (Alamofire) need to expose parts of the internal state to the client. Instead of always trying to customize the parameters in the protocol functions, we should instead define State objects which we can add / remove state from over time which can actually be deprecated properly between MAJOR version changes. Right now, there's no possible way to extend the current adapt API without creating a completely new function. This approach should give us more flexibility over time when we find additional cases where we need to expose a bit more state to provide some extra functionality.

I decided to implement the new API as a protocol extension for backwards compatibility. I also considered adding a new RequestAdapterV2 protocol which extended RequestAdapter. This approach ended up not giving us any actual benefit or easier deprecation strategy than just adding the function on to RequestAdapter directly. I also considered adding a separate RequestAdapterV2 protocol, but that's just not practical given how different public APIs we support with RequestInterceptor. In the end, it ended up making the most sense to add another method to the protocol and add a default protocol extension for backwards compatibility.

Testing Details 🔍

I added a few tests in the cases where I wanted to verify that the classes (Adapter and Interceptor) are actually overriding the protocol extension correctly. They all are and everything is working as expected.

@cnoon cnoon requested a review from jshier October 10, 2021 04:29
Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

👍 This looks good functionally, I just have a testing suggestion. I'd also like to ask you update the various bits of RequestAdapter documentation in the Usage and AdvancedUsage documents (and anywhere else you can think of) to reflect the new protocol signature.

completion(.success(request))
}

let state = RequestAdapterState(requestID: UUID(), session: session)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest capturing the UUID separately and then comparing the values returned from the adaptation to those provided in the RequestAdapterState, just to ensure they're round tripping appropriately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 2dbda96. A little tricky to implement, but a subclass Adapter fixed things right up.

@cnoon
Copy link
Member Author

cnoon commented Oct 11, 2021

That's good feedback @jshier. I'll update the docs as well attempting to explain the second API and why it exists.

@cnoon
Copy link
Member Author

cnoon commented Oct 14, 2021

Updated the docs in a602c12.

Copy link
Contributor

@jshier jshier left a comment

Choose a reason for hiding this comment

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

👍 Looks good with the documentation. I might add a bit about how the new method will become the requirement in the future, but that's up to you. Feel free to merge when you want.

@cnoon
Copy link
Member Author

cnoon commented Oct 19, 2021

Good callout @jshier. I added a blurb about the new requirement in 3c983a6.

@cnoon cnoon merged commit 79e9419 into master Oct 19, 2021
@cnoon cnoon deleted the cnoon/request-adapter-state branch October 19, 2021 18:51
@jshier jshier added this to the 5.5.0 milestone Dec 13, 2021
jshier pushed a commit that referenced this pull request Jan 15, 2022
…ptation (#3504)

* Added RequestAdapterState struct to store additional state during adaptation

* Updated adapter tests to verify RequestAdapterState passthrough

* Updated RequestAdapter documentation in AdvancedUsage

* Added comment about RequestAdapterState method becoming new requirement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants