Merge pull request #2235 from ebean-orm/feature/2223

#2223 #2224 Support BeanMap modification via entrySet() and keySet()

ebean-api/src/main/java/io/ebean/common/BeanMap.java

@@ -175,10 +175,6 @@ public final class BeanMap<K, E> extends AbstractBeanCollection<E> implements Ma
 
   /**
    * Returns the map entrySet.
-   * <p>
-   * This is because the key values may need to be set against the details (so
-   * they don't need to be set twice).
-   * </p>
    */
   @Override
   public Collection<?> getActualEntries() {
@@ -242,17 +238,12 @@ public final class BeanMap<K, E> extends AbstractBeanCollection<E> implements Ma
   }
 
   @Override
-  @SuppressWarnings({"unchecked"})
   public Set<Entry<K, E>> entrySet() {
     init();
     if (isReadOnly()) {
       return Collections.unmodifiableSet(map.entrySet());
     }
-    if (modifyListening) {
-      Set<Entry<K, E>> s = map.entrySet();
-      return new ModifySet(this, s);
-    }
-    return map.entrySet();
+    return modifyListening ? new ModifyEntrySet<>(this, map.entrySet()) : map.entrySet();
   }
 
   @Override
@@ -273,8 +264,7 @@ public final class BeanMap<K, E> extends AbstractBeanCollection<E> implements Ma
     if (isReadOnly()) {
       return Collections.unmodifiableSet(map.keySet());
     }
-    // we don't really care about modifications to the ketSet?
-    return map.keySet();
+    return modifyListening ? new ModifyKeySet<>(this, map.keySet()) : map.keySet();
   }
 
   @Override
@@ -345,11 +335,7 @@ public final class BeanMap<K, E> extends AbstractBeanCollection<E> implements Ma
     if (isReadOnly()) {
       return Collections.unmodifiableCollection(map.values());
     }
-    if (modifyListening) {
-      Collection<E> c = map.values();
-      return new ModifyCollection<>(this, c);
-    }
-    return map.values();
+    return modifyListening ? new ModifyCollection<>(this, map.values()) : map.values();
   }
 
   @Override

ebean-api/src/main/java/io/ebean/common/ModifyCollection.java

@@ -25,7 +25,7 @@ class ModifyCollection<E> implements Collection<E> {
    * The owner is notified of the additions and removals.
    * </p>
    */
-  public ModifyCollection(BeanCollection<E> owner, Collection<E> c) {
+  ModifyCollection(BeanCollection<E> owner, Collection<E> c) {
     this.owner = owner;
     this.c = c;
   }

ebean-api/src/main/java/io/ebean/common/ModifyEntrySet.java

@@ -0,0 +1,131 @@
+package io.ebean.common;
+
+import java.util.*;
+
+/**
+ * Handles the Entry Set for BeanMap.
+ */
+class ModifyEntrySet<K, E> implements Set<Map.Entry<K, E>> {
+
+  private final BeanMap<K, E> owner;
+  private final Set<Map.Entry<K, E>> entrySet;
+
+  ModifyEntrySet(BeanMap<K, E> owner, Set<Map.Entry<K, E>> entrySet) {
+    this.owner = owner;
+    this.entrySet = entrySet;
+  }
+
+  @Override
+  public int size() {
+    return entrySet.size();
+  }
+
+  @Override
+  public boolean isEmpty() {
+    return entrySet.isEmpty();
+  }
+
+  @Override
+  public boolean contains(Object o) {
+    return entrySet.contains(o);
+  }
+
+  @Override
+  public Object[] toArray() {
+    return entrySet.toArray();
+  }
+
+  @Override
+  public <T> T[] toArray(T[] a) {
+    return entrySet.toArray(a);
+  }
+
+  @Override
+  public boolean containsAll(Collection<?> entries) {
+    return entrySet.containsAll(entries);
+  }
+
+  @Override
+  public void clear() {
+    owner.clear();
+  }
+
+  @Override
+  public boolean add(Map.Entry<K, E> entry) {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public boolean addAll(Collection<? extends Map.Entry<K, E>> c) {
+    throw new UnsupportedOperationException();
+  }
+
+  @SuppressWarnings("rawtypes")
+  @Override
+  public boolean remove(Object o) {
+    if (o instanceof Map.Entry) {
+      Map.Entry entry = (Map.Entry) o;
+      final E val = owner.get(entry.getKey());
+      if (Objects.equals(val, entry.getValue())) {
+        owner.remove(entry.getKey());
+        return true;
+      }
+    }
+    return false;
+  }
+
+  @Override
+  public boolean retainAll(Collection<?> entries) {
+    boolean modified = false;
+    final Iterator<Map.Entry<K, E>> it = iterator();
+    while (it.hasNext()) {
+      if (!entries.contains(it.next())) {
+        it.remove();
+        modified = true;
+      }
+    }
+    return modified;
+  }
+
+  @Override
+  public boolean removeAll(Collection<?> entries) {
+    boolean modified = false;
+    for (Object entry : entries) {
+      modified |= remove(entry);
+    }
+    return modified;
+  }
+
+  @Override
+  public Iterator<Map.Entry<K, E>> iterator() {
+    return new EntrySetIterator(new ArrayList<>(entrySet).iterator());
+  }
+
+  class EntrySetIterator implements Iterator<Map.Entry<K, E>> {
+
+    private final Iterator<Map.Entry<K, E>> iterator;
+    private Map.Entry<K, E> entry;
+
+    EntrySetIterator(Iterator<Map.Entry<K, E>> iterator) {
+      this.iterator = iterator;
+    }
+
+    @Override
+    public boolean hasNext() {
+      return iterator.hasNext();
+    }
+
+    @Override
+    public Map.Entry<K, E> next() {
+      entry = iterator.next();
+      return entry;
+    }
+
+    @Override
+    public void remove() {
+      owner.remove(entry.getKey());
+      iterator.remove();
+    }
+  }
+
+}

ebean-api/src/main/java/io/ebean/common/ModifyKeySet.java

@@ -0,0 +1,126 @@
+package io.ebean.common;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.Set;
+
+/**
+ * Handle the Key Set for BeanMap.
+ */
+class ModifyKeySet<E> implements Set<E> {
+
+  private final Set<E> keySet;
+  private final BeanMap<E, ?> owner;
+
+  ModifyKeySet(BeanMap<E, ?> owner, Set<E> keySet) {
+    this.owner = owner;
+    this.keySet = keySet;
+  }
+
+  @Override
+  public int size() {
+    return keySet.size();
+  }
+
+  @Override
+  public boolean isEmpty() {
+    return keySet.isEmpty();
+  }
+
+  @Override
+  public boolean contains(Object o) {
+    return keySet.contains(o);
+  }
+
+  @Override
+  public Object[] toArray() {
+    return keySet.toArray();
+  }
+
+  @Override
+  public <T> T[] toArray(T[] a) {
+    return keySet.toArray(a);
+  }
+
+  @Override
+  public boolean add(E key) {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public boolean addAll(Collection<? extends E> keys) {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public boolean remove(Object o) {
+    return owner.remove(o) != null;
+  }
+
+  @Override
+  public boolean containsAll(Collection<?> keys) {
+    return keySet.containsAll(keys);
+  }
+
+  @Override
+  public void clear() {
+    owner.clear();
+  }
+
+  @Override
+  public Iterator<E> iterator() {
+    return new KeySetIterator<>(new ArrayList<>(keySet).iterator());
+  }
+
+  @Override
+  public boolean retainAll(Collection<?> keys) {
+    return keysMatch(keys, false);
+  }
+
+  @Override
+  public boolean removeAll(Collection<?> keys) {
+    return keysMatch(keys, true);
+  }
+
+  private boolean keysMatch(Collection<?> keys, boolean containsMatch) {
+    boolean changed = false;
+    final Iterator<E> iterator = iterator();
+    while (iterator.hasNext()) {
+      final E key = iterator.next();
+      if (keys.contains(key) == containsMatch) {
+        iterator.remove();
+        changed = true;
+      }
+    }
+    return changed;
+  }
+
+
+  class KeySetIterator<K> implements Iterator<K> {
+
+    private final Iterator<K> iterator;
+    private K key;
+
+    KeySetIterator(Iterator<K> iterator) {
+      this.iterator = iterator;
+    }
+
+    @Override
+    public boolean hasNext() {
+      return iterator.hasNext();
+    }
+
+    @Override
+    public K next() {
+      key = iterator.next();
+      return key;
+    }
+
+    @Override
+    public void remove() {
+      owner.remove(key);
+      iterator.remove();
+    }
+  }
+}

ebean-api/src/main/java/io/ebean/common/ModifySet.java

@@ -1,24 +0,0 @@
-package io.ebean.common;
-
-import io.ebean.bean.BeanCollection;
-
-import java.util.Set;
-
-/**
- * Wraps a Set for the purposes of notifying removals and additions to the
- * BeanCollection owner.
- * <p>
- * This is required for persisting ManyToMany objects. Additions and removals
- * become inserts and deletes to the intersection table.
- * </p>
- */
-class ModifySet<E> extends ModifyCollection<E> implements Set<E> {
-
-  /**
-   * Create with an Owner that is notified of any additions or deletions.
-   */
-  public ModifySet(BeanCollection<E> owner, Set<E> s) {
-    super(owner, s);
-  }
-
-}

ebean-core/src/test/java/io/ebean/common/BeanMapTest.java

@@ -2,18 +2,21 @@ package io.ebean.common;
 
 import io.ebean.bean.BeanCollection;
 import org.junit.Test;
+import org.tests.model.basic.EBasic;
 
-import java.util.LinkedHashMap;
-import java.util.Map;
+import java.util.*;
 
+import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 
 public class BeanMapTest {
 
-  Object object1 = new Object();
-  Object object2 = new Object();
-  Object object3 = new Object();
+  private final EBasic object1 = new EBasic("o1");
+  private final EBasic object2 = new EBasic("o2");
+  private final EBasic object3 = new EBasic("o3");
+  private final EBasic object4 = new EBasic("o4");
+  private final EBasic object5 = new EBasic("o5");
 
   private Map<String, Object> all() {
     Map<String, Object> all = new LinkedHashMap<>();
@@ -174,9 +177,7 @@ public class BeanMapTest {
   @Test
   public void testClear_given_someBeansInAdditions() throws Exception {
 
-    BeanMap<String, Object> map = new BeanMap<>();
-    map.put("1", object1);
-    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+    BeanMap<String, EBasic> map = newModifyListeningMap();
     map.put("2", object2);
     map.put("3", object3);
 
@@ -188,4 +189,252 @@ public class BeanMapTest {
     assertThat(map.getModifyAdditions()).isEmpty();
   }
 
+  @Test(expected = UnsupportedOperationException.class)
+  public void keySet_add_whenModifyListening() {
+    BeanMap<String, EBasic> map = newModifyListeningMap();
+    map.keySet().add("3");
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void keySet_add() {
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.keySet().add("3");
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void keySet_addAll_whenModifyListening() {
+    BeanMap<String, EBasic> map = newModifyListeningMap();
+    map.keySet().addAll(asList("3", "4"));
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void keySet_addAll() {
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.keySet().addAll(asList("3", "4"));
+  }
+
+  @Test
+  public void keySet_remove() {
+
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+
+    final Set<String> keySet = map.keySet();
+    keySet.remove("1");
+
+    assertThat(keySet.contains("1")).isFalse();
+    assertThat(map).doesNotContainKeys("1");
+    assertThat(map.get("1")).isNull();
+
+    assertThat(map.getModifyRemovals()).containsExactly(object1);
+  }
+
+  @Test
+  public void keySet_clear() {
+
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+
+    final Set<String> keySet = map.keySet();
+    keySet.clear();
+
+    assertThat(map).isEmpty();
+    assertThat(keySet).isEmpty();
+
+    assertThat(map.getModifyRemovals()).containsExactly(object1, object2);
+  }
+
+  @Test
+  public void keySet_iterator_remove() {
+
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.put("3", object3);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+
+    final Set<String> keySet = map.keySet();
+    final Iterator<String> iterator = keySet.iterator();
+    while (iterator.hasNext()) {
+      final String key = iterator.next();
+      if (key.equals("2")) {
+        iterator.remove();
+      }
+    }
+
+    assertThat(map).hasSize(2);
+    assertThat(keySet).hasSize(2);
+    assertThat(keySet).containsExactly("1", "3");
+    assertThat(map).containsKeys("1", "3");
+
+    assertThat(map.getModifyRemovals()).containsExactly(object2);
+  }
+
+  @Test
+  public void keySet_removeAll() {
+
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.put("3", object3);
+    map.put("4", object4);
+    map.put("5", object5);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+
+    final Set<String> keySet = map.keySet();
+    final boolean changed = keySet.removeAll(asList("2", "3", "5"));
+
+    assertThat(changed).isTrue();
+    assertThat(map).hasSize(2);
+    assertThat(keySet).hasSize(2);
+    assertThat(keySet).containsExactly("1", "4");
+    assertThat(map).containsKeys("1", "4");
+
+    assertThat(map.getModifyRemovals()).containsExactly(object2, object3, object5);
+  }
+
+
+  @Test
+  public void keySet_retainAll() {
+
+    BeanMap<String, Object> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.put("3", object3);
+    map.put("4", object4);
+    map.put("5", object5);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+
+    final Set<String> keySet = map.keySet();
+    final boolean changed = keySet.retainAll(asList("2", "3", "5"));
+
+    assertThat(changed).isTrue();
+    assertThat(map).hasSize(3);
+    assertThat(keySet).hasSize(3);
+    assertThat(keySet).containsExactly("2", "3", "5");
+    assertThat(map).containsKeys("2", "3", "5");
+
+    assertThat(map.getModifyRemovals()).containsExactly(object1, object4);
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void values_add() {
+    BeanMap<String, EBasic> map = new BeanMap<>();
+    map.values().add(object3);
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void values_addAll() {
+    BeanMap<String, EBasic> map = new BeanMap<>();
+    map.values().addAll(asList(object3, object5));
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void entrySet_add() {
+    newModifyListeningMap()
+      .entrySet()
+      .add(new AbstractMap.SimpleEntry<>("3", object3));
+  }
+
+  @Test
+  public void entrySet_clear() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+    entries.clear();
+
+    assertThat(entries).isEmpty();
+    assertThat(map).isEmpty();
+    assertThat(map.getModifyRemovals()).containsExactly(object1);
+  }
+
+  @Test
+  public void entrySet_remove() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap5();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+
+    assertThat(map).hasSize(5);
+
+    final boolean existed1 = entries.remove(new AbstractMap.SimpleEntry<>("1", object1));
+    assertThat(existed1).isTrue();
+
+    final boolean existed22 = entries.remove(new AbstractMap.SimpleEntry<>("22", object1));
+    assertThat(existed22).isFalse();
+
+    assertThat(map).hasSize(4);
+    assertThat(map.getModifyRemovals()).containsExactly(object1);
+  }
+
+  @Test
+  public void entrySet_remove_whenNotEqualValue() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap5();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+
+    assertThat(map).hasSize(5);
+
+    final boolean modified = entries.remove(new AbstractMap.SimpleEntry<>("1", object2));
+    assertThat(modified).isFalse();
+
+    assertThat(map).hasSize(5);
+    assertThat(map.getModifyRemovals()).isNull();
+  }
+
+  @Test
+  public void entrySet_iterator_remove() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap5();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+    final Iterator<Map.Entry<String, EBasic>> iterator = entries.iterator();
+    while (iterator.hasNext()) {
+      final Map.Entry<String, EBasic> entry = iterator.next();
+      if (entry.getKey().equals("2") || entry.getKey().equals("5")) {
+        iterator.remove();
+      }
+    }
+    assertThat(map).hasSize(3);
+    assertThat(entries).hasSize(3);
+    assertThat(map.getModifyRemovals()).containsExactly(object2, object5);
+  }
+
+  @Test
+  public void entrySet_removeAll() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap5();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+
+    entries.removeAll(asList(new AbstractMap.SimpleEntry<>("1", object1), new AbstractMap.SimpleEntry<>("3", object4), new AbstractMap.SimpleEntry<>("4", object4)));
+    assertThat(map).hasSize(3);
+    assertThat(entries).hasSize(3);
+    assertThat(map.getModifyRemovals()).containsExactly(object1, object4);
+  }
+
+  @Test
+  public void entrySet_retainAll() {
+    final BeanMap<String, EBasic> map = newModifyListeningMap5();
+    final Set<Map.Entry<String, EBasic>> entries = map.entrySet();
+
+    entries.retainAll(asList(new AbstractMap.SimpleEntry<>("1", object1), new AbstractMap.SimpleEntry<>("3", object4), new AbstractMap.SimpleEntry<>("4", object4)));
+    assertThat(map).hasSize(2);
+    assertThat(entries).hasSize(2);
+    assertThat(map.getModifyRemovals()).containsExactly(object2, object3, object5);
+  }
+
+  private BeanMap<String, EBasic> newModifyListeningMap() {
+    BeanMap<String, EBasic> map = new BeanMap<>();
+    map.put("1", object1);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+    return map;
+  }
+
+  private BeanMap<String, EBasic> newModifyListeningMap5() {
+    BeanMap<String, EBasic> map = new BeanMap<>();
+    map.put("1", object1);
+    map.put("2", object2);
+    map.put("3", object3);
+    map.put("4", object4);
+    map.put("5", object5);
+    map.setModifyListening(BeanCollection.ModifyListenMode.ALL);
+    return map;
+  }
 }

ebean-core/src/test/java/org/tests/model/map/BeanMapOrphanRemovalTest.java

@@ -0,0 +1,50 @@
+package org.tests.model.map;
+
+import io.ebean.DB;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Set;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+
+public class BeanMapOrphanRemovalTest {
+
+  @Test
+  public void keySet_retainAll() {
+
+    MpUser user = new MpUser();
+    user.setName("u1");
+    addRoles(user, "r1", "r2", "r3", "r4");
+    DB.save(user);
+
+    final MpUser user1 = DB.find(MpUser.class, user.getId());
+    final Map<String, MpRole> roles = user1.getRoles();
+    assertThat(roles).hasSize(4);
+
+
+    final Set<String> keySet = roles.keySet();
+    keySet.retainAll(Arrays.asList("r2", "r3"));
+
+    DB.save(user1);
+
+    final MpUser user2 = DB.find(MpUser.class, user.getId());
+    final Map<String, MpRole> roles2 = user2.getRoles();
+    assertThat(roles2).hasSize(2);
+  }
+
+  private void addRoles(MpUser user, String... roles){
+    for (String code : roles) {
+      MpRole role = newRole(code);
+      user.getRoles().put(role.getCode(), role);
+    }
+  }
+
+  private MpRole newRole(String code) {
+    MpRole role = new MpRole();
+    role.setCode(code);
+    return role;
+  }
+}

ebean-core/src/test/java/org/tests/model/map/MpUser.java

@@ -1,11 +1,7 @@
 package org.tests.model.map;
 
-import javax.persistence.CascadeType;
-import javax.persistence.Entity;
-import javax.persistence.Id;
-import javax.persistence.MapKey;
-import javax.persistence.OneToMany;
-import java.util.HashMap;
+import javax.persistence.*;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 @Entity
@@ -16,9 +12,9 @@ public class MpUser {
 
   private String name;
 
-  @OneToMany(cascade = CascadeType.ALL)
+  @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
   @MapKey(name = "code")
-  public Map<String, MpRole> roles = new HashMap<>();
+  private Map<String, MpRole> roles = new LinkedHashMap<>();
 
   public Long getId() {
     return id;