GH-516 - Improve support for @PostLoad annotation. #517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
meistermeier
requested changes
Sep 4, 2018
| @@ -991,21 +990,18 @@ public void registerIdGenerationStrategy(IdStrategy strategy) { | |||
| } | |||
| } | |||
|
|
|||
| public MethodInfo postLoadMethodOrNull() { | |||
| public synchronized MethodInfo postLoadMethodOrNull() { | |||
| if (isPostLoadMethodMapped) { | |||
There was a problem hiding this comment.
This will never get true...
| } | ||
| return null; | ||
|
|
||
| this.postLoadMethod = possiblePostLoadMethods.stream().findFirst().orElse(null); |
There was a problem hiding this comment.
Setting an instance variable and returning it afterwards looks like either the calling part uses this method wrong and/or this method has multiple purposes.
Additionally the mixed usage of this and without it.
There was a problem hiding this comment.
I kept the method the way it was (mainly), but indeed, better split it like the other initXXX methods.
| return new ObjectAnnotations(annotationInfo); | ||
| } | ||
|
|
||
| private final Map<String, AnnotationInfo> annotations; |
There was a problem hiding this comment.
Even if the above is a static method I would, at least, suggest to move this above. The other class that gets the of builder method also keeps it fields on top.
…Construct. Forcing the PostLoad method to be public and non final leads to poor domain classes: - often the postload method is also used from the constructor, so they should be at least final so that the class can be safely overwritten - also postload should usually not be an exposed method to calling code, it’s used to be only internal Steps taken: - Include „final“ methods in MethodsInfo. - When postload is executed, check if the annotated method is accessible. If not, try to make it accessible.
michael-simons
force-pushed
the
issue/516
branch
from
13f8fa4 to
5c65222
Compare
This changes the way `@PostLoad` methods are computed: - Don’t store MethodInfo by their name but by their method, as the name is not even unique per class - Traverse class hierarchy for method infos and prioritize lower class methods - Make sure that no ambigious scenerio can be created with multiple `@PostLoad` methods.
meistermeier
force-pushed
the
issue/516
branch
from
5c65222 to
27b3123
Compare
meistermeier
approved these changes
Sep 4, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please see messages of the two separate commits for the details. I have incorporated @jdorleans PR #485 for #414 and added our discussion to make
@PostLoads semantics identical to@PostConstructfrom JSR-250.