OpenLiberty / liberty-arquillian

Arquillian Liberty Managed and Remote containers
Apache License 2.0
10 stars 29 forks source link

Can not inject EntityManager #134

Open hantsy opened 1 year ago

hantsy commented 1 year ago

I am preparing a new template project for Jakarta EE 10, https://github.com/hantsy/jakartaee10-starter-boilerplate

I tried to add tests for CDI repository and EJB stateless bean. https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/src/test/java/com/example/it/CdiTodoRepositoryTest.java

@ExtendWith(ArquillianExtension.class)
public class CdiTodoRepositoryTest {
    private final static Logger LOGGER = Logger.getLogger(CdiTodoRepositoryTest.class.getName());

    @Deployment
    public static WebArchive createDeployment() {
        return ShrinkWrap.create(WebArchive.class, "test.war")
                .addPackage(Todo.class.getPackage())
                .addClasses(CdiTodoRepository.class, CrudRepository.class)
                .addClass(DbUtil.class)
                .addAsResource("test-persistence.xml", "META-INF/persistence.xml")
                .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml");
    }

    @Inject
    CdiTodoRepository todos;

    @PersistenceContext
    EntityManager entityManager;

    @Resource(lookup = "java:comp/DefaultDataSource")
    DataSource dataSource;

    @Inject
    UserTransaction utx;

    private DbUtil dbUtil;

    @BeforeEach()
    public void setup() throws Exception {
        utx.begin();
        dbUtil = new DbUtil(dataSource);
        dbUtil.clearTables();
        utx.commit();
    }

    @Test
    public void testNewTodo() throws Exception {
        utx.begin();
        LOGGER.log(Level.INFO, "new todo ... ");
        Todo todo = Todo.of("test");
        var saved = todos.save(todo);
        utx.commit();

        dbUtil.assertCount("todos", 1);
        var getById = entityManager.find(Todo.class, saved.getId());
        assertNotNull(getById);
        assertEquals("test", saved.getTitle());
    }

}

The above entityManger.find throws a NPE. https://github.com/hantsy/jakartaee10-starter-boilerplate/actions/runs/5235222832/jobs/9451902412

Only OpenLiberty encountered this issue, check my Github actions run result.

hantsy commented 1 year ago

But I checked my Jakarta EE 8 version, https://github.com/hantsy/jakartaee8-starter-boilerplate similar codes worked well.

https://github.com/hantsy/jakartaee8-starter-boilerplate/blob/master/src/test/java/com/example/it/OrderDaoTest.java

hantsy commented 1 year ago

There are several posts on Stackoverflow reported this issue, https://stackoverflow.com/questions/66971101/entitymanager-is-null-in-my-test-class-but-working-in-my-service https://stackoverflow.com/questions/41553376/arquillian-on-was-remote-container-and-persistencecontext

Azquelt commented 1 year ago

Sounds entityManager is not injected. Arquillian does its own injection of resources, so that could be going wrong I guess.

I would want someone who knows JPA to check the setup is correct.

I think the relevant parts of the configuration are (in addition to the test class above): server.xml persistence.xml

Azquelt commented 1 year ago

Looked some more and noted that todos.save(todo); does not throw an NPE, showing that the injection of EntityManager into CdiTodoRepository did work correctly, so the JPA config is probably fine and the problem is just with arquillian injecting the EntityManager into the test class.

Azquelt commented 1 year ago

As a workaround you could try calling CdiTodoRepository.findById instead of entityManager.find in your test.

Azquelt commented 1 year ago

This code from Arquillian should run to do injection for the test class:

    protected void injectNonContextualInstance(BeanManager manager, Object instance) {
        CreationalContext<Object> creationalContext = getCreationalContext();
        InjectionTarget<Object> injectionTarget = (InjectionTarget<Object>) manager
                .getInjectionTargetFactory(manager.createAnnotatedType(instance.getClass()))
                .createInjectionTarget(null);
        injectionTarget.inject(instance, creationalContext);
    }

Where manager is the bean manager looked up from OSGi and instance is the test class instance (i.e. CdiTodoRepositoryTest)

I would want to look to see why that isn't working when the regular injection of CdiTodoRepository appears to be fine.

Edit: updated to link to the up to date version from the arquillian repository.

hantsy commented 1 year ago

