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

Review and promote Jakarta Persistence TCK for JakartaEE 9 release #385

Closed
alwin-joseph opened this issue Jul 27, 2020 · 36 comments
Closed

Comments

@alwin-joseph
Copy link
Contributor

alwin-joseph commented Jul 27, 2020

  1. Complete userguide/documentation changes for Jakarta Persistence TCK.

  2. Review the jakarta-persistence-tck-3.0.0.zip bundle at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/staged-900/ (or persistence-tck-3.0.0.zip bundle at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9/nightly) to look for any changes missed to be included in JakartaEE 9 release.

  3. Promote the bundle https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/staged-900/jakarta-persistence-tck-3.0.0.zip to https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/promoted/ by using the job https://ci.eclipse.org/jakartaee-tck/job/promote_jakartaee9_eftl_bundles/.

  4. Remove the boolean parameter (PERSISTENCE_TCK) and comment out the respective wget + if condition in https://ci.eclipse.org/jakartaee-tck/job/promote_jakartaee9_eftl_bundles/configure so that the same bundle is not promoted again.

@alwin-joseph alwin-joseph changed the title Review and promote Jakarta Persistence for JakartaEE 9 release Review and promote Jakarta Persistence TCK for JakartaEE 9 release Jul 27, 2020
@scottmarlow
Copy link
Contributor

scottmarlow commented Aug 10, 2020

@lukasj Could you please confirm that you done with making Persistence TCK changes and are ready to consider the TCK frozen?

CC @alwin-joseph @edbratt

Note that @edbratt already made some Persistence TCK doc changes via #411

@lukasj
Copy link
Contributor

lukasj commented Aug 10, 2020

@scottmarlow what exactly I'm supposed to confirm?? This is not assigned to me. I have also never been doing any changes to TCKs nor I'm planning to do directly. I also do not have enough rights to work on TCK project/jobs themselves.

@edbratt
Copy link
Contributor

edbratt commented Aug 10, 2020 via email

@lukasj
Copy link
Contributor

lukasj commented Aug 10, 2020

Have you reviewed the archive (.zip) contents for the TCK?

OK, where can I find the zip I'm supposed to review if it is not the one under https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/staged-900/ folder? The one here has ie wrong jpa spec version number, among other things.

@edbratt
Copy link
Contributor

edbratt commented Aug 10, 2020 via email

@lukasj
Copy link
Contributor

lukasj commented Aug 10, 2020

basically #364 is not fixed, that is currently the main blocker.

@edbratt
Copy link
Contributor

edbratt commented Aug 10, 2020 via email

@edbratt
Copy link
Contributor

edbratt commented Aug 10, 2020

I wasn't aware these properties (or the results of their settings) showed up in any of the material included in the TCK zip files. However, I have proposed #419 to update the user_guide pom file and also the _config.yml file. Please check th PR and let me know if more changes are needed.
I don't honestly understand how src/main/jbake/assets/_config.yml shows up anywhere, except in a GH Pages web-site (which I wasn't aware existed). On the off chance that the build pom is the problem, I did update that. Regardless, please review. If you find anything else, let me know

@alwin-joseph
Copy link
Contributor Author

Just FYI, after the documentation changes for persistence was made in #411 (and also #419) , it would require to regenerate the tck bundles to be updated in https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/staged-900/jakarta-persistence-tck-3.0.0.zip.
We can do that once #419 is merged.

@alwin-joseph
Copy link
Contributor Author

I have merged #419 . Please let us know if it is ok to refresh the https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/staged-900/jakarta-persistence-tck-3.0.0.zip .

@lukasj
Copy link
Contributor

lukasj commented Aug 11, 2020

Is there a nightly where I can verify recent changes? The most recent build I found at https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9/nightly/ is from July 30. Based on PRs and briefly checking sources, it looks good to me

@lukasj
Copy link
Contributor

lukasj commented Aug 12, 2020

based on the current staged build:

@alwin-joseph
Copy link
Contributor Author

alwin-joseph commented Aug 12, 2020

based on the current staged build:

* assertion files do refer to version 2.2 - #423

Thanks, I merged this and refreshed the bundle.

