From 107d71697d0d41ecdec296634cfb303c96db4c0e Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 25 Jun 2024 19:16:49 +0400 Subject: [PATCH 1/3] chore(deps): update dependency microsoft.codeanalysis.csharp.sourcegenerators.testing to v1.1.2-beta1.24314.1 (#1736) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- Refit.Tests/Refit.Tests.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Refit.Tests/Refit.Tests.csproj b/Refit.Tests/Refit.Tests.csproj index 3dbe07664..d9b22ac5e 100644 --- a/Refit.Tests/Refit.Tests.csproj +++ b/Refit.Tests/Refit.Tests.csproj @@ -21,7 +21,7 @@ - + From b320e4eb4e9049210fe7655e0c6f9fda6b1c8e77 Mon Sep 17 00:00:00 2001 From: Tim M <49349513+TimothyMakkison@users.noreply.github.com> Date: Tue, 25 Jun 2024 19:26:43 +0100 Subject: [PATCH 2/3] feat: optimize `CachedRequestBuilder` (#1716) Co-authored-by: Chris Pulman --- Refit.Tests/CachedRequestBuilder.cs | 148 ++++++++++++++++++++ Refit/CachedRequestBuilderImplementation.cs | 108 ++++++++++---- 2 files changed, 232 insertions(+), 24 deletions(-) create mode 100644 Refit.Tests/CachedRequestBuilder.cs diff --git a/Refit.Tests/CachedRequestBuilder.cs b/Refit.Tests/CachedRequestBuilder.cs new file mode 100644 index 000000000..e9b88c8dd --- /dev/null +++ b/Refit.Tests/CachedRequestBuilder.cs @@ -0,0 +1,148 @@ +using System.Net; +using System.Net.Http; +using System.Reflection; + +using RichardSzalay.MockHttp; + +using Xunit; + +namespace Refit.Tests; + +public interface IGeneralRequests +{ + [Post("/foo")] + Task Empty(); + + [Post("/foo")] + Task SingleParameter(string id); + + [Post("/foo")] + Task MultiParameter(string id, string name); + + [Post("/foo")] + Task SingleGenericMultiParameter(string id, string name, TValue generic); +} + +public interface IDuplicateNames +{ + [Post("/foo")] + Task SingleParameter(string id); + + [Post("/foo")] + Task SingleParameter(int id); +} + +public class CachedRequestBuilderTests +{ + [Fact] + public async Task CacheHasCorrectNumberOfElementsTest() + { + var mockHttp = new MockHttpMessageHandler(); + var settings = new RefitSettings { HttpMessageHandlerFactory = () => mockHttp }; + + var fixture = RestService.For("http://bar", settings); + + // get internal dictionary to check count + var requestBuilderField = fixture.GetType().GetFields(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public).Single(x => x.Name == "requestBuilder"); + var requestBuilder = requestBuilderField.GetValue(fixture) as CachedRequestBuilderImplementation; + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .Respond(HttpStatusCode.OK); + await fixture.Empty(); + Assert.Single(requestBuilder.MethodDictionary); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter("id"); + Assert.Equal(2, requestBuilder.MethodDictionary.Count); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .WithQueryString("name", "name") + .Respond(HttpStatusCode.OK); + await fixture.MultiParameter("id", "name"); + Assert.Equal(3, requestBuilder.MethodDictionary.Count); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .WithQueryString("name", "name") + .WithQueryString("generic", "generic") + .Respond(HttpStatusCode.OK); + await fixture.SingleGenericMultiParameter("id", "name", "generic"); + Assert.Equal(4, requestBuilder.MethodDictionary.Count); + + mockHttp.VerifyNoOutstandingExpectation(); + } + + [Fact] + public async Task NoDuplicateEntriesTest() + { + var mockHttp = new MockHttpMessageHandler(); + var settings = new RefitSettings { HttpMessageHandlerFactory = () => mockHttp }; + + var fixture = RestService.For("http://bar", settings); + + // get internal dictionary to check count + var requestBuilderField = fixture.GetType().GetFields(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public).Single(x => x.Name == "requestBuilder"); + var requestBuilder = requestBuilderField.GetValue(fixture) as CachedRequestBuilderImplementation; + + // send the same request repeatedly to ensure that multiple dictionary entries are not created + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter("id"); + Assert.Single(requestBuilder.MethodDictionary); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter("id"); + Assert.Single(requestBuilder.MethodDictionary); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter("id"); + Assert.Single(requestBuilder.MethodDictionary); + + mockHttp.VerifyNoOutstandingExpectation(); + } + + [Fact] + public async Task SameNameDuplicateEntriesTest() + { + var mockHttp = new MockHttpMessageHandler(); + var settings = new RefitSettings { HttpMessageHandlerFactory = () => mockHttp }; + + var fixture = RestService.For("http://bar", settings); + + // get internal dictionary to check count + var requestBuilderField = fixture.GetType().GetFields(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public).Single(x => x.Name == "requestBuilder"); + var requestBuilder = requestBuilderField.GetValue(fixture) as CachedRequestBuilderImplementation; + + // send the two different requests with the same name + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "id") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter("id"); + Assert.Single(requestBuilder.MethodDictionary); + + mockHttp + .Expect(HttpMethod.Post, "http://bar/foo") + .WithQueryString("id", "10") + .Respond(HttpStatusCode.OK); + await fixture.SingleParameter(10); + Assert.Equal(2, requestBuilder.MethodDictionary.Count); + + mockHttp.VerifyNoOutstandingExpectation(); + } +} diff --git a/Refit/CachedRequestBuilderImplementation.cs b/Refit/CachedRequestBuilderImplementation.cs index 951b08d9e..1a0908330 100644 --- a/Refit/CachedRequestBuilderImplementation.cs +++ b/Refit/CachedRequestBuilderImplementation.cs @@ -20,10 +20,10 @@ public CachedRequestBuilderImplementation(IRequestBuilder innerBuilder) } readonly IRequestBuilder innerBuilder; - readonly ConcurrentDictionary< - string, + internal readonly ConcurrentDictionary< + MethodTableKey, Func - > methodDictionary = new(); + > MethodDictionary = new(); public Func BuildRestResultFuncForMethod( string methodName, @@ -31,13 +31,22 @@ readonly ConcurrentDictionary< Type[]? genericArgumentTypes = null ) { - var cacheKey = GetCacheKey( + var cacheKey = new MethodTableKey( methodName, parameterTypes ?? Array.Empty(), genericArgumentTypes ?? Array.Empty() ); - var func = methodDictionary.GetOrAdd( - cacheKey, + + if (MethodDictionary.TryGetValue(cacheKey, out var methodFunc)) + { + return methodFunc; + } + + // use GetOrAdd with cloned array method table key. This prevents the array from being modified, breaking the dictionary. + var func = MethodDictionary.GetOrAdd( + new MethodTableKey(methodName, + parameterTypes?.ToArray() ?? Array.Empty(), + genericArgumentTypes?.ToArray() ?? Array.Empty()), _ => innerBuilder.BuildRestResultFuncForMethod( methodName, @@ -48,37 +57,88 @@ readonly ConcurrentDictionary< return func; } + } - static string GetCacheKey( - string methodName, - Type[] parameterTypes, - Type[] genericArgumentTypes - ) + /// + /// Represents a method composed of its name, generic arguments and parameters. + /// + internal readonly struct MethodTableKey : IEquatable + { + /// + /// Constructs an instance of . + /// + /// Represents the methods name. + /// Array containing the methods parameters. + /// Array containing the methods generic arguments. + public MethodTableKey (string methodName, Type[] parameters, Type[] genericArguments) { - var genericDefinition = GetGenericString(genericArgumentTypes); - var argumentString = GetArgumentString(parameterTypes); - - return $"{methodName}{genericDefinition}({argumentString})"; + MethodName = methodName; + Parameters = parameters; + GenericArguments = genericArguments; } - static string GetArgumentString(Type[] parameterTypes) + /// + /// The methods name. + /// + string MethodName { get; } + + /// + /// Array containing the methods parameters. + /// + Type[] Parameters { get; } + + /// + /// Array containing the methods generic arguments. + /// + Type[] GenericArguments { get; } + + public override int GetHashCode() { - if (parameterTypes == null || parameterTypes.Length == 0) + unchecked { - return ""; - } + var hashCode = MethodName.GetHashCode(); + + foreach (var argument in Parameters) + { + hashCode = (hashCode * 397) ^ argument.GetHashCode(); + } - return string.Join(", ", parameterTypes.Select(t => t.FullName)); + foreach (var genericArgument in GenericArguments) + { + hashCode = (hashCode * 397) ^ genericArgument.GetHashCode(); + } + return hashCode; + } } - static string GetGenericString(Type[] genericArgumentTypes) + public bool Equals(MethodTableKey other) { - if (genericArgumentTypes == null || genericArgumentTypes.Length == 0) + if (Parameters.Length != other.Parameters.Length + || GenericArguments.Length != other.GenericArguments.Length + || MethodName != other.MethodName) { - return ""; + return false; } - return "<" + string.Join(", ", genericArgumentTypes.Select(t => t.FullName)) + ">"; + for (var i = 0; i < Parameters.Length; i++) + { + if (Parameters[i] != other.Parameters[i]) + { + return false; + } + } + + for (var i = 0; i < GenericArguments.Length; i++) + { + if (GenericArguments[i] != other.GenericArguments[i]) + { + return false; + } + } + + return true; } + + public override bool Equals(object? obj) => obj is MethodTableKey other && Equals(other); } } From 03d7bbc7abae4e0e6440f635223cf6f6f64fb922 Mon Sep 17 00:00:00 2001 From: Tim M <49349513+TimothyMakkison@users.noreply.github.com> Date: Tue, 25 Jun 2024 19:36:50 +0100 Subject: [PATCH 3/3] feat: use `TryGetSingle` instead of collection enumerable to lists. (#1738) Co-authored-by: Chris Pulman --- Refit.Benchmarks/Program.cs | 2 +- Refit/EnumerableExtensions.cs | 27 +++++++ Refit/RestMethodInfo.cs | 136 +++++++++++++++++----------------- 3 files changed, 94 insertions(+), 71 deletions(-) create mode 100644 Refit/EnumerableExtensions.cs diff --git a/Refit.Benchmarks/Program.cs b/Refit.Benchmarks/Program.cs index 1383cae13..55f81ef15 100644 --- a/Refit.Benchmarks/Program.cs +++ b/Refit.Benchmarks/Program.cs @@ -9,5 +9,5 @@ { BenchmarkRunner.Run(); // BenchmarkRunner.Run(); - // BenchmarkRunner.Run(); + // BenchmarkRunner.Run(); } diff --git a/Refit/EnumerableExtensions.cs b/Refit/EnumerableExtensions.cs new file mode 100644 index 000000000..b4c131fb3 --- /dev/null +++ b/Refit/EnumerableExtensions.cs @@ -0,0 +1,27 @@ +namespace Refit; + +internal static class EnumerableExtensions +{ + internal static EnumerablePeek TryGetSingle(this IEnumerable enumerable, out T? value) + { + value = default(T); + using var enumerator = enumerable.GetEnumerator(); + var hasFirst = enumerator.MoveNext(); + if (!hasFirst) + return EnumerablePeek.Empty; + + value = enumerator.Current; + if (!enumerator.MoveNext()) + return EnumerablePeek.Single; + + value = default(T); + return EnumerablePeek.Many; + } +} + +internal enum EnumerablePeek +{ + Empty, + Single, + Many +} diff --git a/Refit/RestMethodInfo.cs b/Refit/RestMethodInfo.cs index 3716cb0df..2139fa8e3 100644 --- a/Refit/RestMethodInfo.cs +++ b/Refit/RestMethodInfo.cs @@ -85,29 +85,29 @@ public RestMethodInfoInternal( DetermineIfResponseMustBeDisposed(); // Exclude cancellation token parameters from this list - var parameterList = methodInfo + var parameterArray = methodInfo .GetParameters() - .Where(p => p.ParameterType != typeof(CancellationToken)) - .ToList(); - ParameterInfoMap = parameterList + .Where(static p => p.ParameterType != typeof(CancellationToken)) + .ToArray(); + ParameterInfoMap = parameterArray .Select((parameter, index) => new { index, parameter }) .ToDictionary(x => x.index, x => x.parameter); - ParameterMap = BuildParameterMap(RelativePath, parameterList); - BodyParameterInfo = FindBodyParameter(parameterList, IsMultipart, hma.Method); - AuthorizeParameterInfo = FindAuthorizationParameter(parameterList); + ParameterMap = BuildParameterMap(RelativePath, parameterArray); + BodyParameterInfo = FindBodyParameter(parameterArray, IsMultipart, hma.Method); + AuthorizeParameterInfo = FindAuthorizationParameter(parameterArray); Headers = ParseHeaders(methodInfo); - HeaderParameterMap = BuildHeaderParameterMap(parameterList); + HeaderParameterMap = BuildHeaderParameterMap(parameterArray); HeaderCollectionParameterMap = RestMethodInfoInternal.BuildHeaderCollectionParameterMap( - parameterList + parameterArray ); - PropertyParameterMap = BuildRequestPropertyMap(parameterList); + PropertyParameterMap = BuildRequestPropertyMap(parameterArray); // get names for multipart attachments AttachmentNameMap = []; if (IsMultipart) { - for (var i = 0; i < parameterList.Count; i++) + for (var i = 0; i < parameterArray.Length; i++) { if ( ParameterMap.ContainsKey(i) @@ -119,19 +119,19 @@ public RestMethodInfoInternal( continue; } - var attachmentName = GetAttachmentNameForParameter(parameterList[i]); + var attachmentName = GetAttachmentNameForParameter(parameterArray[i]); if (attachmentName == null) continue; AttachmentNameMap[i] = Tuple.Create( attachmentName, - GetUrlNameForParameter(parameterList[i]) + GetUrlNameForParameter(parameterArray[i]) ); } } QueryParameterMap = []; - for (var i = 0; i < parameterList.Count; i++) + for (var i = 0; i < parameterArray.Length; i++) { if ( ParameterMap.ContainsKey(i) @@ -145,21 +145,21 @@ public RestMethodInfoInternal( continue; } - QueryParameterMap.Add(i, GetUrlNameForParameter(parameterList[i])); + QueryParameterMap.Add(i, GetUrlNameForParameter(parameterArray[i])); } - var ctParams = methodInfo + var ctParamEnumerable = methodInfo .GetParameters() .Where(p => p.ParameterType == typeof(CancellationToken)) - .ToList(); - if (ctParams.Count > 1) + .TryGetSingle(out var ctParam); + if (ctParamEnumerable == EnumerablePeek.Many) { throw new ArgumentException( $"Argument list to method \"{methodInfo.Name}\" can only contain a single CancellationToken" ); } - CancellationToken = ctParams.FirstOrDefault(); + CancellationToken = ctParam; IsApiResponse = ReturnResultType!.GetTypeInfo().IsGenericType @@ -170,31 +170,30 @@ public RestMethodInfoInternal( || ReturnResultType == typeof(IApiResponse); } - static HashSet BuildHeaderCollectionParameterMap(List parameterList) + static HashSet BuildHeaderCollectionParameterMap(ParameterInfo[] parameterArray) { var headerCollectionMap = new HashSet(); - for (var i = 0; i < parameterList.Count; i++) + for (var i = 0; i < parameterArray.Length; i++) { - var param = parameterList[i]; + var param = parameterArray[i]; var headerCollection = param .GetCustomAttributes(true) .OfType() .FirstOrDefault(); - if (headerCollection != null) + if (headerCollection == null) continue; + + //opted for IDictionary semantics here as opposed to the looser IEnumerable> because IDictionary will enforce uniqueness of keys + if (param.ParameterType.IsAssignableFrom(typeof(IDictionary))) { - //opted for IDictionary semantics here as opposed to the looser IEnumerable> because IDictionary will enforce uniqueness of keys - if (param.ParameterType.IsAssignableFrom(typeof(IDictionary))) - { - headerCollectionMap.Add(i); - } - else - { - throw new ArgumentException( - $"HeaderCollection parameter of type {param.ParameterType.Name} is not assignable from IDictionary" - ); - } + headerCollectionMap.Add(i); + } + else + { + throw new ArgumentException( + $"HeaderCollection parameter of type {param.ParameterType.Name} is not assignable from IDictionary" + ); } } @@ -209,13 +208,13 @@ static HashSet BuildHeaderCollectionParameterMap(List parame public RestMethodInfo ToRestMethodInfo() => new(Name, Type, MethodInfo, RelativePath, ReturnType); - static Dictionary BuildRequestPropertyMap(List parameterList) + static Dictionary BuildRequestPropertyMap(ParameterInfo[] parameterArray) { var propertyMap = new Dictionary(); - for (var i = 0; i < parameterList.Count; i++) + for (var i = 0; i < parameterArray.Length; i++) { - var param = parameterList[i]; + var param = parameterArray[i]; var propertyAttribute = param .GetCustomAttributes(true) .OfType() @@ -233,12 +232,11 @@ static Dictionary BuildRequestPropertyMap(List param return propertyMap; } - static PropertyInfo[] GetParameterProperties(ParameterInfo parameter) + static IEnumerable GetParameterProperties(ParameterInfo parameter) { return parameter .ParameterType.GetProperties(BindingFlags.Public | BindingFlags.Instance) - .Where(p => p.CanRead && p.GetMethod?.IsPublic == true) - .ToArray(); + .Where(static p => p.CanRead && p.GetMethod?.IsPublic == true); } static void VerifyUrlPathIsSane(string relativePath) @@ -254,7 +252,7 @@ static void VerifyUrlPathIsSane(string relativePath) static Dictionary BuildParameterMap( string relativePath, - List parameterInfo + ParameterInfo[] parameterInfo ) { var ret = new Dictionary(); @@ -311,11 +309,11 @@ List parameterInfo }; #if NET6_0_OR_GREATER ret.TryAdd( - parameterInfo.IndexOf(restMethodParameterInfo.ParameterInfo), + Array.IndexOf(parameterInfo, restMethodParameterInfo.ParameterInfo), restMethodParameterInfo ); #else - var idx = parameterInfo.IndexOf(restMethodParameterInfo.ParameterInfo); + var idx = Array.IndexOf(parameterInfo, restMethodParameterInfo.ParameterInfo); if (!ret.ContainsKey(idx)) { ret.Add(idx, restMethodParameterInfo); @@ -329,7 +327,7 @@ List parameterInfo ) { var property = value1; - var parameterIndex = parameterInfo.IndexOf(property.Item1); + var parameterIndex = Array.IndexOf(parameterInfo, property.Item1); //If we already have this parameter, add additional ParameterProperty if (ret.TryGetValue(parameterIndex, out var value2)) { @@ -355,12 +353,12 @@ List parameterInfo ); #if NET6_0_OR_GREATER ret.TryAdd( - parameterInfo.IndexOf(restMethodParameterInfo.ParameterInfo), + Array.IndexOf(parameterInfo, restMethodParameterInfo.ParameterInfo), restMethodParameterInfo ); #else // Do the contains check - var idx = parameterInfo.IndexOf(restMethodParameterInfo.ParameterInfo); + var idx = Array.IndexOf(parameterInfo, restMethodParameterInfo.ParameterInfo); if (!ret.ContainsKey(idx)) { ret.Add(idx, restMethodParameterInfo); @@ -421,7 +419,7 @@ HttpMethod method // 2) POST/PUT/PATCH: Reference type other than string // 3) If there are two reference types other than string, without the body attribute, throw - var bodyParams = parameterList + var bodyParamEnumerable = parameterList .Select( x => new @@ -433,12 +431,12 @@ HttpMethod method } ) .Where(x => x.BodyAttribute != null) - .ToList(); + .TryGetSingle(out var bodyParam); // multipart requests may not contain a body, implicit or explicit if (isMultipart) { - if (bodyParams.Count > 0) + if (bodyParamEnumerable != EnumerablePeek.Empty) { throw new ArgumentException( "Multipart requests may not contain a Body parameter" @@ -447,19 +445,18 @@ HttpMethod method return null; } - if (bodyParams.Count > 1) + if (bodyParamEnumerable == EnumerablePeek.Many) { throw new ArgumentException("Only one parameter can be a Body parameter"); } // #1, body attribute wins - if (bodyParams.Count == 1) + if (bodyParamEnumerable == EnumerablePeek.Single) { - var ret = bodyParams[0]; return Tuple.Create( - ret.BodyAttribute!.SerializationMethod, - ret.BodyAttribute.Buffered ?? RefitSettings.Buffered, - parameterList.IndexOf(ret.Parameter) + bodyParam!.BodyAttribute!.SerializationMethod, + bodyParam.BodyAttribute.Buffered ?? RefitSettings.Buffered, + parameterList.IndexOf(bodyParam.Parameter) ); } @@ -475,7 +472,7 @@ HttpMethod method // see if we're a post/put/patch // explicitly skip [Query], [HeaderCollection], and [Property]-denoted params - var refParams = parameterList + var refParamEnumerable = parameterList .Where( pi => !pi.ParameterType.GetTypeInfo().IsValueType @@ -484,22 +481,22 @@ HttpMethod method && pi.GetCustomAttribute() == null && pi.GetCustomAttribute() == null ) - .ToList(); + .TryGetSingle(out var refParam); // Check for rule #3 - if (refParams.Count > 1) + if (refParamEnumerable == EnumerablePeek.Many) { throw new ArgumentException( "Multiple complex types found. Specify one parameter as the body using BodyAttribute" ); } - if (refParams.Count == 1) + if (refParamEnumerable == EnumerablePeek.Single) { return Tuple.Create( BodySerializationMethod.Serialized, RefitSettings.Buffered, - parameterList.IndexOf(refParams[0]) + parameterList.IndexOf(refParam!) ); } @@ -508,7 +505,7 @@ HttpMethod method static Tuple? FindAuthorizationParameter(IList parameterList) { - var authorizeParams = parameterList + var authorizeParamsEnumerable = parameterList .Select( x => new @@ -520,19 +517,18 @@ HttpMethod method } ) .Where(x => x.AuthorizeAttribute != null) - .ToList(); + .TryGetSingle(out var authorizeParam); - if (authorizeParams.Count > 1) + if (authorizeParamsEnumerable == EnumerablePeek.Many) { throw new ArgumentException("Only one parameter can be an Authorize parameter"); } - if (authorizeParams.Count == 1) + if (authorizeParamsEnumerable == EnumerablePeek.Single) { - var ret = authorizeParams[0]; return Tuple.Create( - ret.AuthorizeAttribute!.Scheme, - parameterList.IndexOf(ret.Parameter) + authorizeParam!.AuthorizeAttribute!.Scheme, + parameterList.IndexOf(authorizeParam.Parameter) ); } @@ -582,13 +578,13 @@ HttpMethod method return ret; } - static Dictionary BuildHeaderParameterMap(List parameterList) + static Dictionary BuildHeaderParameterMap(ParameterInfo[] parameterArray) { var ret = new Dictionary(); - for (var i = 0; i < parameterList.Count; i++) + for (var i = 0; i < parameterArray.Length; i++) { - var header = parameterList[i] + var header = parameterArray[i] .GetCustomAttributes(true) .OfType() .Select(ha => ha.Header)