jakartaee / data

Data-API
Apache License 2.0
93 stars 26 forks source link

[TCK Challenge]: ExtensionTests #709

Closed starksm64 closed 5 months ago

starksm64 commented 5 months ago

Specification

https://github.com/jakartaee/data/blob/main/tck/src/main/java/ee/jakarta/tck/data/core/cdi/ExtensionTests.java

Assertion

CDI beans, custom entity annotation

TCK Version

SNAPSHOT (Locally built)

Implementation being tested

HIbernate / Weld

Challenge Scenario

Claims that a test is not portable or depends on a particular implementation.

Full Description

We are running into two problems with the ExtensionTests, both ee.jakarta.tck.data.core.cdi and ee.jakarta.tck.data.web.cdi. The first issue is that the ee.jakarta.tck.data.common.cdi.Directory has a custom provider and so Hibernate does not process it to generate an implementation that is a CDI bean. Instead there is a ee.jakarta.tck.data.common.cdi.DirectoryRepository implementation, but it does not have a bean defining annotation, and so is not picked up for processing by the BuildCompatibleExtensionImpl. When Hibernate generates a repository bean it annotates it with a @RequestScoped annotation. The DirectoryRepository should have a bean defining annotation or a beans.xml descriptor that identifies this as a CDI bean.

The second problem relates to the ee.jakarta.tck.data.common.cdi.AddressBook repository interface. This is using a custom EntityDefining annotation, and Hibernate does not generate a repository bean for this as expected. However, the ee.jakarta.tck.data.common.cdi.AddressRepository is also missing a bean defining annotation.

If I add @RequestScoped annotations to both repository beans, we see all tests passing.

Additional Context

No response

Is there an existing challenge for this?

gavinking commented 5 months ago

So I spent like 2 hours trying to understand all this last night, and it's all still very murky to me.

Apparently, there are two competing ways that these beans are supposed to get registered, both working via a CDI extension. Either:

  1. by ExtensionImpl registering a DirectoryRepositoryProducer which creates a DirectoryRepository, or
  2. by BuildCompatibleExtensionImpl registering the DirectoryRepository bean directly.

But as a special twist, neither of the extensions just registers the bean. Instead, the extensions want to be notified of the existence of the repository interface via a ProcessAnnotatedType event, or via a @Enhancement callback. I can only guess at the motivation behind this: I speculate that what it's doing is attempting to simulate how the OpenLiberty implementation of Jakarta Data works.

But this is a maximally-overcomplicated way to use CDI to perform the required task; as Scott says, the same total effect may be obtained by sticking a bean-defining annotation on DirectoryRepository and AddressRepository. No CDI extension is necessary.

So, anyway, I spent time trying to figure out why this is not working for us when we run these tests. I see that the extensions (both) get created, and called, but they're never notified of the existence of the Directory or AddressBook interfaces, and so they don't register the DirectoryRepository or AddressRepository implementations. So it seems that the ShrinkWrap-created WAR is not being properly scanned.

This is well beyond my knowledge of Arquillian and ShrinkWrap to understand. But whatever is going wrong here has for sure absolutely nothing to do with our Jakarta Data implementation, and it seems to me that the TCK has no good motivation to do things in such a complicated way.

njr-11 commented 5 months ago

I believe these parts of the extension tests were copied from some Open Liberty tests that attempted to simulate some ways we thought a NoSQL provider might try to plug in via CDI (at least at that point in time of the spec), as part of an effort to help ensure there would be no collisions. This approach for testing does seem to be unnecessarily complicated and it should be fine to simplify it to what is suggested here. That should still cover avoiding conflicts.

gavinking commented 5 months ago

Excellent 🙏🙏