-
-
Notifications
You must be signed in to change notification settings - Fork 990
#2946 allow update methods with @SubclassMapping
#3330
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
base: main
Are you sure you want to change the base?
Conversation
this change implements the functionality to generate update methods (`@MappingTarget` methods) in conjunction with `@SubclassMapping`
@SubclassMapping
@SubclassMapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this @d3psi. I've left some comments.
In addition to the comments I have some other remarks:
- Please add some more tests, perhaps by copying the existing tests for normal mapping and applying them for update mappings
- How does the check for subclass mapping exhaustion look like? From what I saw we are doing
if (source instanceof SourceType && target instanceof TargetType)
then the mapping is done. However, this means that people can make mistakes. Therefore, we need to add some errors and other things like that for this. There are also other scenarios where the target type you are updating might not matter.
Having written the second remark we might need to think for a bit better solution for subclass update mappings. Especially in combinations of what you would like to upgrade and from what.
e.g. Consider the following
class Vehicle {}
class Car extends Vehicle {}
class Bike extends Vehicle {}
class VehicleDto {}
class CarDto extends VehicleDto {}
class BikeDto extends VehicleDto {}
When doing update mappings there are various scenarios
@Mapper
public interface CustomMapper {
void update(@MappingTarget Vehicle target, VehicleDto source);
}
So what you can now do is the following:
- Use
CarDto
,BikeDto
orVehicleDto
to update the target - Use
CarDto
to update theCar
target - Use
CarDto
to update theBike
target
...
As you can see with updates there are way more possible combinations that can be done.
ParameterBinding.fromParameters( methodRef.getParameters() ) | ||
ParameterBinding.fromParameters( methodRef.getSourceParameters() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed?
public boolean isUpdateMethod() { | ||
return Parameter.getMappingTargetParameter( getParameters() ) != null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and use isExistingInstanceMapping
instead
if ( basedOn.isUpdateMethod() ) { | ||
this.parameters.add( Parameter.forForgedMappingTarget( returnType ) ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Why don't we pass everything in the additional parameters?
void unsupportedUpdateMethod() { | ||
@WithClasses({ SubclassUpdateMapper.class }) | ||
@ExpectedCompilationOutcome(value = CompilationResult.SUCCEEDED) | ||
void updateMethod() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this test and write more thorough tests
Any progress on this PR? |
Just putting my 2c into this issue (I made the original suggestion a while back). I believe it would be prudent to generate a single mapping method per permutation. You would then use the @SubclassMapping as the source of truth, only falling back to the most specific common declared ancestor if an exact match was not found. In the case of second-generation ancestors, we would add a new annotation to specify the common ancestor of each defined subclass mapping. So if I created an inheritance tree as seen below:
In this case, the This would mean we would not generate a mapping method that allows the In this case, I think its okay to omit non-explicit downcasts of 2nd-generation-onward ancestors, because otherwise you get the whole problem of generating a mapper for the cross product of every class and every DTO that is an ancestor of the @SubclassMapping. If we allow the craziness to unfold, we are going to get people asking why they cannot provide a Without the @SubparentMapping you would get this: Imagine a heirarchy like A -> B -> C You would have to generate: Adto -> A etc. etc. Instead of it being linear, you now have to add: With the @SubparentMapping(source=Cdto.class, target=C.class) Ddto -> D Finally, I am willing to work on this in my spare time if needed. I would just need to learn the mapstruct ecosystem first. |
this change implements the functionality to generate update methods
(
@MappingTarget
methods) in conjunction with@SubclassMapping