Remove redundant null checks in nested properties #3275
Conversation
filiphr
left a comment
filiphr
left a comment
There was a problem hiding this comment.
Thanks for the work on this @jccampanero.
I think that there might be situations where this does not work as it should. Can we add test cases for that perhaps:
- Using nested source with a
NullValueMappingStrategy.RETURN_DEFAULT - Have a presence check / condition check for the last element in the nesting
|
You are welcome @filiphr, I just tried to work in some of the planned issues as you mentioned, although I understand this issue is not planned for the 1.6 release; I appreciate that even in that case you reviewed the code, thank you very much for the feedback. I tested the code with a public class PersonDTO {
private String name;
private PersonDTO parent;
private PersonDTO grandParent;
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public PersonDTO getParent() {
return parent;
}
public void setParent(PersonDTO parent) {
this.parent = parent;
}
public PersonDTO getGrandParent() {
return grandParent;
}
public void setGrandParent(PersonDTO grandParent) {
this.grandParent = grandParent;
}
}and entity: public class Person {
private String name;
private Person parent;
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public Person getParent() {
return parent;
}
public void setParent(Person parent) {
this.parent = parent;
}
}for this @Mapper(
nullValueMappingStrategy = NullValueMappingStrategy.RETURN_DEFAULT
)
public interface PersonMapper {
@Mapping(target = "grandParent", source = "parent.parent")
PersonDTO entityToDTO(Person person);
}it generates: package org.mapstruct.ap.test.nestedproperties.redundantNulls;
import javax.annotation.processing.Generated;
@Generated(
value = "org.mapstruct.ap.MappingProcessor",
date = "2023-05-20T17:29:04+0200",
comments = "version: , compiler: javac, environment: Java 11.0.19 (Ubuntu)"
)
public class PersonMapperImpl implements PersonMapper {
@Override
public PersonDTO entityToDTO(Person person) {
PersonDTO personDTO = new PersonDTO();
if ( person != null ) {
personDTO.setGrandParent( entityToDTO( personParentParent( person ) ) );
personDTO.setName( person.getName() );
personDTO.setParent( entityToDTO( person.getParent() ) );
}
return personDTO;
}
private Person personParentParent(Person person) {
Person parent = person.getParent();
if ( parent == null ) {
return null;
}
return parent.getParent();
}
}Please, in any case, can you propose a test case that you think could fail? I will work on it as necessary. Please, could you elaborate the idea behind the last presence check as well? Could you provide a skeleton of the test case necessary to test that functionally? I will expand and test it with glad. |
|
How to restart the build which got a timeout on external resource? |
|
@MrDolch we have some problems with the Windows build. No need to restart. We merge things when it is clear that the test failures are not related to some flakiness. Thanks for checking the Regarding the other test case I mentioned. What I meant is something like we class Song {
private Artist artist;
}
class Artist {
private String name;
public boolean hasName() {
return name != null && name.length() > 0;
}
}and a mapper like: If you invoke this with |
|
@jccampanero sorry I closed the PR by accident (pressed a wrong shortcut 😄). I've also updated my last comment to include the examples I wanted to include |
Don't worry, don't worry,... 😠... Now seriously, please, don't worry @filiphr, it is perfectly fine 😃 I reviewed the use case that you provided and I think the changes are still working, probably because of the first condition in the FreeMarker template. I used the following POJOs: public class Artist {
private String name;
public boolean hasName() {
return name != null && name.length() > 0;
}
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
}public class Song {
private Artist artist;
public Artist getArtist() {
return artist;
}
public void setArtist(Artist artist) {
this.artist = artist;
}
}public class SongDTO {
private String artistName;
public String getArtistName() {
return artistName;
}
public void setArtistName(String artistName) {
this.artistName = artistName;
}
}the following mapper definition: import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.factory.Mappers;
@Mapper
public interface SongMapper {
SongMapper INSTANCE = Mappers.getMapper( SongMapper.class );
@Mapping(target = "artistName", source = "artist.name")
SongDTO map(Song song);
}and this test case: mport org.mapstruct.ap.testutil.ProcessorTest;
import org.mapstruct.ap.testutil.WithClasses;
import static org.assertj.core.api.Assertions.assertThat;
public class SongMapperTest {
@ProcessorTest
@WithClasses({Artist.class, Song.class, SongDTO.class, SongMapper.class})
void shouldDealWithPresenceCheckInABackwardCompatibleWay() {
Artist artist = new Artist();
artist.setName("");
Song song = new Song();
song.setArtist(artist);
SongDTO songDTO = SongMapper.INSTANCE.map(song);
assertThat(songDTO.getArtistName()).isNull();
}
@ProcessorTest
@WithClasses({Artist.class, Song.class, SongDTO.class, SongMapper.class})
void shouldBehaveCorrectly() {
Artist artist = new Artist();
String artistName = "U2";
artist.setName(artistName);
Song song = new Song();
song.setArtist(artist);
SongDTO songDTO = SongMapper.INSTANCE.map(song);
assertThat(songDTO.getArtistName()).isEqualTo(artistName);
}
}and it seems to work property. The generated mapper looks like this: import javax.annotation.processing.Generated;
@Generated(
value = "org.mapstruct.ap.MappingProcessor",
date = "2023-05-21T23:54:42+0200",
comments = "version: , compiler: javac, environment: Java 11.0.19 (Ubuntu)"
)
public class SongMapperImpl implements SongMapper {
@Override
public SongDTO map(Song song) {
if ( song == null ) {
return null;
}
SongDTO songDTO = new SongDTO();
songDTO.setArtistName( songArtistName( song ) );
return songDTO;
}
private String songArtistName(Song song) {
Artist artist = song.getArtist();
if ( artist == null ) {
return null;
}
if ( artist == null || !artist.hasName() ) {
return null;
}
return artist.getName();
}
}Maybe I'm forgetting something? |
|
Thanks for checking it @jccampanero. Really nice improvement. Thanks for working on it |
|
You're welcome @filiphr, my pleasure. I hope that in the end there won't be any use case where the new code would lead to an error. I will pay attention to that. |
3 participants
The code changes provided try removing redundant null checks for nested properties.
Fixes #3245.