From 4c3af009cb4f684a377056b49d40ccb0aea0f169 Mon Sep 17 00:00:00 2001
From: Eric Bottard <eric.bottard+github@gmail.com>
Date: Thu, 17 Dec 2015 16:10:36 +0100
Subject: [PATCH] Improve javadoc and DefaultParameterResolver tests

---
 .../shell2/DefaultMethodTargetResolver.java   |  4 +-
 .../shell2/DefaultParameterResolver.java      | 24 +++++---
 .../springframework/shell2/ShellMehtod.java   | 21 -------
 .../springframework/shell2/ShellMethod.java   | 37 ++++++++++++
 .../springframework/shell2/ShellOption.java   | 12 +++-
 .../shell2/DefaultParameterResolverTest.java  | 56 +++++++++++++++++++
 .../org/springframework/shell2/Remote.java    |  4 +-
 7 files changed, 125 insertions(+), 33 deletions(-)
 delete mode 100644 src/main/java/org/springframework/shell2/ShellMehtod.java
 create mode 100644 src/main/java/org/springframework/shell2/ShellMethod.java

diff --git a/src/main/java/org/springframework/shell2/DefaultMethodTargetResolver.java b/src/main/java/org/springframework/shell2/DefaultMethodTargetResolver.java
index ddf9bb74..15d3e062 100644
--- a/src/main/java/org/springframework/shell2/DefaultMethodTargetResolver.java
+++ b/src/main/java/org/springframework/shell2/DefaultMethodTargetResolver.java
@@ -20,7 +20,7 @@ public class DefaultMethodTargetResolver implements MethodTargetResolver {
 		for (Object bean : commandBeans.values()) {
 			Class<?> clazz = bean.getClass();
 			ReflectionUtils.doWithMethods(clazz, method -> {
-				ShellMehtod shellMapping = method.getAnnotation(ShellMehtod.class);
+				ShellMethod shellMapping = method.getAnnotation(ShellMethod.class);
 				String[] keys = shellMapping.value();
 				if (keys.length == 1 && "".equals(keys[0])) {
 					keys[0] = method.getName();
@@ -28,7 +28,7 @@ public class DefaultMethodTargetResolver implements MethodTargetResolver {
 				for (String key : keys) {
 					methodTargets.put(key, new MethodTarget(method, bean, shellMapping.help()));
 				}
-			}, method -> method.getAnnotation(ShellMehtod.class) != null);
+			}, method -> method.getAnnotation(ShellMethod.class) != null);
 		}
 		return methodTargets;
 	}
