#2179 - Improved fix for (2179) Fetch on nested one-to-many collections results in duplicates

The improvement here is that SqlTreeNodeManyRoot uses load and isContextBean() so that it only checks if the collection already contains the detailBean if the detailBean was already in the persistence context (aka not newly loaded and added).  The vast majority case is that detailBean is loaded fresh so for the common case we can skip the extra collection contains() check.

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

@@ -70,9 +70,8 @@ public final class BeanSet<E> extends AbstractBeanCollection<E> implements Set<E
 
   @Override
   public void internalAddWithCheck(Object bean) {
-    if (set == null || bean == null || !set.contains(bean)) {
-      internalAdd(bean);
-    }
+    // set add() already de-dups so just add it
+    internalAdd(bean);
   }
 
   @Override

ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java

@@ -230,7 +230,7 @@ class SqlTreeNodeBean implements SqlTreeNode {
   /**
    * Load a bean instance.
    */
-  private class Load {
+  class Load {
 
     final DbReadContext ctx;
     final EntityBean parentBean;
@@ -324,17 +324,13 @@ class SqlTreeNodeBean implements SqlTreeNode {
     }
 
     private void loadChildren() throws SQLException {
-      //boolean lazyLoadMany = false;
       if (localBean == null && queryMode == Mode.LAZYLOAD_MANY) {
         // batch lazy load many into existing contextBean
         localBean = contextBean;
         lazyLoadMany = true;
       }
-      // recursively continue reading...
-      for (SqlTreeNode aChildren : children) {
-        // read each child... and let them set their
-        // values back to this localBean
-        aChildren.load(ctx, localBean, contextBean);
+      for (SqlTreeNode child : children) {
+        child.load(ctx, localBean, contextBean);
       }
     }
 
@@ -425,7 +421,6 @@ class SqlTreeNodeBean implements SqlTreeNode {
           ctx.setLazyLoadedChildBean(localBean, lazyLoadParentId);
         }
         return localBean;
-
       } else {
         if (lazyLoadParentId != null) {
           ctx.setLazyLoadedChildBean(contextBean, lazyLoadParentId);
@@ -443,6 +438,27 @@ class SqlTreeNodeBean implements SqlTreeNode {
       loadProperties();
       loadChildren();
     }
+
+    /**
+     * Perform the load returning the loaded bean.
+     */
+    EntityBean perform() throws SQLException {
+      initialise();
+      if (isLazyLoadManyRoot()) {
+        return getContextBean();
+      }
+      postLoad();
+      setBeanToParent();
+      return complete();
+    }
+
+    /**
+     * Return true if this bean was already in the context. If already in the
+     * context we need to check if it is already contained in the collection.
+     */
+    boolean isContextBean() {
+      return localBean == null;
+    }
   }
 
   /**
@@ -450,14 +466,14 @@ class SqlTreeNodeBean implements SqlTreeNode {
    */
   @Override
   public EntityBean load(DbReadContext ctx, EntityBean parentBean, EntityBean contextParent) throws SQLException {
-    Load load = (inheritInfo != null) ? new LoadInherit(ctx, parentBean) : new Load(ctx, parentBean);
-    load.initialise();
-    if (load.isLazyLoadManyRoot()) {
-      return load.getContextBean();
-    }
-    load.postLoad();
-    load.setBeanToParent();
-    return load.complete();
+    return createLoad(ctx, parentBean).perform();
+  }
+
+  /**
+   * Create the loader with or without inheritance.
+   */
+  Load createLoad(DbReadContext ctx, EntityBean parentBean) {
+    return (inheritInfo != null) ? new LoadInherit(ctx, parentBean) : new Load(ctx, parentBean);
   }
 
   @Override

ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java

@@ -25,13 +25,14 @@ final class SqlTreeNodeManyRoot extends SqlTreeNodeBean {
 
   @Override
   public EntityBean load(DbReadContext cquery, EntityBean parentBean, EntityBean contextParent) throws SQLException {
-    // pass in null for parentBean because the localBean
-    // that is built is added to a collection rather than
-    // being set to the parentBean directly
-    EntityBean detailBean = super.load(cquery, null, null);
-    // initialise the collection and add detailBean if it is not null
+    // pass in null for parentBean because added to a collection rather than set to the parentBean
+    SqlTreeNodeBean.Load load = createLoad(cquery, null);
+    EntityBean detailBean = load.perform();
     if (contextParent != null) {
-      manyProp.addBeanToCollectionWithCreate(contextParent, detailBean, true);
+      // Add to the collection and initialise collection if needed
+      // A null detailBean may initialise an empty collection
+      // Check for bean existing in collection based on load.isContextBean()
+      manyProp.addBeanToCollectionWithCreate(contextParent, detailBean, load.isContextBean());
     }
     return detailBean;
   }