@AfterMapping not called for target beans with builder

[nils4cosee]

• Is this an issue (and hence not a question)?

I tried to map a class that has uses a builder-method for bean creation and noticed the @AfterMapping method is not called. A minimal example is here: (repository deleted)

The example does not use any lombok annotations to omit side-effects from other annotations processors, but we also saw the issue when using lombok's @Builder annotation. In such a case, the issue may depend on which annotation processor runs when.

#1433 sounds like the same issue, and should thus be solved in version 1.3.0.FINAL, but our example shows that it is not solved completely.

[sjaakd]

Did you try passing the builder in stead of the target?

[nils4cosee]

Passing the builder works and when I think about it, it makes more sense to use the builder than what I tried.

Is this the preferred way? It's just that I could not find any mention of it in the docs.

[sjaakd]

Is this the preferred way? It's just that I could not find any mention of it in the docs.

It is the only way. IMHO Builders are used ICW immutable objects. I think it is bad practice to allow setters for fields that are also build. We don't support mixed objects (partly mutable) yet.

The question has popped up earlier, so I reckon we need to update the documentation in that particular area.

[sjaakd]

See #1864 for doc update

[nils4cosee]

Thanks for this clarification. I certainly agree with you. From my point, feel free to close the issue.

[sjaakd]

No problem.. have a look at the referenced PR if that suffices. I'll wait for the review until closing this one.

[filiphr]

@nils4cosee We also have #1454 which still has some points open.

I'll close this since it is the same as point

@AfterMapping with @MappingTarget Person

[erlioniel]

Hello @sjaakd , sorry for resurrecting this thread, but I want to clarify the situation a bit.

The main question is why mapstruct team is assuming that @Builder annotation is only for immutable objects? There is nothing related to this in the lombok documentation, so basically it's just an utility which is generating a builder for a class.

I wouldn't ask you if I didn't have a situation, when this behaviour ruined code in unpredictable way. We had a data class used in mapper with a few @AfterMapping methods. And one developer decided to add a @Builder annotation to it. The new mapper's code didn't contain any call of @AfterMapping methods (which is obvious, after your explanation) and wasn't working correctly. The problem in this case that the behavior of the mapstruct changed dramatically and silently after adding only one annotation which isn't connected to mapstruct anyhow.

I don't think that using @Builder annotation as a duck-typing for immutable data classes is a good idea. But even if you did so why the compilation isn't failing if @AfterMapping method is contain not a builder class? It's obvious that it won't be called anyhow by generated code and most probably is pointing a code design issue.

[sjaakd]

The main question is why mapstruct team is assuming that @Builder annotation is only for immutable objects? There is nothing related to this in the lombok documentation, so basically it's just an utility which is generating a builder for a class.

Lombok is just a technology that helps you with you with a deficiency in java the way I see it. It generates getters / setters / equals / constructors / etc. If you look at Kotlin, the same vm is used, you can cross compile with java and there's a solution out of the box. There are many more of course that solve part of this puzzle, like AutoValue, Immutables, etc.

Its not Lombok's responsibility to explain in which scenarios to use a supported feature. Having said that, there are some good articles when to use a builder.My favourite one is this one on the AutoValue solution: it coins a name for immutable objects ValueObjects.

I wouldn't ask you if I didn't have a situation, when this behaviour ruined code in unpredictable way. We had a data class used in mapper with a few @AfterMapping methods. And one developer decided to add a @Builder annotation to it.

At one point in time we decided to make the Builder the default mechanism. That was a bit unfortunate, since we don't support mixed scenarios (objects partly immutable). The code at our current master branch allows for switching of the builder discovery on all levels. Try it and hopefully it fixes your issue in the mapper.

The point is: when a framework (such as Lombok) offers a possibility to generate stuff, people tend to use it it all without questioning whether that's a good or a bad idea. For instance, people use it in unit test to construct test data in a fluent way. Conceptually that means that you're introducing test facilities in your code that you don't / or are not suppose to use otherwise. This becomes tricky, especially when you have many designers working on the same code bas. Personally, I think that's a bad practice.

I don't think that using @builder annotation as a duck-typing for immutable data classes is a good idea.

Builders are a mechanism to create objects that should not be changed anymore during its life time (for the fields that are in the builder). It has advantages over direct use of a constructor (for instance when constructor arguments are of the same type and its easy to confuse the order, or when you have many constructor arguments). It's something that you as a user of that object should be able to rely on.

But even if you did so why the compilation isn't failing if @AfterMapping method is contain not a builder class? It's obvious that it won't be called anyhow by generated code and most probably is pointing a code design issue.

But even if you did so why the compilation isn't failing if @AfterMapping method is contain not a builder class? It's obvious that it won't be called anyhow by generated code and most probably is pointing a code design issue.

That's an interesting point. It's not that simple. Annotation processors work by selecting an annotation. Our point of entry is @Mapper: that's what we scan for. The whole tree is derived from that. It means, it's (harder) to detect stuff that is not used / referred by the class indicated with @Mapper. Effectively it means that we also have tho scan for @AfterMapping, @BeforeMapping, @ObjectFacgtory and cross-reference those with all the ones we found in @Mapper. It is doable of course, but not simple.

Perhaps you could scan if there's already an issue on that one and add this stuff to it. If you're interested, it is an open source project and we're always looking for good PR's and for people that write them 😄.

[erlioniel]

Thank you for the explanation.

I agree that mutating code in order to make it more testable is very discussable approach, but we both know that it's usually the case. What I'm bothered about is that this behavior of the library making changes in a code unpredictable.

That's an interesting point. It's not that simple. Annotation processors work by selecting an annotation. Our point of entry is @Mapper: that's what we scan for. The whole tree is derived from that. It means, it's (harder) to detect stuff that is not used / referred by the class indicated with @Mapper. Effectively it means that we also have tho scan for @AfterMapping, @BeforeMapping, @ObjectFacgtory and cross-reference those with all the ones we found in @Mapper. It is doable of course, but not simple.

I'm more or less aware how the annotation processors work, but aren't you are already scanning for @AfterMapping etc. annotations in order to generate a mapping method in the mapper? I think it might be a good point to check validity of it.

Perhaps you could scan if there's already an issue on that one and add this stuff to it. If you're interested, it is an open source project and we're always looking for good PR's and for people that write them 😄.

A good point :) I think the first step should be to register a correct issue in the tracker if you think that it will be a constructive change. Don't feel myself able to determine how it should be done properly.

[sjaakd]

I'm more or less aware how the annotation processors work, but aren't you are already scanning for @AfterMapping etc. annotations in order to generate a mapping method in the mapper? I think it might be a good point to check validity of it.

Yes, but we get to an @AfterMapping via the @Mapper, this is the annotation we scan for primarily.

[erlioniel]

Yes, but we get to an @AfterMapping via the @Mapper, this is the annotation we scan for primarily.

As far as I remember @AfterMapping outside of the @Mapper won't work in any case. I don't think that it's a big issue, but when @AfterMapping inside @Mapper isn't called without any reason (because code was generated with a Builder instead of setters approach) might be very tricky to find and understand for the end user.

[sjaakd]

That's not what I meant. The starting point of annotation processor are classes annotated with the @Mapper annotation. Only types used direct or indirect by these classes are analysed. The problem here is that it concerns methods which are by definition not referenced

[nils4cosee]

At some point, mapstruct has to find the @AfterMapping method that has to be called from the generated mapping method, doesn't it?

When it does that and it finds a method that takes the target-class as @MappingTarget instead of the Builder-class, then it could issue a warning.

I have to admit that the code is slightly to big for me to grasp everything at once, but it seems to me that the org.mapstruct.ap.internal.model.source.selector.TypeSelector class should come across the wrongly defined @AfterMapping. Maybe it can see that its parameter type is the class built by the builder-class it is actually looking for.

[sjaakd]

@erlioniel , @nils4cosee : I have been thinking a bit more on this. It is tricky to find non consumed @AfterMapping, @BeforeMapping and @ObjectFactory..

Consider this scenario:

@Mapper( uses = UtilMapper.class )
public interface MapperAB {
      TypeA map(TypeB b);
}

@Mapper( uses = UtilMapper.class )
public interface MapperXY {
      TypeX map(TypeY y);
}

public class UtilMapper {

    @AfterMapping
   void afterAB(TypeB source, @MappingTarget TypeA target) {
      // do something after
   }

    @AfterMapping
   void  afterXY(TypeY source, @MappingTarget TypeX target) {
      // do something after
   }
}

As you can see, afterXY will never be consumed by MapperAB and likewise afterAB will never be consumed by MapperXY. Still they are both available for both mappers.

So to detect if this @AfterMapping is consumed would mean keeping some global state somewhere over mapper generation. That is probably possible.

But what if the UsedMapper is in another jar?

[erlioniel]

@sjaakd I think the main idea not to point all unused @AfterMappers, it's most probably not possible at all. But to point @AfterMappers which might be applied to target class, but won't be used because of builder. As an example in the code below that fact that afterMN will not be marked is OK. Because most probably when the code will be developed it will be tested and the mapping problem will be visible. But is it possible to atleast detect afterKL as a wrong one?

@Builder
public TypeK {
}

@Mapper
public abstract class ExampleMapper {
	abstract TypeK map(TypeL l);
	
	@AfterMapper
	void afterKL(TypeK source, @MappingTarget TypeL target) {
		// something
	}
	
	@AfterMapper
	void afterMN(TypeM source, @MappingTarget TypeN target) {
		// something
	}
}

[sjaakd]

But is it possible to atleast detect afterKL as a wrong one?

Probably.. But to be honest here, I expected life cycle methods to be triggered which weren't. It would be nice if we could detect more generic scenarios than this specific one.

BTW.. Did you spot 1.3.1 is released? That offers the possibility to switch off builders. That might solve your direct issue.

[filiphr]

@erlioniel what you are asking for is part of #1454 which is for supporting exactly the problem you encountered.

I would say coming up with a generic solution for not triggered lifecycle methods is not easy. I would say that the easiest way to spot this is to have your own tests. You know what your mapping does and if you test that the output is OK then all would be fine.

[erlioniel]

@sjaakd thank you, I'll check

[erijonhson]

Using org.mapstruct:mapstruct-processor:1.4.2.Final and lombok and it's works fine:

@Builder
public TypeK {
}

@Mapper(builder = @Builder(disableBuilder = true))
public abstract class ExampleMapper {
	abstract TypeK map(TypeL l);
	
	@AfterMapper
	void afterKL(@MappingTarget TypeK target, TypeL source) {
		// something
	}
}