Redundant null checks for nested properties

[cpgerber]

Expected behavior

Consider this simple DTO and Entity classes:

@Data
public class PersonDTO {
    private String name;
    private String parentName;
}

@Data
public class Person {
    private String name;
    private Person parent;
}

And a simple mapper:

@Mapper
public interface PersonMapper {

    @Mapping(target = "parentName", source = "parent.name")
    PersonDTO entityToDTO(Person person);
}

Generated code should be:

public class PersonMapperImpl implements PersonMapper {

    @Override
    public PersonDTO entityToDTO(Person person) {
        if ( person == null ) {
            return null;
        }

        PersonDTO personDTO = new PersonDTO();

        personDTO.setParentName( personParentName( person ) );
        personDTO.setName( person.getName() );

        return personDTO;
    }

    private String personParentName(Person person) {
        Person parent = person.getParent();
        if ( parent == null ) {
            return null;
        }
        return parent.getName();
    }

Actual behavior

Generated code is this:

public class PersonMapperImpl implements PersonMapper {

    @Override
    public PersonDTO entityToDTO(Person person) {
        if ( person == null ) {
            return null;
        }

        PersonDTO personDTO = new PersonDTO();

        personDTO.setParentName( personParentName( person ) );
        personDTO.setName( person.getName() );

        return personDTO;
    }

    private String personParentName(Person person) {
        if ( person == null ) {       // Redundant null check, unreachable return statement
            return null;
        }
        Person parent = person.getParent();
        if ( parent == null ) {
            return null;
        }
        String name = parent.getName();
        if ( name == null ) {      // Redundant null check, just return the parent name
            return null;
        }
        return name;
    }

As marked, two additional redundant null checks were generated. The first one makes it impossible to reach full code coverage (unless using Reflection) and the second is just unnecessary code.

Steps to reproduce the problem

No special configuration needed to reproduce this.

MapStruct Version

MapStruct 1.5.4.Final

[filiphr]

I wouldn't treat this as a bug, but rather as a possible enhancement. I do agree with you that we can optimize this code a bit. The past check can indeed be removed (we need to make sure that it works properly with a presence check still.

We can consider this for some future release

[jccampanero]

Hi @filiphr.

I provided an initial PR that could resolve the redundant nulls checks for nested properties described in the issue.

I tested, mostly "manually" the generated mappers and at first glande they look fine.

I will wait for the results of the tests suites in the repository Github actions to see if the change breaks something.

Please, feel free to make the changes you deem appropriate to the provided code.

[jccampanero]

No way @filiphr, the change breaks every fixture based test that is related to nested properties. I will try to fix them and update the PR.

[jccampanero]

After adapting the fixtures everything seems to be fine again.