New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong prototype types identification for mapper configuration #1255

Closed
leomillon opened this Issue Jul 20, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@leomillon

leomillon commented Jul 20, 2017

Hello,

I have a problem with the prototype definition in MapperConfig since the 1.2.0.Beta2 version.

When I try to reproduce the exemple 65 of the documentation ( Example 65. Mapper configuration class with prototype methods), it works well in 1.2.0.Beta1 but not in the next version (1.2.0.CR1 included).

Here is a sample code to show the problem (Java8, JUnit/AssertJ).

Model :

public abstract class AbstractA {
  private String field1;

  public String getField1() {
    return field1;
  }

  public void setField1(String field1) {
    this.field1 = field1;
  }
}

public class SomeA extends AbstractA {
  private String field2;

  public String getField2() {
    return field2;
  }

  public void setField2(String field2) {
    this.field2 = field2;
  }
}

public class SomeB {
  private String field1;
  private String field2;

  public String getField1() {
    return field1;
  }

  public void setField1(String field1) {
    this.field1 = field1;
  }

  public String getField2() {
    return field2;
  }

  public void setField2(String field2) {
    this.field2 = field2;
  }
}

Mappers and Config :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

Tests :

public class SomeMapperTest {
  @Test
  public void should_map_someB_to_someA_without_field1() throws Exception {
    SomeB someB = new SomeB();
    someB.setField1("value1");
    someB.setField2("value2");

    SomeA someA = SomeMapper.instance().toSomeA(someB);

    assertThat(someA.getField1())
        .isNotEqualTo(someB.getField1())
        .isNull();
    assertThat(someA.getField2()).isEqualTo(someB.getField2());
  }

  @Test
  public void should_map_someA_to_someB() throws Exception {
    SomeA someA = new SomeA();
    someA.setField1("value1");
    someA.setField2("value2");

    SomeB someB = SomeMapper.instance().toSomeB(someA);

    assertThat(someB.getField1()).isEqualTo(someA.getField1()); // Fails because someB.getField1() == null since 1.2.0.Beta2
    assertThat(someB.getField2()).isEqualTo(someA.getField2());
  }
}

So the goal is to map all fields from SomeA to SomeB (field1 + field2) and to map only field2 from SomeA to SomeB.
This should be possible according to the documentation :

AUTO_INHERIT_FROM_CONFIG: the configuration will be inherited automatically, if the source and target types of the target mapping method are assignable to the corresponding types of the prototype method.

The 2 tests are OK in 1.2.0.Beta1, but since 1.2.0.Beta2 the test should_map_someA_to_someB fails with someB.getField1 being null.
So it seems that the prototype does not care about which is the source and the target anymore.

Do you confirm this issue or am I missing something?
Is there any chance to have a fix soon in the next release please?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jul 21, 2017

Member

Thanks a lot for such a detailed write up.

This one is actually a consequence of #1065. It was wrongly assigned to a wrong milestone and it was not in the migration notes (I have fixed this in the meantime), sorry for this.

The problem is that @InheritInverseConfiguration is not also implicitly inherited with that configuration, which and the prototype method is a valid one for reverse inheritance.

As part of #1065 the Javadoc of the MappingInheritanceStrategy was not updated. We need to decide what exactly is the "correct" way to resolve this. It needs to be either documented in the Javadoc better and/or reviewed the implementation.

@gunnarmorling, @agudian, @sjaakd What do you think about this?

Member

filiphr commented Jul 21, 2017

Thanks a lot for such a detailed write up.

This one is actually a consequence of #1065. It was wrongly assigned to a wrong milestone and it was not in the migration notes (I have fixed this in the meantime), sorry for this.

The problem is that @InheritInverseConfiguration is not also implicitly inherited with that configuration, which and the prototype method is a valid one for reverse inheritance.

As part of #1065 the Javadoc of the MappingInheritanceStrategy was not updated. We need to decide what exactly is the "correct" way to resolve this. It needs to be either documented in the Javadoc better and/or reviewed the implementation.

@gunnarmorling, @agudian, @sjaakd What do you think about this?

@filiphr filiphr added this to the 1.2.0.Final milestone Jul 21, 2017

@leomillon

This comment has been minimized.

Show comment
Hide comment
@leomillon

leomillon Jul 27, 2017

Thanks for your quick answer. Is there any news about a potential fix (not only in documentation)?
If this "new" behaviour is the target one, is there any solution to reproduce the previous one with other configuration parameters or mapper definitions?

