GeezerVison

Sorry, I haven't had the time and mental capacity to think this through.
This is tricky, thanks for your effort in trying to tackle this issues.

Users would now have to provide the @SourcePropertyName or @TargetPropertyName just to get the condition to work (maybe like it used to).
So if the property name is unused (which it most likely will be), the IDE and other static code analysis tools will warn the user about the unused parameter. For example, we are currently improving our code quality with SonarQube, and false positives like this are a nuisance.
It also feels like a workaround.

So I would therefore prefer the second approach with the specific annotation, as you said "something like @SourceParameter and / or @TargetProperty".

Whichever way we go, users will still wonder why this is needed (e.g. "why do I have to add something extra?"). So this should be well documented in the docs and release notes.
But I also think that this backwards incompatible change needs to be done, and that this is the right time to do it.

I just played with your branch and saw that a condition written in the old way is simply not used. Is it possible to throw a compile error (or a warning)? (You have better insight into the underlying code)
This would help users migrate the code, rather than a condition that is just not picked up. This can lead to a lot of pain if the mapping code is not thoroughly tested.

Sorry, I haven't had the time and mental complexity to think this through.
This is tricky, thanks for your effort in trying to tackle this issues.

No rush, I've been working on this one for a while, it took me a bit of time to wrap my head properly around it as well. Thanks a lot for going through it.

So I would therefore prefer the second approach with the specific annotation, as you said "something like @SourceParameter and / or @TargetProperty".

That was my gut feeling as well, or perhaps have a different annotation that would be applicable for source parameters condition check.

I just played with your branch and saw that a condition written in the old way is simply not used.

I didn't like that at all as well. I'll look into an alternative for this

@thunderhook, I gave another go at this. Basically, there is a new @SourceCondition (open to better naming suggestions) that should be used for source parameter checks. Let me know what you think about this approach

I really like this approach.

One thing I noticed while playing with the branch: Is there anything against a condition method being a @SourceCondition and a @Condition at the same time? I do not see why this should not be allowed. Maybe there are some scenarios where this is useful.

With the current implementation, the method won't be used if you add a @Condition there:
https://github.com/filiphr/mapstruct/blob/efc374163242119df2b27d83f412a0ce699b7bf9/processor/src/test/java/org/mapstruct/ap/test/conditional/basic/ConditionalMethodForSourceBeanMapper.java#L22

I haven't tried it explicitly, but I don't think it would be picked up as a presence check method either.

Otherwise, I really like it!

You are indeed right. It won't be caught due to the MethodFamilySelector

method.getMethod().isObjectFactory() == criteria.isObjectFactoryRequired()
                && method.getMethod().isLifecycleCallbackMethod() == criteria.isLifecycleCallbackRequired()
                && method.getMethod().isPresenceCheck() == criteria.isPresenceCheckRequired()
                && method.getMethod().isSourceParameterCheck() == criteria.isSourceParameterCheckRequired()

I'll make sure that it works in that case as well.

I should also add some error handlings:

• Using @SourceCondition with @TargetPropertyName or @SourcePropertyName should lead to a compile error

An alternative to the @SourceCondition could be something like:

public enum ConditionStrategy {
    PROPERTIES,
    SOURCE_PARAMETERS,
}

then the condition would be

@Target({ ElementType.METHOD })
@Retention(RetentionPolicy.CLASS)
public @interface Condition {

    ConditionStrategy[] appliesTo() default ConditionStrategy.PROPERTIES;

}

it can then be used

• @Condition(appliesTo = ConditionStrategy.SOURCE_PARAMETERS) - when we want to apply a condition only on source parameters
• @Condition(appliesTo = { ConditionStrategy.PROPERTIES, ConditionStrategy.SOURCE_PARAMETERS}) - when it applied both to properties and source parameters

I am not sure which approach I like more. What do you think @thunderhook

I haven't fully thought this through, but I think the second approach might be more flexible when introducing conditions for other scenarios. Like collection items #1610 or TARGET_PARAMETERS. Don't know if there are other scenarios.

If there are other strategies added, then it might make sense to provide a ConditionStrategy.ALL.

Another idea might be to support both approaches. Where @SourceCondition is a meta-annotation like:

@Condition( appliesTo = ConditionStrategy.SOURCE_PARAMETERS )
public @interface SourceCondition {

}

This would allow users to write their own Condition meta annotations, like:

@Condition( appliesTo = { ConditionStrategy.SOURCE_PARAMETERS, ... } )
public @interface MyOwnEntityCondition {

}

However, this may cause problems if multiple annotations are set. Should it cause a compile error? If not, should they be combined? What has higher priority?

@SourceCondition
@Condition(appliesTo = { ConditionStrategy.PROPERTIES, ConditionStrategy.SOURCE_PARAMETERS})
Target map(Source source);

So maybe it's better to stick to one of these approaches rather than a combination of the two.

Really good points @thunderhook. It would indeed work nicely for #1610 as well. That would basically be an additional ConditionStrategy#ITERABLE_ELEMENTS

Another idea might be to support both approaches. Where @SourceCondition is a meta-annotation like:

I liked this even more. We could indeed provide @SourceParameterCondition or @SourceCondition that would be meta annotated with @Condition(appliesTo = ConditionStrategy.SOURCE_PARAMETERS). Users can define their own annotations if they want to as well.

However, this may cause problems if multiple annotations are set. Should it cause a compile error? If not, should they be combined? What has higher priority?

I don't think that this would cause problems. We would basically just merge all of the applicable appliesTo into a single set.

Thanks a lot for the feedback @thunderhook, I'll try to rework this in that direction :).

@thunderhook I finally found the time to go over what we discussed last time. Can you please have a look at this and let me know what you think

Thanks a lot for the review @thunderhook. All your comments make a lot of sense. I've added some docs, added a new test and also added the class to the <code> tag. Do you think that the doc is enough?

Thanks for the review @thunderhook