From b4c3bb5d214462532a50a1939b6692dbcbd25533 Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Tue, 11 Apr 2017 18:30:05 -0400
Subject: [PATCH] Reject duplicate settings on the command line

Today Elasticsearch and other CLI tools that rely on environment aware
command leniently accept duplicate settings with the last one
winning. This commit removes this leniency.

Relates #24053
---
 .../cli/EnvironmentAwareCommand.java          | 11 +++++++-
 .../bootstrap/ElasticsearchCliTests.java      | 26 +++++++++++--------
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java b/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java
index 8372a6b8ab8..79a4fd7329f 100644
--- a/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java
+++ b/core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java
@@ -45,7 +45,16 @@ public abstract class EnvironmentAwareCommand extends Command {
         final Map<String, String> settings = new HashMap<>();
         for (final KeyValuePair kvp : settingOption.values(options)) {
             if (kvp.value.isEmpty()) {
-                throw new UserException(ExitCodes.USAGE, "Setting [" + kvp.key + "] must not be empty");
+                throw new UserException(ExitCodes.USAGE, "setting [" + kvp.key + "] must not be empty");
+            }
+            if (settings.containsKey(kvp.key)) {
+                final String message = String.format(
+                        Locale.ROOT,
+                        "setting [%s] already set, saw [%s] and [%s]",
+                        kvp.key,
+                        settings.get(kvp.key),
+                        kvp.value);
+                throw new UserException(ExitCodes.USAGE, message);
             }
             settings.put(kvp.key, kvp.value);
         }
diff --git a/core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java b/core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java
index 8a00a430dbc..07c5a7e157f 100644
--- a/core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java
+++ b/core/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java
@@ -79,22 +79,19 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase {
             false,
             output -> assertThat(output, containsString("Positional arguments not allowed, found [foo]")),
             (foreground, pidFile, quiet, esSettings) -> {},
-            "foo"
-        );
+            "foo");
         runTest(
             ExitCodes.USAGE,
             false,
             output -> assertThat(output, containsString("Positional arguments not allowed, found [foo, bar]")),
             (foreground, pidFile, quiet, esSettings) -> {},
-            "foo", "bar"
-        );
+            "foo", "bar");
         runTest(
             ExitCodes.USAGE,
             false,
             output -> assertThat(output, containsString("Positional arguments not allowed, found [foo]")),
             (foreground, pidFile, quiet, esSettings) -> {},
-            "-E", "foo=bar", "foo", "-E", "baz=qux"
-        );
+            "-E", "foo=bar", "foo", "-E", "baz=qux");
     }
 
     public void testThatPidFileCanBeConfigured() throws Exception {
@@ -157,18 +154,25 @@ public class ElasticsearchCliTests extends ESElasticsearchCliTestCase {
                     assertThat(settings, hasEntry("foo", "bar"));
                     assertThat(settings, hasEntry("baz", "qux"));
                 },
-                "-Efoo=bar", "-E", "baz=qux"
-        );
+                "-Efoo=bar", "-E", "baz=qux");
     }
 
     public void testElasticsearchSettingCanNotBeEmpty() throws Exception {
         runTest(
                 ExitCodes.USAGE,
                 false,
-                output -> assertThat(output, containsString("Setting [foo] must not be empty")),
+                output -> assertThat(output, containsString("setting [foo] must not be empty")),
                 (foreground, pidFile, quiet, esSettings) -> {},
-                "-E", "foo="
-        );
+                "-E", "foo=");
+    }
+
+    public void testElasticsearchSettingCanNotBeDuplicated() throws Exception {
+        runTest(
+                ExitCodes.USAGE,
+                false,
+                output -> assertThat(output, containsString("setting [foo] already set, saw [bar] and [baz]")),
+                (foreground, pidFile, quiet, initialEnv) -> {},
+                "-E", "foo=bar", "-E", "foo=baz");
     }
 
     public void testUnknownOption() throws Exception {