leomillon commented Jul 27, 2017

Thanks for your quick answer. Is there any news about a potential fix (not only in documentation)?
If this "new" behaviour is the target one, is there any solution to reproduce the previous one with other configuration parameters or mapper definitions?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Jul 27, 2017

Member

@leomillon at this moment we don't have more news. However, a resolution would be made before going 1.2.0.Final.

The only way to reproduce the previous one is to use MappingInheritanceStrategy#EXPLICIT and use the annotations for inheritance, instead of MappingInheritanceStrategy#AUTO_INHERIT_FROM_CONFIG.

Member

filiphr commented Jul 27, 2017

@leomillon at this moment we don't have more news. However, a resolution would be made before going 1.2.0.Final.

The only way to reproduce the previous one is to use MappingInheritanceStrategy#EXPLICIT and use the annotations for inheritance, instead of MappingInheritanceStrategy#AUTO_INHERIT_FROM_CONFIG.

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 17, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 17, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 17, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 18, 2017

Member

@leomillon We are currently working on this issue and we have a working PR #1276 about it.

The thing is that we have something that explicitly allows to have your case and ignore source parameters in the reverse case as well. Basically if a property for the target element method exists in the reversing method than it will be applied.

So this means that in your example the reverse would not be applied. However, if you had both the source and target specified then the ignore would be applied to the reverse method as well (this is my proposal). Another proposal is to completely ignore mappings that have ignore = true for the reversal.

We are going to update the Javadoc as well and document this much better in the release notes.

Which approach is more acceptable in your opinion? What would be clearer to you for example? What would you expect?

Member

filiphr commented Aug 18, 2017

@leomillon We are currently working on this issue and we have a working PR #1276 about it.

The thing is that we have something that explicitly allows to have your case and ignore source parameters in the reverse case as well. Basically if a property for the target element method exists in the reversing method than it will be applied.

So this means that in your example the reverse would not be applied. However, if you had both the source and target specified then the ignore would be applied to the reverse method as well (this is my proposal). Another proposal is to completely ignore mappings that have ignore = true for the reversal.

We are going to update the Javadoc as well and document this much better in the release notes.

Which approach is more acceptable in your opinion? What would be clearer to you for example? What would you expect?

@leomillon

This comment has been minimized.

Show comment
Hide comment
@leomillon

leomillon Aug 21, 2017

I'm not sure to understand exactly your suggestion :s

To simplify my need and to explain what I liked in the previous behaviour, my use case is the following :

With this context :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

In the configuration, I only defined the method signature to have a mapping configuration only from SomeB to AbstractA, so I'm expecting it to only be applied automatically to this mapping (SomeB -> AbstractA). If I want to apply the reverse mapping (AbstractA -> SomeB), I should define it explicitly in the configuration :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  SomeB toSomeB(AbstractA source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

Or maybe using the @InheritInverseConfiguration annotation but still by defining it in the configuration on an explicite method signature :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);

  @InheritInverseConfiguration
  SomeB toSomeB(AbstractA source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

Please tell me if this is not clear enough, maybe I'm not understanding how your mapping strategies are really working but this is how I understood it the first time I used it : it has first to match the defined method signatures (using parameters and return types) and then apply the given mappings in annotations.

Thanks a lot for your time spent on this issue.

leomillon commented Aug 21, 2017

I'm not sure to understand exactly your suggestion :s

To simplify my need and to explain what I liked in the previous behaviour, my use case is the following :

With this context :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

In the configuration, I only defined the method signature to have a mapping configuration only from SomeB to AbstractA, so I'm expecting it to only be applied automatically to this mapping (SomeB -> AbstractA). If I want to apply the reverse mapping (AbstractA -> SomeB), I should define it explicitly in the configuration :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  SomeB toSomeB(AbstractA source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

Or maybe using the @InheritInverseConfiguration annotation but still by defining it in the configuration on an explicite method signature :

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true)
  })
  AbstractA toAbstractA(SomeB source);

  @InheritInverseConfiguration
  SomeB toSomeB(AbstractA source);
}

@Mapper(config = SomeMapperConfig.class)
public interface SomeMapper {

  static SomeMapper instance() {
    return Mappers.getMapper(SomeMapper.class);
  }

  SomeA toSomeA(SomeB source);

  SomeB toSomeB(SomeA source);
}

Please tell me if this is not clear enough, maybe I'm not understanding how your mapping strategies are really working but this is how I understood it the first time I used it : it has first to match the defined method signatures (using parameters and return types) and then apply the given mappings in annotations.

