Do not throw BeanHasBeenDeleted on PrimaryKeyJoinColumn (#70)

Co-authored-by: Roland Praml <roland.praml@foconis.de>

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanProperty.java

@@ -880,6 +880,13 @@ public class BeanProperty implements ElPropertyValue, Property, STreeProperty {
     return importedPrimaryKey;
   }
 
+  /**
+   * If true, this property references O2O with its primary key.
+   */
+  public boolean isPrimaryKeyExport() {
+    return false;
+  }
+
   @Override
   public boolean isAssocMany() {
     // Returns false - override in BeanPropertyAssocMany.

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanPropertyAssocOne.java

@@ -364,6 +364,11 @@ public class BeanPropertyAssocOne<T> extends BeanPropertyAssoc<T> implements STr
     return orphanRemoval;
   }
 
+  @Override
+  public boolean isPrimaryKeyExport() {
+    return primaryKeyExport;
+  }
+
   @Override
   public void diff(String prefix, Map<String, ValuePair> map, EntityBean newBean, EntityBean oldBean) {
     Object newEmb = (newBean == null) ? null : getValue(newBean);
@@ -601,20 +606,27 @@ public class BeanPropertyAssocOne<T> extends BeanPropertyAssoc<T> implements STr
     return findMatch(embeddedProp, prop, prop.dbColumn(), tableJoin);
   }
 
+  /**
+   * If column is a primaryKeyExport colum, we can directly use our own ID and do not need to add a join if the relation is not optional
+   */
+  boolean requiresJoin() {
+    return !primaryKeyExport || isNullable();
+  }
+
   @Override
   public void appendSelect(DbSqlContext ctx, boolean subQuery) {
     if (!isTransient) {
-      if (primaryKeyExport) {
-        descriptor.idProperty().appendSelect(ctx, subQuery);
-      } else {
+      if (requiresJoin()) {
         localHelp.appendSelect(ctx, subQuery);
+      } else {
+        descriptor.idProperty().appendSelect(ctx, subQuery);
       }
     }
   }
 
   @Override
   public void appendFrom(DbSqlContext ctx, SqlJoinType joinType, String manyWhere) {
-    if (!isTransient && !primaryKeyExport) {
+    if (!isTransient && requiresJoin()) {
       localHelp.appendFrom(ctx, joinType);
       if (sqlFormulaJoin != null) {
         ctx.appendFormulaJoin(sqlFormulaJoin, joinType, manyWhere);

ebean-core/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java

@@ -14,7 +14,6 @@ import io.ebeaninternal.server.deploy.*;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import javax.persistence.PersistenceException;
 import java.sql.SQLException;
 import java.util.*;
 
@@ -938,7 +937,10 @@ public final class DefaultPersister implements Persister {
       for (BeanPropertyAssocOne<?> prop : expOnes) {
         // for soft delete check cascade type also supports soft delete
         if (deleteMode.isHard() || prop.isTargetSoftDelete()) {
-          if (request.isLoadedProperty(prop)) {
+          if (prop.isPrimaryKeyExport()) {
+            // we can delete by id, neither if property loaded or not
+            delete(prop.targetDescriptor(), prop.descriptor().id(parentBean), null, t, deleteMode);
+          } else if (request.isLoadedProperty(prop)) {
             Object detailBean = prop.getValue(parentBean);
             if (detailBean != null) {
               deleteRecurse((EntityBean) detailBean, t, deleteMode);

ebean-test/src/test/java/org/tests/model/onetoone/OtoBMaster.java

@@ -10,7 +10,7 @@ public class OtoBMaster {
 
   String name;
 
-  @OneToOne(cascade = CascadeType.ALL, mappedBy = "master", fetch = FetchType.LAZY)
+  @OneToOne(cascade = CascadeType.ALL, mappedBy = "master", fetch = FetchType.LAZY, optional = false)
   OtoBChild child;
 
   public Long getId() {

ebean-test/src/test/java/org/tests/model/onetoone/OtoUBPrime.java

@@ -1,9 +1,6 @@
 package org.tests.model.onetoone;
 
-import javax.persistence.Entity;
-import javax.persistence.Id;
-import javax.persistence.OneToOne;
-import javax.persistence.Version;
+import javax.persistence.*;
 import java.util.UUID;
 
 @Entity
@@ -17,7 +14,7 @@ public class OtoUBPrime {
   /**
    * Master side of bi-directional PrimaryJoinColumn.
    */
-  @OneToOne(mappedBy = "prime")
+  @OneToOne(mappedBy = "prime", optional = false)
   OtoUBPrimeExtra extra;
 
   @Version

ebean-test/src/test/java/org/tests/model/onetoone/OtoUBPrimeExtra.java

@@ -14,7 +14,7 @@ public class OtoUBPrimeExtra {
   /**
    * Child side of bi-directional PrimaryJoinColumn.
    */
-  @OneToOne
+  @OneToOne(optional = false)
   @PrimaryKeyJoinColumn
   OtoUBPrime prime;
 

ebean-test/src/test/java/org/tests/model/onetoone/OtoUPrime.java

@@ -13,15 +13,27 @@ public class OtoUPrime {
 
   String name;
 
+
   /**
    * Effectively Ebean automatically sets Cascade PERSIST and mapped by for PrimaryKeyJoinColumn.
-   * This OneToOne is optional so left join to extra.
+   * This OneToOne is not optional so use inner join to extra (unless DbForeignkey(noConstraint = true) is set)
+   * Note: Violating the contract (Storing OtoUPrime without extra) may cause problems:
+   * - due the inner join, you might not get results from the query
+   * - you might get a "Beah has been deleted" if lazy load occurs on 'extra'
    */
-  @OneToOne
+  @OneToOne(orphanRemoval = true, optional = false)
   @PrimaryKeyJoinColumn
+  // enforcing left join - without 'noConstraint = true', an inner join is used
   @DbForeignKey(noConstraint = true)
   OtoUPrimeExtra extra;
 
+  /**
+   * This OneToOne is optional so left join to extra.
+   * Setting FetchType.LAZY will NOT add the left join by default to the query.
+   */
+  @OneToOne(mappedBy = "prime", fetch = FetchType.LAZY, orphanRemoval = true, optional = true)
+  OtoUPrimeOptionalExtra optionalExtra;
+
   @Version
   Long version;
 
@@ -65,4 +77,12 @@ public class OtoUPrime {
   public void setVersion(Long version) {
     this.version = version;
   }
+
+  public OtoUPrimeOptionalExtra getOptionalExtra() {
+    return optionalExtra;
+  }
+
+  public void setOptionalExtra(OtoUPrimeOptionalExtra optionalExtra) {
+    this.optionalExtra = optionalExtra;
+  }
 }

ebean-test/src/test/java/org/tests/model/onetoone/OtoUPrimeExtra.java

@@ -1,8 +1,8 @@
 package org.tests.model.onetoone;
 
-import javax.persistence.Entity;
-import javax.persistence.Id;
-import javax.persistence.Version;
+import io.ebean.annotation.Formula;
+
+import javax.persistence.*;
 import java.util.UUID;
 
 @Entity
@@ -22,7 +22,7 @@ public class OtoUPrimeExtra {
 
   @Override
   public String toString() {
-    return "exId:"+ eid +" "+extra;
+    return "exId:" + eid + " " + extra;
   }
 
   public UUID getEid() {
@@ -48,4 +48,5 @@ public class OtoUPrimeExtra {
   public void setVersion(Long version) {
     this.version = version;
   }
+
 }

ebean-test/src/test/java/org/tests/model/onetoone/OtoUPrimeExtraWithConstraint.java

@@ -0,0 +1,52 @@
+package org.tests.model.onetoone;
+
+import javax.persistence.Entity;
+import javax.persistence.Id;
+import javax.persistence.Version;
+import java.util.UUID;
+
+@Entity
+public class OtoUPrimeExtraWithConstraint {
+
+  @Id
+  UUID eid;
+
+  String extra;
+
+  @Version
+  Long version;
+
+  public OtoUPrimeExtraWithConstraint(String extra) {
+    this.extra = extra;
+  }
+
+  @Override
+  public String toString() {
+    return "exId:" + eid + " " + extra;
+  }
+
+  public UUID getEid() {
+    return eid;
+  }
+
+  public void setEid(UUID eid) {
+    this.eid = eid;
+  }
+
+  public String getExtra() {
+    return extra;
+  }
+
+  public void setExtra(String extra) {
+    this.extra = extra;
+  }
+
+  public Long getVersion() {
+    return version;
+  }
+
+  public void setVersion(Long version) {
+    this.version = version;
+  }
+
+}

ebean-test/src/test/java/org/tests/model/onetoone/OtoUPrimeOptionalExtra.java

@@ -0,0 +1,61 @@
+package org.tests.model.onetoone;
+
+import javax.persistence.*;
+import java.util.UUID;
+
+@Entity
+public class OtoUPrimeOptionalExtra {
+
+  @Id
+  UUID eid;
+
+  String extra;
+
+  @OneToOne(optional = false)
+  @PrimaryKeyJoinColumn
+  private OtoUPrime prime;
+
+  @Version
+  Long version;
+
+  public OtoUPrimeOptionalExtra(String extra) {
+    this.extra = extra;
+  }
+
+  @Override
+  public String toString() {
+    return "exId:" + eid + " " + extra;
+  }
+
+  public UUID getEid() {
+    return eid;
+  }
+
+  public void setEid(UUID eid) {
+    this.eid = eid;
+  }
+
+  public String getExtra() {
+    return extra;
+  }
+
+  public void setExtra(String extra) {
+    this.extra = extra;
+  }
+
+  public Long getVersion() {
+    return version;
+  }
+
+  public void setVersion(Long version) {
+    this.version = version;
+  }
+
+  public OtoUPrime getPrime() {
+    return prime;
+  }
+
+  public void setPrime(OtoUPrime prime) {
+    this.prime = prime;
+  }
+}

ebean-test/src/test/java/org/tests/model/onetoone/OtoUPrimeWithConstraint.java

@@ -0,0 +1,63 @@
+package org.tests.model.onetoone;
+
+import javax.persistence.*;
+import java.util.UUID;
+
+@Entity
+public class OtoUPrimeWithConstraint {
+
+  @Id
+  UUID pid;
+
+  String name;
+
+  @OneToOne(orphanRemoval = true, optional = false)
+  // @DbForeignKey(noConstraint = true) see OtoUPrime
+  @PrimaryKeyJoinColumn
+  OtoUPrimeExtraWithConstraint extra;
+
+  @Version
+  Long version;
+
+  public OtoUPrimeWithConstraint(String name) {
+    this.name = name;
+  }
+
+  @Override
+  public String toString() {
+    return "id:" + pid + " name:" + name + " extra:" + extra;
+  }
+
+  public UUID getPid() {
+    return pid;
+  }
+
+  public void setPid(UUID pid) {
+    this.pid = pid;
+  }
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(String name) {
+    this.name = name;
+  }
+
+  public OtoUPrimeExtraWithConstraint getExtra() {
+    return extra;
+  }
+
+  public void setExtra(OtoUPrimeExtraWithConstraint extra) {
+    this.extra = extra;
+  }
+
+  public Long getVersion() {
+    return version;
+  }
+
+  public void setVersion(Long version) {
+    this.version = version;
+  }
+
+}

ebean-test/src/test/java/org/tests/model/onetoone/TestOneToOneImportedPkNative.java

@@ -34,7 +34,7 @@ public class TestOneToOneImportedPkNative extends BaseTestCase {
 
     String sql = sqlOf(query);
     assertThat(sql).contains("select t0.id, t0.name from oto_bmaster t0 where t0.id ");
-    assertThat(sql).doesNotContain("left join oto_bchild");
+    assertThat(sql).doesNotContain("join oto_bchild");
 
     assertThat(one).isNotNull();
 

ebean-test/src/test/java/org/tests/model/onetoone/TestOneToOnePrimaryKeyJoinBidi.java

@@ -45,7 +45,7 @@ public class TestOneToOnePrimaryKeyJoinBidi extends BaseTestCase {
     OtoUBPrime oneWith = queryWithFetch.findOne();
 
     assertThat(oneWith).isNotNull();
-    assertThat(sqlOf(queryWithFetch, 10)).contains("select t0.pid, t0.name, t0.version, t1.eid, t1.extra, t1.version, t1.eid from oto_ubprime t0 left join oto_ubprime_extra t1 on t1.eid = t0.pid where t0.pid = ?")
+    assertThat(sqlOf(queryWithFetch, 10)).contains("select t0.pid, t0.name, t0.version, t1.eid, t1.extra, t1.version, t1.eid from oto_ubprime t0 join oto_ubprime_extra t1 on t1.eid = t0.pid where t0.pid = ?")
       .as("we join to oto_prime_extra");
 
     assertThat(oneWith.getExtra().getExtra()).isEqualTo("v" + desc);

ebean-test/src/test/java/org/tests/model/onetoone/TestOneToOnePrimaryKeyJoinOptional.java

@@ -1,14 +1,21 @@
 package org.tests.model.onetoone;
 
-import io.ebean.xtest.BaseTestCase;
 import io.ebean.DB;
 import io.ebean.Query;
+import io.ebean.plugin.Property;
 import io.ebean.test.LoggedSql;
+import io.ebean.xtest.BaseTestCase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import javax.persistence.EntityNotFoundException;
+import javax.persistence.PersistenceException;
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.*;
 
 public class TestOneToOnePrimaryKeyJoinOptional extends BaseTestCase {
 
@@ -21,23 +28,120 @@ public class TestOneToOnePrimaryKeyJoinOptional extends BaseTestCase {
     return prime;
   }
 
-  @Test
-  public void insertWithoutExtra() {
+  @BeforeEach
+  void prepare() {
+    OtoUPrime p1Single = new OtoUPrime("Prime without optional");
+    p1Single.setExtra(new OtoUPrimeExtra("Non optional prime required"));
+    DB.save(p1Single);
+
+    OtoUPrimeExtra p2 = new OtoUPrimeExtra("SinglePrimeExtra");
+    try {
+      DB.save(p2);
+      fail("PrimExtra cannot exist without Prime");
+    } catch (PersistenceException pe) {
+
+    }
 
     String desc = "" + System.currentTimeMillis();
     OtoUPrime p1 = new OtoUPrime("u" + desc);
+    p1.setExtra(new OtoUPrimeExtra("u" + desc));
+    p1.setOptionalExtra(new OtoUPrimeOptionalExtra("This one has also an optional"));
     DB.save(p1);
+  }
 
-    Query<OtoUPrime> query = DB.find(OtoUPrime.class)
-      .setId(p1.getPid())
-      .fetch("extra", "eid");
+  @AfterEach
+  void cleanup() {
+    DB.find(OtoUPrime.class).delete();
+    assertThat(DB.find(OtoUPrimeExtra.class).findList()).isEmpty();
+    assertThat(DB.find(OtoUPrimeOptionalExtra.class).findList()).isEmpty();
+  }
 
-    OtoUPrime found = query.findOne();
+  public void doTest1(boolean extraFetch, boolean optionalFetch) {
 
-    if (found.getExtra() != null) {
-      found.getExtra().getExtra(); // fails here, because getExtra should be null
+    // Query for "fetch" case - extra bean joined by left join
+
+    Query<OtoUPrime> query1 = DB.find(OtoUPrime.class);
+    if (extraFetch) {
+      query1.fetch("extra");
     }
-    assertThat(found.getExtra()).isNull();
+    if (optionalFetch) {
+      query1.fetch("optionalExtra");
+    }
+    List<OtoUPrime> primes = query1.findList();
+    if (extraFetch && optionalFetch) {
+      assertThat(query1.getGeneratedSql()).isEqualTo("select t0.pid, t0.name, t0.version, " +
+        "t1.eid, t1.extra, t1.version, " +
+        "t2.eid, t2.extra, t2.version, t2.eid " +
+        "from oto_uprime t0 " +
+        "left join oto_uprime_extra t1 on t1.eid = t0.pid " +  // left join on non-optional, because DbForeignKey(noConstraint=true) is set
+        "left join oto_uprime_optional_extra t2 on t2.eid = t0.pid"); // left join on optional
+    } else if (extraFetch) {
+      assertThat(query1.getGeneratedSql()).isEqualTo("select t0.pid, t0.name, t0.version, t1.eid, t1.extra, t1.version from oto_uprime t0 left join oto_uprime_extra t1 on t1.eid = t0.pid");
+    } else if (optionalFetch) {
+      assertThat(query1.getGeneratedSql()).isEqualTo("select t0.pid, t0.name, t0.pid, t0.version, t1.eid, t1.extra, t1.version, t1.eid from oto_uprime t0 left join oto_uprime_optional_extra t1 on t1.eid = t0.pid");
+
+    } else {
+      assertThat(query1.getGeneratedSql()).isEqualTo("select t0.pid, t0.name, t0.pid, t0.version from oto_uprime t0");
+    }
+
+    List<Long> versions = new ArrayList<>();
+    for (OtoUPrime prime : primes) {
+      if (prime.getOptionalExtra() != null) {
+        versions.add(prime.getOptionalExtra().getVersion());
+      }
+    }
+    assertThat(primes).hasSize(2);
+    assertThat(versions).containsExactly(1L);
+  }
+
+  public void doTest2(boolean withFetch) {
+
+    Query<OtoUPrimeOptionalExtra> query2 = DB.find(OtoUPrimeOptionalExtra.class);
+    if (withFetch) {
+      query2.fetch("prime");
+    }
+    List<OtoUPrimeOptionalExtra> extraPrimes = query2.findList();
+    if (withFetch) {
+      assertThat(query2.getGeneratedSql()).isEqualTo("select t0.eid, t0.extra, t0.version, t1.pid, t1.name, t1.pid, t1.version from oto_uprime_optional_extra t0 join oto_uprime t1 on t1.pid = t0.eid");
+    } else {
+      assertThat(query2.getGeneratedSql()).isEqualTo("select t0.eid, t0.extra, t0.version, t0.eid from oto_uprime_optional_extra t0");
+    }
+    List<Long> versions = new ArrayList<>();
+    for (OtoUPrimeOptionalExtra extraPrime : extraPrimes) {
+      versions.add(extraPrime.getPrime().getVersion());
+    }
+    assertThat(extraPrimes).hasSize(1);
+    assertThat(versions).containsExactly(1L);
+  }
+
+  @Test
+  void testWithExtraFetch1() {
+    doTest1(true, false);
+  }
+
+  @Test
+  void testWithOptionalFetch1() {
+    doTest1(false, true);
+  }
+
+  @Test
+  void testWithBothFetch1() {
+    doTest1(true, true);
+  }
+
+  @Test
+  void testWithoutFetch1() {
+    doTest1(false, false);
+  }
+
+  @Test
+  void testWithFetch2() {
+    doTest2(true);
+  }
+
+  @Test
+  void testWithoutFetch2() {
+    doTest2(false);
   }
 
   @Test
@@ -54,7 +158,7 @@ public class TestOneToOnePrimaryKeyJoinOptional extends BaseTestCase {
     OtoUPrime found = query.findOne();
 
     assertThat(found).isNotNull();
-    assertThat(sqlOf(query, 4)).contains("select t0.pid, t0.name, t0.version, t0.pid from oto_uprime t0 where t0.pid = ?")
+    assertThat(sqlOf(query, 4)).contains("select t0.pid, t0.name, t0.pid, t0.version from oto_uprime t0 where t0.pid = ?")
       .as("we don't join to oto_uprime_extra");
 
     assertThat(found.getName()).isEqualTo("u" + desc);
@@ -66,7 +170,8 @@ public class TestOneToOnePrimaryKeyJoinOptional extends BaseTestCase {
     OtoUPrime oneWith = queryWithFetch.findOne();
 
     assertThat(oneWith).isNotNull();
-    assertThat(sqlOf(queryWithFetch, 6)).contains("select t0.pid, t0.name, t0.version, t1.eid, t1.extra, t1.version from oto_uprime t0 left join oto_uprime_extra t1 on t1.eid = t0.pid where t0.pid = ?")
+    assertThat(sqlOf(queryWithFetch, 6))
+      .contains("select t0.pid, t0.name, t0.version, t1.eid, t1.extra, t1.version from oto_uprime t0 left join oto_uprime_extra t1 on t1.eid = t0.pid where t0.pid = ?")
       .as("we join to oto_prime_extra");
 
 
@@ -98,8 +203,77 @@ public class TestOneToOnePrimaryKeyJoinOptional extends BaseTestCase {
     DB.delete(bean);
 
     List<String> sql = LoggedSql.stop();
-    assertThat(sql).hasSize(2);
+
+    assertThat(sql).hasSize(3);
     assertSql(sql.get(0)).contains("delete from oto_uprime_extra where");
-    assertSql(sql.get(1)).contains("delete from oto_uprime where");
+    assertSql(sql.get(1)).contains("delete from oto_uprime_optional_extra where");
+    assertSql(sql.get(2)).contains("delete from oto_uprime where");
+  }
+
+  @Test
+  void testDdl() {
+    Collection<? extends Property> props = DB.getDefault().pluginApi().beanType(OtoUPrime.class).allProperties();
+
+    for (Property prop : props) {
+      System.out.println(prop);
+    }
+  }
+
+  @Test
+  void testContractViolation1() {
+
+    OtoUPrime p1 = new OtoUPrime("Prime having no extra");
+    // extra is "optional=false" - and this is a violating of the contract
+    DB.save(p1);
+
+    Query<OtoUPrime> query = DB.find(OtoUPrime.class).setId(p1.pid);
+
+    OtoUPrime found1 = query.findOne();
+    assertThat(query.getGeneratedSql()).doesNotContain("join");
+
+    assertThat(found1.getExtra()).isNotNull();
+    assertThatThrownBy(() -> found1.getExtra().getVersion()).isInstanceOf(EntityNotFoundException.class);
+
+    query.fetch("extra");
+    OtoUPrime found2 = query.findOne();
+    // Note: We use "left join" here, because 'DbForeignKey(noConstraint=true)' is set oh the property
+    // if this annotation is not preset, an inner join would be used and 'found2' would be 'null' then
+    assertThat(query.getGeneratedSql()).contains("from oto_uprime t0 left join oto_uprime_extra");
+    assertThat(found2.getExtra()).isNull();
+
+  }
+
+  @Test
+  void testContractViolation2() {
+
+    OtoUPrimeExtraWithConstraint p1Const = new OtoUPrimeExtraWithConstraint("test");
+    try {
+      // a foreign key prevents from saving
+      DB.save(p1Const);
+      fail("PrimExtra cannot exist without Prime");
+    } catch (PersistenceException pe) {
+      // OK
+    }
+
+    OtoUPrimeWithConstraint p1 = new OtoUPrimeWithConstraint("Prime having no extra");
+    // extra is "optional=false" - and this is a violating of the contract
+    // Note there is no real foreign key in the database, that would prevent saving this entity
+    DB.save(p1);
+
+    Query<OtoUPrimeWithConstraint> query = DB.find(OtoUPrimeWithConstraint.class).setId(p1.pid);
+
+    OtoUPrimeWithConstraint found1 = query.findOne();
+    assertThat(query.getGeneratedSql()).doesNotContain("join");
+
+    assertThat(found1.getExtra()).isNotNull();
+    assertThatThrownBy(() -> found1.getExtra().getVersion()).isInstanceOf(EntityNotFoundException.class);
+
+    query.fetch("extra");
+    OtoUPrimeWithConstraint found2 = query.findOne();
+    // Note: We use "left join" here, because 'DbForeignKey(noConstraint=true)' is set oh the property
+    // if this annotation is not preset, an inner join would be used and 'found2' would be 'null' then
+    assertThat(query.getGeneratedSql()).contains("from oto_uprime_with_constraint t0 join oto_uprime_extra_with_constraint");
+    assertThat(found2).isNull();
+
   }
 }