Clarify documentation about Condition for update mappings

[Herioz]

When using condition (regardless of the way it has been qualified) with @MappingTarget generated code assign null (ie. default value) in else block although news, documentation and their examples state that mapping will be skipped if condition isn't met. Moreover it doesn't generate any else statement when operating on primitives making it inconsistent.

Tested on both MapStruct 1.5.0.Beta1 and Beta2.
Here is code I've used to produce this behaviour:

public abstract class ExemplaryMapper {

  static class SourceClass {

    private boolean primitiveBoolean;
    private Boolean objectBoolean;

    public boolean isPrimitiveBoolean() {
      return this.primitiveBoolean;
    }

    public Boolean getObjectBoolean() {
      return this.objectBoolean;
    }

    public void setPrimitiveBoolean(boolean primitiveBoolean) {
      this.primitiveBoolean = primitiveBoolean;
    }

    public void setObjectBoolean(Boolean objectBoolean) {
      this.objectBoolean = objectBoolean;
    }
  }

  static class TargetClass {
    private boolean primitiveBoolean;
    private Boolean objectBoolean;

    public boolean isPrimitiveBoolean() {
      return this.primitiveBoolean;
    }

    public Boolean getObjectBoolean() {
      return this.objectBoolean;
    }

    public void setPrimitiveBoolean(boolean primitiveBoolean) {
      this.primitiveBoolean = primitiveBoolean;
    }

    public void setObjectBoolean(Boolean objectBoolean) {
      this.objectBoolean = objectBoolean;
    }
  }

  @Condition
  @ConditionAnnotation
  public boolean condition(boolean opt) {
    return opt;
  }

  @Condition
  @ConditionAnnotationObj
  public boolean conditionObj(Boolean opt) {
    return opt;
  }


  @Qualifier
  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.METHOD)
  public static @interface ConditionAnnotation {
  }

  @Qualifier
  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.METHOD)
  public static @interface ConditionAnnotationObj {
  }

  @Mapping(target = "primitiveBoolean", source = "primitiveBoolean",
      conditionQualifiedBy = ConditionAnnotation.class)
  @Mapping(target = "objectBoolean", source = "objectBoolean",
      conditionQualifiedBy = ConditionAnnotationObj.class)
  public abstract TargetClass map(@MappingTarget TargetClass targetObj, SourceClass sourceObj);

}

And here is the result of generation:

@Override
public TargetClass map(TargetClass targetObj, SourceClass sourceObj) {
    if ( sourceObj == null ) {
        return null;
    }

    if ( condition( sourceObj.isPrimitiveBoolean() ) ) {
        targetObj.setPrimitiveBoolean( sourceObj.isPrimitiveBoolean() );
    }
    if ( conditionObj( sourceObj.getObjectBoolean() ) ) {
        targetObj.setObjectBoolean( sourceObj.getObjectBoolean() );
    }
    else {
        targetObj.setObjectBoolean( null );
    }

    return targetObj;
}

[Zegveld]

Thank you for reporting this.

First off, the reason for the inconsistency can be explained because primitives by design in java cannot be null. So having an else statement with targetObj.setPrimitiveBoolean( null ); will result in compilation errors.

I believe the documentation will need to be updated here to include the else statement, because the condition is a replacement for the null-check.

null-check behavior:
MapStruct does a null-check and if it fails that check it will be set according to the nullValuePropertyMappingStrategy.

In case of conditions we then get the following:
Mapstruct does a condition-check and if it fails that check it will be set according to the nullValuePropertyMappingStrategy.

setting the nullValuePropertyMappingStrategy to IGNORE would remove the else statement.
For example:
@Mapper( nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE )

[Herioz]

Thanks for the reply, I've just came back to office and tested that nullValuePropertyMappingStrategy does the trick.

I was aware that primitive can't be null but it wasn't the main reason why I considered it inconsistent. The real reason/problem is actually using setter at all for objects while for primitives it's skipped. It leads to dichotomy where object are set to their de facto default value while primitives aren't touched (int can be set to it's 'default' ie. 0).
Just calling the setter might have unwanted side effects, like dirtying some context/state or overriding value that shouldn't be modified. It's also more error prone if someone decides to refactor the code, changing from primitive to it's wrapper or vice versa might change the behaviour of mapper in I would say quite unexpected way.

I'm still quite the beginner when it comes to mapstruct but if I could chose I would skip else statement every time by default and activate it by some setting, preferably no nullValuePropertyMappingStrategy as it's name isn't the most obvious in this use case. But that's just my opinion as mere consumer of API not developer of it.
Anyway it would be nice for documentation to explain how to skip else statement and/or give warning/notice about this behaviour.

[Zegveld]

Thanks for the more detailed explanation of your expectations, we will take changing the default strategy into consideration for the 2.0.0 release. Changing this would be a breaking change, therefor not possible with the minor release builds we are doing now.

Also reopening so we can use this issue to fix the documentation :)

[Lautre3091]

Just in case I faced the same issue and the @Mapper( nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE ) clearly do the trick. so thx :)