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

[core-http] Redirect pipeline #11863

Merged
3 commits merged into from Oct 15, 2020
Merged

[core-http] Redirect pipeline #11863

3 commits merged into from Oct 15, 2020

Conversation

joheredi
Copy link
Member

@joheredi joheredi commented Oct 15, 2020

core-http has been using fetch without explicitly specifying a redirect behavior which defaults to follow. This caused our redirectPipeline to never actually execute.

In this PR I'm setting redirect to manual so that our pipeline handles redirects. Unfortunately there doesn't seem to be a way to set manual redirect handling for XMLHttpRequest, so browsers will keep the default behavior.

I'm also fixing the redirect logic to align with the tests in Autorest Test Server and be consistent with the other languages

Fixes #10103

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I love the tests and thanks for porting the changes to core-https!

One small suggestion, but otherwise LGTM

sdk/core/core-http/src/policies/redirectPolicy.ts Outdated Show resolved Hide resolved
sdk/core/core-https/src/policies/redirectPolicy.ts Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 15, 2020

Hello @joheredi!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

This pull request was closed.
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.

[core-http] Fetch default redirect handling disables core-http RedirectPolicy
3 participants