steveloughran commented on code in PR #6546:
URL: https://github.com/apache/hadoop/pull/6546#discussion_r1490947738
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java:
##########
@@ -494,9 +504,22 @@ public static Map<String, String>
getTrimmedStringCollectionSplitByEquals(
String[] trimmedList = getTrimmedStrings(str);
Map<String, String> pairs = new HashMap<>();
for (String s : trimmedList) {
+ if (s.length() == 0) {
+ continue;
+ }
String[] splitByKeyVal = getTrimmedStringsSplitByEquals(s);
if (splitByKeyVal.length == 2) {
- pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
+ boolean emptyKey =
org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[0]);
+ boolean emptyVal =
org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[1]);
+ if (!emptyKey && !emptyVal) {
+ pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
+ } else {
+ throw new IllegalArgumentException(
Review Comment:
use Preconditions.checkArgument(!emptyKey && !emptyVal with its built in
string formatting.
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -792,6 +793,105 @@ public void testStringCollectionSplitByEquals() {
.containsEntry("element.xyz.key7", "element.abc.val7");
}
+ /**
+ * Tests for the string utility that will be used by S3A credentials
provider.
+ */
+ @Test
+ public void testStringCollectionSplitByEquals2() {
+ final Configuration configuration = new Configuration();
+ configuration.set("custom_key", " = element.abc.val1");
+ try {
+ S3AUtils.getTrimmedStringCollectionSplitByEquals(
Review Comment:
intercept(), and again, consider a factored out assertion to call repeatedly
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java:
##########
@@ -494,9 +504,22 @@ public static Map<String, String>
getTrimmedStringCollectionSplitByEquals(
String[] trimmedList = getTrimmedStrings(str);
Map<String, String> pairs = new HashMap<>();
for (String s : trimmedList) {
+ if (s.length() == 0) {
+ continue;
+ }
String[] splitByKeyVal = getTrimmedStringsSplitByEquals(s);
if (splitByKeyVal.length == 2) {
- pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
+ boolean emptyKey =
org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[0]);
+ boolean emptyVal =
org.apache.commons.lang3.StringUtils.isEmpty(splitByKeyVal[1]);
+ if (!emptyKey && !emptyVal) {
+ pairs.put(splitByKeyVal[0], splitByKeyVal[1]);
+ } else {
+ throw new IllegalArgumentException(
+ STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG + " Input: " + str);
+ }
+ } else {
+ throw new IllegalArgumentException(
Review Comment:
again, make a precondition on L511
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -792,6 +793,105 @@ public void testStringCollectionSplitByEquals() {
.containsEntry("element.xyz.key7", "element.abc.val7");
}
+ /**
+ * Tests for the string utility that will be used by S3A credentials
provider.
+ */
+ @Test
+ public void testStringCollectionSplitByEquals2() {
+ final Configuration configuration = new Configuration();
Review Comment:
use new Configuration(false); ensures no core-site/auth-keys values can
interferer
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestStringUtils.java:
##########
@@ -566,6 +568,86 @@ public void testStringCollectionSplitByEquals() {
.containsEntry("element.xyz.key5", "element.abc.val5")
.containsEntry("element.xyz.key6", "element.abc.val6")
.containsEntry("element.xyz.key7", "element.abc.val7");
+
+ try {
+ StringUtils.getTrimmedStringCollectionSplitByEquals(
+ " = element.abc.val1");
+ throw new RuntimeException("Expected to throw IllegalArgumentException");
+ } catch (IllegalArgumentException e) {
+ Assertions
+ .assertThat(e)
+ .describedAs("Exception thrown due to illegal arguments")
+ .hasMessageStartingWith(STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG);
+ }
+
+ 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");
+
+ try {
+ StringUtils.getTrimmedStringCollectionSplitByEquals(
Review Comment:
LambdaTestUtils.intercept(). It's predictable I will insist on that, so save
time in future. and it includes a string value check. so all you need is
```java
intercept(IllegalArgumentException.class,
STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG, () ->
StringUtils.getTrimmedStringCollectionSplitByEquals(
"element.abc.key1="));
```
See: much better. and if the closure doesn't fail, will even include the
string value of the result in the exception raised.
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AAWSCredentialsProvider.java:
##########
@@ -792,6 +793,105 @@ public void testStringCollectionSplitByEquals() {
.containsEntry("element.xyz.key7", "element.abc.val7");
}
+ /**
+ * Tests for the string utility that will be used by S3A credentials
provider.
+ */
+ @Test
+ public void testStringCollectionSplitByEquals2() {
+ final Configuration configuration = new Configuration();
+ configuration.set("custom_key", " = element.abc.val1");
+ try {
+ S3AUtils.getTrimmedStringCollectionSplitByEquals(
+ configuration, "custom_key");
+ throw new RuntimeException("Expected to throw IllegalArgumentException");
+ } catch (IllegalArgumentException e) {
+ Assertions
+ .assertThat(e)
+ .describedAs("Exception thrown due to illegal arguments")
+ .hasMessageStartingWith(STRING_COLLECTION_SPLIT_EQUALS_INVALID_ARG);
+ }
+
+ configuration.set(
Review Comment:
pull the expected-to-succeed tests into their own test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
