Skip to content

Avoiding ParameterBindingMethodCache exceptions in MVC#40738

Merged
brunolins16 merged 4 commits into
dotnet:mainfrom
brunolins16:brunolins16/issues/40233v2
Mar 22, 2022
Merged

Avoiding ParameterBindingMethodCache exceptions in MVC#40738
brunolins16 merged 4 commits into
dotnet:mainfrom
brunolins16:brunolins16/issues/40233v2

Conversation

@brunolins16
Copy link
Copy Markdown
Member

In MVC, we don't need to throw an InvalidOperationException when an invalid TryParse is detected, since it will fallback to a Converter if one exists or to the previous behavior treating the type as a complex type. Also, the exception is already swallowed.

This PR includes a small change to the Microsoft.AspNetCore.Http.ParameterBindingMethodCache to avoid throwing and handling the exception in MVC.

The change consists of adding a new optional parameter to the constructor:

-public ParameterBindingMethodCache()
+public ParameterBindingMethodCache(bool throwOnInvalidMethod = true)
-public ParameterBindingMethodCache(bool preferNonGenericEnumParseOverload)
+public ParameterBindingMethodCache(bool preferNonGenericEnumParseOverload, bool throwOnInvalidMethod = true)

And the usage of this new information to avoid the exception to be thrown, eg:

if (candidateInterfaceMethodInfo is not null)
{  
-      throw new InvalidOperationException($"{TypeNameHelper.GetTypeDisplayName(type, fullName: false)} implements multiple interfaces defining a static {interfaceMethod} method causing ambiguity.");
+      return !throwOnInvalidMethod ? null : 
+                 throw new InvalidOperationException($"{TypeNameHelper.GetTypeDisplayName(type, fullName: false)} implements multiple interfaces defining a static {interfaceMethod} method causing ambiguity.");
}

Related PR: #40233

@brunolins16 brunolins16 requested a review from a team March 16, 2022 16:42
@Pilchie Pilchie added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Mar 16, 2022
Comment thread src/Shared/ParameterBindingMethodCache.cs Outdated
Copy link
Copy Markdown
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🇮🇹


IsConvertibleType = TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string));
IsParseableType = ModelMetadata.FindTryParseMethod(ModelType) is not null;
IsParseableType = FindTryParseMethod(ModelType) is not null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider moving line 651 above this and use UnderlyingOrModelType instead.

Comment thread src/Shared/ParameterBindingMethodCache.cs Outdated
Comment thread src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs Outdated
@brunolins16 brunolins16 merged commit 66e149e into dotnet:main Mar 22, 2022
@ghost ghost added this to the 7.0-preview3 milestone Mar 22, 2022
@brunolins16 brunolins16 deleted the brunolins16/issues/40233v2 branch March 22, 2022 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants