Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

Merged
merged 6 commits into from
May 14, 2018
Merged

Using provider instead of HttpServletRequest in ServletJaxRsContextFactoryProvider #42

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1b8c80
Using provider instead of HttpServletRequest in ServletJaxRsContextFa…
Apr 24, 2018
5275b6d
Fixed review comments
May 11, 2018
43141ff
Fixed indentation
May 11, 2018
36c2246
Fixed review comments, added comments and updated README
May 14, 2018
2d46d88
updated the readme
May 14, 2018
6c64dae
updated the readme
May 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
<version>1.2</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>javax.inject</groupId>
<artifactId>javax.inject</artifactId>
<version>1</version>
</dependency>
<dependency>
<groupId>org.pac4j</groupId>
<artifactId>pac4j-core</artifactId>
Expand Down
10 changes: 6 additions & 4 deletions core/src/main/java/org/pac4j/jax/rs/servlet/features/ServletJaxRsContextFactoryProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package org.pac4j.jax.rs.servlet.features;

import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
import javax.ws.rs.core.Context;
import javax.ws.rs.ext.Provider;

import org.pac4j.jax.rs.features.JaxRsContextFactoryProvider;
import org.pac4j.jax.rs.servlet.pac4j.ServletJaxRsContext;
Expand All @@ -15,13 +16,14 @@
* @since 1.0.0
*
*/
@Provider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

public class ServletJaxRsContextFactoryProvider extends JaxRsContextFactoryProvider {

@Context
private HttpServletRequest request;
@Inject
private javax.inject.Provider<HttpServletRequest> requestProvider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can now use an import instead :)


@Override
public JaxRsContextFactory getContext(Class<?> type) {
return context -> new ServletJaxRsContext(getProviders(), context, getConfig().getSessionStore(), request);
return context -> new ServletJaxRsContext(getProviders(), context, getConfig().getSessionStore(), requestProvider.get());
}
}
9 changes: 9 additions & 0 deletions core/src/main/resources/META-INF/beans.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

<beans
xmlns="http://xmlns.jcp.org/xml/ns/javaee"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/beans_1_1.xsd"
bean-discovery-mode="all"
>

</beans>
19 changes: 19 additions & 0 deletions resteasy/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
</dependencies>
</dependencyManagement>


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this extra line

<dependencies>
<dependency>
<groupId>${project.parent.groupId}</groupId>
Expand All @@ -45,11 +46,29 @@
<artifactId>resteasy-undertow</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jboss.resteasy</groupId>
<artifactId>resteasy-cdi</artifactId>
<scope>test</scope>
<version>3.1.4.Final</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</dependency>
<dependency>
<groupId>javax</groupId>
<artifactId>javaee-api</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for?

<version>8.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.undertow</groupId>
<artifactId>undertow-servlet</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jboss.weld.servlet</groupId>
<artifactId>weld-servlet</artifactId>
<version>2.4.7.Final</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.glassfish.jersey.core</groupId>
<artifactId>jersey-client</artifactId>
Expand Down
24 changes: 18 additions & 6 deletions resteasy/src/test/java/org/pac4j/jax/rs/rules/RestEasyUndertowServletRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.awaitility.Duration;
import org.glassfish.jersey.client.JerseyClientBuilder;
import org.jboss.resteasy.plugins.server.undertow.UndertowJaxrsServer;
import org.jboss.resteasy.spi.ResteasyDeployment;
import org.jboss.resteasy.test.TestPortProvider;
import org.junit.rules.ExternalResource;
import org.pac4j.jax.rs.features.JaxRsConfigProvider;
Expand All @@ -23,26 +24,31 @@
import org.pac4j.jax.rs.servlet.features.ServletJaxRsContextFactoryProvider;

import io.undertow.server.session.SessionCookieConfig;
import io.undertow.servlet.Servlets;
import io.undertow.servlet.api.DeploymentInfo;

public class RestEasyUndertowServletRule extends ExternalResource implements SessionContainerRule {

private UndertowJaxrsServer server;

private Client client;

public class MyApp extends Application {
public static class MyApp extends Application {

private RestEasyUndertowServletRule rule = new RestEasyUndertowServletRule();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Override
public Set<Class<?>> getClasses() {
return getResources();
Set<Class<?>> classes = rule.getResources();
classes.add(ServletJaxRsContextFactoryProvider.class);
classes.add(Pac4JProfileInjectorFactory.class);
return classes;
}

@Override
public Set<Object> getSingletons() {
return Sets.newLinkedHashSet(
new JaxRsConfigProvider(getConfig()),
new ServletJaxRsContextFactoryProvider(),
new Pac4JProfileInjectorFactory(),
new JaxRsConfigProvider(rule.getConfig()),
new Pac4JSecurityFeature());
}
}
Expand All @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to use CdiInjectorFactory.class.getName() instead

deployment.setApplication(new MyApp());
DeploymentInfo di = server.undertowDeployment(deployment).setContextPath("/").setDeploymentName("DI")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

improve indentation (one call per line)

.setClassLoader(RestEasyUndertowServletRule.class.getClassLoader())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to use getClass().getClassLoader()

.addListeners(Servlets.listener(org.jboss.weld.environment.servlet.Listener.class));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

server.deploy(di);
}

@Override
Expand Down