Changes per PR comments

pranavkm · pranavkm · commit efe27b0506ef · 2020-08-14T07:43:06.000-07:00
* Cleanup unused exception code path, fix doc comments
* Clean up usage of variables
* Adjust logging to be consistent
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs
@@ -3,7 +3,6 @@
 
 using System;
 using System.Globalization;
-using System.Runtime.ExceptionServices;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 
@@ -18,9 +17,9 @@ public class DateTimeModelBinder : IModelBinder
         private readonly ILogger _logger;
 
         /// <summary>
-        /// Initializes a new instance of <see cref="DecimalModelBinder"/>.
+        /// Initializes a new instance of <see cref="DateTimeModelBinder"/>.
         /// </summary>
-        /// <param name="supportedStyles">The <see cref="NumberStyles"/>.</param>
+        /// <param name="supportedStyles">The <see cref="DateTimeStyles"/>.</param>
         /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
         public DateTimeModelBinder(DateTimeStyles supportedStyles, ILoggerFactory loggerFactory)
         {
@@ -61,8 +60,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             var type = metadata.UnderlyingOrModelType;
             try
             {
+
                 var value = valueProviderResult.FirstValue;
-                var culture = valueProviderResult.Culture;
 
                 object model;
                 if (string.IsNullOrWhiteSpace(value))
@@ -72,7 +71,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 }
                 else if (type == typeof(DateTime))
                 {
-                    model = DateTime.Parse(value, culture, _supportedStyles);
+                    model = DateTime.Parse(value, valueProviderResult.Culture, _supportedStyles);
                 }
                 else
                 {
@@ -97,17 +96,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             }
             catch (Exception exception)
             {
-                var isFormatException = exception is FormatException;
-                if (!isFormatException && exception.InnerException != null)
-                {
-                    // Unlike TypeConverters, floating point types do not seem to wrap FormatExceptions. Preserve
-                    // this code in case a cursory review of the CoreFx code missed something.
-                    exception = ExceptionDispatchInfo.Capture(exception.InnerException).SourceException;
-                }
-
-                modelState.TryAddModelError(modelName, exception, metadata);
-
                 // Conversion failed.
+                modelState.TryAddModelError(modelName, exception, metadata);
             }
 
             _logger.DoneAttemptingToBindModel(bindingContext);
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinderProvider.cs
@@ -24,9 +24,9 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
             }
 
             var modelType = context.Metadata.UnderlyingOrModelType;
