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
feat: add new PathPrefix attribute #1685
base: main
Are you sure you want to change the base?
feat: add new PathPrefix attribute #1685
Conversation
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.
@@ -68,7 +68,7 @@ internal class RestMethodInfoInternal | |||
var hma = methodInfo.GetCustomAttributes(true).OfType<HttpMethodAttribute>().First(); | |||
|
|||
HttpMethod = hma.Method; | |||
RelativePath = hma.Path; | |||
RelativePath = ReflectionHelpers.GetPathPrefixFor(targetInterface) + hma.Path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is concatenation enough? I think it should also add a /
separator if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaslevesque Agreed, on both points. I will take a look into that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Let's leave it like this for now.
Codecov ReportAttention: Patch coverage is
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. |
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 |
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. |
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);
} |
@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 [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 |
@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. |
Maybe don't, for now, unless @anaisbetts changes her mind 😉 |
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