I just extracted it from the two JSF event bridges that happened to need it.
And a week ago you said "I like the super class approach, it makes the
ugliness of it apparent nicely :-)" when I suggested the static helper
On Thu, Apr 22, 2010 at 1:45 PM, Pete Muir <pmuir(a)redhat.com> wrote:
Nik, why did you do it as a superclass in the first place?
On 21 Apr 2010, at 21:38, Dan Allen wrote:
> On Wed, Apr 21, 2010 at 4:22 PM, Lincoln Baxter, III <
lincolnbaxter(a)gmail.com> wrote:
> I'm in agreement with this train of logic.
>
> BeanManagerLocator sounds fine to me. BeanManagerAware may then go away
since this would be providing a utility class (not a superclass.)
>
> You could still have BeanManagerAware I suppose...it would just be
another way to get the BeanManager. You can either do the static call, or
you can inherit and have the static call done for you.
>
> public class NonCDIBean extends BeanManagerAware {
>
> public void doSomething() {
> BeanManager beanManager = getBeanManager();
> ...
> }
> }
>
> vs
>
> public class NonCDIBean {
>
> public void doSomething() {
> BeanManager beanManager = BeanManagerLocator.getBeanManager();
> }
> }
>
> Keep in mind, these non-CDI beans are not in the end developers code,
this is internal extension stuff, for example the SystemEventListener and
PhaseListener implementations for CDI event bridging.
>
> As for the locator, we are currently refining the strategy and will
submit a formal proposal for review in a separate thread.
>
> -Dan
>
>
> On Wed, Apr 21, 2010 at 4:07 PM, Dan Allen <dan.j.allen(a)gmail.com>
wrote:
> On Wed, Apr 21, 2010 at 3:51 PM, Dan Allen <dan.j.allen(a)gmail.com>
wrote:
>
> On Thu, Apr 15, 2010 at 6:14 AM, Pete Muir <pmuir(a)redhat.com> wrote:
> Guys,
>
> There still seems to be some confusion about why this is an issue that we
*must* address. Taking a step back, we have some general requirements for
Seam 3:
>
> * that we want to use the Java SE platform
> * that we also want to use "app servers"
> * that we want these extensions to be portable not just across Java EE
containers but across Servlet containers too
> * that we want this to work in more esoteric deployment scenarios than a
single-war-per-server-instance OR bundling the extension as a library of the
war OR using special tricks like deployers in JBoss AS
>
> Then there is *no* general solution to this problem that I know of (I'm
not just not telling people what the solution because I'm some sort of
sadist ;-). Instead, we have a number of partial solutions, which, when
combined should get us the whole way there.
>
> I see that I was wrong to not put this in Weld Extensions from the
beginning. We can take Nik's basic design, add service providers into the
mix (to make this extensible and remove the hard dep on the servlet
container) which should at least mean we don't reinvent a square wheel many
times ;-).
>
> However, it is still *extremely* important that each time you can't just
inject the BeanManager you take a step back, and consider how you need to
access the BeanManager. For example, it is infinitely better to push the
BeanManager into some sort of locally propagated context than to resort to
lookup techniques. There is no guarantee that lookup techniques will work
everywhere, whilst a actively propagating the BeanManager will.
>
> So, before you extend BeanManagerAware, please please consider whether
there is a better option. It is a measure of last resort *only*. Often
people on the dev list will have a good idea about how to get it, so please
ask.
>
> I will add these notes to the lookup class. Before we do release
candidates of a module, you can expect the BeanManagerAware police to visit
and possibly demand changes too ;-)
>
> On 14 Apr 2010, at 20:53, Nicklas Karlsson wrote:
>
> > I think the real issue was the singleton extension...
> >
> > On Wed, Apr 14, 2010 at 10:39 PM, Lincoln Baxter, III <
lincolnbaxter(a)gmail.com> wrote:
> > The statics are only used instead of new BeanManagerProvider{} ... they
are not a problem and can go away. Is there another complaint?
> >
> >
> > On Wed, Apr 14, 2010 at 2:59 PM, Nicklas Karlsson <nickarls(a)gmail.com>
wrote:
> > Reverting & repenting
> >
> > It's for the JSF -> CDI event bridge. I need a BeanManager to propagate
the events to but the problem is that the first JSF global system event is a
PostApplicationConstructed which is pretty soon.
> >
> > First I try to look for it in a servlet context attribute (which the
seam-servlet puts there but it's not there yet in this case)
> > Then I try to look in JNDI (sure, it's there but you recommended we
should try to avoid this to make this available in as many places as
possible)
> > Then I tried the static one as a last effort.
> >
> >
> > On Wed, Apr 14, 2010 at 5:54 PM, Pete Muir <pmuir(a)redhat.com> wrote:
> > Guys,
> >
> > We cannot rely on statics in Seam 3. Please don't add code like this.
If you have a problem accessing the BeanManager from your code, please ask
here about the best solution.
> >
> > Nik, can you revert this please. And then explain what situation you
are trying to address.
> >
> > Thanks.
> >
> > Begin forwarded message:
> >
> > > From: seam-commits(a)lists.jboss.org
> > > Date: 14 April 2010 13:50:21 GMT+01:00
> > > To: seam-commits(a)lists.jboss.org
> > > Subject: [seam-commits] Seam SVN: r12493 - in
modules/faces/trunk/impl/src/main: resources/META-INF/services and 1 other
directory.
> > > Reply-To: seam-commits(a)lists.jboss.org
> > >
> > > Author: nickarls
> > > Date: 2010-04-14 08:50:21 -0400 (Wed, 14 Apr 2010)
> > > New Revision: 12493
> > >
> > > Added:
> > >
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerPickupExtension.java
> > >
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/SingletonBeanManagerProvider.java
> > > Modified:
> > >
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerAware.java
> > >
modules/faces/trunk/impl/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension
> > > Log:
> > > Last attempt singletonish approach when others fail. Yes, I'm
ashamed.
> > >
> > > Modified:
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerAware.java
> > > ===================================================================
> > > ---
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerAware.java
2010-04-14 12:21:26 UTC (rev 12492)
> > > +++
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerAware.java
2010-04-14 12:50:21 UTC (rev 12493)
> > > @@ -45,6 +45,7 @@
> > >
beanManagerProviders.add(ServletContextBeanManagerProvider.DEFAULT);
> > > beanManagerProviders.add(JndiBeanManagerProvider.DEFAULT);
> > > beanManagerProviders.add(JndiBeanManagerProvider.JBOSS_HACK);
> > > +
beanManagerProviders.add(SingletonBeanManagerProvider.DEFAULT);
> > > }
> > >
> > > protected BeanManager getBeanManager()
> > >
> > > Added:
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerPickupExtension.java
> > > ===================================================================
> > > ---
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerPickupExtension.java
(rev 0)
> > > +++
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/BeanManagerPickupExtension.java
2010-04-14 12:50:21 UTC (rev 12493)
> > > @@ -0,0 +1,56 @@
> > > +/*
> > > + * JBoss, Home of Professional Open Source
> > > + * Copyright 2010, Red Hat, Inc., and individual contributors
> > > + * by the @authors tag. See the copyright.txt in the distribution
for a
> > > + * full listing of individual contributors.
> > > + *
> > > + * This is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU Lesser General Public License as
> > > + * published by the Free Software Foundation; either version 2.1 of
> > > + * the License, or (at your option) any later version.
> > > + *
> > > + * This software is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this software; if not, write to the Free
> > > + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
MA
> > > + * 02110-1301 USA, or see the FSF site:
http://www.fsf.org.
> > > + */
> > > +package org.jboss.seam.faces.cdi;
> > > +
> > > +import javax.enterprise.event.Observes;
> > > +import javax.enterprise.inject.spi.AfterBeanDiscovery;
> > > +import javax.enterprise.inject.spi.BeanManager;
> > > +import javax.enterprise.inject.spi.Extension;
> > > +
> > > +/**
> > > + * Singleton(ish) extension that observes the AfterBeanDiscovery
event and stores the BeanManager for access
> > > + * in places where injection is not available and JNDI or
ServletContext access is not preferable.
> > > + *
> > > + * @author Nicklas Karlsson
> > > + *
> > > + */
> > > +public class BeanManagerPickupExtension implements Extension
> > > +{
> > > + private static BeanManagerPickupExtension instance;
> > > + private volatile BeanManager beanManager;
> > > +
> > > + public BeanManager getBeanManager()
> > > + {
> > > + return beanManager;
> > > + }
> > > +
> > > + public static BeanManagerPickupExtension getInstance()
> > > + {
> > > + return instance;
> > > + }
> > > +
> > > + public void pickupBeanManager(@Observes AfterBeanDiscovery e,
BeanManager beanManager)
> > > + {
> > > + this.beanManager = beanManager;
> > > + BeanManagerPickupExtension.instance = this;
> > > + }
> > > +}
> > >
> > > Added:
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/SingletonBeanManagerProvider.java
> > > ===================================================================
> > > ---
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/SingletonBeanManagerProvider.java
(rev 0)
> > > +++
modules/faces/trunk/impl/src/main/java/org/jboss/seam/faces/cdi/SingletonBeanManagerProvider.java
2010-04-14 12:50:21 UTC (rev 12493)
> > > @@ -0,0 +1,43 @@
> > > +/*
> > > + * JBoss, Home of Professional Open Source
> > > + * Copyright 2010, Red Hat, Inc., and individual contributors
> > > + * by the @authors tag. See the copyright.txt in the distribution
for a
> > > + * full listing of individual contributors.
> > > + *
> > > + * This is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU Lesser General Public License as
> > > + * published by the Free Software Foundation; either version 2.1 of
> > > + * the License, or (at your option) any later version.
> > > + *
> > > + * This software is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this software; if not, write to the Free
> > > + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
MA
> > > + * 02110-1301 USA, or see the FSF site:
http://www.fsf.org.
> > > + */
> > > +package org.jboss.seam.faces.cdi;
> > > +
> > > +import javax.enterprise.inject.spi.BeanManager;
> > > +
> > > +/**
> > > + * A BeanManager provider for an extension provided singleton
> > > + *
> > > + * @author Nicklas Karlsson
> > > + *
> > > + */
> > > +public class SingletonBeanManagerProvider implements
BeanManagerProvider
> > > +{
> > > +
> > > + public static final BeanManagerProvider DEFAULT = new
SingletonBeanManagerProvider();
> > > +
> > > + @Override
> > > + public BeanManager getBeanManager()
> > > + {
> > > + return
BeanManagerPickupExtension.getInstance().getBeanManager();
> > > + }
> > > +
> > > +}
> > >
> > > Modified:
modules/faces/trunk/impl/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension
> > > ===================================================================
> > > ---
modules/faces/trunk/impl/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension
2010-04-14 12:21:26 UTC (rev 12492)
> > > +++
modules/faces/trunk/impl/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension
2010-04-14 12:50:21 UTC (rev 12493)
> > > @@ -1,3 +1,4 @@
> > > org.jboss.seam.faces.context.ViewScopedExtension
> > > org.jboss.seam.faces.context.FlashScopedExtension
> > > -org.jboss.seam.faces.context.FacesAnnotationsAdapterExtension
> > > \ No newline at end of file
> > > +org.jboss.seam.faces.context.FacesAnnotationsAdapterExtension
> > > +org.jboss.seam.faces.cdi.BeanManagerPickupExtension
> > >
>
> On Thu, Apr 15, 2010 at 10:51 AM, Lincoln Baxter, III <
lincolnbaxter(a)gmail.com> wrote:
>
> I like what Nik has done so far (in terms of the chain of responsibility
pattern for various BM lookups.) So keeping that, let's talk about what else
we need to provide the missing lookup functionality (that we can identify.)
>
> What's there now is good for a start.
>
> Funny that I implemented almost the same approach a year ago. Only, it
had two major shortcomings:
>
> a) I was lazy and just used the Java 6 service loader API
> b) I used bad names for things (I called it a bridge API and that gave
Pete chills :))
>
>
> I agree -- BeanManagerAware is ONLY for places where you absolutely
cannot access the beanmanager in a proper fashion, or cannot achieve
injection in the proper fashion. This should be very clearly stated -- I'll
update the docs.
>
> I'm in tune with Pete and Lincoln that the locator should be used when
there is no other option. Yet, when it is used, it needs to avoid assuming
anything about the deployment environment (i.e., be pluggable).
>
> With that said, I think we should spend time getting the naming right.
BeanManagerAware just doesn't sound right to me. First of all, it sounds
like something in Spring. And second, it's not making the bean aware of
anything, it's a lookup mechanism. I'll throw in some proposals, but feel
free to refine:
>
> BeanManagerLookup#getBeanManager()
> BeanManagerContext#getBeanManager()
> BeanManagerContext#lookupBeanManager()
> BeanManagerLocator#getBeanManager()
>
> We should avoid the use of statics to hold values too, because that can
cause real problems in certain deployment scenarios. I would think using a
ThreadLocal would be better.
>
> I didn't realize the BeanManagerAware was an abstract super class. In
that context, the name makes sense. But I don't think it should be doing to
resolution. The resolution itself should be handled in a separate class that
BeanManagerAware uses. That way, we don't mix the two concerns.
>
> -Dan
>
> --
> Dan Allen
> Senior Software Engineer, Red Hat | Author of Seam in Action
> Registered Linux User #231597
>
>
http://mojavelinux.com
>
http://mojavelinux.com/seaminaction
>
http://www.google.com/profiles/dan.j.allen
>
>
>
> --
> Lincoln Baxter, III
>
http://ocpsoft.com
>
http://scrumshark.com
> "Keep it Simple"
>
>
>
> --
> Dan Allen
> Senior Software Engineer, Red Hat | Author of Seam in Action
> Registered Linux User #231597
>
>
http://mojavelinux.com
>
http://mojavelinux.com/seaminaction
>
http://www.google.com/profiles/dan.j.allen
_______________________________________________
seam-dev mailing list
seam-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/seam-dev