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

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.Assert;
 import org.springframework.util.ObjectUtils;
 
 /**
- * Object representing the override of a method on a managed
+ * Object representing the override of a method on a managed object by the IoC
- * object by the IoC container.
+ * container.
  *
- * <p>Note that the override mechanism is <i>not</i> intended as a
+ * <p>Note that the override mechanism is <em>not</em> intended as a generic
- * generic means of inserting crosscutting code: use AOP for that.
+ * 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 abstract class MethodOverride implements BeanMetadataElement {
 	}
 
 	/**
-	 * Set whether the overridden method has to be considered as overloaded
+	 * Set whether the overridden method is <em>overloaded</em> (i.e., whether argument
-	 * (that is, whether arg type matching has to happen).
+	 * type matching needs to occur to disambiguate methods of the same name).
-	 * <p>Default is "true"; can be switched to "false" to optimize runtime performance.
+	 * <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
+	 * Return whether the overridden method is <em>overloaded</em> (i.e., whether argument
-	 * (that is, whether arg type matching has to happen).
+	 * type matching needs to occur to disambiguate methods of the same name).
 	 */
 	protected boolean isOverloaded() {
 		return this.overloaded;
@@ -88,17 +90,15 @@ public abstract class MethodOverride implements BeanMetadataElement {
 		return this.source;
 	}
 
-
 	/**
-	 * Subclasses must override this to indicate whether they match
+	 * Subclasses must override this to indicate whether they <em>match</em> the
-	 * the given method. This allows for argument list checking
+	 * given method. This allows for argument list checking as well as method
-	 * as well as method name checking.
+	 * 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) {
@@ -109,7 +109,6 @@ public abstract class MethodOverride implements BeanMetadataElement {
 		}
 		MethodOverride that = (MethodOverride) other;
 		return (ObjectUtils.nullSafeEquals(this.methodName, that.methodName) &&
-				this.overloaded == that.overloaded &&
 				ObjectUtils.nullSafeEquals(this.source, that.source));
 	}
 
@@ -117,7 +116,6 @@ public abstract class MethodOverride implements BeanMetadataElement {
 	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,62 +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")
+	@Test
-	public void testRootBeanDefinitionEqualsAndHashCode() throws Exception {
+	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);
+		assertEqualsAndHashCodeContracts(master, equal, notEqual, subclass);
-		assertEquals("Hash code for equal instances should match", master.hashCode(), equal.hashCode());
+	}
+
+	/**
+	 * @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());
 	}
 
-	@SuppressWarnings("serial")
+	@Test
-	public void testChildBeanDefinitionEqualsAndHashCode() throws Exception {
+	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);
+		assertEqualsAndHashCodeContracts(master, equal, notEqual, subclass);
-		assertEquals("Hash code for equal instances should match", master.hashCode(), equal.hashCode());
 	}
 
-	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"){};
+		RuntimeBeanReference subclass = new RuntimeBeanReference("name") {
-		assertEqualsContract(master, equal, notEqual, subclass);
+		};
+		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);
@@ -88,10 +121,15 @@ public class DefinitionMetadataEqualsHashCodeTests extends TestCase {
 		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());
 	}
 
 }