From 1a09c21b09de83008b3064d2c97ab8517ac20969 Mon Sep 17 00:00:00 2001 From: Adam Bezverkov Date: Wed, 17 Apr 2019 09:22:58 -0500 Subject: [PATCH] bugfix for setting IsOptional on nullable types --- .gitignore | 2 +- .../HttpPropertyExtractorIsNullableShould.cs | 40 +++++ ...pPropertyExtractorWithQueryParamsShould.cs | 71 ++++++++ ...pPropertyExtractorWithRouteParamsShould.cs | 86 +++++++++ .../Builders/HttpFunctionBuilder.cs | 6 +- .../Infrastructure/HttpParameterExtractor.cs | 164 ++++++++++++++++++ .../Infrastructure/PostBuildPatcher.cs | 142 +-------------- ...nMonkey.Tests.Integration.Functions.csproj | 4 +- 8 files changed, 373 insertions(+), 142 deletions(-) create mode 100644 FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorIsNullableShould.cs create mode 100644 FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithQueryParamsShould.cs create mode 100644 FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithRouteParamsShould.cs create mode 100644 Source/FunctionMonkey/Infrastructure/HttpParameterExtractor.cs diff --git a/.gitignore b/.gitignore index d3d0f8d0..1d4402d6 100644 --- a/.gitignore +++ b/.gitignore @@ -137,7 +137,7 @@ _TeamCity* *.coveragexml # NCrunch -_NCrunch_* +*NCrunch* .*crunch*.local.xml nCrunchTemp_* diff --git a/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorIsNullableShould.cs b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorIsNullableShould.cs new file mode 100644 index 00000000..9bf3fd38 --- /dev/null +++ b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorIsNullableShould.cs @@ -0,0 +1,40 @@ +using System; +using System.Collections.Generic; +using System.Text; +using FunctionMonkey.Infrastructure; +using Xunit; + +namespace FunctionMonkey.Testing.Tests.Infrastructure +{ + public class NullableTestObject + { + public int A { get; set; } + public int? B { get; set; } + } + + public class HttpPropertyExtractorIsNullableShould + { + [Theory] + [InlineData(typeof(int?), typeof(int))] + [InlineData(typeof(float?), typeof(float))] + [InlineData(typeof(bool?), typeof(bool))] + public void IdentifyAsNullable(Type t, Type nullT) + { + Assert.Equal(nullT, Nullable.GetUnderlyingType(t)); + Assert.True(HttpParameterExtractor.IsNullableType(t)); + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(float))] + [InlineData(typeof(bool))] + [InlineData(typeof(string))] + [InlineData(typeof(object))] + [InlineData(typeof(NullableTestObject))] + public void IdentifyAsNotNullable(Type t) + { + Assert.Null(Nullable.GetUnderlyingType(t)); + Assert.False(HttpParameterExtractor.IsNullableType(t)); + } + } +} diff --git a/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithQueryParamsShould.cs b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithQueryParamsShould.cs new file mode 100644 index 00000000..ecac41be --- /dev/null +++ b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithQueryParamsShould.cs @@ -0,0 +1,71 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using AzureFromTheTrenches.Commanding.Abstractions; +using FunctionMonkey; +using FunctionMonkey.Abstractions.Builders.Model; +using FunctionMonkey.Builders; +using FunctionMonkey.Infrastructure; +using FunctionMonkey.Model; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace FunctionMonkey.Testing.Tests.Infrastructure +{ + public class TestCommand : ICommand + { + public int IntRequired { get; set; } + public int? IntOptional { get; set; } + } + + public class HttpPropertyExtractorWithQueryParamsShould + { + private readonly HttpParameterExtractor _parameterExtractor; + + public HttpPropertyExtractorWithQueryParamsShould() + { + var definitions = new List(); + var routeConfiguration = new HttpRouteConfiguration(); + ConnectionStringSettingNames connStringSettingsNames = null; + + var functionBuilder = new HttpFunctionBuilder(connStringSettingsNames, routeConfiguration, definitions) + .HttpFunction("/testcommand"); + + var httpFunctionDefinition = definitions.First() as HttpFunctionDefinition; + httpFunctionDefinition.RouteParameters = new HttpParameter[0]; + + _parameterExtractor = new HttpParameterExtractor(httpFunctionDefinition); + } + + [Fact] + public void ExtractPossibleQueryParametersAsOptional() + { + + var httpParams = _parameterExtractor.ExtractPossibleQueryParameters(); + + var param = httpParams.First(x => x.Name == "IntRequired"); + Assert.Equal(typeof(int), param.Type); + Assert.Equal("System.Int32", param.TypeName); + Assert.False(param.IsOptional); + Assert.False(param.IsNullable); + Assert.False(param.IsNullableType); + Assert.False(param.IsString); + } + + [Fact] + public void ExtractPossibleQueryParametersAsOptional2() + { + + var httpParams = _parameterExtractor.ExtractPossibleQueryParameters(); + + var param = httpParams.First(x => x.Name == "IntOptional"); + Assert.Equal(typeof(int?), param.Type); + Assert.Equal("System.Nullable", param.TypeName); + Assert.True(param.IsOptional); + Assert.True(param.IsNullable); + Assert.True(param.IsNullableType); + Assert.False(param.IsString); + } + } +} diff --git a/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithRouteParamsShould.cs b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithRouteParamsShould.cs new file mode 100644 index 00000000..b7e2956e --- /dev/null +++ b/FunctionMonkey.Testing.Tests/Infrastructure/HttpPropertyExtractorWithRouteParamsShould.cs @@ -0,0 +1,86 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using AzureFromTheTrenches.Commanding.Abstractions; +using FunctionMonkey; +using FunctionMonkey.Abstractions.Builders.Model; +using FunctionMonkey.Builders; +using FunctionMonkey.Infrastructure; +using FunctionMonkey.Model; +using Microsoft.Extensions.DependencyInjection; +using Xunit; + +namespace FunctionMonkey.Testing.Tests.Infrastructure +{ + public class RouteParamTestCommand : ICommand + { + public int A { get; set; } + public int? B { get; set; } + public int? C { get; set; } + } + + public class HttpPropertyExtractorWithRouteParamsShould + { + private readonly HttpParameterExtractor _parameterExtractor; + + public HttpPropertyExtractorWithRouteParamsShould() + { + var definitions = new List(); + var routeConfiguration = new HttpRouteConfiguration(); + ConnectionStringSettingNames connStringSettingsNames = null; + + var functionBuilder = new HttpFunctionBuilder(connStringSettingsNames, routeConfiguration, definitions) + .HttpFunction("/testcommand/{A}/{B}/{C?}"); + + var httpFunctionDefinition = definitions.First() as HttpFunctionDefinition; + + _parameterExtractor = new HttpParameterExtractor(httpFunctionDefinition); + } + + [Fact] + public void ExtractPossibleRouteParametersAsNotOptional() + { + + var httpParams = _parameterExtractor.ExtractRouteParameters(); + + var param = httpParams.First(x => x.Name == "A"); + Assert.Equal(typeof(int), param.Type); + Assert.Equal("System.Int32", param.TypeName); + Assert.False(param.IsOptional); + Assert.False(param.IsNullable); + Assert.False(param.IsNullableType); + Assert.False(param.IsString); + } + + [Fact] + public void ExtractPossibleRouteParametersAsNotOptional2() + { + + var httpParams = _parameterExtractor.ExtractRouteParameters(); + + var param = httpParams.First(x => x.Name == "B"); + Assert.Equal(typeof(int?), param.Type); + Assert.Equal("System.Nullable", param.TypeName); + Assert.False(param.IsOptional); + Assert.True(param.IsNullable); + Assert.True(param.IsNullableType); + Assert.False(param.IsString); + } + + [Fact] + public void ExtractPossibleRouteParametersAsOptional() + { + + var httpParams = _parameterExtractor.ExtractRouteParameters(); + + var param = httpParams.First(x => x.Name == "C"); + Assert.Equal(typeof(int?), param.Type); + Assert.Equal("System.Nullable", param.TypeName); + Assert.True(param.IsOptional); + Assert.True(param.IsNullable); + Assert.True(param.IsNullableType); + Assert.False(param.IsString); + } + } +} diff --git a/Source/FunctionMonkey/Builders/HttpFunctionBuilder.cs b/Source/FunctionMonkey/Builders/HttpFunctionBuilder.cs index c3434ff1..23f76a86 100644 --- a/Source/FunctionMonkey/Builders/HttpFunctionBuilder.cs +++ b/Source/FunctionMonkey/Builders/HttpFunctionBuilder.cs @@ -8,7 +8,7 @@ namespace FunctionMonkey.Builders { - class HttpFunctionBuilder : IHttpFunctionBuilder + public class HttpFunctionBuilder : IHttpFunctionBuilder { private static readonly HttpMethod DefaultMethod = HttpMethod.Get; @@ -22,8 +22,8 @@ public HttpFunctionBuilder( List definitions) { _connectionStringSettingNames = connectionStringSettingNames; - _routeConfiguration = routeConfiguration; - _definitions = definitions; + _routeConfiguration = routeConfiguration ?? throw new System.ArgumentNullException(nameof(routeConfiguration)); + _definitions = definitions ?? throw new System.ArgumentNullException(nameof(definitions)); } public IHttpFunctionConfigurationBuilder HttpFunction() where TCommand : ICommand diff --git a/Source/FunctionMonkey/Infrastructure/HttpParameterExtractor.cs b/Source/FunctionMonkey/Infrastructure/HttpParameterExtractor.cs new file mode 100644 index 00000000..4ebdae15 --- /dev/null +++ b/Source/FunctionMonkey/Infrastructure/HttpParameterExtractor.cs @@ -0,0 +1,164 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Reflection; +using System.Text; +using System.Text.RegularExpressions; +using AzureFromTheTrenches.Commanding.Abstractions; +using FunctionMonkey.Abstractions.Extensions; +using FunctionMonkey.Model; +using Microsoft.AspNetCore.Http; + +namespace FunctionMonkey.Infrastructure +{ + public class HttpParameterExtractor + { + private readonly HttpFunctionDefinition _httpFunctionDefinition; + + public HttpParameterExtractor(HttpFunctionDefinition httpFunctionDefinition) + { + this._httpFunctionDefinition = httpFunctionDefinition ?? throw new ArgumentNullException(nameof(httpFunctionDefinition)); + } + + public HttpParameter[] ExtractPossibleQueryParameters() + { + //Debug.Assert(_httpFunctionDefinition.RouteParameters != null); + + var properties = _httpFunctionDefinition + .CommandType + .GetProperties(BindingFlags.Instance | BindingFlags.Public); + + properties = properties + .Where(x => x.GetCustomAttribute() == null + && x.SetMethod != null + && (x.PropertyType == typeof(string) + || x.PropertyType.GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse") + || x.PropertyType.IsEnum + || HttpParameterExtractor.IsNullableType(x.PropertyType)) + && _httpFunctionDefinition.RouteParameters.All(y => y.Name != x.Name) // we can't be a query parameter and a route parameter + ).ToArray(); + + return properties.Select(x => CreateHttpParameter(x)).ToArray(); + } + + private HttpParameter CreateHttpParameter(PropertyInfo x, bool? optional = null) + { + var isOptional = optional ?? !x.PropertyType.IsValueType || HttpParameterExtractor.IsNullableType(x.PropertyType); + + return new HttpParameter + { + Name = x.Name, + Type = x.PropertyType, + TypeName = x.PropertyType.EvaluateType(), + IsOptional = isOptional, + IsNullableType = HttpParameterExtractor.IsNullableType(x.PropertyType), + }; + } + + public HttpParameter[] ExtractPossibleFormParameters() + { + return _httpFunctionDefinition + .CommandType + .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .Where(x => x.GetCustomAttribute() == null + && x.SetMethod != null + && (x.PropertyType == typeof(IFormCollection))) + .Select(x => CreateHttpParameter(x)) + .ToArray(); + } + + public HttpParameter[] ExtractRouteParameters() + { + List routeParameters = new List(); + if (_httpFunctionDefinition.Route == null) + { + _httpFunctionDefinition.RouteParameters = routeParameters; + return routeParameters.ToArray(); + } + + PropertyInfo[] candidateCommandProperties = _httpFunctionDefinition + .CommandType + .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .Where(x => x.GetCustomAttribute() == null + && x.SetMethod != null + && (x.PropertyType == typeof(string) + || HttpParameterExtractor.IsNullableType(x.PropertyType) + || x.PropertyType.GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse"))).ToArray(); + Regex regex = new Regex("{(.*?)}"); + MatchCollection matches = regex.Matches(_httpFunctionDefinition.Route); + foreach (Match match in matches) //you can loop through your matches like this + { + string routeParameter = match.Groups[1].Value; + bool isOptional = routeParameter.EndsWith("?"); + string[] routeParameterParts = routeParameter.Split(':'); + if (routeParameterParts.Length == 0) + { + throw new ConfigurationException($"Bad route parameter in route {_httpFunctionDefinition.Route} for command type {_httpFunctionDefinition.CommandType.FullName}"); + } + + string routeParameterName = routeParameterParts[0].TrimEnd('?'); + PropertyInfo[] candidateProperties = candidateCommandProperties + .Where(p => p.Name.ToLower() == routeParameterName.ToLower()).ToArray(); + PropertyInfo matchedProperty = null; + if (candidateProperties.Length == 1) + { + matchedProperty = candidateProperties[0]; + } + else if (candidateProperties.Length > 1) + { + matchedProperty = candidateProperties.SingleOrDefault(x => x.Name == routeParameterName); + } + + if (matchedProperty == null) + { + throw new ConfigurationException($"Unable to match route parameter {routeParameterName} to property on command type {_httpFunctionDefinition.CommandType}"); + } + + bool isPropertyNullable = !matchedProperty.PropertyType.IsValueType || + HttpParameterExtractor.IsNullableType(matchedProperty.PropertyType); + + string routeTypeName; + if (isOptional && matchedProperty.PropertyType.IsValueType && + HttpParameterExtractor.IsNullableType(matchedProperty.PropertyType)) + { + routeTypeName = $"{matchedProperty.PropertyType.EvaluateType()}?"; + } + else + { + routeTypeName = matchedProperty.PropertyType.EvaluateType(); + } + + var param = CreateHttpParameter(matchedProperty, isOptional); + param.RouteName = routeParameterName; + param.RouteTypeName = routeTypeName; + routeParameters.Add(param); + } + + return routeParameters.ToArray(); + + /*string lowerCaseRoute = httpFunctionDefinition1.Route.ToLower(); + httpFunctionDefinition1.RouteParameters = httpFunctionDefinition1 + .CommandType + .GetProperties(BindingFlags.Instance | BindingFlags.Public) + .Where(x => x.GetCustomAttribute() == null + && x.SetMethod != null + && (x.PropertyType == typeof(string) || x.PropertyType + .GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse")) + && lowerCaseRoute.Contains("{" + x.Name.ToLower() + "}")) + .Select(x => new HttpParameter + { + Name = x.Name, + TypeName = x.PropertyType.EvaluateType(), + Type = x.PropertyType + }) + .ToArray();*/ + } + + public static bool IsNullableType(Type t) + { + return Nullable.GetUnderlyingType(t) != null; + //return (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(Nullable<>)); + } + } +} diff --git a/Source/FunctionMonkey/Infrastructure/PostBuildPatcher.cs b/Source/FunctionMonkey/Infrastructure/PostBuildPatcher.cs index 22b52e24..32c2bde2 100644 --- a/Source/FunctionMonkey/Infrastructure/PostBuildPatcher.cs +++ b/Source/FunctionMonkey/Infrastructure/PostBuildPatcher.cs @@ -108,11 +108,13 @@ private static void CompleteHttpFunctionDefinition(FunctionHostBuilder builder, throw new ConfigurationException($"Command {httpFunctionDefinition.CommandType.Name} expects to be authenticated with token validation but no token validator is registered"); } - ExtractRouteParameters(httpFunctionDefinition); + HttpParameterExtractor hpe = new HttpParameterExtractor(httpFunctionDefinition); - ExtractPossibleQueryParameters(httpFunctionDefinition); + httpFunctionDefinition.RouteParameters = hpe.ExtractRouteParameters(); - ExtractPossibleFormParameters(httpFunctionDefinition); + httpFunctionDefinition.PossibleBindingProperties = hpe.ExtractPossibleQueryParameters(); + + httpFunctionDefinition.PossibleFormProperties = hpe.ExtractPossibleFormParameters(); EnsureOpenApiDescription(httpFunctionDefinition); } @@ -175,139 +177,5 @@ private static void ExtractCosmosCommandProperties(CosmosDbFunctionDefinition fu }) .ToArray(); } - - private static void ExtractPossibleQueryParameters(HttpFunctionDefinition httpFunctionDefinition) - { - Debug.Assert(httpFunctionDefinition.RouteParameters != null); - - httpFunctionDefinition.PossibleBindingProperties = httpFunctionDefinition - .CommandType - .GetProperties(BindingFlags.Instance | BindingFlags.Public) - .Where(x => x.GetCustomAttribute() == null - && x.SetMethod != null - && (x.PropertyType == typeof(string) - || x.PropertyType.GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse") - || x.PropertyType.IsEnum - || x.PropertyType.IsGenericType && x.PropertyType.GetGenericTypeDefinition() == typeof(Nullable<>)) - && httpFunctionDefinition.RouteParameters.All(y => y.Name != x.Name) // we can't be a query parameter and a route parameter - ) - .Select(x => new HttpParameter - { - Name = x.Name, - TypeName = x.PropertyType.EvaluateType(), - Type = x.PropertyType, - IsOptional = !x.PropertyType.IsValueType - }) - .ToArray(); - } - - private static void ExtractPossibleFormParameters(HttpFunctionDefinition httpFunctionDefinition) - { - httpFunctionDefinition.PossibleFormProperties = httpFunctionDefinition - .CommandType - .GetProperties(BindingFlags.Instance | BindingFlags.Public) - .Where(x => x.GetCustomAttribute() == null - && x.SetMethod != null - && (x.PropertyType == typeof(IFormCollection))) - .Select(x => new HttpParameter - { - Name = x.Name, - TypeName = x.PropertyType.EvaluateType(), - Type = x.PropertyType - }) - .ToArray(); - } - - private static void ExtractRouteParameters(HttpFunctionDefinition httpFunctionDefinition1) - { - List routeParameters = new List(); - if (httpFunctionDefinition1.Route == null) - { - httpFunctionDefinition1.RouteParameters = routeParameters; - return; - } - - PropertyInfo[] candidateCommandProperties = httpFunctionDefinition1.CommandType - .GetProperties(BindingFlags.Instance | BindingFlags.Public) - .Where(x => x.GetCustomAttribute() == null - && x.SetMethod != null - && (x.PropertyType == typeof(string) - || Nullable.GetUnderlyingType(x.PropertyType) != null - || x.PropertyType.GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse"))).ToArray(); - Regex regex = new Regex("{(.*?)}"); - MatchCollection matches = regex.Matches(httpFunctionDefinition1.Route); - foreach (Match match in matches) //you can loop through your matches like this - { - string routeParameter = match.Groups[1].Value; - bool isOptional = routeParameter.EndsWith("?"); - string[] routeParameterParts = routeParameter.Split(':'); - if (routeParameterParts.Length == 0) - { - throw new ConfigurationException($"Bad route parameter in route {httpFunctionDefinition1.Route} for command type {httpFunctionDefinition1.CommandType.FullName}"); - } - - string routeParameterName = routeParameterParts[0].TrimEnd('?'); - PropertyInfo[] candidateProperties = candidateCommandProperties - .Where(p => p.Name.ToLower() == routeParameterName.ToLower()).ToArray(); - PropertyInfo matchedProperty = null; - if (candidateProperties.Length == 1) - { - matchedProperty = candidateProperties[0]; - } - else if (candidateProperties.Length > 1) - { - matchedProperty = candidateProperties.SingleOrDefault(x => x.Name == routeParameterName); - } - - if (matchedProperty == null) - { - throw new ConfigurationException($"Unable to match route parameter {routeParameterName} to property on command type {httpFunctionDefinition1.CommandType}"); - } - - bool isPropertyNullable = !matchedProperty.PropertyType.IsValueType || - Nullable.GetUnderlyingType(matchedProperty.PropertyType) != null; - - string routeTypeName; - if (isOptional && matchedProperty.PropertyType.IsValueType && - Nullable.GetUnderlyingType(matchedProperty.PropertyType) == null) - { - routeTypeName = $"{matchedProperty.PropertyType.EvaluateType()}?"; - } - else - { - routeTypeName = matchedProperty.PropertyType.EvaluateType(); - } - - routeParameters.Add(new HttpParameter - { - Name = matchedProperty.Name, - Type = matchedProperty.PropertyType, - TypeName = matchedProperty.PropertyType.EvaluateType(), - IsOptional = isOptional, - IsNullableType = Nullable.GetUnderlyingType(matchedProperty.PropertyType) != null, - RouteName = routeParameterName, - RouteTypeName = routeTypeName - }); - } - - httpFunctionDefinition1.RouteParameters = routeParameters; - - /*string lowerCaseRoute = httpFunctionDefinition1.Route.ToLower(); - httpFunctionDefinition1.RouteParameters = httpFunctionDefinition1 - .CommandType - .GetProperties(BindingFlags.Instance | BindingFlags.Public) - .Where(x => x.GetCustomAttribute() == null - && x.SetMethod != null - && (x.PropertyType == typeof(string) || x.PropertyType - .GetMethods(BindingFlags.Public | BindingFlags.Static).Any(y => y.Name == "TryParse")) - && lowerCaseRoute.Contains("{" + x.Name.ToLower() + "}")) - .Select(x => new HttpParameter - { - Name = x.Name, - TypeName = x.PropertyType.EvaluateType(), - Type = x.PropertyType - }) - .ToArray();*/ - } } } diff --git a/Tests/FunctionMonkey.Tests.Integration.Functions/FunctionMonkey.Tests.Integration.Functions.csproj b/Tests/FunctionMonkey.Tests.Integration.Functions/FunctionMonkey.Tests.Integration.Functions.csproj index 2d0b4dae..ba3b6bfb 100644 --- a/Tests/FunctionMonkey.Tests.Integration.Functions/FunctionMonkey.Tests.Integration.Functions.csproj +++ b/Tests/FunctionMonkey.Tests.Integration.Functions/FunctionMonkey.Tests.Integration.Functions.csproj @@ -6,10 +6,12 @@ FunctionMonkey.Tests.Integration.Functions + + + -