refactor to make request non-throwing

[artemredkin]

Makes Request init non-throwing, also addresess #43

[artemredkin]

@swift-server-bot test this please

[artemredkin]

@swift-server-bot test this please

[weissi]

@swift-server-bot test this please

[weissi]

@swift-server-bot test this please

[tomerd]

@swift-server-bot test this please

[weissi]

thanks @tomerd , CI working again. Over to @artemredkin

[weissi]

cool, let's get this one in

[ianpartridge]

I'm sorry, I don't quite follow why this is necessary. I must be missing something... Couldn't we treat a Request's url property as the source of truth and control access to that?

[artemredkin]

It's not about access control. URL's host property is optional, but we require it to be non-optional (so we know where to connect, what default domain to use in cookies and how to redirect using relative URL). Before that we would unwrap host (or throw) and then store in the request, but this leads to two downsides:

1. We have to keep in sync host and url properties (and we cannot throw if we change a property, so url property would have to be set using a function, and this is a bit ugly, imho)
2. creating request is a throwing init
   To address those two points I propose to introduce new internal type, sort of like "validated request", where user will not be able to change it and Request will not be throwing.
   Does that make sense?

[ianpartridge]

Thanks for the explanation, I see now.

As we control the URL so we know its host property is set, can't we force unwrap instead of throwing?

[artemredkin]

What do you mean control the URL? URL is passed in by the user, so we don't know what will be inside it.

[ianpartridge]

Doh. I'm sorry, I see now. So we swap the throwing initializer for a failed future from execute(). I guess that's a better tradeoff although it does mean that one initializer throws and the other doesn't.

[tomerd]

1. i prefer non-throwing APIs

2. another option here is to define a custom url type in which host is not optional, and with [optional] initializers from string and URL

[artemredkin]

converted to optional. I'm not sure having our own URL type is the way to go atm, URL is used everywhere.

[tomerd]

I'm not sure having our own URL type is the way to go atm, URL is used everywhere.

@artemredkin the point was that instead of an internal RequestWithHost you can create and internal UrlWithHost (or whatever), as this is really what you want here, not a custom request object. this will help remove all the boilerplate wrapping eg L105-121

[artemredkin]

But how one would pass it further? Handler needs access to URL, host, method and headers. If we don't want to expose UrlWithHost we will still need some wrapper to pass both Request and UrlWithHost to `TaskHandler. Does it make sense?

[tomerd]

as i was working on demonstrating what i meant above, i believe i ended up with an even simpler and safer approach: #66

also fixes a bunch of other small bugs and removes runtime preconditions

wdyt?

[artemredkin]

@tanner0101 @tomerd @ianpartridge wdyt? should we merge?

[Lukasa]

Why do we consider making Request non-throwing to be an API goal?

[Lukasa]

As @weissi pointed out to me, if you want to replace a function that is throws with one that returns nil, there's a keyword for that: try?. It is always possible to trivially transform a throwing function to a nil-returning one. However, it is not possible to get the throwing information back.

Right now we notify the user of what exactly we think they did wrong, and I think it's simply the wrong thing to do to hide this information.

[artemredkin]

In this PR we don't remove any diagnostic information, we just shift it from Request.init to client.execute. Main goal of this PR is to fix an issue where if you change the a URL of the request, host will not update. We only need host right before we start the execution, and that can make unit non-throwing

[Lukasa]

Why are we doing that, though? Why is it better to move that failure into execute?

[Lukasa]

The only reason I can see for doing this is #43, but we can enforce #43 by using computed properties or controlling modification via functions. I'm not sure it's a good idea to allow construction of a request object that is not valid.

[artemredkin]

#43 is exactly the reason. How can we achieve this using computed properties?

We cannot validate the request completely until we start to execute it. This is why I think we can remove host from the Request and move validation (since host is the main reason we throw from Request.init) to the request execution instead.

[Lukasa]

We cannot validate the request completely until we start to execute it.

What's the evidence for that assertion?

How can we achieve this using computed properties?

Don't: use a function instead.

[artemredkin]

What's the evidence for that assertion?

