From commits-return-49243-archive-asf-public=cust-asf.ponee.io@cxf.apache.org Fri May 25 01:03:24 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 29C81180636 for ; Fri, 25 May 2018 01:03:22 +0200 (CEST) Received: (qmail 98281 invoked by uid 500); 24 May 2018 23:03:22 -0000 Mailing-List: contact commits-help@cxf.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cxf.apache.org Delivered-To: mailing list commits@cxf.apache.org Received: (qmail 98272 invoked by uid 99); 24 May 2018 23:03:22 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 24 May 2018 23:03:22 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id A522582B17; Thu, 24 May 2018 23:03:21 +0000 (UTC) Date: Thu, 24 May 2018 23:03:21 +0000 To: "commits@cxf.apache.org" Subject: [cxf] branch master updated: CXF-7742 handling scopes in CdiResourceProvider (#418) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <152720300158.12025.5907120297531240371@gitbox.apache.org> From: reta@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: cxf X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: 77059523298743400e5099ba7ff535ed14120a31 X-Git-Newrev: 0da0a3883ad99fb5b14af806cba4becbf2f65774 X-Git-Rev: 0da0a3883ad99fb5b14af806cba4becbf2f65774 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. reta pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/cxf.git The following commit(s) were added to refs/heads/master by this push: new 0da0a38 CXF-7742 handling scopes in CdiResourceProvider (#418) 0da0a38 is described below commit 0da0a3883ad99fb5b14af806cba4becbf2f65774 Author: Romain Manni-Bucau AuthorDate: Fri May 25 01:03:17 2018 +0200 CXF-7742 handling scopes in CdiResourceProvider (#418) * CXF-7742 handling scopes in CdiResourceProvider * addressing comments + splitting singleton and per request providers, makes the code easier to follow and understand --- .../apache/cxf/cdi/JAXRSCdiResourceExtension.java | 49 +++++++- .../{CdiResourceProvider.java => Lifecycle.java} | 40 ++----- ...ovider.java => PerRequestResourceProvider.java} | 42 +++---- ...rovider.java => SingletonResourceProvider.java} | 35 ++---- .../apache/cxf/cdi/CdiResourceProviderTest.java | 125 +++++++++++++++++++++ 5 files changed, 207 insertions(+), 84 deletions(-) diff --git a/integration/cdi/src/main/java/org/apache/cxf/cdi/JAXRSCdiResourceExtension.java b/integration/cdi/src/main/java/org/apache/cxf/cdi/JAXRSCdiResourceExtension.java index d2b76db..9c5b48c 100644 --- a/integration/cdi/src/main/java/org/apache/cxf/cdi/JAXRSCdiResourceExtension.java +++ b/integration/cdi/src/main/java/org/apache/cxf/cdi/JAXRSCdiResourceExtension.java @@ -29,6 +29,7 @@ import java.util.Objects; import java.util.ServiceLoader; import java.util.Set; +import javax.enterprise.context.Dependent; import javax.enterprise.context.spi.CreationalContext; import javax.enterprise.event.Observes; import javax.enterprise.inject.spi.AfterBeanDiscovery; @@ -45,6 +46,7 @@ import javax.enterprise.inject.spi.ProcessBean; import javax.enterprise.inject.spi.ProcessProducerField; import javax.enterprise.inject.spi.ProcessProducerMethod; import javax.enterprise.inject.spi.WithAnnotations; +import javax.inject.Singleton; import javax.ws.rs.ApplicationPath; import javax.ws.rs.Path; import javax.ws.rs.core.Application; @@ -60,6 +62,7 @@ import org.apache.cxf.feature.Feature; import org.apache.cxf.jaxrs.JAXRSServerFactoryBean; import org.apache.cxf.jaxrs.ext.ContextClassProvider; import org.apache.cxf.jaxrs.ext.JAXRSServerFactoryCustomizationExtension; +import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; import org.apache.cxf.jaxrs.provider.ServerConfigurableFactory; import org.apache.cxf.jaxrs.utils.InjectionUtils; import org.apache.cxf.jaxrs.utils.JAXRSServerFactoryCustomizationUtils; @@ -82,6 +85,8 @@ public class JAXRSCdiResourceExtension implements Extension { private final List< Bean< ? extends Feature > > featureBeans = new ArrayList< Bean< ? extends Feature > >(); private final List< CreationalContext< ? > > disposableCreationalContexts = new ArrayList< CreationalContext< ? > >(); + private final List< Lifecycle > disposableLifecycles = + new ArrayList<>(); private final Set< Type > contextTypes = new LinkedHashSet<>(); private final Collection< String > existingStandardClasses = new HashSet<>(); @@ -93,7 +98,7 @@ public class JAXRSCdiResourceExtension implements Extension { private static class ClassifiedClasses { private List< Object > providers = new ArrayList<>(); private List< Feature > features = new ArrayList<>(); - private List< CdiResourceProvider > resourceProviders = new ArrayList<>(); + private List resourceProviders = new ArrayList<>(); public void addProviders(final Collection< Object > others) { this.providers.addAll(others); @@ -103,7 +108,7 @@ public class JAXRSCdiResourceExtension implements Extension { this.features.addAll(others); } - public void addResourceProvider(final CdiResourceProvider other) { + public void addResourceProvider(final ResourceProvider other) { this.resourceProviders.add(other); } @@ -115,7 +120,7 @@ public class JAXRSCdiResourceExtension implements Extension { return features; } - public List getResourceProviders() { + public List getResourceProviders() { return resourceProviders; } } @@ -303,7 +308,11 @@ public class JAXRSCdiResourceExtension implements Extension { for (final CreationalContext disposableCreationalContext: disposableCreationalContexts) { disposableCreationalContext.release(); } + disposableCreationalContexts.clear(); } + + disposableLifecycles.forEach(Lifecycle::destroy); + disposableLifecycles.clear(); } private Class toClass(String name) { @@ -347,7 +356,7 @@ public class JAXRSCdiResourceExtension implements Extension { instance.setProviders(classified.getProviders()); instance.getFeatures().addAll(classified.getFeatures()); - for (final CdiResourceProvider resourceProvider: classified.getResourceProviders()) { + for (final ResourceProvider resourceProvider: classified.getResourceProviders()) { instance.setResourceProvider(resourceProvider.getResourceClass(), resourceProvider); } @@ -372,7 +381,28 @@ public class JAXRSCdiResourceExtension implements Extension { for (final Bean< ? > bean: serviceBeans) { if (classes.contains(bean.getBeanClass())) { - classified.addResourceProvider(new CdiResourceProvider(beanManager, bean)); + // normal scoped beans will return us a proxy in getInstance so it is singletons for us, + // @Singleton is indeed a singleton + // @Dependent should be a request scoped instance but for backward compat we kept it a singleton + // + // other scopes are considered request scoped (for jaxrs) + // and are created per request (getInstance/releaseInstance) + final ResourceProvider resourceProvider; + if (isCxfSingleton(beanManager, bean)) { + final Lifecycle lifecycle = new Lifecycle(beanManager, bean); + resourceProvider = new SingletonResourceProvider(lifecycle, bean.getBeanClass()); + + // if not a singleton we manage it per request + // if @Singleton the container handles it + // so we only need this case here + if (Dependent.class == bean.getScope()) { + disposableLifecycles.add(lifecycle); + } + } else { + resourceProvider = new PerRequestResourceProvider( + () -> new Lifecycle(beanManager, bean), bean.getBeanClass()); + } + classified.addResourceProvider(resourceProvider); } } } @@ -380,6 +410,15 @@ public class JAXRSCdiResourceExtension implements Extension { return classified; } + boolean isCxfSingleton(final BeanManager beanManager, final Bean bean) { + return beanManager.isNormalScope(bean.getScope()) || isConsideredSingleton(bean.getScope()); + } + + // warn: several impls use @Dependent == request so we should probably add a flag + private boolean isConsideredSingleton(final Class scope) { + return Singleton.class == scope || Dependent.class == scope; + } + /** * Load external providers from service loader * @return loaded external providers diff --git a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java b/integration/cdi/src/main/java/org/apache/cxf/cdi/Lifecycle.java similarity index 59% copy from integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java copy to integration/cdi/src/main/java/org/apache/cxf/cdi/Lifecycle.java index 68d3b2b..089b25a 100644 --- a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java +++ b/integration/cdi/src/main/java/org/apache/cxf/cdi/Lifecycle.java @@ -22,46 +22,28 @@ import javax.enterprise.context.spi.CreationalContext; import javax.enterprise.inject.spi.Bean; import javax.enterprise.inject.spi.BeanManager; -import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; -import org.apache.cxf.message.Message; - -public class CdiResourceProvider implements ResourceProvider { - private Object instance; - private CreationalContext< ? > context; - +class Lifecycle { private final BeanManager beanManager; - private final Bean< ? > bean; + private final Bean bean; + private Object instance; + private CreationalContext context; - CdiResourceProvider(final BeanManager beanManager, final Bean< ? > bean) { + Lifecycle(final BeanManager beanManager, final Bean bean) { this.beanManager = beanManager; this.bean = bean; } - @Override - public Object getInstance(Message m) { - if (instance == null) { - context = beanManager.createCreationalContext(bean); - instance = beanManager.getReference(bean, bean.getBeanClass(), context); - } - + Object create() { + context = beanManager.createCreationalContext(bean); + instance = beanManager.getReference(bean, bean.getBeanClass(), context); return instance; } - @Override - public void releaseInstance(Message m, Object o) { + void destroy() { if (context != null) { context.release(); instance = null; + context = null; } } - - @Override - public Class getResourceClass() { - return bean.getBeanClass(); - } - - @Override - public boolean isSingleton() { - return !beanManager.isNormalScope(bean.getScope()); - } -} +} \ No newline at end of file diff --git a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java b/integration/cdi/src/main/java/org/apache/cxf/cdi/PerRequestResourceProvider.java similarity index 52% copy from integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java copy to integration/cdi/src/main/java/org/apache/cxf/cdi/PerRequestResourceProvider.java index 68d3b2b..127bf3e 100644 --- a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java +++ b/integration/cdi/src/main/java/org/apache/cxf/cdi/PerRequestResourceProvider.java @@ -18,50 +18,42 @@ */ package org.apache.cxf.cdi; -import javax.enterprise.context.spi.CreationalContext; -import javax.enterprise.inject.spi.Bean; -import javax.enterprise.inject.spi.BeanManager; +import java.util.function.Supplier; import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; import org.apache.cxf.message.Message; -public class CdiResourceProvider implements ResourceProvider { - private Object instance; - private CreationalContext< ? > context; +public class PerRequestResourceProvider implements ResourceProvider { + private final Supplier lifecycle; + private final Class clazz; - private final BeanManager beanManager; - private final Bean< ? > bean; - - CdiResourceProvider(final BeanManager beanManager, final Bean< ? > bean) { - this.beanManager = beanManager; - this.bean = bean; + PerRequestResourceProvider(final Supplier lifecycle, final Class clazz) { + this.lifecycle = lifecycle; + this.clazz = clazz; } @Override - public Object getInstance(Message m) { - if (instance == null) { - context = beanManager.createCreationalContext(bean); - instance = beanManager.getReference(bean, bean.getBeanClass(), context); - } - - return instance; + public Object getInstance(final Message m) { + final Lifecycle currentLifecycle = this.lifecycle.get(); + m.put(Lifecycle.class, currentLifecycle); + return currentLifecycle.create(); } @Override - public void releaseInstance(Message m, Object o) { - if (context != null) { - context.release(); - instance = null; + public void releaseInstance(final Message m, final Object o) { + final Lifecycle contextualLifecycle = m.get(Lifecycle.class); + if (contextualLifecycle != null) { + contextualLifecycle.destroy(); } } @Override public Class getResourceClass() { - return bean.getBeanClass(); + return clazz; } @Override public boolean isSingleton() { - return !beanManager.isNormalScope(bean.getScope()); + return false; } } diff --git a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java b/integration/cdi/src/main/java/org/apache/cxf/cdi/SingletonResourceProvider.java similarity index 54% rename from integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java rename to integration/cdi/src/main/java/org/apache/cxf/cdi/SingletonResourceProvider.java index 68d3b2b..8b7e02f 100644 --- a/integration/cdi/src/main/java/org/apache/cxf/cdi/CdiResourceProvider.java +++ b/integration/cdi/src/main/java/org/apache/cxf/cdi/SingletonResourceProvider.java @@ -18,50 +18,35 @@ */ package org.apache.cxf.cdi; -import javax.enterprise.context.spi.CreationalContext; -import javax.enterprise.inject.spi.Bean; -import javax.enterprise.inject.spi.BeanManager; - import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; import org.apache.cxf.message.Message; -public class CdiResourceProvider implements ResourceProvider { +public class SingletonResourceProvider implements ResourceProvider { + private final Class clazz; private Object instance; - private CreationalContext< ? > context; - - private final BeanManager beanManager; - private final Bean< ? > bean; - CdiResourceProvider(final BeanManager beanManager, final Bean< ? > bean) { - this.beanManager = beanManager; - this.bean = bean; + SingletonResourceProvider(final Lifecycle lifecycle, final Class clazz) { + this.instance = lifecycle.create(); + this.clazz = clazz; } @Override - public Object getInstance(Message m) { - if (instance == null) { - context = beanManager.createCreationalContext(bean); - instance = beanManager.getReference(bean, bean.getBeanClass(), context); - } - + public Object getInstance(final Message m) { return instance; } @Override - public void releaseInstance(Message m, Object o) { - if (context != null) { - context.release(); - instance = null; - } + public void releaseInstance(final Message m, final Object o) { + // no-op } @Override public Class getResourceClass() { - return bean.getBeanClass(); + return clazz; } @Override public boolean isSingleton() { - return !beanManager.isNormalScope(bean.getScope()); + return true; } } diff --git a/integration/cdi/src/test/java/org/apache/cxf/cdi/CdiResourceProviderTest.java b/integration/cdi/src/test/java/org/apache/cxf/cdi/CdiResourceProviderTest.java new file mode 100644 index 0000000..3c47198 --- /dev/null +++ b/integration/cdi/src/test/java/org/apache/cxf/cdi/CdiResourceProviderTest.java @@ -0,0 +1,125 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cxf.cdi; + +import java.lang.annotation.Documented; + +import javax.enterprise.context.ApplicationScoped; +import javax.enterprise.context.NormalScope; +import javax.enterprise.context.RequestScoped; +import javax.enterprise.inject.spi.Bean; +import javax.enterprise.inject.spi.BeanManager; +import javax.inject.Singleton; + +import org.apache.cxf.jaxrs.lifecycle.ResourceProvider; +import org.apache.cxf.message.MessageImpl; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.when; + +@SuppressWarnings({"rawtypes", "unchecked"}) +@RunWith(MockitoJUnitRunner.class) +public class CdiResourceProviderTest { + @Mock + private BeanManager beanManager; + + @Mock + private Bean bean; + + @Before + public void setup() { + final Class beanClass = Object.class; + when(bean.getBeanClass()).thenReturn(beanClass); + when(beanManager.isNormalScope(any())).thenAnswer(invocationOnMock -> + Class.class.cast(invocationOnMock.getArguments()[0]).isAnnotationPresent(NormalScope.class)); + when(beanManager.getReference(eq(bean), eq(beanClass), any())) + .thenAnswer(i -> new Object()); // ensure to create one instance per call, this is what we test + } + + @Test + public void normalScoped() { + when(bean.getScope()).thenReturn(Class.class.cast(ApplicationScoped.class)); + assertSingleton(); + } + + @Test + public void singleton() { + when(bean.getScope()).thenReturn(Class.class.cast(Singleton.class)); + assertSingleton(); + } + + @Test + public void dependent() { + when(bean.getScope()).thenReturn(Class.class.cast(Singleton.class)); + assertSingleton(); + } + + @Test + public void requestScoped() { + when(bean.getScope()).thenReturn(Class.class.cast(RequestScoped.class)); + assertSingleton(); // yes, this is a singleton since we look up the proxy + } + + @Test + public void perRequest() { + // not a scope so will be considered as a not singleton bean + when(bean.getScope()).thenReturn(Class.class.cast(Documented.class)); + assertNotSingleton(); + } + + private void assertNotSingleton() { + final ResourceProvider provider = new PerRequestResourceProvider( + () -> new Lifecycle(beanManager, bean), Object.class); + assertFalse(new JAXRSCdiResourceExtension().isCxfSingleton(beanManager, bean)); + assertFalse(provider.isSingleton()); + + final MessageImpl message1 = new MessageImpl(); + final MessageImpl message2 = new MessageImpl(); + final Object instance = provider.getInstance(message1); + assertNotNull(instance); + assertNotEquals(provider.getInstance(message1), provider.getInstance(message2)); + + // ensure we can close the lifecycle + final Lifecycle lifecycle1 = message1.get(Lifecycle.class); + assertNotNull(lifecycle1); + assertNotNull(message2.get(Lifecycle.class)); + } + + private void assertSingleton() { + final ResourceProvider provider = new SingletonResourceProvider(new Lifecycle(beanManager, bean), Object.class); + assertTrue(new JAXRSCdiResourceExtension().isCxfSingleton(beanManager, bean)); + assertTrue(provider.isSingleton()); + + final Object instance = provider.getInstance(new MessageImpl()); + assertNotNull(instance); + assertEquals(instance, provider.getInstance(new MessageImpl())); + } +} -- To stop receiving notification emails like this one, please contact reta@apache.org.