Modify interface to support HttpClientFactory users #494

thecontrarycat · 2018-06-02T18:41:07Z

PR for #491

…used in an HttpFactory approach while maintaining the advantages of the cache.

clairernovotny · 2018-06-03T14:31:49Z

InterfaceStubGenerator.Core/GeneratedInterfaceStubTemplate.mustache

@@ -63,7 +62,7 @@ namespace {{Namespace}}
        {
 {{#IsRefitMethod}}    
            var arguments = new object[] { {{ArgumentList}} };
-            var func = methodImpls.GetOrAdd({{#MethodTypeParameterNames}}${{/MethodTypeParameterNames}}"{{Name}}{{#MethodTypeParameterNames}}<{{.}}>{{/MethodTypeParameterNames}}({{ArgumentListWithTypes}})", _ => requestBuilder.BuildRestResultFuncForMethod("{{Name}}", new Type[] { {{ArgumentTypesList}} }{{#MethodTypeParameterList}}, new Type[] { {{.}} }{{/MethodTypeParameterList}}));
+            var func = requestBuilder.BuildRestResultFuncForMethod("{{Name}}", new Type[] { {{ArgumentTypesList}} }{{#MethodTypeParameterList}}, new Type[] { {{.}} }{{/MethodTypeParameterList}});


We can't do this....this means we're running potentially expensive reflection code on every method invocation and not caching the result.

I'd need to see a benchmark comparing the two before removing this cache.

clairernovotny · 2018-06-03T14:34:48Z

I may have spoken a little too soon in my code review comment as I was going top-down and didn't see the new cache.

Given the structural changes, can you please describe what you changed and how it's working & caching things?

thecontrarycat · 2018-06-04T08:23:10Z

Basically, I've made the following changes:

Remove the caching layer from the generated code.
Introduced CachedRequestBuilderImplementation which is a decorator for IRequestBuilder and contains the caching layer that used to be in the generated code.
Changed RequestBuilderFactory so that RequestBuilder.ForType<T>() returns a CachedRequestBuilderImplementation that wraps a RequestBuilderImplementation.
Added an overload to RestService.For<T> that takes an HttpClient and an IRequestBuilder, allowing callers to hold the IRequestBuilder as a singleton and have transient HttpClient and Generated Code instances. This supports the HttpClientFactory approach.
(Bonus) Cache the code in RestService.cs that resolves the interface type to generated code type, as in our scenario this code will be called multiple times and it's easy to cache it.

bennor

This looks good to me other than a few minor readability things.

bennor · 2018-06-07T00:23:12Z

Refit/CachedRequestBuilderImplementation.cs

+                return "";
+            }
+
+            return parameterTypes.Select(t => t.Name).Aggregate((s1, s2) => s1 + ", " + s2);


string.Join(", ", parameterTypes.Select(t => t.Name) would be a little clearer here. There's another one like this in the next method too.

clairernovotny · 2018-06-07T12:16:22Z

Refit/RequestBuilderImplementation.cs

@@ -27,7 +27,7 @@ partial class RequestBuilderImplementation : IRequestBuilder
        readonly ConcurrentDictionary<CloseGenericMethodKey, RestMethodInfo> interfaceGenericHttpMethods;
        readonly JsonSerializer serializer;
        readonly RefitSettings settings;
-        readonly Type targetType;
+        public Type targetType { get; }


Since this is a property, change to PascalCase

Good spot. Fixing now

clairernovotny · 2018-06-13T10:54:53Z

Haven't forgotten about this. Will try to get this reviewed and merged in the next few days.

clairernovotny · 2018-06-15T18:16:44Z

@thecontrarycat This looks good, thanks! One thing that would be helpful though is a doc update that shows how to use this with IHttpClientFactory. Can you please add that and I'll complete the merge?

thecontrarycat · 2018-06-18T13:30:50Z

@onovotny I've added a note to the documentation showing how you can use it with HttpClientFactory

clairernovotny · 2018-06-18T13:35:13Z

@thecontrarycat Thanks, that makes sense.

In light of the pattern in the docs, what would we need to expose a single extension method (or two with overloads) to add Refit? Something like .AddRefit<T>(...) with the settings, or whatever?

thecontrarycat · 2018-06-18T15:59:12Z

I think it would be complicated to do a single IServiceCollection.AddRefit<T>() method because we'd need to implement the whole HttpClient configuration part (there are additional lines you can put between the AddHttpClient and AddTypedClient methods to customise the handlers used by the client).

I did start thinking that we could make IRequestHandler become IRequestHandler<T> and we could then replace the AddTypedClient call with an AddRefitClient<T> call where we could singleton register the appropriate IRequestHandler<T> in the same call.

Refactoring the IRequestHandler was a larger job than I wanted to do unless you think it makes sense.

Alternatively, we could static cache the IRequestHandler in RestService and not expose it publicly at all... (but I don't much care for static caches).

…alents, allowing us to provide a generic extension method for HttpClientFactory users.

thecontrarycat · 2018-06-19T09:44:11Z

I did that refactor. IRequestBuilder and IRequestBuilderInternal have become IRequestBuilder<T> and IRequestBuilder, with the generic version being used during creation. This allows us to have a single extension method to configure Refit in the HttpClientFactory world (see updated docs).

clairernovotny · 2018-06-19T12:58:49Z

Refit/HttpClientFactoryExtensions.cs

+{
+    public static class HttpClientFactoryExtensions
+    {
+        public static IHttpClientBuilder AddRefitClient<T>(this IServiceCollection services) where T : class


I think we need an overload that can specify the refit settings. Can either be as another parameter or as an Action<RefitSettings> where the method news it up and passes it to the user for setting?

clairernovotny · 2018-06-19T13:00:28Z

This looks really great, I love having an easy one-liner to add in the startup that "does the right thing"

thecontrarycat · 2018-06-19T13:38:17Z

Added the overload for RefitSettings. It's not an Action<RefitSettings> because the IRequestBuilder<T> will be a singleton and therefore there's no ability to change the value at runtime.

clairernovotny · 2018-06-20T16:41:44Z

Thank for this!

clairernovotny · 2018-06-20T21:10:39Z

@thecontrarycat I know I already merged it, but any chance you could add a test or two that covers what the extension method is supposed to do and any tests that cover the new behavior? (new PR)

Change the way we cache the generated method calls so that it can be …

bf8b0f9

…used in an HttpFactory approach while maintaining the advantages of the cache.

thecontrarycat mentioned this pull request Jun 2, 2018

Using Refit with ASP.Net Core 2.1's HttpClientFactory #491

Closed

Oren Novotny added 2 commits June 3, 2018 10:21

Merge branch 'master' into cachemethods

276103d

Merge branch 'master' into cachemethods

142a5f1

clairernovotny reviewed Jun 3, 2018

View reviewed changes

bennor suggested changes Jun 7, 2018

View reviewed changes

Readability changes

c61d569

bennor approved these changes Jun 7, 2018

View reviewed changes

clairernovotny reviewed Jun 7, 2018

View reviewed changes

Fix casing on TargetType property

83b8ce1

clairernovotny mentioned this pull request Jun 13, 2018

Customizing RefitStubs.g.cs generatation #499

Closed

Merge branch 'master' into cachemethods

af08f8c

Oren Novotny added 4 commits June 15, 2018 13:45

Remove unused namespaces

c69ffbd

Remove dead code

9444f53

Make a few types internal

f320392

Style

34f2930

Oren Novotny and others added 5 commits June 15, 2018 18:07

merge from master

e44ffc3

Merge branch 'master' into cachemethods

af1c33f

Update documentation to show usage with HttpClientFactory

3a09d2b

Further documentation of HttpClientFactory

36397b3

Fix typo in documentation

9f7a3ec

Refactor IRequestBuilder and IRequestBuilderInternal to generic equiv…

1dae8ed

…alents, allowing us to provide a generic extension method for HttpClientFactory users.

Fix typo in documentation

c4547d9

clairernovotny reviewed Jun 19, 2018

View reviewed changes

Add optional RefitSettings to HttpClientFactory integration

6f53806

clairernovotny merged commit d097564 into reactiveui:master Jun 20, 2018

lock bot locked and limited conversation to collaborators Jun 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify interface to support HttpClientFactory users #494

Modify interface to support HttpClientFactory users #494

thecontrarycat commented Jun 2, 2018

clairernovotny Jun 3, 2018

clairernovotny commented Jun 3, 2018

thecontrarycat commented Jun 4, 2018

bennor left a comment

bennor Jun 7, 2018

clairernovotny Jun 7, 2018

thecontrarycat Jun 7, 2018

clairernovotny commented Jun 13, 2018

clairernovotny commented Jun 15, 2018

thecontrarycat commented Jun 18, 2018

clairernovotny commented Jun 18, 2018

thecontrarycat commented Jun 18, 2018 •

edited

thecontrarycat commented Jun 19, 2018

clairernovotny Jun 19, 2018

clairernovotny commented Jun 19, 2018

thecontrarycat commented Jun 19, 2018

clairernovotny commented Jun 20, 2018

clairernovotny commented Jun 20, 2018 •

edited

Modify interface to support HttpClientFactory users #494

Modify interface to support HttpClientFactory users #494

Conversation

thecontrarycat commented Jun 2, 2018

clairernovotny Jun 3, 2018

Choose a reason for hiding this comment

clairernovotny commented Jun 3, 2018

thecontrarycat commented Jun 4, 2018

bennor left a comment

Choose a reason for hiding this comment

bennor Jun 7, 2018

Choose a reason for hiding this comment

clairernovotny Jun 7, 2018

Choose a reason for hiding this comment

thecontrarycat Jun 7, 2018

Choose a reason for hiding this comment

clairernovotny commented Jun 13, 2018

clairernovotny commented Jun 15, 2018

thecontrarycat commented Jun 18, 2018

clairernovotny commented Jun 18, 2018

thecontrarycat commented Jun 18, 2018 • edited

thecontrarycat commented Jun 19, 2018

clairernovotny Jun 19, 2018

Choose a reason for hiding this comment

clairernovotny commented Jun 19, 2018

thecontrarycat commented Jun 19, 2018

clairernovotny commented Jun 20, 2018

clairernovotny commented Jun 20, 2018 • edited

thecontrarycat commented Jun 18, 2018 •

edited

clairernovotny commented Jun 20, 2018 •

edited