From 9ddee615cc8ba9be3de7333f2d7211e906821304 Mon Sep 17 00:00:00 2001 From: Floris Robbemont Date: Tue, 1 Oct 2013 14:09:38 +0200 Subject: [PATCH 1/2] U4-3007 RouteDefinition should not store instance of controller --- src/Umbraco.Web/Mvc/RouteDefinition.cs | 7 +--- src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs | 35 +---------------- src/Umbraco.Web/Mvc/UmbracoPageResult.cs | 49 +++++++++++++++--------- 3 files changed, 33 insertions(+), 58 deletions(-) diff --git a/src/Umbraco.Web/Mvc/RouteDefinition.cs b/src/Umbraco.Web/Mvc/RouteDefinition.cs index 49ba64fac5..f0334202dd 100644 --- a/src/Umbraco.Web/Mvc/RouteDefinition.cs +++ b/src/Umbraco.Web/Mvc/RouteDefinition.cs @@ -11,12 +11,7 @@ namespace Umbraco.Web.Mvc { public string ControllerName { get; set; } public string ActionName { get; set; } - - /// - /// The Controller instance found for routing to - /// - public ControllerBase Controller { get; set; } - + /// /// The Controller type found for routing to /// diff --git a/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs b/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs index 649ac54f27..88cc42a8f1 100644 --- a/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs +++ b/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs @@ -25,46 +25,13 @@ namespace Umbraco.Web.Mvc : base(requestContext) { } - - private void StoreControllerInRouteDefinition() - { - var routeDef = (RouteDefinition)RequestContext.RouteData.DataTokens["umbraco-route-def"]; - - if (routeDef == null) return; - - // Get the factory and controller and create a new instance of the controller - var factory = ControllerBuilder.Current.GetControllerFactory(); - var controller = factory.CreateController(RequestContext, routeDef.ControllerName) as ControllerBase; - - // Store the controller - routeDef.Controller = controller; - } - + /// /// This is used internally purely to render an Umbraco MVC template to string and shouldn't be used for anything else. /// internal void ExecuteUmbracoRequest() { - StoreControllerInRouteDefinition(); base.ProcessRequest(RequestContext.HttpContext); } - - protected override void ProcessRequest(HttpContextBase httpContext) - { - StoreControllerInRouteDefinition(); - - // Let MVC do its magic and continue the request - base.ProcessRequest(httpContext); - } - - protected override IAsyncResult BeginProcessRequest(HttpContextBase httpContext, AsyncCallback callback, - object state) - { - StoreControllerInRouteDefinition(); - - return base.BeginProcessRequest(httpContext, callback, state); - } } - - } \ No newline at end of file diff --git a/src/Umbraco.Web/Mvc/UmbracoPageResult.cs b/src/Umbraco.Web/Mvc/UmbracoPageResult.cs index db0f352645..2ae32e6ad5 100644 --- a/src/Umbraco.Web/Mvc/UmbracoPageResult.cs +++ b/src/Umbraco.Web/Mvc/UmbracoPageResult.cs @@ -11,8 +11,7 @@ namespace Umbraco.Web.Mvc { public override void ExecuteResult(ControllerContext context) { - - //since we could be returning the current page from a surface controller posted values in which the routing values are changed, we + //since we could be returning the current page from a surface controller posted values in which the routing values are changed, we //need to revert these values back to nothing in order for the normal page to render again. context.RouteData.DataTokens["area"] = null; context.RouteData.DataTokens["Namespaces"] = null; @@ -24,30 +23,44 @@ namespace Umbraco.Web.Mvc } var routeDef = (RouteDefinition)context.RouteData.DataTokens["umbraco-route-def"]; + var factory = ControllerBuilder.Current.GetControllerFactory(); //ensure the original template is reset context.RouteData.Values["action"] = routeDef.ActionName; //ensure ModelState is copied across - routeDef.Controller.ViewData.ModelState.Merge(context.Controller.ViewData.ModelState); + ControllerBase controller = null; - //ensure TempData and ViewData is copied across - foreach (var d in context.Controller.ViewData) - routeDef.Controller.ViewData[d.Key] = d.Value; - routeDef.Controller.TempData = context.Controller.TempData; + try + { + controller = factory.CreateController(context.RequestContext, routeDef.ControllerName) as ControllerBase; - using (DisposableTimer.TraceDuration("Executing Umbraco RouteDefinition controller", "Finished")) - { - try - { - ((IController)routeDef.Controller).Execute(context.RequestContext); - } - finally - { - routeDef.Controller.DisposeIfDisposable(); - } - } + if (controller == null) + { + throw new InvalidOperationException("Could not create controller with name " + routeDef.ControllerName + " in the context of an Http POST when using a SurfaceController form"); + } + controller.ViewData.ModelState.Merge(context.Controller.ViewData.ModelState); + + //ensure TempData and ViewData is copied across + foreach (var d in context.Controller.ViewData) + controller.ViewData[d.Key] = d.Value; + + controller.TempData = context.Controller.TempData; + + using (DisposableTimer.TraceDuration("Executing Umbraco RouteDefinition controller", "Finished")) + { + ((IController)controller).Execute(context.RequestContext); + } + } + finally + { + if (controller != null) + factory.ReleaseController(controller); + + if (controller != null) + controller.DisposeIfDisposable(); + } } } } \ No newline at end of file From b02657f32c3aede1e2118810f79c96bcde43a723 Mon Sep 17 00:00:00 2001 From: Floris Robbemont Date: Sat, 5 Oct 2013 23:22:17 +0200 Subject: [PATCH 2/2] Cleaning up commit for issue U4-3007 Changed documentation on UmbracoMvcHandler --- src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs | 19 ++-- src/Umbraco.Web/Mvc/UmbracoPageResult.cs | 111 ++++++++++++++++------- 2 files changed, 85 insertions(+), 45 deletions(-) diff --git a/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs b/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs index 88cc42a8f1..6e7a92ea10 100644 --- a/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs +++ b/src/Umbraco.Web/Mvc/UmbracoMvcHandler.cs @@ -1,23 +1,18 @@ -using System; -using System.Collections; -using System.Collections.Generic; -using System.IO; -using System.Linq; -using System.Text; -using System.Web; -using System.Web.Mvc; +using System.Web.Mvc; using System.Web.Routing; namespace Umbraco.Web.Mvc { /// - /// Mvc handler class to intercept creation of controller and store it for later use. - /// This means we create two instances of the same controller to support some features later on. + /// MVC handler to facilitate the TemplateRenderer. This handler can execute an MVC request and return it as a string. /// - /// The alternate option for this is to completely rewrite all MvcHandler methods. + /// Original: /// - /// This is currently needed for the 'return CurrentUmbracoPage()' surface controller functionality + /// This handler also used to intercept creation of controllers and store it for later use. + /// This was needed for the 'return CurrentUmbracoPage()' surface controller functionality /// because it needs to send data back to the page controller. + /// + /// The creation of this controller has been moved to the UmbracoPageResult class which will create a controller when needed. /// internal class UmbracoMvcHandler : MvcHandler { diff --git a/src/Umbraco.Web/Mvc/UmbracoPageResult.cs b/src/Umbraco.Web/Mvc/UmbracoPageResult.cs index 2ae32e6ad5..6b44959c8c 100644 --- a/src/Umbraco.Web/Mvc/UmbracoPageResult.cs +++ b/src/Umbraco.Web/Mvc/UmbracoPageResult.cs @@ -1,5 +1,6 @@ using System; using System.Web.Mvc; +using System.Web.Routing; using Umbraco.Core; namespace Umbraco.Web.Mvc @@ -11,56 +12,100 @@ namespace Umbraco.Web.Mvc { public override void ExecuteResult(ControllerContext context) { - //since we could be returning the current page from a surface controller posted values in which the routing values are changed, we - //need to revert these values back to nothing in order for the normal page to render again. - context.RouteData.DataTokens["area"] = null; - context.RouteData.DataTokens["Namespaces"] = null; + ResetRouteData(context.RouteData); - //validate that the current page execution is not being handled by the normal umbraco routing system - if (!context.RouteData.DataTokens.ContainsKey("umbraco-route-def")) - { - throw new InvalidOperationException("Can only use " + typeof(UmbracoPageResult).Name + " in the context of an Http POST when using a SurfaceController form"); - } + ValidateRouteData(context.RouteData); var routeDef = (RouteDefinition)context.RouteData.DataTokens["umbraco-route-def"]; var factory = ControllerBuilder.Current.GetControllerFactory(); - //ensure the original template is reset context.RouteData.Values["action"] = routeDef.ActionName; - //ensure ModelState is copied across ControllerBase controller = null; try { - controller = factory.CreateController(context.RequestContext, routeDef.ControllerName) as ControllerBase; - - if (controller == null) - { - throw new InvalidOperationException("Could not create controller with name " + routeDef.ControllerName + " in the context of an Http POST when using a SurfaceController form"); - } - + controller = CreateController(context, factory, routeDef); + controller.ViewData.ModelState.Merge(context.Controller.ViewData.ModelState); - //ensure TempData and ViewData is copied across - foreach (var d in context.Controller.ViewData) - controller.ViewData[d.Key] = d.Value; + CopyControllerData(context, controller); - controller.TempData = context.Controller.TempData; - - using (DisposableTimer.TraceDuration("Executing Umbraco RouteDefinition controller", "Finished")) - { - ((IController)controller).Execute(context.RequestContext); - } + ExecuteControllerAction(context, controller); } - finally + finally { - if (controller != null) - factory.ReleaseController(controller); - - if (controller != null) - controller.DisposeIfDisposable(); + CleanupController(controller, factory); } } + + /// + /// Executes the controller action + /// + private static void ExecuteControllerAction(ControllerContext context, IController controller) + { + using (DisposableTimer.TraceDuration("Executing Umbraco RouteDefinition controller", "Finished")) + { + controller.Execute(context.RequestContext); + } + } + + /// + /// Since we could be returning the current page from a surface controller posted values in which the routing values are changed, we + /// need to revert these values back to nothing in order for the normal page to render again. + /// + private static void ResetRouteData(RouteData routeData) + { + routeData.DataTokens["area"] = null; + routeData.DataTokens["Namespaces"] = null; + } + + /// + /// Validate that the current page execution is not being handled by the normal umbraco routing system + /// + private static void ValidateRouteData(RouteData routeData) + { + if (!routeData.DataTokens.ContainsKey("umbraco-route-def")) + { + throw new InvalidOperationException("Can only use " + typeof(UmbracoPageResult).Name + + " in the context of an Http POST when using a SurfaceController form"); + } + } + + /// + /// Ensure TempData and ViewData is copied across + /// + private static void CopyControllerData(ControllerContext context, ControllerBase controller) + { + foreach (var d in context.Controller.ViewData) + controller.ViewData[d.Key] = d.Value; + + controller.TempData = context.Controller.TempData; + } + + /// + /// Creates a controller using the controller factory + /// + private static ControllerBase CreateController(ControllerContext context, IControllerFactory factory, RouteDefinition routeDef) + { + var controller = factory.CreateController(context.RequestContext, routeDef.ControllerName) as ControllerBase; + + if (controller == null) + throw new InvalidOperationException("Could not create controller with name " + routeDef.ControllerName + "."); + + return controller; + } + + /// + /// Cleans up the controller by releasing it using the controller factory, and by disposing it. + /// + private static void CleanupController(IController controller, IControllerFactory factory) + { + if (controller != null) + factory.ReleaseController(controller); + + if (controller != null) + controller.DisposeIfDisposable(); + } } } \ No newline at end of file