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

[http] ✨ Add redirects in IOStreamedResponse #1076

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AlexV525
Copy link
Contributor

@AlexV525 AlexV525 commented Dec 10, 2023

The request provides the ability to extract the redirections from an IOStreamedResponse.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@AlexV525 AlexV525 changed the title ✨ Add redirects in IOStreamedResponse [http] ✨ Add redirects in IOStreamedResponse Dec 10, 2023
@AlexV525 AlexV525 marked this pull request as ready for review December 10, 2023 03:31
Copy link
Collaborator

@brianquinlan brianquinlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AlexV525

Why are you only adding this to IOStreamedResponse? We want the different Client implementations to behave as similarly as possible and this approach requires that the caller cast into an implementation-specific type.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Dec 16, 2023

Why are you only adding this to IOStreamedResponse?

Because RedirectInfo is available in dart:io. I can keep this implementation or add the same class in the package.

@glanium
Copy link

glanium commented Dec 16, 2023

Please add redirects in IOStreamedResponse for workaround (#699)
Then no breaking changes.

With this, we can cast and then get redirect(final) url in specific usecase.
There's better than nothing.

When will you add Response.uri (#699)?

Http is one of most important feature in app.
dart:io's http has no http2 no http3
dart:http has no Response.url.
This is really annoying.

@AlexV525
Copy link
Contributor Author

When will you add Response.uri (#699)

That is the next thing I'm considering.

@brianquinlan brianquinlan requested a review from natebosch January 4, 2024 18:28
@brianquinlan
Copy link
Collaborator

@natebosch this is a less-breaking version of your change #699

My concern is that making this work only for IOClient will result in people writing more implementation-specific code.

Maybe it would make sense to do a major release with #699 and possibly also:

  1. progress notification: https://github.com/dart-lang/http/pull/579/files
  2. request cancellation/timeout: Add support for request timeout #521

@natebosch
Copy link
Member

Maybe it would make sense to do a major release with #699 and possibly also:

Yeah a long while ago I had anticipated bundling some breaking changes and doing a major version release, but it never bubbled up to a high enough priority that I took the time to plan out the full migration story.

This package requires incremental migration paths across major versions - we will need to be compatible with the same Flutter SDK version across both major versions, so that we can use the new version internally before publishing, and Flutter can wait for publish until it bumps a dependency.

  1. progress notification: https://github.com/dart-lang/http/pull/579/files

I still have concerns about the fidelity of this API. (Although I don't see my comments there - was there a similar PR before?) Have we verified that if we are communicating with a server that stops reading halfway through the request it is correctly reflected in the upload progress?

The web implementation looks like it can make the same promises as a supported JS API, so there shouldn't be a problem there. The VM implementation looks like it relies on there not being any caching anywhere between the user's Stream and the communicating socket, and that there is appropriate backpressure applied to the Stream. If there can be a large difference between the amount of events sent to .map and the amount of data actually added to the socket, this wouldn't be reliable.

@natebosch
Copy link
Member

My concern is that making this work only for IOClient will result in people writing more implementation-specific code.

+1. I would be OK with a temporary situation where the API is only available on the IO specific subclasses to minimize breakage while we roll it out, but I would much prefer consistency across the APIs as long as it's feasible to support on each platform. Can we support redirect on the web and with the cronet clients etc?

@brianquinlan
Copy link
Collaborator

My concern is that making this work only for IOClient will result in people writing more implementation-specific code.

+1. I would be OK with a temporary situation where the API is only available on the IO specific subclasses to minimize breakage while we roll it out, but I would much prefer consistency across the APIs as long as it's feasible to support on each platform. Can we support redirect on the web and with the cronet clients etc?

We can support redirect everywhere except on the web (using XMLHTTPRequest). The formulation where we only provide the final URL (e.g. as with #699) is supportable everywhere.

How about this (your opinion would be useful here too @AlexV525):

  1. We modify this PR to capture just the final url rather than the complete redirect list. Maybe:
/// The final [Uri] returned by the server after any redirects.
///
/// [finalUri] will only be `null` in certain edge cases. For example, when fetching the "about:" URL in a browser, [finalUri]
/// may be `null`.
final Uri? finalUri;
  1. We plan on moving that definition to BaseResponse in the next breaking release (with appropriate modifications to CronetClient, BrowserClient and CupertinoClient as well as conformance tests)

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 4, 2024

The redirects are still valuable for a response. I can add finalUri too. In dio, we also have a fork class for redirects and a field named realUri. Redirects is an empty list on the Web platform.

Meanwhile, can we push splitting basic http classes from io?

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 4, 2024

@brianquinlan
Copy link
Collaborator

The redirects are still valuable for a response. I can add finalUri too. In dio, we also have a fork class for redirects and a field named realUri. Redirects is an empty list on the Web platform.

I think that adding an equivalent of realUri would be less controversial since it has a clear use case (#293) and can be implemented for all Client implementations.

Meanwhile, can we push splitting basic http classes from io?

I don't know what you mean.

@brianquinlan
Copy link
Collaborator

Please add redirects in IOStreamedResponse for workaround (#699) Then no breaking changes.

With this, we can cast and then get redirect(final) url in specific usecase. There's better than nothing.

When will you add Response.uri (#699)?

Http is one of most important feature in app. dart:io's http has no http2 no http3 dart:http has no Response.url. This is really annoying.

Fixing this in IOStreamedResponse won't fix your issue because IOClient only supports http1.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 4, 2024

Meanwhile, can we push splitting basic http classes from io?

I don't know what you mean.

One of the reason I've only added this field to the IOStreamedResponse is #1076 (comment).

@brianquinlan
Copy link
Collaborator

Meanwhile, can we push splitting basic http classes from io?

I don't know what you mean.

One of the reason I've only added this field to the IOStreamedResponse is #1076 (comment).

Ah, I understand. I think that the usual approach for package:http would be to define your own class. But now now maybe we can just support finalUri ;-)

@brianquinlan
Copy link
Collaborator

Another approach that we could define a mixins like:

mixin FinalUrl on BaseResponse {
  abstract Uri? finalUrl;
}

Users could consume it like:

void main() async {
  final response = await client.get();
  if (response is FinalUrl) {
    doSomethingWithFinalUrl(response.finalUrl);
  }
}

We could define a hidden class that implements that mixin. In the next breaking release, we could add finalUrl to BaseResponse and deprecate the mixin. Then remove it in package:http 3.0

@natebosch
Copy link
Member

Another approach that we could define a mixins

Interesting, the subclasses could publicly claim to support this interface which would allow the same code as adding them directly on those implementations, but also give a way to use it without a platform specific import.

I'd think we wouldn't want the if (response is FinalUrl) { patterns to spread very far, but having to keep the mixins around until version 3.0 might not be too bad.

We might want to flesh this idea out some more and see if there are any negative implications.

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 5, 2024

Please don't hesitate to make breaking changes. I'll try adding a new class for redirects and adding finalUri too.

@AlexV525 AlexV525 marked this pull request as draft January 5, 2024 00:15
@brianquinlan
Copy link
Collaborator

Please don't hesitate to make breaking changes.

That would go against our general policy when developing packages ;-)

I'll try adding a new class for redirects and adding finalUri too.

How about just for finalUri?

@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 5, 2024

I'll try adding a new class for redirects and adding finalUri too.

How about just for finalUri?

That would be fine too, but I don't see any strong reason to abandon this PR.

@brianquinlan
Copy link
Collaborator

I'll try adding a new class for redirects and adding finalUri too.

How about just for finalUri?

That would be fine too, but I don't see any strong reason to abandon this PR.

You don't need to abandon this PR but I would start with just finalUri because:

  1. we have a use case for it
  2. it is less complex to implement
  3. it is implementable in all of our Clients

@AlexV525 AlexV525 marked this pull request as ready for review January 5, 2024 02:03
@AlexV525
Copy link
Contributor Author

AlexV525 commented Jan 5, 2024

Marking as ready for review again.

@AlexV525
Copy link
Contributor Author

ping @brianquinlan

@brianquinlan
Copy link
Collaborator

brianquinlan commented Jan 10, 2024

ping @brianquinlan

  1. Are you sure that this is needed? Now that the final URL is available in BaseResponse, do we need a list of redirects?

  2. If we do need the list of redirects, could we follow the pattern of Add BaseResponseWithUrl.url #1109 so that other clients can offer that data too?

Of the clients that I know about:

  1. after this change lands, we should update package:http_client_conformance_tests redirect_tests.dart to ensure that all Clients are implementing this consistently.

@Zekfad
Copy link
Contributor

Zekfad commented Jan 10, 2024

Browser fetch can at least provide info if there was (or not) redirect and final URL at no additional cost:

https://github.com/Zekfad/fetch_api/blob/master/lib/src/response/response.dart#L53

https://github.com/Zekfad/fetch_api/blob/master/lib/src/response/response.dart#L69

But manual redirect handling is expensive, as you point out it requires additional HEAD (or GET) request to retrieve final URL and forge a fake redirect response accordingly.

XHR has responseURL and checked if normalized initial URI != responseURL would partially polyfill fetch behavior, though I haven't checked it personally.

@AlexV525
Copy link
Contributor Author

  1. Are you sure that this is needed? Now that the final URL is available in BaseResponse, do we need a list of redirects?

I believe we should definitely provide such info to users.

  1. If we do need the list of redirects, could we follow the pattern of Add BaseResponseWithUrl.url #1109 so that other clients can offer that data too?

That's a great suggestion. I'll take a look

  1. after this change lands, we should update package:http_client_conformance_tests redirect_tests.dart to ensure that all Clients are implementing this consistently.

Sure, will check this later.

@AlexV525 AlexV525 marked this pull request as draft January 10, 2024 16:19
@brianquinlan
Copy link
Collaborator

  1. Are you sure that this is needed? Now that the final URL is available in BaseResponse, do we need a list of redirects?

I believe we should definitely provide such info to users.

I'll let this be your decision but, as far as I know, we don't have an actual use case for this.

  1. If we do need the list of redirects, could we follow the pattern of Add BaseResponseWithUrl.url #1109 so that other clients can offer that data too?

That's a great suggestion. I'll take a look

  1. after this change lands, we should update package:http_client_conformance_tests redirect_tests.dart to ensure that all Clients are implementing this consistently.

Sure, will check this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants