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

[browser][http] Add support for AllowAutoRedirect. #41394

Merged
merged 11 commits into from
Sep 10, 2020
Merged

[browser][http] Add support for AllowAutoRedirect. #41394

merged 11 commits into from
Sep 10, 2020

Conversation

kjpou1
Copy link
Contributor

@kjpou1 kjpou1 commented Aug 26, 2020

Right now the property AllowAutoRedirect throws Platform Not Supported Exception trying to access this parameter.

  • Add property support for SupportsRedirectConfiguration, AllowAutoRedirect.
  • Property MaxAutomaticRedirections still throws PNSE as per comments below. Leaving it at Platform Not Supported Exception since it is not supported by platform fetch api. It may be confusing to see this set and fetch not respecting the value.

The fetch api supports three values for redirection and transparently follows HTTP-redirects, like 301, 302 etc.

  • The redirect option allows to change that:

    • "follow" – the default, follow HTTP-redirects,
    • "error" – error in case of HTTP-redirect,
    • "manual" – don’t follow HTTP-redirect, but response.url will be the new URL, and response.redirected will be true, so that we can perform the redirect manually to the new URL (if needed).

Note
The manual value above does not really follow what the documentation says. See issue Cannot get next URL for redirect="manual" another issue pertaining to this issue is Header to opt out of opaque redirect .


This PR will set the redirect property on the request object to manual if AllowAutoRedirect is false:

  • This stops the automatic redirection
  • Sets the HttpResponseMessage as follow:
response message: StatusCode: 0, ReasonPhrase: 'opaqueredirect', Version: 1.1, Content: System.Net.Http.BrowserHttpHandler+BrowserHttpContent, Headers:
{
   Content-Length: 0
}

One way to handle the redirect manually is checking the response message IsSuccessStatusCode is false and the ReasonPhrase is opaqueredirect.

As per the issue that is referenced the next URL is not modified to the the next URL location.

sample output from simulated test:

Response {type: "opaqueredirect", url: "https://localhost:6001/WeatherForecast/GetRedirect", redirected: false, status: 0, ok: false, …}
body: null
bodyUsed: false
headers: Headers {}
ok: false
redirected: false
status: 0
statusText: ""
type: "opaqueredirect"
url: "https://localhost:6001/WeatherForecast/GetRedirect"

If trying to set the AllowAutoRedirect after any request has been made then the following error is thrown:

 (index):1 Uncaught (in promise) System.AggregateException: One or more errors occurred. (This instance has already started one or more requests. Properties can only be modified before sending the first request.)
  ---> System.InvalidOperationException: This instance has already started one or more requests. Properties can only be modified before sending the first request.
    at System.Net.Http.BrowserHttpHandler.CheckDisposedOrStarted()
    at System.Net.Http.BrowserHttpHandler.set_AllowAutoRedirect(Boolean value)
    at System.Net.Http.HttpClientHandler.set_AllowAutoRedirect(Boolean value)
    at WasmHttpClientTest.Client.HttpClientRequestAsync(String url)
    --- End of inner exception stack trace ---


There are a number of differences when using the fetch api that does not line up with the dotnet core implementation.

Implementation summary:


References:


Close: #41255
Close: #39365

@ghost
Copy link

ghost commented Aug 26, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@kjpou1 kjpou1 self-assigned this Aug 26, 2020
@kjpou1 kjpou1 added the arch-wasm WebAssembly architecture label Aug 26, 2020
@karelz karelz added this to the 6.0.0 milestone Aug 26, 2020
kjpou1 and others added 2 commits August 27, 2020 08:56
…andler/BrowserHttpHandler.cs

Co-authored-by: campersau <buchholz.bastian@googlemail.com>
kjpou1 and others added 3 commits August 28, 2020 11:36
…andler/BrowserHttpHandler.cs

Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
@kjpou1 kjpou1 requested review from ManickaP and marek-safar August 28, 2020 09:52
@kjpou1
Copy link
Contributor Author

kjpou1 commented Aug 28, 2020

/cc @mkArtakMSFT @pranavkm just wanted to make sure you guys saw this in case you want to weigh in on it.

kjpou1 added 2 commits August 28, 2020 12:01
…into wasm-http-redirect

# Conflicts:
#	src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs
// The Response's status is 0, headers are empty, body is null and trailer is empty.
if (status.ResponseType == "opaqueredirect")
{
httpResponse.SetReasonPhraseWithoutValidation(status.ResponseType);
Copy link
Contributor

Choose a reason for hiding this comment

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

@karelz who could review this part? Should we try harder to map the result to status codes or expose that differently to the developers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello everyone. Is there any update on this?

/cc @marek-safar @karelz @ManickaP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also there is a line in the PR comments about mapping a result and the approach for this PR:

Instead of faking a 3XX status code the implementation uses the ReasonPhrase to return the fetch response type opaqueredirect.

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @dotnet/ncl

Hrm this is an interesting one. It seems like there is not a perfect solution here -- fetch API seems poorly made. ¯\_(ツ)_/¯

Without setting a normal redirect status code, we break compat with any existing code handling redirects.

But, it seems wrong to fake a status code when we don't know what the actual status code was. If we were to fake a status code, a temporary redirect seems like the best choice -- permanent redirect could cause dangerous handling e.g. updating a URI in a database.

To add a third bad option, what do we think of this? It at least gives the user some flexibility here.

public class BrowserHttpHandler
{
    public const HttpStatusCode OpaqueRedirect = (HttpStatusCode)1000;

    // if false, return TemporaryRedirect. if true, return OpaqueRedirect.
    public bool ReturnOpaqueRedirect { get; set; } = false;
}

(note, using a status code like this may not be advisable as an on-by-default option, as you might have some code doing e.g. if(StatusCode >= 500))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am understanding the above wouldn't this change the Public API as well?

@geoffkizer
Copy link
Contributor

Just to be clear -- since the fetch API does not give you the redirect URL, even with 'manual', there's basically nothing you can do with a redirect result other than know that it was a redirect, right?

So any existing code that wants to handle redirects manually is going to be broken anyway.

As such I'm fine just indicating the redirect via the ReasonPhrase, as originally proposed. It seems like fetch basically gives you two options, either auto-follow redirects, or don't handle redirects at all.

@scalablecory
Copy link
Contributor

scalablecory commented Sep 8, 2020

Just to be clear -- since the fetch API does not give you the redirect URL, even with 'manual', there's basically nothing you can do with a redirect result other than know that it was a redirect, right?

It seems to be used very little, I'm having trouble finding an exact answer, but some places indicate that manual redirect will still fill response.url.

Otherwise I'm not sure how it would be different from 'error`. @marek-safar can you clarify?

@kjpou1
Copy link
Contributor Author

kjpou1 commented Sep 9, 2020

@scalablecory The only case to go on is the issue Cannot get next URL for redirect="manual" that is closed on the actual fetch implementation repo which is what browsers actually use.

There is another issue labeled as Clarify "manual" redirect mode . Maybe that helps to clarify as It actually specifies the following:

"manual"
Retrieves an opaque-redirect filtered response when a request is met with a redirect, to allow a service worker to replay the redirect offline. The response is otherwise indistinguishable from a network error, to not violate atomic HTTP redirect handling.

@kjpou1
Copy link
Contributor Author

kjpou1 commented Sep 9, 2020

Just to follow up. If one uses error then an uncatchable error will be thrown in the browser with no relative/usable information being able to be captured.

@kjpou1
Copy link
Contributor Author

kjpou1 commented Sep 9, 2020

It seems to be used very little, I'm having trouble finding an exact answer, but some places indicate that manual redirect will still fill response.url.

My problem with adding code for this if it does work in some browsers and not others was to have a consistent interface at the least. If one browser work and another does not it will just cause even more confusion to the user. At least this way manual is "broken" consistently across browsers.

@scalablecory
Copy link
Contributor

scalablecory commented Sep 9, 2020

Okay, if "manual" is not giving consistent useful information across browsers, I guess setting the reason phrase is fine. It seems like it has a use that only makes sense in the browser, so portability is not a concern.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

:shipit: since @scalablecory is fine with it

@kjpou1 kjpou1 merged commit 4ad3912 into dotnet:master Sep 10, 2020
@kjpou1
Copy link
Contributor Author

kjpou1 commented Sep 10, 2020

Thanks everyone

@dazinator
Copy link

dazinator commented Sep 10, 2020

It seems like fetch basically gives you two options, either auto-follow redirects, or don't handle redirects at all.

In a blazor scenario using http client, having made a request that gets redirected - I'd like to auto follow but via a browser navigate to the new URL (i.e change the window.location to the new URL) instead of following via a new fetch request. Is this option possible? I don't need to have the new URL or the response actually surfaced to me from http client for this scenario.

@kjpou1 kjpou1 deleted the wasm-http-redirect branch September 11, 2020 04:04
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net.Http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[browser][http] HttpClient does not support redirect. [wasm] Blazor 303 responses, and fetch vs navigation
9 participants