possible bug in @ManyToOne soft-deletes

[Roland Praml]

Hello Rob,

We have the following problem with soft-deletions:

assume we have this entity:

@Entity
public class ESoftDelBook extends BaseSoftDelete {

  String bookTitle;

  @ManyToOne(cascade = CascadeType.PERSIST, fetch = FetchType.EAGER)
  ESoftDelUser lendBy;
  ...
}

and this test case:

    ESoftDelUser user1 = new ESoftDelUser("user1");
    Ebean.save(user1);

    ESoftDelBook  book1 = new ESoftDelBook("book1");
    book1.setLendBy(user1);
    Ebean.save(book1);
// now I delete user1
    Ebean.delete(user1);
// and fetch the book again from the DB:
   book1 = Ebean.find(ESoftDelBook.class).where().eq("bookTitle", "book1").findUnique();

I would expect now, that "book1.getLendBy()" is "null" and not a "dead reference".
getLendBy().getUserName() will throw a "EntityNotFoundException" - bean has been deleted.

I also tried FetchType.LAZY, the only difference I see is that " t0.lend_by_id c4" is missing in the inital request.
With FetchType.LAZY a "EntityNotFoundException" would be acceptable, but not with FetchType.EAGER (my opinion)


cheers
Roland

[Roland Praml]

Pull request which describes the problem: https://github.com/ebean-orm/ebean/pull/819

[Rob Bygrave]

Tricky ...

Now, when we fetch book1 again it doesn't do any join against user and instead only needs/uses the foreign key column (and hence doesn't hence for logically deleted).

That is:

book1 = Ebean.find(ESoftDelBook.class).where().eq("bookTitle", "book1").findUnique();

results in:

select t0.id c0, t0.book_title c1, t0.version c2, t0.deleted c3, t0.lend_by_id c4 

from esoft_del_book t0 where t0.book_title = ? and coalesce(t0.deleted,false)=false; --bind(book1)

... and this only reads the t0.lend_by_id foreign key column ... which still has the value of 1 (the pk of user1). Hence Ebean is creating a "reference bean" for user1 and setting this to the book bean.

-----

One optional would be to additionally do book1.setLendBy(null); book1.save(); first before we delete user1 (just as we would need to do to support deletePermanent(user1)) but I suspect this is not the preferred option in this case.

Assuming setting the FK value to null is not the preferred option the alternative is to accept that lendBy might return a non-null inactive user bean.  The problem currently with this is that the lazy loading query fails with - javax.persistence.EntityNotFoundException: Lazy loading failed on type:com.avaje.tests.model.softdelete.ESoftDelUser id:1 - Bean has been deleted.

I believe the "fix" for this issue is to not throw javax.persistence.EntityNotFoundException in this case (and have the deleted property return true).

That is, the last assert then becomes:

ESoftDelUser lendBy = book1.getLendBy();
assertThat(lendBy.isDeleted()).isTrue();

How does that sound?

Cheers, Rob.

--

---
You received this message because you are subscribed to the Google Groups "Ebean ORM" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ebean+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Rob Bygrave]

I've pushed that "fix" as https://github.com/ebean-orm/ebean/commit/d218fff374304bbf0e5410840ffbc392e98fca57

[Roland Praml]

Am Mittwoch, 31. August 2016 12:51:05 UTC+2 schrieb Rob Bygrave:

That is, the last assert then becomes:

ESoftDelUser lendBy = book1.getLendBy();
assertThat(lendBy.isDeleted()).isTrue();

How does that sound?

It's not exactly what I want, but if I put additional logic in the getter it should be OK for my use case

public getUser() {
   if (user == null || user.isDeleted()) return null; // I see a problem, if query.temporalMode=SOFT_DELETE
   return user;
}

cheers
Roland
 

Cheers, Rob.

To unsubscribe from this group and stop receiving emails from it, send an email to ebean+un...@googlegroups.com.

[Rob Bygrave]

> It's not exactly what I want

User is a "reference bean" at this point.  That is, it only have it's Id value set.  

To try and make it null we would need Ebean to actually hit the DB to check the deleted status ... and the implication is that we would get Ebean to do that anytime it created a reference bean on a entity type with soft delete support - this could get expensive.

Hence, the approach of leaving the bean as non-null ... and setting isDeleted() to true for this use case.

An alternative it that prior to deleting user1 set  the associated lendBy references in books to null -   Update books set lendBy = null where lendBy = user1 

Is there another approach?  Or do you really want Ebean to hit the DB and check the deleted status for reference beans?

Cheers, Rob.

To unsubscribe from this group and stop receiving emails from it, send an email to ebean+unsubscribe@googlegroups.com.

[Roland Praml]

Am Mittwoch, 31. August 2016 23:54:14 UTC+2 schrieb Rob Bygrave:

> It's not exactly what I want

User is a "reference bean" at this point.  That is, it only have it's Id value set.  

To try and make it null we would need Ebean to actually hit the DB to check the deleted status ... and the implication is that we would get Ebean to do that anytime it created a reference bean on a entity type with soft delete support - this could get expensive.

