HADOOP-19161. review feedback + copy() operation

* Mukund's feedback
* Remove unused "throws Throwable" on test cases as appropriate

Save enumClass constructor parameter of FlagSet
* add accessor
* compare in equals
* use in a copy() operation which creates a mutable deep copy
  of an object.
* explicit checks for null enumclass, prefix in constructor

Change-Id: I8080d772bfd301ab6e3ed802d54a060a63feea78

Author: steveloughran

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/FlagSet.java

@@ -23,7 +23,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
@@ -33,6 +32,7 @@
 import org.apache.hadoop.util.ConfigurationHelper;
 import org.apache.hadoop.util.Preconditions;
 
+import static java.util.Objects.requireNonNull;
 import static org.apache.hadoop.util.ConfigurationHelper.mapEnumNamesToValues;
 
 /**
@@ -47,10 +47,22 @@
  */
 public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
 
+  /**
+   * Class of the enum.
+   * Used for duplicating the flags as java type erasure
+   * loses this information otherwise.
+   */
+  private final Class<E> enumClass;
+
+  /**
+   * Prefix for path capabilities probe.
+   */
+  private final String prefix;
+
   /**
    * Set of flags.
    */
-  private final Set<E> flags;
+  private final EnumSet<E> flags;
 
   /**
    * Is the set immutable?
@@ -71,7 +83,8 @@ public final class FlagSet<E extends Enum<E>> implements StreamCapabilities {
   private FlagSet(final Class<E> enumClass,
       final String prefix,
       @Nullable final EnumSet<E> flags) {
-
+    this.enumClass = requireNonNull(enumClass, "null enumClass");
+    this.prefix = requireNonNull(prefix, "null prefix");
     this.flags = flags != null
         ? EnumSet.copyOf(flags)
         : EnumSet.noneOf(enumClass);
@@ -164,6 +177,22 @@ public void makeImmutable() {
     immutable.set(true);
   }
 
+  /**
+   * Is the FlagSet immutable?
+   * @return true iff the FlagSet is immutable.
+   */
+  public boolean isImmutable() {
+    return immutable.get();
+  }
+
+  /**
+   * Get the enum class.
+   * @return the enum class.
+   */
+  public Class<E> getEnumClass() {
+    return enumClass;
+  }
+
   @Override
   public String toString() {
     return "{" +
@@ -183,11 +212,16 @@ public List<String> pathCapabilities() {
         .collect(Collectors.toList());
   }
 
-
   /**
-   * Equality is based on the set.
+   * Equality is based on the value of {@link #enumClass} and
+   * {@link #prefix} and the contents of the set, which must match.
+   * <p>
+   * The immutability flag is not considered, nor is the
+   * {@link #namesToValues} map, though as that is generated from
+   * the enumeration and prefix, it is implicitly equal if the prefix
+   * and enumClass fields are equal.
    * @param o other object
-   * @return true iff the flags are equal.
+   * @return true iff the equality condition is met.
    */
   @Override
   public boolean equals(final Object o) {
@@ -198,7 +232,9 @@ public boolean equals(final Object o) {
       return false;
     }
     FlagSet<?> flagSet = (FlagSet<?>) o;
-    return Objects.equals(flags, flagSet.flags);
+    return Objects.equals(enumClass, flagSet.enumClass)
+        && Objects.equals(prefix, flagSet.prefix)
+        && Objects.equals(flags, flagSet.flags);
   }
 
   /**
@@ -210,14 +246,22 @@ public int hashCode() {
     return Objects.hashCode(flags);
   }
 
+  /**
+   * Create a copy of the FlagSet.
+   * @return a new mutable instance with a separate copy of the flags
+   */
+  public FlagSet<E> copy() {
+    return new FlagSet<>(enumClass,  prefix, flags);
+  }
+
   /**
    * Convert to a string which can be then set in a configuration.
    * This is effectively a marshalled form of the flags.
    * @return a comma separated list of flag names.
    */
   public String toConfigurationString() {
     return flags.stream()
-        .map(e -> e.name())
+        .map(Enum::name)
         .collect(Collectors.joining(", "));
   }
 
@@ -244,12 +288,17 @@ public static <E extends Enum<E>> FlagSet<E> createFlagSet(
    * @param <E> enum type
    * @return a mutable FlagSet
    */
+  @SafeVarargs
   public static <E extends Enum<E>> FlagSet<E> createFlagSet(
       final Class<E> enumClass,
       final String prefix,
       final E... enabled) {
     final FlagSet<E> flagSet = new FlagSet<>(enumClass, prefix, null);
-    Arrays.stream(enabled).forEach(flagSet::enable);
+    Arrays.stream(enabled).forEach(flag -> {
+      if (flag != null) {
+        flagSet.enable(flag);
+      }
+    });
     return flagSet;
   }
 

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

@@ -55,13 +55,13 @@ private ConfigurationHelper() {
    * trim the list, map to enum values in the message (case insensitive)
    * and return the set.
    * Special handling of "*" meaning: all values.
-   * @param key key for error messages.
+   * @param key Configuration object key -used in error messages.
    * @param valueString value from Configuration
    * @param enumClass class of enum
    * @param ignoreUnknown should unknown values be ignored?
    * @param <E> enum type
-   * @return a mutable set of enum values listed in the valueString, with any unknown
-   * matches stripped.
+   * @return a mutable set of enum values parsed from the valueString, with any unknown
+   * matches stripped if {@code ignoreUnknown} is true.
    * @throws IllegalArgumentException if one of the entries was unknown and ignoreUnknown is false,
    * or there are two entries in the enum which differ only by case.
    */

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/impl/TestFlagSet.java

@@ -58,6 +58,11 @@ public final class TestFlagSet extends AbstractHadoopTestBase {
    */
   private enum SimpleEnum { a, b, c }
 
+  /**
+   * Enum with a single value.
+   */
+  private enum OtherEnum { a }
+
   /**
    * Test that an entry can be enabled and disabled.
    */
@@ -312,6 +317,50 @@ public void testInequality() {
         .isNotEqualTo(s2);
   }
 
+  @Test
+  public void testClassInequality() {
+    final FlagSet<?> s1 =
+        createFlagSet(SimpleEnum.class, KEYDOT, noneOf(SimpleEnum.class));
+    final FlagSet<?> s2 =
+        createFlagSet(OtherEnum.class, KEYDOT, OtherEnum.a);
+    Assertions.assertThat(s1)
+        .describedAs("s1 == s2")
+        .isNotEqualTo(s2);
+  }
+
+  /**
+   * The copy operation creates a new instance which is now mutable,
+   * even if the original was immutable.
+   */
+  @Test
+  public void testCopy() throws Throwable {
+    FlagSet<SimpleEnum> s1 =
+            createFlagSet(SimpleEnum.class, KEYDOT, SimpleEnum.a, SimpleEnum.b);
+    s1.makeImmutable();
+    FlagSet<SimpleEnum> s2 = s1.copy();
+    Assertions.assertThat(s2)
+        .describedAs("copy of %s", s1)
+        .isNotSameAs(s1);
+    Assertions.assertThat(!s2.isImmutable())
+        .describedAs("set %s is immutable", s2)
+        .isTrue();
+    Assertions.assertThat(s1)
+        .describedAs("s1 == s2")
+        .isEqualTo(s2);
+  }
+
+  @Test
+  public void testCreateNullEnumClass() throws Throwable {
+    intercept(NullPointerException.class, () ->
+        createFlagSet(null, KEYDOT, SimpleEnum.a));
+  }
+
+  @Test
+  public void testCreateNullPrefix() throws Throwable {
+    intercept(NullPointerException.class, () ->
+        createFlagSet(SimpleEnum.class, null, SimpleEnum.a));
+  }
+
   /**
    * Round trip a FlagSet.
    * @param flagset FlagSet to save to a configuration and retrieve.
@@ -354,7 +403,7 @@ private void assertFlags(final SimpleEnum... flags) {
   }
 
   /**
-   * Assert that a flagset contains an exclusive set of values.
+   * Assert that a FlagSet contains an exclusive set of values.
    * @param flags flags which must be set.
    */
   private void assertFlagSetMatches(

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

@@ -82,25 +82,25 @@ private Configuration confWithKey(String value) {
   }
 
   @Test
-  public void testEnumParseAll() throws Throwable {
+  public void testEnumParseAll() {
     assertEnumParse("*", SimpleEnum.class, false)
         .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i);
   }
 
   @Test
-  public void testEnumParse() throws Throwable {
+  public void testEnumParse() {
     assertEnumParse("a, b,c", SimpleEnum.class, false)
         .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c);
   }
 
   @Test
-  public void testEnumCaseIndependence() throws Throwable {
+  public void testEnumCaseIndependence() {
     assertEnumParse("A, B, C, I", SimpleEnum.class, false)
         .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i);
   }
 
   @Test
-  public void testEmptyArguments() throws Throwable {
+  public void testEmptyArguments() {
     assertEnumParse(" ", SimpleEnum.class, false)
         .isEmpty();
   }
@@ -119,25 +119,19 @@ public void testUnknownEnumNotIgnoredThroughConf() throws Throwable {
   }
 
   @Test
-  public void testUnknownEnumIgnored() throws Throwable {
+  public void testUnknownEnumIgnored() {
     assertEnumParse("c, d", SimpleEnum.class, true)
         .containsExactly(SimpleEnum.c);
   }
 
-  @Test
-  public void testStarEnum() throws Throwable {
-    assertEnumParse("*", SimpleEnum.class, false)
-        .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i);
-  }
-
   @Test
   public void testUnknownStarEnum() throws Throwable {
     intercept(IllegalArgumentException.class, "unrecognized", () ->
         parseEnumSet("key", "*, unrecognized", SimpleEnum.class, false));
   }
 
   @Test
-  public void testUnknownStarEnumIgnored() throws Throwable {
+  public void testUnknownStarEnumIgnored() {
     assertEnumParse("*, d", SimpleEnum.class, true)
         .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c, SimpleEnum.i);
   }
@@ -157,7 +151,7 @@ public void testCaseConflictingEnumNotSupported() throws Throwable {
   }
 
   @Test
-  public void testEmptyEnumMap() throws Throwable {
+  public void testEmptyEnumMap() {
     Assertions.assertThat(mapEnumNamesToValues("", EmptyEnum.class))
         .isEmpty();
   }
@@ -166,8 +160,15 @@ public void testEmptyEnumMap() throws Throwable {
    * A star enum for an empty enum must be empty.
    */
   @Test
-  public void testEmptyStarEnum() throws Throwable {
+  public void testEmptyStarEnum() {
     assertEnumParse("*", EmptyEnum.class, false)
         .isEmpty();
   }
+
+  @Test
+  public void testDuplicateValues() {
+    assertEnumParse("a, a, c, b, c", SimpleEnum.class, true)
+        .containsExactly(SimpleEnum.a, SimpleEnum.b, SimpleEnum.c);
+  }
+
 }