oasp4j-jpa-envers: org.hibernate.MappingException: Only one entity may be annotated with @RevisionEntity!

hohwille commented 7 years ago

The module oasp4j-jpa-envers is containing two entities that are both annotated with @RevisionEntity:

This has been introduced in oasp4j release 2.3.0. Existing applications or project with code like:

@EntityScan(basePackages = "com.mycustomer.app", basePackageClasses = { AdvancedRevisionEntity.class })

Break when upgrading from 2.2.0 to 2.3.0 (or current 2.4.0). An easy solution is possible by just changing the import of AdvancedRevisionEntity from 1. to 2. (see above).

According to OASP4J package convention (https://github.com/oasp/oasp4j/wiki/coding-conventions#packages) common shall be used for layer so the new entity (2.) is not really in a sane package.

After diffing 1. and 2. I figured out that the actual difference is that the property user has been renamed to userLogin. IMHO this was to solve problems with keywords and support for multiple databases. Can someone confirm this? In this context you should at least know that we are using the 2.2.0 code (1.) in my project together with h2 and oracle DB without problems.

As the problem has already been introduced the question now is how to "fix" or proceed with this. I am currently quite unsure what should be the best solution. I do not prefer the new package of 2. but shall we create yet another 3. variant of the class in a new package? I guess this could cause even more confusion. Should we leave it like this and note in the release-notes how to migrate from 2.2.0 to 2.3.0+ and that is all? IMHO the most important thing is that we learn from such mistakes for the future. Mistakes can happen and this one was not obvious so nobody to blame. However, we need to ensure that we learn from our mistakes...

maybeec commented 7 years ago

my suggestion for a compliant package: io.oasp.module.jpa.general.dataaccess.api

maybeec commented 7 years ago

@hohwille this class is detected by scanning the classpath. So deprecation does not work at all just by annotating it like this. So from my point of view, we have to specify an upgrade path for such beans. Otherwise, you have to step back again and add another configuration which class to use directly. I would not prefer the latter solution.

I think this is a general problem and we should have a general answer to this.

Especially, looking at this case, we already broke release 2.3.0 with jpa-envers. So my suggestion would be to clean code up, provide the newest version in whatever package and provide a migration path. Nobody seems to use 2.3.0 envers. Otherwise, we would have heard from that.

Another option would be to rollback the renaming of user -> userlogin and continue with the old version in 2.3.0 as it maybe was even not valid to provide such kind of change in a minor release.

My vote however, goes for cleanup + general concept of updating scanned beans by providing an upgrade path rather than deprecating stuff.

hohwille commented 7 years ago

According to https://github.com/oasp/oasp4j/blob/fa2b448a3cfb3d517a9232a82cefb344b33492b8/modules/basic/src/main/java/io/oasp/module/basic/common/api/reflect/OaspPackage.java the segment before the «layer» is the «component» and before that it is «app». So you suggestion @maybeec would change from app: "module", component: "jpa" to app: "jpa", component: "general". IMHO that does not make sense either. If we really want to go for a new package namespace here and go for all the changes, I would suggest to go from io.oasp.module.jpa to io.oasp.module.jpaenvers or io.oasp.module.jpa_envers so we are already prepared for JDK9/Jigsaw. But maybe even envers was not the best choice even for the module name as it is the name of the implementation and we should better go for the name of the aspect what is actually historization or journal. Still I would like not to make a huge impact out of this.

hohwille commented 7 years ago

@hohwille this class is detected by scanning the classpath. So deprecation does not work at all just by annotating it like this. So from my point of view, we have to specify an upgrade path for such beans. Otherwise, you have to step back again and add another configuration which class to use directly. I would not prefer the latter solution.

The problem arises if you are scanning the package io.oasp.module.jpa.dataaccess.api but it goes away if you scan io.oasp.module.jpa.dataaccess.api.common instead. Instead of common it could still be anything else like envers or journal but the compatibility issue comes from the fact that a sub-package was used. Instead in such case a new sibling package has to be used to ensure compatibility.

maybeec commented 7 years ago

@maybeec would change from app: "module", component: "jpa" to app: "jpa", component: "general". IMHO that does not make sense either.

You are right, I did not get that jpa is already the app segment...

The problem arises if you are scanning the package io.oasp.module.jpa.dataaccess.api but it goes away if you scan io.oasp.module.jpa.dataaccess.api.common instead. Instead of common it could still be anything else like envers or journal but the compatibility issue comes from the fact that a sub-package was used. Instead in such case a new sibling package has to be used to ensure compatibility.

Sure, but always introducing a new package to be scanned might even be an issue or at least code smell. So how to continue? api -> api2 -> api3?

kiran-vadla commented 7 years ago

@maybeec and @hohwille , Thanks for the feedback . If you can come up with the decision on the approach/solution to follow (including the documentation if at all it applies ) and summarize , I can take it up in the morning and raise a PR in the morning tomorrow and you can review the same.

Thanks, @kiran-vadla

hohwille commented 7 years ago

To conclude our meeting:

we will move 1. to io.oasp.module.jpa.dataaccess.base
we will move 2. to io.oasp.module.jpa.dataaccess.api (the old place of 1.).
we will add some hint in JavaDocs of 1. and 2.
we will add a note to release-notes for upgrading (if you want to keep compatibility use the deprecated 1. and if you start from scratch or want to go the suggested way use the new 2. - the impact of the change is that the column in DDL has to be renamed from user to userLogin).

kiran-vadla commented 7 years ago

Thanks @hohwille

Thanks, @kiran-vadla

kiran-vadla commented 7 years ago

Closing this issue as it is addressed by PR #597

Thanks, @kiran-vadla

hohwille commented 7 years ago

I was a little busy but meanwhile I was able to test the new release. It still does not work as this change is not backward compatible. It actually forces me to migrate the column to userLogin. If I change my code to reference the old deprecated entity, I will get this error:

Caused by: java.lang.ClassCastException: io.oasp.module.jpa.dataaccess.base.AdvancedRevisionEntity cannot be cast to io.oasp.module.jpa.dataaccess.api.AdvancedRevisionEntity
    at io.oasp.module.jpa.dataaccess.api.AdvancedRevisionListener.newRevision(AdvancedRevisionListener.java:27)
    at org.hibernate.envers.internal.revisioninfo.DefaultRevisionInfoGenerator.generate(DefaultRevisionInfoGenerator.java:93)
    at org.hibernate.envers.internal.synchronization.AuditProcess.getCurrentRevisionData(AuditProcess.java:114)
    at org.hibernate.envers.internal.synchronization.AuditProcess.executeInSession(AuditProcess.java:96)
    at org.hibernate.envers.internal.synchronization.AuditProcess.doBeforeTransactionCompletion(AuditProcess.java:153)
    at org.hibernate.envers.internal.synchronization.AuditProcessManager$1.doBeforeTransactionCompletion(AuditProcessManager.java:46)
    at org.hibernate.engine.spi.ActionQueue$BeforeTransactionCompletionProcessQueue.beforeTransactionCompletion(ActionQueue.java:899)
    ... 69 more

This is because both versions of AdvancedRevisionEntity are refering the same AdvancedRevisionListener class and that one refers to the new class and fills the properties. To guarantee compatibility we would also need to duplicate a deorecated AdvancedRevisionListener class.

kiran-vadla commented 7 years ago

Closing this issue as the fix for the issue raised above by @hohwille is provided as part of PR #598.

oasp / oasp4j

oasp4j-jpa-envers: org.hibernate.MappingException: Only one entity may be annotated with @RevisionEntity! #596