oasp / oasp4j

The Open Application Standard Platform for Java
Apache License 2.0
60 stars 303 forks source link

IdRef feature for typesafe and expressive entity references #617

Closed hohwille closed 5 years ago

hohwille commented 6 years ago

With OASP4J so far references to other entities (in TOs) are represented as Long. This is not type-safe, not expressive and self-descriptive and finally error-prone: https://github.com/oasp/oasp4j/blob/develop/samples/core/src/main/java/io/oasp/gastronomy/restaurant/salesmanagement/logic/api/to/OrderPositionEto.java#L17 Without further documentation or guessing from names you can not determine the type of the linked entity as you only see a Long value. Even worse to map such a reference back from TO to entity we are using this "hack": https://github.com/oasp/oasp4j/blob/develop/samples/core/src/main/java/io/oasp/gastronomy/restaurant/salesmanagement/dataaccess/api/OrderPositionEntity.java#L84 While this "hack" is working in 95% of the cases it relies on JPA/hibernate internal magic and I faced the ugly 5% cases in my customer project where this "hack" is simply not working and you end up with complex hibernate stacktraces.

To address this and improve OASP4J this PR introduces a generic interface Ref (used as shortcut for Reference as the name will be spread around the code-base and should therefore be short) and a standard datatype implementation IdRef that is just a wrapper for the referenced ID as Long. So instead of

private Long orderId;

You can now write

private IdRef<Order> orderId;

What makes your reference type-safe and expressive. Starting with Java8 type-inference you can easily write this.orderId = IdRef.of(myOrder);. As a solution to avoid the "hack" mentioned above I introduced JpaHelper. It still requires some static "hack" internally but solves the hibernate issues so it works in 100% of the cases (though it will cause problems if you have multiple EntityManagers in a single app - what is rather discouraged but might also happen). However, such projects are still free to use the old "hack" instead. One could also implement a tweaked version of JpaHelper that does exactly this instead.

Example usage (created for JUnit testing): https://github.com/hohwille/oasp4j/blob/fd13d104a386a164ba068cea834a4b55ab1bda7d/modules/jpa/src/test/java/io/oasp/example/component/dataaccess/api/FooEntity.java#L61

Test-Cases:

As an enhancement I could create a spring-boot-starter for JpaInitializer as oasp4j-starter-jpa. See: https://github.com/hohwille/oasp4j/blob/fd13d104a386a164ba068cea834a4b55ab1bda7d/modules/jpa/src/test/java/io/oasp/example/TestApplication.java#L18

hohwille commented 6 years ago

However, your PRs are always a lot of effort to review due to the fact, that they do not concentrate on a specific task

Indeed I removed some empty JavaDoc lines from classes I came along on my way that are not directly related. However, even though it might not look like in the first place: All I did is to add this feature and then create a test for the feature. The test itself requires a lot of implicit changes what is IMHO what you are talking of here. In the past I added features without JUnits but I learned that this is even worse so I now always add tests. In my PR I added a lot of explanation why I had to do all these related changes. Of course you are right that this change is then hard to review. Anyhow I still would not see how to separate this properly into multiple PRs that make sense of its own. Maybe for #627 your feedback applies however... Thanks for your feedback and review. I would love to see some more discussion with others before we finally decide. IMHO if we merge this we have to ask ourselves if this is the new recommended way to deal with relations. If so we would change cobigen templates accordingly, etc.

amarinso commented 6 years ago

Seems a nice idea to me.

Could it create problems if using automated mapper utilities? meaning, if you have to convert from one Ref of a package to a different one... Just wondering, maybe is not related at all, didn't have the chance to play with it

hohwille commented 6 years ago

@amarinso I had to tweak our beanmapping config so IdRef is considered as data-type that is not to be copied but used as value. So yes, it can have such impact but it will IMHO be minimal and easy to solve. Actually I would love to change the entire JPA to address this feature even more deeply integrated so the primary key would always be such an object and changing the internal underlying type of a primary key would have a lot less impact on the actual code, but this would really interfere with all the existing frameworks (I do such experiments in my private spare time though - see e.g. https://github.com/m-m-m/util/blob/master/data/src/main/java/net/sf/mmm/util/data/api/id/Id.java, I also integrated this with JavaFx properties and can map dynamic beans with NoSQL Databases such as OrientDB but this is not what I am proposing here).

hohwille commented 6 years ago

After all this is a nice new feature to OASP/Devon that does not interfere if not being used. I therefore propose to include it in Fred (3.0.0). However, some alignment with PR #627 (spring-data) is IMHO needed. We should therefore clarify which PR shall be merged first and which one to adopt.

maybeec commented 6 years ago

Did you check javax.persistence.MapsId? Maybe this already solved this issue.

hohwille commented 5 years ago

Did you check javax.persistence.MapsId?

I am not sure if I am getting this right. @MapsId is a JPA annotation to map the ID of an entity to some transient object like an embedabble.

Maybe this already solved this issue.

Also I am not so sure if I understand which issue you are actually talking about. If you are referring to my conversation with @amarinso - do you think we could IdRef directly as ID in our entities instead of Long? This would IMHO be a nice additional experiment to look at. However, I think this can be done after this PR is completed. I do see that currently all our entities are using Long and we do have DAO and Repository interfaces as well as GenericEntity that would all then not work anymore as is. Further according to my experience with JPA and hibernate such features only work on a conceptual level and cause many headaches in real life scenarios. Do not get me wrong. This would be a nice experiment. But before proposing this to projects, I would rather have this tested in a real world project before.

hohwille commented 5 years ago

This PR is ready for merge now. If there are any further concerns or improvements to consider please hurry up. We will most probably merge this by end of this week.