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

[1.2.0.CR1] Error on mapping generation with multiple sources in target & sub target fields #1269

Closed
Soulangel89 opened this Issue Aug 13, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@Soulangel89

Soulangel89 commented Aug 13, 2017

Hello,

I'm facing an issue with the mapping generation of multiple sources mapped in a target and sub target.
You can find in attachment my case my case with which I have the problem (inspired by the Example 25. Mapper controlling nested beans mappings (FishTankMapperWithDocument) )

mapstruct-nested-bean-mappings-vehicle.zip

public interface VehicleMapper {
  @Mappings({
      @Mapping(source = "vehicleTypeInfo" , target = "vehicleInfo"),
      @Mapping(source = "images" , target = "vehicleInfo.images"),
  })
  VehicleDto map(Vehicle in);
}

The generated mapping:

    @Override
    public VehicleDto map(Vehicle in) {
        if ( in == null ) {
            return null;
        }

        VehicleDto vehicleDto = new VehicleDto();

        vehicleDto.setVehicleInfo( vehicleTypeInfoToVehicleInfoDto( in.getVehicleTypeInfo() ) );
        vehicleDto.setVehicleInfo( vehicleToVehicleInfoDto( in ) );

        return vehicleDto;
    }

     protected VehicleInfoDto vehicleToVehicleInfoDto(Vehicle vehicle) {
        if ( vehicle == null ) {
            return null;
        }

        VehicleInfoDto vehicleInfoDto = new VehicleInfoDto();

        vehicleInfoDto.setImages( vehicleImageListToVehicleImageDtoList( vehicle.getImages() ) );

        return vehicleInfoDto;
    }

As you can see, the second call to setVehicleInfo create a new VehicleInfoDto by setting only images and of course the set erase the previous call to setVehicleInfo .

Solutions:

What I propose but probably tricky is a mean to call a custom mapper:

  //  The mapping I would like to call from the method above
  VehicleInfoDto mapVehicleInfo(VehicleTypeInfo vhInfo, List<VehicleImage> images);

The generation of the mapping above is exactly what I expect but this method is currently not call in the mapping of VehicleToVehicleDTO ( VehicleDto map(Vehicle in) )

public VehicleInfoDto mapVehicleInfo(VehicleTypeInfo vhInfo, List<VehicleImage> images) {
  if(vhInfo == null && images == null) {
    return null;
  } else {
    VehicleInfoDto vehicleInfoDto = new VehicleInfoDto();
    if(vhInfo != null) {
      vehicleInfoDto.setName(vhInfo.getName());
      vehicleInfoDto.setDoors(vhInfo.getDoors());
    }

    if(images != null) {
      vehicleInfoDto.setImages(this.vehicleImageListToVehicleImageDtoList1(images));
    }

    return vehicleInfoDto;
  }
}

protected List<VehicleImageDto> vehicleImageListToVehicleImageDtoList1(List<VehicleImage> list) {
    if ( list == null ) {
        return null;
    }

    List<VehicleImageDto> list1 = new ArrayList<VehicleImageDto>( list.size() );
    for ( VehicleImage vehicleImage : list ) {
        list1.add( vehicleImageToVehicleImageDto( vehicleImage ) );
    }

    return list1;
}

Otherwise, as a basic solution, I expect something like below:

@Override
public VehicleDto map(Vehicle in) {
    if ( in == null ) {
       return null;
    }

    VehicleDto vehicleDto = new VehicleDto();
    out.setVehicleInfo(vehicleTypeInfoToVehicleInfoDto( in.getVehicleTypeInfo() ));
    if (in.getSizedPictures() != null) {
      if (out.getVehicleInfo() == null) {
        out.setVehicleInfo(new VehicleInfoDto());
      }
      out.getVehicleInfo().setImages(vehicleImageListToVehicleImageDtoList1( in.getImages() ) );
    }
  }
  return out;
}

