#330: Fixed issue with the duplicate declared cli options being recorded. Also fixed broken CLI support with the latest performance upgrade. Also greatly improved the error feedback for failed CLI commands

Author: bbottema

modules/batch-module/pom.xml

@@ -34,7 +34,7 @@
         <dependency>
             <groupId>org.simplejavamail</groupId>
             <artifactId>smtp-connection-pool</artifactId>
-            <version>1.1.1</version>
+            <version>1.1.2</version>
         </dependency>
     </dependencies>
 </project>

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/BuilderApiToPicocliCommandsMapper.java

@@ -37,13 +37,13 @@
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.UUID;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -102,16 +102,15 @@ private BuilderApiToPicocliCommandsMapper() {
 
 	@NotNull
 	static List<CliDeclaredOptionSpec> generateOptionsFromBuilderApi(@SuppressWarnings("SameParameterValue") Class<?>[] relevantBuilderRootApi) {
-		final List<CliDeclaredOptionSpec> cliOptions = new ArrayList<>();
+		final Set<CliDeclaredOptionSpec> cliOptions = new TreeSet<>();
 		final Set<Class<?>> processedApiNodes = new HashSet<>();
 		for (Class<?> apiRoot : relevantBuilderRootApi) {
 			generateOptionsFromBuilderApiChain(apiRoot, processedApiNodes, cliOptions);
 		}
-		Collections.sort(cliOptions);
-		return cliOptions;
+		return new ArrayList<>(cliOptions);
 	}
 
-	private static void generateOptionsFromBuilderApiChain(Class<?> apiNode, Set<Class<?>> processedApiNodes, List<CliDeclaredOptionSpec> cliOptionsFoundSoFar) {
+	private static void generateOptionsFromBuilderApiChain(Class<?> apiNode, Set<Class<?>> processedApiNodes, Set<CliDeclaredOptionSpec> cliOptionsFoundSoFar) {
 		Class<?> apiNodeChainClass = apiNode;
 		while (apiNodeChainClass != null && apiNodeChainClass.getPackage().getName().contains("org.simplejavamail")) {
 			for (Class<?> apiInterface : apiNodeChainClass.getInterfaces()) {
@@ -126,7 +125,7 @@ private static void generateOptionsFromBuilderApiChain(Class<?> apiNode, Set<Cla
 	 * Produces all the --option Picocli-based params for specific API class. <br>
 	 * Recursive for returned API class (since builders can return different builders.
 	 */
-	private static void generateOptionsFromBuilderApi(Class<?> apiNode, Set<Class<?>> processedApiNodes, List<CliDeclaredOptionSpec> cliOptionsFoundSoFar) {
+	private static void generateOptionsFromBuilderApi(Class<?> apiNode, Set<Class<?>> processedApiNodes, Set<CliDeclaredOptionSpec> cliOptionsFoundSoFar) {
 		if (processedApiNodes.contains(apiNode)) {
 			return;
 		}

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumer.java

@@ -50,9 +50,9 @@ static CliReceivedCommand consumeCommandLineInput(ParseResult providedCommand, @
 			final int mandatoryParameters = MiscUtil.countMandatoryParameters(sourceMethod);
 			final List<String> providedStringValues = cliOption.getValue().getValue();
             assumeTrue(providedStringValues.size() >= mandatoryParameters,
-                    format("provided %s arguments, but need at least %s", providedStringValues.size(), mandatoryParameters));
+                    format("provided %s arguments for '%s', but need at least %s", providedStringValues.size(), cliOption.getKey(), mandatoryParameters));
             assumeTrue(providedStringValues.size() <= sourceMethod.getParameterTypes().length,
-                    format("provided %s arguments, but need at most %s", providedStringValues.size(), sourceMethod.getParameterTypes().length));
+                    format("provided %s arguments for '%s', but need at most %s", providedStringValues.size(), cliOption.getKey(), sourceMethod.getParameterTypes().length));
 			receivedOptions.add(new CliReceivedOptionData(cliOption.getKey(), convertProvidedOptionValues(providedStringValues, sourceMethod)));
 			LOGGER.debug("\tconverted option values: {}", getLast(receivedOptions).getProvidedOptionValues());
         }

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumerResultHandler.java

@@ -1,5 +1,6 @@
 package org.simplejavamail.internal.clisupport;
 
+import org.jetbrains.annotations.NotNull;
 import org.simplejavamail.api.email.EmailPopulatingBuilder;
 import org.simplejavamail.api.internal.clisupport.model.CliBuilderApiType;
 import org.simplejavamail.api.internal.clisupport.model.CliReceivedCommand;
@@ -10,8 +11,10 @@
 import org.slf4j.Logger;
 
 import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.List;
 
+import static java.lang.String.format;
 import static org.slf4j.LoggerFactory.getLogger;
 
 class CliCommandLineConsumerResultHandler {
@@ -58,14 +61,30 @@ private static <T> T invokeBuilderApi(List<CliReceivedOptionData> cliReceivedOpt
 			if (option.determineTargetBuilderApi() == builderApiType) {
 				try {
 					LOGGER.debug("\t\t.{}({})", option.getDeclaredOptionSpec().getSourceMethod().getName(), option.getProvidedOptionValues());
-					currentBuilder = option.getDeclaredOptionSpec().getSourceMethod().invoke(currentBuilder, option.getProvidedOptionValues().toArray());
+
+					Method sourceMethod = determineTrueSourceMethod(option.getDeclaredOptionSpec().getSourceMethod());
+
+					currentBuilder = sourceMethod.invoke(currentBuilder, option.getProvidedOptionValues().toArray());
 				} catch (IllegalArgumentException e) {
-					throw new CliExecutionException(CliExecutionException.WRONG_CURRENT_BUILDER, e);
+					throw new CliExecutionException(formatCliInvocationError(CliExecutionException.WRONG_CURRENT_BUILDER, option), e);
 				} catch (IllegalAccessException | InvocationTargetException e) {
-					throw new CliExecutionException(CliExecutionException.ERROR_INVOKING_BUILDER_API, e);
+					throw new CliExecutionException(formatCliInvocationError(CliExecutionException.ERROR_INVOKING_BUILDER_API, option), e);
+				} catch (NoSuchMethodException e) {
+					throw new CliExecutionException("This should never happen", e);
 				}
 			}
 		}
 		return (T) currentBuilder;
 	}
+
+	@NotNull
+	// yeah, so after deserializing a java.lang.Method, it's actually a fake, so let's find it's real counter version
+	private static Method determineTrueSourceMethod(@NotNull Method sourceMethod)
+			throws NoSuchMethodException {
+		return sourceMethod.getDeclaringClass().getDeclaredMethod(sourceMethod.getName(), sourceMethod.getParameterTypes());
+	}
+
+	private static String formatCliInvocationError(final String exceptionTemplate, final CliReceivedOptionData option) {
+		return format(exceptionTemplate, option.getProvidedOptionValues(), option.getDeclaredOptionSpec().getName());
+	}
 }

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliExecutionException.java

@@ -8,7 +8,7 @@
 @SuppressWarnings("serial")
 class CliExecutionException extends MailException {
 
-	static final String WRONG_CURRENT_BUILDER = "Wrong argument for the current builder API. Make sure you start with one of the following options:\n" +
+	static final String WRONG_CURRENT_BUILDER = "Wrong argument(s) '%s' for '%s'.\nAlso, make sure you start with one of the following options:\n" +
 			"\t\t--email:startingBlank\n" +
 			"\t\t--email:copying message(=FILE)\n" +
 			"\t\t--email:forwarding message(=FILE)\n" +
@@ -17,7 +17,7 @@ class CliExecutionException extends MailException {
 			"\t\t--email:replyingToSenderWithDefaultQuoteMarkup message(=FILE)\n" +
 			"\t\t--email:replyingToAll message(=FILE) customQuotingTemplate(=TEXT)\n" +
 			"\t\t--email:replyingToAllWithDefaultQuoteMarkup message(=FILE)";
-	static final String ERROR_INVOKING_BUILDER_API = "Got error while invoking Builder API";
+	static final String ERROR_INVOKING_BUILDER_API = "Got error while invoking Builder API with argument(s) '%s' for '%s'";
 
 	CliExecutionException(String message, Exception cause) {
 		super(message, cause);

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliSupport.java

@@ -37,8 +37,8 @@ private static List<CliDeclaredOptionSpec> produceCliDeclaredOptionSpec() {
 		try {
 			if (!CLI_DATAFILE.exists()) {
 				LOGGER.info("Initial cli.data not found, writing to (one time action): {}", CLI_DATAFILE);
-				List<CliDeclaredOptionSpec> serializable = generateOptionsFromBuilderApi(RELEVANT_BUILDER_ROOT_API);
-				MiscUtil.writeFileBytes(CLI_DATAFILE, SerializationUtil.serialize(serializable));
+				List<CliDeclaredOptionSpec> declaredOptions = generateOptionsFromBuilderApi(RELEVANT_BUILDER_ROOT_API);
+				MiscUtil.writeFileBytes(CLI_DATAFILE, SerializationUtil.serialize(declaredOptions));
 			}
 			return SerializationUtil.deserialize(MiscUtil.readFileBytes(CLI_DATAFILE));
 		} catch (IOException e) {

modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/serialization/SerializationUtil.java

@@ -3,8 +3,6 @@
 import com.esotericsoftware.kryo.Kryo;
 import com.esotericsoftware.kryo.io.Input;
 import com.esotericsoftware.kryo.io.Output;
-import com.esotericsoftware.kryo.serializers.FieldSerializer;
-import com.esotericsoftware.kryo.serializers.JavaSerializer;
 import com.esotericsoftware.kryo.util.DefaultInstantiatorStrategy;
 import de.javakaffee.kryoserializers.UnmodifiableCollectionsSerializer;
 import org.jetbrains.annotations.NotNull;
@@ -29,20 +27,10 @@ private static Kryo initKryo() {
 		Kryo kryo = new Kryo();
 		kryo.setRegistrationRequired(false);
 		kryo.setInstantiatorStrategy(new DefaultInstantiatorStrategy(new StdInstantiatorStrategy()));
-//		final FieldSerializer.FieldSerializerConfig config = new FieldSerializer.FieldSerializerConfig();
-//		config.setSerializeTransient(true);
 		UnmodifiableCollectionsSerializer.registerSerializers(kryo);
 		return kryo;
 	}
 
-	private static <T> void registerClassSerializer(final Kryo kryo, final FieldSerializer.FieldSerializerConfig config, Class<T> type) {
-		kryo.register(type, new FieldSerializer<T>(kryo, type, config));
-	}
-
-	private static <T> void registerClassJavaSerializer(final Kryo kryo, Class<T> type) {
-		kryo.register(type, new JavaSerializer());
-	}
-
 	@NotNull
 	public static byte[] serialize(@NotNull final Object serializable)
 			throws IOException {