Panache : deleteById()

[loicmathieu]

Add the deleteById() method for both Hibernate and MongoDB with Panache.

Fixes #6270

[geoand]

@loicmathieu do you mind if I push a commit to your PR to update the Spring Data JPA implementation to use this new method?

[loicmathieu]

@geoand of course not ! go ahead !

[geoand]

Thanks, done :)

[gastaldi]

LGTM

[loicmathieu]

@gastaldi I apply your suggestions then squashed the commits.
Should be OK to merge now :)

[geoand]

@FroMage OK with the implementation?

[FroMage]

I think we should start with the improved version ;)

[loicmathieu]

It's not as easy as it looks, for PanacheEntity and for the repository pattern we don't know the name of the ID field so we need to retrieve it somehow ...

[loicmathieu]

I find a way to retrieve from Hiberate the name of the identifier field:

        EntityManager entityManager = getEntityManager();
        Session session = entityManager.unwrap(Session.class);
        SessionFactory sessionFactory = session.getSessionFactory();
        ClassMetadata metadata = sessionFactory.getClassMetadata(entityClass);
        String idField = metadata.getIdentifierPropertyName();

WDYT ?

[loicmathieu]

@FroMage I sucessfully create a query with the trick I described above. This couple Panache directly with Hibernate and not just JPA but I think it's OK. After all it's Hibernate with Panache not JPA with Panache :)

If the new impl is OK for you I'll squash it and we could merge it

[FroMage]

This looks good. Can you just check what happens with composite IDs? This should work with @CompositeId but not @IdClass, I assume.

But this should already be a limitation of findById() which only takes a single argument, so perhaps it's acceptable.

[Sanne]

@Sanne getReference() performs a select in case the entity is not yet inside the persistence context and throws EntityNotFoundException if it didn't exist.

Hi @loicmathieu , no that's not accurate. getReference() will not hit the database at all.

But like I warned, it will assume that you know for sure that the entity exists: it's not correct to use this method if you're in doubt.
I guess what you observe is that some subsequent operation later on is forcing the reference to be loaded - most likely the delete action itself.

TBH as I suggested previous I don't think you can implement this feature to be all three of:

• generic enough for any model
• correct in all such cases
• skipping the load

It's one of those situations in which you have to pick 2.

[loicmathieu]

it will assume that you know for sure that the entity exists: it's not correct to use this method if you're in doubt.

Yes, in fact this is my assumption also, I write my comment too quickly.

OK, so you advise to stick to find(id) + remove(), as it's @FroMage that ask for a JPQL implementation I would like to see if he is on the same page (I'm pretty sure yes but there is no hurry)

[FroMage]

If the Hibernate experts tell us to use one strategy, I won't object. Just make sure to add a comment to the impl explaining why we use what appears to be a supoptimal strategy, so that no-one wants to "improve" it later.

[Sanne]

yeah. We could explore some optimisations in Hibernate ORM; for example I was chatting with the team about this yesterday as I wondered why we don't do just the update statement at least for the simple cases in which we can proof there is no cascading requirements.

Turns out the team sentiment is that while that would be possible, it's actually highly unlikely for an entity to satisfy such requirements in more realistic models than our simple demos, and possibly not trivial to do.. so that explains why it doesn't do it but there certainly is the option to eventually improve on that.

[Sanne]

BTW another angle to this is that even if we did (hypotethically speaking) implement such an optimisation in ORM, you'd have no way to return true/false about knowing if the specific entity was actually existing before... so probably really not worth it.

[loicmathieu]

@Sanne @FroMage I review the implementation to use an JPQL query where possible and switch back to using find()/remove() in case the entity is annotated by @IdClass. We cannot use getReference() as it throws an exception if the entity cannot be foud.

I also add some note on the JavaDoc to advise to use remove() if the entity is already loaded inside the persistence contexte, as I understand it this is when remove() is more performant that an JPQL query right ?

If it's OK, I'll squash the commit and made a last review for doc/typos/...

[loicmathieu]

@Sanne @FroMage I switch back to find()/remove() for Hibernate.
I add an implementation note to the implementation inside JpaOperations.

I also add the documentation part inside the guides.

[FroMage]

Aside from the typo, LGTM.

[loicmathieu]

@FroMage sorry, I double check the documentation but the typo slipped throuhg.

Fixed.

[FroMage]

Great!

[loicmathieu]

@FroMage @gastaldi can we merge it now ?