Skip to content

Conversation

@ivlcic
Copy link
Contributor

@ivlcic ivlcic commented Apr 28, 2022

Implemented

Access to the target property name #2688
As agreed in discussion: #2831

Rationale

Currently there is no option to do run-time filtering based on target property name.

This comes very handy when mapping larger object graphs from ORM frameworks, to block
unnecessary loading from database and reuse mapping code.

With this feature you can for instance block further mapping in runtime based on some @Context variable state and reuse mapper code without having to foresee every possible property mapping combination.

Implementation

Implementation introduces support for new presence check method parameter annotated with @TargetPropertyName.

It is implemented with minimum impact to current code base, so it might be in architecture/elegance sense problematic.

It introduces no changes any current features; it's only an addition.

Unit tests are provided in org/mapstruct/ap/test/conditional.

Example 1

@Mapper
public interface MyMapper {

    MyMapper INSTANCE = Mappers.getMapper( MyMapper.class );

    Employee map(EmployeeDto employee);

    @Condition
    default <T> boolean isNotEmpty(Collection<T> collection, @TargetPropertyName String propName) {
        if ( "addresses".equalsIgnoreCase( propName ) ) {
            return false;
        }
        return collection != null && !collection.isEmpty();
    }

    @Condition
    default boolean isNotBlank(String value, @TargetPropertyName String propName) {
        if ( propName.equalsIgnoreCase( "lastName" ) ) {
            return false;
        }
        return value != null && !value.trim().isEmpty();
    }
}

Example 2

@Mapper
public interface MyMapper {

    MyMapper INSTANCE = Mappers.getMapper( MyMapper.class );

    Employee map(EmployeeDto employee, @Context PresenceUtils utils);

    Address map(AddressDto addressDto, @Context PresenceUtils utils);

    class PresenceUtils {
        Set<String> visited = new LinkedHashSet<>();

        @Condition
        public boolean decide(String value, @TargetPropertyName String propName) {
            return true; //decide if continue mapping
        }
    }
}

Complex Example

interface Entity {}
interface Dto {}

class Employee implements Entity { // setters and getters ommited
    String name;
    Employee boss;
    Address address;
    List<Employee> subordinates;
    String title;
}

class Address implements Entity { // setters and getters ommited
    String street;
}

class EmployeeDto implements Dto { // default constructor, setters and getters ommited
    String name;
    EmployeeDto boss;
    AddressDto address;
    List<EmployeeDto> subordinates;
    String title;

    public EmployeeDto(String name, EmployeeDto boss, AddressDto address) {
        this.name = name;
        this.boss = boss;
        this.address = address;
    }

    public EmployeeDto(String name, AddressDto address, List<EmployeeDto> subordinates) {
        this.name = name;
        this.address = address;
        this.subordinates = subordinates;
    }
}

class AddressDto implements Dto { // default constructor, setters and getters ommited
    String street;
    
    public AddressDto(String street) {
        this.street = street;
    }
}

class PathSegment {
    String name;
    boolean cycleDetected;

    public PathSegment(String name, boolean cycleDetected) {
        this.name = name;
        this.cycleDetected = cycleDetected;
    }
}

class CollectionPathSegment extends PathSegment {
    int index;

    public CollectionPathSegment(String name, boolean cycleDetected) {
        super( name, cycleDetected );
    }
}

class CollectionElementPathSegment extends PathSegment {
    int index;
    public CollectionElementPathSegment(String name, int index, boolean cycleDetected) {
        super( name, cycleDetected );
        this.index = index;
    }
}

class MapContext {
    Set<String> ignoreProps = new HashSet<>();
    Set<Integer> mapped = new HashSet<>();
    Deque<PathSegment> path = new LinkedList<>();
    Deque<String> visited = new LinkedList<>();
    String lastProp = null;

