[GitHub] [cxf] andymc12 opened a new pull request #619: CXF-8188: Inject into ClientHeadersFactory

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 opened a new pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
andymc12 opened a new pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619
 
 
   Implements [CXF-8188](https://issues.apache.org/jira/browse/CXF-8188) - injecting `@Context` and `@Inject` into custom `ClientHeadersFactory` implementations.  
   
   This tests JAX-RS injection.  The MP Rest Client 2.0 TCK will test CDI injection.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
reta commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r363103058
 
 

 ##########
 File path: systests/microprofile/client/jaxrs/src/test/java/org/apache/cxf/systest/microprofile/rest/client/InjectClientHeadersFactory.java
 ##########
 @@ -0,0 +1,42 @@
+/**
+ * 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.systest.microprofile.rest.client;
+
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MultivaluedHashMap;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Request;
+
+import org.eclipse.microprofile.rest.client.ext.ClientHeadersFactory;
+
+public class InjectClientHeadersFactory implements ClientHeadersFactory {
+
+    @Context
+    Request jaxrsRequest;
+
+    @Override
+    public MultivaluedMap<String, String> update(MultivaluedMap<String, String> incomingHeaders,
+                                                 MultivaluedMap<String, String> clientOutgoingHeaders) {
+
+        System.out.println("InjectClientHeadersFactory update");
 
 Review comment:
   Debug leftovers?  ;-)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-570926673
 
 
   @andymc12 please correct me if I am wrong, if we consider CDI-based deployments, the instances of the `MicroProfileClientProxyImpl` (and others) should be created as CDI beans, right?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r363126553
 
 

 ##########
 File path: systests/microprofile/client/jaxrs/src/test/java/org/apache/cxf/systest/microprofile/rest/client/InjectClientHeadersFactory.java
 ##########
 @@ -0,0 +1,42 @@
+/**
+ * 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.systest.microprofile.rest.client;
+
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MultivaluedHashMap;
+import javax.ws.rs.core.MultivaluedMap;
+import javax.ws.rs.core.Request;
+
+import org.eclipse.microprofile.rest.client.ext.ClientHeadersFactory;
+
+public class InjectClientHeadersFactory implements ClientHeadersFactory {
+
+    @Context
+    Request jaxrsRequest;
+
+    @Override
+    public MultivaluedMap<String, String> update(MultivaluedMap<String, String> incomingHeaders,
+                                                 MultivaluedMap<String, String> clientOutgoingHeaders) {
+
+        System.out.println("InjectClientHeadersFactory update");
 
 Review comment:
   Good catch - I'll remove that.  Thanks!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-570961902
 
 
   > if we consider CDI-based deployments, the instances of the MicroProfileClientProxyImpl (and others) should be created as CDI beans, right?
   
   @reta yes, it is done indirectly by creating the client proxy instance using the `CxfTypeSafeClientBuilder` from the `RestClientBean` / `RestClientExtension`.  
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-570975732
 
 
   Excellent, thanks @andymc12 , so here is my follow up question / concern: why won't we make `CxfTypeSafeClientBuilder` CDI-aware (using extension or alike) so the proxies are going to be managed by `BeanManager` and as such, could use `@Inject` annotation directly? In this case, the integration should work in all scenarios (managed, standalone, ...). Does it make sense?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-571243451
 
 
   @reta I think CDI is already managing the proxy instances.  The extension manages the creation of the proxy (it has to provide additional config via the MP annotations, like `@RegisterProvider`, `@RegisterClientHeaders`, etc.), but once it is created, then it turns the proxy over to CDI to manage.
   
   The problem with the ClientHeadersFactory instance is that it is associated with the client interface proxy by annotation, not injected.  The other problem is that we want the ClientHeadersFactory to be injected with any JAX-RS contextual info too.  
   
   Does that makes sense?  Thanks again for your review!
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-573453118
 
 
   hey @andymc12, sorry for the delay, finally found the time to finish the review:
    - `CDIUtils` and `CDIFacade`: we already use reflection-base `BeanManager` detection in `CDIInterceptorWrapper::createWrapper`, it think we should stick to one way to do it
    - may be `MicroProfileClientFactoryBean` is a better place for dealing with CDI & `ClientHeadersFactory`, which could be passed to `MicroProfileClientProxyImpl` in a similar way as `CDIInterceptorWrapper`. The `BeanManager` detection logic (whichever we pick), could be reused for both of them.
   
   What do you think?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r365608073
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
 
 Review comment:
   this is a slow operation, it should likely not be done at runtime but the BM should be bound (at least boundable) to CXF bus as an extension probably

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r365608102
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   if !beanManager.isNormalScope(bean.getScope()) then ctx must be released to not leak

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r366075124
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
 
 Review comment:
   in some cases this is called before the bus is created, but when the bus is available, my latest change will use the BeanManager associated with the bus before calling `CDI.current().getBeanManager()`.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r366075555
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   @rmannibucau That makes sense.  When does the ctx need to be released?  Can I release it as soon as I have a reference to the bean, or do I need to wait until it is out of scope?  Thanks for the review!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-573916489
 
 
   @reta
   > CDIUtils and CDIFacade: we already use reflection-base BeanManager detection in CDIInterceptorWrapper::createWrapper, it think we should stick to one way to do it
   
   Agreed.  I updated the CDIInterceptorWrapper to use CDIFacade/CDIUtils.
   
   > may be MicroProfileClientFactoryBean is a better place for dealing with CDI & ClientHeadersFactory, which could be passed to MicroProfileClientProxyImpl in a similar way as CDIInterceptorWrapper. The BeanManager detection logic (whichever we pick), could be reused for both of them.
   
   My latest changes reuse the BeanManager detection logic.  I'm not sure if moving the ClientHeadersFactory logic into MicroProfileClientFactoryBean will work since that is when the client proxy is being created.  It's possible the the rest client instance is created in one JAX-RS context (or outside of a JAX-RS request entirely) and then used in a different JAX-RS context.  So then the injection would need to occur on each request, right?
   
   Thanks again!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
reta commented on issue #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#issuecomment-573945176
 
 
   Thanks @andymc12 , regarding
   
   > My latest changes reuse the BeanManager detection logic. I'm not sure if moving the ClientHeadersFactory logic into MicroProfileClientFactoryBean will work since that is when the client proxy is being created. It's possible the the rest client instance is created in one JAX-RS context (or outside of a JAX-RS request entirely) and then used in a different JAX-RS context. So then the injection would need to occur on each request, right?
   
   you are right, it might happen, but probably the same would apply to `CDIInterceptorWrapper`: it will pick up the interceptors from the different context. I don't know what is the best option to be fair, just an opinion if we have a single place to make the decisions (the factory bean), it would be much easier to reason about the outcome (proxy) and maintain it. The uncertainty with contexts may cause issues but I would leave the decision to you, I think you have a better view of the picture.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r366161319
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   It will trigger dependent predestroys so out of scope I think.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r366614181
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   I added a new commit that will release the contexts (for all scopes) when the client instance is closed.  Can you take one more peek at my changes?  If all looks good, I think we can merge.  Thanks again!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r366706385
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   Works since there is one factory per bus
   Side note being: why cdi utility does not return an Instance{value,runnable} and you use the runnable as a close callback to call the release method. Main difference is then the storage can be an instance list (not set ;)) in the factory instead of a static method which would leak if anything happens wrong in the app lifecycle.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r367073516
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   I like it!  I've just made those changes.  One last (hopefully) look?  Thanks again!

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
rmannibucau commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r367076744
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   Second param should be sthg like:
   
        beanManager.isNormalScope(bean.getScope()) ? () -> {} : context::release
   
   Normal scope lifecycle is already handled by the container.
   
   Otherwise perfect for me.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory

GitBox
In reply to this post by GitBox
andymc12 commented on a change in pull request #619: CXF-8188: Inject into ClientHeadersFactory
URL: https://github.com/apache/cxf/pull/619#discussion_r367079729
 
 

 ##########
 File path: rt/rs/microprofile-client/src/main/java/org/apache/cxf/microprofile/client/cdi/CDIUtils.java
 ##########
 @@ -0,0 +1,56 @@
+/**
+ * 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.microprofile.client.cdi;
+
+import java.util.NoSuchElementException;
+
+import javax.enterprise.context.spi.CreationalContext;
+import javax.enterprise.inject.spi.Bean;
+import javax.enterprise.inject.spi.BeanManager;
+import javax.enterprise.inject.spi.CDI;
+
+public final class CDIUtils {
+
+    private CDIUtils() {
+    }
+
+    public static <T> T getInstanceFromCDI(Class<T> clazz) {
+        T t;
+        try {
+            t = findBean(clazz);
+        } catch (ExceptionInInitializerError | NoClassDefFoundError | IllegalStateException ex) {
+            // expected if no CDI implementation is available
+            t = null;
+        } catch (NoSuchElementException ex) {
+            // expected if ClientHeadersFactory is not managed by CDI
+            t = null;
+        }
+        return t;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static <T> T findBean(Class<T> clazz) {
+        BeanManager beanManager = CDI.current().getBeanManager();
+        Bean<?> bean = beanManager.getBeans(clazz).iterator().next();
+        CreationalContext<?> ctx = beanManager.createCreationalContext(bean);
 
 Review comment:
   Shouldn't it be the other way around?   We should call release if it _is_ a normal scope, right?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[hidden email]


With regards,
Apache Git Services
12