URL(string: "http://some.host/some/path") - host here can be invalid, it can be https server, path may be absent, method might be different on that path, etc. There are only two things we can definitely say about request: that it contains some host and scheme of the URL is http or https. Maybe 'valid' is not the best word to use in this context.

Don't: use a function instead.

Do you think we should use functions for other properties as well for consistency? Or only for url?

[Lukasa]

Do you think we should use functions for other properties as well for consistency? Or only for url?

Only for updating URL. Essentially, url would become public private(set).

[tomerd]

@artemredkin why do we feel the request url need to be mutable in the first place? i can see somewhat of a reason to be able to change headers and body, tho arguably they should be immutable too, as demonstrated in #66

[Lukasa]

I disagree that they should be immutable, tbh: making structs mutable generally leads to nicer design patterns. Any mutation that needs to be validated, though, should be a function.

[tomerd]

I disagree that they should be immutable, tbh: making structs mutable generally leads to nicer design patterns.

maybe in general. focusing on this use case, what are some concrete use cases when mutating the request's url is desirable, and cannot be more clearly achieved compared to constructing a new request? i dont see this adding much value and its obv causing validation races in this use case

[Lukasa]

The concrete case in this repo is for redirects, but in general when mutating state it is nicer to mutate the state directly, rather than via a copy-and-replace. As an example, consider redirection in the face of a 302 with Location: /foo. Ignoring bodies, headers, and other things for a moment, as well as ignoring error handling, we can do this in two ways:

mutating func redirect(to newTarget: String) {
    self.url = URL(string: newTarget, relativeTo: self.url)!
}

or

mutating func redirect(to newTarget: String) {
    let newURL = URL(string: newTarget, relativeTo: self.url)!
    self = Request(url: url, version = self.version, method: self.method, headers: self.headers, body: self.body)
}

I don't think the second case is any clearer: there are a lot of extra moving pieces, I have to update this code any time I change the list of fields, there are more places for me to make typos, and there's extra code. More importantly, IMO, this is something we already have a spelling for: it's the spelling in the first method.

The only reason to make the url field immutable is if any change to it will violate one of the invariants of the struct. This is not the case for this type: only some changes violate those invariants. It seems to me that the better design is to remove the property setter and replace it with a function that throws when you attempt to set a new URL.

[tomerd]

changing url, schema or host on the request object as implemented now invalidates the logical state of that object since it has business logic tied to these three fields and they can get out of sync under the current implementation, so this needs to be addressed

when it comes to redirects, you typically mutate more than the url (headers, sometimes body) so that is not a strong enough reason imo. all in all, i think a better tradeoff here is to not allow the modification of these three fields as it solves a class of state issues, current and future. we can always add it later via a validating function as you suggested above, if someone needs it

[Lukasa]

when it comes to redirects, you typically mutate more than the url (headers, sometimes body) so that is not a strong enough reason imo. all in all, i think a better tradeoff here is to not allow the modification of these three fields as it solves a class of state issues, current and future. we can always add it later via a validating function as you suggested above, if someone needs it

Not really. I think this is the crux of our confusion.

Request is a struct, meaning it is a value type. Unless the user elects to share it by storing it in a reference type, all instances of Request in a program are completely independent. Modifying the state of a given Request object changes nothing else in the program, including any other copies of the Request object. For this reason, there is no requirement to make the object immutable: it's not a concept that makes much sense for value types.

What does make sense is to restrict the domain of the stored properties where their domain is too large. My proposal would be to do something like this:

public struct Request {
    public private(set) var url: URL
    
    public init(url: URL, version: HTTPVersion = HTTPVersion(major: 1, minor: 1), method: HTTPMethod = .GET, headers: HTTPHeaders = HTTPHeaders(), body: Body? = nil) throws {
        self.version = version
        self.method = method
        self.headers = headers
        self.body = body
        self.url = url

        try self.url.validate()
    }
}

extension URL {
    internal func validate() throws {
        guard let scheme = self.scheme else {
            throw HTTPClientError.emptyScheme
        }

        guard Request.isSchemeSupported(scheme: scheme) else {
            throw HTTPClientError.unsupportedScheme(scheme)
        }

        guard self.host != nil else {
            throw HTTPClientError.emptyHost
        }
    }
}

