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

Adds support for jakarta persistence annotations #576

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

Ahli
Copy link
Contributor

@Ahli Ahli commented Jan 22, 2022

What problem does this pull request solve?

Jakarta persistence's annotations were not supported.

Please provide any additional information below.

Jakarta persistence's annotations only rename "javax.persistence" to "jakarta.persistence" afaIk.

NOTE

  • Please run mvn spotless:apply to format the code before opening a PR. Otherwise, GitHub Actions will complain at you 😉. Unfortunately, you will need to have Node installed to do so.
  • Mutation tests will be run by PITest after opening the PR. It will post comments in the PR for each issue found. Please take a look at them, but if the comments don't make sense, please don't worry about them.

@github-actions
Copy link
Contributor

Looks good. No mutations were possible for these changes.
Mutation testing report generated by PITest - https://pitest.org - if there are surviving mutants, please check the line comments under 'Files changed', or the full report under the 'CI / pitest' check below this comment.

@jqno
Copy link
Owner

jqno commented Jan 24, 2022

Thanks for this PR! It's a very useful addition.

Would you like to add unit tests for this as well? I can do it myself but it might take a little longer :).

@Ahli
Copy link
Contributor Author

Ahli commented Jan 24, 2022

Would you like to add unit tests for this as well? I can do it myself but it might take a little longer :).

I wasn't sure where to add them. I saw test class that used javax.persistence.Entity. In theory all of these need to run with jakarta.persistence.Entity, too. In theory, those could be copied or adapted. I did not see any unit test that directly checks the detected annotations. But I did not invest a lot of time exploring the code.

Where and how would you test these based on the existing tests? If you point me into the direction, I might add them in near future

@jqno
Copy link
Owner

jqno commented Jan 24, 2022

That's a good question, the answer is indeed not very obvious.
There is JpaEntityTest and JpaIdTest that check for "JpaAnnotations" (from the javax.annotation package) and "NonJpaAnnotations" (from any other package), so in theory it's already covered, but it would be nice to have "JakartaAnnotations" as well, just to be sure it really does pick those up. You can basically copy-paste the existing "JpaAnnotations" tests. Maybe this could be done in a nicer way, but that doesn't need to happen right away.

I have no experience with Jakarta, so I'm not sure how it's structured. If there's a small dependency that contains just the annotations and not (much) more, just add it to the pom. If the only way to add it to the pom is with a dependency of 120MB pulling in all of Jakarta, maybe do what I did with javax.persistence.Embeddable; EqualsVerifier checks the package names for annotations with .endsWith so you can just put them in a similar package to the javax.persistence ones.

@jqno
Copy link
Owner

jqno commented Jan 24, 2022

BTW, I'm actually in the process of re-writing the project to a multi-module setup, so depending on when I merge, you might have merge conflicts. If that happens, I will make sure they get fixed. I mention it just so you won't be surprised if it happens :)

@Ahli
Copy link
Contributor Author

Ahli commented Jan 25, 2022

Jakarta (or more specific JPA 3.0) is JPA 2.x with new package names. See Thorben Janssen's article.

What I did:

  • copied tests from JpaEntityTest and JpaIdTest
  • renamed them from Jpa to Jakarta (although they are still JPA, just JPA 3.0 instead of JPA 2.x)
  • added Jakarta's annotations imported via jakarta's persistence-api dependency (152kb)
  • removed tests that check other annotations as the Jpa*Test already test that. These two new classes could be merged with the JPA ones as they are just add variations of the annotation used.

@github-actions
Copy link
Contributor

Looks good. No mutations were possible for these changes.
Mutation testing report generated by PITest - https://pitest.org - if there are surviving mutants, please check the line comments under 'Files changed', or the full report under the 'CI / pitest' check below this comment.

@jqno
Copy link
Owner

jqno commented Jan 25, 2022

Yeah, I agree they should probably be merged somehow, because it's a lot of duplication. I'll put it on the backlog, for now this will do. Thanks for adding the tests so quickly! Much appreciated!

@jqno jqno merged commit 56fb255 into jqno:main Jan 25, 2022
@jqno
Copy link
Owner

jqno commented Jan 25, 2022

I'll try to make a release today or tomorrow, I'll let you know when it happens.

@jqno
Copy link
Owner

jqno commented Jan 25, 2022

Released in version 3.8.3!

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

Successfully merging this pull request may close these issues.

2 participants