Fix issue with resolving Errors controller argument

The ErrorsMethodArgumentResolver expects the preceding @ModelAttribute
in the controller method signature to be the last one added in the
model -- an assumption that can break if a model attribute is added
earlier (e.g. through a @ModelAttribute method) and more attributes
are added as well. This fix ensures when an @ModelAttribute is resolved
as a controller method argument it has the highest index in the model.

Issue: SPR-9378

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java

@@ -17,10 +17,10 @@
 package org.springframework.web.method.annotation;
 
 import java.lang.annotation.Annotation;
+import java.util.Map;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.beans.BeanUtils;
 import org.springframework.core.MethodParameter;
 import org.springframework.core.annotation.AnnotationUtils;
@@ -31,7 +31,6 @@
 import org.springframework.web.bind.support.WebDataBinderFactory;
 import org.springframework.web.bind.support.WebRequestDataBinder;
 import org.springframework.web.context.request.NativeWebRequest;
-import org.springframework.web.method.annotation.ModelFactory;
 import org.springframework.web.method.support.HandlerMethodArgumentResolver;
 import org.springframework.web.method.support.HandlerMethodReturnValueHandler;
 import org.springframework.web.method.support.ModelAndViewContainer;
@@ -99,10 +98,10 @@ public final Object resolveArgument(
 			throws Exception {
 
 		String name = ModelFactory.getNameForParameter(parameter);
-		Object target = (mavContainer.containsAttribute(name)) ?
+		Object attribute = (mavContainer.containsAttribute(name)) ?
 				mavContainer.getModel().get(name) : createAttribute(name, parameter, binderFactory, request);
 
-		WebDataBinder binder = binderFactory.createBinder(request, target, name);
+		WebDataBinder binder = binderFactory.createBinder(request, attribute, name);
 		if (binder.getTarget() != null) {
 			bindRequestParameters(binder, request);
 			validateIfApplicable(binder, parameter);
@@ -113,7 +112,12 @@ public final Object resolveArgument(
 			}
 		}
 
-		mavContainer.addAllAttributes(binder.getBindingResult().getModel());
+		// Add resolved attribute and BindingResult at the end of the model
+
+		Map<String, Object> bindingResultModel = binder.getBindingResult().getModel();
+		mavContainer.removeAttributes(bindingResultModel);
+		mavContainer.addAllAttributes(bindingResultModel);
+
 		return binder.getTarget();
 	}
 

spring-web/src/main/java/org/springframework/web/method/support/ModelAndViewContainer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,29 +25,29 @@
 import org.springframework.web.bind.support.SimpleSessionStatus;
 
 /**
- * Records model and view related decisions made by 
- * {@link HandlerMethodArgumentResolver}s and 
- * {@link HandlerMethodReturnValueHandler}s during the course of invocation of 
+ * Records model and view related decisions made by
+ * {@link HandlerMethodArgumentResolver}s and
+ * {@link HandlerMethodReturnValueHandler}s during the course of invocation of
  * a controller method.
- * 
+ *
  * <p>The {@link #setRequestHandled} flag can be used to indicate the request
  * has been handled directly and view resolution is not required.
- * 
- * <p>A default {@link Model} is automatically created at instantiation. 
- * An alternate model instance may be provided via {@link #setRedirectModel} 
- * for use in a redirect scenario. When {@link #setRedirectModelScenario} is set 
- * to {@code true} signalling a redirect scenario, the {@link #getModel()} 
+ *
+ * <p>A default {@link Model} is automatically created at instantiation.
+ * An alternate model instance may be provided via {@link #setRedirectModel}
+ * for use in a redirect scenario. When {@link #setRedirectModelScenario} is set
+ * to {@code true} signalling a redirect scenario, the {@link #getModel()}
  * returns the redirect model instead of the default model.
- * 
+ *
  * @author Rossen Stoyanchev
  * @since 3.1
  */
 public class ModelAndViewContainer {
 
 	private Object view;
-	
+
 	private boolean requestHandled = false;
-	
+
 	private final ModelMap defaultModel = new BindingAwareModelMap();
 
 	private ModelMap redirectModel;
@@ -57,31 +57,31 @@ public class ModelAndViewContainer {
 	private boolean ignoreDefaultModelOnRedirect = false;
 
 	private final SessionStatus sessionStatus = new SimpleSessionStatus();
-	
+
 	/**
 	 * Create a new instance.
 	 */
 	public ModelAndViewContainer() {
 	}
 
 	/**
-	 * Set a view name to be resolved by the DispatcherServlet via a ViewResolver. 
+	 * Set a view name to be resolved by the DispatcherServlet via a ViewResolver.
 	 * Will override any pre-existing view name or View.
 	 */
 	public void setViewName(String viewName) {
 		this.view = viewName;
 	}
 
 	/**
-	 * Return the view name to be resolved by the DispatcherServlet via a 
+	 * Return the view name to be resolved by the DispatcherServlet via a
 	 * ViewResolver, or {@code null} if a View object is set.
 	 */
 	public String getViewName() {
 		return (this.view instanceof String ? (String) this.view : null);
 	}
-	
+
 	/**
-	 * Set a View object to be used by the DispatcherServlet. 
+	 * Set a View object to be used by the DispatcherServlet.
 	 * Will override any pre-existing view name or View.
 	 */
 	public void setView(Object view) {
@@ -97,28 +97,28 @@ public Object getView() {
 	}
 
 	/**
-	 * Whether the view is a view reference specified via a name to be 
+	 * Whether the view is a view reference specified via a name to be
 	 * resolved by the DispatcherServlet via a ViewResolver.
 	 */
 	public boolean isViewReference() {
 		return (this.view instanceof String);
 	}
-	
+
 	/**
-	 * Signal a scenario where the request is handled directly. 
-	 * <p>A {@link HandlerMethodReturnValueHandler} may use this flag to 
-	 * indicate the response has been fully handled and view resolution 
+	 * Signal a scenario where the request is handled directly.
+	 * <p>A {@link HandlerMethodReturnValueHandler} may use this flag to
+	 * indicate the response has been fully handled and view resolution
 	 * is not required (e.g. {@code @ResponseBody}).
 	 * <p>A {@link HandlerMethodArgumentResolver} may also use this flag
-	 * to indicate the presence of an argument (e.g. 
-	 * {@code ServletResponse} or {@code OutputStream}) that may lead to 
+	 * to indicate the presence of an argument (e.g.
+	 * {@code ServletResponse} or {@code OutputStream}) that may lead to
 	 * a complete response depending on the method return value.
 	 * <p>The default value is {@code true}.
 	 */
 	public void setRequestHandled(boolean requestHandled) {
 		this.requestHandled = requestHandled;
 	}
-	
+
 	/**
 	 * Whether the request is handled directly.
 	 */
@@ -129,7 +129,7 @@ public boolean isRequestHandled() {
 	/**
 	 * Return the model to use: the "default" or the "redirect" model.
 	 * <p>The default model is used if {@code "redirectModelScenario=false"} or
-	 * if the redirect model is {@code null} (i.e. it wasn't declared as a 
+	 * if the redirect model is {@code null} (i.e. it wasn't declared as a
 	 * method argument) and {@code ignoreDefaultModelOnRedirect=false}.
 	 */
 	public ModelMap getModel() {
@@ -140,17 +140,17 @@ public ModelMap getModel() {
 			return (this.redirectModel != null) ? this.redirectModel : new ModelMap();
 		}
 	}
-	
+
 	/**
 	 * Whether to use the default model or the redirect model.
 	 */
 	private boolean useDefaultModel() {
 		return !this.redirectModelScenario || ((this.redirectModel == null) && !this.ignoreDefaultModelOnRedirect);
 	}
-	
+
 	/**
-	 * Provide a separate model instance to use in a redirect scenario. 
-	 * The provided additional model however is not used used unless 
+	 * Provide a separate model instance to use in a redirect scenario.
+	 * The provided additional model however is not used used unless
 	 * {@link #setRedirectModelScenario(boolean)} gets set to {@code true} to signal
 	 * a redirect scenario.
 	 */
@@ -168,7 +168,7 @@ public void setRedirectModelScenario(boolean redirectModelScenario) {
 
 	/**
 	 * When set to {@code true} the default model is never used in a redirect
-	 * scenario. So if a redirect model is not available, an empty model is 
+	 * scenario. So if a redirect model is not available, an empty model is
 	 * used instead.
 	 * <p>When set to {@code false} the default model can be used in a redirect
 	 * scenario if a redirect model is not available.
@@ -179,7 +179,7 @@ public void setIgnoreDefaultModelOnRedirect(boolean ignoreDefaultModelOnRedirect
 	}
 
 	/**
-	 * Return the {@link SessionStatus} instance to use that can be used to 
+	 * Return the {@link SessionStatus} instance to use that can be used to
 	 * signal that session processing is complete.
 	 */
 	public SessionStatus getSessionStatus() {
@@ -188,16 +188,16 @@ public SessionStatus getSessionStatus() {
 
 	/**
 	 * Add the supplied attribute to the underlying model.
-	 * @see ModelMap#addAttribute(String, Object)
+	 * A shortcut for {@code getModel().addAttribute(String, Object)}.
 	 */
 	public ModelAndViewContainer addAttribute(String name, Object value) {
 		getModel().addAttribute(name, value);
 		return this;
 	}
-	
+
 	/**
 	 * Add the supplied attribute to the underlying model.
-	 * @see Model#addAttribute(Object)
+	 * A shortcut for {@code getModel().addAttribute(Object)}.
 	 */
 	public ModelAndViewContainer addAttribute(Object value) {
 		getModel().addAttribute(value);
@@ -206,26 +206,38 @@ public ModelAndViewContainer addAttribute(Object value) {
 
 	/**
 	 * Copy all attributes to the underlying model.
-	 * @see ModelMap#addAllAttributes(Map)
+	 * A shortcut for {@code getModel().addAllAttributes(Map)}.
 	 */
 	public ModelAndViewContainer addAllAttributes(Map<String, ?> attributes) {
 		getModel().addAllAttributes(attributes);
 		return this;
 	}
 
 	/**
-	 * Copy attributes in the supplied <code>Map</code> with existing objects of 
+	 * Copy attributes in the supplied <code>Map</code> with existing objects of
 	 * the same name taking precedence (i.e. not getting replaced).
-	 * @see ModelMap#mergeAttributes(Map)
+	 * A shortcut for {@code getModel().mergeAttributes(Map<String, ?>)}.
 	 */
 	public ModelAndViewContainer mergeAttributes(Map<String, ?> attributes) {
 		getModel().mergeAttributes(attributes);
 		return this;
 	}
 
+	/**
+	 * Remove the given attributes from the model.
+	 */
+	public ModelAndViewContainer removeAttributes(Map<String, ?> attributes) {
+		if (attributes != null) {
+			for (String key : attributes.keySet()) {
+				getModel().remove(key);
+			}
+		}
+		return this;
+	}
+
 	/**
 	 * Whether the underlying model contains the given attribute name.
-	 * @see ModelMap#containsAttribute(String)
+	 * A shortcut for {@code getModel().containsAttribute(String)}.
 	 */
 	public boolean containsAttribute(String name) {
 		return getModel().containsAttribute(name);
@@ -257,5 +269,5 @@ public String toString() {
 		}
 		return sb.toString();
 	}
-	
+
 }