Hi Craig,
Thanks for the great feedback -- some comments below....
On 7/20/2011 6:18 PM, Linda DeMichiel wrote:
> Feedback from Craig Ringer on the users list.
>
>
> -------- Original Message --------
> Subject: Re: [jpa-spec users] [jsr338-experts] injection into entity listeners
> Date: Thu, 21 Jul 2011 08:51:45 +0800
> From: Craig Ringer <craig_at_postnewspapers.com.au>
> To: jsr338-experts_at_jpa-spec.java.net
> CC: Linda DeMichiel <linda.demichiel_at_oracle.com>, gordon.yorke_at_oracle.com, Adam Bien <abien_at_adam-bien.com>
>
>
>
> On 21/07/2011 3:51 AM, Linda DeMichiel wrote:
>> * An entity listener class may be defined for CDI injection.
>> A listener class that is intended to receive CDI injection must
>> be annotated with the @BeanListener annotation (final annotation
>> name TBD).
> Is that a performance optimization out of concern that unnecessary injection or indirection of instantiation via CDI may
> slow things down if it's not needed?
>
It is to allow more flexibility to JPA implementations, which might need to distinguish the two.
It is also an optimization.
> By and large, in Java EE 6 post-CDI, things are injectable by default with no class annotations or any other
> configuration required to enable injection. The presence of CDI injection points like @Inject or @Resource is
> sufficient. For consistency and usability it might (see below) be desirable to retain that for entity listeners if it's
> posible to do so without harmful side effects. Adding a magic "enable CDI" annotation may cause headaches for people
> chasing unexpected NPEs /when/ they forget it and breaks the usual pattern of CDI-enabled apps.
>
> In this case I actually think requiring an enabler annotation /will /be justified. I don't think using the BeanManager
> SPI to do injection at instantiation time will be enough (see below). If full CDI support is needed, I can see good
> reasons to keep listeners that don't need CDI injection and scoping as simple and fast as possible.
>
>> * An entity listener class that is defined as a @BeanListener
>> may specify PostConstruct and PreDestroy methods. These methods
>> will be invoked by the provider after injection has taken
>> place and before the entity listener instance is destroyed respectively.
> That should be taken care of by use of the CDI SPI to allocate and destroy entity listeners. The provider should not
> need to invoke them directly, and may land up invoking them again if CDI has already done so.
>
Right. The persistence provider calls into CDI to have these methods invoked.
> Perhaps: "If any no-argument method annotated @PostConstruct is found on an entity listener annotated with
> @BeanListener, this method must be invoked exactly once after the constructor and initializer have run and before any
> other methods are called."
>
> I don't think this section will matter anyway if CDI-enabled listeners become managed beans, as I suspect will need to
> happen. See below.
>
>> * The persistence provider is only required to support CDI
>> injection into listeners in Java EE container environments
>> where the BeanManager is available in JNDI as java:comp/BeanManager.
>> When this is not the case, @BeanListener classes will be
>> ignored.
>
> Total agreement there.
>
> Implementations might decide to log a warning, but the spec doesn't need to care about that.
>> Finally, just to be very clear about this, an entity listener
>> would thus be, in CDI terminology, an "unmanaged object".
>> In other words, it would receive CDI injections, would not itself
>> be available for injection into other CDI-managed beans.
>
> At first reading I didn't see any reason to make entity listeners managed objects. After writing my comments on
> lifecycle below, though, I realized that they probably should be because (IMO) they need to be able to be scoped and
> because injection is nearly useless without context.
>
The context would need to be that of the calling component.
>
> The JPA 2.0 spec states that:
>
> "Entity listeners are stateless. The lifecycle of an entity listener is unspecified."
>
> Is that necessarily going to be useful for a listener subject to injection?
>
I'd like to have the group discuss what we say about the lifecycle issue. AFAICT, however,
the safest approach seems to be the creation of listener instances per invocation.
If the information I have about the container/CDI integration is correct, that will
result in the injection using the context of the calling component, which is what you
would get if JNDI lookup were used.
> Consider just one use case for injection: injecting out the current security principal to update the "last changed by
> user" field of an entity. If no lifecycle is defined and no consideration of state is made, there is no obvious point at
> which (re)injection should be performed to ensure that the injected principal matches the current state of the
> application. Since providers are also currently free to re-use and recycle entity listeners, the java.security.Principal
> of the user active at the time the listener was created will be injected, and /will not be replaced when the listener is
> called again on entities changed by other users/. The listener will be getting the wrong answer.
>
> The only way to work around this at the application level will be by always injecting an Instance<Principal> and looking
> it up on each use within the entity listener ... or to grab an EJB from JNDI and use it, ignoring CDI completely. While
> that works, it is a big break from the general CDI programming model and - IMO - if CDI is to be supported, it should be
> supported properly.
>
> Right now, if an EntityListener wants any kind of information related to the user session, it has to look things up in
> JNDI. If injection is to substitute for this, it needs to offer at least the same match with current context.
>
> I'd go so far as to say that supporting injection without definining re-injection points in a lifecycle for entity
> listeners is going to be nearly useless. CDI - /Contexts /and dependency injection - is all about scope and lifetime.
> Applications are going to want access to more than just @ApplicationScoped managed beans, they're going to want to have
> access to application state that may be bound to the current user session or even user request. They can already do this
> to some extent via JNDI lookups of @Stateful EJBs, so I'm not suggesting introducing a completely new idea, just a way
> to fit it into the CDI model now standardized in EE 6.
>
> I know that getting into state and lifecycle is a can of worms it may not be desirable to open. Nonetheless it should
> probably be considered in more detail - if only to confirm that it's right to sidestep it if that turns out to be the
> case. It'll be hard to change the behavior of injection-aware entity listeners down the track if JPA 2.1 includes them
> with undefined life cycle, and it's my strong belief that in the long run such a change /will/ be needed.
>
> If a @BeanListener annotation (perhaps better as @EntityListenerBean) *is* to be used, one advantage is that it'd allow
> the JPA 2.1 spec to define a lifecycle for such listeners without risking messing with existing listeners and risking
> introducing backward compatibility issues.
>
Yes, that was part of it.
> Here's a broad outline of what I'm thinking:
>
> - Unless annotated with @EntityListenerBean, entity listeners are stateless, have unspecified lifecycle, are not subject
> to injection or CDI lifecycle callbacks, and are instantiated directly by persistence providers.
>
Yes
> - Persistence providers may choose to emit a warning if CDI annotations are detected on an entity listener that is not
> annotated @EntityListenerBean .
>
Yes
> - An entity listener annotated with @EntityListenerBean is a managed bean. Such an "entity listener bean" has a
> lifecycle managed by CDI, is scoped by CDI and is subject to injection.
>
No and Yes (or Yes and No). I say this because in my experience there is a lot of confusion
about what the term "managed bean" means. What I don't want to do is to turn entity listeners
into full-fledged CDI managed beans (i.e., objects that can be injected by CDI into other
objects). I.e., entity listeners are not intended to be contextual instances.
> - Persistence providers must obtain references to entity listener beans from CDI. Providers must never directly
> instantiate entity listener beans.
>
Yes
> In other words, an entity listener bean, identified by @EntityListenerBean, is a regular CDI managed bean that is
> referenced as an entity listener and defines one or more entity listener callback methods.
>
No and Yes again
> That isn't especially cheap, but it's a cost only paid by listeners that need those features. It doesn't impose any cost
> on existing listeners.
>
> I don't have the experience with the guts of EclipseLink and Hibernate required to have much of an opinion on
> implementation difficulty. Given the way the BeanManager is focused on supporting this use case I suspect it won't be
> much more difficult than what Linda proposed, but I don't know that for sure. I'm hoping it'll be as simple as
> categorizing listeners into CDI- and non-CDI during annotation processing at startup, so instead of one list of
> listeners for each entity there are two. The non-CDI listeners are unaffected. For the CDI listeners, the BeanManager is
> asked for an instance of the class whenever it is needed and the returned instance is called.
>
> I'd love to have the feedback from someone with experience with CDI integration on this. The Mojarra team integrated CDI
> support so they may have some useful feedback, as may the Seam 3 Faces team (http://seamframework.org/Seam3/FacesModule)
> who extended and enhanced it.
>
Thanks again for all of your feedback.
regards,
-Linda
p.s. Unfortunately java.net still seems to be setting the reply-to field for mail that
is forwarded from the experts list to the users list to the experts list. We have
already filed an issue on this, but suggest you redirect the to: field to the users
list (or I can continue to forward).
-l
> --
> Craig Ringer
>
> POST Newspapers 276 Onslow Rd, Shenton Park Ph: 08 9381 3088 Fax: 08 9388 2258 ABN: 50 008 917 717
> http://www.postnewspapers.com.au/