Re: [seam-dev] Fwd: [seam-commits] Seam SVN: r12493 - in modules/faces/trunk/impl/src/main: resources/META-INF/services and 1 other directory.

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

On Wed, Apr 21, 2010 at 4:07 PM, Dan Allen <dan.j.allen(a)gmail.com&gt; wrote: > On Wed, Apr 21, 2010 at 3:51 PM, Dan Allen <dan.j.allen(a)gmail.com&gt; wrote: > >> >>> On Thu, Apr 15, 2010 at 6:14 AM, Pete Muir <pmuir(a)redhat.com&gt; 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&gt; 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&gt; 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&gt; 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&gt; 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"