feat: add new PathPrefix attribute

[JesseKlaasse]

What kind of change does this PR introduce?
When a group of methods share the same relative URL path prefix, you can use the PathPrefix annotation. When given, the path prefix will be concatenated after the base URL and before the relative path as specified in the method's annotation.

What is the current behavior?
This is an added feature, so the current behavior is that this is not possible. At the moment, the constructed URL is always a concatenation of the base URL and the method specific relative path.

What is the new behavior?
You now have the possibility to define a PathPrefix on interface level.

What might this PR break?
URL construction in RestMethodInfoInternal.RelativePath, although it should and does not break. When a PathPrefix is omitted, it simply will not be used.

Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

[thomaslevesque]

Is concatenation enough? I think it should also add a / separator if necessary.

[JesseKlaasse]

I don't think Refit should try to be intelligent by adding slashes. I think it's the responsibility of the implementing user to annotate correctly. There could be situations where you actually want to concatenate instead of an automatically added slash. Don't you think?

[thomaslevesque]

There could be situations where you actually want to concatenate instead of an automatically added slash.

To be honest, I can't really think of any situation where I would want to concatenate without the slash. Unless you want to have a prefix like "i" and suffixes like "tem" and "nventory", but that seems like a bad idea to me...

I think Refit should "do the right thing" to make things easier for the developer. But I guess the right answer is to check what Refit does elsewhere, and do the same...

[JesseKlaasse]

@thomaslevesque Agreed, on both points. I will take a look into that.

[JesseKlaasse]

https://github.com/reactiveui/refit/blob/main/Refit/RequestBuilderImplementation.cs#L639-L640

It seems like Refit is also just concatenating the basepath and the relative path (unless the basepath is empty). I agree that this could be more foolproof, however, it seems like a separate issue to me.

[thomaslevesque]

Agreed. Let's leave it like this for now.

[codecov]

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 87.72%. Comparing base (6ebeda5) to head (2e87757).
Report is 21 commits behind head on main.

❗ Current head 2e87757 differs from pull request most recent head a7ead77

Please upload reports for the commit a7ead77 to get more accurate results.

Files	Patch %	Lines
Refit/ReflectionHelpers.cs	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1685      +/-   ##
==========================================
- Coverage   87.73%   87.72%   -0.02%     
==========================================
  Files          33       34       +1     
  Lines        2348     2362      +14     
  Branches      294      297       +3     
==========================================
+ Hits         2060     2072      +12     
- Misses        208      209       +1     
- Partials       80       81       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anaisbetts]

I appreciate why one might want this feature but it doesn't feel Great to me, it makes it really easy to misread what the actual URL is and sets developers up for being confused. Consider your example:

[PathPrefix("/resources")]
public interface IResourcesService
{
    [Get("")]
    Task<List<Resource>> ReadAll();

    // ***** Imagine pages and pages of text here, also imagine that you 
    // were not the programmer that put in the PathPrefix but a coworker 
    // did, and maybe you don't even know what this feature is or does *****

    [Get("/{key}")]
    Task<Resource> ReadOne(TKey key);
}

It makes it really easy to misread the ReadOne method and think it's a URL for /{key}, then spend hours banging your head against a wall only to find that Refit set a troll up on you and that it's not really /{key} at all

[JesseKlaasse]

I appreciate why one might want this feature but it doesn't feel Great to me, it makes it really easy to misread what the actual URL is and sets developers up for being confused. Consider your example:

[PathPrefix("/resources")]
public interface IResourcesService
{
    [Get("")]
    Task<List<Resource>> ReadAll();

    // ***** Imagine pages and pages of text here, also imagine that you 
    // were not the programmer that put in the PathPrefix but a coworker 
    // did, and maybe you don't even know what this feature is or does *****

    [Get("/{key}")]
    Task<Resource> ReadOne(TKey key);
}

It makes it really easy to misread the ReadOne method and think it's a URL for /{key}, then spend hours banging your head against a wall only to find that Refit set a troll up on you and that it's not really /{key} at all

Thank you for your input. Personally, I don't see why this would make interpreting the correct URL any harder than it is now. Now, you have to concat two strings (base URL and the endpoint path), and this way, you would have to concat three string instead.

A big advantage of the new annotation would be something like this (think of an API which uses the same endpoints for a lot of exposed entity types):

[UrlPrefix("/contacts")]
public interface IContactsClient<TEntity> : IPagedCollectionClient<TEntity>
    where TEntity : Contact, new()
{
}

[UrlPrefix("/companies")]
public interface ICompaniesClient<TEntity> : IPagedCollectionClient<TEntity>
    where TEntity : Company, new()
{
}

public interface IPagedCollectionClient<TEntity>
    where TEntity : new()
{
    [Get("")]
    Task<ApiResponse<PagedCollectionResult<ResultObject<TEntity>>>> GetPage(GetPageRequest? listParameters = default, CancellationToken cancellationToken = default);

    [Get("/{id}")]
    Task<ApiResponse<ResultObject<TEntity>>> Get(string id, GetRequest? readParameters = default, CancellationToken cancellationToken = default);

    [Post("")]
    Task<ApiResponse<ResultObject<TEntity>>> Post([Body] PostObject<TEntity> post, CancellationToken cancellationToken = default);

    [Patch("/{id}")]
    Task<ApiResponse<ResultObject<TEntity>>> Patch(string id, [Body] PostObject<TEntity> obj, CancellationToken cancellationToken = default);

    [Delete("/{id}")]
    Task<IApiResponse> Delete(string id, CancellationToken cancellationToken = default);
}

Not having the attribute, this would not be possible without defining the (same) endpoints over and over again. At least without defining a separate Refit client per entity.

[anaisbetts]

A big advantage of the new annotation would be something like this

Again while I see why someone might want to do that, that example has even more potential for trolling developers, you might not even see the annotation at all! Your code is easy to see in your example, but it wouldn't be if e.g. the base class was in another file, or even worse in another assembly (especially if it was binary-only).

Instead, this appears to work - not as pretty, but it makes it much explicit for others reading your code later:

    public interface IResourcesService
    {
        const string prefix = "/foo/bar/baz/bamf";

        [Get(prefix + "")]
        Task<List<Resource>> ReadAll();

        [Get(prefix + "/{key}")]
        Task<Resource> ReadOne(string key);
    }

[thomaslevesque]

@anaisbetts if avoiding to repeat the prefix was the only benefit of this change, I would agree that it's probably not worth it, since you can just introduce the prefix via a constant. However, if you look at the issue where I first mentioned the idea, another benefit would be to be able to pass values for placeholders in the prefix to RestService.For, so that it doesn't need to be passed explicitly to all methods:

[Prefix("/api/tenant/{tenantKey}/item"]
interface IItemsClient
{
    [Get("/{itemId}")]
    Task<FileSystemItem> GetItemAsync(string itemId, CancellationToken cancellationToken = default);

    [Get("/{parentId}/children")]
    Task<CollectionResponse<FileSystemItem>> GetChildrenAsync(string parentId, CancellationToken cancellationToken = default);

    ...
}

var itemsClient = RestService.For<IItemsClient>(httpClient, refitSettings, new { tenantKey = "foo" });

In this example, I don't need to add a string tenantKey parameter to each method, it's specified when creating the client. To me, that's the main benefit of this approach. However, this PR doesn't cover that part of my idea...

[JesseKlaasse]

@thomaslevesque Interesting. Since this didn't come up in my real-world use case, I hadn't thought of that. I will take a look into how that would be implemented.

[thomaslevesque]

I will take a look into how that would be implemented.

Maybe don't, for now, unless @anaisbetts changes her mind 😉