    private String pathToString() {
        StringBuilder strPath = new StringBuilder();
        for ( PathSegment segment : path ) {
            if ( segment instanceof CollectionElementPathSegment ) {
                strPath.append( segment.name );
            } else {
                if ( strPath.length() > 0 ) {
                    strPath.append( "." );
                }
                strPath.append( segment.name );
            }
        }
        if (path.isEmpty()) {
            return lastProp;
        }
        strPath.append( "." );
        strPath.append( lastProp );
        return strPath.toString();
    }

    @Condition
    public boolean collect(@TargetPropertyName String propName) {
        PathSegment segment = path.peekLast();
        if ( ignoreProps.contains( propName ) || (segment != null && segment.cycleDetected)) {
            return false;
        }
        this.lastProp = propName;
        this.visited.offerLast( pathToString() );
        return true;
    }
}

interface FirmMapper<E extends Entity, D extends Dto> {

    E map(D dto, @Context MapContext ctx);

    @BeforeMapping
    default void before(Object dto, @Context MapContext ctx) {
        if ( dto == null ) {
            return;
        }
        boolean cycleDetected = !ctx.mapped.add( dto.hashCode() );

        if ( ctx.lastProp == null ) {
            return;
        }
        if ( dto instanceof Collection ) {
            ctx.path.offerLast( new CollectionPathSegment( ctx.lastProp, cycleDetected ) );
        } else {
            PathSegment segment = ctx.path.peekLast();
            if ( segment instanceof CollectionPathSegment ) {
                ctx.path.offerLast(
                        new CollectionElementPathSegment(
                                "[" + ( (CollectionPathSegment) segment ).index + "]",
                                ( (CollectionPathSegment) segment ).index,
                                cycleDetected
                        )
                );
                ( (CollectionPathSegment) segment ).index++;
            } else {
                ctx.path.offerLast( new PathSegment( ctx.lastProp, cycleDetected ) );
            }
        }
    }

    @AfterMapping
    default void after(Object dto, @Context MapContext ctx) {
        ctx.path.pollLast();
        PathSegment segment = ctx.path.peekLast();
        if ( segment != null ) {
            ctx.lastProp = segment.name;
        } else {
            ctx.lastProp = null;
        }
    }
}

@Mapper
interface EmployeeMapper extends FirmMapper<Employee, EmployeeDto> {
    EmployeeMapper INSTANCE = Mappers.getMapper( EmployeeMapper.class );
}

@Mapper
interface AddressMapper extends FirmMapper<Address, AddressDto> {
    AddressMapper INSTANCE = Mappers.getMapper( AddressMapper.class );
}

// ... more mappers

public void test) {
    EmployeeMapper mapper = EmployeeMapper.INSTANCE;
    MapContext context = new MapContext();
    //context.ignoreProps.add( "boss" ); // glob - like exclude / include could be implemented

    EmployeeDto bossDto = new EmployeeDto( "Boss", new AddressDto("Testing st. 18"), new ArrayList<>() );
    bossDto.subordinates.add( new EmployeeDto( "Employee1", bossDto, new AddressDto("Street 1") ) );
    bossDto.subordinates.add( new EmployeeDto( "Employee2", bossDto, new AddressDto("Street 2") ) );
    bossDto.subordinates.add( new EmployeeDto( "Employee3", bossDto, new AddressDto("Street 3") ) );

    Employee boss = mapper.map( bossDto, context );
    assertThat( List.of(
            "name",
            "boss",
            "address",
            "address.street",
            "subordinates",
            "subordinates[0].name",
            "subordinates[0].boss",
            "subordinates[0].address",
            "subordinates[0].address.street",
            "subordinates[0].subordinates",
            "subordinates[0].title",
            "subordinates[1].name",
            "subordinates[1].boss",
            "subordinates[1].address",
            "subordinates[1].address.street",
            "subordinates[1].subordinates",
            "subordinates[1].title",
            "subordinates[2].name",
            "subordinates[2].boss",
            "subordinates[2].address",
            "subordinates[2].address.street",
            "subordinates[2].subordinates",
            "subordinates[2].title",
            "title"
    ) ).isEqualTo( context.visited );
}

