Redirect config per request

[ncke]

Allows redirect handling to be configured per request.

Motivation:

Redirect configuration is set globally when configuring the HTTPClient. However, it would also be useful to configure redirects on a per request basis. This would mean that a single HTTPClient could be configured and shared widely throughout an application, while retaining the flexibility to tailor specific requests with custom redirect handling should the need arise.

Modifications:

This change adds an optional redirectConfiguration parameter to all request methods of HTTPClient, with a nil default value. When the request is executed the prevailingConfiguration is determined using the redirectConfiguration argument, if set. Otherwise the global configuration is used.

Result:

Allows redirect handling to be configured on a per request basis, closes #196

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[swift-server-bot]

Can one of the admins verify this patch?

[Lukasa]

@swift-server-bot test this please

[Lukasa]

Before we go down this road, we should take this opportunity to ask ourselves whether there is any other configuration that can sensibly be provided "per call". If there is, we may want to whack it all in at once.

[Lukasa]

On the code front, can you clean up the formatter issues? They're shown in the CI output.

[Lukasa]

I think I've talked myself into saying that redirect is special here. Most of the others are either managed by the connection pool or cannot safely be changed without invalidating the pool, or are otherwise already handled. So I think I'm ok with the shape of this change. @artemredkin any preference for anything else here?

[Lukasa]

Oh, one note though @ncke: we can't add optional parameters to these methods, it's a semver major change to do it. We need to add new methods with extra arguments instead.

[ncke]

I'll add new methods rather than the arguments.

I was also wondering whether the Task might be a better place to store redirect state (e.g. count) rather than the Request. I'm concerned that replaying an executed (or executing) Request may copy the dirty redirect state rather than creating pristine state for the second execution, with unexpected results.

[Lukasa]

Request is a value type: mutations made to that field by HTTPClient are not reflected in your local copy.

[Lukasa]

Great, I think this broadly looks good, just a single thing I'd like to see addressed.

[Lukasa]

Can we update these docstrings to explain what nil means? This applies to all new methods.

[ncke]

Sure. Can we also change the argument name from redirectConfiguration to redirects? I think there was an earlier suggestion that this is more pithy and I like the idea.

[Lukasa]

No objection from me.

[dimitribouniol]

I think I've talked myself into saying that redirect is special here. Most of the others are either managed by the connection pool or cannot safely be changed without invalidating the pool, or are otherwise already handled. So I think I'm ok with the shape of this change. @artemredkin any preference for anything else here?

Perhaps we can have a configuration type specific to requests (ie. execute(..., configuration: RequestConfiguration? = nil))? One more option that would be configurable per request would be the ability to specify how Host is represented, as brought up here: #229 (comment), and having a configuration type we can extend and add to could be useful here.

[dimitribouniol]

Oh, one note though @ncke: we can't add optional parameters to these methods, it's a semver major change to do it. We need to add new methods with extra arguments instead.

@Lukasa Most of these methods (ie. the ones that add logging and conveniences for socket paths) are new and haven't actually been released — would modifying those be ok?

[ncke]

@dimitribouniol With regard to simplifying future extension I wondered about retaining the current httpClient.get(...) style for simple cases, but contriving to allow httpClient.with(configuration: ...).get(...) to allow the caller to chain modifications to the execution context before reaching the final verb?

[Lukasa]

I definitely like the idea of having request configuration. I don't think I love the chaining idea though: I don't think it adds meaningful API clarity. Having a struct you can pass that defines a per-request configuration overload seems sensible though.

As for changing methods, anything that hasn't shipped in an existing release can be changed, yeah.

[ktoso]

Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.

This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.