diff --git a/src/main/java/org/springframework/shell2/DefaultParameterResolver.java b/src/main/java/org/springframework/shell2/DefaultParameterResolver.java
index 18fd42dd..ce6d767f 100644
--- a/src/main/java/org/springframework/shell2/DefaultParameterResolver.java
+++ b/src/main/java/org/springframework/shell2/DefaultParameterResolver.java
@@ -23,7 +23,7 @@ import org.springframework.util.ConcurrentReferenceHashMap;
 
 /**
  * Default ParameterResolver implementation that supports the following features:<ul>
- *     <li>named parameters (recognized because they start with some {@link ShellMehtod#prefix()}</li>
+ *     <li>named parameters (recognized because they start with some {@link ShellMethod#prefix()}</li>
  *     <li>implicit named parameters (from the actual method parameter name)</li>
  *     <li>positional parameters (in order, for all parameter values that were not resolved via named parameters)</li>
  *     <li>default values (for all remaining parameters)</li>
@@ -63,7 +63,7 @@ class DefaultParameterResolver implements ParameterResolver {
 
 	@Override
 	public Object resolve(MethodParameter methodParameter, List<String> words) {
-		String prefix = methodParameter.getMethod().getAnnotation(ShellMehtod.class).prefix();
+		String prefix = methodParameter.getMethod().getAnnotation(ShellMethod.class).prefix();
 
 		CacheKey cacheKey = new CacheKey(methodParameter.getMethod(), words);
 		Map<Parameter, String> resolved = parameterCache.computeIfAbsent(cacheKey, (k) -> {
@@ -77,11 +77,11 @@ class DefaultParameterResolver implements ParameterResolver {
 				String word = words.get(i);
 				if (word.startsWith(prefix)) {
 					String key = word.substring(prefix.length());
-					Parameter parameter = lookupParameterForKey(methodParameter.getMethod(), key);
+					Parameter parameter = lookupParameterForKey(methodParameter.getMethod(), key, prefix);
 					int arity = getArity(parameter);
 
 					String raw = words.subList(i + 1, i + 1 + arity).stream().collect(Collectors.joining(","));
-					Assert.isTrue(!namedParameters.containsKey(key), String.format("Option '%s' has already been specified", word));
+					Assert.isTrue(!namedParameters.containsKey(key), String.format("Parameter for '%s' has already been specified", word));
 					namedParameters.put(key, raw);
 					result.put(parameter, raw);
 					i += arity;
@@ -123,9 +123,12 @@ class DefaultParameterResolver implements ParameterResolver {
 					}
 				}
 				else if (copy.size() > 1) {
-					throw new RuntimeException("Named parameter has been specified multiple times via: " + copy);
+					throw new IllegalArgumentException("Named parameter has been specified multiple times via " + prefix(copy, prefix));
 				}
 			}
+
+			Assert.isTrue(offset == positionalValues.size(), "Too many arguments: the following could not be mapped to parameters: "
+					+ positionalValues.subList(offset, positionalValues.size()).stream().collect(Collectors.joining(" ", "'", "'")));
 			return result;
 		});
 
@@ -133,6 +136,13 @@ class DefaultParameterResolver implements ParameterResolver {
 		return conversionService.convert(s, TypeDescriptor.valueOf(String.class), new TypeDescriptor(methodParameter));
 	}
 
+	/**
+	 * Add the command prefix back to the list of keys that was used to invoke the method.
+	 */
+	private String prefix(Collection<String> keys, String prefix) {
+		return keys.stream().map(k -> prefix + k).collect(Collectors.joining(", ", "'", "'"));
+	}
+
 	private boolean booleanDefaultValue(Parameter parameter) {
 		ShellOption option = parameter.getAnnotation(ShellOption.class);
 		if (option != null && !ShellOption.NULL.equals(option.defaultValue())) {
@@ -177,7 +187,7 @@ class DefaultParameterResolver implements ParameterResolver {
 	/**
 	 * Return the method parameter that should be bound to the given key.
 	 */
-	private Parameter lookupParameterForKey(Method method, String key) {
+	private Parameter lookupParameterForKey(Method method, String key, String prefix) {
 		Parameter[] parameters = method.getParameters();
 		for (int i = 0, parametersLength = parameters.length; i < parametersLength; i++) {
 			Parameter p = parameters[i];
@@ -185,7 +195,7 @@ class DefaultParameterResolver implements ParameterResolver {
 				return p;
 			}
 		}
-		throw new IllegalStateException(String.format("Could not lookup parameter for '%s' in %s", key, method));
+		throw new IllegalArgumentException(String.format("Could not look up parameter for '%s%s' in %s", prefix, key, method));
 	}
 
 	private static class CacheKey {
diff --git a/src/main/java/org/springframework/shell2/ShellMehtod.java b/src/main/java/org/springframework/shell2/ShellMehtod.java
deleted file mode 100644
index 1a5ac72e..00000000
--- a/src/main/java/org/springframework/shell2/ShellMehtod.java
+++ /dev/null
@@ -1,21 +0,0 @@
-package org.springframework.shell2;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-
-/**
- * Created by ericbottard on 27/11/15.
- */
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface ShellMehtod {
-
-	String[] value() default "";
-
-	String help() default "";
-
-	String prefix() default "--";
-
-}
diff --git a/src/main/java/org/springframework/shell2/ShellMethod.java b/src/main/java/org/springframework/shell2/ShellMethod.java
new file mode 100644
index 00000000..c7f2003f
--- /dev/null
+++ b/src/main/java/org/springframework/shell2/ShellMethod.java
@@ -0,0 +1,37 @@
+package org.springframework.shell2;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Used to mark a method as invokable via Spring Shell.
+ *
+ * @author Eric Bottard
+ * @author Florent Biville
+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+@Documented
+public @interface ShellMethod {
+
+	/**
+	 * The name(s) by which this method can be invoked via Spring Shell. If not specified, the actual method name
+	 * will be used (turning camelCase humps into "-").
+	 */
+	String[] value() default {};
+
+	/**
+	 * A description for the command. Should not contain any formatting (e.g. html) characters and would typically
+	 * start with a capital letter and end with a dot.
+	 */
+	String help() default "";
+
+	/**
+	 * The prefix to use for assigning parameters by name.
+	 */
+	String prefix() default "--";
+
+}
diff --git a/src/main/java/org/springframework/shell2/ShellOption.java b/src/main/java/org/springframework/shell2/ShellOption.java
index 377b97c7..598b0b17 100644
--- a/src/main/java/org/springframework/shell2/ShellOption.java
+++ b/src/main/java/org/springframework/shell2/ShellOption.java
@@ -7,7 +7,7 @@ import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
 /**
- * Created by ericbottard on 27/11/15.
+ * Used to customize handling of a {@link ShellMethod} parameters.
  */
 @Documented
 @Retention(RetentionPolicy.RUNTIME)
@@ -16,9 +16,19 @@ public @interface ShellOption {
 
 	String NULL = "__NULL__";
 
+	/**
+	 * The key(s) (without the {@link ShellMethod#prefix()}) by which this parameter can be referenced
+	 * when using named parameters. If none is specified, the actual method parameter name will be used.
+	 */
 	String[] value() default {};
 
+	/**
+	 * Return the number of input "words" this parameter consumes.
+	 */
 	int arity() default 1;
 
+	/**
+	 * The textual (pre-conversion) value to assign to this parameter if no value is provided by the user.
+	 */
 	String defaultValue() default NULL;
 }
diff --git a/src/test/java/org/springframework/shell2/DefaultParameterResolverTest.java b/src/test/java/org/springframework/shell2/DefaultParameterResolverTest.java
index cb9401a3..4f89315b 100644
--- a/src/test/java/org/springframework/shell2/DefaultParameterResolverTest.java
+++ b/src/test/java/org/springframework/shell2/DefaultParameterResolverTest.java
@@ -1,6 +1,8 @@
 package org.springframework.shell2;
 
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.springframework.core.DefaultParameterNameDiscoverer;
 import org.springframework.core.MethodParameter;
 import org.springframework.core.convert.support.DefaultConversionService;
@@ -19,6 +21,8 @@ import static org.springframework.util.ReflectionUtils.findMethod;
  */
 public class DefaultParameterResolverTest {
 
+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
 
 	private DefaultParameterResolver resolver = new DefaultParameterResolver(new DefaultConversionService());
 
@@ -45,6 +49,58 @@ public class DefaultParameterResolverTest {
 
 	}
 
+	@Test
+	public void testParameterSpecifiedTwiceViaDifferentAliases() throws Exception {
+		Method method = findMethod(Remote.class, "zap", boolean.class, String.class, String.class, String.class);
+
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Named parameter has been specified multiple times via '--bar, --baz'");
+
+		resolver.resolve(
+				makeMethodParameter(method, 0),
+				asList("--force --name --foo y --bar x --baz z".split(" "))
+		);
+	}
+
+	@Test
+	public void testParameterSpecifiedTwiceViaSameKey() throws Exception {
+		Method method = findMethod(Remote.class, "zap", boolean.class, String.class, String.class, String.class);
+
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Parameter for '--baz' has already been specified");
+
+		resolver.resolve(
+				makeMethodParameter(method, 0),
+				asList("--force --name --foo y --baz x --baz z".split(" "))
+		);
+	}
+
+	@Test
+	public void testUnknownParameter() throws Exception {
+		Method method = findMethod(Remote.class, "zap", boolean.class, String.class, String.class, String.class);
+
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Could not look up parameter for '--unknown' in " + method);
+
+		resolver.resolve(
+				makeMethodParameter(method, 0),
+				asList("--unknown --foo bar".split(" "))
+		);
+	}
+
+	@Test
+	public void testTooMuchInput() throws Exception {
+		Method method = findMethod(Remote.class, "zap", boolean.class, String.class, String.class, String.class);
+
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("the following could not be mapped to parameters: 'leftover'");
+
+		resolver.resolve(
+				makeMethodParameter(method, 0),
+				asList("--foo hello --name bar --force --bar well leftover".split(" "))
+		);
+	}
+
 	private MethodParameter makeMethodParameter(Method method, int parameterIndex) {
 		MethodParameter methodParameter = new MethodParameter(method, parameterIndex);
 		methodParameter.initParameterNameDiscovery(new DefaultParameterNameDiscoverer());
diff --git a/src/test/java/org/springframework/shell2/Remote.java b/src/test/java/org/springframework/shell2/Remote.java
index f7af0ead..f41a5e94 100644
--- a/src/test/java/org/springframework/shell2/Remote.java
+++ b/src/test/java/org/springframework/shell2/Remote.java
@@ -15,11 +15,11 @@ public class Remote {
 	 *     <li>default value supplying (foo and bar)</li>
 	 * </ul>
 	 */
-	@ShellMehtod
+	@ShellMethod
 	public void zap(boolean force,
 	                String name,
 	                @ShellOption(defaultValue="defoolt") String foo,
-	                @ShellOption(defaultValue = "last") String bar) {
+	                @ShellOption(value = {"bar", "baz"}, defaultValue = "last") String bar) {
 
 	}
 }