From cdfdbb4e3162815410ac6e45a0a4785efea94b25 Mon Sep 17 00:00:00 2001 From: bbottema Date: Fri, 2 Jul 2021 23:22:44 +0200 Subject: [PATCH] #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 --- modules/batch-module/pom.xml | 2 +- .../BuilderApiToPicocliCommandsMapper.java | 11 ++++---- .../clisupport/CliCommandLineConsumer.java | 4 +-- .../CliCommandLineConsumerResultHandler.java | 25 ++++++++++++++++--- .../clisupport/CliExecutionException.java | 4 +-- .../internal/clisupport/CliSupport.java | 4 +-- .../serialization/SerializationUtil.java | 12 --------- 7 files changed, 34 insertions(+), 28 deletions(-) diff --git a/modules/batch-module/pom.xml b/modules/batch-module/pom.xml index d7212b26d..08d58e967 100644 --- a/modules/batch-module/pom.xml +++ b/modules/batch-module/pom.xml @@ -34,7 +34,7 @@ org.simplejavamail smtp-connection-pool - 1.1.1 + 1.1.2 diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/BuilderApiToPicocliCommandsMapper.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/BuilderApiToPicocliCommandsMapper.java index b825c770d..0b9848d1a 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/BuilderApiToPicocliCommandsMapper.java +++ b/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 generateOptionsFromBuilderApi(@SuppressWarnings("SameParameterValue") Class[] relevantBuilderRootApi) { - final List cliOptions = new ArrayList<>(); + final Set cliOptions = new TreeSet<>(); final Set> 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> processedApiNodes, List cliOptionsFoundSoFar) { + private static void generateOptionsFromBuilderApiChain(Class apiNode, Set> processedApiNodes, Set 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 * Recursive for returned API class (since builders can return different builders. */ - private static void generateOptionsFromBuilderApi(Class apiNode, Set> processedApiNodes, List cliOptionsFoundSoFar) { + private static void generateOptionsFromBuilderApi(Class apiNode, Set> processedApiNodes, Set cliOptionsFoundSoFar) { if (processedApiNodes.contains(apiNode)) { return; } diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumer.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumer.java index 259321d46..6682fc6d1 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumer.java +++ b/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 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()); } diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumerResultHandler.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumerResultHandler.java index 35199e26c..b99b63b9d 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliCommandLineConsumerResultHandler.java +++ b/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 invokeBuilderApi(List 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()); + } } diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliExecutionException.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliExecutionException.java index 6f82ef67f..88243e8fc 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliExecutionException.java +++ b/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); diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliSupport.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliSupport.java index da8466224..85e45b99c 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliSupport.java +++ b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/CliSupport.java @@ -37,8 +37,8 @@ private static List produceCliDeclaredOptionSpec() { try { if (!CLI_DATAFILE.exists()) { LOGGER.info("Initial cli.data not found, writing to (one time action): {}", CLI_DATAFILE); - List serializable = generateOptionsFromBuilderApi(RELEVANT_BUILDER_ROOT_API); - MiscUtil.writeFileBytes(CLI_DATAFILE, SerializationUtil.serialize(serializable)); + List declaredOptions = generateOptionsFromBuilderApi(RELEVANT_BUILDER_ROOT_API); + MiscUtil.writeFileBytes(CLI_DATAFILE, SerializationUtil.serialize(declaredOptions)); } return SerializationUtil.deserialize(MiscUtil.readFileBytes(CLI_DATAFILE)); } catch (IOException e) { diff --git a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/serialization/SerializationUtil.java b/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/serialization/SerializationUtil.java index 6e18edf59..4a5785d5f 100644 --- a/modules/cli-module/src/main/java/org/simplejavamail/internal/clisupport/serialization/SerializationUtil.java +++ b/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 void registerClassSerializer(final Kryo kryo, final FieldSerializer.FieldSerializerConfig config, Class type) { - kryo.register(type, new FieldSerializer(kryo, type, config)); - } - - private static void registerClassJavaSerializer(final Kryo kryo, Class type) { - kryo.register(type, new JavaSerializer()); - } - @NotNull public static byte[] serialize(@NotNull final Object serializable) throws IOException {