From 04b3532e9de9bebff3a21b2ccd6db874e8c248a7 Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 22 Feb 2016 18:38:06 +0100 Subject: [PATCH 1/5] U4-8030 - Fix RenderModelBinder issue with surface --- src/Umbraco.Web/Mvc/RenderModelBinder.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/Umbraco.Web/Mvc/RenderModelBinder.cs b/src/Umbraco.Web/Mvc/RenderModelBinder.cs index 32802ccadc..ed07275378 100644 --- a/src/Umbraco.Web/Mvc/RenderModelBinder.cs +++ b/src/Umbraco.Web/Mvc/RenderModelBinder.cs @@ -23,6 +23,19 @@ namespace Umbraco.Web.Mvc if (controllerContext.RouteData.DataTokens.TryGetValue(Core.Constants.Web.UmbracoDataToken, out model) == false) return null; + // when rendering "special" stuff such as surface controllers, etc, the token does *not* contain + // the model source, but some special strings - and then we have to find the source using the + // "default" MVC way + var modelString = model as string; + if (modelString == "surface" || modelString == "api" || modelString == "backoffice") // fixme - more? + { + var value = bindingContext.ValueProvider.GetValue(bindingContext.ModelName); + model = value.RawValue; + + // fixme - should we return here? + // or go with the binding logic below that's nicer with strongly typed models + } + //default culture var culture = CultureInfo.CurrentCulture; From 9c121f0044b2aab9d59ccc0c384e2e3b290e9e9a Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 7 Mar 2016 16:26:30 +0100 Subject: [PATCH 2/5] Fixes RenderModelBinder to inherit from DefaultModelBinder and use the default logic if the model in the route values is not IRenderModel --- src/Umbraco.Web/Mvc/RenderModelBinder.cs | 35 ++++++++++++++---------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Web/Mvc/RenderModelBinder.cs b/src/Umbraco.Web/Mvc/RenderModelBinder.cs index ed07275378..d219a438e0 100644 --- a/src/Umbraco.Web/Mvc/RenderModelBinder.cs +++ b/src/Umbraco.Web/Mvc/RenderModelBinder.cs @@ -8,7 +8,10 @@ using Umbraco.Web.Models; namespace Umbraco.Web.Mvc { - public class RenderModelBinder : IModelBinder, IModelBinderProvider + /// + /// Allows for Model Binding any IPublishedContent or IRenderModel + /// + public class RenderModelBinder : DefaultModelBinder, IModelBinder, IModelBinderProvider { /// /// Binds the model to a value by using the specified controller context and binding context. @@ -17,27 +20,31 @@ namespace Umbraco.Web.Mvc /// The bound value. /// /// The controller context.The binding context. - public object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext) + public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext) { object model; if (controllerContext.RouteData.DataTokens.TryGetValue(Core.Constants.Web.UmbracoDataToken, out model) == false) return null; - // when rendering "special" stuff such as surface controllers, etc, the token does *not* contain - // the model source, but some special strings - and then we have to find the source using the - // "default" MVC way - var modelString = model as string; - if (modelString == "surface" || modelString == "api" || modelString == "backoffice") // fixme - more? + //This model binder deals with IRenderModel and IPublishedContent by extracting the model from the route's + // datatokens. This data token is set in 2 places: RenderRouteHandler, UmbracoVirtualNodeRouteHandler + // and both always set the model to an instance of `RenderModel`. So if this isn't an instance of IRenderModel then + // we need to let the DefaultModelBinder deal with the logic. + var renderModel = model as IRenderModel; + if (renderModel == null) { var value = bindingContext.ValueProvider.GetValue(bindingContext.ModelName); + if (value == null) return null; + model = value.RawValue; + } - // fixme - should we return here? - // or go with the binding logic below that's nicer with strongly typed models - } + //if for any reason the model is not either IRenderModel or IPublishedContent, then we return since those are the only + // types this binder is dealing with. + if ((model is IRenderModel) == false && (model is IPublishedContent) == false) return null; - //default culture - var culture = CultureInfo.CurrentCulture; + //default culture + var culture = CultureInfo.CurrentCulture; var umbracoContext = controllerContext.GetUmbracoContext() ?? UmbracoContext.Current; @@ -47,8 +54,8 @@ namespace Umbraco.Web.Mvc culture = umbracoContext.PublishedContentRequest.Culture; } - return BindModel(model, bindingContext.ModelType, culture); - } + return BindModel(model, bindingContext.ModelType, culture); + } // source is the model that we have // modelType is the type of the model that we need to bind to From 5f365241a8410a7692517cff155f71347e78537b Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 7 Mar 2016 16:29:40 +0100 Subject: [PATCH 3/5] updates RenderModelBinder to simply check for IPublishedContent or IRenderModel --- src/Umbraco.Web/Mvc/RenderModelBinder.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Web/Mvc/RenderModelBinder.cs b/src/Umbraco.Web/Mvc/RenderModelBinder.cs index d219a438e0..49bb4e3026 100644 --- a/src/Umbraco.Web/Mvc/RenderModelBinder.cs +++ b/src/Umbraco.Web/Mvc/RenderModelBinder.cs @@ -160,15 +160,9 @@ namespace Umbraco.Web.Mvc public IModelBinder GetBinder(Type modelType) { - // can bind to RenderModel - if (modelType == typeof(RenderModel)) return this; - - // can bind to RenderModel - if (modelType.IsGenericType && modelType.GetGenericTypeDefinition() == typeof(RenderModel<>)) return this; - - // can bind to TContent where TContent : IPublishedContent - if (typeof(IPublishedContent).IsAssignableFrom(modelType)) return this; - return null; + return TypeHelper.IsTypeAssignableFrom(modelType) || TypeHelper.IsTypeAssignableFrom(modelType) + ? this + : null; } } } \ No newline at end of file From d5a57c505cfcc2f2b5fb2f122c9e3fd0eccae59e Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 7 Mar 2016 16:53:57 +0100 Subject: [PATCH 4/5] adds RenderModelBinderTests --- src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + .../Web/Mvc/RenderModelBinderTests.cs | 84 +++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index e553be9b49..83a7ec903b 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -244,6 +244,7 @@ + diff --git a/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs b/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs new file mode 100644 index 0000000000..5c2c1390fa --- /dev/null +++ b/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs @@ -0,0 +1,84 @@ +using System.Globalization; +using Moq; +using NUnit.Framework; +using Umbraco.Core.Models; +using Umbraco.Core.Models.PublishedContent; +using Umbraco.Web.Models; +using Umbraco.Web.Mvc; + +namespace Umbraco.Tests.Web.Mvc +{ + [TestFixture] + public class RenderModelBinderTests + { + [Test] + public void Returns_Binder_For_IPublishedContent_And_IRenderModel() + { + var binder = new RenderModelBinder(); + var found = binder.GetBinder(typeof (IPublishedContent)); + Assert.IsNotNull(found); + found = binder.GetBinder(typeof(IRenderModel)); + Assert.IsNotNull(found); + found = binder.GetBinder(typeof(RenderModel)); + Assert.IsNotNull(found); + found = binder.GetBinder(typeof(DynamicPublishedContent)); + Assert.IsNotNull(found); + found = binder.GetBinder(typeof(MyContent)); + Assert.IsNotNull(found); + + found = binder.GetBinder(typeof(MyOtherContent)); + Assert.IsNull(found); + } + + [Test] + public void BindModel_Null_Source_Returns_Null() + { + Assert.IsNull(RenderModelBinder.BindModel(null, typeof(MyContent), CultureInfo.CurrentCulture)); + } + + [Test] + public void BindModel_Returns_If_Same_Type() + { + var content = new MyContent(Mock.Of()); + var bound = RenderModelBinder.BindModel(content, typeof (IPublishedContent), CultureInfo.CurrentCulture); + Assert.AreSame(content, bound); + } + + [Test] + public void BindModel_RenderModel_To_IPublishedContent() + { + var content = new MyContent(Mock.Of()); + var renderModel = new RenderModel(content, CultureInfo.CurrentCulture); + var bound = RenderModelBinder.BindModel(renderModel, typeof(IPublishedContent), CultureInfo.CurrentCulture); + Assert.AreSame(content, bound); + } + + [Test] + public void BindModel_IPublishedContent_To_RenderModel() + { + var content = new MyContent(Mock.Of()); + var bound = (IRenderModel)RenderModelBinder.BindModel(content, typeof(RenderModel), CultureInfo.CurrentCulture); + Assert.AreSame(content, bound.Content); + } + + [Test] + public void BindModel_IPublishedContent_To_Generic_RenderModel() + { + var content = new MyContent(Mock.Of()); + var bound = (IRenderModel)RenderModelBinder.BindModel(content, typeof(RenderModel), CultureInfo.CurrentCulture); + Assert.AreSame(content, bound.Content); + } + + public class MyOtherContent + { + + } + + public class MyContent : PublishedContentWrapped + { + public MyContent(IPublishedContent content) : base(content) + { + } + } + } +} \ No newline at end of file From b9765c3bec173c4431a45ffa7bd8a68ba9684b40 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 7 Mar 2016 17:52:28 +0100 Subject: [PATCH 5/5] fixes RenderModelBinder to use base implementation,adds unit tests --- .../Web/Mvc/RenderModelBinderTests.cs | 73 +++++++++++++++++++ src/Umbraco.Web/Mvc/RenderModelBinder.cs | 6 +- 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs b/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs index 5c2c1390fa..5ec039a4dd 100644 --- a/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/RenderModelBinderTests.cs @@ -1,4 +1,8 @@ +using System.Collections.Generic; using System.Globalization; +using System.Web; +using System.Web.Mvc; +using System.Web.Routing; using Moq; using NUnit.Framework; using Umbraco.Core.Models; @@ -69,6 +73,75 @@ namespace Umbraco.Tests.Web.Mvc Assert.AreSame(content, bound.Content); } + [Test] + public void No_DataToken_Returns_Null() + { + var binder = new RenderModelBinder(); + var routeData = new RouteData(); + var result = binder.BindModel(new ControllerContext(Mock.Of(), routeData, Mock.Of()), + new ModelBindingContext()); + + Assert.IsNull(result); + } + + [Test] + public void Invalid_DataToken_Model_Type_Returns_Null() + { + var binder = new RenderModelBinder(); + var routeData = new RouteData(); + routeData.DataTokens[Core.Constants.Web.UmbracoDataToken] = "hello"; + + //the value provider is the default implementation + var valueProvider = new Mock(); + //also IUnvalidatedValueProvider + var invalidatedValueProvider = valueProvider.As(); + invalidatedValueProvider.Setup(x => x.GetValue(It.IsAny(), It.IsAny())).Returns(() => + new ValueProviderResult(null, "", CultureInfo.CurrentCulture)); + + var controllerCtx = new ControllerContext( + Mock.Of(http => http.Items == new Dictionary()), + routeData, + Mock.Of()); + + var result = binder.BindModel(controllerCtx, + new ModelBindingContext + { + ValueProvider = valueProvider.Object, + ModelMetadata = new ModelMetadata(new EmptyModelMetadataProvider(), null, () => null, typeof(IPublishedContent), "content") + }); + + Assert.IsNull(result); + } + + [Test] + public void IPublishedContent_DataToken_Model_Type_Uses_DefaultImplementation() + { + var content = new MyContent(Mock.Of()); + var binder = new RenderModelBinder(); + var routeData = new RouteData(); + routeData.DataTokens[Core.Constants.Web.UmbracoDataToken] = content; + + //the value provider is the default implementation + var valueProvider = new Mock(); + //also IUnvalidatedValueProvider + var invalidatedValueProvider = valueProvider.As(); + invalidatedValueProvider.Setup(x => x.GetValue(It.IsAny(), It.IsAny())).Returns(() => + new ValueProviderResult(content, "content", CultureInfo.CurrentCulture)); + + var controllerCtx = new ControllerContext( + Mock.Of(http => http.Items == new Dictionary()), + routeData, + Mock.Of()); + var result = binder.BindModel(controllerCtx, + new ModelBindingContext + { + ValueProvider = valueProvider.Object, + ModelMetadata = new ModelMetadata(new EmptyModelMetadataProvider(), null, () => null, typeof(IPublishedContent), "content") + }); + + Assert.AreEqual(content, result); + } + public class MyOtherContent { diff --git a/src/Umbraco.Web/Mvc/RenderModelBinder.cs b/src/Umbraco.Web/Mvc/RenderModelBinder.cs index 49bb4e3026..dd9af16140 100644 --- a/src/Umbraco.Web/Mvc/RenderModelBinder.cs +++ b/src/Umbraco.Web/Mvc/RenderModelBinder.cs @@ -33,10 +33,8 @@ namespace Umbraco.Web.Mvc var renderModel = model as IRenderModel; if (renderModel == null) { - var value = bindingContext.ValueProvider.GetValue(bindingContext.ModelName); - if (value == null) return null; - - model = value.RawValue; + model = base.BindModel(controllerContext, bindingContext); + if (model == null) return null; } //if for any reason the model is not either IRenderModel or IPublishedContent, then we return since those are the only