From a2cea185296464af6eec3d0af27d6c7997c8e339 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 25 Mar 2014 11:19:45 -0700 Subject: [PATCH] Replacing argument not null checks in ModelBinding with NotNullAttribute * Removing not null guard tests in ModelBinding --- .../Binders/ComplexModelDto.cs | 14 ++------ .../Internal/Error.cs | 34 ------------------- .../Internal/ModelBindingHelper.cs | 15 +++----- .../Metadata/AssociatedMetadataProvider.cs | 5 ++- .../Metadata/ModelMetadata.cs | 15 ++------ .../Validation/ModelValidationNode.cs | 7 +--- .../ValueProviders/CompositeValueProvider.cs | 13 ++----- .../DictionaryBasedValueProvider.cs | 8 +---- .../QueryStringValueProviderFactory.cs | 7 +--- .../ReadableStringCollectionValueProvider.cs | 24 +++---------- .../Binders/ComplexModelDtoTest.cs | 21 ------------ .../Metadata/ModelMetadataTest.cs | 23 ------------- .../Validation/ModelValidationNodeTest.cs | 12 ------- .../QueryStringValueProviderFactoryTest.cs | 7 ---- ...dableStringCollectionValueProviderTests.cs | 20 ----------- 15 files changed, 22 insertions(+), 203 deletions(-) delete mode 100644 src/Microsoft.AspNet.Mvc.ModelBinding/Internal/Error.cs diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDto.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDto.cs index 8293659af3..03200e55f1 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDto.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ComplexModelDto.cs @@ -1,25 +1,15 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; -using Microsoft.AspNet.Mvc.ModelBinding.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { // Describes a complex model, but uses a collection rather than individual properties as the data store. public class ComplexModelDto { - public ComplexModelDto(ModelMetadata modelMetadata, IEnumerable propertyMetadata) + public ComplexModelDto([NotNull] ModelMetadata modelMetadata, + [NotNull] IEnumerable propertyMetadata) { - if (modelMetadata == null) - { - throw Error.ArgumentNull("modelMetadata"); - } - - if (propertyMetadata == null) - { - throw Error.ArgumentNull("propertyMetadata"); - } - ModelMetadata = modelMetadata; PropertyMetadata = new Collection(propertyMetadata.ToList()); Results = new Dictionary(); diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/Error.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/Error.cs deleted file mode 100644 index f9df587396..0000000000 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/Error.cs +++ /dev/null @@ -1,34 +0,0 @@ -using System; -using System.Globalization; - -namespace Microsoft.AspNet.Mvc.ModelBinding.Internal -{ - internal static class Error - { - internal static ArgumentException ArgumentNull(string paramName) - { - return new ArgumentNullException(paramName); - } - - /// - /// Creates an with the provided properties. - /// - /// The name of the parameter that caused the current exception. - /// A composite message explaining the reason for the exception. - /// The logged . - internal static ArgumentException Argument(string parameterName, string message) - { - return new ArgumentException(message, parameterName); - } - - /// - /// Creates an with a default message. - /// - /// The name of the parameter that caused the current exception. - /// The logged . - internal static ArgumentException ArgumentNullOrEmpty(string parameterName) - { - return Error.Argument(parameterName, Resources.FormatArgumentNullOrEmpty(parameterName)); - } - } -} diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/ModelBindingHelper.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/ModelBindingHelper.cs index 5110a50088..ffc2872b4b 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/ModelBindingHelper.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Internal/ModelBindingHelper.cs @@ -53,16 +53,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Internal } } - internal static void ValidateBindingContext(ModelBindingContext bindingContext) + internal static void ValidateBindingContext([NotNull] ModelBindingContext bindingContext) { - if (bindingContext == null) - { - throw Error.ArgumentNull("bindingContext"); - } - if (bindingContext.ModelMetadata == null) { - throw Error.Argument("bindingContext", Resources.ModelBinderUtil_ModelMetadataCannotBeNull); + throw new ArgumentException("bindingContext", Resources.ModelBinderUtil_ModelMetadataCannotBeNull); } } @@ -73,13 +68,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Internal if (bindingContext.ModelType != requiredType) { var message = Resources.FormatModelBinderUtil_ModelTypeIsWrong(bindingContext.ModelType, requiredType); - throw Error.Argument("bindingContext", message); + throw new ArgumentException(message, "bindingContext"); } if (!allowNullModel && bindingContext.Model == null) { var message = Resources.FormatModelBinderUtil_ModelCannotBeNull(requiredType); - throw Error.Argument("bindingContext", message); + throw new ArgumentException(message, "bindingContext"); } if (bindingContext.Model != null && !bindingContext.ModelType.GetTypeInfo().IsAssignableFrom(requiredType.GetTypeInfo())) @@ -87,7 +82,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Internal var message = Resources.FormatModelBinderUtil_ModelInstanceIsWrong( bindingContext.Model.GetType(), requiredType); - throw Error.Argument("bindingContext", message); + throw new ArgumentException(message, "bindingContext"); } } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs index 78997c3c81..1ad3bbb886 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs @@ -5,7 +5,6 @@ using System.Diagnostics.Contracts; using System.Linq; using System.Reflection; using System.Reflection.Emit; -using Microsoft.AspNet.Mvc.ModelBinding.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { @@ -23,14 +22,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { if (string.IsNullOrEmpty(propertyName)) { - throw Error.ArgumentNullOrEmpty("propertyName"); + throw new ArgumentException(Resources.FormatArgumentNullOrEmpty("propertyName"), "propertyName"); } var typeInfo = GetTypeInformation(containerType); PropertyInformation propertyInfo; if (!typeInfo.Properties.TryGetValue(propertyName, out propertyInfo)) { - throw Error.Argument("propertyName", Resources.FormatCommon_PropertyNotFound(containerType, propertyName)); + throw new ArgumentException(Resources.FormatCommon_PropertyNotFound(containerType, propertyName), "propertyName"); } return CreatePropertyMetadata(modelAccessor, propertyInfo); diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs index 9128b59ec4..97b8f7f4cc 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ModelMetadata.cs @@ -18,21 +18,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private IEnumerable _properties; private Type _realModelType; - public ModelMetadata(IModelMetadataProvider provider, + public ModelMetadata([NotNull] IModelMetadataProvider provider, Type containerType, - Func modelAccessor, - Type modelType, + Func modelAccessor, + [NotNull] Type modelType, string propertyName) { - if (provider == null) - { - throw Error.ArgumentNull("provider"); - } - if (modelType == null) - { - throw Error.ArgumentNull("modelType"); - } - Provider = provider; _containerType = containerType; diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs index 8d3686c438..bb0ccfe8b5 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs @@ -97,13 +97,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Validate(validationContext, parentNode: null); } - public void Validate(ModelValidationContext validationContext, ModelValidationNode parentNode) + public void Validate([NotNull] ModelValidationContext validationContext, ModelValidationNode parentNode) { - if (validationContext == null) - { - throw Error.ArgumentNull("validationContext"); - } - if (SuppressValidation) { // no-op diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/CompositeValueProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/CompositeValueProvider.cs index 1c3a593b88..07a61554c1 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/CompositeValueProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/CompositeValueProvider.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics.CodeAnalysis; using System.Linq; -using Microsoft.AspNet.Mvc.ModelBinding.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { @@ -68,21 +67,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return (enumeratedProvider != null) ? enumeratedProvider.GetKeysFromPrefix(prefix) : null; } - protected override void InsertItem(int index, IValueProvider item) + protected override void InsertItem(int index, [NotNull] IValueProvider item) { - if (item == null) - { - throw Error.ArgumentNull("item"); - } base.InsertItem(index, item); } - protected override void SetItem(int index, IValueProvider item) + protected override void SetItem(int index, [NotNull] IValueProvider item) { - if (item == null) - { - throw Error.ArgumentNull("item"); - } base.SetItem(index, item); } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/DictionaryBasedValueProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/DictionaryBasedValueProvider.cs index a5af3a7908..88c7eb5cf5 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/DictionaryBasedValueProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/DictionaryBasedValueProvider.cs @@ -1,7 +1,6 @@  using System.Collections.Generic; using System.Globalization; -using Microsoft.AspNet.Mvc.ModelBinding.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { @@ -19,13 +18,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return _values.ContainsKey(key); } - public ValueProviderResult GetValue(string key) + public ValueProviderResult GetValue([NotNull] string key) { - if (key == null) - { - throw Error.ArgumentNull("key"); - } - object value; if (_values.TryGetValue(key, out value)) { diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/QueryStringValueProviderFactory.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/QueryStringValueProviderFactory.cs index d33c9c07e2..9b22eba721 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/QueryStringValueProviderFactory.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/QueryStringValueProviderFactory.cs @@ -8,13 +8,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { private static readonly object _cacheKey = new object(); - public Task GetValueProviderAsync(RequestContext requestContext) + public Task GetValueProviderAsync([NotNull] RequestContext requestContext) { - if (requestContext == null) - { - throw Error.ArgumentNull("requestContext"); - } - // Process the query collection once-per request. var storage = requestContext.HttpContext.Items; object value; diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/ReadableStringCollectionValueProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/ReadableStringCollectionValueProvider.cs index 5ed9f2e6b9..888600328e 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/ReadableStringCollectionValueProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ValueProviders/ReadableStringCollectionValueProvider.cs @@ -17,13 +17,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// The key value pairs to wrap. /// The culture to return with ValueProviderResult instances. - public ReadableStringCollectionValueProvider(IReadableStringCollection values, CultureInfo culture) + public ReadableStringCollectionValueProvider([NotNull] IReadableStringCollection values, CultureInfo culture) { - if (values == null) - { - throw Error.ArgumentNull("values"); - } - _values = values; _culture = culture; } @@ -56,24 +51,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return PrefixContainer.ContainsPrefix(prefix); } - public virtual IDictionary GetKeysFromPrefix(string prefix) + public virtual IDictionary GetKeysFromPrefix([NotNull] string prefix) { - if (prefix == null) - { - throw Error.ArgumentNull("prefix"); - } - return PrefixContainer.GetKeysFromPrefix(prefix); } - public virtual ValueProviderResult GetValue(string key) - { - if (key == null) - { - throw Error.ArgumentNull("key"); - } - IList values = _values.GetValues(key); + public virtual ValueProviderResult GetValue([NotNull] string key) + { + var values = _values.GetValues(key); if (values == null) { return null; diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ComplexModelDtoTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ComplexModelDtoTest.cs index 511d5bed41..cabd8e3446 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ComplexModelDtoTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ComplexModelDtoTest.cs @@ -5,27 +5,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { public class ComplexModelDtoTest { - [Fact] - public void ConstructorThrowsIfModelMetadataIsNull() - { - // Act & assert - ExceptionAssert.ThrowsArgumentNull( - () => new ComplexModelDto(null, Enumerable.Empty()), - "modelMetadata"); - } - - [Fact] - public void ConstructorThrowsIfPropertyMetadataIsNull() - { - // Arrange - ModelMetadata modelMetadata = GetModelMetadata(); - - // Act & assert - ExceptionAssert.ThrowsArgumentNull( - () => new ComplexModelDto(modelMetadata, null), - "propertyMetadata"); - } - [Fact] public void ConstructorSetsProperties() { diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs index 69bb8733ac..b67e8f8da9 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs @@ -10,30 +10,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { public class ModelMetadataTest { - // Guard clauses - - [Fact] - public void NullProviderThrows() - { - // Act & Assert - ExceptionAssert.ThrowsArgumentNull( - () => new ModelMetadata(provider: null, containerType: null, modelAccessor: null, modelType: typeof(object), propertyName: null), - "provider"); - } - #if NET45 - [Fact] - public void NullTypeThrows() - { - // Arrange - Mock provider = new Mock(); - - // Act & Assert - ExceptionAssert.ThrowsArgumentNull( - () => new ModelMetadata(provider: provider.Object, containerType: null, modelAccessor: null, modelType: null, propertyName: null), - "modelType"); - } - // Constructor [Fact] diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs index a6ce9de632..3215da0ead 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelValidationNodeTest.cs @@ -215,18 +215,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Empty(log); } - [Fact] - public void Validate_ThrowsIfControllerContextIsNull() - { - // Arrange - var node = new ModelValidationNode(GetModelMetadata(), "someKey"); - - // Act & assert - ExceptionAssert.ThrowsArgumentNull( - () => node.Validate(null), - "validationContext"); - } - [Fact] [ReplaceCulture] public void Validate_ValidateAllProperties_AddsValidationErrors() diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/QueryStringValueProviderFactoryTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/QueryStringValueProviderFactoryTest.cs index d262223503..ef86c868b8 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/QueryStringValueProviderFactoryTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/QueryStringValueProviderFactoryTest.cs @@ -13,13 +13,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { private readonly QueryStringValueProviderFactory _factory = new QueryStringValueProviderFactory(); - [Fact] - public void GetValueProvider_WhenrequestContextParameterIsNull_Throws() - { - // Act and Assert - ExceptionAssert.ThrowsArgumentNull(() => _factory.GetValueProviderAsync(requestContext: null), "requestContext"); - } - #if NET45 [Fact] public async Task GetValueProvider_ReturnsQueryStringValueProviderInstaceWithInvariantCulture() diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/ReadableStringCollectionValueProviderTests.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/ReadableStringCollectionValueProviderTests.cs index 38515ec4a0..49bb65827e 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/ReadableStringCollectionValueProviderTests.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/ValueProviders/ReadableStringCollectionValueProviderTests.cs @@ -18,14 +18,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test {"prefix.null_value", null} }); - [Fact] - public void Constructor_GuardClauses() - { - // Act & assert - ExceptionAssert.ThrowsArgumentNull( - () => new ReadableStringCollectionValueProvider(values: null, culture: CultureInfo.InvariantCulture), - "values"); - } [Fact] public void ContainsPrefix_GuardClauses() @@ -91,18 +83,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.False(result); } - [Fact] - public void GetKeysFromPrefix_GuardClauses() - { - // Arrange - var valueProvider = new ReadableStringCollectionValueProvider(_backingStore, null); - - // Act & assert - ExceptionAssert.ThrowsArgumentNull( - () => valueProvider.GetKeysFromPrefix(null), - "prefix"); - } - [Fact] public void GetKeysFromPrefix_EmptyPrefix_ReturnsAllPrefixes() {