From 7b58d569ebf64d86c204533b492c1cf515183a47 Mon Sep 17 00:00:00 2001 From: ianhong Date: Thu, 13 Nov 2014 14:11:25 -0800 Subject: [PATCH 1/2] ReadOnly attributes for Action, Controller, and Parameter --- .../ApplicationModels/ActionModel.cs | 10 +++-- .../ApplicationModels/ControllerModel.cs | 7 ++-- .../DefaultActionModelBuilder.cs | 8 +--- .../DefaultControllerModelBuilder.cs | 4 +- .../ApplicationModels/ParameterModel.cs | 7 ++-- .../ApplicationModel/ActionModelTest.cs | 13 +++--- .../ApplicationModel/ControllerModelTest.cs | 10 +++-- .../ApplicationModel/ParameterModelTest.cs | 6 +-- ...ControllerActionDescriptorProviderTests.cs | 41 +++++++++++++++++-- 9 files changed, 71 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ActionModel.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ActionModel.cs index 689c8ff9b3..03ae05474b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ActionModel.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ActionModel.cs @@ -9,12 +9,13 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels { public class ActionModel { - public ActionModel([NotNull] MethodInfo actionMethod) + public ActionModel([NotNull] MethodInfo actionMethod, + [NotNull] IReadOnlyList attributes) { ActionMethod = actionMethod; ApiExplorer = new ApiExplorerModel(); - Attributes = new List(); + Attributes = new List(attributes); ActionConstraints = new List(); Filters = new List(); HttpMethods = new List(); @@ -45,9 +46,10 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels AttributeRouteModel = new AttributeRouteModel(other.AttributeRouteModel); } } + public List ActionConstraints { get; private set; } - public MethodInfo ActionMethod { get; private set; } + public MethodInfo ActionMethod { get; } public string ActionName { get; set; } @@ -60,7 +62,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels /// public ApiExplorerModel ApiExplorer { get; set; } - public List Attributes { get; private set; } + public IReadOnlyList Attributes { get; } public ControllerModel Controller { get; set; } diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ControllerModel.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ControllerModel.cs index 347ca9ef48..ca6651efcd 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ControllerModel.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ControllerModel.cs @@ -9,13 +9,14 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels { public class ControllerModel { - public ControllerModel([NotNull] TypeInfo controllerType) + public ControllerModel([NotNull] TypeInfo controllerType, + [NotNull] IReadOnlyList attributes) { ControllerType = controllerType; Actions = new List(); ApiExplorer = new ApiExplorerModel(); - Attributes = new List(); + Attributes = new List(attributes); AttributeRoutes = new List(); ActionConstraints = new List(); Filters = new List(); @@ -56,7 +57,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public List AttributeRoutes { get; private set; } - public List Attributes { get; private set; } + public IReadOnlyList Attributes { get; } public string ControllerName { get; set; } diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs index 281ca3b02f..ec69dab2a3 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultActionModelBuilder.cs @@ -213,13 +213,11 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels [NotNull] MethodInfo methodInfo, [NotNull] IReadOnlyList attributes) { - var actionModel = new ActionModel(methodInfo) + var actionModel = new ActionModel(methodInfo, attributes) { IsActionNameMatchRequired = true, }; - actionModel.Attributes.AddRange(attributes); - actionModel.ActionConstraints.AddRange(attributes.OfType()); actionModel.Filters.AddRange(attributes.OfType()); @@ -268,12 +266,10 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels /// A for the given . protected virtual ParameterModel CreateParameterModel([NotNull] ParameterInfo parameterInfo) { - var parameterModel = new ParameterModel(parameterInfo); - // CoreCLR returns IEnumerable from GetCustomAttributes - the OfType // is needed to so that the result of ToArray() is object var attributes = parameterInfo.GetCustomAttributes(inherit: true).OfType().ToArray(); - parameterModel.Attributes.AddRange(attributes); + var parameterModel = new ParameterModel(parameterInfo, attributes); parameterModel.BinderMetadata = attributes.OfType().FirstOrDefault(); diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs index d7a4344347..f58c3e1989 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/DefaultControllerModelBuilder.cs @@ -88,12 +88,10 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels /// A for the given . protected virtual ControllerModel CreateControllerModel([NotNull] TypeInfo typeInfo) { - var controllerModel = new ControllerModel(typeInfo); - // CoreCLR returns IEnumerable from GetCustomAttributes - the OfType // is needed to so that the result of ToArray() is object var attributes = typeInfo.GetCustomAttributes(inherit: true).OfType().ToArray(); - controllerModel.Attributes.AddRange(attributes); + var controllerModel = new ControllerModel(typeInfo, attributes); controllerModel.ControllerName = typeInfo.Name.EndsWith("Controller", StringComparison.OrdinalIgnoreCase) ? diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs index 3860768292..85520a0545 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs @@ -9,11 +9,12 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels { public class ParameterModel { - public ParameterModel(ParameterInfo parameterInfo) + public ParameterModel(ParameterInfo parameterInfo, + IReadOnlyList attributes) { ParameterInfo = parameterInfo; - Attributes = new List(); + Attributes = new List(attributes); } public ParameterModel([NotNull] ParameterModel other) @@ -28,7 +29,7 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public ActionModel Action { get; set; } - public List Attributes { get; private set; } + public IReadOnlyList Attributes { get; } public IBinderMetadata BinderMetadata { get; set; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ActionModelTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ActionModelTest.cs index 57f2ff5e18..1a1f8b389b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ActionModelTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ActionModelTest.cs @@ -14,9 +14,11 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void CopyConstructor_DoesDeepCopyOfOtherModels() { // Arrange - var action = new ActionModel(typeof(TestController).GetMethod("Edit")); + var action = new ActionModel(typeof(TestController).GetMethod("Edit"), + new List()); - var parameter = new ParameterModel(action.ActionMethod.GetParameters()[0]); + var parameter = new ParameterModel(action.ActionMethod.GetParameters()[0], + new List()); parameter.Action = action; action.Parameters.Add(parameter); @@ -40,13 +42,14 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void CopyConstructor_CopiesAllProperties() { // Arrange - var action = new ActionModel(typeof(TestController).GetMethod("Edit")); + var action = new ActionModel(typeof(TestController).GetMethod("Edit"), + new List() { new HttpGetAttribute() }); action.ActionConstraints.Add(new HttpMethodConstraint(new string[] { "GET" })); action.ActionName = "Edit"; - action.Attributes.Add(new HttpGetAttribute()); - action.Controller = new ControllerModel(typeof(TestController).GetTypeInfo()); + action.Controller = new ControllerModel(typeof(TestController).GetTypeInfo(), + new List()); action.Filters.Add(new AuthorizeAttribute()); action.HttpMethods.Add("GET"); action.IsActionNameMatchRequired = true; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ControllerModelTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ControllerModelTest.cs index 15603583ea..390c607f19 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ControllerModelTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ControllerModelTest.cs @@ -14,9 +14,11 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void CopyConstructor_DoesDeepCopyOfOtherModels() { // Arrange - var controller = new ControllerModel(typeof(TestController).GetTypeInfo()); + var controller = new ControllerModel(typeof(TestController).GetTypeInfo(), + new List()); - var action = new ActionModel(typeof(TestController).GetMethod("Edit")); + var action = new ActionModel(typeof(TestController).GetMethod("Edit"), + new List()); controller.Actions.Add(action); action.Controller = controller; @@ -46,11 +48,11 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void CopyConstructor_CopiesAllProperties() { // Arrange - var controller = new ControllerModel(typeof(TestController).GetTypeInfo()); + var controller = new ControllerModel(typeof(TestController).GetTypeInfo(), + new List() { new HttpGetAttribute() }); controller.ActionConstraints.Add(new HttpMethodConstraint(new string[] { "GET" })); controller.Application = new ApplicationModel(); - controller.Attributes.Add(new HttpGetAttribute()); controller.ControllerName = "cool"; controller.Filters.Add(new AuthorizeAttribute()); controller.RouteConstraints.Add(new AreaAttribute("Admin")); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs index 9311fb5371..72f3c7db93 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ApplicationModel/ParameterModelTest.cs @@ -14,10 +14,10 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels public void CopyConstructor_CopiesAllProperties() { // Arrange - var parameter = new ParameterModel(typeof(TestController).GetMethod("Edit").GetParameters()[0]); + var parameter = new ParameterModel(typeof(TestController).GetMethod("Edit").GetParameters()[0], + new List() { new FromBodyAttribute() }); - parameter.Action = new ActionModel(typeof(TestController).GetMethod("Edit")); - parameter.Attributes.Add(new FromBodyAttribute()); + parameter.Action = new ActionModel(typeof(TestController).GetMethod("Edit"), new List()); parameter.BinderMetadata = (IBinderMetadata)parameter.Attributes[0]; parameter.IsOptional = true; parameter.ParameterName = "id"; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs index 2d28f6a940..f17c6d52da 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs @@ -1136,19 +1136,25 @@ namespace Microsoft.AspNet.Mvc.Test var options = new MockMvcOptionsAccessor(); options.Options.ApplicationModelConventions.Add(applicationConvention.Object); - + var provider = GetProvider(typeof(ConventionsController).GetTypeInfo(), options); var model = provider.BuildModel(); var controller = model.Controllers.Single(); - controller.Attributes.Add(controllerConvention.Object); + model.Controllers.Remove(controller); + controller = BuildControllerModel(controller, controllerConvention.Object); + model.Controllers.Add(controller); var action = controller.Actions.Single(); - action.Attributes.Add(actionConvention.Object); + controller.Actions.Remove(action); + action = BuildActionModel(action, actionConvention.Object); + controller.Actions.Add(action); var parameter = action.Parameters.Single(); - parameter.Attributes.Add(parameterConvention.Object); + action.Parameters.Remove(parameter); + parameter = BuildParameterModel(parameter, parameterConvention.Object); + action.Parameters.Add(parameter); // Act ApplicationModelConventions.ApplyConventions(model, options.Options.ApplicationModelConventions); @@ -1300,6 +1306,33 @@ namespace Microsoft.AspNet.Mvc.Test Assert.Single(action.ActionConstraints, a => a is ConstraintAttribute); } + private ActionModel BuildActionModel(ActionModel action, IActionModelConvention actionConvention) + { + var t = new ActionModel(action.ActionMethod, + new List(action.Attributes) { actionConvention }); + t.Parameters.AddRange(action.Parameters); + + return t; + } + + private ParameterModel BuildParameterModel(ParameterModel parameter, IParameterModelConvention parameterConvention) + { + var t = new ParameterModel(parameter.ParameterInfo, + new List(parameter.Attributes) { parameterConvention }); + t.Action = parameter.Action; + + return t; + } + + private ControllerModel BuildControllerModel(ControllerModel controller, IControllerModelConvention controllerConvention) + { + var t = new ControllerModel(controller.ControllerType, + new List(controller.Attributes) { controllerConvention }); + t.Actions.AddRange(controller.Actions); + + return t; + } + private ControllerActionDescriptorProvider GetProvider( TypeInfo controllerTypeInfo, IEnumerable filters = null) From 59b7352e8edf17a0f87ae816a767bb9da3366315 Mon Sep 17 00:00:00 2001 From: ianhong Date: Mon, 17 Nov 2014 15:30:55 -0800 Subject: [PATCH 2/2] Update per comment --- .../ApplicationModels/ParameterModel.cs | 4 +- ...ControllerActionDescriptorProviderTests.cs | 60 +++++-------------- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs index 85520a0545..4753eac379 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ApplicationModels/ParameterModel.cs @@ -9,8 +9,8 @@ namespace Microsoft.AspNet.Mvc.ApplicationModels { public class ParameterModel { - public ParameterModel(ParameterInfo parameterInfo, - IReadOnlyList attributes) + public ParameterModel([NotNull] ParameterInfo parameterInfo, + [NotNull] IReadOnlyList attributes) { ParameterInfo = parameterInfo; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs index f17c6d52da..e4af25eb31 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionDescriptorProviderTests.cs @@ -1137,32 +1137,31 @@ namespace Microsoft.AspNet.Mvc.Test var options = new MockMvcOptionsAccessor(); options.Options.ApplicationModelConventions.Add(applicationConvention.Object); - var provider = GetProvider(typeof(ConventionsController).GetTypeInfo(), options); + var applicationModel = new ApplicationModel(); - var model = provider.BuildModel(); + var controller = new ControllerModel(typeof(ConventionsController).GetTypeInfo(), + new List() { controllerConvention.Object }); + controller.Application = applicationModel; + applicationModel.Controllers.Add(controller); - var controller = model.Controllers.Single(); - model.Controllers.Remove(controller); - controller = BuildControllerModel(controller, controllerConvention.Object); - model.Controllers.Add(controller); + var methodInfo = typeof(ConventionsController).GetMethod("Create"); + var actionModel = new ActionModel(methodInfo, new List() { actionConvention.Object }); + actionModel.Controller = controller; + controller.Actions.Add(actionModel); - var action = controller.Actions.Single(); - controller.Actions.Remove(action); - action = BuildActionModel(action, actionConvention.Object); - controller.Actions.Add(action); - - var parameter = action.Parameters.Single(); - action.Parameters.Remove(parameter); - parameter = BuildParameterModel(parameter, parameterConvention.Object); - action.Parameters.Add(parameter); + var parameterInfo = actionModel.ActionMethod.GetParameters().Single(); + var parameterModel = new ParameterModel(parameterInfo, + new List() { parameterConvention.Object }); + parameterModel.Action = actionModel; + actionModel.Parameters.Add(parameterModel); // Act - ApplicationModelConventions.ApplyConventions(model, options.Options.ApplicationModelConventions); + ApplicationModelConventions.ApplyConventions(applicationModel, options.Options.ApplicationModelConventions); // Assert Assert.Equal(4, sequence); } - + [Fact] public void BuildModel_SplitsConstraintsBasedOnRoute() { @@ -1306,33 +1305,6 @@ namespace Microsoft.AspNet.Mvc.Test Assert.Single(action.ActionConstraints, a => a is ConstraintAttribute); } - private ActionModel BuildActionModel(ActionModel action, IActionModelConvention actionConvention) - { - var t = new ActionModel(action.ActionMethod, - new List(action.Attributes) { actionConvention }); - t.Parameters.AddRange(action.Parameters); - - return t; - } - - private ParameterModel BuildParameterModel(ParameterModel parameter, IParameterModelConvention parameterConvention) - { - var t = new ParameterModel(parameter.ParameterInfo, - new List(parameter.Attributes) { parameterConvention }); - t.Action = parameter.Action; - - return t; - } - - private ControllerModel BuildControllerModel(ControllerModel controller, IControllerModelConvention controllerConvention) - { - var t = new ControllerModel(controller.ControllerType, - new List(controller.Attributes) { controllerConvention }); - t.Actions.AddRange(controller.Actions); - - return t; - } - private ControllerActionDescriptorProvider GetProvider( TypeInfo controllerTypeInfo, IEnumerable filters = null)