@ivlcic ivlcic changed the title Implemented #2688 #2688: Access to the target property name Apr 28, 2022
Copy link
Contributor

@sjaakd sjaakd left a comment

Choose a reason for hiding this comment

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

@ivlcic I need to review the implementation a bit more thoroughly. But we have a dependsOn in @Mapping.. without digging (yet) to deep into this problem.. Isn't that usable.?

@ivlcic
Copy link
Contributor Author

ivlcic commented May 30, 2022

@ivlcic I need to review the implementation a bit more thoroughly. But we have a dependsOn in @Mapping.. without digging (yet) to deep into this problem.. Isn't that usable.?

I don't really see how this is connected. Basically what I needed was to block contained (sub)collection(s) mapping at runtime based on target property name while reusing same mapper.

I have a REST client which can filter out returned properties from deep nested structures (GraphML like), and this is done via runtime parameters. That's why I can't foresee every possible combination and use different mapper (and hence different compile time @Mapping) for specific desired client response.

btw: I squashed commits on my branch again after review. I guessed that this would be easier to compare to master. I hope I didn't spoil your workflow.

@ivlcic ivlcic force-pushed the target_prop_name branch from 14b5848 to cece0df Compare May 30, 2022 10:34
@filiphr
Copy link
Member

filiphr commented May 30, 2022

I'll need to have a better look into this. However, I would like to thank you for the contribution, the examples look really good (although one might be a bit too complex 😄 ).

I don't really see how this is connected. Basically what I needed was to block contained (sub)collection(s) mapping at runtime based on target property name while reusing same mapper.

I think that @sjaakd wrote that because of the "Complex Example" that you have. Which is doing something really complex, and I do understand why you would need it in your code, we would need to see if we want to keep it in our tests (we would need to scratch our head always when we see it)

btw: I squashed commits on my branch again after review. I guessed that this would be easier to compare to master. I hope I didn't spoil your workflow.

It is all fine, we are squashing everything when we merge anyways. The diff is also complete towards the merge branch in a PR, so it is all good.


Since we are already out with 1.5.0.RC1, I would like to target this towards 1.6 (with hopefully faster release this time)

@ivlcic
Copy link
Contributor Author

ivlcic commented May 31, 2022

I think that @sjaakd wrote that because of the "Complex Example" that you have. Which is doing something really complex, and I do understand why you would need it in your code, we would need to see if we want to keep it in our tests (we would need to scratch our head always when we see it)

I'm sorry for that "Complex example" and similar - derived tests. I'm a complete noob to Mapstruct and I had only two days to implement "runtime object graph path filtering". So I started @TargetPropertyName implementation through tests just to prove to my self, that I can pull it off with a single, minimal and useful change to Mapstruct's code base.

I was trying to convince (you) the contributors why this would be useful and is unachievable with @Mapping annotation and combination of other Mapstruct's features. So I left it in PR's tests. Therefore I have no problem with removing the complex tests (and/or any other code that you don't like for that matter).

You can just leave a comment and I'll fix/remove it.

Copy link
Contributor

@sjaakd sjaakd left a comment

Choose a reason for hiding this comment

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

some small comments after more thorough review.. In general it looks good to me.. Nice job.

@sjaakd
Copy link
Contributor

sjaakd commented May 31, 2022

You can just leave a comment and I'll fix/remove it.

I'm fine with it. I first focused on what I observed (did not have too much time either yesterday) and at first glance I thought it also concerned the order of property-mapping (which is a kind of strange assumption at hindsight admittedly). I can see - in your example - you got your inspiration from the life-cycle avoidance context testcases. I'm also happy to see that this did not make it to the unit testcases.

…not detecting PresenceCheck method.

Fixed @TargetPropertyName tests with more assertions.
Fixed ConditionalMappingTest @TargetPropertyName testing method names.
@ivlcic ivlcic requested review from Zegveld, filiphr and sjaakd June 4, 2022 05:32
Copy link
Contributor

@Zegveld Zegveld left a comment

Choose a reason for hiding this comment

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

It feels to me that there is a lot more pass-through code present then necessary.
The set of changes that I request all focus on the same topic, using the same style as is done for TargetType.
I haven't placed remarks at each location where adjustments might be required.

The tests look good, which should make it easier to refactor this.

@ivlcic ivlcic requested a review from Zegveld June 12, 2022 06:11
Copy link
Contributor

@Zegveld Zegveld left a comment

Choose a reason for hiding this comment

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

Starts to look great, just one issue remaining concerning the way things are handled at the ParameterBindings.

I think it is also good to add a compiler error in case the @TargetPropertyName annotation is used in a method other than one annotated with @Condition. Because that currently is not supported. What do you think, @filiphr?

Keep up the good work, @ivlcic.

@ivlcic ivlcic requested a review from Zegveld June 13, 2022 07:38
@ivlcic ivlcic requested a review from Zegveld June 14, 2022 04:54
Copy link
Contributor

@sjaakd sjaakd left a comment

Choose a reason for hiding this comment

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

Nice piece of work @ivlcic .. thanks!

@filiphr
Copy link
Member

filiphr commented Jun 18, 2022

I think it is also good to add a compiler error in case the @TargetPropertyName annotation is used in a method other than one annotated with @Condition. Because that currently is not supported.

I do not think that we are doing that for @TargetType, not sure if we need to do it for this at the beginning to be honest.

@filiphr
Copy link
Member

filiphr commented Jun 18, 2022

In any case if we need to add custom checks for some annotations we can always add a custom Processor that would validate those. e.g. A non string method parameter annotated with @TargetPropertyName

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.

I finally to the time to have a proper look into this.

Thanks @sjaakd and @Zegveld for the reviews.

@ivlcic, I've pushed some small cleanup changes. Hope you are OK with them.

This is almost ready to be merged. I have 2 things to ask:

  • Can we please add some documentation for this?
  • Can we please move the tests to their own package and dedicated test. e.g. org.mapstruct.ap.test.conditional.targetpropertyname. Would also be good to extract the different implementation of the TargetPropertyNameModel in their own classes. I think that it will make the tests a bit more readable.

ivlcic added 2 commits June 20, 2022 09:02
Moved test domain model classes out of the hiding TargetPropertyNameModel interface.
Renamed TargetPropertyNameModel to DomainModel since it is still used in tests.
@ivlcic
Copy link
Contributor Author

ivlcic commented Jun 20, 2022

  • I've added some (minimal) documentation (chapter 10).
  • I've moved tests to its own package and moved classes as suggested.

@ivlcic
Copy link
Contributor Author

ivlcic commented Jun 20, 2022

@Zegveld, @filiphr, @sjaakd ... Thanks for all your help with this!

One small side note (ticket #2882, commit #2883):
In processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl we pass down targetPropertyName=ext.targetPropertyName and targetType=ext.targetType in <#macro handleSourceReferenceNullCheck>

Correction for targetType=ext.targetType is missing in <#macro handleLocalVarNullCheck needs_explicit_local_var> or it will fail in obscure usage of @TargetType when @Conditional method is defined in @Context variable.

Regards!

@ivlcic
Copy link
Contributor Author

ivlcic commented Jul 11, 2022

I there anything more to be done from my side?

@filiphr
Copy link
Member

filiphr commented Jul 14, 2022

Thanks for your work on this @ivlcic. I think that the only think that is reaming is for us to integrate this into main. I hope to do this as soon as possible.

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.

I did a final pass of this and started to integrate it into main. I have only one question @ivlcic.

Can you please let me know what you think about that?

I'll merge and fix the conflcits afterwards (almost done with that locally)

@filiphr
Copy link
Member

filiphr commented Aug 1, 2022

I've manually merged and polished this a bit in 8fa286f. Thanks a lot for the contribution @ivlcic

@filiphr filiphr closed this Aug 1, 2022
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