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

Follow redirects #2058

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Follow redirects #2058

wants to merge 1 commit into from

Conversation

alexeyzimarev
Copy link
Member

@alexeyzimarev alexeyzimarev commented Apr 7, 2023

Description

Fixes #2059

Implement custom follow redirect functionality instead of relying on the message handler as it has limitations described in the issue.

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@stale
Copy link

stale bot commented May 21, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 21, 2023
@repo-ranger
Copy link

repo-ranger bot commented May 21, 2023

⚠️ This issue has been marked wontfix and will be closed in 3 days

@rassilon
Copy link

@alexeyzimarev , did you think about maybe borrowing RedirectHandler from net core here? i.e.: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs it might make the weird edge cases simpler to deal with... Having said that, i'll submit what I have so far in terms of improvements shortly.

@rassilon
Copy link

I've added a POST -> GET redirect with recieving cookies during the original 302 redirect response test to #2119.

@rassilon
Copy link

@alexeyzimarev , would you like me to try doing a set of your and my changes for follow redirects on top of the merged PR #2056?

@alexeyzimarev
Copy link
Member Author

@rassilon I actually didn't check the default implementation in the socket message handler, thanks for the links. My idea was more or less to follow, at least from settings perspective, the Flurl path. They have things like allow redirecting from https to http, something that just doesn't work with HttpClient. I haven't looked at your PR yet, will check soon, and big thanks for this contribution. It would be great to resolve the redirect issue.

@rassilon
Copy link

Ahh.. I see what you mean by Flurl.. very neat idea. Would you want to add settings for each of the decisions in the socket message handler? Or support the simpler approach Flurl uses? Which is apparently at: https://github.com/tmenier/Flurl/blob/dev/src/Flurl.Http/FlurlClient.cs#L162 atm. (i.e. a simple settings object and an event handler)

@rassilon
Copy link

I can give a whirl at making some more progress on this next week if you have other higher priority fish to fry.

@alexeyzimarev
Copy link
Member Author

simpler approach Flurl uses

We can't use the socket message handler anyway as it's only .NET 5+, or wrap the handler instantiation in pragma (also an option). But anyway, yes, I just want to make own decisions as I mentioned #2059 (comment)

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

Successfully merging this pull request may close these issues.

[RFC] Implement follow redirects
2 participants