I totally agree in this point. Fetching just the Id and creating a reference "User" bean in the "Book" makes sense
 

Hence, the approach of leaving the bean as non-null ... and setting isDeleted() to true for this use case.

To determine, if it is deleted, a DB hit is required
 

An alternative it that prior to deleting user1 set  the associated lendBy references in books to null -   Update books set lendBy = null where lendBy = user1 

This makes proper undelete very very difficult
 

Is there another approach?  Or do you really want Ebean to hit the DB and check the deleted status for reference beans?

Of course, I want as less as DB hits as possible.

So if the "book" is fetched from the DB, ebean reads Id and creates a reference bean with just the Id set.
This is OK and matches the current behaviour.

Now the internal "lendBy" property contains a User and we do not know if it is deleted or not.

But when "getLendBy" is called on the "book" bean, it must check if the "user"-bean that should be returned is valid. This would require a DB hit of course.
I think if getLendBy is called, we can afford the DB-hit to check if it is deleted and return null in this case, because the DB hit will happen anyway when any property of the reference-bean is accessed.

My additional code in the getter from my last post does exactly this.

Adding code to the getter(s) that checks "lendBy.isDeleted()" and then returns the bean or null would be acceptable for me here. (but it would be nice if the ebean-enhancer does this ;)
One thing to keep in mind is that the check must be skipped if the "book" bean was loaded with a query where includeSoftDeletes = true.

Cheers,
Roland
 

[Rob Bygrave]

> I think if getLendBy is called, we can afford the DB-hit to check if it is deleted and return null in this case

To be clear ... getLendBy does not invoke a query but just returns the reference bean.  There is no "is this deleted" check at that point.

It is not until a property on the bean is read that lazy loading is invoked on the reference bean ... and that sql query returns 0 rows due to being logically deleted and at that point Ebean can interpret that as "the bean you tried to lazy load has been logically deleted" and sets the soft deleted property to true.

That is, the timing of when the logical delete is checked is relatively late (and if it is earlier then we have to hit the DB for each reference bean).

  

To unsubscribe from this group and stop receiving emails from it, send an email to ebean+unsubscribe@googlegroups.com.

[Roland Praml]

Am Donnerstag, 1. September 2016 04:13:07 UTC+2 schrieb Rob Bygrave:

> I think if getLendBy is called, we can afford the DB-hit to check if it is deleted and return null in this case

To be clear ... getLendBy does not invoke a query but just returns the reference bean.  There is no "is this deleted" check at that point.

That's what I tried to say. I see the missing "is this deleted" check here as problem.
The returned reference-bean is useless, because every property (expect Id) is null now (before your fix, an exception was thrown)
 

It is not until a property on the bean is read that lazy loading is invoked on the reference bean ... and that sql query returns 0 rows due to being logically deleted and at that point Ebean can interpret that as "the bean you tried to lazy load has been logically deleted" and sets the soft deleted property to true.

That is, the timing of when the logical delete is checked is relatively late (and if it is earlier then we have to hit the DB for each reference bean).

But if the isDeleted check is performed in the getter, the DB hit is mostly done just <5 lines of code earlier:
(My englisch is not so good, so I try to explain it in code)

Book book = server.find

User user = book.getLendBy(); // you: return an unchecked reference bean / I: perform a DB hit to check isDeleted and return null
if (user != null) {
   System.out.println("The book is lent by " + user.getName()); // you: hit the DB here and return null / I: this line would not be executed
}

Hmm... while writing the above line it came in my mind, that I might get a problem now, that I cannot decide if a book returned or still lent to a deleted user.
So you may be right, that the following application code has to check IsDeleted:

System.out.println("The book is lent by " + user.getName());
}

cheers
Roland

[Rob Bygrave]

User user = book.getLendBy(); // you: return an unchecked reference bean / I: perform a DB hit to check isDeleted and return null

What happens at book.getLendBy() is that a preGetter() check is called to see if the lendBy property is loaded.  As the lend by property is loaded it is returned (returning the reference bean). 

Most importantly this operation does not go to the EbeanServer and it doesn't "know" anything about soft delete etc. 

If the next bit of code did:   user.getId() ... that would just return the Id value. The Id property is loaded, it would be returned, no DB hit would occur etc.

It is not until user.getName() is called that a DB hit actually occurs as the name property is not loaded. Lazy loading is invoked and as this a logically deleted bean that query doesn't find the row etc, at this point Ebean can check if it is soft deleted.

That is, I don't think Ebean can't really make the soft delete check earlier without an extra join or extra query prior to building the reference bean (extra costs Ebean can't know if they are required).  Alternatively, you can choose to do that check by including lendBy in the original query.  That is, a join to user can then include the user.deleted check at the point of creating the reference bean.

In short, I can't see how Ebean can provide better behavior at this stage.

 

Cheers, Rob.

--

---
You received this message because you are subscribed to the Google Groups "Ebean ORM" group.