protected VehicleInfoDto vehicleTypeInfoToVehicleInfoDto(VehicleTypeInfo vehicleTypeInfo) {
    if ( vehicleTypeInfo == null ) {
        return null;
    }

    VehicleInfoDto vehicleInfoDto = new VehicleInfoDto();

    vehicleInfoDto.setName( vehicleTypeInfo.getName() );
    vehicleInfoDto.setDoors( vehicleTypeInfo.getDoors() );

    return vehicleInfoDto;
}

protected List<VehicleImageDto> vehicleImageListToVehicleImageDtoList1(List<VehicleImage> list) {
    if ( list == null ) {
        return null;
    }

    List<VehicleImageDto> list1 = new ArrayList<VehicleImageDto>( list.size() );
    for ( VehicleImage vehicleImage : list ) {
        list1.add( vehicleImageToVehicleImageDto( vehicleImage ) );
    }

    return list1;
}

Thank to tell me if both cases are possible or at least the second.

Best regards,
Xavier

@filiphr filiphr added this to the 1.2.0.Final milestone Aug 13, 2017

@filiphr filiphr added the bug label Aug 13, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 13, 2017

Member

Thanks a lot for your reproducer. I'll have a look at it this week.

Regarding the solutions. I would say that the first one is not possible, currently we have no mechanism that can support something like that. The second solution should be the one that we would probably go for. I played around a bit and I managed to get that code generated, but there were some not needed warnings there 😄 .

Member

filiphr commented Aug 13, 2017

Thanks a lot for your reproducer. I'll have a look at it this week.

Regarding the solutions. I would say that the first one is not possible, currently we have no mechanism that can support something like that. The second solution should be the one that we would probably go for. I played around a bit and I managed to get that code generated, but there were some not needed warnings there 😄 .

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 16, 2017

Member

@Soulangel89 I have managed to fix the problem 😄 . The generated mapper will look like the one in this gist.

If you can, can you please try the fix in the #1272 PR on your code as well.

Member

filiphr commented Aug 16, 2017

@Soulangel89 I have managed to fix the problem 😄 . The generated mapper will look like the one in this gist.

If you can, can you please try the fix in the #1272 PR on your code as well.

@Soulangel89

This comment has been minimized.

Show comment
Hide comment
@Soulangel89

Soulangel89 Aug 17, 2017

I checked the gist and it looks fine for what I expected.

I will try to test it in my environment.
It is possible to download your PR with maven ? or I have to check out your code and build it?

Soulangel89 commented Aug 17, 2017

I checked the gist and it looks fine for what I expected.

I will try to test it in my environment.
It is possible to download your PR with maven ? or I have to check out your code and build it?

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 17, 2017

Member

Yes you can try with jitpack. Here is the link, you should just add the jitpack link as a repository. Click on maven there and everything is explained shown 😄

Member

filiphr commented Aug 17, 2017

Yes you can try with jitpack. Here is the link, you should just add the jitpack link as a repository. Click on maven there and everything is explained shown 😄

@Soulangel89

This comment has been minimized.

Show comment
Hide comment
@Soulangel89

Soulangel89 Aug 20, 2017

Hello @filiphr

I confirm it is working as expected :)

Thank you for your quick fix 🥇

Best regards,
Xavier

Soulangel89 commented Aug 20, 2017

Hello @filiphr

I confirm it is working as expected :)

Thank you for your quick fix 🥇

Best regards,
Xavier

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 20, 2017

Member

@Soulangel89 that's great to hear 😄 . Thanks a lot for checking it against your codebase. This will be part of the next release, either 1.2.0.RC2 or 1.2.0.Final. We might go for RC2 due to another issue.

Member

filiphr commented Aug 20, 2017

@Soulangel89 that's great to hear 😄 . Thanks a lot for checking it against your codebase. This will be part of the next release, either 1.2.0.RC2 or 1.2.0.Final. We might go for RC2 due to another issue.

@lex-em

This comment has been minimized.

Show comment
Hide comment
@lex-em

lex-em Aug 24, 2017

@filiphr When the next release is expected?

lex-em commented Aug 24, 2017

@filiphr When the next release is expected?

@lex-em

This comment has been minimized.

Show comment
Hide comment
@lex-em

lex-em Aug 24, 2017

it is the same isuue as #1247, it does not work at now

lex-em commented Aug 24, 2017

