Re: [hibernate-dev] ManagedBeanRegistry - dependency injection support

Hi, Following our conversations on HipChat and the various changes you implemented (thanks!), I tested the current implementation. There were a few issues, but I managed to fix them and make all the tests in Hibernate Search pass. Here is a PR with the fixes: https://github.com/hibernate/hibernate-orm/pull/2092 Yoann Rodière Hibernate NoORM Team yoann(a)hibernate.org On 14 December 2017 at 18:42, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > Here is the commit with initial support for named CDI beans and support > for bypassing registry caching of ManagedBeans : > https://github.com/hibernate/hibernate-orm/commit/ddc1f03abc675a27ed025b8... > > There is still a question of whether named beans support needs to do > the javax.enterprise.inject.spi.InjectionTarget stuff > > Christian, I ended up going to "beans" as the package name because this > supports non-CDI environments (direct instantiation) too. Not overly a fan > of "beans" (overloaded term) but it was a lesser of evils. > > On Thu, Dec 14, 2017 at 10:57 AM Steve Ebersole <steve(a)hibernate.org&gt; > wrote: > >> Yoann, does this approach still need to do the injections >> (javax.enterprise.inject.spi.InjectionTarget)? >> >> >> On Thu, Dec 14, 2017 at 8:01 AM Yoann Rodiere <yoann(a)hibernate.org&gt; >> wrote: >> >>> Here is how it should work from what I understand (adapted from an >>> implementation in Search, which has slightly different requirements): >>> >>> static <T> T getBeanInstance(BeanManager beanManager, String beanName, >>> Class<T> contract) { >>> Set<Bean<?>> beans = beanManager.getBeans(contract, new NamedQualifier( >>> beanName)); >>> if ( beans.isEmpty() || beans.size() > 1 ) { >>> // TODO proper error messages >>> throw new IllegalArgumentException( "No matching beans or multiple >>> matching beans" ); >>> } >>> Bean<?> bean = beans.iterator().next(); >>> CreationalContext<T> creationalContext = >>> beanManager.createCreationalContext( bean ); >>> return contract.cast( beanManager.getReference( bean, contract, >>> creationalContext ) ); >>> } >>> >>> With NamedQualifier being the implementation I gave before. >>> >>> Sure, let's talk about it later on HipChat. Especially the caching >>> thing, it's really a blocker for Search. >>> >>> I'll be online to travel in about 3 hours, though. >>> >>> >>> Yoann Rodière >>> Hibernate NoORM Team >>> yoann(a)hibernate.org >>> >>> On 14 December 2017 at 14:46, Steve Ebersole <steve(a)hibernate.org&gt; >>> wrote: >>> >>>> I'll be on HipChat later after I get back from taking my son and >>>> daughter to school. Maybe it is easier to discuss there. >>>> >>>> On Thu, Dec 14, 2017 at 7:44 AM Steve Ebersole <steve(a)hibernate.org&gt; >>>> wrote: >>>> >>>>> But your answer above does not answer my question ;) >>>>> >>>>> I still have no idea how to go from name+Class -> bean. >>>>> >>>>> >>>>> On Thu, Dec 14, 2017 at 7:41 AM Yoann Rodiere <yoann(a)hibernate.org&gt; >>>>> wrote: >>>>> >>>>>> Yeah, it was 4AM in France when you asked :) I answered later on >>>>>> HipChat, the answer is basically the one I gave in my email. >>>>>> >>>>>> Yoann Rodière >>>>>> Hibernate NoORM Team >>>>>> yoann(a)hibernate.org >>>>>> >>>>>> On 14 December 2017 at 14:38, Steve Ebersole <steve(a)hibernate.org&gt; >>>>>> wrote: >>>>>> >>>>>>> WRT to named beans, I asked Guillaume on HipChat what that is >>>>>>> supposed to look like. IIRC he mentioned producers in Paris, but I found >>>>>>> no straight-forward way to get from name+class to a bean. >>>>>>> >>>>>>> He may have answered, I just have not been on HipChat yet today... >>>>>>> >>>>>>> On Thu, Dec 14, 2017 at 7:36 AM Steve Ebersole <steve(a)hibernate.org&gt; >>>>>>> wrote: >>>>>>> >>>>>>>> Its easier to cleanup >>>>>>>> >>>>>>>> On Thu, Dec 14, 2017 at 6:52 AM Steve Ebersole < >>>>>>>> steve(a)hibernate.org&gt; wrote: >>>>>>>> >>>>>>>>> There are a lot of changes to digest here, but if anyone wanted >>>>>>>>> to take a look at this so far... >>>>>>>>> >>>>>>>>> >>>>>>>>> https://github.com/hibernate/hibernate-orm/commit/564ec55ca10c0d5d2afd732... >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Dec 14, 2017 at 6:47 AM Steve Ebersole < >>>>>>>>> steve(a)hibernate.org&gt; wrote: >>>>>>>>> >>>>>>>>>> Actually my fault. Apparently renaming the package was way too >>>>>>>>>> aggressive and renamed the artifact >>>>>>>>>> >>>>>>>>>> On Thu, Dec 14, 2017 at 6:40 AM Steve Ebersole < >>>>>>>>>> steve(a)hibernate.org&gt; wrote: >>>>>>>>>> >>>>>>>>>>> Ah, nm. They change the artifact name. Boo! >>>>>>>>>>> >>>>>>>>>>> On Thu, Dec 14, 2017 at 6:39 AM Steve Ebersole < >>>>>>>>>>> steve(a)hibernate.org&gt; wrote: >>>>>>>>>>> >>>>>>>>>>>> Anyone know what happened to the 2.0 CDI artifact on Maven >>>>>>>>>>>> Central? It was there last week, but is no longer there... >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Dec 14, 2017 at 5:54 AM Steve Ebersole < >>>>>>>>>>>> steve(a)hibernate.org&gt; wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Thanks for the replies. So unless we hear otherwise from >>>>>>>>>>>>> anyone else, I will plan on supporting just one DI container. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Dec 14, 2017 at 2:54 AM Yoann Rodiere < >>>>>>>>>>>>> yoann(a)hibernate.org&gt; wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Same here, compositions don't seem to be a reasonable use >>>>>>>>>>>>>> case. And even if >>>>>>>>>>>>>> users provide a custom bean registry, they could just >>>>>>>>>>>>>> implement their >>>>>>>>>>>>>> specific behavior for a few specific case, then retrieve >>>>>>>>>>>>>> another >>>>>>>>>>>>>> implementations on their own and delegate to it however they >>>>>>>>>>>>>> want. >>>>>>>>>>>>>> Overriding the service initiator looks like a very >>>>>>>>>>>>>> reasonable way to do >>>>>>>>>>>>>> that. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regarding the package, "org.hibernate.resource.beans" seems >>>>>>>>>>>>>> more >>>>>>>>>>>>>> appropriate to me, since CDI is not the only implementation >>>>>>>>>>>>>> we will get and >>>>>>>>>>>>>> we know it. Also, if I wanted to nitpick, injection is not >>>>>>>>>>>>>> really something >>>>>>>>>>>>>> the bean registry must provide. We could imagine a bean >>>>>>>>>>>>>> registry without >>>>>>>>>>>>>> any support for injection, after all, just providing >>>>>>>>>>>>>> "monolithic beans". It >>>>>>>>>>>>>> would still make sense with respect to your >>>>>>>>>>>>>> ManagedBeanRegistry API. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yoann Rodière >>>>>>>>>>>>>> Hibernate NoORM Team >>>>>>>>>>>>>> yoann(a)hibernate.org >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 14 December 2017 at 08:01, Christian Beikov < >>>>>>>>>>>>>> christian.beikov(a)gmail.com&gt; >>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> > I don't think someone is actually going to use more than a >>>>>>>>>>>>>> single DI >>>>>>>>>>>>>> > framework and even if they do, they will probably bridge >>>>>>>>>>>>>> one way or >>>>>>>>>>>>>> > another between the DI frameworks to be able to access >>>>>>>>>>>>>> beans from one in >>>>>>>>>>>>>> > the other. >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > So I don't think we should do "compositions" since it's >>>>>>>>>>>>>> not a big deal >>>>>>>>>>>>>> > to integrate different DIs and is also IMO an edge case. >>>>>>>>>>>>>> I'd prefer the >>>>>>>>>>>>>> > package name `org.hibernate.resource.di` since CDI seems >>>>>>>>>>>>>> to be just one >>>>>>>>>>>>>> > of the possible "integrations". >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > Mit freundlichen Grüßen, >>>>>>>>>>>>>> > >>>>>>>>>>>>>> ------------------------------------------------------------------------ >>>>>>>>>>>>>> > *Christian Beikov* >>>>>>>>>>>>>> > Am 13.12.2017 um 21:04 schrieb Steve Ebersole: >>>>>>>>>>>>>> > > https://hibernate.atlassian.net/browse/HHH-11259 and >>>>>>>>>>>>>> friends are mainly >>>>>>>>>>>>>> > > about back porting the work I did on 6.0 for the >>>>>>>>>>>>>> ManagedBeanRegistry >>>>>>>>>>>>>> > > abstraction over dependency injection containers. We >>>>>>>>>>>>>> will ship support >>>>>>>>>>>>>> > for >>>>>>>>>>>>>> > > CDI as well as non-managed beans (things we directly >>>>>>>>>>>>>> instantiate). Of >>>>>>>>>>>>>> > > course we'd ideally make it easy to plug in other DI >>>>>>>>>>>>>> containers such as >>>>>>>>>>>>>> > > Spring. So I wanted to discuss the configuration of >>>>>>>>>>>>>> this support. >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > The first thing to consider is whether we want to >>>>>>>>>>>>>> support using multiple >>>>>>>>>>>>>> > DI >>>>>>>>>>>>>> > > containers simultaneously. E.g. is it conceivable that >>>>>>>>>>>>>> an application >>>>>>>>>>>>>> > > might want to use both CDI and Spring simultaneously? I >>>>>>>>>>>>>> started building >>>>>>>>>>>>>> > > in support for that via a CompositeManagedBeanRegistry >>>>>>>>>>>>>> implementation, >>>>>>>>>>>>>> > but >>>>>>>>>>>>>> > > stepping back I want to gauge whether that is >>>>>>>>>>>>>> "reasonable" before >>>>>>>>>>>>>> > > continuing down that path >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > Assuming that we do want to support such "compositions" >>>>>>>>>>>>>> the next question >>>>>>>>>>>>>> > > is how we see this being configured. Clearly any time a >>>>>>>>>>>>>> CDI BeanManager >>>>>>>>>>>>>> > is >>>>>>>>>>>>>> > > present during bootstrap we want to enable CDI >>>>>>>>>>>>>> ManagedBeanRegistry >>>>>>>>>>>>>> > > support. How would users indicate additional >>>>>>>>>>>>>> ManagedBeanRegistry impls >>>>>>>>>>>>>> > be >>>>>>>>>>>>>> > > added to the CompositeManagedBeanRegistry? I have >>>>>>>>>>>>>> opinions about this, >>>>>>>>>>>>>> > but >>>>>>>>>>>>>> > > I'd like to hear other's thoughts... >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > Note that ManagedBeanRegistry is a service and is >>>>>>>>>>>>>> initiated >>>>>>>>>>>>>> > > via >>>>>>>>>>>>>> org.hibernate.resource.cdi.spi.ManagedBeanRegistryInitiator. So it >>>>>>>>>>>>>> > > would be possible to completely redefine >>>>>>>>>>>>>> ManagedBeanRegistry support >>>>>>>>>>>>>> > simply >>>>>>>>>>>>>> > > by replacing that initiator. >>>>>>>>>>>>>> > > >>>>>>>>>>>>>> > > A minor point... notice that the package name here is >>>>>>>>>>>>>> > > `org.hibernate.resource.cdi`, even though one of the >>>>>>>>>>>>>> goals here is to >>>>>>>>>>>>>> > > support non-CDI ManagedBeanRegistry impls. Do we want >>>>>>>>>>>>>> to use a different >>>>>>>>>>>>>> > > package name? Maybe `org.hibernate.resource.beans`? >>>>>>>>>>>>>> > > ``org.hibernate.resource.di`? >>>>>>>>>>>>>> ``org.hibernate.resource.injection`? >>>>>>>>>>>>>> > > Other suggestions? I'm actually ok with >>>>>>>>>>>>>> `org.hibernate.resource.cdi` - >>>>>>>>>>>>>> > imo >>>>>>>>>>>>>> > > "cdi" conveys the proper intent. But if others feel >>>>>>>>>>>>>> strongly it should >>>>>>>>>>>>>> > be >>>>>>>>>>>>>> > > something else, I am open to hearing what and why. >>>>>>>>>>>>>> > > _______________________________________________ >>>>>>>>>>>>>> > > hibernate-dev mailing list >>>>>>>>>>>>>> > > hibernate-dev(a)lists.jboss.org >>>>>>>>>>>>>> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > _______________________________________________ >>>>>>>>>>>>>> > hibernate-dev mailing list >>>>>>>>>>>>>> > hibernate-dev(a)lists.jboss.org >>>>>>>>>>>>>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>>>>>>>>>>> > >>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>> hibernate-dev mailing list >>>>>>>>>>>>>> hibernate-dev(a)lists.jboss.org >>>>>>>>>>>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>> >>>