From 437f1498802730fb1e7a80706e95f2a49576c1e6 Mon Sep 17 00:00:00 2001 From: John Luo Date: Tue, 3 Sep 2019 10:39:23 -0700 Subject: [PATCH 1/3] Build analyzers and bundled dotnet tools in source build (#13569) * Add external package available in source buid --- Directory.Build.targets | 3 ++- eng/Dependencies.props | 5 ++++- eng/Versions.props | 2 +- src/Tools/dotnet-dev-certs/src/dotnet-dev-certs.csproj | 1 + src/Tools/dotnet-user-secrets/src/dotnet-user-secrets.csproj | 1 + src/Tools/dotnet-watch/src/dotnet-watch.csproj | 1 + 6 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Directory.Build.targets b/Directory.Build.targets index 5006951f11..f5b217f365 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -16,7 +16,8 @@ false - true + + true diff --git a/eng/Dependencies.props b/eng/Dependencies.props index 4419f0a1e5..0cd5ac2537 100644 --- a/eng/Dependencies.props +++ b/eng/Dependencies.props @@ -154,6 +154,10 @@ and are generated based on the last package release. + + + + @@ -171,7 +175,6 @@ and are generated based on the last package release. - diff --git a/eng/Versions.props b/eng/Versions.props index 17f53f876a..296f132e7d 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -235,7 +235,7 @@ 4.10.0 0.10.1 1.0.2 - 12.0.1 + 12.0.2 13.0.4 3.12.1 17.17134.0 diff --git a/src/Tools/dotnet-dev-certs/src/dotnet-dev-certs.csproj b/src/Tools/dotnet-dev-certs/src/dotnet-dev-certs.csproj index a7903fc79c..27fea60070 100644 --- a/src/Tools/dotnet-dev-certs/src/dotnet-dev-certs.csproj +++ b/src/Tools/dotnet-dev-certs/src/dotnet-dev-certs.csproj @@ -9,6 +9,7 @@ true false + false diff --git a/src/Tools/dotnet-user-secrets/src/dotnet-user-secrets.csproj b/src/Tools/dotnet-user-secrets/src/dotnet-user-secrets.csproj index f9e7f56f0b..5f4ec16f4b 100644 --- a/src/Tools/dotnet-user-secrets/src/dotnet-user-secrets.csproj +++ b/src/Tools/dotnet-user-secrets/src/dotnet-user-secrets.csproj @@ -10,6 +10,7 @@ true false + false diff --git a/src/Tools/dotnet-watch/src/dotnet-watch.csproj b/src/Tools/dotnet-watch/src/dotnet-watch.csproj index e50c349324..f77d472c09 100644 --- a/src/Tools/dotnet-watch/src/dotnet-watch.csproj +++ b/src/Tools/dotnet-watch/src/dotnet-watch.csproj @@ -9,6 +9,7 @@ true false + false From 54710e4671bd140413fa2dc047b8921b7b4615dc Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 3 Sep 2019 11:13:16 -0700 Subject: [PATCH 2/3] Do not infer Required attributes based on context for non-nullable generic types (#13551) Fixes https://github.com/aspnet/AspNetCore/issues/13512 --- .../src/DataAnnotationsMetadataProvider.cs | 21 +- .../DataAnnotationsMetadataProviderTest.cs | 32 +++ .../CollectionModelBinderIntegrationTest.cs | 99 +++++++++ .../DictionaryModelBinderIntegrationTest.cs | 206 ++++++++++++++++++ 4 files changed, 352 insertions(+), 6 deletions(-) diff --git a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs index e63b02edce..0e975e697f 100644 --- a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs +++ b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs @@ -333,7 +333,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations var contextAttributes = context.Attributes; var contextAttributesCount = contextAttributes.Count; var attributes = new List(contextAttributesCount); - + for (var i = 0; i < contextAttributesCount; i++) { var attribute = contextAttributes[i]; @@ -367,15 +367,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations else if (context.Key.MetadataKind == ModelMetadataKind.Property) { addInferredRequiredAttribute = IsNullableReferenceType( - context.Key.ContainerType, - member: null, + context.Key.ContainerType, + member: null, context.PropertyAttributes); } else if (context.Key.MetadataKind == ModelMetadataKind.Parameter) { addInferredRequiredAttribute = IsNullableReferenceType( - context.Key.ParameterInfo?.Member.ReflectedType, - context.Key.ParameterInfo.Member, + context.Key.ParameterInfo?.Member.ReflectedType, + context.Key.ParameterInfo.Member, context.ParameterAttributes); } else @@ -494,6 +494,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations internal static bool IsNullableBasedOnContext(Type containingType, MemberInfo member) { + // For generic types, inspecting the nullability requirement additionally requires + // inspecting the nullability constraint on generic type parameters. This is fairly non-triviial + // so we'll just avoid calculating it. Users should still be able to apply an explicit [Required] + // attribute on these members. + if (containingType.IsGenericType) + { + return false; + } + // The [Nullable] and [NullableContext] attributes are not inherited. // // The [NullableContext] attribute can appear on a method or on the module. @@ -516,7 +525,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations } type = type.DeclaringType; - } + } while (type != null); // If we don't find the attribute on the declaring type then repeat at the module level diff --git a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs index 677c452d2d..f93ad363cc 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs @@ -1339,6 +1339,38 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations Assert.True(result); } + [Fact] + public void IsNullableReferenceType_ReturnsFalse_ForKeyValuePairWithoutNullableConstraints() + { + // Arrange + var type = typeof(KeyValuePair); + var property = type.GetProperty(nameof(KeyValuePair.Key)); + + // Act + var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true)); + + // Assert + Assert.False(result); + } + +#nullable enable + [Fact] + public void IsNullableReferenceType_ReturnsTrue_ForKeyValuePairWithNullableConstraints() + { + // Arrange + var type = typeof(KeyValuePair); + var property = type.GetProperty(nameof(KeyValuePair.Key))!; + + // Act + var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true)); + + // Assert + // While we'd like for result to be 'true', we don't have a very good way of actually calculating it correctly. + // This test is primarily here to document the behavior. + Assert.False(result); + } +#nullable restore + [Fact] public void IsNonNullable_FindsNullableProperty() { diff --git a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index ebe8c73ce9..34a0e4fca8 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -1126,6 +1126,105 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(expectedMessage, exception.Message); } + [Fact] + public async Task CollectionModelBinder_CollectionOfSimpleTypes_DoesNotResultInValidationError() + { + // Regression test for https://github.com/aspnet/AspNetCore/issues/13512 + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Collection), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?[0]=hello&[1]="); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var model = Assert.IsType>(result.Model); + Assert.Collection( + model, + item => Assert.Equal("hello", item), + item => Assert.Null(item)); + + Assert.Collection( + modelState, + kvp => + { + Assert.Equal("[0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("[1]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + } + + [Fact] + public async Task CollectionModelBinder_CollectionOfNonNullableTypes_AppliesImplicitRequired() + { + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Collection), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?[0]=hello&[1]="); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var model = Assert.IsType>(result.Model); + Assert.Collection( + model, + item => Assert.Equal("hello", item), + item => Assert.Null(item)); + + Assert.Collection( + modelState, + kvp => + { + Assert.Equal("[0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("[1]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + } + private class ClosedGenericCollection : Collection { } diff --git a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs index e48a33586f..5bdcafa6c5 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; +using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -1162,6 +1163,211 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(expectedMessage, exception.Message); } + [Fact] + public async Task DictionaryModelBinder_DictionaryOfSimpleType_NullValue_DoesNotResultInRequiredValidation() + { + // Regression test for https://github.com/aspnet/AspNetCore/issues/13512 + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0]="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + Assert.Null(kvp.Value); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + +#nullable enable + public class NonNullPerson + { + public int Age { get; set; } + + // This should be implicitly required + public string Name { get; set; } = default!; + } +#nullable restore + + [Fact] + public async Task DictionaryModelBinder_ValuesIsNonNullableType_AppliesImplicitRequired() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0].Age=¶meter[key0].Name=name0¶meter[key1].Age=27¶meter[key1].Name="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + var person = kvp.Value; + Assert.Equal(0, person.Age); + Assert.Equal("name0", person.Name); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + var person = kvp.Value; + Assert.Equal(27, person.Age); + Assert.Null(person.Name); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0].Age", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The value '' is invalid.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }, + kvp => + { + Assert.Equal("parameter[key0].Name", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Age", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Name", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The Name field is required.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }); + Assert.Equal(2, modelState.ErrorCount); + Assert.False(modelState.IsValid); + } + +#nullable enable + public class NonNullPersonWithRequiredProperties + { + public int Age { get; set; } + + [Required] + public string? Name { get; set; } + } +#nullable restore + + [Fact] + public async Task DictionaryModelBinder_ValuesNullableTypeWithRequiredAttributes_AppliesValidation() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0].Age=¶meter[key0].Name=name0¶meter[key1].Age=27¶meter[key1].Name="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + var person = kvp.Value; + Assert.Equal(0, person.Age); + Assert.Equal("name0", person.Name); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + var person = kvp.Value; + Assert.Equal(27, person.Age); + Assert.Null(person.Name); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0].Age", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The value '' is invalid.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }, + kvp => + { + Assert.Equal("parameter[key0].Name", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Age", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Name", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The Name field is required.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }); + Assert.Equal(2, modelState.ErrorCount); + Assert.False(modelState.IsValid); + } + private class ClosedGenericDictionary : Dictionary { } From 1095971a80987867808ae492b148af658ee19e72 Mon Sep 17 00:00:00 2001 From: Chris Ross Date: Tue, 3 Sep 2019 12:40:50 -0700 Subject: [PATCH 3/3] Skip broken shutdown test (#13652) --- src/Hosting/test/FunctionalTests/ShutdownTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Hosting/test/FunctionalTests/ShutdownTests.cs b/src/Hosting/test/FunctionalTests/ShutdownTests.cs index b7f837c110..ce79a471d5 100644 --- a/src/Hosting/test/FunctionalTests/ShutdownTests.cs +++ b/src/Hosting/test/FunctionalTests/ShutdownTests.cs @@ -24,7 +24,7 @@ namespace Microsoft.AspNetCore.Hosting.FunctionalTests public ShutdownTests(ITestOutputHelper output) : base(output) { } - [ConditionalFact] + [ConditionalFact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/2577")] [OSSkipCondition(OperatingSystems.Windows)] [OSSkipCondition(OperatingSystems.MacOSX)] [Flaky("https://github.com/aspnet/AspNetCore-Internal/issues/2577", FlakyOn.All)]