first rough proposal for events

[gavinking]

See #373. [Note that the first to propose this was @hantsy.]

Here's a rough and fairly minimal idea for what this could look like.

Note that here there is an Event class for each kind of event, instead of a single LifecycleEvent with @Before @Update and @After @Insert qualifiers on the observer side. The reason for this is that we don't have a dependency on CDI, so I can't use the @Qualifier annotation to declare event qualifier types.

An event observer would look like:

void afterUpdate(@Observes PostUpdateEvent<Book> event) { ... }

I would love to be able to filter by entity type, but I don't think CDI allows filtering events by type argument. (At least it didn't in 1.0, though that was a long time ago.)

As a workaround for that, we could have @Before @Update @EntityType(Book.class) as qualifiers, but, again, that would pick up a dependency on CDI.

[gavinking]

I would love to be able to filter by entity type, but I don't think CDI allows filtering events by type argument.

An intriguing possibility here would be to generate event types into the static metamodel, so that you could write:

void afterUpdate(@Observes _Book.PostUpdateEvent event) { ... }

[gavinking]

I don't think CDI allows filtering events by type argument. (At least it didn't in 1.0, though that was a long time ago.)

I take that back. Apparently they relaxed this restriction, and you can now fire a parameterized event as long as the type argument is somehow resolvable, for example:

    @Inject
    Event<MyEvent<String>> event;

Or:

        beanManager.getEvent()
                .select(new TypeLiteral<MyEvent<String>>() {})
                .fire(new MyEvent<>(title));

So in fact we can use a parameterized event type, e.g. PostUpdateEvent<Book>.

[njr-11]

This is nicely written and I like the change to use the parameterized entity type rather than Object.

I made some minor suggestions and proposed switching the Abstract supertype for the event into an interface.

I addition to the inline review comments, there are a few other things we will need to consider.

One important item is clarifying what the user is (or is not) allowed to do with the entity instance that they get within the event. Are they allowed to modify it? And if so, do those modifications get persisted to the datastore?

The entity value returned on the Post events is a difficult question to consider. I can certainly see why you wrote it to state the value passed in the lifecycle method, because that makes the most sense for PostDeleteEvent (to avoid extra fetch of values) and is consistent with the Delete lifecycle method which only supports a void return value.
For the PostInsertEvent and PostUpdateEvent, I wonder if some implementations based on a JPA provider might inadvertently end up with the entity value updated to include an automatically generated id and autoincremented version, which would no longer match the passed in entity value. And having that information might be useful to the user as well, so I could see a case for defining these differently, but then they would be inconsistent. Not really sure what is best here, but I thought I would at least bring it up.

[gavinking]

One important item is clarifying what the user is (or is not) allowed to do with the entity instance that they get within the event. Are they allowed to modify it? And if so, do those modifications get persisted to the datastore?

We can look at section 3.6.2 of the JPA spec for inspiration here.

For the PostInsertEvent and PostUpdateEvent, I wonder if some implementations based on a JPA provider might inadvertently end up with the entity value updated to include an automatically generated id and autoincremented version, which would no longer match the passed in entity value.

Yeah good point. Hrrm. Not sure.

[njr-11]

For the PostInsertEvent and PostUpdateEvent, I wonder if some implementations based on a JPA provider might inadvertently end up with the entity value updated to include an automatically generated id and autoincremented version, which would no longer match the passed in entity value.

Yeah good point. Hrrm. Not sure.

I tried this out on the EclipseLink JPA provider and observed it to be doing the following:

After invoking persist, the AutoGenerated Id value was updated on the passed-in entity, but the Version value was not incremented. After also invoking flush, then it updated the Version value on that entity instance.

After invoking merge, the Version value was not immediately incremented on the passed-in entity. As in the former case, after also invoking flush, it then updated the Version value on the passed-in entity instance.

Note that Jakarta Data's CrudRepository.insert has language like, "After invoking this method, do not continue to use the instance that is supplied as a parameter. This method makes no guarantees about the state of the instance that is supplied as a parameter." All of this makes me uncomfortable about supplying that same instance via the PostInsert/PostUpdate events. But I can also see why we don't want to require it to supply an updated entity instance. That might require some implementations to do extra fetching just for the sake of publishing events that the application might not even care about. Leaving out the entity from the event makes the Post event not useful because you cannot correlate it with the Pre event. I suppose one option would be to supply only the Id value and entity class (not instance) on the Post event which would at least identify it as matching the Pre event, but that is awkward. Not really sure what would be best.

