ROO-2850: Tab completion ignores some available commands - reverted p…

…revious fix, implemented equals and hashCode properly in MethodTarget instead

shell/src/main/java/org/springframework/roo/shell/CommandMarker.java

@@ -1,3 +1,6 @@
 package org.springframework.roo.shell;
 
+/**
+ * Marker interface indicating a provider of one or more shell commands.
+ */
 public interface CommandMarker {}

shell/src/main/java/org/springframework/roo/shell/MethodTarget.java

@@ -4,56 +4,68 @@
 
 import org.springframework.roo.support.style.ToStringCreator;
 import org.springframework.roo.support.util.Assert;
+import org.springframework.roo.support.util.ObjectUtils;
+import org.springframework.roo.support.util.StringUtils;
 
 /**
+ * A method that can be executed via a shell command.
+ * <p>
  * Immutable since 1.2.0.
  *
  * @author Ben Alex
  */
-public class MethodTarget implements Comparable<MethodTarget> {
+public class MethodTarget {
 
 	// Fields
 	private final Method method;
 	private final Object target;
 	private final String remainingBuffer;
 	private final String key;
+
+	/**
+	 * Constructor for a <code>null remainingBuffer</code> and <code>key</code>
+	 *
+	 * @param method the method to invoke (required)
+	 * @param target the object on which the method is to be invoked (required)
+	 * @since 1.2.0
+	 */
+	public MethodTarget(final Method method, final Object target) {
+		this(method, target, null, null);
+	}
 
 	/**
-	 * Constructor
+	 * Constructor that allows all fields to be set
 	 *
-	 * @param method the method (required)
+	 * @param method the method to invoke (required)
 	 * @param target the object on which the method is to be invoked (required)
-	 * @param remainingBuffer can be <code>null</code>
-	 * @param key can be <code>null</code>
+	 * @param remainingBuffer can be blank
+	 * @param key can be blank
 	 * @since 1.2.0
 	 */
 	public MethodTarget(final Method method, final Object target, final String remainingBuffer, final String key) {
 		Assert.notNull(method, "Method is required");
 		Assert.notNull(target, "Target is required");
-		this.key = key;
+		this.key = StringUtils.trimToEmpty(key);
 		this.method = method;
-		this.remainingBuffer = remainingBuffer;
+		this.remainingBuffer = StringUtils.trimToEmpty(remainingBuffer);
 		this.target = target;
 	}
 
 	@Override
 	public boolean equals(final Object other) {
-		return other == this || (other instanceof MethodTarget && compareTo((MethodTarget) other) == 0);
+		if (other == this) {
+			return true;
+		}
+		if (!(other instanceof MethodTarget)) {
+			return false;
+		}
+		final MethodTarget otherMethodTarget = (MethodTarget) other;
+		return this.method.equals(otherMethodTarget.getMethod()) && this.target.equals(otherMethodTarget.getTarget());
 	}
 
 	@Override
 	public int hashCode() {
-		return remainingBuffer.hashCode();
-	}
-
-	public int compareTo(final MethodTarget other) {
-		if (other == null) {
-			throw new NullPointerException();
-		}
-		if (this == other) {
-			return 0;
-		}
-		return this.remainingBuffer.compareTo(other.remainingBuffer);
+		return ObjectUtils.nullSafeHashCode(method, target);
 	}
 
 	@Override

shell/src/main/java/org/springframework/roo/shell/SimpleParser.java

@@ -275,11 +275,11 @@ protected void commandNotFound(final Logger logger, final String buffer) {
 
 	private Collection<MethodTarget> locateTargets(final String buffer, final boolean strictMatching, final boolean checkAvailabilityIndicators) {
 		Assert.notNull(buffer, "Buffer required");
-		final Collection<MethodTarget> result = new ArrayList<MethodTarget>();
+		final Collection<MethodTarget> result = new HashSet<MethodTarget>();
 
 		// The reflection could certainly be optimised, but it's good enough for now (and cached reflection
 		// is unlikely to be noticeable to a human being using the CLI)
-		for (final Object command : commands) {
+		for (final CommandMarker command : commands) {
 			for (final Method method : command.getClass().getMethods()) {
 				CliCommand cmd = method.getAnnotation(CliCommand.class);
 				if (cmd != null) {
@@ -1034,7 +1034,7 @@ public final void add(final CommandMarker command) {
 					Assert.isTrue(method.getReturnType().equals(Boolean.TYPE), "CliAvailabilityIndicator is only legal for primitive boolean return types (" + method.toGenericString() + ")");
 					for (String cmd : availability.value()) {
 						Assert.isTrue(!availabilityIndicators.containsKey(cmd), "Cannot specify an availability indicator for '" + cmd + "' more than once");
-						availabilityIndicators.put(cmd, new MethodTarget(method, command, null, null));
+						availabilityIndicators.put(cmd, new MethodTarget(method, command));
 					}
 				}
 			}

shell/src/test/java/org/springframework/roo/shell/MethodTargetTest.java

@@ -0,0 +1,52 @@
+package org.springframework.roo.shell;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+import java.lang.reflect.Method;
+
+import org.junit.Test;
+
+/**
+ * Unit test of {@link MethodTarget}
+ *
+ * @author Andrew Swan
+ * @since 1.2.0
+ */
+public class MethodTargetTest {
+
+	// Constants
+	private static final Object TARGET_1 = new CommandMarker() {};
+	private static final Object TARGET_2 = new CommandMarker() {};
+	private static final Method METHOD_1 = TARGET_1.getClass().getMethods()[0];	// unmockable
+	private static final Method METHOD_2 = TARGET_2.getClass().getMethods()[1];	// unmockable
+
+	@Test
+	public void testInstanceEqualsItself() {
+		final MethodTarget instance = new MethodTarget(METHOD_1, TARGET_1);
+		assertEquals(instance, instance);
+	}
+
+	@Test
+	public void testInstanceDoesNotEqualNull() {
+		assertFalse(new MethodTarget(METHOD_1, TARGET_1).equals(null));
+	}
+
+	@Test
+	public void testInstancesWithSameMethodAndTargetAreEqualAndHaveSameHashCode() {
+		final MethodTarget instance1 = new MethodTarget(METHOD_1, TARGET_1, "the-buff", "the-key");
+		final MethodTarget instance2 = new MethodTarget(METHOD_1, TARGET_1);
+		assertEquals(instance1, instance2);
+		assertEquals(instance1.hashCode(), instance2.hashCode());
+	}
+
+	@Test
+	public void testInstancesWithDifferentMethodAreNotEqual() {
+		assertFalse(new MethodTarget(METHOD_1, TARGET_1).equals(new MethodTarget(METHOD_2, TARGET_1)));
+	}
+
+	@Test
+	public void testInstancesWithDifferentTargetAreNotEqual() {
+		assertFalse(new MethodTarget(METHOD_1, TARGET_1).equals(new MethodTarget(METHOD_1, TARGET_2)));
+	}
+}