Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use CultureInfo.InvariantCulture when TryParsing minimal action parameters #33624

Merged
9 commits merged into from
Jun 28, 2021
Merged

Use CultureInfo.InvariantCulture when TryParsing minimal action parameters #33624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a751fa0
Use CultureInfo.Invariant - WIP
wcontayon Jun 17, 2021
510ffef
Add unit test and fix tryParse error
wcontayon Jun 18, 2021
0eaeb20
Merge branch 'main' into use_cultureInvariant_parse_mininalAction_par…
wcontayon Jun 18, 2021
3b0e8bd
Update TryParseMethodCache and fix build error - WIP
wcontayon Jun 19, 2021
b884edc
Refacto TryParseMethodCache
wcontayon Jun 20, 2021
3bf3a0e
PR review
wcontayon Jun 22, 2021
a843c58
Merge branch 'master' into use_cultureInvariant_parse_mininalAction_p…
wcontayon Jun 23, 2021
9061776
PR review
wcontayon Jun 25, 2021
c85a14c
Add test for TryParse with CultureInfo.Invariant value
wcontayon Jun 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,9 +505,9 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
var isNotNullable = underlyingNullableType is null;

var nonNullableParameterType = underlyingNullableType ?? parameter.ParameterType;
var tryParseMethod = TryParseMethodCache.FindTryParseMethod(nonNullableParameterType);
var tryParseMethodCall = TryParseMethodCache.FindTryParseMethodCall(parameter);
wcontayon marked this conversation as resolved.
Show resolved Hide resolved

if (tryParseMethod is null)
if (tryParseMethodCall is null)
{
throw new InvalidOperationException($"No public static bool {parameter.ParameterType.Name}.TryParse(string, out {parameter.ParameterType.Name}) method found for {parameter.Name}.");
}
Expand Down Expand Up @@ -562,7 +562,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
Expression.Call(LogParameterBindingFailureMethod,
HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TempSourceStringExpr));

MethodCallExpression? tryParseCall = CreateTryParseCall(tryParseMethod, parameter, parsedValue);
MethodCallExpression? tryParseCall = tryParseMethodCall(TempSourceStringExpr, parsedValue);
wcontayon marked this conversation as resolved.
Show resolved Hide resolved

// If the parameter is nullable, we need to assign the "parsedValue" local to the nullable parameter on success.
Expression tryParseExpression = isNotNullable ?
Expand Down
98 changes: 88 additions & 10 deletions src/Shared/TryParseMethodCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Linq.Expressions;
using System.Numerics;
using System.Reflection;

Expand All @@ -17,11 +18,11 @@ internal static class TryParseMethodCache

// Since this is shared source, the cache won't be shared between RequestDelegateFactory and the ApiDescriptionProvider sadly :(
private static readonly ConcurrentDictionary<Type, MethodInfo?> Cache = new();
private static readonly ConcurrentDictionary<Type, Func<ParameterExpression, Expression, MethodCallExpression>?> MethodCallCache = new();
halter73 marked this conversation as resolved.
Show resolved Hide resolved

public static bool HasTryParseMethod(ParameterInfo parameter)
{
var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType;
return FindTryParseMethod(nonNullableParameterType) is not null;
return FindTryParseMethodCall(parameter) is not null;
}

// TODO: Use InvariantCulture where possible? Or is CurrentCulture fine because it's more flexible?
Expand All @@ -34,9 +35,9 @@ public static bool HasTryParseMethod(ParameterInfo parameter)
return EnumTryParseMethod.MakeGenericMethod(type);
}

if (UseTryParseWithDateTimeStyleOptions(type))
if (TryGetDateTimeTryPareMethod(type, out var methodDateInfo))
{
return GetDateTimeTryPareMethod(type);
return methodDateInfo;
}

if (TryGetNumberStylesTryGetMethod(type, out var methodInfo))
Expand Down Expand Up @@ -79,6 +80,83 @@ public static bool HasTryParseMethod(ParameterInfo parameter)
return Cache.GetOrAdd(type, Finder);
}

