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

[Bug]: BaseAddress / relative URL formats - HttpClient style vs Refit style #1803

Open
hansmbakker opened this issue Sep 3, 2024 · 6 comments
Labels

Comments

@hansmbakker
Copy link
Contributor

hansmbakker commented Sep 3, 2024

Describe the bug 🐞

Looking at the sources linked below, Refit seems to expect a configuration of HttpClient's `BaseAddress and relative URLs in a way that is different than how HttpClient is meant to be used.

If I misunderstood those sources - please close this issue.

Unfortunately HttpClient is not "smart" about combining the BaseAddress with relative urls and e.g. if you configure the base address with a trailing slash while also configuring the relative urls with a leading slash, it will result in a double // which results in a 404 error at runtime.

Actual behavior

Refit expects the paths to start with a /:

if (!relativePath.StartsWith("/"))
throw new ArgumentException(
$"URL path {relativePath} must start with '/' and be of the form '/foo/bar/baz'"
);

and the samples show things like c.BaseAddress = new Uri("https://api.github.com") (missing /) and Get("/users/{user}") (leading /):

refit/README.md

Lines 15 to 35 in f77f0f3

```csharp
public interface IGitHubApi
{
[Get("/users/{user}")]
Task<User> GetUser(string user);
}
```
The `RestService` class generates an implementation of `IGitHubApi` that uses
`HttpClient` to make its calls:
```csharp
var gitHubApi = RestService.For<IGitHubApi>("https://api.github.com");
var octocat = await gitHubApi.GetUser("octocat");
```
.NET Core supports registering via HttpClientFactory
```csharp
services
.AddRefitClient<IGitHubApi>()
.ConfigureHttpClient(c => c.BaseAddress = new Uri("https://api.github.com"));
```

Expected behavior

However, the expected HttpClient usage seems to be this

  • the BaseAddress is expected to end with a /
  • relative URLs are expected to start without a /

Therefore, the expected situation is either:

  • Refit following how HttpClient is intended to be used, or
  • clear documentation that Refit is doing it different for a reason.

Sources

Step to reproduce

  1. Follow the Refit samples, but configure the BaseAddress as https://api.github.com/ and the GET path as users/{user}
  2. See an exception
  Message: 
    System.ArgumentException : URL path users/{user} must start with '/' and be of the form '/foo/bar/baz'

  Stack Trace: 
    RestMethodInfoInternal.VerifyUrlPathIsSane(String relativePath) line 259

Reproduction repository

https://github.com/reactiveui/refit

@hansmbakker hansmbakker added the bug label Sep 3, 2024
@hansmbakker hansmbakker changed the title [Bug]: BaseAddress / relative URL formats in documentation [Bug]: BaseAddress / relative URL formats - HttpClient style vs Refit style Sep 3, 2024
@Dreamescaper
Copy link
Contributor

if you configure the base address with a trailing slash while also configuring the relative urls with a leading slash, it will result in a double // which results in a 404 error at runtime.

That's actually not true, there would be no double slashes. On the contrary - it will remove part of the base url.

HttpClient actually is smart about combining baseUrl and relativeUrl, it's just not obvious if you don't know about it. But - it is based on RFC, and it matches other implementations, like javascript's URL.

That's said, I agree that it's strange that Refit doesn't allow URLs without leading slashes.
(I realise that changing the behavior when trailing slash is present would be a huge breaking change.)

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Sep 4, 2024

That's actually not true, there would be no double slashes. On the contrary - it will remove part of the base url.

That's not the behavior I saw - if I configure

  • the BaseAddress as https://somehostname/somepath/rest/v1/
  • Refit as [Get("/inspection_results")]

Then the URL that is called by Refit is https://somehostname/somepath/rest/v1//inspection_results.

I specifically configured the BaseAddress with the trailing slash because of the two pages I linked in the sources section in this issue.

(I realise that changing the behavior when trailing slash is present would be a huge breaking change.)

I agree - if this was documented better then that would already help a lot.

@Dreamescaper
Copy link
Contributor

Dreamescaper commented Sep 4, 2024

@hansmbakker
Yeah, I meant HttpClient's behavior, not Refit's.
HttpClient would invoke https://somehostname/inspection_results in this case:

https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYCMBYAUHgNwEMwACAIyIGcBTAVTABtSBeUqGgd1IYEsAKAEQALYMAAOVEAHppVAPYBbGsPlVgUIsrlKa4osGHSwNddIKZpggJQBuQiVInGB3gXpNW7LjzADKtAyMMKSC0rxQVOI0AMbAvPJQAPomVBCMwFQ29riomACc/M6u7kEAdAAq8gDKwH5QAOb81nZAA=

@hansmbakker
Copy link
Contributor Author

hansmbakker commented Sep 4, 2024

I just found some more issues related to this.

Looking at #1358 (which I now see my issue is a duplicate of), people mention that Refit is not RFC compliant because of this behavior.

A few years ago, changing this behavior was refused and using a custom delegating HttpHandler was recommended.

If this could be reconsidered, or if this could be documented better, it would be really helpful.

Related issues:

@aj-scram
Copy link

aj-scram commented Sep 26, 2024

lol I just was tearing my hair out yesterday about HttpClient not working correctly at it's cause of it needing the ending '/'

Honestly, I prefer Refits way though as it makes more intuitive sense to me. Additionally, at least Refit immediatly throws an error telling you the problem.

HttpClient will work with GetAsJsonAsync using the wrong way and lead you on a merry chase on why POST is failing. Plus, if you use HttpClientExtendedLogging ? Well, that will also cause POST to work cause of how it manipulated the request uri internally I guess

RFC compliance is not needed; clear errors and docs are, so I would say MSFT is the one that needs to step up here, personally. Though, I do think that the fact that Refit uses the HttpClientBuilder can cause confusion between the two, so it would be nice for consistency if it was supported

Just my two cents as a dev that was spending way too much time on this yesterday :P

@StevenQuickGS1
Copy link

StevenQuickGS1 commented Dec 8, 2024

I guess it would be nice if Refit trimmed the trailing slash automatically when joining to the routes, but I just do this myself when using typed http client for refit

services.AddRefitClient<IMyApiClient>()
  .ConfigureHttpClient((services, client) =>
  {
    var options = services.GetRequiredService<MyApiOptions>();
    // remove trailing slash as the refit routes are prefixed with a slash
    client.BaseAddress = new Uri(options.BaseUri.TrimEnd('/'));
    client.DefaultRequestHeaders.Add(options.ApiKeyHeader, options.ApiKey);
});
public interface IMyApiClient
  {
    [Get("/something/{id}")]
    Task<Entity> GetSomething(string id);

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

No branches or pull requests

4 participants