Skip to content

fix: resolve duplicate invocation of overloaded lifecycle methods with inheritance#3856

Merged
filiphr merged 3 commits intomapstruct:mainfrom
tangyang9464:feature/3849
May 25, 2025
Merged

fix: resolve duplicate invocation of overloaded lifecycle methods with inheritance#3856
filiphr merged 3 commits intomapstruct:mainfrom
tangyang9464:feature/3849

Conversation

@tangyang9464
Copy link
Contributor

@tangyang9464 tangyang9464 commented May 13, 2025

fix: 3849

Newly added LifecycleOverloadDeduplicateSelector, which uses multiple features such as class name and method name as keys to group methods.

And then selects the best method within the group by comparing the inheritance distance of parameters,
to achieve deduplication between similar overloaded methods.

The parameters involved in the inheritance distance calculation are not limited to @MappingTarget, but all input parameters of a group of candidate methods are traversed and calculated in sequence until a parameter is found with the shortest distance, then the traversal is terminated. (Avoid the situation where the @MappingTarget parameter type is the same but other parameters are different).

filiphr
filiphr requested changes May 17, 2025
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

Thanks @tangyang9464, the solution looks good to me. I've left some comments, which are mainly for performance optimization, as this can get called a lot, especially if you have a lot of lifecycle methods.

Additionally, I think that it'll be good if we can add some tests with generics. I am not sure whether everything would fine then.

Also, can we perhaps add some compiler flag so that people can disable this particular selector in case it causes them performance problems and / or something isn't working properly, just in case. I think we can pass the Options to the MethodsSelectors and then check whether we should include this new one or not. What do you think about that?

@filiphr
Copy link
Member

filiphr commented May 17, 2025

btw, the build is failing due to the javadoc

Error:  /home/runner/work/mapstruct/mapstruct/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/LifecycleOverloadDeduplicateSelector.java:35: error: unknown tag: AfterMapping
Error:   * @AfterMapping default void afterMapping(Parent source, @MappingTarget ParentDto target) { ... }
Error:     ^
Error:  /home/runner/work/mapstruct/mapstruct/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/LifecycleOverloadDeduplicateSelector.java:36: error: unknown tag: AfterMapping
Error:   * @AfterMapping default void afterMapping(Parent source, @MappingTarget ChildDto target) { ... }
Error:     ^
Error:  /home/runner/work/mapstruct/mapstruct/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/LifecycleOverloadDeduplicateSelector.java:37: error: unexpected end tag: </pre>
Error:   * </pre>

@tangyang9464
Copy link
Contributor Author

tangyang9464 commented May 17, 2025

I'll modify based on the comments and fix CICD later

@tangyang9464
1. add disable compile args for LifecycleOverloadDeduplicateSelector
07c5bc4 
2. add test for compile args and generic

Signed-off-by: TangYang <[email protected]>
@tangyang9464
Copy link
Contributor Author

tangyang9464 commented May 17, 2025 •
edited
Loading

@filiphr plz review again. I did the following:

  1. Added compile args to disable the selector and add related tests
  2. Added generic tests
  3. Optimized the code to improve performance based on your comments

@tangyang9464
Copy link
Contributor Author

tangyang9464 commented May 21, 2025

@filiphr any viewpoint？

@tangyang9464 tangyang9464 requested a review from filiphr May 21, 2025 14:03
@filiphr
Copy link
Member

filiphr commented May 21, 2025

Thanks for the changes @tangyang9464. During the week it's a bit more difficult for me to check these stuff out. I'll review it soon and merge it

filiphr
filiphr approved these changes May 25, 2025
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

LGTM

@filiphr filiphr merged commit 0badba7 into mapstruct:main May 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filiphr filiphr filiphr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Overloaded AfterMapping is called twice

2 participants

@tangyang9464 @filiphr