Property provider factory

[james-s-tayler]

What kind of change does this PR introduce? This is a new feature that extends/enhances the work done around the [Property] attribute. It adds the notion of a PropertyProvider which allows adding properties into HttpRequestMessage.Properties on every request without explicitly having to pass in a [Property] attribute but instead using information derived from the MethodInfo of the currently executing method on the Refit instance, as well as the Type of the Refit target interface.

PropertyProviderFactory allows you to build a List<PropertyProvider> to populate RefitSettings.PropertyProviders and comes with built-in support for common scenarios such as adding the refit target interface type, the method info, and any custom attributes into the HttpRequestMessage.Properties.

Users can write their own PropertyProvider implementations by simply implementing the PropertyProvider interface and passing into the PropertyProviderFactory when configuring RefitSettings.PropertyProviders.

What is the current behavior? The current behavior is you can only pass [Property] attributes as part of the method signature.

Community feedback on the [Property] feature seems to indicate the ability to pass properties is something useful, but the current functionality doesn't quite support some great use cases.

See https://github.com/reactiveui/refit/issues/1156 and https://github.com/reactiveui/refit/issues/1150

What is the new behavior? Users can leverage the new PropertyProvider implementation as well as write their own in order to extend the functionality of Refit via passing state to HttpClient middleware.

What might this PR break? It touches a core part of HttpRequestBuilderImplementation as such it has been programmed defensively, so as not to blow up if anything goes wrong with a user provided implementation of PropertyProvider. It has a fairly extensive set of tests that include this scenario too to ensure the request can still succeed.

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)

Other information: This PR introduces a new namespace Refit.Extensions to aide in discoverability. It also shifts the existing ExceptionFactory stuff into that namespace. I hope that's not too invasive. If there's any changes you'd rather see backed out in that area, just let me know.

[clairernovotny]

This is a big PR and will need some time to review.

[clairernovotny]

One initial thought here is that this seems quite complicated. Why do we need more than the method name as per #1150?

[james-s-tayler]

It's so that consumers of the library can grow its functionality to support their use cases without being blocked on anyone.

Also means Refit itself doesn't have to have too strong am opinion of its own on what winds up in the request properties.

And it's done in a way that's composible such that consumers of the library don't have to accept or reject the default opinion, but can instead choose to augment it, or replace it.

Personally, I myself don't have a need for the method info, but can definitely see use cases others might have around that. I'm most excited about and in need of flowing data from custom attributes into the request properties as that opens a very powerful extension point that allows for pretty much anything. Which is what #1156 is about.

[anaisbetts]

Hey @james-s-tayler, while it's super clear that you've done a ton of work on this and definitely crossed the i's and dotted the t's, I think the design of this change is Too Complicated. It has breaking changes (changing the base class of an exception, moving types to other namespaces), and overall it introduces too much complexity / new Concepts.

I think that we should start instead with directly implementing this comment from #1150:

Include the path in the Get/Post/Etc attribute ("/path/{path}/{id}") and/or the Method name in HttpRequestMessage.Properties. This makes it easier to add things like metrics and telemetry via DelegatingHandler without having to parse out the dynamic parts of the url.

Let's do this First, then see what scenarios are still painful to do and think about how to solve them. Parsing string URLs might not be as Elegant as a fancy MethodInfo and Provider setup but like, nearly everyone given the URL and Method will be like "I understand what to do next to Solve My Business Problem (even if it's writing a bunch of ifs and Regexes)".

[lostincomputer]

My business problem is I want to be able to configure Polly without needing to add an argument to the method. For example:

interface IApiClient
{
   [MaxRetry(3)]
   string DoSomething();

   [MaxRetry(1)]
   string DoAnotherThing();
}

[james-s-tayler]

@anaisbetts The design is targeted more at solving #1156. I've had plenty of uses for this in the past. @lostincomputer mentions one particular case, but there are others...

Perhaps you have a versioned API and want to be able to do something like:

interface IApiClient
{
   [Version(1)]
   [Get("/Something")]
   Task<string> GetSomethingV1(sring id);

   [Version(2)]
   [Get("/Something")]
   Task<string> GetSomethingV2(string id);
}

Or maybe you simply want a declarative way to output messages under certain conditions on certain endpoints without littering the code base with a lot of custom logic at each of the callsites like:

interface IApiClient
{
   [SuccessMessage("Yay - we did the thing!")]
   [ErrorMessage("Oops - looks like we couldn't do the thing :(")]
   [Post("/Something")]
   Task<string> DoSomething([Body] object something);

   [SuccessMessage("Yay - we did the other thing!")]
   [ErrorMessage("Oops - looks like we couldn't do the other thing :(")]
   [Put("/Something")]
   Task<string> DoTheOtherThing([Body] object somethingEle);
}

Could probably support https://github.com/reactiveui/refit/issues/163 too by something like

[SomeData("123")]
interface IApiClient
{
   [Post("/Something")]
   Task<string> DoSomething([Body] object something);

   [Put("/Something")]
   Task<string> DoTheOtherThing([Body] object somethingEle);
}

These particular use cases aren't something Refit would likely want to directly support, but this makes it extensible such that the developer can solve whatever problem they have without any specific details of that having to leak into the Refit codebase itself.

I feel like solving just #1150 doesn't help with that while at the same time making Refit more opinionated about what winds up in the request properties. Doing so may have unintended consequences as we have no knowledge of what DelegatingHandlers are present and what they do. It's better if the developer is in control of what winds up in there. It's ok for Refit to have some default opinion, but quite a bit nicer if it is optional.

I've read through several hundred issues raised on this repo and one of the overall themes that seems to repeat is certain parts of Refit not being extensible enough and users getting blocked. Over the years certain points of extensibility have been added and each time Refit gets a bit more useful. When I saw there was already a HttpMessageHandlerFactory and an ExceptionFactory it made me feet that a PropertyProviderFactory was in line with idioms already present in the library. I think these points of extensibility make a lot of sense. They allow Refit to be grown by its users, and they allow whole classes of problems to be solved at once.

In terms of breaking changes... Yeah I wasn't 100% sure about that. I think it's just shifting DefaultApiExceptionFactory into its own class and a different namespace. I can certainly update the PR and undo that particular change. Though I was brave enough to try it since its is the default ExceptionFactory configured internally by RefitSettings it's probably not actually a breaking change to any consumers of the library, as no one would/should actually have any need to be referencing it directly, and was considering to do some work on https://github.com/reactiveui/refit/issues/1168 to perhaps provide an implementation of ExceptionFactory out of the box that could live in that namespace that makes it easy to suppress errors in certain status codes so users don't wind up in a situation like https://github.com/reactiveui/refit/issues/1153. I'm not married to that particular idea though, could be a better idea to finish this PR. But... up to you. Just let me know. Happy to back that particular part out!

Anyway, sorry for the essay, appreciate the review and would love to hear if you have any alternative suggestions on a design that can meet the needs I've outlined here. I hope I can convince you this design, though seemingly complicated, does have its merits!

[Misiu]

This would be awesome. I have multiple interfaces, all hitting the same endpoint, but with a different suffix. My API URLs: https://example.com/api/claims, https://example.com/api/users

    public interface IClaimsApi
    {
        [Get("claims/")]
        Task<IEnumerable<Claim>> GetAll();

        [Get("claims/{claimId}")]
        Task<ApiResponse<Claim>> Get(int claimId);
    }

    public interface IUsersApi
    {
        [Get("users/")]
        Task<IEnumerable<User>> GetAll();

        [Get("users/{userId}")]
        Task<ApiResponse<User>> Get(int userId);
    }

This could be easily simplified to this:

    [Prefix("claims") ]
    public interface IClaimsApi
    {
        [Get("/")]
        Task<IEnumerable<Claim>> GetAll();

        [Get("/{claimId}")]
        Task<ApiResponse<Claim>> Get(int claimId);
    }

    [Prefix("users") ]
    public interface IUsersApi
    {
        [Get("/")]
        Task<IEnumerable<User>> GetAll();

        [Get("/{userId}")]
        Task<ApiResponse<User>> Get(int userId);
    }

Example Domain

Example Domain

[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.