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

Better compatibility with Hibernate Envers #1432

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

EddyVerbruggen
Copy link
Contributor

Purpose

When Hibernate Envers is triggered via an @Audited annotation, it attempts to insert a record in a related AUD table.

However, Play! assumes all related entities extend JPABase, which the AUD tables don't. This leads to a ClassCastException because HashMap is cast to JPABase.

By adding a check for JPABase here, we can avoid this problem. This effectively also checks for 'not null' which the previous implementation did.

…ed via an `@Audited` annotation, it attempts to insert a record in a related `AUD` table. However, Play! assumes all related entities extend `JPABase`, which the `AUD` tables don't. This leads to a `ClassCastException` because `HashMap` is cast to `JPABase`. By adding a check for `JPABase` here, we can avoid this problem. This effectively also checks for 'not null' which the previous implementation did.
@aleksandy
Copy link
Contributor

@EddyVerbruggen, could you please provide a small test case demonstrating the problem?
I've been using envers since play! 1.2.x and have never experienced this.

@EddyVerbruggen
Copy link
Contributor Author

Hi @aleksandy! I'd love to, but if you can use it without problems and I can't then it's probably something in the configuration of my entities (or something deeper down the stack) which probably makes it pretty time consuming to come up with a good test case.

So before I do, can I ask you to consider the change on merit (outside the context of Envers)?

I mean, checking for instance of X before trying to cast to X (instead of the current 'not null' check) is arguably better anyway. And it fixes the Envers compatibility along the way for my app. 🙏

@EddyVerbruggen EddyVerbruggen changed the title Compatibility with Hibernate Envers Better compatibility with Hibernate Envers Nov 22, 2022
@aleksandy
Copy link
Contributor

@EddyVerbruggen, I agree your point. But unfortunately I don't have enough rights for it.

@asolntsev, @xael-fry, could you approve and merge PR?

@EddyVerbruggen
Copy link
Contributor Author

Hi friends, at Combidesk we're huge fans of Play!(!). We're using Play 1 for a few of our backend systems and would love to have this PR merged and released.

Is there anything we can do to help drive this forward? As OSS maintainers ourselves we know it may be hard to find time and prioritise unpaid work, so we'd be happy to do a decent donation for your efforts as well.

Cheers, and keep up the great work!

Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@xael-fry xael-fry added this to the 1.8.0 milestone Oct 16, 2023
@xael-fry xael-fry merged commit ee3958a into playframework:master Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants