Exclude overloaded from equals & hashCode in MethodOverride

Prior to this commit, the inclusion of the 'overloaded' flag in the
implementations of equals() and hashCode() in MethodOverride could lead
to adverse effects in the outcome of equals() in AbstractBeanDefinition.

For example, given two bean definitions A and B that represent the
exact same bean definition metadata for a bean that relies on method
injection, if A has been validated and B has not, then A.equals(B) will
potentially return false, which is not acceptable behavior.

This commit addresses this issue by removing the 'overloaded' flag from
the implementations of equals() and hashCode() for MethodOverride.

Issue: SPR-11420
(cherry picked from commit 9534245)

Author: sbrannen

spring-beans/src/main/java/org/springframework/beans/factory/support/MethodOverride.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -23,14 +23,15 @@
 import org.springframework.util.ObjectUtils;
 
 /**
- * Object representing the override of a method on a managed
- * object by the IoC container.
+ * Object representing the override of a method on a managed object by the IoC
+ * container.
  *
- * <p>Note that the override mechanism is <i>not</i> intended as a
- * generic means of inserting crosscutting code: use AOP for that.
+ * <p>Note that the override mechanism is <em>not</em> intended as a generic
+ * means of inserting crosscutting code: use AOP for that.
  *
  * @author Rod Johnson
  * @author Juergen Hoeller
+ * @author Sam Brannen
  * @since 1.1
  */
 public abstract class MethodOverride implements BeanMetadataElement {
@@ -59,17 +60,18 @@ public String getMethodName() {
 	}
 
 	/**
-	 * Set whether the overridden method has to be considered as overloaded
-	 * (that is, whether arg type matching has to happen).
-	 * <p>Default is "true"; can be switched to "false" to optimize runtime performance.
+	 * Set whether the overridden method is <em>overloaded</em> (i.e., whether argument
+	 * type matching needs to occur to disambiguate methods of the same name).
+	 * <p>Default is {@code true}; can be switched to {@code false} to optimize
+	 * runtime performance.
 	 */
 	protected void setOverloaded(boolean overloaded) {
 		this.overloaded = overloaded;
 	}
 
 	/**
-	 * Return whether the overridden method has to be considered as overloaded
-	 * (that is, whether arg type matching has to happen).
+	 * Return whether the overridden method is <em>overloaded</em> (i.e., whether argument
+	 * type matching needs to occur to disambiguate methods of the same name).
 	 */
 	protected boolean isOverloaded() {
 		return this.overloaded;
@@ -87,17 +89,15 @@ public Object getSource() {
 		return this.source;
 	}
 
-
 	/**
-	 * Subclasses must override this to indicate whether they match
-	 * the given method. This allows for argument list checking
-	 * as well as method name checking.
+	 * Subclasses must override this to indicate whether they <em>match</em> the
+	 * given method. This allows for argument list checking as well as method
+	 * name checking.
 	 * @param method the method to check
 	 * @return whether this override matches the given method
 	 */
 	public abstract boolean matches(Method method);
 
-
 	@Override
 	public boolean equals(Object other) {
 		if (this == other) {
@@ -108,15 +108,13 @@ public boolean equals(Object other) {
 		}
 		MethodOverride that = (MethodOverride) other;
 		return (ObjectUtils.nullSafeEquals(this.methodName, that.methodName) &&
-				this.overloaded == that.overloaded &&
 				ObjectUtils.nullSafeEquals(this.source, that.source));
 	}
 
 	@Override
 	public int hashCode() {
 		int hashCode = ObjectUtils.nullSafeHashCode(this.methodName);
 		hashCode = 29 * hashCode + ObjectUtils.nullSafeHashCode(this.source);
-		hashCode = 29 * hashCode + (this.overloaded ? 1 : 0);
 		return hashCode;
 	}
 

spring-beans/src/test/java/org/springframework/beans/factory/support/DefinitionMetadataEqualsHashCodeTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -16,63 +16,95 @@
 
 package org.springframework.beans.factory.support;
 
-import junit.framework.TestCase;
+import org.junit.Test;
 
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.RuntimeBeanReference;
 import org.springframework.tests.sample.beans.TestBean;
 
+import static org.junit.Assert.*;
 
 /**
+ * Unit tests for {@code equals()} and {@code hashCode()} in bean definitions.
+ *
  * @author Rob Harrop
+ * @author Sam Brannen
  */
-public class DefinitionMetadataEqualsHashCodeTests extends TestCase {
+@SuppressWarnings("serial")
+public class DefinitionMetadataEqualsHashCodeTests {
 
-	@SuppressWarnings("serial")
-	public void testRootBeanDefinitionEqualsAndHashCode() throws Exception {
+	@Test
+	public void rootBeanDefinition() {
 		RootBeanDefinition master = new RootBeanDefinition(TestBean.class);
 		RootBeanDefinition equal = new RootBeanDefinition(TestBean.class);
 		RootBeanDefinition notEqual = new RootBeanDefinition(String.class);
-		RootBeanDefinition subclass = new RootBeanDefinition(TestBean.class) {};
+		RootBeanDefinition subclass = new RootBeanDefinition(TestBean.class) {
+		};
 		setBaseProperties(master);
 		setBaseProperties(equal);
 		setBaseProperties(notEqual);
 		setBaseProperties(subclass);
 
-		assertEqualsContract(master, equal, notEqual, subclass);
-		assertEquals("Hash code for equal instances should match", master.hashCode(), equal.hashCode());
+		assertEqualsAndHashCodeContracts(master, equal, notEqual, subclass);
 	}
 
-	@SuppressWarnings("serial")
-	public void testChildBeanDefinitionEqualsAndHashCode() throws Exception {
+	/**
+	 * @since 3.2.8
+	 * @see <a href="https://jira.springsource.org/browse/SPR-11420">SPR-11420</a>
+	 */
+	@Test
+	public void rootBeanDefinitionAndMethodOverridesWithDifferentOverloadedValues() {
+		RootBeanDefinition master = new RootBeanDefinition(TestBean.class);
+		RootBeanDefinition equal = new RootBeanDefinition(TestBean.class);
+
+		setBaseProperties(master);
+		setBaseProperties(equal);
+
+		// Simulate AbstractBeanDefinition.validate() which delegates to
+		// AbstractBeanDefinition.prepareMethodOverrides():
+		master.getMethodOverrides().getOverrides().iterator().next().setOverloaded(false);
+		// But do not simulate validation of the 'equal' bean. As a consequence, a method
+		// override in 'equal' will be marked as overloaded, but the corresponding
+		// override in 'master' will not. But... the bean definitions should still be
+		// considered equal.
+
+		assertEquals("Should be equal", master, equal);
+		assertEquals("Hash code for equal instances must match", master.hashCode(), equal.hashCode());
+	}
+
+	@Test
+	public void childBeanDefinition() {
 		ChildBeanDefinition master = new ChildBeanDefinition("foo");
 		ChildBeanDefinition equal = new ChildBeanDefinition("foo");
 		ChildBeanDefinition notEqual = new ChildBeanDefinition("bar");
-		ChildBeanDefinition subclass = new ChildBeanDefinition("foo"){};
+		ChildBeanDefinition subclass = new ChildBeanDefinition("foo") {
+		};
 		setBaseProperties(master);
 		setBaseProperties(equal);
 		setBaseProperties(notEqual);
 		setBaseProperties(subclass);
 
-		assertEqualsContract(master, equal, notEqual, subclass);
-		assertEquals("Hash code for equal instances should match", master.hashCode(), equal.hashCode());
+		assertEqualsAndHashCodeContracts(master, equal, notEqual, subclass);
 	}
 
-	public void testRuntimeBeanReference() throws Exception {
+	@Test
+	public void runtimeBeanReference() {
 		RuntimeBeanReference master = new RuntimeBeanReference("name");
 		RuntimeBeanReference equal = new RuntimeBeanReference("name");
 		RuntimeBeanReference notEqual = new RuntimeBeanReference("someOtherName");
-		RuntimeBeanReference subclass = new RuntimeBeanReference("name"){};
-		assertEqualsContract(master, equal, notEqual, subclass);
+		RuntimeBeanReference subclass = new RuntimeBeanReference("name") {
+		};
+		assertEqualsAndHashCodeContracts(master, equal, notEqual, subclass);
 	}
+
 	private void setBaseProperties(AbstractBeanDefinition definition) {
 		definition.setAbstract(true);
 		definition.setAttribute("foo", "bar");
 		definition.setAutowireCandidate(false);
 		definition.setAutowireMode(AbstractBeanDefinition.AUTOWIRE_BY_TYPE);
-		//definition.getConstructorArgumentValues().addGenericArgumentValue("foo");
+		// definition.getConstructorArgumentValues().addGenericArgumentValue("foo");
 		definition.setDependencyCheck(AbstractBeanDefinition.DEPENDENCY_CHECK_OBJECTS);
-		definition.setDependsOn(new String[]{"foo", "bar"});
+		definition.setDependsOn(new String[] { "foo", "bar" });
 		definition.setDestroyMethodName("destroy");
 		definition.setEnforceDestroyMethod(false);
 		definition.setEnforceInitMethod(true);
@@ -89,10 +121,15 @@ private void setBaseProperties(AbstractBeanDefinition definition) {
 		definition.setSource("foo");
 	}
 
-	private void assertEqualsContract(Object master, Object equal, Object notEqual, Object subclass) {
+	private void assertEqualsAndHashCodeContracts(Object master, Object equal, Object notEqual, Object subclass) {
 		assertEquals("Should be equal", master, equal);
-		assertFalse("Should not be equal", master.equals(notEqual));
+		assertEquals("Hash code for equal instances should match", master.hashCode(), equal.hashCode());
+
+		assertNotEquals("Should not be equal", master, notEqual);
+		assertNotEquals("Hash code for non-equal instances should not match", master.hashCode(), notEqual.hashCode());
+
 		assertEquals("Subclass should be equal", master, subclass);
+		assertEquals("Hash code for subclass should match", master.hashCode(), subclass.hashCode());
 	}
 
 }