jakartaee / nosql

The specification in Jakarta EE to help Jakarta EE developers create enterprise-grade applications using Java® and NoSQL technologies.
https://projects.eclipse.org/projects/ee4j.nosql
Eclipse Public License 2.0
92 stars 28 forks source link

PrePersist & PostPersist events #66

Closed amoscatelli closed 1 year ago

amoscatelli commented 3 years ago

Hi there, first of all I want to thank you for all this wonderfull work. Nosql was the kind of specification/abstraction Jakarta EE needed and I think that all of this (beginning with the 4 different kind of nosql interfaces) is a stroke of genius.

Let's get into business:

1) There is a typo : jakarta.nosql.mapping.EntityPostPersit should be jakarta.nosql.mapping.EntityPostPersist

2) I believe using interfaces to manage PrePersist & PostPersist events is a mistake. JPA uses annotations : javax.persistence.PrePersist javax.persistence.PostPersist

Why annotations are better ? Imagine you have a hierarchy of MappedSuperclass, if you override EntityPrePersist methods you have to pay attention to call super implementations. This is prone error, you may just want to "do things" in every MappedSuperclass without knowing such interfaces were already implemented.

Please consider using annotations like jpa and scanning every method (in the whole hierarchy) and call each of them to handle prepersist and post persist events :

package jakarta.nosql.mapping;

import java.lang.annotation.Target;
import java.lang.annotation.Retention;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

/**
 * Specifies a callback method for the corresponding 
 * lifecycle event. This annotation may be applied to methods 
 * of an entity class or a mapped superclass.
 *
 * @since 1.0
 */
@Target({METHOD}) 
@Retention(RUNTIME)

public @interface PrePersist {}

and

package jakarta.nosql.mapping;

import java.lang.annotation.Target;
import java.lang.annotation.Retention;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

/**
 * Specifies a callback method for the corresponding 
 * lifecycle event. This annotation may be applied to methods 
 * of an entity class or a mapped superclass.
 *
 * @since 1.0
 */
@Target({METHOD}) 
@Retention(RUNTIME)

public @interface PostPersist {}
amoscatelli commented 3 years ago

Even more, you may have entities with an attribute "value" and the getValue() could be misleading and could get in the way.

amoscatelli commented 3 years ago

If you agree with me (to change the spec) I can make a pull request here (the spec) and a pull request for the reference implementation to help you.

otaviojava commented 3 years ago

Hello @amoscatelli Thank you for the suggestion. That sounds like an amazing idea. I'll put it as an enhancement to release after the 1.0.0 release. Right now, we're closing the changes in the API to release this version and then push new improvements in the next versions.

amoscatelli commented 3 years ago

Hi @otaviojava
So you plan to keep both of them (interfaces and annotations) ? Otherwise you'll need to deprecate and remove interfaces later ...

otaviojava commented 3 years ago

Yeap, or maybe use the Jakarta commons annotation instead:

amoscatelli commented 3 years ago

Probably keeping them both would be better. I can't see a clear mapping/semantic with those annotations.

otaviojava commented 1 year ago

Hey, @amoscatelli, we reduced the spec to this first version. I'll close this one; we can start on the JNoSQL side instead.