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
Backport Issue: SPR-9687

org.springframework.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.annotation.ModelAttribute;
 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;
@@ -113,7 +112,12 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 			}
 		}
 
-		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();
 	}
 

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

@@ -223,6 +223,18 @@ public class ModelAndViewContainer {
 		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)

org.springframework.web/src/test/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessorTests.java

@@ -16,17 +16,34 @@
 
 package org.springframework.web.method.annotation;
 
+import static java.lang.annotation.ElementType.CONSTRUCTOR;
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.eq;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.notNull;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
 import java.lang.reflect.Method;
 
 import org.junit.Before;
 import org.junit.Test;
-
 import org.springframework.beans.TestBean;
 import org.springframework.core.MethodParameter;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.validation.BindException;
+import org.springframework.validation.BindingResult;
 import org.springframework.validation.Errors;
 import org.springframework.web.bind.WebDataBinder;
 import org.springframework.web.bind.annotation.ModelAttribute;
@@ -36,23 +53,17 @@ import org.springframework.web.bind.support.WebRequestDataBinder;
 import org.springframework.web.context.request.NativeWebRequest;
 import org.springframework.web.context.request.ServletWebRequest;
 import org.springframework.web.context.request.WebRequest;
-import org.springframework.web.method.annotation.ModelAttributeMethodProcessor;
 import org.springframework.web.method.support.ModelAndViewContainer;
 
-import static java.lang.annotation.ElementType.*;
-import static java.lang.annotation.RetentionPolicy.*;
-import static org.easymock.EasyMock.*;
-import static org.junit.Assert.*;
-
 /**
  * Test fixture with {@link ModelAttributeMethodProcessor}.
- * 
+ *
  * @author Rossen Stoyanchev
  */
 public class ModelAttributeMethodProcessorTests {
 
 	private ModelAttributeMethodProcessor processor;
-	
+
 	private MethodParameter paramNamedValidModelAttr;
 
 	private MethodParameter paramErrors;
@@ -62,31 +73,31 @@ public class ModelAttributeMethodProcessorTests {
 	private MethodParameter paramModelAttr;
 
 	private MethodParameter paramNonSimpleType;
-	
+
 	private MethodParameter returnParamNamedModelAttr;
-	
+
 	private MethodParameter returnParamNonSimpleType;
 
 	private ModelAndViewContainer mavContainer;
 
 	private NativeWebRequest webRequest;
-	
+
 	@Before
 	public void setUp() throws Exception {
 		processor = new ModelAttributeMethodProcessor(false);
 
-		Method method = ModelAttributeHandler.class.getDeclaredMethod("modelAttribute", 
+		Method method = ModelAttributeHandler.class.getDeclaredMethod("modelAttribute",
 				TestBean.class, Errors.class, int.class, TestBean.class, TestBean.class);
-		
+
 		paramNamedValidModelAttr = new MethodParameter(method, 0);
 		paramErrors = new MethodParameter(method, 1);
 		paramInt = new MethodParameter(method, 2);
 		paramModelAttr = new MethodParameter(method, 3);
 		paramNonSimpleType = new MethodParameter(method, 4);
-		
+
 		returnParamNamedModelAttr = new MethodParameter(getClass().getDeclaredMethod("annotatedReturnValue"), -1);
 		returnParamNonSimpleType = new MethodParameter(getClass().getDeclaredMethod("notAnnotatedReturnValue"), -1);
-		
+
 		mavContainer = new ModelAndViewContainer();
 
 		webRequest = new ServletWebRequest(new MockHttpServletRequest());
@@ -97,16 +108,16 @@ public class ModelAttributeMethodProcessorTests {
 		// Only @ModelAttribute arguments
 		assertTrue(processor.supportsParameter(paramNamedValidModelAttr));
 		assertTrue(processor.supportsParameter(paramModelAttr));
-		
+
 		assertFalse(processor.supportsParameter(paramErrors));
 		assertFalse(processor.supportsParameter(paramInt));
 		assertFalse(processor.supportsParameter(paramNonSimpleType));
 	}
-	
+
 	@Test
 	public void supportedParametersInDefaultResolutionMode() throws Exception {
 		processor = new ModelAttributeMethodProcessor(true);
-		
+
 		// Only non-simple types, even if not annotated
 		assertTrue(processor.supportsParameter(paramNamedValidModelAttr));
 		assertTrue(processor.supportsParameter(paramErrors));
@@ -115,21 +126,21 @@ public class ModelAttributeMethodProcessorTests {
 
 		assertFalse(processor.supportsParameter(paramInt));
 	}
-	
+
 	@Test
 	public void supportedReturnTypes() throws Exception {
 		processor = new ModelAttributeMethodProcessor(false);
 		assertTrue(processor.supportsReturnType(returnParamNamedModelAttr));
 		assertFalse(processor.supportsReturnType(returnParamNonSimpleType));
 	}
-	
+
 	@Test
 	public void supportedReturnTypesInDefaultResolutionMode() throws Exception {
 		processor = new ModelAttributeMethodProcessor(true);
 		assertTrue(processor.supportsReturnType(returnParamNamedModelAttr));
 		assertTrue(processor.supportsReturnType(returnParamNonSimpleType));
 	}
-	
+
 	@Test
 	public void bindExceptionRequired() throws Exception {
 		assertTrue(processor.isBindExceptionRequired(null, paramNonSimpleType));
@@ -155,9 +166,9 @@ public class ModelAttributeMethodProcessorTests {
 		WebDataBinderFactory factory = createMock(WebDataBinderFactory.class);
 		expect(factory.createBinder(webRequest, target, expectedAttributeName)).andReturn(dataBinder);
 		replay(factory);
-		
+
 		processor.resolveArgument(param, mavContainer, webRequest, factory);
-		
+
 		verify(factory);
 	}
 
@@ -168,43 +179,69 @@ public class ModelAttributeMethodProcessorTests {
 		WebDataBinderFactory factory = createMock(WebDataBinderFactory.class);
 		expect(factory.createBinder((NativeWebRequest) anyObject(), notNull(), eq("attrName"))).andReturn(dataBinder);
 		replay(factory);
-		
+
 		processor.resolveArgument(paramNamedValidModelAttr, mavContainer, webRequest, factory);
-		
+
 		verify(factory);
 	}
 
 	@Test
 	public void automaticValidation() throws Exception {
+		String name = "attrName";
 		Object target = new TestBean();
-		mavContainer.addAttribute("attrName", target);
-		
-		StubRequestDataBinder dataBinder = new StubRequestDataBinder(target);
+		mavContainer.addAttribute(name, target);
+
+		StubRequestDataBinder dataBinder = new StubRequestDataBinder(target, name);
 		WebDataBinderFactory binderFactory = createMock(WebDataBinderFactory.class);
-		expect(binderFactory.createBinder(webRequest, target, "attrName")).andReturn(dataBinder);
+		expect(binderFactory.createBinder(webRequest, target, name)).andReturn(dataBinder);
 		replay(binderFactory);
-		
+
 		processor.resolveArgument(paramNamedValidModelAttr, mavContainer, webRequest, binderFactory);
 
 		assertTrue(dataBinder.isBindInvoked());
 		assertTrue(dataBinder.isValidateInvoked());
 	}
-	
+
 	@Test(expected=BindException.class)
 	public void bindException() throws Exception {
 		Object target = new TestBean();
 		mavContainer.getModel().addAttribute(target);
 
-		StubRequestDataBinder dataBinder = new StubRequestDataBinder(target);
+		StubRequestDataBinder dataBinder = new StubRequestDataBinder(target, "testBean");
 		dataBinder.getBindingResult().reject("error");
 
 		WebDataBinderFactory binderFactory = createMock(WebDataBinderFactory.class);
 		expect(binderFactory.createBinder(webRequest, target, "testBean")).andReturn(dataBinder);
 		replay(binderFactory);
-		
+
 		processor.resolveArgument(paramNonSimpleType, mavContainer, webRequest, binderFactory);
 	}
-	
+
+	// SPR-9378
+
+	@Test
+	public void resolveArgumentOrdering() throws Exception {
+		String name = "testBean";
+		Object testBean = new TestBean(name);
+		mavContainer.addAttribute(name, testBean);
+		mavContainer.addAttribute(BindingResult.MODEL_KEY_PREFIX + name, testBean);
+
+		Object anotherTestBean = new TestBean();
+		mavContainer.addAttribute("anotherTestBean", anotherTestBean);
+
+		StubRequestDataBinder dataBinder = new StubRequestDataBinder(testBean, name);
+		WebDataBinderFactory binderFactory = createMock(WebDataBinderFactory.class);
+		expect(binderFactory.createBinder(webRequest, testBean, name)).andReturn(dataBinder);
+		replay(binderFactory);
+
+		processor.resolveArgument(paramModelAttr, mavContainer, webRequest, binderFactory);
+
+		assertSame("Resolved attribute should be updated to be last in the order",
+				testBean, mavContainer.getModel().values().toArray()[1]);
+		assertSame("BindingResult of resolved attribute should be last in the order",
+				dataBinder.getBindingResult(), mavContainer.getModel().values().toArray()[2]);
+	}
+
 	@Test
 	public void handleAnnotatedReturnValue() throws Exception {
 		processor.handleReturnValue("expected", returnParamNamedModelAttr, mavContainer, webRequest);
@@ -215,18 +252,18 @@ public class ModelAttributeMethodProcessorTests {
 	public void handleNotAnnotatedReturnValue() throws Exception {
 		TestBean testBean = new TestBean("expected");
 		processor.handleReturnValue(testBean, returnParamNonSimpleType, mavContainer, webRequest);
-		
+
 		assertSame(testBean, mavContainer.getModel().get("testBean"));
 	}
-	
+
 	private static class StubRequestDataBinder extends WebRequestDataBinder {
-		
+
 		private boolean bindInvoked;
-		
+
 		private boolean validateInvoked;
 
-		public StubRequestDataBinder(Object target) {
-			super(target);
+		public StubRequestDataBinder(Object target, String objectName) {
+			super(target, objectName);
 		}
 
 		public boolean isBindInvoked() {
@@ -258,7 +295,7 @@ public class ModelAttributeMethodProcessorTests {
 	@SessionAttributes(types=TestBean.class)
 	private static class ModelAttributeHandler {
 		@SuppressWarnings("unused")
-		public void modelAttribute(@ModelAttribute("attrName") @Valid TestBean annotatedAttr, 
+		public void modelAttribute(@ModelAttribute("attrName") @Valid TestBean annotatedAttr,
 								   Errors errors,
 								   int intArg,
 								   @ModelAttribute TestBean defaultNameAttr,
@@ -276,5 +313,5 @@ public class ModelAttributeMethodProcessorTests {
 	private TestBean notAnnotatedReturnValue() {
 		return null;
 	}
-	
+
 }