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

Obtain Response URL #790

Closed
0xifarouk opened this issue Dec 6, 2024 · 7 comments · Fixed by #817
Closed

Obtain Response URL #790

0xifarouk opened this issue Dec 6, 2024 · 7 comments · Fixed by #817
Labels
kind/enhancement Improvements to existing feature.

Comments

@0xifarouk
Copy link

how can I get the response url?
in URLSession we can say:

let (data, response) = try await URLSession.shared.data(for: request)
print(response.url)

how to do that using AsyncHttpClient?

@Lukasa
Copy link
Collaborator

Lukasa commented Dec 9, 2024

Unfortunately, this information is not exposed today. We'd need some new API surface on the response to expose it, which in my view should also expose the rest of the redirect history.

@Lukasa Lukasa added the kind/enhancement Improvements to existing feature. label Dec 9, 2024
@gregcotten
Copy link
Contributor

This is particularly bumpy problem when trying to replace URLSession with async-http-client. @Lukasa do you have any ideas on where this API surface should go?

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 17, 2025

Response would be a fine place to put it, I think.

@gregcotten
Copy link
Contributor

gregcotten commented Feb 19, 2025

Unfortunately, this information is not exposed today. We'd need some new API surface on the response to expose it, which in my view should also expose the rest of the redirect history.

@Lukasa, I'll probably take a stab at this.

In Response, would we want a simple list of URLs?

var visitedURLs: [String]

var url: String? {
     get {
          self.visitedURLs.last
     }
}

or actually pairing a url with its corresponding HTTPResponseHead

var visitedURLs: [(url: String, head: HTTPResponseHead)]

or something really wild

var visitedURLs: [(request: HTTPClient.Request, head: HTTPResponseHead)]

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 19, 2025

So we should definitely avoid using a non-nominal type here, so I think the tuples are out: they will be hard to evolve if we want to add other things.

I wonder if we should go whole-hog and offer:

var history: [RedirectedRequest]

struct RedirectedRequest {
    var request: HTTPClient.Request
    var response: HTTPResponseHead
}

@gregcotten
Copy link
Contributor

gregcotten commented Feb 19, 2025

So we should definitely avoid using a non-nominal type here, so I think the tuples are out: they will be hard to evolve if we want to add other things.

I wonder if we should go whole-hog and offer:

var history: [RedirectedRequest]

struct RedirectedRequest {
var request: HTTPClient.Request
var response: HTTPResponseHead
}

That could work! There are some cases where we would have no history (for example, no redirects occur), and we'd still want to be able to access the URL of the response (as desired in this issue).

We could rename this struct something like RequestResult or VisitedURL, and always store the "ultimate" request/response as the last one so we can have some nice API like:

/// The target URL after redirects
var url: URL? {
   get {
      self.history.last?.request.url
   }
}

Another cause for concern: none of the HTTPClientResponseDelegate methods actually pass a HTTPClient.Request with a corresponding HTTPResponseHead, nor do they ever call existing delegate methods if the Task is simply redirecting. We would probably need to design a new delegate method that is called for every request/response including redirects. What should we name it?

func didReceiveHead(task: HTTPClient.Task<Response>, _ head: HTTPResponseHead, _ request: HTTPClient.Request) -> EventLoopFuture<Void>

Would we need the EventLoopFuture<Void>?

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 19, 2025

Your proposal generally sounds reasonable.

Extending the delegate should probably be done in a separate PR to avoid making the size get out of hand, as it will require somewhat meaningful surgery and testing. The name you propose is fine, but we do still want it to return a Future to enable delaying the I/O.

Lukasa pushed a commit that referenced this issue Feb 21, 2025
Trying to pave the way for closing
#790 with some
direction from @Lukasa.

I have no idea where is best to insert this new delegate method. I'm
currently doing it first thing in `receiveResponseHead0`, and not using
an EventLoopFuture for back pressure management. The state machine seems
pretty fragile and I don't want to leave too much of an imprint. Trying
to be a part of an EventLoopFuture chain seems really complicated and
would really leave a mark on the codebase, so I'm wondering if it's
possible to just warn the user "do not block"?

Anyway, just a jumping-off point and happy to take direction!
Lukasa pushed a commit that referenced this issue Mar 3, 2025
Work to close
#790

The fact that `HTTPClient.Request` is not Sendable make me think we're
going to need to store something else, such as a `URL` and
`HTTPRequestHead`, instead?
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

Successfully merging a pull request may close this issue.

3 participants