public static Func<ParameterExpression, Expression, MethodCallExpression>? FindTryParseMethodCall(ParameterInfo parameter)
{
static Func<ParameterExpression, Expression, MethodCallExpression>? Finder(Type type)
{
MethodInfo? methodInfo;

if (type.IsEnum)
{
methodInfo = EnumTryParseMethod.MakeGenericMethod(type);
if (methodInfo != null)
{
return (parameterExpression, expression) => Expression.Call(methodInfo!, parameterExpression, expression);
}

return null;
}

if (TryGetDateTimeTryPareMethod(type, out methodInfo))
{
return (parameterExpression, expression) => Expression.Call(
methodInfo!,
parameterExpression,
Expression.Constant(CultureInfo.InvariantCulture),
Expression.Constant(DateTimeStyles.None),
expression);
}

if (TryGetNumberStylesTryGetMethod(type, out methodInfo))
{
return (parameterExpression, expression) => Expression.Call(
methodInfo!,
parameterExpression,
Expression.Constant(SetRightNumberStyles(type)),
Expression.Constant(CultureInfo.InvariantCulture),
expression);
}

var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static)
.OrderByDescending(m => m.GetParameters().Length); ;

foreach (var method in staticMethods)
{
if (method.Name != "TryParse" || method.ReturnType != typeof(bool))
{
continue;
}

var tryParseParameters = method.GetParameters();

if (tryParseParameters.Length == 3 &&
tryParseParameters[0].ParameterType == typeof(string) &&
tryParseParameters[1].ParameterType == typeof(IFormatProvider) &&
tryParseParameters[2].IsOut &&
tryParseParameters[2].ParameterType == type.MakeByRefType())
{
return (parameterExpression, expression) => Expression.Call(
method,
parameterExpression,
Expression.Constant(CultureInfo.InvariantCulture),
expression);
}
else if (tryParseParameters.Length == 2 &&
tryParseParameters[0].ParameterType == typeof(string) &&
tryParseParameters[1].IsOut &&
tryParseParameters[1].ParameterType == type.MakeByRefType())
{
return (parameterExpression, expression) => Expression.Call(method, parameterExpression, expression);
}
}

return null;
}

var underlyingType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType;
return MethodCallCache.GetOrAdd(underlyingType, Finder);
}

private static MethodInfo GetEnumTryParseMethod()
{
var staticEnumMethods = typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static);
Expand All @@ -104,13 +182,13 @@ private static MethodInfo GetEnumTryParseMethod()
throw new Exception("static bool System.Enum.TryParse<TEnum>(string? value, out TEnum result) not found.");
}

private static MethodInfo GetDateTimeTryPareMethod(Type type)
private static bool TryGetDateTimeTryPareMethod(Type type, out MethodInfo? methodInfo)
wcontayon marked this conversation as resolved.
Show resolved Hide resolved
{
methodInfo = null;
if (type != typeof(DateTime) && type != typeof(DateOnly) &&
type != typeof(DateTimeOffset) && type != typeof(TimeOnly))
{
Debug.Fail("Parameter is not of type of DateTime, DateOnly, DateTimeOffset, TimeOnly!");
throw new Exception("Parameter is not of type of DateTime, DateOnly, DateTimeOffset, TimeOnly !");
return false;
}

var staticDateMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static)
halter73 marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -128,12 +206,12 @@ private static MethodInfo GetDateTimeTryPareMethod(Type type)
tryParseParameters[3].IsOut &&
tryParseParameters[3].ParameterType == type.MakeByRefType())
{
return method;
methodInfo = method;
break;
}
}

Debug.Fail("static bool TryParse(string?, IFormatProvider, DateTimeStyles, out DateTime result) does not exit!!?!?");
throw new Exception("static bool TryParse(string?, IFormatProvider, DateTimeStyles, out DateTime result) does not exit!!?!?");
return methodInfo != null;
}

private static bool TryGetNumberStylesTryGetMethod(Type type, out MethodInfo? method)
Expand Down