-            var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
             if (modelType == typeof(DateTime))
             {
+                var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
                 return new DateTimeModelBinder(SupportedStyles, loggerFactory);
             }
 
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DecimalModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DecimalModelBinder.cs
@@ -63,7 +63,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             try
             {
                 var value = valueProviderResult.FirstValue;
-                var culture = valueProviderResult.Culture;
 
                 object model;
                 if (string.IsNullOrWhiteSpace(value))
@@ -73,7 +72,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 }
                 else if (type == typeof(decimal))
                 {
-                    model = decimal.Parse(value, _supportedStyles, culture);
+                    model = decimal.Parse(value, _supportedStyles, valueProviderResult.Culture);
                 }
                 else
                 {
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DoubleModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/DoubleModelBinder.cs
@@ -63,7 +63,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             try
             {
                 var value = valueProviderResult.FirstValue;
-                var culture = valueProviderResult.Culture;
 
                 object model;
                 if (string.IsNullOrWhiteSpace(value))
@@ -73,7 +72,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 }
                 else if (type == typeof(double))
                 {
-                    model = double.Parse(value, _supportedStyles, culture);
+                    model = double.Parse(value, _supportedStyles, valueProviderResult.Culture);
                 }
                 else
                 {
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/FloatModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/FloatModelBinder.cs
@@ -63,7 +63,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             try
             {
                 var value = valueProviderResult.FirstValue;
-                var culture = valueProviderResult.Culture;
 
                 object model;
                 if (string.IsNullOrWhiteSpace(value))
@@ -73,7 +72,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 }
                 else if (type == typeof(float))
                 {
-                    model = float.Parse(value, _supportedStyles, culture);
+                    model = float.Parse(value, _supportedStyles, valueProviderResult.Culture);
                 }
                 else
                 {
diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs
@@ -46,6 +46,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 throw new ArgumentNullException(nameof(bindingContext));
             }
 
+            _logger.AttemptingToBindModel(bindingContext);
+
             var valueProviderResult = bindingContext.ValueProvider.GetValue(bindingContext.ModelName);
             if (valueProviderResult == ValueProviderResult.None)
             {
@@ -56,8 +58,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                 return Task.CompletedTask;
             }
 
-            _logger.AttemptingToBindModel(bindingContext);
-
             bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);
 
             try
diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/DateTimeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/DateTimeModelBinderTest.cs
@@ -33,11 +33,11 @@ public async Task BindModel_ReturnsFailure_IfAttemptedValueCannotBeParsed()
         public async Task BindModel_CreatesError_IfAttemptedValueCannotBeParsed()
         {
             // Arrange
-            var message = "The value 'not a number' is not valid.";
+            var message = "The value 'not a date' is not valid.";
             var bindingContext = GetBindingContext();
             bindingContext.ValueProvider = new SimpleValueProvider
             {
-                { "theModelName", "not a number" },
+                { "theModelName", "not a date" },
             };
             var binder = GetBinder();
 
@@ -164,7 +164,7 @@ public async Task BindModel_ReturnsModel_IfAttemptedValueIsValid(Type type)
             var bindingContext = GetBindingContext(type);
             bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("fr-FR"))
             {
-                { "theModelName", "2019-06-14T02:30:04.0Z" }
+                { "theModelName", "2019-06-14T02:30:04.0000000Z" }
             };
             var binder = GetBinder();
 
@@ -184,10 +184,10 @@ public async Task UsesSpecifiedStyleToParseModel()
         {
             // Arrange
             var bindingContext = GetBindingContext();
-            var expected = DateTime.Parse("2019-06-14T02:30:04.0Z");
+            var expected = DateTime.Parse("2019-06-14T02:30:04.0000000Z");
             bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("fr-FR"))
             {
-                { "theModelName", "2019-06-14T02:30:04.0Z" }
+                { "theModelName", "2019-06-14T02:30:04.0000000Z" }
             };
             var binder = GetBinder(DateTimeStyles.AssumeLocal);
 
@@ -218,9 +218,5 @@ private static DefaultModelBindingContext GetBindingContext(Type modelType = nul
                 ValueProvider = new SimpleValueProvider() // empty
             };
         }
-
-        private sealed class TestClass
-        {
-        }
     }
 }
diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs
@@ -194,7 +194,7 @@ public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccess
             // Assert
             Assert.Equal(ModelBindingResult.Failed(), bindingContext.Result);
             Assert.Empty(bindingContext.ModelState);
-            Assert.Equal(2, sink.Writes.Count());
+            Assert.Equal(3, sink.Writes.Count());
         }
 
         [Theory]

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,6 @@`
`3`	`3`
`4`	`4`	`using System;`
`5`	`5`	`using System.Globalization;`
`6`		`-using System.Runtime.ExceptionServices;`
`7`	`6`	`using System.Threading.Tasks;`
`8`	`7`	`using Microsoft.Extensions.Logging;`
`9`	`8`
`@@ -18,9 +17,9 @@ public class DateTimeModelBinder : IModelBinder`
`18`	`17`	`private readonly ILogger _logger;`
`19`	`18`
`20`	`19`	`/// <summary>`
`21`		`- /// Initializes a new instance of <see cref="DecimalModelBinder"/>.`
	`20`	`+ /// Initializes a new instance of <see cref="DateTimeModelBinder"/>.`
