From 4b41a217982f98fce15b95b17e06ae147d676662 Mon Sep 17 00:00:00 2001 From: wtfacoconut Date: Wed, 17 May 2023 15:55:55 +1000 Subject: [PATCH 1/5] Issue 1848 fix and unit test Applying patch for issue 1848 (Required ArgGroup combined with DefaultValueProvider issue) with unit test. --- src/main/java/picocli/CommandLine.java | 29 +++++++++++- .../java/picocli/DefaultProviderTest.java | 47 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java index 81e4df731..d5f377dde 100644 --- a/src/main/java/picocli/CommandLine.java +++ b/src/main/java/picocli/CommandLine.java @@ -12994,7 +12994,7 @@ void validate(CommandLine commandLine) { validationResult = matches.isEmpty() ? GroupValidationResult.SUCCESS_ABSENT : GroupValidationResult.SUCCESS_PRESENT; for (ArgGroupSpec missing : unmatchedSubgroups) { - if (missing.validate() && missing.multiplicity().min > 0) { + if (missing.validate() && missing.multiplicity().min > 0 && containsRequiredOptionsOrSubgroups(missing)) { int presentCount = 0; boolean haveMissing = true; boolean someButNotAllSpecified = false; @@ -13024,6 +13024,33 @@ void validate(CommandLine commandLine) { } } + private boolean containsRequiredOptionsOrSubgroups(ArgGroupSpec argGroupSpec) { + return containsRequiredOptions(argGroupSpec) || containsRequiredSubgroups(argGroupSpec); + } + + private boolean containsRequiredOptions(ArgGroupSpec argGroupSpec) { + for ( OptionSpec option : argGroupSpec.options() ) { + if ( option.required() ) { return true; } + } + return false; + } + + private boolean containsRequiredSubgroups(ArgGroupSpec argGroupSpec) { + for ( ArgGroupSpec subgroup : argGroupSpec.subgroups() ) { + if ( subgroup.exclusive() ) { + // Only return true if all of the subgroups contain required options or subgroups + boolean result = true; + for ( ArgGroupSpec subsubgroup : subgroup.subgroups() ) { + result &= containsRequiredOptionsOrSubgroups(subsubgroup); + } + return result && containsRequiredOptions(subgroup); + } else { + return containsRequiredOptionsOrSubgroups(subgroup); + } + } + return false; + } + private void failGroupMultiplicityExceeded(List groupMatches, CommandLine commandLine) { Map>> matchesPerGroup = new LinkedHashMap>>(); String msg = ""; diff --git a/src/test/java/picocli/DefaultProviderTest.java b/src/test/java/picocli/DefaultProviderTest.java index 991d288af..6d322542d 100644 --- a/src/test/java/picocli/DefaultProviderTest.java +++ b/src/test/java/picocli/DefaultProviderTest.java @@ -7,6 +7,7 @@ import org.junit.rules.TestRule; import picocli.CommandLine.Command; import picocli.CommandLine.IDefaultValueProvider; +import picocli.CommandLine.ArgGroup; import picocli.CommandLine.Model.ArgSpec; import picocli.CommandLine.Option; import picocli.CommandLine.Parameters; @@ -436,4 +437,50 @@ class App { assertEquals(123, app1.a); } + /** + * Tests issue 1848 https://github.com/remkop/picocli/issues/1848 + * Test to ensure that ArgGroups with a multiplicity of 1, with a required option, and with a default value + * provider, will properly show that required option as required, rather than optional. + * */ + @Test + public void testIssue1848ArgGroupWithRequiredOptionWithDefaultValueProvider() { + + @Command(name = "issue1848Command", defaultValueProvider = Issue1848CommandDefaultProvider.class) + class App { + @ArgGroup(exclusive = false, multiplicity = "1", order = 1) + public Issue1848CommandConfigOptions issue1848CommandConfigOptions; + + @Option(names = {"--opt1"}) + private String opt1; + } + String helpOutput = new CommandLine(new App()).getHelp().fullSynopsis(); + assertTrue(helpOutput.contains("--opt2=")); // Check that "--opt2=" exists. + assertFalse(helpOutput.contains("[--opt2=]")); // But make sure it's not surrounded by square brackets. + } + + class Issue1848CommandConfigOptions { + @Option(names = {"--opt2"}, required = true, order = 1) + private String opt2; + + @Option(names = {"--opt3"}, required = false, order = 2) + private String opt3; + } + + static class Issue1848CommandDefaultProvider implements CommandLine.IDefaultValueProvider { + @Override + public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws Exception { + // Commenting out for now as I'm unsure if it's expected behavior for default values supplied to a required + // option, should result in that option's help/usage information indicating that the option is not required. + /* + if (argSpec.isOption()) { + CommandLine.Model.OptionSpec option = (CommandLine.Model.OptionSpec) argSpec; + if ("--url".equals(option.longestName())) { + return "https://localhost:8080"; + } + } + */ + return null; + } + } + } From 9add2af4aa4de9026c15e167cad61367564beb88 Mon Sep 17 00:00:00 2001 From: wtfacoconut Date: Wed, 24 May 2023 13:43:55 +1000 Subject: [PATCH 2/5] Update 1848 fix to not break testIssue1027RepeatingPositionalParamsWithMinMultiplicity --- src/main/java/picocli/CommandLine.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java index d5f377dde..f0ad3490a 100644 --- a/src/main/java/picocli/CommandLine.java +++ b/src/main/java/picocli/CommandLine.java @@ -13025,13 +13025,16 @@ void validate(CommandLine commandLine) { } private boolean containsRequiredOptionsOrSubgroups(ArgGroupSpec argGroupSpec) { - return containsRequiredOptions(argGroupSpec) || containsRequiredSubgroups(argGroupSpec); + return containsRequiredOptionsOrParameters(argGroupSpec) || containsRequiredSubgroups(argGroupSpec); } - private boolean containsRequiredOptions(ArgGroupSpec argGroupSpec) { + private boolean containsRequiredOptionsOrParameters(ArgGroupSpec argGroupSpec) { for ( OptionSpec option : argGroupSpec.options() ) { if ( option.required() ) { return true; } } + for ( PositionalParamSpec param : argGroupSpec.positionalParameters() ){ + if( param.required() ){return true;} + } return false; } @@ -13043,7 +13046,7 @@ private boolean containsRequiredSubgroups(ArgGroupSpec argGroupSpec) { for ( ArgGroupSpec subsubgroup : subgroup.subgroups() ) { result &= containsRequiredOptionsOrSubgroups(subsubgroup); } - return result && containsRequiredOptions(subgroup); + return result && containsRequiredOptionsOrParameters(subgroup); } else { return containsRequiredOptionsOrSubgroups(subgroup); } From d20d711b88a34cf69ea40f5cfa2d8fa74537122d Mon Sep 17 00:00:00 2001 From: Remko Popma Date: Wed, 31 May 2023 23:52:29 +0900 Subject: [PATCH 3/5] [DEP] change junit-dep version to 4.11 from 4.11.20120805.1225 --- dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dependencies.gradle b/dependencies.gradle index 43953d99b..0d08b3bb1 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -22,7 +22,7 @@ ext { jansiVersion = "2.4.0" jline2Version = "2.14.6" jline3Version = "3.23.0" - junitDepVersion = "4.11.20120805.1225" + junitDepVersion = "4.11" junitVersion = "4.13.2" log4j2Version = "2.20.0" springBootVersion = "2.7.10" // Spring Boot 3.0 requires Java 17 as a minimum version From 7c7c8b954277b6e18c57ff5ea1791b59569c0511 Mon Sep 17 00:00:00 2001 From: Bartosz Spyrko-Smietanko Date: Tue, 30 May 2023 11:01:50 +0100 Subject: [PATCH 4/5] [2029] Fallback to default charset if the charset provided by sun.st*.encoding is invalid --- src/main/java/picocli/CommandLine.java | 12 +++++- src/test/java/picocli/Issue2029.java | 55 ++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 src/test/java/picocli/Issue2029.java diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java index f0ad3490a..8c98f74e5 100644 --- a/src/main/java/picocli/CommandLine.java +++ b/src/main/java/picocli/CommandLine.java @@ -15045,7 +15045,15 @@ static Charset getStderrEncoding() { static Charset charsetForName(String encoding) { if (encoding != null) { if ("cp65001".equalsIgnoreCase(encoding)) { encoding = "UTF-8"; } // #1474 MS Windows uses code page 65001 for UTF8 - return Charset.forName(encoding); + try { + return Charset.forName(encoding); + } catch (Exception e) { + // fallback to default charset if the requested encoding is not available + final Charset defaultCharset = Charset.defaultCharset(); + CommandLine.tracer().info("The %s encoding in not available, falling back to %s", encoding, + defaultCharset.name()); + return defaultCharset; + } } return Charset.defaultCharset(); } @@ -18705,7 +18713,7 @@ private List prefixCommandName(List suggestions) if(commandName == null || commandName.trim().isEmpty()) { return suggestions; } List prefixedSuggestions = new ArrayList(); for (String s : suggestions) { - prefixedSuggestions.add(commandName + " " + s); + prefixedSuggestions.add(commandName + " " + s); } return prefixedSuggestions; } diff --git a/src/test/java/picocli/Issue2029.java b/src/test/java/picocli/Issue2029.java new file mode 100644 index 000000000..f0f27f5ac --- /dev/null +++ b/src/test/java/picocli/Issue2029.java @@ -0,0 +1,55 @@ +package picocli; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; +import org.junit.contrib.java.lang.system.SystemErrRule; +import org.junit.contrib.java.lang.system.SystemOutRule; +import org.junit.rules.TestRule; + +import static org.junit.Assert.assertEquals; + +public class Issue2029 { + @Rule + public final TestRule restoreSystemProperties = new RestoreSystemProperties(); + + @Rule + public final SystemOutRule systemOutRule = new SystemOutRule().enableLog(); + + @Rule + public final SystemErrRule systemErrRule = new SystemErrRule().enableLog(); + + @CommandLine.Command(name = "test") + static class TestCommand implements Runnable { + + @CommandLine.Parameters + String text; + + @CommandLine.Spec + CommandLine.Model.CommandSpec spec; + + //@Override + public void run() { + spec.commandLine().getOut().print(text); + spec.commandLine().getOut().flush(); + spec.commandLine().getErr().print(text); + spec.commandLine().getErr().flush(); + } + } + + @Test + public void invalidEncodingFallsbackToDefaultEncoding() { + resetLogs(); + System.setProperty("sun.stdout.encoding", "cp0"); + System.setProperty("sun.stdout.encoding", "cp0"); + + assertEquals(CommandLine.ExitCode.OK, new CommandLine(new Issue1320.TestCommand()).execute("test")); + assertEquals("test", systemOutRule.getLog()); + assertEquals("test", systemErrRule.getLog()); + } + + private void resetLogs() { + systemOutRule.clearLog(); + systemErrRule.clearLog(); + } +} From 5984589f57bcf2bb716e3dee21025ddaa387c3da Mon Sep 17 00:00:00 2001 From: Dan Ziemba Date: Thu, 1 Jun 2023 17:51:19 -0400 Subject: [PATCH 5/5] Fixes #2035 - include mapFallbackValue for inherited opts --- src/main/java/picocli/CommandLine.java | 1 + src/test/java/picocli/MapOptionsTest.java | 53 +++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java index 8c98f74e5..d370511a6 100644 --- a/src/main/java/picocli/CommandLine.java +++ b/src/main/java/picocli/CommandLine.java @@ -9526,6 +9526,7 @@ abstract static class Builder> { setter = original.setter; scope = original.scope; scopeType = original.scopeType; + mapFallbackValue = original.mapFallbackValue; originalDefaultValue = original.originalDefaultValue; originalMapFallbackValue = original.originalMapFallbackValue; } diff --git a/src/test/java/picocli/MapOptionsTest.java b/src/test/java/picocli/MapOptionsTest.java index 3bb469544..31a71c227 100644 --- a/src/test/java/picocli/MapOptionsTest.java +++ b/src/test/java/picocli/MapOptionsTest.java @@ -5,6 +5,7 @@ import org.junit.contrib.java.lang.system.ProvideSystemProperty; import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.junit.rules.TestRule; +import picocli.CommandLine.Command; import picocli.CommandLine.Option; import picocli.CommandLine.ParameterException; import picocli.CommandLine.Parameters; @@ -12,6 +13,7 @@ import java.util.Map; import static org.junit.Assert.*; +import static picocli.CommandLine.ScopeType.INHERIT; /** * Verifies https://github.com/remkop/picocli/issues/1214 @@ -75,6 +77,40 @@ class App { assertEquals(null, app.map.get("key")); } + @Test + public void testInheritedOptionMapFallbackValueEmptyString() { + class App { + @Option(names = "-D", mapFallbackValue = "", scope = INHERIT) Map map; + } + @Command(name = "sub") + class Sub { + } + App app = new App(); + Sub sub = new Sub(); + new CommandLine(app) + .addSubcommand(sub) + .parseArgs("sub", "-Dkey"); + assertEquals(1, app.map.size()); + assertEquals("", app.map.get("key")); + } + + @Test + public void testInheritedOptionMapFallbackValueNull() { + class App { + @Option(names = "-D", mapFallbackValue = Option.NULL_VALUE, scope = INHERIT) Map map; + } + @Command(name = "sub") + class Sub { + } + App app = new App(); + Sub sub = new Sub(); + new CommandLine(app) + .addSubcommand(sub) + .parseArgs("sub", "-Dkey"); + assertEquals(1, app.map.size()); + assertEquals(null, app.map.get("key")); + } + @Test public void testPositionalMapFallbackValueNull() { class App { @@ -85,6 +121,23 @@ class App { assertEquals(null, app.map.get("key")); } + @Test + public void testInheritedPositionalMapFallbackValueNull() { + class App { + @Parameters(mapFallbackValue = Option.NULL_VALUE, scope = INHERIT) Map map; + } + @Command(name = "sub") + class Sub { + } + App app = new App(); + Sub sub = new Sub(); + new CommandLine(app) + .addSubcommand(sub) + .parseArgs("sub", "key"); + assertEquals(1, app.map.size()); + assertEquals(null, app.map.get("key")); + } + @Test public void testMapFallbackValueEmptyStringMultiple() { class App {