Thanks a lot for your time spent on this issue.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 21, 2017

Member

Thanks for your explanation.

To simplify my need and to explain what I liked in the previous behaviour, my use case is the following :

That's the thing in the previous behaviour when you would have used MappingInheritanceStrategy#EXPLICIT you would have had the exact same problem. This means that it was not consistent and that is why we changed it.

Or maybe using the @InheritInverseConfiguration annotation but still by defining it in the configuration on an explicite method signature :

The entire idea behind MappingInheritanceStrategy#AUTO_INHERIT_FROM_CONFIG has been to automatically inherit configuration without the need to use more annotations.

In the open PR we are discussing how we should resolve this issue.

The solution that we would most probable go is the following:

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true), // will not be automatically reverse
      @Mapping(target = "field2", source = "field3") // will be automatically reversed
  })
  AbstractA toAbstractA(SomeB source);
}
Member

filiphr commented Aug 21, 2017

Thanks for your explanation.

To simplify my need and to explain what I liked in the previous behaviour, my use case is the following :

That's the thing in the previous behaviour when you would have used MappingInheritanceStrategy#EXPLICIT you would have had the exact same problem. This means that it was not consistent and that is why we changed it.

Or maybe using the @InheritInverseConfiguration annotation but still by defining it in the configuration on an explicite method signature :

The entire idea behind MappingInheritanceStrategy#AUTO_INHERIT_FROM_CONFIG has been to automatically inherit configuration without the need to use more annotations.

In the open PR we are discussing how we should resolve this issue.

The solution that we would most probable go is the following:

@MapperConfig(
    mappingInheritanceStrategy = MappingInheritanceStrategy.AUTO_INHERIT_FROM_CONFIG
)
public interface SomeMapperConfig {

  @Mappings({
      @Mapping(target = "field1", ignore = true), // will not be automatically reverse
      @Mapping(target = "field2", source = "field3") // will be automatically reversed
  })
  AbstractA toAbstractA(SomeB source);
}
@leomillon

This comment has been minimized.

Show comment
Hide comment
@leomillon

leomillon Aug 21, 2017

This result is quite unexpected and not natural oO. I don't see the point to have different behaviours between mappings of the same method.

My understanding with the word AUTO_INHERIT_FROM_CONFIG is that it will "pre-define" mappings, so if there is no reverse mapping (by annotation or implicit method signature) there is no need to enable it.

But with this solution, if I don't want to enable the reverse mapping, then I have to explicitly annotate all the mappers' methods (that are using this configuration) to inherit the only method available... It's kind of overkill.

If you really want it to be automatic, couldn't you just add a new strategy for auto inherit including the reverse mapping, and the user will be able to select the behaviour?

leomillon commented Aug 21, 2017

This result is quite unexpected and not natural oO. I don't see the point to have different behaviours between mappings of the same method.

My understanding with the word AUTO_INHERIT_FROM_CONFIG is that it will "pre-define" mappings, so if there is no reverse mapping (by annotation or implicit method signature) there is no need to enable it.

But with this solution, if I don't want to enable the reverse mapping, then I have to explicitly annotate all the mappers' methods (that are using this configuration) to inherit the only method available... It's kind of overkill.

If you really want it to be automatic, couldn't you just add a new strategy for auto inherit including the reverse mapping, and the user will be able to select the behaviour?

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Aug 21, 2017

Contributor

If you really want it to be automatic, couldn't you just add a new strategy for auto inherit including the reverse mapping, and the user will be able to select the behaviour?

I think that's a reasonable suggestion. So, we should probably have something like the current: AUTO_INHERIT_FROM_CONFIG (forward), AUTO_INHERIT_REVERSE_FROM_CONFIG (backward) and AUTO_INHERIT_BOTH_FROM_CONFIG.

My idea of reverse / forward mappings is that they should work in a similar. That's why its possible to define reverse prototype methods as well now. I did not foresee this side effect when implementing it.

Also: I'm still of the opinion we should remove the possibility to have source-target-ignore in one combination (and the ignore when names match) as proposed in the PR.. This seems to break the clean separation of normal and reverse mapping inheritance.

@filiphr , @leomillon what do you think ?

I'm going to work on the implementation.

Contributor

sjaakd commented Aug 21, 2017

If you really want it to be automatic, couldn't you just add a new strategy for auto inherit including the reverse mapping, and the user will be able to select the behaviour?

