Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

ganeshs · 2018-04-24T18:35:24Z

This is the fix for pac4j/dropwizard-pac4j#43

…ctoryProvider

victornoel · 2018-04-25T11:35:42Z

@ganeshs FYI I won't be able to review this right now, but as soon as I can I will experiment with it.

Could you explain briefly why you had to change things for resteasy, what are the new dependencies for and if it means that resteasy users need a special setup for pac4j to work now?

Thanks for your help and patience :)

ganeshs · 2018-04-25T12:53:56Z

The change is to inject a javax.inject.provider instead httpservletrequest. For the resteasy module, I have modified the tests to use cdi. This means, the resteasy users will have to use cdi for injecting the provider. I have added a test that would act as an example for cdi integration.

ganeshs · 2018-05-05T03:26:00Z

@victornoel any update on this

victornoel

Ok, I've reviewed it.
I'm not totally convinced this is the right solution for pac4j/dropwizard-pac4j#43 though... this seem to add more complexity to the already complex situation.

Until you fix this, I will try to better understand why it happens, it would be very helpful to have a reproduction of the original problem in dropwizard-pac4j :) if we can't find something more elegant then we will go with this.

victornoel · 2018-05-05T15:41:51Z

core/src/main/java/org/pac4j/jax/rs/servlet/features/ServletJaxRsContextFactoryProvider.java

@@ -15,13 +16,14 @@
 * @since 1.0.0
 *
 */
+@Provider


I would prefer not to use @Provider.

