mapstruct-examples icon indicating copy to clipboard operation
mapstruct-examples copied to clipboard

mapstruct-mapper-repo example bug

Open everettwolf opened this issue 5 years ago • 4 comments

in testMapObjectToObject add: carDto.setSetCount(7); and Assert.assertEquals(5, carDto.getSeatCount()); test will fail

Also, despite the annotation @Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE) on CarMapper: in testMapObjectToObject change Car car = new Car("Morris", 5, CarType.SPORTS); to Car car = new Car("Morris", 5, null); and add carDto.setType("anything"); and Assert.assertEquals("anything", carDto.getType()); test will fail

For the latter, I tried to make sense of your recent change of "don't overwrite target if source is null" pull request, but there was so much discussion on what the annotation should be, I couldn't easily figure out the end result. ;o)

I also can't find a test case in this git repo that covers it. Can you point me to it, if there is one? Thanks!

everettwolf avatar Apr 10 '20 16:04 everettwolf

@everettwolf I don't think I understand what you are trying to do?

Can you provide a test case with what is wrong? The NullValuePropertyMappingStrategy is only applied for updated mappings. Basically for

https://github.com/mapstruct/mapstruct-examples/blob/974fb1d3a7f0a2a8101513f27978f5534c80de39/mapstruct-mapper-repo/generator/src/main/java/org/mapstruct/example/repo/StandardMapper.java#L29

which the CarMapper from the test inherits from. The test you are referring to tests only the map method

filiphr avatar Apr 10 '20 16:04 filiphr

Hey @filiphr thanks for the reply! For the former, I think it's just a bug in your example code. I believe the expectation of the mapper is that if the source -- in this case Car -- has a different type and a different value that CarDto, the values should get updated on Target as the target datatype. That IS the case for brand and type, but not for seat count. Maybe it's having a difficult time both translating the datatype and updating the value. I believe it'd be self-explanatory if you add what I put in my original comment into the existing test and just run it.

For the latter, my use case is this (and this was what I was trying to solve when I stumbled over the above): Source{ String stringone; String stringtwo; } Target { String iamstringone; String iamstringtwo; } So, same datatypes, different fieldnames. I want to map the values from a Source to an existing Target. So source could look like this: stringone="foo" stringtwo=null

and Target could look like this: iamstringone="hello" iamstringtwo="goodbye"

After mapping the source to the target, I would want Target to look like this: iamstringone="foo" iamstringtwo="goodbye"

IOW, any field where the source property is null, the equivalent target property is NOT updated.

I saw a few people asking about this, and a response in the form of this PR: https://github.com/mapstruct/mapstruct/pull/1618

However, I can't get it to work with the latest version, and the examples provided in this repo don't appear to cover it.

It's no big deal; I was hoping to drop mapstruct into a project I'm working on to solve the use case I mentioned, but I ended up just creating my own mapping annotations, and it works fine.

Thanks!!

everettwolf avatar Apr 10 '20 20:04 everettwolf

Just took a quick look (at home w/ the kids due to Covid, so it's a bit tough ;o))

For mapstruct-mapper-repo, this is what the generated Implementation of the CarMapper looks like: public class CarMapperImpl implements CarMapper {

@Override
public CarDto update(Car arg0, CarDto arg1) {
    if ( arg0 == null ) {
        return null;
    }

    if ( arg0.getMake() != null ) {
        arg1.setMake( arg0.getMake() );
    }
    if ( arg0.getType() != null ) {
        arg1.setType( arg0.getType().name() );
    }

    return arg1;
}

@Override
public CarDto map(Car CarDto) {
    if ( CarDto == null ) {
        return null;
    }

    CarDto carDto = new CarDto();

    carDto.setSeatCount( CarDto.getNumberOfSeats() );
    carDto.setMake( CarDto.getMake() );
    if ( CarDto.getType() != null ) {
        carDto.setType( CarDto.getType().name() );
    }

    return carDto;
}

}

Note that the mapping method includes the seatCount conversion, but the update method does not. If this is by design, can you explain why?

Thanks!

everettwolf avatar Apr 10 '20 22:04 everettwolf

Hmm, thought I was on to something: numberOfSeats and seatCount (in Car & CarDto) were a primitive (int) so the null checking annotation wouldn't be able to do anything with that, so thinking the generator realizes that and doesn't add primitives to the update method in this scenario.

However, I changed both to Integer and regenerated (by the way, have to delete the target directory to get that kind of change picked up on a mvn clean compile), and still the same issue. I can go in and modify the generated output to do what I want, but that wouldn't be very elegant.

I'll just stop and let you respond, thus far; I'm probably missing something.

Not sure you'd want to embark on an Autoboxing/Unboxing rabbit hole to work around the "check for null on source" issue.

everettwolf avatar Apr 10 '20 23:04 everettwolf