`22`	`21`	`/// </summary>`
`23`		`- /// <param name="supportedStyles">The <see cref="NumberStyles"/>.</param>`
	`22`	`+ /// <param name="supportedStyles">The <see cref="DateTimeStyles"/>.</param>`
`24`	`23`	`/// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>`
`25`	`24`	`public DateTimeModelBinder(DateTimeStyles supportedStyles, ILoggerFactory loggerFactory)`
`26`	`25`	`{`
`@@ -61,8 +60,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`61`	`60`	`var type = metadata.UnderlyingOrModelType;`
`62`	`61`	`try`
`63`	`62`	`{`
	`63`	`+`
`64`	`64`	`var value = valueProviderResult.FirstValue;`
`65`		`- var culture = valueProviderResult.Culture;`
`66`	`65`
`67`	`66`	`object model;`
`68`	`67`	`if (string.IsNullOrWhiteSpace(value))`
`@@ -72,7 +71,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`72`	`71`	`}`
`73`	`72`	`else if (type == typeof(DateTime))`
`74`	`73`	`{`
`75`		`- model = DateTime.Parse(value, culture, _supportedStyles);`
	`74`	`+ model = DateTime.Parse(value, valueProviderResult.Culture, _supportedStyles);`
`76`	`75`	`}`
`77`	`76`	`else`
`78`	`77`	`{`
`@@ -97,17 +96,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`97`	`96`	`}`
`98`	`97`	`catch (Exception exception)`
`99`	`98`	`{`
`100`		`- var isFormatException = exception is FormatException;`
`101`		`- if (!isFormatException && exception.InnerException != null)`
`102`		`- {`
`103`		`- // Unlike TypeConverters, floating point types do not seem to wrap FormatExceptions. Preserve`
`104`		`- // this code in case a cursory review of the CoreFx code missed something.`
`105`		`- exception = ExceptionDispatchInfo.Capture(exception.InnerException).SourceException;`
`106`		`- }`
`107`		`-`
`108`		`- modelState.TryAddModelError(modelName, exception, metadata);`
`109`		`-`
`110`	`99`	`// Conversion failed.`
	`100`	`+ modelState.TryAddModelError(modelName, exception, metadata);`
`111`	`101`	`}`
`112`	`102`
`113`	`103`	`_logger.DoneAttemptingToBindModel(bindingContext);`
Original file line number	Diff line number	Diff line change
`@@ -24,9 +24,9 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)`
`24`	`24`	`}`
`25`	`25`
`26`	`26`	`var modelType = context.Metadata.UnderlyingOrModelType;`
`27`		`- var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();`
`28`	`27`	`if (modelType == typeof(DateTime))`
`29`	`28`	`{`
	`29`	`+ var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();`
`30`	`30`	`return new DateTimeModelBinder(SupportedStyles, loggerFactory);`
`31`	`31`	`}`
`32`	`32`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`63`	`63`	`try`
`64`	`64`	`{`
`65`	`65`	`var value = valueProviderResult.FirstValue;`
`66`		`- var culture = valueProviderResult.Culture;`
`67`	`66`
`68`	`67`	`object model;`
`69`	`68`	`if (string.IsNullOrWhiteSpace(value))`
`@@ -73,7 +72,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`73`	`72`	`}`
`74`	`73`	`else if (type == typeof(decimal))`
`75`	`74`	`{`
`76`		`- model = decimal.Parse(value, _supportedStyles, culture);`
	`75`	`+ model = decimal.Parse(value, _supportedStyles, valueProviderResult.Culture);`
`77`	`76`	`}`
`78`	`77`	`else`
`79`	`78`	`{`
Original file line number	Diff line number	Diff line change
`@@ -46,6 +46,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`46`	`46`	`throw new ArgumentNullException(nameof(bindingContext));`
`47`	`47`	`}`
`48`	`48`
	`49`	`+ _logger.AttemptingToBindModel(bindingContext);`
	`50`	`+`
`49`	`51`	`var valueProviderResult = bindingContext.ValueProvider.GetValue(bindingContext.ModelName);`
`50`	`52`	`if (valueProviderResult == ValueProviderResult.None)`
`51`	`53`	`{`
`@@ -56,8 +58,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)`
`56`	`58`	`return Task.CompletedTask;`
`57`	`59`	`}`
`58`	`60`
`59`		`- _logger.AttemptingToBindModel(bindingContext);`
`60`		`-`
`61`	`61`	`bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);`
`62`	`62`
`63`	`63`	`try`
Original file line number	Diff line number	Diff line change
`@@ -33,11 +33,11 @@ public async Task BindModel_ReturnsFailure_IfAttemptedValueCannotBeParsed()`
`33`	`33`	`public async Task BindModel_CreatesError_IfAttemptedValueCannotBeParsed()`
`34`	`34`	`{`
`35`	`35`	`// Arrange`
`36`		`- var message = "The value 'not a number' is not valid.";`
	`36`	`+ var message = "The value 'not a date' is not valid.";`
`37`	`37`	`var bindingContext = GetBindingContext();`
`38`	`38`	`bindingContext.ValueProvider = new SimpleValueProvider`
`39`	`39`	`{`
`40`		`- { "theModelName", "not a number" },`
	`40`	`+ { "theModelName", "not a date" },`
`41`	`41`	`};`
`42`	`42`	`var binder = GetBinder();`
`43`	`43`
`@@ -164,7 +164,7 @@ public async Task BindModel_ReturnsModel_IfAttemptedValueIsValid(Type type)`
`164`	`164`	`var bindingContext = GetBindingContext(type);`
`165`	`165`	`bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("fr-FR"))`
`166`	`166`	`{`
`167`		`- { "theModelName", "2019-06-14T02:30:04.0Z" }`
	`167`	`+ { "theModelName", "2019-06-14T02:30:04.0000000Z" }`
`168`	`168`	`};`
`169`	`169`	`var binder = GetBinder();`
`170`	`170`
`@@ -184,10 +184,10 @@ public async Task UsesSpecifiedStyleToParseModel()`
`184`	`184`	`{`
`185`	`185`	`// Arrange`
`186`	`186`	`var bindingContext = GetBindingContext();`
`187`		`- var expected = DateTime.Parse("2019-06-14T02:30:04.0Z");`
	`187`	`+ var expected = DateTime.Parse("2019-06-14T02:30:04.0000000Z");`
`188`	`188`	`bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("fr-FR"))`
`189`	`189`	`{`
`190`		`- { "theModelName", "2019-06-14T02:30:04.0Z" }`
	`190`	`+ { "theModelName", "2019-06-14T02:30:04.0000000Z" }`
`191`	`191`	`};`
`192`	`192`	`var binder = GetBinder(DateTimeStyles.AssumeLocal);`
`193`	`193`
`@@ -218,9 +218,5 @@ private static DefaultModelBindingContext GetBindingContext(Type modelType = nul`
`218`	`218`	`ValueProvider = new SimpleValueProvider() // empty`
`219`	`219`	`};`
`220`	`220`	`}`
`221`		`-`
`222`		`- private sealed class TestClass`
`223`		`- {`
`224`		`- }`
`225`	`221`	`}`
`226`	`222`	`}`
Original file line number	Diff line number	Diff line change
`@@ -194,7 +194,7 @@ public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccess`
`194`	`194`	`// Assert`
`195`	`195`	`Assert.Equal(ModelBindingResult.Failed(), bindingContext.Result);`
`196`	`196`	`Assert.Empty(bindingContext.ModelState);`
`197`		`- Assert.Equal(2, sink.Writes.Count());`
	`197`	`+ Assert.Equal(3, sink.Writes.Count());`
`198`	`198`	`}`
`199`	`199`
`200`	`200`	`[Theory]`