Not all `@AfterMapping` methods executed when using record

[anno1985]

Expected behavior

When several @AfterMapping methods are applicable, they should effectively be chained by the mapper (cf. example).

Actual behavior

Mapper backs out of calling subsequent @AfterMapping methods when any returns non-null value.

Steps to reproduce the problem

import lombok.Data;
import lombok.With;
import org.junit.jupiter.api.Test;
import org.mapstruct.AfterMapping;
import org.mapstruct.Mapper;
import org.mapstruct.MappingTarget;
import org.mapstruct.factory.Mappers;

import static org.junit.jupiter.api.Assertions.assertTrue;

/**
 * <pre>
 *     <org.mapstruct.version>1.6.3</org.mapstruct.version>
 *     <org.projectlombok.version>1.18.36</org.projectlombok.version>
 * </pre>
 */
public class FooTest {

    @Test
    void test() {
        Bar bar = new Bar();
        bar.setMsg("foo");
        Foo foo = Mappers.getMapper(FooTest.FooMapper.class).toFoo(bar);
        assertTrue(foo.msg().contains("foo"), "foo missing");
        assertTrue(foo.msg().contains("bar"), "bar missing");
        assertTrue(foo.msg().contains("baz"), "baz missing");
    }

    @Mapper
    interface FooMapper {

        /**
         * <blockquote cite="https://mapstruct.org/documentation/stable/reference/html/#customizing-mappings-with-before-and-after">All before/after-mapping methods that can be applied to a mapping method will be used.</blockquote>
         * Actual: Generated code does not call all @AfterMapping methods if any returns non-null value:
         * <pre>
         *         FooTest.Foo foo1 = new FooTest.Foo( foo );
         *
         *         FooTest.Foo target = addBar( foo1, bar );
         *         if ( target != null ) {
         *             return target;
         *         }
         *         FooTest.Foo target1 = addBaz( foo1, bar );
         *         if ( target1 != null ) {
         *             return target1;
         *         }
         *
         *         return foo1;
         * </pre>
         * Expected: method calls are chained (order of calls not guaranteed):
         * <pre>
         *         FooTest.Foo foo1 = new FooTest.Foo( foo );
         *
         *         FooTest.Foo target = addBar( foo1, bar );
         *         if ( target != null ) {
         *             foo1 = target;
         *         }
         *         FooTest.Foo target1 = addBaz( foo1, bar );
         *         if ( target1 != null ) {
         *             foo1 = target1;
         *         }
         *
         *         return foo1;
         * </pre>
         */
        Foo toFoo(Bar bar);

        @AfterMapping
        default Foo addBar(@MappingTarget Foo foo, Bar bar) {
            // assume bar is used to calculate the new message
            return foo.withMsg(foo.msg() + "bar");
        }

        @AfterMapping
        default Foo addBaz(@MappingTarget Foo foo, Bar bar) {
            // assume bar is used to caculate the new message
            return foo.withMsg(foo.msg() + "baz");
        }

    }

    record Foo(@With String msg) {
    }

    @Data
    static class Bar {
        private String msg;
    }
}

MapStruct Version

1.6.3

[thunderhook]

Hi @anno1985!

I stumbled across this, while implementing the feature, see this comment: #3267 (review)

@filiphr WDYT? Is this a valid change request? You mentioned that...

We have clearly documented how things work when something is returned from a lifecycle method, so it is completely valid to use it like that if someone wants to do it.

I couldn't find it, can you point me to the documentation? Otherwise we could adjust the reference documentation.

[anno1985]

I have looked at the docs again, and I think this is the relevant bit:

For non-void methods, the return value of the method invocation is returned as the result of the mapping method if it is not null.

I think I might have skipped past it because it's visually separated from what I thought was the relevant part of the reference docs (cf. above) by a code sample block.

That quote can also be found in the JavaDocs for @AfterMapping - where I didn't look initially because I had understood the reference docs differently.

[anno1985]

I guess that makes this more of a feature request than a bug - but I think it's somewhat unfortunate that when changing from mutable to immutable target objects (where you necessarily have to return from after mapping lifecycle functions) the way these functions work (no return value: "chainable"; with returned object: unchainable) changes. It's unexpected, even if it's documented.

[filiphr]

@anno1985 I am not sure what is expected here. You are expecting something, someone else might be expecting something else. This is clearly documented.

The way that MapStruct is done in the moment is that it is assuming that the target is mutable, either by using a builder or the target itself is mutable. Your example looks to me like it is using immutable approach for changing the target.

I am going to close this for now as something like in your example is something that we do not support at the moment and I there are no plans to support it in the near feature

[filiphr]

@thunderhook do you think that there is something that we can improve in the documentation?

[thunderhook]

Maybe a note/warning block with something like this?

@AfterMapping is best suited for mutating the mapping target (@MappingTarget). With mutable objects, all @AfterMapping methods are applied sequentially. For immutable objects, if an @AfterMapping method returns a new instance, it replaces the target, and subsequent @AfterMapping methods will not be invoked. Chaining transformations requires mutable targets.

I'm struggling with the wording to keep it short, but more visible, feedback is welcome!