* the user guide often refers to Persistence specification (short spec name) instead of Jakarta Persistence specification (don't think this is a big deal)

* schemas updated for EE 9 are not present (nor used?), source folder (https://github.com/eclipse-ee4j/jakartaee-tck/tree/master/lib/schemas) has none of them, persistence related updates are at https://jakarta.ee/xml/ns/persistence/

@gurunrao @scottmarlow Do you know if we need to update the schemas with the https://jakarta.ee/xml/ns/persistence for jpa

@scottmarlow
Copy link
Contributor

@alwin-joseph if we update the persistence tests to use https://jakarta.ee/xml/ns/persistence, that would likely mean that JPA 1.0 persistence providers would not work with Jakarta EE 9.

Some years ago, the JPA TCK used to include a TopLink (JPA 1.0 I think) persistence provider that was used to test that EE servers still supported JPA 1.0 persistence providers. This was rewritten and no longer tests with a JPA 1.0 persistence provider, however I think the contract for JPA 1.0 support is still required by the https://jakarta.ee/specifications/persistence/3.0/persistence_3.0-RC2.html#a12802 which mentions:

When the container finds a persistence.xml file, it must process the persistence unit definitions that it contains. The container must validate the persistence.xml file against the persistence_2_2.xsd, persistence_2_1.xsd, persistence_2_0.xsd, or persistence_1_0.xsd schema in accordance with the version specified by the persistence.xml file and report any validation errors. Provider or data source information not specified in the persistence.xml file must be provided at deployment time or defaulted by the container. The container may optionally add any container-specific properties to be passed to the provider when creating the entity manager factory for the persistence unit.

In contrast, Jakarta EE 9 is only required to support backward compatibility with Jakarta EE 8 compatible applications. IMO, JPA has its own rules, as mentioned in https://jakarta.ee/specifications/persistence/3.0/persistence_3.0-RC2.html#a12802.

I do speak from my experience developing + maintaining the WildFly/JBOSS JPA container (from JBoss AS 7+) and being on the JPA expert group (I think from 2.0+).

@lukasj do you agree^?

In theory, we could add/change a single JPA test to use the https://jakarta.ee/xml/ns/persistence/ to ensure that all tested persistence providers (including EclipseLink) support https://jakarta.ee/xml/ns/persistence but I think we are past the point of adding new tests.

cc: @kwsutter @edbratt

@lukasj
Copy link
Contributor

lukasj commented Aug 12, 2020

wrt persistence spec itself - given the requirement on backward compatibility is EE 8 only, I'd just remove versions < 2.2 from the spec and add 3.0 there. I believe it's better to be done now than later (which can easily lead to never).

if we update the persistence tests to use https://jakarta.ee/xml/ns/persistence, that would likely mean that JPA 1.0 persistence providers would not work with Jakarta EE 9.

Wouldn't the same be true even for 2.0 - generally for anything "older" than 3.0?. I also wouldn't be surprised if these new namespaces are missed on the implementations side...

@scottmarlow
Copy link
Contributor

@lukasj
Wouldn't the same be true even for 2.0 - generally for anything "older" than 3.0?. I also wouldn't be surprised if these new namespaces are missed on the implementations side...

Yes, you are right, as anything older than 3.0 will still reference javax class namespace. So, yes, we should update to https://jakarta.ee/xml/ns/persistence.

cc: @alwin-joseph

@scottmarlow
Copy link
Contributor

scottmarlow commented Aug 12, 2020

Yes, you are right, as anything older than 3.0 will still reference javax class namespace. So, yes, we should update to https://jakarta.ee/xml/ns/persistence.

The reason being that JPA 3.0 persistence providers shouldn't be required to support the older schemas.

@scottmarlow
Copy link
Contributor

I also expect that many JPA container + Persistence providers will also still handle the older schemas for support of the linked https://jakarta.ee/specifications/persistence/3.0/persistence_3.0-RC2.html#a12802 section.

@lukasj
Copy link
Contributor

lukasj commented Aug 12, 2020

@edbratt few minor things:

  • section 1.2.7 of the user guide refers to Jakarta Persistence 23.0 TCK in one of the notes
  • would be good for Jakarta Persistence TCK 3.0 User's Guide to be clickable link in docs/index.html as well as in the release notes

the only remaining item is how to deal with updated schema...

@scottmarlow
Copy link
Contributor

How? I think it depends on if we agree on the reason for switching persistence.xml/orm.xml to 3.0 persistence XSD, being persistence providers shouldn't be required to support the older schemas to pass the TCK.

We can change one persistence.xml or all of them, there aren't that many. Same with orm.xml:

jakarta/jakartaee-tck $ find -name persistence.xml | wc -l
30
jakarta/jakartaee-tck $ find -name orm.xml | wc -l
28

The pluggability tests could also be updated to 3.0.

@lukasj
Copy link
Contributor

lukasj commented Aug 12, 2020

How? I think it depends on if we agree on the reason for switching persistence.xml/orm.xml to 3.0 persistence XSD, being persistence providers shouldn't be required to support the older schemas to pass the TCK.

the decision here is going to have cascading effect on other specs as there are other schemas as well, ie soap ws do have bunch of them, then there are also ie faces, bean val.... It does not make much sense to me for one spec to have different handling and tests than others. If the decision is that new namespaces are optional - not what I would prefer but I could live with it - then we're already done. Anything else means some work to do and the question is how much work that would be.

@scottmarlow
Copy link
Contributor

scottmarlow commented Aug 12, 2020

Good points @lukasj, thanks for raising them and clarifying. I will step back and restate your question now but first, there was a related vote on the https://www.eclipse.org/lists/jakartaee-platform-dev/msg01400.html thread, I do see various votes for #3 that is reflected in the Jakarta EE Platform call minutes from January 28, 2020.

So, in summary, the question is are we required to update TCK tests to use the EE 8+ schemas?

What would the impact be if we don't complete update the remaining TCK references to schemas that pre-date Jakarta EE 8?

What would the impact be if we do update the remaining TCK test references to schemas that pre-date Jakarta EE 8?

For reference the issue for updating the remaining schemas is #266 (I updated a comment at the end to include an updated list of http://xmlns.jcp.org/xml/ns/javaee references but we should list others as well in that issue so we can be sure to make a complete sweep) of them all.

@scottmarlow
Copy link
Contributor

scottmarlow commented Aug 13, 2020

Also see What is the best way to replace remaining < EE 8 XSD schemas in the Platform TCK with EE 9 Schemas? email conversation on https://www.eclipse.org/mhonarc/lists/jakartaee-tck-dev/msg00876.html

@scottmarlow
Copy link
Contributor

scottmarlow commented Aug 13, 2020

the decision here is going to have cascading effect on other specs as there are other schemas as well, ie soap ws do have bunch of them, then there are also ie faces, bean val.... It does not make much sense to me for one spec to have different handling and tests than others. If the decision is that new namespaces are optional - not what I would prefer but I could live with it - then we're already done. Anything else means some work to do and the question is how much work that would be.

As per https://eclipse-ee4j.github.io/jakartaee-platform/minutes/2020-01-28.html the requirement is:
3. Support for Jakarta EE 8 but not earlier schemas is required in Jakarta EE 9 products; support for schemas earlier than Jakarta EE 8 is optional in Jakarta EE 9 products. Votes: 7

@scottmarlow
Copy link
Contributor

So, in summary, the question is are we required to update TCK tests to use the EE 8+ schemas?

IMO, yes, as per https://eclipse-ee4j.github.io/jakartaee-platform/minutes/2020-01-28.html.

What would the impact be if we don't complete update the remaining TCK references to schemas that pre-date Jakarta EE 8?

TCK challenges might later be filed by any implementations that doesn't think they are required to pass TCK tests that require schemas that are < Jakarta EE 8. IMO, the challenges would likely result in said TCK tests being excluding which lowers the bar for compatibility, which isn't good.

What would the impact be if we do update the remaining TCK test references to schemas that pre-date Jakarta EE 8?

Updating the remaining TCK tests that still use older (pre Jakarta EE 8) to instead use Jakarta EE 9 schemas could introduce TCK test failures, so I think we need to get the TCK test schema updates in soon so that impacted SPEC API ballots can conclude. We also need to identify which TCKs are impacted (see comment from Lukas above that mentioned some of them).

@alwin-joseph
Copy link
Contributor Author

alwin-joseph commented Aug 14, 2020

@lukasj

  1. I was replacing http://xmlns.jcp.org/xml/ns/persistence & http://xmlns.jcp.org/xml/ns/persistence/persistence_2_1.xsd with https://jakarta.ee/xml/ns/persistence & https://jakarta.ee/xml/ns/persistence/persistence_3_0.xsd.
    But I get jakarta.persistence.PersistenceException: No Persistence provider for EntityManager exception as below from jakarta.persistence.Persistence.createEntityManagerFactory.

[javatest.batch] 08-14-2020 03:50:07: ERROR: Test case throws exception: com.sun.ts.lib.harness.EETest$Fault: Setup failed:
[javatest.batch] 08-14-2020 03:50:07: ERROR: Exception at:
[javatest.batch] 08-14-2020 03:50:07: ERROR: jakarta.persistence.PersistenceException: No Persistence provider for EntityManager named JPATCK
[javatest.batch] at jakarta.persistence.Persistence.createEntityManagerFactory(Persistence.java:86)

The PU properties we pass is :

[javatest.batch] 08-14-2020 03:50:07: TRACE: persistence unit properites from user: {jakarta.persistence.jdbc.password=cts1, jakarta.persistence.provider=org.eclipse.persistence.jpa.PersistenceProvider, eclipselink.logging.level=OFF, jakarta.persistence.jdbc.url=jdbc:derby://localhost:1527/derbyDB;create=true, jakarta.persistence.jdbc.driver=org.apache.derby.jdbc.ClientDriver, jakarta.persistence.jdbc.user=cts1}

Can you suggest if we are missing any property or something else.

  1. The https://jakarta.ee/xml/ns/persistence/orm/orm_3_0.xsd describes to use the schema as below:

// <entity-mappings xmlns="https://jakarta.ee/xml/ns/persistence/orm"
// xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
// xsi:schemaLocation="https://jakarta.ee/xml/ns/persistence/orm
// https://jakarta.ee/xml/ns/persistence/orm_3_0.xsd"
// version="3.0">
// ...

But it seems https://jakarta.ee/xml/ns/persistence/orm_3_0.xsd is not the correct link instead https://jakarta.ee/xml/ns/persistence/orm/orm_3_0.xsd is.

Hence the below mapping was used :
// <entity-mappings xmlns="https://jakarta.ee/xml/ns/persistence/orm"
// xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
// xsi:schemaLocation="https://jakarta.ee/xml/ns/persistence/orm
// https://jakarta.ee/xml/ns/persistence/orm/orm_3_0.xsd"
// version="3.0">
// ...

But both above mappings do give below exception for orm. Can you suggest how should the mapping be.

[javatest.batch] Internal Exception: Exception [EclipseLink-7305] (Eclipse Persistence Services - 3.0.0.v202004301223): org.eclipse.persistence.exceptions.ValidationException
[javatest.batch] Exception Description: An exception was thrown while processing the mapping file from URL: [jar:file:/home/jenkins/agent/workspace/jakartaee-tck-alwin_jpanamespace/persistence-tck/dist/com/sun/ts/tests/jpa/core/annotations/elementcollection/jpa_core_annotations_elementcollection.jar!/myMappingFile.xml].
[javatest.batch] Internal Exception: Exception [EclipseLink-25008] (Eclipse Persistence Services - 3.0.0.v202004301223): org.eclipse.persistence.exceptions.XMLMarshalException
[javatest.batch] Exception Description: A descriptor with default root element {https://jakarta.ee/xml/ns/persistence/orm}entity-mappings was not found in the project

cc @gurunrao @scottmarlow

@scottmarlow
Copy link
Contributor

@lukasj Is there a tracking issue/pullrequest for switching to https://jakarta.ee/xml/ns/persistence/orm/orm_3_0.xsd as mentioned in #2 in previous comment?

@lukasj
Copy link
Contributor

lukasj commented Aug 26, 2020

nope, feel free to create it. note that orm_3.0.xsd is not the only one, at least persistence_3.0.xsd and probably jaxb cust schema may have to be adopted too

@scottmarlow
Copy link
Contributor

@lukasj I created jakartaee/persistence#286 for a update in orm_3_0.xsd.

Are there EclipseLink or other https://github.com/jakartaee/specifications issues needed?

jakartaee/specifications#246 does seem to have https://github.com/jakartaee/specifications/pull/246/files#diff-579dce887502f6db9ebf8c8e758ccba4R16 which refers to orm_3_0.xsd.

What additional changes are needed for persistence_3.0.xsd and probably jaxb cust schema?

CC @alwin-joseph

@lukasj
Copy link
Contributor

lukasj commented Aug 26, 2020

eclipselink as a compatible implementation included in GF does not support new schemas yet, that is the main thing at this point.

@scottmarlow
Copy link
Contributor

I just added eclipse-ee4j/eclipselink#886 for updating EclipseLink to reference https://jakarta.ee/xml/ns/persistence/orm/orm_3_0.xsd in:

  • jpa/org.eclipse.persistence.jpa/src/main/resources/org/eclipse/persistence/jpa/orm_3_0.xsd
  • jpa/org.eclipse.persistence.jpa/src/main/resources/org/eclipse/persistence/jpa/persistence_3_0.xsd

Sound right?

@alwin-joseph
Copy link
Contributor Author

@scottmarlow
Copy link
Contributor

@lukasj do you want us to stage a new jakarta-persistence-tck-3.0.0.zip with the new changes from #537?

If you then approve of the new TCK, we can promote that over https://download.eclipse.org/ee4j/jakartaee-tck/jakartaee9-eftl/promoted/jakarta-persistence-tck-3.0.0.zip.

CC @alwin-joseph

@lukasj
Copy link
Contributor

lukasj commented Oct 6, 2020

@scottmarlow I think that would be the best thing to do but I'm not sure if we can do so, since the ballot is already running and I don't want it to be restarted for the 3rd time. Maybe @kwsutter knows if we can just go ahead and update TCK binary. Since tomitribe did not vote '-1' on the ballot due to this, it can probably wait for the next release (be it 3.0.1 or version for EE 10).

Rerunning TCKs and updating checksums in existing PRs and CI requests is not a big deal on my end should the decision be to update the tck binary now.

@scottmarlow
Copy link
Contributor

Thanks @lukasj , I agree that including the changes in the next TCK release will also be good.

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

4 participants