From 297c1d7abb2b28bb5457a9f51e14644159a52193 Mon Sep 17 00:00:00 2001
From: Robin Bygrave <robin.bygrave@gmail.com>
Date: Mon, 1 Mar 2021 20:27:50 +1300
Subject: [PATCH] #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.
---
 .../main/java/io/ebean/common/BeanSet.java    |  5 +-
 .../server/query/SqlTreeNodeBean.java         | 48 ++++++++++++-------
 .../server/query/SqlTreeNodeManyRoot.java     | 13 ++---
 3 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/ebean-api/src/main/java/io/ebean/common/BeanSet.java b/ebean-api/src/main/java/io/ebean/common/BeanSet.java
index 91f0ae672..880ece106 100644
--- a/ebean-api/src/main/java/io/ebean/common/BeanSet.java
+++ b/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
diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java
index 011d9f1a7..b9df45760 100644
--- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java
+++ b/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
diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java
index 4947514d1..8c5517a76 100644
--- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java
+++ b/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;
   }