I think that's a reasonable suggestion. So, we should probably have something like the current: AUTO_INHERIT_FROM_CONFIG (forward), AUTO_INHERIT_REVERSE_FROM_CONFIG (backward) and AUTO_INHERIT_BOTH_FROM_CONFIG.

My idea of reverse / forward mappings is that they should work in a similar. That's why its possible to define reverse prototype methods as well now. I did not foresee this side effect when implementing it.

Also: I'm still of the opinion we should remove the possibility to have source-target-ignore in one combination (and the ignore when names match) as proposed in the PR.. This seems to break the clean separation of normal and reverse mapping inheritance.

@filiphr , @leomillon what do you think ?

I'm going to work on the implementation.

@leomillon

This comment has been minimized.

Show comment
Hide comment
@leomillon

leomillon Aug 22, 2017

@sjaakd I think this is the best option. With theses strategies, there will be no magic/surprise : the intention is clear.

I also agree with your point about the removing "source-target-ignore" feature.

Thx a lot @filiphr and @sjaakd for your actions!

leomillon commented Aug 22, 2017

@sjaakd I think this is the best option. With theses strategies, there will be no magic/surprise : the intention is clear.

I also agree with your point about the removing "source-target-ignore" feature.

Thx a lot @filiphr and @sjaakd for your actions!

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 22, 2017

Member

@sjaakd I completely agree with you. We can add those 2 new strategies so it is more clear to the users what is doing what. I think that it won't be that difficult to do this as we are only doing it on one place.

Regarding the source-target-ignore, I am a bit torn, this can theoretically break something that worked before. However, I am OK with removing it.

Regarding the opinion from @gunnarmorling, I think that you can write him an email (as he is on holidays and I doubt he'll see it here 😄 )

Member

filiphr commented Aug 22, 2017

@sjaakd I completely agree with you. We can add those 2 new strategies so it is more clear to the users what is doing what. I think that it won't be that difficult to do this as we are only doing it on one place.

Regarding the source-target-ignore, I am a bit torn, this can theoretically break something that worked before. However, I am OK with removing it.

Regarding the opinion from @gunnarmorling, I think that you can write him an email (as he is on holidays and I doubt he'll see it here 😄 )

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Aug 22, 2017

Contributor

Regarding the source-target-ignore, I am a bit torn, this can theoretically break something that worked before. However, I am OK with removing it.

Lets remove the automatic name based ignore then. Because that's confusing. Lets leave source-target-ignore for later discussion and bring out a CR2. That buys room in time and we can discuss in an hangout.

Contributor

sjaakd commented Aug 22, 2017

Regarding the source-target-ignore, I am a bit torn, this can theoretically break something that worked before. However, I am OK with removing it.

Lets remove the automatic name based ignore then. Because that's confusing. Lets leave source-target-ignore for later discussion and bring out a CR2. That buys room in time and we can discuss in an hangout.

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Aug 22, 2017

Contributor

Or.. We could also do this (source-target-ignore) in a later release. Its there for years already.

Contributor

sjaakd commented Aug 22, 2017

Or.. We could also do this (source-target-ignore) in a later release. Its there for years already.

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 22, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 22, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 23, 2017

Member

Or.. We could also do this (source-target-ignore) in a later release. Its there for years already.

This is OK I think. We can remove the confusing one. Although now with the multiple choices it should be clearer.

Member

filiphr commented Aug 23, 2017

Or.. We could also do this (source-target-ignore) in a later release. Its there for years already.

This is OK I think. We can remove the confusing one. Although now with the multiple choices it should be clearer.

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit to sjaakd/mapstruct that referenced this issue Aug 23, 2017

sjaakd added a commit that referenced this issue Aug 23, 2017

@sjaakd

This comment has been minimized.

Show comment
Hide comment
@sjaakd

sjaakd Aug 23, 2017

Contributor

Should be fixed now by PR #1276 . @leomillon thanks for reporting. If you want, please test the latest master.

Contributor

sjaakd commented Aug 23, 2017

Should be fixed now by PR #1276 . @leomillon thanks for reporting. If you want, please test the latest master.

@sjaakd sjaakd closed this Aug 23, 2017

@leomillon

This comment has been minimized.

Show comment
Hide comment
@leomillon

leomillon Aug 24, 2017

It's working perfectly :)

Thx a lot @filiphr and @sjaakd for these improvements!

leomillon commented Aug 24, 2017

It's working perfectly :)

Thx a lot @filiphr and @sjaakd for these improvements!

@filiphr filiphr added bug and removed for:team-discussion labels Aug 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment