Core: Map methods should return immutable collections

apache · Oct 11, 2024 · 33b3a97 · 33b3a97
1 parent 67dc9e5
commit 33b3a97
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 8 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java b/api/src/main/java/org/apache/iceberg/util/CharSequenceMap.java
@@ -20,6 +20,7 @@
 
 import java.io.Serializable;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -132,12 +133,12 @@ public Set<CharSequence> keySet() {
       keySet.add(wrapper.get());
     }
 
-    return keySet;
+    return Collections.unmodifiableSet(keySet);
   }
 
   @Override
   public Collection<V> values() {
-    return wrapperMap.values();
+    return Collections.unmodifiableCollection(wrapperMap.values());
   }
 
   @Override
@@ -148,7 +149,7 @@ public Set<Entry<CharSequence, V>> entrySet() {
       entrySet.add(new CharSequenceEntry<>(entry));
     }
 
-    return entrySet;
+    return Collections.unmodifiableSet(entrySet);
   }
 
   public V computeIfAbsent(CharSequence key, Supplier<V> valueSupplier) {

diff --git a/api/src/test/java/org/apache/iceberg/util/TestCharSequenceMap.java b/api/src/test/java/org/apache/iceberg/util/TestCharSequenceMap.java
@@ -19,7 +19,11 @@
 package org.apache.iceberg.util;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -88,20 +92,38 @@ public void testClear() {
     assertThat(map).isEmpty();
   }
 
+  @Test
+  public void testKeySet() {
+    CharSequenceMap<String> map = CharSequenceMap.create();
+    map.put("key1", "value1");
+    map.put("key2", "value2");
+    Set<CharSequence> keySet = map.keySet();
+    assertThat(keySet).containsAll(ImmutableList.of("key1", "key2"));
+    assertThatThrownBy(() -> keySet.remove("key1"))
+        .isInstanceOf(UnsupportedOperationException.class);
+  }
+
   @Test
   public void testValues() {
     CharSequenceMap<String> map = CharSequenceMap.create();
     map.put("key1", "value1");
     map.put("key2", "value2");
-    assertThat(map.values()).containsAll(ImmutableList.of("value1", "value2"));
+    Collection<String> values = map.values();
+    assertThat(values).containsAll(ImmutableList.of("value1", "value2"));
+    assertThatThrownBy(() -> values.remove("value1"))
+        .isInstanceOf(UnsupportedOperationException.class);
   }
 
   @Test
   public void testEntrySet() {
     CharSequenceMap<String> map = CharSequenceMap.create();
     map.put("key1", "value1");
     map.put(new StringBuilder("key2"), "value2");
-    assertThat(map.entrySet()).hasSize(2);
+    Set<Map.Entry<CharSequence, String>> entrySet = map.entrySet();
+    assertThat(entrySet).hasSize(2);
+    Map.Entry<CharSequence, String> entryToRemove = entrySet.iterator().next();
+    assertThatThrownBy(() -> entrySet.remove(entryToRemove))
+        .isInstanceOf(UnsupportedOperationException.class);
   }
 
   @Test

diff --git a/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java b/core/src/main/java/org/apache/iceberg/util/StructLikeMap.java
@@ -20,6 +20,7 @@
 
 import java.util.AbstractMap;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
@@ -108,12 +109,12 @@ public Set<StructLike> keySet() {
     for (StructLikeWrapper wrapper : wrapperMap.keySet()) {
       keySet.add(wrapper.get());
     }
-    return keySet;
+    return Collections.unmodifiableSet(keySet);
   }
 
   @Override
   public Collection<T> values() {
-    return wrapperMap.values();
+    return Collections.unmodifiableCollection(wrapperMap.values());
   }
 
   @Override
@@ -122,7 +123,7 @@ public Set<Entry<StructLike, T>> entrySet() {
     for (Entry<StructLikeWrapper, T> entry : wrapperMap.entrySet()) {
       entrySet.add(new StructLikeEntry<>(entry));
     }
-    return entrySet;
+    return Collections.unmodifiableSet(entrySet);
   }
 
   private static class StructLikeEntry<R> implements Entry<StructLike, R> {

diff --git a/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java b/core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java
@@ -19,6 +19,7 @@
 package org.apache.iceberg.util;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.Collection;
 import java.util.Map;
@@ -63,6 +64,16 @@ public void testSingleRecord() {
       assertThat(entry.getValue()).isEqualTo("1-aaa");
       break;
     }
+
+    Map.Entry<StructLike, String> entryToRemove = entrySet.iterator().next();
+    assertThatThrownBy(() -> entrySet.remove(entryToRemove))
+        .isInstanceOf(UnsupportedOperationException.class);
+
+    assertThatThrownBy(() -> keySet.remove(record1))
+        .isInstanceOf(UnsupportedOperationException.class);
+
+    assertThatThrownBy(() -> values.remove("1-aaa"))
+        .isInstanceOf(UnsupportedOperationException.class);
   }
 
   @Test