Enable the implicit cascading delete for Spring data jpa

[aureamunoz]

fix #11398

@geoand WDYT?

[geoand]

LGTM, thanks!

[geoand]

Although this isn't a breaking change, it's nonetheless something we need to document in https://github.com/quarkusio/quarkus/wiki/Migration-Guide-1.8

[aureamunoz]

Ok, I will fix also the failing check

[aureamunoz]

to add to the migration guide:
The deleteAll() method has been modified for delete-cascading to work. It needs to delete the entities one by one using an entity manager so it loads the entity then delete it.

[lw-mcno]

Hi @aureamunoz & @geoand, thanks for preparing the fix so fast. Does calling getEntityManager() in a loop have some additional cost that could be made smaller by caching the found instance in a variable?

[geoand]

Compared to the cost of going to the database, it's should be negligible

[famod]

Compared to the cost of going to the database, it's should be negligible

I've seen massive bottlenecks in older Hibernate versions (for years!) caused by bad synchronization...not actual DB access.
Just saying. 😉

But I guess deleteAll() is nothing that gets called all the time in production code, so...

[gavinking]

OMG I'm dying inside, this is such a rubbish way to use a relational database. (But believable, I guess, given that we're dealing with Spring here.)

I would almost say don't try to reproduce the broken semantics of the Spring API but I guess the whole point of this is to do exactly whatever broken stuff they do. Probably (hopefully) people are only using this for tests anyway so perhaps it's not such a big deal.

If you did want to do something nonbroken, you could always try looking at the metamodel and detecting the case where there are no cascade-delete associations, and do the nonbroken thing (the bulk delete query) at least in that case.

[geoand]

@gavinking exactly... When I first implemented this, I definitely didn't expect Spring would be exhibiting this behavior...

But we do try to follow the semantics of Spring as much as possible, so that's why we are changing it.

And thanks for the suggestion! I really like the idea and I think we should do it.

@aureamunoz do you want to tackle that?

[gavinking]

Yes sure, I understand but it still makes me sad.

I mean imagine the mind of the person who originally designed this. "Oh Hibernate doesn't have a deleteAll() method with exactly the semantics I naively want after considering the problem for a total of 5 minutes" and then, instead of asking whether there might be some reason Hibernate doesn't have an operation like that, the very next thought is "OK I'm going to help my users by making a method that truncs a table by literally loading every row of that table eagerly into the session cache, and then going back to the db deleting the rows one at a time".

So Spring.

[geoand]

Very sad indeed...

[aureamunoz]

Hi,

Yes, I will do it.

(I've just seen this messages now!)

[aureamunoz]

Hi! I've just pushed the code with the metamodel checking. I need maybe to add some more test and I have to squash commits but I would like to have your feedback first. WDYT @geoand ?

[geoand]

@gavinking is the real expert here, I don't think I can really add anything more :)

[gavinking]

So I really wish I could suggest something better than this, but apparently it's the only way:

CascadeStyle[] propertyCascadeStyles = 
        entityManager.unwrap(SessionImplementor.class)
                .getEntityPersister( entityClass.getName(), null )
                .getPropertyCascadeStyles();

Hibernate really should expose this in a better way.

[geoand]

Thanks a lot @gavinking!

[aureamunoz]

Hi, here is the new code, let me know wdyt when you have some time @geoand @gavinking :-)

[aureamunoz]

This test fails because we delete here the child entity and the line after the parent one. We don't need anymore to delete manually child entities.
I think we should have the same behaviour in the others delete methods that use directly the HQL DELETE FROM enityName. This and This.

WDYT @geoand @gavinking ? Thanks!!

[gavinking]

@aureamunoz I'm a little lost because I'm not sure where is the Spring in that code.

[geoand]

@aureamunoz that makes sense with this change I think

[aureamunoz]

Hi! The checks passed and I think it's ok for review @geoand @gavinking

[gavinking]

the deleteAll with cascade is Spring specific

Yes, exactly, we don't want to reproduce that behavior for regular Panache usage, only where we're aiming for strict compatibility with Spring.

[aureamunoz]

New push adding a test to verify the many-to-many associations and cascade deleting. When you have some spare time it would be cool to have a review! Thanks @gavinking @geoand

[geoand]

The Spring and Quarkus parts look good.

I'll let @gavinking comment on the Hibernate part

[gavinking]

New push adding a test to verify the many-to-many associations and cascade deleting. When you have some spare time it would be cool to have a review! Thanks @gavinking @geoand

The problem is to detect many-to-many associations that are not set up for cascade delete. Something needs to delete the links from the association table in that case.

[geoand]

I was looking to see what Spring Data does in this case and I can't much information on how exactly it's done in that case.

@gavinking how about we merge this as it seems to handle the typical cascade cases and leave MANY-TO-MANY for if / when someone asks for it?

[famod]

leave MANY-TO-MANY for if / when someone asks for it?

My 2 cents: if there is a known limitation, we should document that clearly.

[geoand]

Yeah, that definitely makes sense

[aureamunoz]

Hi!
I've not worked very much on this last week. J've just seen it. In fact, @gavinking is right, I need to detect the many to many association. I didn't understand why because I have a (wrong) test where the deleting with cascade on a Many to many association was working. But the test works because I have already an @ElementCollection in the entity so is this item who enables the delete with cascade. I'm going to push a fix soon.

[aureamunoz]

Hi again, I have been working on this, specificly on the many to many association.In fact, for a many-to-many association, both parts are handled differently if an entity is removed. If we remove the entity that owns the association, hibernate-orm-panache also removes all its associations, the links from the association table. But it doesn’t do that if we delete the "other side" entity. In that case, we need to remove all associations ourself before removing the entity.
I have added some tests:
here the entity Person, with a list of liked Songs, is removed. The association table (liked_songs) rows for that person are automatically removed.
here the entity Song is removed. We need to remove the association table before remove the song.

I have verified and this is also the behaviour of Spring.

So, is this ok for you @gavinking ?

[aureamunoz]

what a hell! I've just pushed a refactor for tests that are checking the delete in which they prepare the data to delete in order to not depends on data available on database. Let's see if the CI pass

[aureamunoz]

Awesome! unbelieveble! All checks passed, could you please take a look? @gavinking @geoand

[aureamunoz]

Hey @gavinking , could you please take a look at this if you have some spare time?

[geoand]

I'm gonna go ahead and merge this one. If we feel it's not right for some deep technical reason, we can always revert as we have plenty of time until the 1.10 release

Comments addressed