HADOOP-18980. Invalid inputs for getTrimmedStringCollectionSplitByEquals (ADDENDUM) (#6546)

This is a followup to #6406:
HADOOP-18980. S3A credential provider remapping: make extensible

It adds extra validation of key-value pairs in a configuration
option, with tests.

Contributed by Viraj Jasani

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java

@@ -40,6 +40,7 @@ import org.apache.commons.lang3.SystemUtils;
 import org.apache.commons.lang3.time.FastDateFormat;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.classification.VisibleForTesting;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.net.NetUtils;
 import org.apache.log4j.LogManager;
@@ -79,6 +80,18 @@ public class StringUtils {
   public static final Pattern ENV_VAR_PATTERN = Shell.WINDOWS ?
     WIN_ENV_VAR_PATTERN : SHELL_ENV_VAR_PATTERN;
 
+  /**
+   * {@link #getTrimmedStringCollectionSplitByEquals(String)} throws
+   * {@link IllegalArgumentException} with error message starting with this string
+   * if the argument provided is not valid representation of non-empty key-value
+   * pairs.
+   * Value = {@value}
+   */
+  @VisibleForTesting
+  public static final String STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG =
+      "Trimmed string split by equals does not correctly represent "
+          + "non-empty key-value pairs.";
+
   /**
    * Make a string representation of the exception.
    * @param e The exception to stringify
@@ -494,10 +507,19 @@ public class StringUtils {
     String[] trimmedList = getTrimmedStrings(str);
     Map<String, String> pairs = new HashMap<>();
     for (String s : trimmedList) {
-      String[] splitByKeyVal = getTrimmedStringsSplitByEquals(s);
-      if (splitByKeyVal.length == 2) {
-        pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
+      if (s.isEmpty()) {
+        continue;
       }
+      String[] splitByKeyVal = getTrimmedStringsSplitByEquals(s);
+      Preconditions.checkArgument(
+          splitByKeyVal.length == 2,
+          STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG + " Input: " + str);
+      boolean emptyKey = org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[0]);
+      boolean emptyVal = org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[1]);
+      Preconditions.checkArgument(
+          !emptyKey && !emptyVal,
+          STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG + " Input: " + str);
+      pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
     }
     return pairs;
   }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestStringUtils.java

@@ -19,6 +19,9 @@
 package org.apache.hadoop.util;
 
 import java.util.Locale;
+
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.apache.hadoop.util.StringUtils.STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG;
 import static org.apache.hadoop.util.StringUtils.TraditionalBinaryPrefix.long2String;
 import static org.apache.hadoop.util.StringUtils.TraditionalBinaryPrefix.string2long;
 import static org.junit.Assert.assertArrayEquals;
@@ -515,7 +518,7 @@ public class TestStringUtils extends UnitTestcaseTimeLimit {
   }
 
   @Test
-  public void testStringCollectionSplitByEquals() {
+  public void testStringCollectionSplitByEqualsSuccess() {
     Map<String, String> splitMap =
         StringUtils.getTrimmedStringCollectionSplitByEquals("");
     Assertions
@@ -566,6 +569,59 @@ public class TestStringUtils extends UnitTestcaseTimeLimit {
         .containsEntry("element.xyz.key5", "element.abc.val5")
         .containsEntry("element.xyz.key6", "element.abc.val6")
         .containsEntry("element.xyz.key7", "element.abc.val7");
+
+    splitMap = StringUtils.getTrimmedStringCollectionSplitByEquals(
+        "element.first.key1 = element.first.val2 ,element.first.key1 =element.first.val1");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(1)
+        .containsEntry("element.first.key1", "element.first.val1");
+
+    splitMap = StringUtils.getTrimmedStringCollectionSplitByEquals(
+        ",,, , ,, ,element.first.key1 = element.first.val2 ,"
+            + "element.first.key1 = element.first.val1 , ,,, ,");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(1)
+        .containsEntry("element.first.key1", "element.first.val1");
+
+    splitMap = StringUtils.getTrimmedStringCollectionSplitByEquals(
+        ",, , ,      ,, ,");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(0);
+
+  }
+
+  @Test
+  public void testStringCollectionSplitByEqualsFailure() throws Exception {
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> StringUtils.getTrimmedStringCollectionSplitByEquals(" = element.abc.val1"));
+
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> StringUtils.getTrimmedStringCollectionSplitByEquals("element.abc.key1="));
+
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> StringUtils.getTrimmedStringCollectionSplitByEquals("="));
+
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> StringUtils.getTrimmedStringCollectionSplitByEquals("== = =    ="));
+
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> StringUtils.getTrimmedStringCollectionSplitByEquals(",="));
   }
 
   // Benchmark for StringUtils split

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java

@@ -70,6 +70,7 @@ import static org.apache.hadoop.fs.s3a.impl.InstantiationIOException.DOES_NOT_IM
 import static org.apache.hadoop.fs.s3a.test.PublicDatasetTestUtils.getExternalData;
 import static org.apache.hadoop.test.LambdaTestUtils.intercept;
 import static org.apache.hadoop.test.LambdaTestUtils.interceptFuture;
+import static org.apache.hadoop.util.StringUtils.STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG;
 
 /**
  * Unit tests for {@link Constants#AWS_CREDENTIALS_PROVIDER} logic.
@@ -722,8 +723,8 @@ public class TestS3AAWSCredentialsProvider extends AbstractS3ATestBase {
    * Tests for the string utility that will be used by S3A credentials provider.
    */
   @Test
-  public void testStringCollectionSplitByEquals() {
-    final Configuration configuration = new Configuration();
+  public void testStringCollectionSplitByEqualsSuccess() {
+    final Configuration configuration = new Configuration(false);
     configuration.set("custom_key", "");
     Map<String, String> splitMap =
         S3AUtils.getTrimmedStringCollectionSplitByEquals(
@@ -775,8 +776,7 @@ public class TestS3AAWSCredentialsProvider extends AbstractS3ATestBase {
             + "element.abc.val5 ,\n \n \n "
             + " element.xyz.key6      =       element.abc.val6 \n , \n"
             + "element.xyz.key7=element.abc.val7,\n");
-    splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals(
-        configuration, "custom_key");
+    splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals(configuration, "custom_key");
 
     Assertions
         .assertThat(splitMap)
@@ -790,6 +790,75 @@ public class TestS3AAWSCredentialsProvider extends AbstractS3ATestBase {
         .containsEntry("element.xyz.key5", "element.abc.val5")
         .containsEntry("element.xyz.key6", "element.abc.val6")
         .containsEntry("element.xyz.key7", "element.abc.val7");
+
+    configuration.set("custom_key",
+        "element.first.key1 = element.first.val2 ,element.first.key1 =element.first.val1");
+    splitMap =
+        S3AUtils.getTrimmedStringCollectionSplitByEquals(
+            configuration, "custom_key");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(1)
+        .containsEntry("element.first.key1", "element.first.val1");
+
+    configuration.set("custom_key",
+        ",,, , ,, ,element.first.key1 = element.first.val2 ,"
+            + "element.first.key1 = element.first.val1 , ,,, ,");
+    splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals(
+        configuration, "custom_key");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(1)
+        .containsEntry("element.first.key1", "element.first.val1");
+
+    configuration.set("custom_key", ",, , ,      ,, ,");
+    splitMap = S3AUtils.getTrimmedStringCollectionSplitByEquals(
+        configuration, "custom_key");
+    Assertions
+        .assertThat(splitMap)
+        .describedAs("Map of key value pairs split by equals(=) and comma(,)")
+        .hasSize(0);
+  }
+
+  /**
+   * Validates that the argument provided is invalid by intercepting the expected
+   * Exception.
+   *
+   * @param propKey The property key to validate.
+   * @throws Exception If any error occurs.
+   */
+  private static void expectInvalidArgument(final String propKey) throws Exception {
+    final Configuration configuration = new Configuration(false);
+    configuration.set("custom_key", propKey);
+
+    intercept(
+        IllegalArgumentException.class,
+        STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG,
+        () -> S3AUtils.getTrimmedStringCollectionSplitByEquals(
+            configuration, "custom_key"));
+  }
+
+  /**
+   * Tests for the string utility that will be used by S3A credentials provider.
+   */
+  @Test
+  public void testStringCollectionSplitByEqualsFailure() throws Exception {
+    expectInvalidArgument(" = element.abc.val1");
+    expectInvalidArgument("=element.abc.val1");
+    expectInvalidArgument("= element.abc.val1");
+    expectInvalidArgument(" =element.abc.val1");
+    expectInvalidArgument("element.abc.key1=");
+    expectInvalidArgument("element.abc.key1= ");
+    expectInvalidArgument("element.abc.key1 =");
+    expectInvalidArgument("element.abc.key1 = ");
+    expectInvalidArgument("=");
+    expectInvalidArgument(" =");
+    expectInvalidArgument("= ");
+    expectInvalidArgument(" = ");
+    expectInvalidArgument("== = =    =");
+    expectInvalidArgument(", = ");
   }
 
   /**