Quarkus: Panache: deleteAll not deleting child entities when using cascade ALL or REMOVE

[Sgitario]

Describe the bug
When calling the deleteAll function to delete all the entities. It's expected to delete also the child entities if it has been configured to use cascade ALL or REMOVE:

@Entity(name = "application")
public class ApplicationEntity extends PanacheEntity {
    // ...

    @OneToMany(mappedBy = "application", cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.EAGER)
    public List<ServiceEntity> services = new ArrayList<>();

Delete all method usage:

ApplicationEntity.deleteAll();

This works fine if we delete the entity individually:

ApplicationEntity.findById(actualEntityId).delete();

Expected behavior
Delete one item and delete all should behave the same, so both methods should delete the child entities if cascade is properly configured.

Actual behavior
DeleteAll method is not deleting child entities even though the cascade is properly configured.

To Reproduce
You can run this test as a reproducer.
The test is failing and it should pass.

Environment (please complete the following information):

• GraalVM version (if different from Java):
• Quarkus version or git rev: 1.10.3.Final
• Build tool (ie. output of mvnw --version or gradlew --version):

[quarkus-bot[bot]]

/cc @FroMage, @loicmathieu

[FroMage]

I'm not sure deleteAll makes such guarantees. I remember @gavinking and @Sanne talking about this recently, but I can't recall their opinion?

[gavinking]

I imagine that deleteAll() is using a bulk delete query, and it's quite normal for cascade delete to be ignored in that case. Indeed, it would be terribly inefficient to delete parents and children using N+1 delete statements. (That's the really bad implementation we already noticed Spring using somewhere, and we definitely shouldn't do the same thing here.)

Now, in principle you could make it work by issuing two bulk delete queries but that's not functionality that JPA or Hibernate have ever provided, and if it's needed it would probably be better to implement it in Hibernate, not in Panache. OTOH, in 20 years I've never noticed that this was something users really ask for.

[Sanne]

Right; personally I'm not a big fan of the "cascade" semantics as defined on the JPA spec, but that's what we're stuck with.

Ideally I think such cascading rules from the model should be applied as DDL table options: far more efficient and less cumbersome to deal with.
But Hibernate ORM isn't really meant as a tool to define all advanced aspects of DDL generation, people should hopefully manage their schema with other means.

[FroMage]

Fair enough. So in that case I advise that you "override" the ApplicationEntity.deleteAll() static method for your special use-case.

[FroMage]

Now, as to whether Hibernate ORM provides an operation for that that doesn't require loading the entities, or if there's a plain SQL alternative that does it, I'm not really sure.

[Sanne]

Fair enough. So in that case I advise that you "override" the ApplicationEntity.deleteAll() static method for your special use-case.

Or edit the schema so to have the DB deal with it.

[Sgitario]

Taking into account the developer experience, users would expect that deleteAll method behaves as the delete method. In fact, as you said above, in Spring Data, It's working like this.
However, I agree with you about this is not something users usually do or ask for. More if this is something really complex to do. Therefore, maybe a simple note in the documentation to state that the cascade rules are not in consideration when calling to the deleteAll would suffice. What do you think?

[gavinking]

You should pretty much never use Spring's deleteAll(), since it is a performance bug.

[gavinking]

I mean, "avoid N+1 queries" is one of the most basic axioms of data access.

[FroMage]

If this table is the only table using ServiceEntity you can trivially do:

public class ApplicationEntity extends PanacheEntity {
// ...
  public static void deleteAll(){
    // this is basically super.deleteAll()
    JpaOperations.INSTANCE.deleteAll(ApplicationEntity.class);
    ServiceEntity.deleteAll();
  }
}

[gavinking]

Right; personally I'm not a big fan of the "cascade" semantics as defined on the JPA spec, but that's what we're stuck with.

Well, that would be my fault, since they're copied directly from Hibernate 2. ;-)

But yeah, Ive never been terribly sure that this stuff is totally the right thing either.

[gavinking]

Ideally I think such cascading rules from the model should be applied as DDL table options: far more efficient and less cumbersome to deal with.

The issue with that approach is keeping the second-level cache in sync.

[gavinking]

maybe a simple note in the documentation to state that the cascade rules are not in consideration

deleteAll() is a framework-defined method with Javadoc, no, @FroMage?

[Sanne]

Ideally I think such cascading rules from the model should be applied as DDL table options: far more efficient and less cumbersome to deal with.

The issue with that approach is keeping the second-level cache in sync.

Good point; I wonder if one could still fire the right eviction events, but omit the SQL statements. It would certainly require ORM to _know_ about the DDL structure (which we don't) and be able to rely on it.

[FroMage]

deleteAll() is a framework-defined method with Javadoc, no, @FroMage?

Yes, we could improve the javadoc.

[gavinking]

@Sanne well for delete everything it could be made to work. But when the delete statement has a where clause I think we're screwed. Which is why we don't have this feature.

[hprange]

Premature optimization is the root of all evil.

When I first reported this issue to the mailing list, I was trying to use the deleteAll method to clear the database after running each test. This is a very common use case for this method. And performance isn't relevant in this scenario.

IMHO, the way it is, the current implementation also violates the principle of least surprise. I expect the deleteAll method to behave indistinguishably from the delete method, but for all records.

So, if cascading is not expected to occur when calling the deleteAll method, can you consider changing the method name to something less ambiguous?

[gavinking]

Counterpoint: when I first learned the semantics of deleteAll() in Spring, I wasn't merely _suprised_, I was shocked that they would have implemented something so crap. So "least surprise" can be a pretty misleading principle. We're surprised by things we're not used to: you're used to Spring, and I'm used to data access code that isn't crap 🤷‍♂️😆

[gavinking]

Anyway, the conclusion is that there's actually a really good reason that Hibernate itself doesn't have cascading bulk deletes or a deleteAll() operation: the reason is that all the obvious ways to implement it are actually really bad, and to do it right would require a pretty weird and tricky implementation involving multiple select and delete queries. Perhaps someday we could do a really correct implementation of that, but it seems to have never come up as a priority in 20 years. Sometimes it's better to simply not have a feature than to have a bad implementation of the feature.

[gavinking]

@Sanne a crazy thought that just occurred to me is that the only actual usecase we have for this thing is cleaning up after tests. And I thought, well, I never have this need because I use schema export for that. But then I remembered that schema export is really slow on DB2. So what I would really like is for schema export to be able to delete instead of drop tables. FK constraints are a potential problem with that idea, however.