4

I implemented the 2nd response to the "Preserve ModelState Errors Across RedirectToAction?" question which involves using two custom ActionFilterAttributes. I like the solution, it keeps the code clean by just adding an attribute to methods that need the functionality.

The solution works well in most cases, but I've run across an issue with a repeating Partial View. Basically I have Partial View that uses it's own Model, separate from the Model that parent view uses.

The simplified version of my code from the main View:

@for (int i = 0; i < Model.Addresses.Count; i++)
{
        address = (Address)Model.Addresses[i];
        @Html.Partial("_AddressModal", address);
}

The Partial View "_AddressModal":

@model Acme.Domain.Models.Address
[...]
@Html.TextBoxFor(model => model.Address1, new { @class = "form-control" } )
[...]

When not using the custom ActionFilterAttributes everything works as expected. With each execution of the Partial View, the lamba expressions such "model => model.Address1" pull the correct value from ModelState.

The problem is when I get redirect and have used the custom ActionFilterAttributes. The core problem is that not only is the ModelState for the one instance of Address updated, but the ModelState of all the Addresses built by the Partial View get overwritten so they contain the same values, instead of the correct instance values.

My question is how do I modify the custom ActionFilterAttributes so that it only updates the ModelState of the one affected Address instance, not all the ModelStates? I want to avoid adding anything to the methods that use the attribute, to keep the clean implementation.

Here is the custom ActionFilterAttributes code from the other question:

public class SetTempDataModelStateAttribute : ActionFilterAttribute
{
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        base.OnActionExecuted(filterContext);         
        filterContext.Controller.TempData["ModelState"] = 
           filterContext.Controller.ViewData.ModelState;
    }
}

public class RestoreModelStateFromTempDataAttribute : ActionFilterAttribute
{
    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        base.OnActionExecuting(filterContext);
        if (filterContext.Controller.TempData.ContainsKey("ModelState"))
        {
            filterContext.Controller.ViewData.ModelState.Merge(
                (ModelStateDictionary)filterContext.Controller.TempData["ModelState"]);
        }
    }
}
Community
  • 1
  • 1
Josh
  • 8,219
  • 13
  • 76
  • 123
  • Based on [your previous question](http://stackoverflow.com/questions/28347792/asp-net-mvc-html-editor-pass-model-to-editor-template) I think you are misunderstanding the use of `EditorTemplate`'s and partials. You cannot use a partial view that way to render a collection - inspect the html and you will see the duplicate name and id attributes so the collection cannot be bound properly on post back anyway. –  Feb 07 '15 at 00:42
  • @StephenMuecke - if you have a better approach to achieve the same UX goal I'm all ears. I've posted the question at: http://stackoverflow.com/questions/28386959/asp-net-mvc-best-ux-approach-to-allowing-child-object-editing – Josh Feb 07 '15 at 20:25

1 Answers1

3

Check out if this implementation (ben foster) does work : I used it heavily and never had a problem.

Are you setting the attributes correctly ? ' RestoreModelStateFromTempDataAttribute on the get action and SetTempDataModelState on your post action ?

Here are the 4 classes needed (Export, Import, Transfer and Validate)ModelState

 [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class ExportModelStateToTempDataAttribute : ModelStateTempDataTransfer
    {
        public override void OnActionExecuted(ActionExecutedContext filterContext)
        {
            // Only copy when ModelState is invalid and we're performing a Redirect (i.e. PRG)
            if (!filterContext.Controller.ViewData.ModelState.IsValid &&
                (filterContext.Result is RedirectResult || filterContext.Result is RedirectToRouteResult)) 
            {
                ExportModelStateToTempData(filterContext);
            }

            base.OnActionExecuted(filterContext);
        }
    }


 /// <summary>
    /// An Action Filter for importing ModelState from TempData.
    /// You need to decorate your GET actions with this when using the <see cref="ValidateModelStateAttribute"/>.
    /// </summary>
    /// <remarks>
    /// Useful when following the PRG (Post, Redirect, Get) pattern.
    /// </remarks>
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class ImportModelStateFromTempDataAttribute : ModelStateTempDataTransfer
    {
        public override void OnActionExecuted(ActionExecutedContext filterContext)
        {
            // Only copy from TempData if we are rendering a View/Partial
            if (filterContext.Result is ViewResult)
            {
                ImportModelStateFromTempData(filterContext);
            }
            else 
            {
                // remove it
                RemoveModelStateFromTempData(filterContext);
            }

            base.OnActionExecuted(filterContext);
        }
    }

 [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public abstract class ModelStateTempDataTransfer : ActionFilterAttribute
    {
        protected static readonly string Key = typeof(ModelStateTempDataTransfer).FullName;

        /// <summary>
        /// Exports the current ModelState to TempData (available on the next request).
        /// </summary>       
        protected static void ExportModelStateToTempData(ControllerContext context)
        {
            context.Controller.TempData[Key] = context.Controller.ViewData.ModelState;
        }

        /// <summary>
        /// Populates the current ModelState with the values in TempData
        /// </summary>
        protected static void ImportModelStateFromTempData(ControllerContext context)
        {
            var prevModelState = context.Controller.TempData[Key] as ModelStateDictionary;
            context.Controller.ViewData.ModelState.Merge(prevModelState);
        }

        /// <summary>
        /// Removes ModelState from TempData
        /// </summary>
        protected static void RemoveModelStateFromTempData(ControllerContext context)
        {
            context.Controller.TempData[Key] = null;
        }
    }

  /// <summary>
    /// An ActionFilter for automatically validating ModelState before a controller action is executed.
    /// Performs a Redirect if ModelState is invalid. Assumes the <see cref="ImportModelStateFromTempDataAttribute"/> is used on the GET action.
    /// </summary>
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
    public class ValidateModelStateAttribute : ModelStateTempDataTransfer
    {
        public override void OnActionExecuting(ActionExecutingContext filterContext)
        {
            if (!filterContext.Controller.ViewData.ModelState.IsValid)
            {
                if (filterContext.HttpContext.Request.IsAjaxRequest())
                {
                    ProcessAjax(filterContext);
                }
                else
                {
                    ProcessNormal(filterContext);
                }
            }

            base.OnActionExecuting(filterContext);
        }

        protected virtual void ProcessNormal(ActionExecutingContext filterContext)
        {
            // Export ModelState to TempData so it's available on next request
            ExportModelStateToTempData(filterContext);

            // redirect back to GET action
            filterContext.Result = new RedirectToRouteResult(filterContext.RouteData.Values);
        }

        protected virtual void ProcessAjax(ActionExecutingContext filterContext)
        {
            var errors = filterContext.Controller.ViewData.ModelState.ToSerializableDictionary();
            var json = new JavaScriptSerializer().Serialize(errors);

            // send 400 status code (Bad Request)
            filterContext.Result = new HttpStatusCodeResult((int)HttpStatusCode.BadRequest, json);
        }
    }

EDIT

This is a normal (non action filter) PRG pattern :

    [HttpGet]
    public async Task<ActionResult> Edit(Guid id)
    {
        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (calendarEvent == null) return this.RedirectToAction<CalendarController>(c => c.Index());
        var model = new CalendarEditViewModel(calendarEvent);
        ViewData.Model = model;
        return View();
    }

    [HttpPost]
    public async Task<ActionResult> Edit(Guid id, CalendarEventBindingModel binding)
    {
        if (!ModelState.IsValid) return await Edit(id);

        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (calendarEvent != null)
        {
            CalendarEvent model = calendarService.Update(calendarEvent, binding);
            await context.SaveChangesAsync();
        }
        return this.RedirectToAction<CalendarController>(c => c.Index());
    }

What do you want to avoid with action filters (or their purpose) is to remove the ModelState.IsValid check on every post Action, so the same (with action filters) would be :

    [HttpGet, ImportModelStateFromTempData]
    public async Task<ActionResult> Edit(Guid id)
    {
        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (calendarEvent == null) return this.RedirectToAction<CalendarController>(c => c.Index());
        var model = new CalendarEditViewModel(calendarEvent);
        ViewData.Model = model;
        return View();
    }

    // ActionResult changed to RedirectToRouteResult
    [HttpPost, ValidateModelState]
    public async Task<RedirectToRouteResult> Edit(Guid id, CalendarEventBindingModel binding)
    {
        // removed ModelState.IsValid check
        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (calendarEvent != null)
        {
            CalendarEvent model = calendarService.Update(calendarEvent, binding);
            await context.SaveChangesAsync();
        }
        return this.RedirectToAction<CalendarController>(c => c.Index());
    }

there's no much more happening here. So, if you only use ExportModelState action filter, you will end up with a post action like this :

    [HttpPost, ExportModelStateToTempData]
    public async Task<RedirectToRouteResult> Edit(Guid id, CalendarEventBindingModel binding)
    {
        if (!ModelState.IsValid) return RedirectToAction("Edit", new { id });
        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (calendarEvent != null)
        {
            CalendarEvent model = calendarService.Update(calendarEvent, binding);
            await context.SaveChangesAsync();
        }
        return this.RedirectToAction<CalendarController>(c => c.Index());
    }

Which makes me ask you, why you even bother with ActionFilters in the first place ? While I do like the ValidateModelState pattern, (lots of people doesn't), I don't really see any benefit if you are redirecting in your controller except for one scenario, where you have additional modelstate errors, for completeness let me give you an example:

    [HttpPost, ValidateModelState, ExportModelStateToTempData]
    public async Task<RedirectToRouteResult> Edit(Guid id, CalendarEventBindingModel binding)
    {

        var calendarEvent = await calendarService.FindByIdAsync(id);
        if (!(calendarEvent.DateStart > DateTime.UtcNow.AddDays(7))
            && binding.DateStart != calendarEvent.DateStart)
        {
            ModelState.AddModelError("id", "Sorry, Date start cannot be updated with less than 7 days of event.");
            return RedirectToAction("Edit", new { id });
        }
        if (calendarEvent != null)
        {
            CalendarEvent model = calendarService.Update(calendarEvent, binding);
            await context.SaveChangesAsync();
        }
        return this.RedirectToAction<CalendarController>(c => c.Index());
    }

In the last example, I used both ValidateModelState and ExportModelState, this is because ValidateModelState runs on ActionExecuting so it validates before entering the body of the method, if the binding have some validation error it will redirect automatically. Then I have another check that can't be in the data annotations because it deals with loading an entity and seeing if it has the correct requirements (I know is not the best example, think of it as looking if provided username is available when registration, I know about Remote data annotation but doesn't cover all cases) then I just update the ModelState with my own errors depending on external factors others than the binding. Since ExportModelState runs on ActionExecuted, all my modifications to ModelState are persisted on TempData so I will have them on HttpGet Edit action.

I know all this can confuse some of us, there are no really good indications on how to do MVC on the Controller / PRG side. I was thinking hard in making a blog post to cover all the scenarios and solutions. This is only the 1% of it.

I hope at least I cleared few key points of the POST - GET workflow. If this confuses more than it helps, please let me know. Sorry for the long post.

I wanted to note also that there is ONE subtle difference in the PRG returning an ActionResult that the ones returning a RedirectToRouteResult. If you refresh the page (F5) after having a ValidationError, with the RedirectToRouteResult, the errors will NOT be persisted and you get a clean view as if you entered for the first time. With the ActionResult ones, you refresh and see the exact same page including the errors. This has nothing to do with the ActionResult or RedirectToRouteResult return types, its because in one scenario you redirect always on POST while the other you redirect only on success POST. PRG does not suggest to blinding redirect on unsucessfully POST, yet some people prefer to do redirect on every post, which requires TempData transfer.

Bart Calixto
  • 19,210
  • 11
  • 78
  • 114
  • Yes, I'm setting correctly the RestoreModelStateFromTempDataAttribute on the get action and SetTempDataModelState on your post action. I'll try this now. – Josh Feb 06 '15 at 20:06
  • Ended up with the same problem, it basically updates all Address ModelStates on so they all end up with the same data. So I'm stuck with clearing out ModelState (in my workaround above), unless I can figure out how to just target the correct ModelState. – Josh Feb 06 '15 at 21:50
  • I found the origin of problem why even when it should have been valid all the Address objects were contained the same data. I had a child object called Address.AddressType that had a required property called Name. It wasn't something an edited so it wasn't populated as part of the post.This caused a validation error because it was a required field, so I ended up passing it as a hidden value so that the parent Address object could pass validation. – Josh Feb 07 '15 at 19:40
  • I uncovered this because I noticed in the code you attached it was only writing to TempData when the object was invalid, but since every call was invalid it was writing everything to TempData. So I've merged the code you posted with mine and it cleared up the problem once I passed the required field. Since I'm using the code you sent, and it help figure out the problem I consider that an acceptable answer. The only part of the code I don't like (which I didn't use) was the redirect logic. I prefer to have that in the controller. – Josh Feb 07 '15 at 19:41
  • If you don't want the redirect, instead of using ValidateModelState you can use ExportModelState in your post action and then you can redirect as you wish. Remember if you don't use the redirect, you should also check if (Model.IsValid). – Bart Calixto Feb 07 '15 at 20:30
  • that is basically what I did. ExportModelState has an if that checks !filterContext.Controller.ViewData.ModelState.IsValid -- it is checking if it is valid. – Josh Feb 07 '15 at 22:01
  • @Josh yes, but If you want to redirect on controller, you must redirect if the modelstate is not valid. Even if exportmodelstate has this check you must check to decide to redirect or not inside the controller. I don't think it's a good idea because everytime your modelstate is not valid you would redirect to the get action, that's the whole point of the action filters. If you want to only do the modelstate transfer I would create a simple class that does exactly that. Let me edit my answer to show you the workflow with or without so I can express more clear. – Bart Calixto Feb 08 '15 at 00:58
  • Useful answer, but when I encounter a validation error my form data is lost... Which attribute combination do I use so that I can just "return View(viewModel)" so that I don't lose the data? – Diana Aug 04 '17 at 12:51