In the past we used something like that but the problem was that depending on the use, it was problematic so we decided to let user explicitly define their own Provider class (see https://github.com/pac4j/jax-rs-pac4j/#jax-rs-autoscanning).

If I'm right, this shouldn't change anything for this PR since you register manually the provider below but maybe some more configuration will be needed in the tests?

victornoel · 2018-05-05T15:42:19Z

resteasy/pom.xml

@@ -22,6 +22,7 @@
    </dependencies>
  </dependencyManagement>

+


remove this extra line

victornoel · 2018-05-05T15:43:22Z

resteasy/pom.xml

+      <groupId>org.jboss.resteasy</groupId>
+      <artifactId>resteasy-cdi</artifactId>
+      <scope>test</scope>
+      <version>3.1.4.Final</version>


Could you factor this version number and the same one in dependencyManagement into a property named resteasy.version?

victornoel · 2018-05-05T15:46:06Z

resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java

-    public class MyApp extends Application {
+    public static class MyApp extends Application {
+
+        private RestEasyUndertowServletRule rule = new RestEasyUndertowServletRule(); 


This doesn't seem coherent to have the rule here just to call getResources() on it!

I see you made the class static, but it is not needed at all. Please revert that as well as the calls to getResources() and getConfig() below.

victornoel · 2018-05-05T16:08:04Z

core/src/main/resources/META-INF/beans.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>


Could you move this with the test application? To me it has nothing to do here, it is only needed for resteasy with cdi, no? Or explain why it is needed here please

As per the CDI specification to discover beans from the archive, we need to have beans.xml under META-INF folder in the archive. I have spent enough time in figuring out a workaround for this but no success yet.

Bean classes of enabled beans must be deployed in bean archives.
A library jar, EJB jar, application client jar or rar archive is a bean archive if it has a file named beans.xml in the META-INF directory.
https://docs.jboss.org/cdi/spec/1.0/html/packagingdeployment.html

In this case, the ServletJaxRsContextFactoryProvider has an @Inject on requestProvider and without the beans.xml in its jar, the requestProvider is not getting injected.

@victornoel I would be happy to fix if a solution is known. I don't have enough bandwidth to search for a solution right now. I completely agree with you that this is an unnecessary addition but definitely is harmless. Would really appreciate if we can go ahead for now and fix when a solution is available.

Ok, I agree, thank you for the explanation :)
Could you just add some comment in that file explaining why it is here exactly and maybe saying it is mainly because of resteasy/cdi?

victornoel · 2018-05-05T16:12:32Z

resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java

+        ResteasyDeployment deployment = new ResteasyDeployment();
+        deployment.setInjectorFactoryClass("org.jboss.resteasy.cdi.CdiInjectorFactory");
+        deployment.setApplication(new MyApp());
+        DeploymentInfo di = server.undertowDeployment(deployment).setContextPath("/").setDeploymentName("DI")


improve indentation (one call per line)

victornoel · 2018-05-05T16:12:43Z

resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java

@@ -66,7 +72,13 @@ protected void before() throws Throwable {
        System.setProperty("org.jboss.resteasy.port", "24257");
        server = new UndertowJaxrsServer().start();

-        server.deploy(new MyApp());
+        ResteasyDeployment deployment = new ResteasyDeployment();
+        deployment.setInjectorFactoryClass("org.jboss.resteasy.cdi.CdiInjectorFactory");


prefer to use CdiInjectorFactory.class.getName() instead

victornoel · 2018-05-05T16:17:46Z

resteasy/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>javax</groupId>
+      <artifactId>javaee-api</artifactId>


what is this for?

victornoel · 2018-05-05T16:18:36Z

resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java

+        deployment.setInjectorFactoryClass("org.jboss.resteasy.cdi.CdiInjectorFactory");
+        deployment.setApplication(new MyApp());
+        DeploymentInfo di = server.undertowDeployment(deployment).setContextPath("/").setDeploymentName("DI")
+			.setClassLoader(RestEasyUndertowServletRule.class.getClassLoader())


prefer to use getClass().getClassLoader()

victornoel · 2018-05-05T16:19:33Z

resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java

+        deployment.setApplication(new MyApp());
+        DeploymentInfo di = server.undertowDeployment(deployment).setContextPath("/").setDeploymentName("DI")
+			.setClassLoader(RestEasyUndertowServletRule.class.getClassLoader())
+			.addListeners(Servlets.listener(org.jboss.weld.environment.servlet.Listener.class));


do not use fqdn if possible, it is better to have all imports in import section so we can quickly see the dependencies of the class.

victornoel

We are almost good, thanks for your patience :)

Could you also:

refer to ServletJaxRsContextFactoryProvider by class instead of instance also in jersey's implementations
add some words in README about:
- the need to refer to those classes (and others maybe?) by class and not instance when initializing the jax-rs environment (or simply just update the examples)
- the need to use cdi (just pointing to the test will be enough) for resteasy

Thanks again :)

victornoel · 2018-05-13T09:58:39Z

core/src/main/resources/META-INF/beans.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>


Ok, I agree, thank you for the explanation :)
Could you just add some comment in that file explaining why it is here exactly and maybe saying it is mainly because of resteasy/cdi?

victornoel · 2018-05-13T09:59:26Z

core/src/main/java/org/pac4j/jax/rs/servlet/features/ServletJaxRsContextFactoryProvider.java

-    @Context
-    private HttpServletRequest request;
+    @Inject
+    private javax.inject.Provider<HttpServletRequest> requestProvider;


You can now use an import instead :)

ganeshs · 2018-05-14T04:37:39Z

@victornoel Addressed the changes requested.

victornoel

we are almost there :)

victornoel · 2018-05-14T17:27:34Z

README.md

@@ -36,7 +36,7 @@ These filters can be directly registered by hand, or instead, the following feat
 5) Container/Implementation-specific Providers and Features extend the basic functionality provided by the generic ones

 - The `Pac4JValueFactoryProvider` enables injection of the security profile in resource method (for Apache Jersey <2.26, see [#30](https://github.com/pac4j/jax-rs-pac4j/issues/30))
- The `ServletJaxRsContextFactoryProvider` provides session handling (and thus indirect clients support) by replacing the generic `JaxRsContextFactoryProvider` (for Servlet-based JAX-RS implementations, e.g., Jersey on Netty or Grizzly Servlet, Resteasy on Undertow).
+- The `ServletJaxRsContextFactoryProvider` provides session handling (and thus indirect clients support) by replacing the generic `JaxRsContextFactoryProvider` (for Servlet-based JAX-RS implementations, e.g., Jersey on Netty or Grizzly Servlet, Resteasy on Undertow). This provider returns a RequestScoped context and so should be added as a class `ServletJaxRsContextFactoryProvider.class` instead of instance `new ServletJaxRsContextFactoryProvider()`. For RestEasy, you need to include `resteasy-cdi` support to your project. Refer to the tests in `resteasy-pac4j` module.


Sorry, but that's not the right place where to do that.
This sentence should be kept as it is and the examples below should be updated as well as a little line of explanation about resteasy-cdi next to the section related to resteasy.

Done. Hope this works.

victornoel

ok, last one, there is an error in the example (and a typo)

victornoel · 2018-05-14T18:19:52Z

README.md

+        @Override
+        public Set<Class<?>> getClasses() {
+            Set<Class<?>> classes = new HashSet<>();
+            classes.add(ServletJaxRsContextFactoryProvider.class);


Pac4JProfileInjectorFactory needs to be moved too, I tested and it doesn't work without it

victornoel · 2018-05-14T18:20:16Z

README.md

@@ -50,7 +50,7 @@ You need to add a dependency on:

 - jax-rs-pac4j
  1. for Jersey (<2.26) : the `jersey-pac4j` library (<em>groupId</em>: **org.pac4j**, *version*: **3.0.0**)
-  2. for Resteasy : the `resteasy-pac4j` library (<em>groupId</em>: **org.pac4j**, *version*: **3.0.0**)
+  2. for Resteasy : the `resteasy-pac4j` library (<em>groupId</em>: **org.pac4j**, *version*: **3.0.0**) and resteasy-cdi for CDI support


add backquotes around resteasy-cdi please

victornoel · 2018-05-14T18:43:53Z

Thanks @ganeshs, great work!

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider

victornoel · 2018-05-19T09:21:06Z

This was backported to 2.x too :)

Using provider instead of HttpServletRequest in ServletJaxRsContextFa…

c1b8c80

…ctoryProvider

ganeshs mentioned this pull request Apr 24, 2018

Dropwizard Application doesn't start when used with Guice pac4j/dropwizard-pac4j#43

Closed

leleuj mentioned this pull request Apr 25, 2018

Publishing 3.0-RC1 pac4j/dropwizard-pac4j#40

Closed

ganeshs mentioned this pull request Apr 25, 2018

Pac4JProfileInjectorFactory disable CDI Injection Resteasy #34

Closed

victornoel requested changes May 5, 2018

View reviewed changes

ganesh s added 2 commits May 11, 2018 16:39

Fixed review comments

5275b6d

Fixed indentation

43141ff

victornoel requested changes May 13, 2018

View reviewed changes

Fixed review comments, added comments and updated README

36c2246

victornoel requested changes May 14, 2018

View reviewed changes

updated the readme

2d46d88

victornoel requested changes May 14, 2018

View reviewed changes

updated the readme

6c64dae

victornoel approved these changes May 14, 2018

View reviewed changes

victornoel merged commit d669e9f into pac4j:master May 14, 2018

victornoel added a commit that referenced this pull request May 19, 2018

Merge pull request #42 from ganeshs/master

9e95196

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

ganeshs commented Apr 24, 2018

victornoel commented Apr 25, 2018

ganeshs commented Apr 25, 2018

ganeshs commented May 5, 2018

victornoel left a comment

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

ganeshs May 11, 2018

victornoel May 13, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel May 5, 2018

victornoel left a comment

victornoel May 13, 2018

victornoel May 13, 2018

ganeshs commented May 14, 2018

victornoel left a comment

victornoel May 14, 2018

ganeshs May 14, 2018

victornoel left a comment

victornoel May 14, 2018

victornoel May 14, 2018

victornoel commented May 14, 2018

victornoel commented May 19, 2018

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

Conversation

ganeshs commented Apr 24, 2018

victornoel commented Apr 25, 2018

ganeshs commented Apr 25, 2018

ganeshs commented May 5, 2018

victornoel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshs commented May 14, 2018

victornoel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel commented May 14, 2018

victornoel commented May 19, 2018