From 04d8ae6784282432d54c9178a2994b986d9d072b Mon Sep 17 00:00:00 2001
From: Remko Popma <remkop@yahoo.com>
Date: Fri, 3 May 2019 06:40:59 +0900
Subject: [PATCH] #676 bugfix: required should consider defaultValue _after_
 variable interpolation

Closes #676
---
 RELEASE-NOTES.md                              |  2 +-
 src/main/java/picocli/CommandLine.java        | 13 +++++++----
 .../java/picocli/InterpolatedModelTest.java   | 23 +++++++++++++++++++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
index d8b03e819..8c1d658e9 100644
--- a/RELEASE-NOTES.md
+++ b/RELEASE-NOTES.md
@@ -161,7 +161,7 @@ With the new execute API the ColorScheme class will start to play a more central
 - [#541] Improved exception handling for Runnable/Callable.
 - [#663] How to remove stacktraces on error. Thanks to [Nicolas Mingo](https://github.com/nicolasmingo) and [jrevault](https://github.com/jrevault) for raising this and subsequent discussion.
 - [#672] Need way to send errors back from subcommand. Thanks to [Garret Wilson](https://github.com/garretwilson) for raising this.
-
+- [#676] Bugfix: non-defined variables in `defaultValue` now correctly resolve to `null`, and options and positional parameters are now correctly considered `required` only if their default value is `null` after variable interpolation. Thanks to [ifedorenko](https://github.com/ifedorenko) for raising this.
 
 ## <a name="4.0.0-alpha-3-deprecated"></a> Deprecations
 All variants of the `run`, `call`, `invoke`, and `parseWithHandlers` methods are deprecated from this release, in favor of the new `execute` method.
diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
index 3ca7280f4..32c30f772 100644
--- a/src/main/java/picocli/CommandLine.java
+++ b/src/main/java/picocli/CommandLine.java
@@ -4825,12 +4825,13 @@ public CommandSpec addPositional(PositionalParamSpec positional) {
             }
             private CommandSpec addArg(ArgSpec arg) {
                 args.add(arg);
-                if (arg.required() && arg.group() == null) { requiredArgs.add(arg); }
                 arg.messages(usageMessage().messages());
                 arg.commandSpec = this;
                 if (arg.arity().isUnresolved()) {
                     arg.arity = Range.valueOf(interpolator.interpolate(arg.arity().originalValue));
                 }
+                // do this last: arg.required() needs to resolve variables in arg.defaultValue()
+                if (arg.required() && arg.group() == null) { requiredArgs.add(arg); }
                 return this;
             }
 
@@ -6080,7 +6081,7 @@ private <T extends Builder<T>> ArgSpec(Builder<T> builder) {
                 initialValue = builder.initialValue;
                 hasInitialValue = builder.hasInitialValue;
                 defaultValue = NO_DEFAULT_VALUE.equals(builder.defaultValue) ? null : builder.defaultValue;
-                required = builder.required && defaultValue == null; //#261 not required if it has a default
+                required = builder.required;
                 toString = builder.toString;
                 getter = builder.getter;
                 setter = builder.setter;
@@ -6130,11 +6131,13 @@ void applyInitialValue(Tracer tracer) {
                 }
             }
 
-            /** Returns whether this is a required option or positional parameter.
+            /** Returns whether this is a required option or positional parameter without a default value.
              * If this argument is part of a {@linkplain ArgGroup group}, this method returns whether this argument is required <em>within the group</em> (so it is not necessarily a required argument for the command).
              * @see Option#required() */
-            public boolean required()      { return required; }
-
+            public boolean required() {
+                //#261 not required if it has a default; #676 default value may be a variable
+                return required && defaultValue() == null && defaultValueFromProvider() == null;
+            }
             /** Returns whether this option will prompt the user to enter a value on the command line.
              * @see Option#interactive() */
             public boolean interactive()   { return interactive; }
diff --git a/src/test/java/picocli/InterpolatedModelTest.java b/src/test/java/picocli/InterpolatedModelTest.java
index ba78e0877..f04409c92 100644
--- a/src/test/java/picocli/InterpolatedModelTest.java
+++ b/src/test/java/picocli/InterpolatedModelTest.java
@@ -6,6 +6,7 @@
 import org.junit.contrib.java.lang.system.RestoreSystemProperties;
 import org.junit.rules.TestRule;
 import picocli.CommandLine.Command;
+import picocli.CommandLine.MissingParameterException;
 import picocli.CommandLine.ParseResult;
 import picocli.CommandLine.Mixin;
 import picocli.CommandLine.Parameters;
@@ -265,6 +266,28 @@ void status(
         assertEquals(CommandLine.Range.valueOf("2..3"), status.getCommandSpec().findOption("x").arity());
     }
 
+    // https://github.com/remkop/picocli/issues/676
+    @Test
+    public void testIssue676() {
+        class Issue676 {
+            @Option(names="--mypath", defaultValue = "${sys:MYPATH}", required = true)
+            private String path;
+        }
+        System.clearProperty("MYPATH");
+        Issue676 bean = new Issue676();
+        CommandLine cmd = new CommandLine(bean);
+        try {
+            cmd.parseArgs();
+            fail("Expected exception");
+        } catch (MissingParameterException ex) {
+            assertEquals("Missing required option '--mypath=<path>'", ex.getMessage());
+        }
+
+        System.setProperty("MYPATH", "abc");
+        cmd.parseArgs();
+        assertEquals("abc", bean.path);
+    }
+
     static class CommonMixinOne {
         @Parameters(index = "${sys:commonParam1}", paramLabel = "COMMON-PARAM-ONE")
         private String commonMixinOneParam;