The EJB version EjbTodoRepositoryTest also failed. https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/src/test/java/com/example/it/EjbTodoRepositoryTest.java

And Only OpenLiberty failed, but the Jakarta EE 8 version worked well, https://github.com/hantsy/jakartaee8-starter-boilerplate/blob/master/src/test/java/com/example/it/OrderDaoTest.java (check the project build status), I am not sure where is wrong in the liberty arquillian when upgrading to Jakarta namespace.

And for this same tests, both GlassFish and WildFly worked well.

Checked the build status of this project. https://github.com/hantsy/jakartaee10-starter-boilerplate

Azquelt commented 1 year ago

The EJB version is doing the same thing, using @PersistenceContext to inject an EntityManager into the test class.

Even though it's an EJB test, it would still be the CDIInjectionEnricher which handles that field in the test class.

Azquelt commented 1 year ago

I am not sure where is wrong in the liberty arquillian when upgrading to Jakarta namespace.

Yeah, it is odd. I'm not sure either.

There is a separate version of CDIInjectionEnricher for jakarta classes but the code is the same and it must be working somewhat otherwise the @Inject CdiTodoRepository would also be broken.

hantsy commented 1 year ago

From the stackoverflow issues, it seems the issue was occurred when liberty arquillian is upgraded to EE9/Jakarta. It should be existed for years.

I also prepared a Jakarta EE 9 template project, but it did not contains a JPA test.

In the new Jakarta EE 10 template project, I try to add more missing examples to demo more specs in the Jakarta EE 8 version, and encountered this issue.

hantsy commented 1 year ago

Or there are some specific config in the server.xml for Jakarta EE9/10?

Azquelt commented 1 year ago

As a workaround you could try calling CdiTodoRepository.findById instead of entityManager.find in your test.

Could you confirm whether this works? It would help to confirm my theory as to where the problem is.

If it does work, then I'd be looking for a bug in the way CDI injects EE resources like EntityManager which only occurs when injecting a non-contextual instance and was introduced since EE9.

hantsy commented 1 year ago

The CdiTodoRepository.save is working well, so injecting CdiTodoRepository is 100% Ok.

Ok, I have updated the tests, add the tests as you expected.

Check the new updated https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/src/test/java/com/example/it/CdiTodoRepositoryTest.java

and build result: https://github.com/hantsy/jakartaee10-starter-boilerplate/actions/runs/5244915122/jobs/9471592175

hantsy commented 1 year ago

@Azquelt Any update of this issue?

Azquelt commented 1 year ago

Sorry, I thought I'd replied here.

I had a look into it and it seems like liberty is scanning for classes which should have EE resources injected (with @Resource, @PersistenceContext etc.)

The arquillian test class isn't one that we would generally inject into - it's not a CDI bean or an EE component - it looked like when the CDIInjectionEnricher runs, liberty looks for metadata for the injection point on the test class and doesn't find any because it wasn't scanned at startup.

I haven't been able to look at why you only see this problem with EE 9 onwards and not EE 10.

This issue is mostly impacting arquillian tests because the CDIInjectionEnricher is trying to do EE resource injection into things which aren't EE components.

I also couldn't find anything in the CDI or EE spec to suggest that this behaviour is allowed (though it is odd if it used to work but now doesn't).

Given that this is only impacting arquillian tests relying on behaviour which isn't required in the spec, I don't have enough time to continue looking at this at the moment.

tkburroughs commented 7 months ago

I have confirmed that the change in behavior is due to the CDI 4.0 specification breaking change related to this setting:

<cdi emptyBeansXmlCDI3Compatibility="true"/>

Setting this allows the Arquillian tests to pass.

What is happening is that Aqruillian calls the BeanManager to get the injection targets and inject into the test class. When working, the BeanManager calls into InjectionEngine to get targets for things like @Resoruce, @EJB, @PersistenceContext, etc. When not working, CDI does not use the InjectionEngine, and instead only handles the @Inject annotation.

When failing, @Resrouce and @EJB appear to work, but that is because Aqruillian has code to handle those separately. Arquillian just looks those objects up in JNDI and injects them directly; however, there is no Aqruillian code to handle @PersitenceContext.

I’m not sure that requiring the setting of the above property is the best solution; seems like there should be a better way to tell CDI to process the Arquillian test classes without reverting the CDI behavior for the entire server process.

Also, whatever is done, can it be done in a Liberty specific plugin to Arquillian?

hantsy commented 7 months ago

Just tried to add this to server.xml.

benjamin-confino commented 7 months ago

@tkburroughs

I think you've cracked the case.

Looking at the source code to liberty-arquillian I see several cases where we're explicitly creating an empty beans.xml file. Those lines of code predate EE9, so updating them to return a beans.xml file with a scanning mode of all should resolve this issue.

benjamin-confino commented 7 months ago

@hantsy Is there a way to look at the logs from the liberty server itself? I do not see then in the zip I can get from your URL.

(Running mvn clean verify -Parq-liberty-remote locally got weird and unrelated errors)

hantsy commented 7 months ago

OpenLiberty Remote Adapter server log: server-log.zip From this server log, there are several errors.

One is about the datasource, not sure where is wrong( but I remember at the moment I prepared the codes, it worked, the managed and remote profiles threw the same exception to complain the EntityManager injection).

I used the same config previous Jakarta EE 8, worked.

The server.xml file for this remote adapter: https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/src/test/arq-liberty-remote/server.xml#L87

Install driver is in the Github actions script: https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/.github/workflows/arq-liberty-remote.yml#L44

GitHub actions build log: logs_795.zip

hantsy commented 7 months ago

(Running mvn clean verify -Parq-liberty-remote locally got weird and unrelated errors)

I am not sure if current openliberty arquillian can work like this without a security settings.

In before experience, I have setup security and import the server side cert into client jvm, so you can see this: https://github.com/hantsy/jakartaee10-starter-boilerplate/blob/master/.github/workflows/arq-liberty-remote.yml#L51-L57

benjamin-confino commented 7 months ago

Thank you for that. Having looked at the logs I do not think this error is coming from CDI.

I'll have a closer look during work hours on Monday, as well as ensuring the liberty arqillian plugin is patched to remove the need for <cdi emptyBeansXmlCDI3Compatibility="true"/>

I also see that you have empty beans.xml files in this repository. I would recommend changing them to an explicit scanning mode to ensure consistency across all CDI versions. Where you have .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml"); change it to

import org.jboss.shrinkwrap.impl.BeansXml;
import org.jboss.shrinkwrap.api.BeanDiscoveryMode;

.addAsWebInfResource(new BeansXml(BeanDiscoveryMode.ALL), "beans.xml");

I suggest ALL because in older versions of CDI the default for an empty beans.xml is all, in CDI 4.0+ is it annotated. <cdi emptyBeansXmlCDI3Compatibility="true"/> just changes the default back to all.

hantsy commented 7 months ago

.addAsWebInfResource(new BeansXml(BeanDiscoveryMode.ALL), "beans.xml");

This will change the bean discovery mode in other containers.

If it is an issue from OpenLiberty or OpenLiberty Arquillian, I would like there is a workearound for OpenLiberty only.

CDI 4 changes annotated as default bean discovery mode, as one reason of preparing this small project, I also want to know if it is working in all containers.

benjamin-confino commented 7 months ago

<cdi emptyBeansXmlCDI3Compatibility="true"/> would be the OpenLiberty only workaround.

hantsy commented 7 months ago

The remote server config is fixed, I do not know why in the following config.

<library id="derbyJDBCLib">
    <fileset dir="${shared.resource.dir}" includes="derby*.jar"/>
</library>

It can not find the derby.jar in the resources folder, theoretically the * can match anything, length is 0 or more.

When I save the derby jar with a version as derby-10.14.2.0.jar, it worked.

cherylking commented 7 months ago

The remote server config is fixed, I do not know why in the following config.

<library id="derbyJDBCLib">
    <fileset dir="${shared.resource.dir}" includes="derby*.jar"/>
</library>

It can not find the derby.jar in the resources folder, theoretically the * can match anything, length is 0 or more.

When I save the derby jar with a version as derby-10.14.2.0.jar, it worked.

@hantsy Is this related to the original problem reported in this issue? Or is it a new separate issue? If the latter, you may want to open an issue on Open Liberty itself, as that is where it is determining what files match that includes wildcard.

hantsy commented 7 months ago

@cherylking filed an issue on OpenLiberty https://github.com/OpenLiberty/open-liberty/issues/27197.