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.

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

Can not inject EntityManager #134

Open
hantsy opened this issue Jun 10, 2023 · 27 comments
Open

Can not inject EntityManager #134

hantsy opened this issue Jun 10, 2023 · 27 comments

Comments

@hantsy
Copy link

hantsy commented Jun 10, 2023

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

  • Java 17
  • Jakarta EE 10
  • OpenLiberty 23.0.0.5

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
Copy link
Author

hantsy commented Jun 10, 2023

@hantsy
Copy link
Author

hantsy commented Jun 12, 2023

@Azquelt
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Member

Azquelt commented Jun 12, 2023

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

@Azquelt
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Author

hantsy commented Jun 12, 2023

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
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Author

hantsy commented Jun 12, 2023

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
Copy link
Author

hantsy commented Jun 12, 2023

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

@Azquelt
Copy link
Member

Azquelt commented Jun 12, 2023

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
Copy link
Author

hantsy commented Jun 12, 2023

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
Copy link
Author

hantsy commented Jul 7, 2023

@Azquelt Any update of this issue?

@Azquelt
Copy link
Member

Azquelt commented Jul 10, 2023

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
Copy link
Member

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
Copy link
Author

hantsy commented Dec 8, 2023

Just tried to add this to server.xml.

@benjamin-confino
Copy link
Member

@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
Copy link
Member

benjamin-confino commented Dec 8, 2023

@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
Copy link
Author

hantsy commented Dec 9, 2023

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
Copy link
Author

hantsy commented Dec 9, 2023

(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
Copy link
Member

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
Copy link
Author

hantsy commented Dec 9, 2023

.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
Copy link
Member

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

@hantsy
Copy link
Author

hantsy commented Dec 15, 2023

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
Copy link
Member

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
Copy link
Author

hantsy commented Dec 16, 2023

@cherylking filed an issue on OpenLiberty OpenLiberty/open-liberty#27197.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants