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

Leading slash broke RequestUri in HttpClient with BaseAddress #568

Closed
ch1seL opened this issue Nov 11, 2020 · 9 comments
Closed

Leading slash broke RequestUri in HttpClient with BaseAddress #568

ch1seL opened this issue Nov 11, 2020 · 9 comments

Comments

@ch1seL
Copy link

ch1seL commented Nov 11, 2020

Hi!

I've tried to bump Flurl and faced with broken RequestUri.

My HttpClient has BaseAddress with some relative path like http://fake.com/api/
I compose Uri and Flurl adds leading / when I use AppendPathSegment for example "path1".AppendPathSegment("path2")
As a result, RequestUri equals http://fake.com/path1/path2 but expected http://fake.com/api/path1/path2

See https://github.com/ch1seL/FlurlTest I reproduced it there

Could I use relative paths in BaseAddress or it is a bad practice?

@ch1seL ch1seL added the bug label Nov 11, 2020
@tmenier
Copy link
Owner

tmenier commented Nov 11, 2020

Could you point me to the specific code and not the whole repo?

@ch1seL
Copy link
Author

ch1seL commented Nov 11, 2020

Could you point me to the specific code and not the whole repo?

this is a little solution special for testing this bug =)
there are two projects(depend on Flurl2 and FLurl3) with one test it uses GetRequestUrl

Result:
Flurl2Tests passed
Flurl3Tests failed

@tmenier
Copy link
Owner

tmenier commented Nov 11, 2020

I understand what it is what it is but if you instead provided a minimal repro I could probably answer right now. What's happening in this test is confusing and needs to be reduced. I have no idea why you're doing a fake HTTP call to get the actualUrl string for example, but that shouldn't be necessary to repro a bug in something that only deals with string manipulation. Get it down to a minimal example comparing a hard-coded expected URL to one that's composed using AppendPathSegment and I'll take a look.

@ch1seL
Copy link
Author

ch1seL commented Nov 11, 2020

I do fake HTTP call through HttpClient cause it is correlated to HttpClient logic
Ok, this minimal example without Flurl

            var httpClient = new HttpClient()
            {
                BaseAddress = new Uri("http://fake.com/api/")
            };
            httpClient.GetAsync("path"); // RequestUri is http://fake.com/api/path
            httpClient.GetAsync("/path"); // RequestUri is  http://fake.com/path

Flurl 3 returns path starting with / but Flurl 2 don't

@tmenier
Copy link
Owner

tmenier commented Nov 11, 2020

I meant a minimal example demonstrating the Flurl bug, but let's forget about that and go back to your original tests. Your GetRequestUrl method takes 2 stings and makes a call using HttpClient. It doesn't use Flurl at all. The only place you're using Flurl is to construct that 2nd string (requestUri), so if you're trying to demonstrate that there's a regression bug with Flurl, all you should need to do is inspect the value of requestUri and determine if it's what you expect.

From your test:

// what is the value of requestUri here? is it what you expect? is it different than Flurl2?
Url requestUri = path1.AppendPathSegment(path2);

// you're completely done using Flurl here, so this part doesn't even matter
var actualUrl = await GetRequestUrl(baseAddress, requestUri);

@dalibormesaric
Copy link

I can confirm that we have the same issue after upgrading Flurl to version 3.0.0. No code changes, and now LocalPath in BaseAddress is not respected for some reason. It is reasonable to suspect to Flurl since that is the only thing that was changed.

@ch1seL
Copy link
Author

ch1seL commented Nov 11, 2020

Flurl 2:

"path1".AppendPathSegment("path2") // "path1/path2"

Flurl 3:

"path1".AppendPathSegment("path2") // "/path1/path2"

@tmenier
Copy link
Owner

tmenier commented Nov 11, 2020

@ch1seL Perfect, that's exactly what I was after. I didn't doubt there might be an issue lurking somewhere, it just got rather obfuscated in those tests. :)

I agree that the Flurl 2 behavior is more correct and that this is a regression bug. I will get it fixed. (As a side note, if you use Flurl.Http to make your calls rather than HttpClient, it shouldn't matter.)

@ch1seL ch1seL closed this as completed Nov 12, 2020
@ch1seL ch1seL reopened this Nov 12, 2020
tmenier added a commit that referenced this issue Nov 28, 2020
@tmenier
Copy link
Owner

tmenier commented Dec 14, 2020

Fixed & released in 3.0.1

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

No branches or pull requests

3 participants