From d77a28aac3c185f1fa8436f26576febc69d98ae9 Mon Sep 17 00:00:00 2001
From: Sam Brannen <sbrannen@pivotal.io>
Date: Mon, 3 Feb 2020 15:03:43 +0100
Subject: [PATCH] Support optional command line arguments with empty values

Spring Framework provides two implementations of the
CommandLinePropertySource API: SimpleCommandLinePropertySource and
JOptCommandLinePropertySource.

Prior to this commit, JOptCommandLinePropertySource supported empty
values for optional arguments; whereas, SimpleCommandLinePropertySource
did not.

This commit modifies the implementation of SimpleCommandLinePropertySource
to allow empty values for optional arguments.

Closes gh-24464
---
 .../core/env/SimpleCommandLineArgsParser.java | 31 +++++++-----
 .../env/SimpleCommandLinePropertySource.java  | 32 ++++++++-----
 .../JOptCommandLinePropertySourceTests.java   | 18 +++++--
 ... => SimpleCommandLineArgsParserTests.java} | 48 +++++++++----------
 .../SimpleCommandLinePropertySourceTests.java | 11 ++++-
 5 files changed, 85 insertions(+), 55 deletions(-)
 rename spring-core/src/test/java/org/springframework/core/env/{SimpleCommandLineParserTests.java => SimpleCommandLineArgsParserTests.java} (63%)

diff --git a/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLineArgsParser.java b/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLineArgsParser.java
index e2c57def91..bcf8d07160 100644
--- a/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLineArgsParser.java
+++ b/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLineArgsParser.java
@@ -21,15 +21,20 @@ package org.springframework.core.env;
  * {@link CommandLineArgs} object.
  *
  * <h3>Working with option arguments</h3>
- * Option arguments must adhere to the exact syntax:
+ * <p>Option arguments must adhere to the exact syntax:
+ *
  * <pre class="code">--optName[=optValue]</pre>
- * That is, options must be prefixed with "{@code --}", and may or may not specify a value.
- * If a value is specified, the name and value must be separated <em>without spaces</em>
- * by an equals sign ("=").
+ *
+ * <p>That is, options must be prefixed with "{@code --}" and may or may not
+ * specify a value. If a value is specified, the name and value must be separated
+ * <em>without spaces</em> by an equals sign ("="). The value may optionally be
+ * an empty string.
  *
  * <h4>Valid examples of option arguments</h4>
  * <pre class="code">
  * --foo
+ * --foo=
+ * --foo=""
  * --foo=bar
  * --foo="bar then baz"
  * --foo=bar,baz,biz</pre>
@@ -42,11 +47,12 @@ package org.springframework.core.env;
  * --foo=bar --foo=baz --foo=biz</pre>
  *
  * <h3>Working with non-option arguments</h3>
- * Any and all arguments specified at the command line without the "{@code --}" option
- * prefix will be considered as "non-option arguments" and made available through the
- * {@link CommandLineArgs#getNonOptionArgs()} method.
+ * <p>Any and all arguments specified at the command line without the "{@code --}"
+ * option prefix will be considered as "non-option arguments" and made available
+ * through the {@link CommandLineArgs#getNonOptionArgs()} method.
  *
  * @author Chris Beams
+ * @author Sam Brannen
  * @since 3.1
  */
 class SimpleCommandLineArgsParser {
@@ -61,17 +67,18 @@ class SimpleCommandLineArgsParser {
 		CommandLineArgs commandLineArgs = new CommandLineArgs();
 		for (String arg : args) {
 			if (arg.startsWith("--")) {
-				String optionText = arg.substring(2, arg.length());
+				String optionText = arg.substring(2);
 				String optionName;
 				String optionValue = null;
-				if (optionText.contains("=")) {
-					optionName = optionText.substring(0, optionText.indexOf('='));
-					optionValue = optionText.substring(optionText.indexOf('=') + 1, optionText.length());
+				int indexOfEqualsSign = optionText.indexOf('=');
+				if (indexOfEqualsSign > -1) {
+					optionName = optionText.substring(0, indexOfEqualsSign);
+					optionValue = optionText.substring(indexOfEqualsSign + 1);
 				}
 				else {
 					optionName = optionText;
 				}
-				if (optionName.isEmpty() || (optionValue != null && optionValue.isEmpty())) {
+				if (optionName.isEmpty()) {
 					throw new IllegalArgumentException("Invalid argument syntax: " + arg);
 				}
 				commandLineArgs.addOptionArg(optionName, optionValue);
diff --git a/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLinePropertySource.java b/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLinePropertySource.java
index 8bbc88b7ee..a4a25397ac 100644
--- a/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLinePropertySource.java
+++ b/spring-core/src/main/java/org/springframework/core/env/SimpleCommandLinePropertySource.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,22 +25,28 @@ import org.springframework.util.StringUtils;
  * {@link CommandLinePropertySource} implementation backed by a simple String array.
  *
  * <h3>Purpose</h3>
- * This {@code CommandLinePropertySource} implementation aims to provide the simplest
- * possible approach to parsing command line arguments.  As with all {@code
+ * <p>This {@code CommandLinePropertySource} implementation aims to provide the simplest
+ * possible approach to parsing command line arguments. As with all {@code
  * CommandLinePropertySource} implementations, command line arguments are broken into two
  * distinct groups: <em>option arguments</em> and <em>non-option arguments</em>, as
- * described below <em>(some sections copied from Javadoc for {@link SimpleCommandLineArgsParser})</em>:
+ * described below <em>(some sections copied from Javadoc for
+ * {@link SimpleCommandLineArgsParser})</em>:
  *
  * <h3>Working with option arguments</h3>
- * Option arguments must adhere to the exact syntax:
+ * <p>Option arguments must adhere to the exact syntax:
+ *
  * <pre class="code">--optName[=optValue]</pre>
- * That is, options must be prefixed with "{@code --}", and may or may not specify a value.
- * If a value is specified, the name and value must be separated <em>without spaces</em>
- * by an equals sign ("=").
+ *
+ * <p>That is, options must be prefixed with "{@code --}" and may or may not
+ * specify a value. If a value is specified, the name and value must be separated
+ * <em>without spaces</em> by an equals sign ("="). The value may optionally be
+ * an empty string.
  *
  * <h4>Valid examples of option arguments</h4>
  * <pre class="code">
  * --foo
+ * --foo=
+ * --foo=""
  * --foo=bar
  * --foo="bar then baz"
  * --foo=bar,baz,biz</pre>
@@ -53,11 +59,11 @@ import org.springframework.util.StringUtils;
  * --foo=bar --foo=baz --foo=biz</pre>
  *
  * <h3>Working with non-option arguments</h3>
- * Any and all arguments specified at the command line without the "{@code --}" option
- * prefix will be considered as "non-option arguments" and made available through the
- * {@link #getNonOptionArgs()} method.
+ * <p>Any and all arguments specified at the command line without the "{@code --}"
+ * option prefix will be considered as "non-option arguments" and made available
+ * through the {@link CommandLineArgs#getNonOptionArgs()} method.
  *
- * <h2>Typical usage</h2>
+ * <h3>Typical usage</h3>
  * <pre class="code">
  * public static void main(String[] args) {
  *     PropertySource<?> ps = new SimpleCommandLinePropertySource(args);
@@ -71,7 +77,7 @@ import org.springframework.util.StringUtils;
  * <p>When more fully-featured command line parsing is necessary, consider using
  * the provided {@link JOptCommandLinePropertySource}, or implement your own
  * {@code CommandLinePropertySource} against the command line parsing library of your
- * choice!
+ * choice.
  *
  * @author Chris Beams
  * @since 3.1
diff --git a/spring-core/src/test/java/org/springframework/core/env/JOptCommandLinePropertySourceTests.java b/spring-core/src/test/java/org/springframework/core/env/JOptCommandLinePropertySourceTests.java
index b2b4063557..0ebb4e99e8 100644
--- a/spring-core/src/test/java/org/springframework/core/env/JOptCommandLinePropertySourceTests.java
+++ b/spring-core/src/test/java/org/springframework/core/env/JOptCommandLinePropertySourceTests.java
@@ -28,6 +28,7 @@ import static org.assertj.core.api.Assertions.assertThat;
  * Unit tests for {@link JOptCommandLinePropertySource}.
  *
  * @author Chris Beams
+ * @author Sam Brannen
  * @since 3.1
  */
 class JOptCommandLinePropertySourceTests {
@@ -53,6 +54,17 @@ class JOptCommandLinePropertySourceTests {
 		assertThat(ps.getProperty("foo")).isEqualTo("");
 	}
 
+	@Test // gh-24464
+	void withOptionalArg_andArgIsEmpty() {
+		OptionParser parser = new OptionParser();
+		parser.accepts("foo").withOptionalArg();
+		OptionSet options = parser.parse("--foo=");
+
+		PropertySource<?> ps = new JOptCommandLinePropertySource(options);
+		assertThat(ps.containsProperty("foo")).isTrue();
+		assertThat(ps.getProperty("foo")).isEqualTo("");
+	}
+
 	@Test
 	void withNoArg() {
 		OptionParser parser = new OptionParser();
@@ -74,7 +86,7 @@ class JOptCommandLinePropertySourceTests {
 		OptionSet options = parser.parse("--foo=bar,baz,biz");
 
 		CommandLinePropertySource<?> ps = new JOptCommandLinePropertySource(options);
-		assertThat(ps.getOptionValues("foo")).isEqualTo(Arrays.asList("bar","baz","biz"));
+		assertThat(ps.getOptionValues("foo")).containsExactly("bar", "baz", "biz");
 		assertThat(ps.getProperty("foo")).isEqualTo("bar,baz,biz");
 	}
 
@@ -85,7 +97,7 @@ class JOptCommandLinePropertySourceTests {
 		OptionSet options = parser.parse("--foo=bar", "--foo=baz", "--foo=biz");
 
 		CommandLinePropertySource<?> ps = new JOptCommandLinePropertySource(options);
-		assertThat(ps.getOptionValues("foo")).isEqualTo(Arrays.asList("bar","baz","biz"));
+		assertThat(ps.getOptionValues("foo")).containsExactly("bar", "baz", "biz");
 		assertThat(ps.getProperty("foo")).isEqualTo("bar,baz,biz");
 	}
 
@@ -123,7 +135,7 @@ class JOptCommandLinePropertySourceTests {
 
 		assertThat(ps.containsProperty("nonOptionArgs")).isFalse();
 		assertThat(ps.getProperty("nonOptionArgs")).isNull();
-		assertThat(ps.getPropertyNames().length).isEqualTo(2);
+		assertThat(ps.getPropertyNames()).hasSize(2);
 	}
 
 	@Test
diff --git a/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineParserTests.java b/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineArgsParserTests.java
similarity index 63%
rename from spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineParserTests.java
rename to spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineArgsParserTests.java
index 471af3e759..4e3f186f87 100644
--- a/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineParserTests.java
+++ b/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLineArgsParserTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -25,17 +25,24 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 
-class SimpleCommandLineParserTests {
+/**
+ * Unit tests for {@link SimpleCommandLineArgsParser}.
+ *
+ * @author Chris Beams
+ * @author Sam Brannen
+ */
+class SimpleCommandLineArgsParserTests {
+
+	private final SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
+
 
 	@Test
 	void withNoOptions() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
 		assertThat(parser.parse().getOptionValues("foo")).isNull();
 	}
 
 	@Test
 	void withSingleOptionAndNoValue() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
 		CommandLineArgs args = parser.parse("--o1");
 		assertThat(args.containsOption("o1")).isTrue();
 		assertThat(args.getOptionValues("o1")).isEqualTo(Collections.EMPTY_LIST);
@@ -43,63 +50,52 @@ class SimpleCommandLineParserTests {
 
 	@Test
 	void withSingleOptionAndValue() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
 		CommandLineArgs args = parser.parse("--o1=v1");
 		assertThat(args.containsOption("o1")).isTrue();
-		assertThat(args.getOptionValues("o1").get(0)).isEqualTo("v1");
+		assertThat(args.getOptionValues("o1")).containsExactly("v1");
 	}
 
 	@Test
 	void withMixOfOptionsHavingValueAndOptionsHavingNoValue() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
 		CommandLineArgs args = parser.parse("--o1=v1", "--o2");
 		assertThat(args.containsOption("o1")).isTrue();
 		assertThat(args.containsOption("o2")).isTrue();
 		assertThat(args.containsOption("o3")).isFalse();
-		assertThat(args.getOptionValues("o1").get(0)).isEqualTo("v1");
+		assertThat(args.getOptionValues("o1")).containsExactly("v1");
 		assertThat(args.getOptionValues("o2")).isEqualTo(Collections.EMPTY_LIST);
 		assertThat(args.getOptionValues("o3")).isNull();
 	}
 
 	@Test
 	void withEmptyOptionText() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
-		assertThatIllegalArgumentException().isThrownBy(() ->
-				parser.parse("--"));
+		assertThatIllegalArgumentException().isThrownBy(() -> parser.parse("--"));
 	}
 
 	@Test
 	void withEmptyOptionName() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
-		assertThatIllegalArgumentException().isThrownBy(() ->
-				parser.parse("--=v1"));
+		assertThatIllegalArgumentException().isThrownBy(() -> parser.parse("--=v1"));
 	}
 
 	@Test
 	void withEmptyOptionValue() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
-		assertThatIllegalArgumentException().isThrownBy(() ->
-				parser.parse("--o1="));
+		CommandLineArgs args = parser.parse("--o1=");
+		assertThat(args.containsOption("o1")).isTrue();
+		assertThat(args.getOptionValues("o1")).containsExactly("");
 	}
 
 	@Test
 	void withEmptyOptionNameAndEmptyOptionValue() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
-		assertThatIllegalArgumentException().isThrownBy(() ->
-				parser.parse("--="));
+		assertThatIllegalArgumentException().isThrownBy(() -> parser.parse("--="));
 	}
 
 	@Test
 	void withNonOptionArguments() {
-		SimpleCommandLineArgsParser parser = new SimpleCommandLineArgsParser();
 		CommandLineArgs args = parser.parse("--o1=v1", "noa1", "--o2=v2", "noa2");
-		assertThat(args.getOptionValues("o1").get(0)).isEqualTo("v1");
-		assertThat(args.getOptionValues("o2").get(0)).isEqualTo("v2");
+		assertThat(args.getOptionValues("o1")).containsExactly("v1");
+		assertThat(args.getOptionValues("o2")).containsExactly("v2");
 
 		List<String> nonOptions = args.getNonOptionArgs();
-		assertThat(nonOptions.get(0)).isEqualTo("noa1");
-		assertThat(nonOptions.get(1)).isEqualTo("noa2");
-		assertThat(nonOptions.size()).isEqualTo(2);
+		assertThat(nonOptions).containsExactly("noa1", "noa2");
 	}
 
 	@Test
diff --git a/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLinePropertySourceTests.java b/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLinePropertySourceTests.java
index 4d4ac70d15..9b67142de7 100644
--- a/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLinePropertySourceTests.java
+++ b/spring-core/src/test/java/org/springframework/core/env/SimpleCommandLinePropertySourceTests.java
@@ -26,6 +26,7 @@ import static org.assertj.core.api.Assertions.assertThat;
  * Unit tests for {@link SimpleCommandLinePropertySource}.
  *
  * @author Chris Beams
+ * @author Sam Brannen
  * @since 3.1
  */
 class SimpleCommandLinePropertySourceTests {
@@ -60,6 +61,14 @@ class SimpleCommandLinePropertySourceTests {
 		assertThat(ps.getProperty("o3")).isNull();
 	}
 
+	@Test // gh-24464
+	void withOptionalArg_andArgIsEmpty() {
+		EnumerablePropertySource<?> ps = new SimpleCommandLinePropertySource("--foo=");
+
+		assertThat(ps.containsProperty("foo")).isTrue();
+		assertThat(ps.getProperty("foo")).isEqualTo("");
+	}
+
 	@Test
 	void withDefaultNonOptionArgsNameAndNoNonOptionArgsPresent() {
 		EnumerablePropertySource<?> ps = new SimpleCommandLinePropertySource("--o1=v1", "--o2");
@@ -70,7 +79,7 @@ class SimpleCommandLinePropertySourceTests {
 
 		assertThat(ps.containsProperty("nonOptionArgs")).isFalse();
 		assertThat(ps.getProperty("nonOptionArgs")).isNull();
-		assertThat(ps.getPropertyNames().length).isEqualTo(2);
+		assertThat(ps.getPropertyNames()).hasSize(2);
 	}
 
 	@Test