This provides us a way to modify the URL on a specific request that cannot help but validate it, as well as to force the construction to use a valid URL. This then allows the use of the Request as a fully-featured value type. It no longer becomes possible to create an invalid Request at any stage of the program, while ensuring that the local state is always valid. New Request objects can therefore be created just by mutating the one you already have, as well as being assembled from their component parts.

[tomerd]

I think this is the crux of our confusion.

no, there is not a confusion about value types :) but there may be a confusion about the fact that under the current implementation url, schema and host are all public vars, and you can change any of them and cause business logic problems since they can become out of sync. this is why i am suggesting we change them to let - it will eliminate this class of logic issues, while allowing the modification of the url, schema and host does not buy us a great deal. i do agree that a validating setter the right approach if/when we need to allow url modifications on the request, but imo its "too early" to introduce and we can wait until there is a concrete need/ask. here is what this could look like: #67

[Lukasa]

Oh, sorry, I'm proposing that we make the setter of url private and hide that setter behind a throwing function. At that point, host and scheme can be converted into computed properties (including setters, if we're so inclined).

[tomerd]

@Lukasa we could. i think #67 is okay as is. we could change the validation to be behind an extension too and add a private setter for url, tho it wont be used by anything

[artemredkin]

@Lukasa @tomerd
What about a different approach:

struct HTTPURL {
    let url: URL
    let host: String
    let scheme: String
    init(url: URL, host: String, tls: Bool) {

    }
    init(validating url: URL) throws {
        // validates scheme and host
    }
}
struct Request {
    var url: HTTPURL
    var method: HTTPMethod
    var headers: HTTPHeaders
    var body: Body?
}

That will allow to rewrite redirect handler to:

    func redirectTarget(status: HTTPResponseStatus, headers: HTTPHeaders) -> HTTPURL? {
        /// ....

        guard let url = URL(string: location.value, relativeTo: request.url.url) else {
            return nil
        }

        return try? HTTPURL(validating: url.absoluteURL)
    }

    func redirect(status: HTTPResponseStatus, to redirectURL: HTTPURL, promise: EventLoopPromise<T>) {
        let originalURL = self.request.url
        var request = self.request
        request.url = redirectURL

        var convertToGet = false
        if status == .seeOther, request.method != .HEAD {
            convertToGet = true
        } else if status == .movedPermanently || status == .found, request.method == .POST {
            convertToGet = true
        }

        if convertToGet {
            request.method = .GET
            request.body = nil
            request.headers.remove(name: "Content-Length")
            request.headers.remove(name: "Content-Type")
        }

        if !originalURL.hasTheSameOrigin(as: redirectURL) {
            request.headers.remove(name: "Origin")
            request.headers.remove(name: "Cookie")
            request.headers.remove(name: "Authorization")
            request.headers.remove(name: "Proxy-Authorization")
        }

        self.execute(request).futureResult.cascade(to: promise)
    }

That will allow us to have both throwing Request and HTTPURL inits, as well as non-throwing init. Wdyt? Is it too crazy?

[Lukasa]

Why shouldn't scheme and host simply be computed properties from url? That would make url the source of truth, and so long as all paths to set it ensure that it's validated there is no risk of these things falling out of step.

[artemredkin]

Because they optional. Are you suggesting to force-unwrap them?

[Lukasa]

A URL that has passed your validation rules will never have a nil scheme or host, right?

[tomerd]

agree with @Lukasa, once you pass validation you can just assign to internal lets like demonstrated in #67, or compute them from the url

to reiterate, i think the best tradeoff here is #67, but if we feel strongly that url should be mutable (personally i don't think there enough justification), then we can amend that to include a validating setter for url

[Lukasa]

I think I'd like to see #67 amended to include a validating setter for URL, and to return the various properties to mutable.

[tomerd]

@Lukasa @artemredkin updated #67

[tomerd]

i prefer the approach suggested in #67

[artemredkin]

Closed in favour of #67