[gavinking]

Not really sure what would be best.

I mean with things like this, there's always the option of saying "portable applications should not rely on .... ".

[njr-11]

In addition to the suggested updated, the other item we discussed during today's Jakarta Data spec meeting was making the Post events not include the entity (and alternatively maybe include the Pre event instead if correlation is needed).

To do this, LifecycleEvent will probably need to be split into LifecyclePreEvent and LifecyclePostEvent.

[gavinking]

making the Post events not include the entity (and alternatively maybe include the Pre event instead if correlation is needed).

So we definitely need the Post event to carry some sort of info about the entity (otherwise it's useless).

As you say, we could indeed give it a ref to the corresponding Pre event, which would be nice from the user perspective, but I'm a bit worried about how this might impact implementations. It does seem to make life hard for someone implementing this on top of the JPA @EntityListener stuff, since these @EntityListeners are not really stateful.

[njr-11]

I suppose another alternative is to have the Post event be defined to include the same entity instance as the corresponding Pre event, with the restriction that portable applications must not rely on its state. In this case, the event subscriber would be able to portably perform a == comparison to correlate the events.

[gavinking]

Well they can also use the id.

[njr-11]

Well they can also use the id.

Yes, for update, delete, and insert that doesn't use autogenerated.

[gavinking]

insert that doesn't use autogenerated

Ah yeah, sure, you're right.

[gavinking]

I have taken a stab at the semantics of the entity in Post events.

WDYT?

[njr-11]

Looks good. I added some minor typo corrections and copyright corrections.

[njr-11]

I should also point out that if the application is using transactions and rolls back a successful life cycle operation, the post event will have been sent even though the change is not committed to the database. For example,

tran.begin();
books.update(modifiedBook); // Pre and Post events are sent here
...
tran.rollback();

Just pointing it out in case it is a concern.

[hantsy]

Why not use sealed interface and record class for the events. This feature should be included after data 1.0, we can consider new Java features.

[gavinking]

@hantsy theres no motivation for treating these as a sum type.

1. Client code doesn't switch over them, and
2. anyway, it's an open universe here: the set of lifecycle method annotations is extensible (and we will extend it!) and so therefor is the set of lifecycle event types.

If we made this a sum type, then we would break client code whenever we add a new lifecycle method type.

P.S. this is for Data 1.1, so it's still targeting the same version of the Java platform.

[gavinking]

I should also point out that if the application is using transactions and rolls back a successful life cycle operation, the post event will have been sent even though the change is not committed to the database.

Well, so apparently , back in 2008, I anticipated that we would run into this issue sometime in 2024, and so I gave CDI observer methods the ability to be notified according to transaction phase. This is actually really beautiful for our use case and makes what is being proposed in this PR far more powerful than I realized it was.

public void onNewOrder(@Observes(during=AFTER_SUCCESS) PreInsertEvent<Order> newOrder) {
    ...
}

I just learned about this incredibly nice feature of the CDI spec by from Google, and was then taken by complete surprised to see my own @author tag on it.

[gavinking]

I have added this new language.

I've also addressed all suggestions from @njr-11.

[gavinking]

back in 2008, I anticipated that we would run into this issue sometime in 2024

Ok, I guess that sounds pretty implausible to you guys.

What really happened is that I just went back in time and added TransactionPhase to CDI 1.0 when I saw Nathan's comment.

[njr-11]

What really happened is that I just went back in time and added TransactionPhase to CDI 1.0 when I saw Nathan's comment.

However it was that you got TransactionPhase to already be in CDI, nice work! It fits perfectly with this. And good idea getting it into the LifecycleEvent Javadoc, which will help users know they can use it.

Everything I pointed out for this PR has been addressed, and I'm approving it now.

[otaviojava]

For the first step, it is great, IMHO, I will merge, and we can refactor this code or the documentation with time.

[gavinking]

Excellent, thanks folks!