Skip to content

#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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cedricschwyter
Copy link

this change implements the functionality to generate update methods
(@MappingTarget methods) in conjunction with @SubclassMapping

this change implements the functionality to generate update methods
(`@MappingTarget` methods) in conjunction with `@SubclassMapping`
@cedricschwyter cedricschwyter changed the title \#2946 allow update methods with @SubclassMapping #2946 allow update methods with @SubclassMapping Jul 16, 2023
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 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 or VehicleDto to update the target
  • Use CarDto to update the Car target
  • Use CarDto to update the Bike 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() )
Copy link
Member

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?

Comment on lines +1958 to +1961
public boolean isUpdateMethod() {
return Parameter.getMappingTargetParameter( getParameters() ) != null;
}

Copy link
Member

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

Comment on lines +174 to +176
if ( basedOn.isUpdateMethod() ) {
this.parameters.add( Parameter.forForgedMappingTarget( returnType ) );
}
Copy link
Member

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() {
Copy link
Member

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

@s-jepsen
Copy link

s-jepsen commented Sep 4, 2023

Any progress on this PR?

@heanssgen-troy
Copy link

heanssgen-troy commented Jul 25, 2024

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:

@Mapper
public interface CustomMapper {

    @SubclassMapping(source =CarDTO.class, target=Car.class)
    @SubclassMapping(source =BikeDTO.class, target=Bike.class)
    @SubclassMapping(source =F150.class, target=F150DTO.class)
    @SubclassMapping(source =Corvette.class, target=CorvetteDTO.class)
    @SubclassParent(source=Supercar.class, target=SupercarDTO.class)
    void update(@MappingTarget Vehicle target, VehicleDto source);

}

This would build the tree of:
image

In this case, the F150 and Bike would be downcast to a Vehicle if no exact type-matched mapper was found (because Car was not declared as a @SubclassParent). If the Corvette was similarly downcast, it would match Supercar before it matched Vehicle because of the provided @SubclassParent.

This would mean we would not generate a mapping method that allows the Corvette to be upgraded using a SupercarDTO because the user did not ask us to explicitly create that relation.

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 Corvette to the F150 mapping because they both can be downcast to Car.

Without the @SubparentMapping you would get this:

Imagine a heirarchy like A -> B -> C

You would have to generate:

Adto -> A
Bdto -> B
Bdto -> A
Cdto -> C
Cdto -> B
Cdto -> A

etc. etc.
Now add C -> D and C -> E

Instead of it being linear, you now have to add:
Ddto -> D
Ddto -> C
Ddto -> B
Ddto -> A
Edto -> E
Edto -> C
Edto -> B
Edto -> A

With the @SubparentMapping(source=Cdto.class, target=C.class)

Ddto -> D
Ddto -> C
Edto -> E
Edto -> C

Finally, I am willing to work on this in my spare time if needed. I would just need to learn the mapstruct ecosystem first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants