nullValueIterableMappingStrategy in 1.5.0.RC1 not working properly

I've played around with mapstruct and I have come across a bug about initializing arrays. The following is taken from your guide

However, MapStruct also offers a more dedicated way to control how collections / maps should be mapped. e.g. return default (empty) collections / maps, but return null for beans.
For collections (iterables) this can be controlled through:
MapperConfig#nullValueIterableMappingStrategy

so I figured that if nullValueIterableMappingStrategy is set to NullValueMappingStrategy.RETURN_NULL then this piece of code

public class TestVo {

  private String id;
  private List<String> list = new ArrayList<>();
}

public class TestEntity {

  private String id;
  private List<String> list = new ArrayList<>();
}

@Mapper(
    componentModel = "cdi",
    nullValueIterableMappingStrategy = NullValueMappingStrategy.RETURN_NULL)
@ApplicationScoped
public interface TestMapper {

  TestEntity testVoToTest(TestVo testVo);
}

would conclude to the arraylist if null to initialize with empty collection. Instead this code is generated

@Override
public TestEntity testVoToTest(TestVo testVo) {
    if ( testVo == null ) {
        return null;
    }

    TestEntity.TestEntityBuilder testEntity = TestEntity.builder();

    testEntity.id( testVo.getId() );
    List<String> list = testVo.getList();
    if ( list != null ) {
        testEntity.list( new ArrayList<String>( list ) );
    }

    return testEntity.build();
}

Is this expected? Doesn't the generated code supposed to have an else after the if (list != null)???

The reason why this is not working as expected is because this is using a direct assignment. We are going to have a similar problem for direct assignment of beans as well.

If I change the list to List<Integer> in own of the beans then it works as expected.

Not sure what the best approach for fixing this would be.

We need to investigate what happens when we NullValueMappingStrategy.RETURN_DEFAULT. If that one maps to an empty list then everything works as expected most likely.

I've tried this :

@Mapper(
        componentModel = "spring",
        injectionStrategy = InjectionStrategy.CONSTRUCTOR,
        nullValueIterableMappingStrategy = NullValueMappingStrategy.RETURN_DEFAULT,
        nullValueMappingStrategy = NullValueMappingStrategy.RETURN_DEFAULT,
        unmappedTargetPolicy = ReportingPolicy.IGNORE
)
public interface ClientDocumentMapper {


    @Mapping(target = "labels", expression = "java(List.of())")
    List<ClientDocumentDto> to(List<ClientDocument> clientDocuments);
}

And it doesn't work, that produce this:

@Component
public class ClientDocumentMapperImpl implements ClientDocumentMapper {

    @Override
    public List<ClientDocumentDto> to(List<ClientDocument> clientDocuments) {
        if ( clientDocuments == null ) {
            return new ArrayList<ClientDocumentDto>();
        }

        List<ClientDocumentDto> list = new ArrayList<ClientDocumentDto>( clientDocuments.size() );
        for ( ClientDocument clientDocument : clientDocuments ) {
            list.add( clientDocumentToClientDocumentDto( clientDocument ) );
        }

        return list;
    }

    protected ClientDocumentDto clientDocumentToClientDocumentDto(ClientDocument clientDocument) {

        ClientDocumentDto.ClientDocumentDtoBuilder clientDocumentDto = ClientDocumentDto.builder();

        if ( clientDocument != null ) {
            clientDocumentDto.id( clientDocument.getId() );
            clientDocumentDto.name( clientDocument.getName() );
            clientDocumentDto.displayName( clientDocument.getDisplayName() );
            List<Label> list = clientDocument.getLabels();
            if ( list != null ) {
                clientDocumentDto.labels( new ArrayList<Label>( list ) );
            }
            clientDocumentDto.sharedBy( clientDocument.getSharedBy() );
            clientDocumentDto.sharedAt( clientDocument.getSharedAt() );
            clientDocumentDto.ownerId( clientDocument.getOwnerId() );
        }

        return clientDocumentDto.build();
    }
}

That is using MapStruct 1.4.2

I just realised that ClientDocumentDto to(ClientDocument clientDocument);was missing...

But still, using the strategy didn't produce en empty ArrayList for labels.

I had to use this strategy to replace the null array by an empty array. :

    @Named("ensureLabelsNotNull")
    static List<Label> ensureLabelsNotNull(List<Label> labels) {
        return Objects.isNull(labels) ? List.of() : labels;
    }

    @Mapping(target = "labels", qualifiedByName = "ensureLabelsNotNull")
    ClientDocumentDto to(ClientDocument clientDocument);

Is this resolved? Is there a newer version to cover this case?

I feel like OP's repro is muddied a bit due to the default value shenanigans. The issue is that NullValueMappingStrategy.RETURN_NULL most definitely does not work 'as expected'. Minimum reproduction:

import org.mapstruct.Mapper;
import org.mapstruct.NullValueCheckStrategy;
import org.mapstruct.NullValueMappingStrategy;

import java.util.List;

@Mapper(nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS,
        nullValueIterableMappingStrategy = NullValueMappingStrategy.RETURN_NULL)
public interface TestMapper {

    TestClass2 map(TestClass1 testClass1);

    record TestClass1(List<String> property1) {
    }

    record TestClass2(List<String> property1) {
    }
}

Produces:

import java.util.ArrayList;
import java.util.List;
import javax.annotation.processing.Generated;

@Generated(
    value = "org.mapstruct.ap.MappingProcessor",
    date = "2023-08-28T08:56:19+0200",
    comments = "version: 1.5.5.Final, compiler: javac, environment: Java 17.0.8 (Private Build)"
)
public class TestMapperImpl implements TestMapper {

    @Override
    public TestMapper.TestClass2 map(TestMapper.TestClass1 testClass1) {
        if ( testClass1 == null ) {
            return null;
        }

        List<String> property1 = null;

        List<String> list = testClass1.property1();
        if ( list != null ) {
            property1 = new ArrayList<String>( list );
        }

        TestMapper.TestClass2 testClass2 = new TestMapper.TestClass2( property1 );

        return testClass2;
    }
}

Instead of the expected:

import java.util.ArrayList;
import java.util.List;
import javax.annotation.processing.Generated;

@Generated(
    value = "org.mapstruct.ap.MappingProcessor",
    date = "2023-08-28T08:56:19+0200",
    comments = "version: 1.5.5.Final, compiler: javac, environment: Java 17.0.8 (Private Build)"
)
public class TestMapperImpl implements TestMapper {

    @Override
    public TestMapper.TestClass2 map(TestMapper.TestClass1 testClass1) {
        if ( testClass1 == null ) {
            return null;
        }

        List<String> property1 = null;

        List<String> list = testClass1.property1();
        if ( list != null ) {
            property1 = new ArrayList<String>( list );
        }
        else {
            property1 = null; // This is missing in the generated code
        }

        TestMapper.TestClass2 testClass2 = new TestMapper.TestClass2( property1 );

        return testClass2;
    }
}

EDIT:
It seems adding a custom mapping acts as a workaround to this issue:

@Mapping(target="property1", defaultExpression="java( null )")

However, it would be nice not to have to remember to add it to every single iterable property on the class.

@daniel-frak

@daniel-frak I am not sure that I understand your comment.

You are saying that

        List<String> property1 = null;

        List<String> list = testClass1.property1();
        if ( list != null ) {
            property1 = new ArrayList<String>( list );
        }
        else {
            property1 = null; // This is missing in the generated code
        }

is incorrect and that the null assignment to property1 is not there when using NullValueMappingStrategy.RETURN_NULL, in your example property1 is already set to null, so if list is null the else branch you are proposing is obsolete. However, in the OP example the target is already set to the empty string in the target field declaration.

public class TestVo {

  private String id;
  private List<String> list = new ArrayList<>();
}

In any case, as I said in #2843 (comment), if there is no direct assignment then the mapping strategy works properly. The mapping strategies is applied on the methods and not the direct property mappings. That's why this issue is marked for team discussion

Note:

@filiphr

@filiphr I am saying it is a bug that the generated code is missing the line I annotated with // This is missing in the generated code.

If list is null, that does not mean that property1 is also null (though I admit the example above is not the best to showcase that).

Consider that MapStruct might be used for updating an existing entity. The purpose of the update might be to set a non-null property to null. Currently, this non-null property will stay non-null, because a null property set is being ignored.

I'm not sure what you mean by 'direct assignment'. Could you elaborate on that?

Here's an updated example with a failing test:

import lombok.AllArgsConstructor;
import lombok.Data;
import org.mapstruct.Mapper;
import org.mapstruct.MappingTarget;
import org.mapstruct.NullValueCheckStrategy;
import org.mapstruct.NullValueMappingStrategy;

import java.util.List;

@Mapper(nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS,
        nullValueIterableMappingStrategy = NullValueMappingStrategy.RETURN_NULL)
public interface TestMapper {

    void map(@MappingTarget ObjToUpdate objToUpdate, UpdateData updateData);

    @AllArgsConstructor
    @Data
    final class ObjToUpdate {

        private List<String> property1;
    }

    record UpdateData(List<String> property1) {
    }
}

import org.junit.jupiter.api.Test;
import org.mapstruct.factory.Mappers;

import static java.util.Collections.singletonList;
import static org.assertj.core.api.Assertions.assertThat;

class TestMapperTest {

    @Test
    void shouldMapToNull() {
        TestMapper mapper = Mappers.getMapper(TestMapper.class);
        TestMapper.ObjToUpdate objToUpdate = new TestMapper.ObjToUpdate(singletonList("Test value"));
        TestMapper.UpdateData updateData = new TestMapper.UpdateData(null);

        mapper.map(objToUpdate, updateData);

        assertThat(objToUpdate.getProperty1()).isEmpty();
    }
}

@daniel-frak

@daniel-frak updates are a different thing then what is being asked in the issue. I would suggest that you open a different issue for the update and the missing setting of null.

I'm not sure what you mean by 'direct assignment'. Could you elaborate on that?

Direct assignment happens when the source and target have the same type e.g. List<String>. However, the direct assignment is not affecting the update example you shared, that is entirely different problem

Noticing this issue as well.
If I have a class which has the following declared at field level:

private List<String> myField = new ArrayList<>()

And then I want to create a new instance of this class, using a mapstruct from another class, with this entry for example (or it could be missing and the field names match):

@Mapping(target="myField", source="myFieldInAnotherClass")

Even if I pass an object with a null value for myFieldInAnotherClass, the value of myField is still just resulting in an empty list, rather than null, due to the value being ignored in the generated code rather than explicitly set.

As mentioned previously, having defaultExpression = java(null) works, as does having a mapper of default <T> List<T> from(List<T> source) { return source; }, but I don't think this should be required when SET_TO_NULL is the default?

Navigation Menu

Repository navigation

nullValueIterableMappingStrategy in 1.5.0.RC1 not working properly #2843

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

nullValueIterableMappingStrategy in 1.5.0.RC1 not working properly #2843

Description

Activity

filiphr commented on May 30, 2022

filiphr commented on Jun 6, 2022

christiangoudreau commented on Aug 25, 2022

christiangoudreau commented on Aug 25, 2022

marialenak commented on Mar 28, 2023

daniel-frak commented on Aug 28, 2023

filiphr commented on Sep 10, 2023

daniel-frak commented on Sep 11, 2023

filiphr commented on Sep 16, 2023

jamfor999 commented on Jul 9, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions