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

Derived identity TCK tests rely on unspecified cascading behavior #632

Open
beikov opened this issue May 21, 2024 · 7 comments
Open

Derived identity TCK tests rely on unspecified cascading behavior #632

beikov opened this issue May 21, 2024 · 7 comments
Labels
accepted Accepted certification request challenge TCK challenge

Comments

@beikov
Copy link

beikov commented May 21, 2024

The following tests rely on unspecified cascading behavior of the @MapsId annotation:

  • ee.jakarta.tck.persistence.core.derivedid.ex1a
  • ee.jakarta.tck.persistence.core.derivedid.ex1b
  • ee.jakarta.tck.persistence.core.derivedid.ex2b
  • ee.jakarta.tck.persistence.core.derivedid.ex3a
  • ee.jakarta.tck.persistence.core.derivedid.ex3b
  • ee.jakarta.tck.persistence.core.annotations.mapsid

After fixing a Hibernate issue, we realized that the spec does not define this behavior and that assuming CascadeType.PERSIST for associations that are part of an entity id is not correct.

The mentioned tests essentially assume that persist cascading is applied. By reordering the persist calls in the test, this wrong assumption can be fixed. Interestingly, a few similar other tests e.g. ee.jakarta.tck.persistence.core.derivedid.ex4a and ee.jakarta.tck.persistence.core.derivedid.ex4b do not rely on such behavior, so maybe this behavior was assumed by accident?

@gavinking
Copy link
Contributor

The issue here is that an entity in the persistent state must have a well-defined persistent identity. Yes, right from the very moment that persist() is called.

But in these tests, that's not the case: the dependent objects do not have a well-defined persistent identity when they are made persistent, their persistent identity can only be determined after the Employees are made persistent.

One supposes that other JPA implementations, like Hibernate, have hacks around these TCK tests, but those hacks are not correct.

@gavinking gavinking added the challenge TCK challenge label May 22, 2024
@scottmarlow
Copy link
Contributor

@lukasj do you expect that we will try to make further Persistence 3.2 test changes like jakartaee/platform-tck#1309 which is for this challenge?

@beikov
Copy link
Author

beikov commented Jun 12, 2024

Please exclude the tests from the next 3.1 and 3.2 TCK builds.

@lukasj lukasj added the accepted Accepted certification request label Jun 14, 2024
beikov added a commit to beikov/jakartaee-tck that referenced this issue Jun 14, 2024
scottmarlow added a commit to jakartaee/platform-tck that referenced this issue Jun 19, 2024
Fix jakartaee/persistence#632, Don't assume persist cascading for associations part of the id
@scottmarlow
Copy link
Contributor

scottmarlow commented Jun 19, 2024

Impacted 3.1 tests (for EE 10 Platform TCK testing) would be:

com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_appmanaged
com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_pmservlet
com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_puservlet
com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_stateful3
com/sun/ts/tests/jpa/core/derivedid/ex1a/Client.java#DIDTest_from_stateless3
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_appmanaged
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_pmservlet
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_puservlet
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_stateful3
com/sun/ts/tests/jpa/core/derivedid/ex1b/Client.java#DIDTest_from_stateless3
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_appmanaged
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_pmservlet
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_puservlet
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_stateful3
com/sun/ts/tests/jpa/core/derivedid/ex2b/Client.java#DIDTest_from_stateless3
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_appmanaged
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_pmservlet
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_puservlet
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_stateful3
com/sun/ts/tests/jpa/core/derivedid/ex3a/Client.java#DIDTest_from_stateless3
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_appmanaged
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_pmservlet
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_puservlet
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_stateful3
com/sun/ts/tests/jpa/core/derivedid/ex3b/Client.java#DIDTest_from_stateless3

com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_appmanaged
com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_appmanagedNoTx
com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_pmservlet
com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_puservlet
com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_stateful3
com/sun/ts/tests/jpa/core/annotations/mapsid/Client.java#persistMX1Test1_from_stateless3

scottmarlow added a commit to scottmarlow/jakartaee-tck that referenced this issue Jun 19, 2024
…y on unspecified cascading behavior and increment the JPA TCK version

Signed-off-by: Scott Marlow <smarlow@redhat.com>
@scottmarlow
Copy link
Contributor

jakartaee/platform-tck#1334 is for Persistence 3.1 + Jakarta EE 10 Platform TCK

@scottmarlow
Copy link
Contributor

Any Persistence 3.2 test excludes should be done by updating the tckrefactor branch:

  • Add excluded tests to doc file jpa/docs/TCK-Exclude-List.txt
  • Update test methods to use org.junit.jupiter.api.Disabled
    @disabled
    @test
    public void testMethodName() throws Exception {

For reference see git commit 4eb2b298eaf2a954f3c7e38e1da2a8fb6c8f1b85

@scottmarlow
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted certification request challenge TCK challenge
Projects
None yet
Development

No branches or pull requests

4 participants