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

Conversion from relative HTTPRequest to absolute Foundation URL #36

Open
groue opened this issue Nov 15, 2023 · 5 comments
Open

Conversion from relative HTTPRequest to absolute Foundation URL #36

groue opened this issue Nov 15, 2023 · 5 comments
Labels
kind/enhancement Improvements to existing feature.

Comments

@groue
Copy link

groue commented Nov 15, 2023

Hello,

This issue is a feature request for a new API in the HTTPTypesFoundation module:

extension HTTPRequest {
    public func url(baseURL: URL) -> URL?
}

I met a need for this API while using http://github.com/apple/swift-openapi-generator

When writing a ClientMiddleware that processes http requests performed through URLSession, I need an absolute Foundation.URL in order to use various services such as HTTPCookieStorage.

This package defines a HTTPRequest.url property that looks very promising.

But the HTTPRequest I get from OpenAPIRuntime is not absolute. It is relative to a base URL, and its url property is nil:

struct MyMiddleware: ClientMiddleware {    
    func intercept(
        _ request: HTTPRequest,
        body requestBody: HTTPBody?,
        baseURL: URL,
        operationID: String,
        next: (HTTPRequest, HTTPBody?, URL) async throws -> (HTTPResponse, HTTPBody?)
    ) async throws -> (HTTPResponse, HTTPBody?)
    {
        print(request.url) // nil
    }
}

Since my requests are performed through URLSession, I was pretty sure to find some code that turns a (request, baseURL) pair into an absolute URL, suitable for URLRequest. Indeed, it is there.

This code is not public, so I defined my own version, inspired from the above implementation:

extension HTTPRequest {
    /// Returns an absolute URL.
    ///
    /// Inspired from <https://github.com/apple/swift-openapi-urlsession/blob/0.3.0/Sources/OpenAPIURLSession/URLSessionTransport.swift#L185-L208>
    func url(baseURL: URL) -> URL? {
        guard
            var baseUrlComponents = URLComponents(string: baseURL.absoluteString),
            let requestUrlComponents = URLComponents(string: path ?? "")
        else {
            return nil
        }

        let path = requestUrlComponents.percentEncodedPath
        baseUrlComponents.percentEncodedPath += path
        baseUrlComponents.percentEncodedQuery = requestUrlComponents.percentEncodedQuery
        return baseUrlComponents.url
    }
}

Since the implementation is not quite trivial, I believe it is a good candidate for inclusion in HTTPTypesFoundation.

What do you think?

@groue groue changed the title More conversions from HTTPRequest into a Foundation URL Conversion from relative HTTPRequest to absolute Foundation URL Nov 15, 2023
@groue
Copy link
Author

groue commented Nov 15, 2023

I was wondering if the introduction of url(baseURL:) would be a breaking change. Precisely speaking, would userland code that uses the url property stop compiling due to the new API?

But it looks like the 5.9 compiler is smart enough:

// No error, no warning, all inferences work as expected
func f(_ request: HTTPRequest, baseURL: URL) {
    let a = request.url                   // inferred as URL?
    let b = request.url(baseURL:)         // inferred as (URL) -> URL?
    let c = request.url(baseURL: baseURL) // inferred as URL?
}

I'm not sure, though, if the inference of request.url as URL?, which is important for backward compatibility, was already there in Swift 5.7.1 (the minimum version required by the package).

@guoye-zhang
Copy link
Contributor

URL should always be absolute in an HTTP request. It does not make sense to have a request with a relative URL.

I think this might be a bug in openapi-generator.

@guoye-zhang guoye-zhang transferred this issue from apple/swift-http-types Nov 15, 2023
@czechboy0
Copy link

@guoye-zhang @groue I think there might have been a small miscommunication here.

The HTTPRequest created by the generator never has the necessary pseudoheaders set for the url property to return a non-nil URL. It only has a path, nothing else needed to construct the full URL.

The full URL is constructed by the transport by taking the base URL (which is absolute) and appending the path and query to it.

This is structured this way because it's very common for a REST API to be described as a list of paths and methods, where the paths are e.g. /cats, and you run it against a specific base URL, for example https://catservice-prod.example.com/v1alpha, making the full URL https://catservice-prod.example.com/v1alpha/cats.

I think @groue's ask is reasonable for an extra utils function in HTTPTypes, but it's up to @guoye-zhang to decide whether it's in scope for that library.

This issue was only discovered when using the generator, but that's just because we're one of the adopters of HTTPTypes. There isn't a bug or anything in the generator that made this necessary, it's more about the API of HTTPTypes and a suggestion of extending it.

@guoye-zhang, would you want to transfer the issue back and then decide what to do with it in the swift-http-types repo?

@guoye-zhang
Copy link
Contributor

Discussed with @czechboy0 offline, it's a common use in REST frameworks that we could consider supporting in the future

@guoye-zhang guoye-zhang transferred this issue from apple/swift-openapi-generator Nov 15, 2023
@guoye-zhang guoye-zhang added the kind/enhancement Improvements to existing feature. label Nov 15, 2023
@groue
Copy link
Author

groue commented Nov 16, 2023

Discussed with @czechboy0 offline, it's a common use in REST frameworks that we could consider supporting in the future

Thank you @guoye-zhang.

It does not make sense to have a request with a relative URL. I think this might be a bug in openapi-generator.

Oh, I guess I said "relative url" because that concept was kind of relevant in the context, and because I never heard about HTTP/2 pseudo headers until today :-)

In the end, I don't know how much my version of HTTPRequest.url(baseURL:) depends on implementation details of OpenAPI generator and its URLSession transport. If it is possible that my version is fragile or not future-proof (it is fully quoted in the OP), then please consider prioritizing this feature request, for the benefit of other users of the package. Many people need urls, but only a few know them very well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants