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

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

[codecov[bot]]

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.

:exclamation: 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 :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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 😉

[ChrisPulman]

Sorry to be the bearer of bad news but the possibility for confusion is too high to accept this PR at this time. After looking at the pros and cons of the proposal I feel that there is too much room for inducing issues that cannot be traced easily. Although in smaller solutions this may not be such an issue, once scaled up these become high risk. Thank you for your suggestions, but at this time we will not be adding this.

[github-actions[bot]]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.