Enable Authenticator.response(for:) to return a generic data type

[fritzt0]

First of all, I would like to thank you for providing this well-structured library. It is a joy to use. I use it primarily to control access to a medical records research server secured with OpenID Connect. These records are small, so the provided Authenticator.response(for:) method that returns a Data object is not a problem even for the limited memory on iPhones.

However, I now would like to use the library to access and transfer much larger data sets as well, and using Data as data-type to transfer a single entity is a problem. Streaming would be a much better choice (e.g. AsyncBytes or a file interface). To enable this, I tried to generalize the return type of Authenticator.response(for:) and created a proof-of-concept implementation. It works well, but I'm sure the implementation could be improved.

Would you be interested in adding a more generalized version of Authenticator.response(for:) to the library?

And an additional question: How would you go to provide a convenience interface for data uploading?

[mattmassicotte]

No thank you for identifying (and addressing!) this problem!

You’ve caught me at a bad time and I’m going to be a little slow looking. But in the mean time, if you want to try this against the swift6 branch that could be interesting. Had to make a few potentially-invasive changes…

[fritzt0]

Take your time, there is a solution for the time being. I'll have a look at the Swift 6 branch.

[fritzt0]

The swift6 branch is even better, fewer concurrency issues. I've rebased my changes onto the swift 6 branch (in a new branch) as well but I've no idea how to add it to this thread or if a new pull request is required.

[mattmassicotte]

Thanks again for doing this. A few questions!

I also think it would be great to also change the target branch to swift6. Is that possible without making a new PR?

[mattmassicotte]

Was this moved from Authentication.defaultResponseProvider?

[fritzt0]

Unfortunately, yes. I would be happy to hear of a way to avoid this. Currently, Swift does not support statically stored properties in generic types. I know this would be source breaking. Perhaps using a computed, non-static property could mitigate this problem now that you have also deprecated its use? (But this would require an instance of Authentication.)

[mattmassicotte]

You don't think "Response" was the right term?

[fritzt0]

I've been thinking about this for a while, and also imagined a future use where uploading data to OAuth protected servers can be supported. To minimize the impact on the existing API, I would suggest having two providers (which could be the same in the simple case). One used exclusively for authentication purposes and another for the user data transfer part. The former doesn't need to be generic, while the latter should support URLs, dates, and other types. An upload function (similar to Authenticator.response(for:)) would probably also benefit from a different name.

I'm fine with "Response", I was just thinking of another name for the general data type that could be used for downloading and uploading. In addition. A separate user data function would also provide more freedom in handling the user data, e.g. caching strategies, etc.

[fritzt0]

Thank you for your questions. Maybe I'm missing something, but I don't see any way to addendum to this pull request from another branch. I'll just use my main branch on top of your swift6 branch for the changes.

[fritzt0]

I had to force push my swift6 branch to main, thus the changes in main are now against your swift6 branch.

[mattmassicotte]

Ok you might have thought that I forgot about this but I definitely do not. In fact, I've been thinking about it all week. In fact, I've also being using this library myself a little for a side project again.

A problem I'm worried about is the need for two authenticators for different data types. Do you have any thoughts on something like this:

struct URLResponseProviders {
    let dataProvider: (URLRequest) async throws -> (Data, URLResponse)
    let asyncBytesProvider: (URLRequest) async throws -> (URLSession.AsyncBytes, URLResponse)
}

I kind of don't like typing the URLSession type directly to this, but I'm unsure how to get around this. Maybe a custom wrapper sequence type is necessary?

I'd just really like to be able to have one single authenticator instance that can do both of these things. And I think that may be important at runtime to correctly handle token refresh.

What do you think? (and thanks again for all your work here, I know its getting complex)

[fritzt0]

All good, I have very relaxed expectations when it comes to response times on side projects.

If I understand you correctly, you want to avoid generics and add asyncBytesProvider instead? What about the possibility of using URL as result data type? That is something I actively use as well. The URLSession downloads data and stores it in a temporary location and only returns the URL. This has advantages in certain situations. How would you represent a URL return type?

[mattmassicotte]

Yeah you got it. I think the authentication needs to (or at least should be possible to) apply to all request types, and not just one. Having to make more than one Authenticator instance should work, given they could share the same token storage backing. But, I'm worried about potential problems related to refresh.

But you also bring up a great point that URLSession can execute a bunch of different request forms. I've also used the upload and download mechanisms. The URL download shape is particularly tricky, because that URL must be synchronously accessed - it is deleted afterwards. I think all of this is possible to model with functions. Or perhaps a protocol makes more sense at this point?

[mattmassicotte]

First, to make your life yet more complex, I have merged the swift6 branch into main. It's been working well and the restrictions I've had to add have not been as bad as I feared they might be.

Ok, and now also I had an idea! Instead of modelling all possible forms of request, how about if instead a lower-level function is added like this:

func authenticateRequest(_ request: inout URLRequest) async throws

This will allow us to keep the existing API untouched, but also support more generalized kinds of request operations as long as they work with URLRequest. Could even be further generalized to just produce an "Authorization" header value.

[fritzt0]

Actually, the recent changes have simplified things for me, as my main branch now closely resembles yours, and the pull request checks are in good shape again. That being said, I would love to use your original main branch unchanged.

Your idea of introducing a function that adds the required authentication details to URLRequest sounds promising. However, I have some questions about it. Looking at the original implementation of Authenticator.response(for:), the actual request is wrapped in checks (e.g. the response status). It would be nice to still have data transfer functions that encapsulate the authentication mechanism. How would you imagine to embed the new authenticateRequest function? Thanks Matt.

[mattmassicotte]

Shoot you're right. This cannot work. Because to perform initial auth or refresh the authenticator itself needs to be aware of the response as well.

It's possible that a generic system really will be required here... that will be particularly tricky for handling the file download case. What about a function that takes a request and a generic operation to execute the request. This would allow the system to form the initial request, but give the caller arbitrary control over how to actually execute it and handle the result.

public func response<Payload>(
  for request: URLRequest,
  operation: (URLRequest) async throws -> (Payload, URLResponse)
) async throws -> (Payload , URLResponse)

[fritzt0]

That seems quite appealing and might reduce the effect of the generic abstraction.

[mattmassicotte]

Ok great, would you like to give it a shot?

[fritzt0]

Sure, but I can't promise a specific time frame.

[mattmassicotte]

Nor would I accept one even if you did! I just wanted know if you had interest in it.

[fritzt0]

I attempted to make only minimal changes to your original sources. From my experience, making

public func response<Payload>(
  for request: URLRequest,
  operation: (URLRequest) async throws -> (Payload, URLResponse)
) async throws -> (Payload , URLResponse)

generic has significant implications, akin to what drove me in the initial pull request to make several structs generic. Ultimately, it comes down to ResponseStatusProvider requiring Data as the payload. In my view, making this type generic will also necessitate TokenHandling to be generic and then it spreads.

[mattmassicotte]

That's kind of weird API though. Could it help if the Data parameter to that closure was optional?