REST Data with Panache: support of entity event listeners

[Sgitario]

REST Data with Panache supports the subscription to the following entity events:

• Before/After save events
• Before/After update events
• Before/After delete events

To suscribe to an entity event, you need to provide a bean that implements the interface EntityEventListener, for example:

@ApplicationScoped
public class PeopleRestDataResourceMethodListener implements RestDataResourceMethodListener<Person> {
    @Override
    public void onBeforeAdd(Person person) {
        System.out.println("Before Save Person: " + person.name);
    }
}

When using the REST Data Reactive with Panache extension, you will receive the Uni instance of the entity in the after methods:

@ApplicationScoped
public class PeopleRestDataResourceMethodListener implements RestDataResourceMethodListener<Person> {
    @Override
    public void onAfterAdd(Uni<Person> uni) {
        uni.subscribe().with(person -> {
            System.out.println("After Save Person: " + person.name);
        });
    }
}

Fix #29095

[geoand]

Seems reasonable to me.

@yrodiere do you also think this makes sense?

[yrodiere]

@yrodiere do you also think this makes sense?

While it might seem reasonable at first glance, there's already a very similar feature available: JPA entity listeners. See 0852b10#diff-7ed082417d8730c92d0dcf5c59e547878a5c298311afbd529f1a5c9591f150cc for a few tests.

I'm struggling to see what this brings, beyond duplicating an already available, standard feature?

Note it's also more limited:

• It supports fewer event types than JPA entity listeners
• The listener won't be called for, e.g. cascaded persists or updates, while JPA entity listeners would.

If it's about getting to manipulate a Uni, then I'd suggest improving Hibernate Reactive so that it supports Uni in JPA entity listeners?

[Sgitario]

PR updated according to the comments in #29095
cc @geoand

[geoand]

I can't really say I know much about the subject in hand, so I'll let @yrodiere and @gsmet decide.

[yrodiere]

I can't really say I know much about the subject in hand, so I'll let @yrodiere and @gsmet decide.

You know about as much as I do :)

[yrodiere]

So I dug up a bit the Git history, and I must say I was wrong: judging from his past contributions to this extensions, @geoand knows more about it than I do :)

Anyway: @gytis was the original creator of this extension, and @Sgitario seems to have contributed the most since then. So... I'll trust him on the functional side :)

On the technical side, I must say I'm doubtful about the benefits of all the renamings, considering the noise it brings to this PR overall, but I guess there are reasons. Other than that I just spotted a few things; I'll let you see if you want to address them, @Sgitario. Thanks!

[geoand]

I'll try and have a look later on today or tomorrow

[Sgitario]

So I dug up a bit the Git history, and I must say I was wrong: judging from his past contributions to this extensions, @geoand knows more about it than I do :)

Anyway: @gytis was the original creator of this extension, and @Sgitario seems to have contributed the most since then. So... I'll trust him on the functional side :)

On the technical side, I must say I'm doubtful about the benefits of all the renamings, considering the noise it brings to this PR overall, but I guess there are reasons. Other than that I just spotted a few things; I'll let you see if you want to address them, @Sgitario. Thanks!

Yes, I had refactored the entities used in the tests to reuse the same ones, but it's true that it adds lots of noise in the pull request, so I've reverted these changes to the model and will do it in a separate ticket. Hopefully, now the pull request will be easier to review.

PR updated.

[geoand]

Great!

[geoand]

Actually I have one more question:

Currently the implementation subscribes to the Uni for HR - do we really want that or is it better to instead just use something like onItem?
The reason I am asking is because the use of this will cause the Uni to be evaluated no matter what - whereas otherwise if the user just created the Uni but didn't return it to Quarkus, nothing would have happened.

I am not sure what the proper behavior is here.

[Sgitario]

Actually I have one more question:

Currently the implementation subscribes to the Uni for HR - do we really want that or is it better to instead just use something like onItem? The reason I am asking is because the use of this will cause the Uni to be evaluated no matter what - whereas otherwise if the user just created the Uni but didn't return it to Quarkus, nothing would have happened.

I am not sure what the proper behavior is here.

When I implemented this feature, I tried first using onItem and didn't work (the listeners were not invoked), but I can try again. Will keep you posted.

[geoand]

I tried first using onItem and didn't work (the listeners were not invoked)

Yeah, if the Uni is not subscribed to by Quarkus (or the user), then nothing will get executed.

I am wondering what behavior we really want though... I can see arguments for either way

[Sgitario]

I tried first using onItem and didn't work (the listeners were not invoked)

Yeah, if the Uni is not subscribed to by Quarkus (or the user), then nothing will get executed.

I am wondering what behavior we really want though... I can see arguments for either way

I just confirmed that using onItem().invoke() will not execute the after listeners. So, I think our only way is to keep using the suscribe().with() approach.
Also, take into account that this will only be used if there is at least one after listener implemented by user, not always, so there won't be performance penalty here.

[geoand]

Okay, fine from side :)

[Sgitario]

Merging this pull request as all reviewers are ok with these changes.