it is the same isuue as #1247, it does not work at now

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Aug 25, 2017

Member

@lex-em this was another corner case a bit different than #1247. This is the last open issue so the next release will be once we merge #1272 (I'll do that during this weekend probably). We are going to do another CR2 release, because of another issue

Member

filiphr commented Aug 25, 2017

@lex-em this was another corner case a bit different than #1247. This is the last open issue so the next release will be once we merge #1272 (I'll do that during this weekend probably). We are going to do another CR2 release, because of another issue

@filiphr filiphr closed this in #1272 Aug 27, 2017

filiphr added a commit that referenced this issue Aug 27, 2017

spoerri pushed a commit to spoerri/mapstruct that referenced this issue Nov 26, 2017

# This is a combination of 9 commits.
# This is the 1st commit message:
#1269 use update methods for different sources for nested targets

# This is the commit message #2:

[maven-release-plugin] prepare release 1.2.0.CR2

# This is the commit message #3:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #4:

Switch to OpenJDK 7 on Travis (Oracle Java 7 is no longer present)

# This is the commit message #5:

Run build in VM (fixes issues when build is killed by Travis containers)

# This is the commit message #6:

Fix a few errors in reference guide and readme

# This is the commit message #7:

#1283 Handle no suitable empty constructor during BeanMappingMethod creation

# This is the commit message #8:

#1281 Do not depend on deprecated sonatype parent

# This is the commit message #9:

#1273 Fix for NullValueMappingStrategy#RETURN_DEFAULT when using collections

When mapping a collection using NullValueMappingStrategy#RETURN_DEFAULT and the source is null, the target will be an empty collection.

# This is the commit message #10:

Adding Kevin to copyright.txt

# This is the commit message #1:

#744 Improve support for Java 9

When compilig with Java 9 and and source version 1.8 Elements#getTypeElement(CharSequence) returns the types from all modules (such as java.xml.bind or java.xml.datatype).
However if the required modules are not added the classes cannot be used. Therefore, apart from using the Elements we are also checking if the class is also there.

If source version 9 is used then Elements#getTypeElement(CharSequence) works correctly and does not return the types if the modules are not there

# This is the commit message #2:

#1304 Add thrown exceptions to the generated nested mapping methods

# This is the commit message #3:

Add Darren to copyright.txt

# This is the commit message #4:

[maven-release-plugin] prepare release 1.2.0.Final

# This is the commit message #5:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #6:

Update latest stable badge with 1.2.0.Final

# This is the commit message #7:

#1297 Add IntelliJ Formatter to CONTRIBUTING.md

# This is the commit message #8:

#571 Add Constructor Injection for Annotation Based Component Model

- Allow configuring of injection strategy (Constructor / Field)
- Default injection strategy is Field

# This is the commit message #9:

#571 Add test for default injection strategy

# This is the commit message #10:

#1312 Change MapStruct Version in README to latest 1.2.0.Final

# This is the commit message #1:

#610 Add support for unmappedSourcePolicy

# This is the commit message #2:

#610 remove unmapped source policy processor option

# This is the commit message #3:

610 unmappedSourcePolicy with tests

# This is the commit message #4:

#610 test MapperConfig

# This is the commit message #5:

initial attempt at support for ignoring an unmapped source field

# This is the commit message #6:

revert what pull -Xtheirs didn't

# This is the commit message #7:

revert what pull -Xtheirs didn't

# This is the commit message #8:

revert what pull -Xtheirs didn't

# This is the commit message #9:

revert what pull -Xtheirs didn't

# This is the commit message #10:

revert what pull -Xtheirs didn't

# This is the commit message #1:

revert what pull -Xtheirs didn't

# This is the commit message #2:

revert what pull -Xtheirs didn't

# This is the commit message #3:

revert what pull -Xtheirs didn't

# This is the commit message #4:

Update BeanMappingMethod.java

# This is the commit message #5:

Update IgnoreMapper.java

# This is the commit message #6:

Update UnmappedSourceTest.java

# This is the commit message #7:

Update Mapping.java

# This is the commit message #8:

Update Mapping.java

# This is the commit message #9:

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