Description

belkin1667

opened on Mar 15, 2023

Issue body actions

Expected behavior

Either LocalDateTime map(LocalDate date) results in compilation error or mapping results in date == null ? null : LocalDateTime.of(date, LocalTime.MIN)

Actual behavior

LocalDateTime map(LocalDate date) maps any date to null

The problem seems to be related to incorrect mapping sequence: LocalDate to XMLGregorianCalendar and XMLGregorianCalendar to LocalDateTime. Separately those mappings are correct, but combined are not:
Generated mapper contains the following line:

LocalDateTime localDateTime = xmlGregorianCalendarToLocalDateTime( localDateToXmlGregorianCalendar( date ) ); 

Generated method xmlGregorianCalendarToLocalDateTime(..) returns null if hours, minutes or seconds are equal to DatatypeConstants.FIELD_UNDEFINED, which they are cause localDateToXmlGregorianCalendar(..) calls javax.xml.datatype.DatatypeFactory::newXMLGregorianCalendarDate which sets hours, minuts, seconds and milliseconds as FIELD_UNDEFINED

Methods I'm talking about for reference:

        private static LocalDateTime xmlGregorianCalendarToLocalDateTime( XMLGregorianCalendar xcal ) {
            if ( xcal == null ) {
                return null;
            }

            if ( xcal.getYear() != DatatypeConstants.FIELD_UNDEFINED
                && xcal.getMonth() != DatatypeConstants.FIELD_UNDEFINED
                && xcal.getDay() != DatatypeConstants.FIELD_UNDEFINED
                && xcal.getHour() != DatatypeConstants.FIELD_UNDEFINED
                && xcal.getMinute() != DatatypeConstants.FIELD_UNDEFINED
            ) { //some mapping }
            return null;
        }

private XMLGregorianCalendar localDateToXmlGregorianCalendar( LocalDate localDate ) {
            if ( localDate == null ) {
                return null;
            }

            return datatypeFactory.newXMLGregorianCalendarDate(
                localDate.getYear(),
                localDate.getMonthValue(),
                localDate.getDayOfMonth(),
                DatatypeConstants.FIELD_UNDEFINED );
        }

    public XMLGregorianCalendar newXMLGregorianCalendarDate(
            final int year,
            final int month,
            final int day,
            final int timezone) {

            return newXMLGregorianCalendar(
                    year,
                    month,
                    day,
                    DatatypeConstants.FIELD_UNDEFINED, // hour
                    DatatypeConstants.FIELD_UNDEFINED, // minute
                    DatatypeConstants.FIELD_UNDEFINED, // second
                    DatatypeConstants.FIELD_UNDEFINED, // millisecond
                    timezone);
            }

Steps to reproduce the problem

Define Mapper:

@Mapper
public interface IssueBelkinmikeMapper {

    IssueBelkinmikeMapper INSTANCE = Mappers.getMapper( IssueBelkinmikeMapper.class );

    LocalDateTime map(LocalDate date);
}

Define Test:

@WithClasses({
    IssueBelkinmikeMapper.class
})
@IssueKey("belkinmike")
class IssueBelkinmikeMapperTest {

    @ProcessorTest
    void throwsException() {
        LocalDate source = LocalDate.of(2022, 2, 2);
        LocalDateTime target = IssueBelkinmikeMapper.INSTANCE.map(source);
        assertThat(target).isNotNull();
    }
}

Test should pass or fail at mapper generation stage, but it is not

IMHO: It should fail at generating such mapping, cause I met this problem mapping dtos:

I had this classes

class EntityDto {
    LocalDate dateFrom;
}
class Entity {
    LocalDate dateFrom;
}

interface MyMapper {
    EntityDto map(Entity e);
}

then dto field's type changed to LocalDateTime:

class EntityDto {
    LocalDateTime dateFrom;
}
class Entity {
    LocalDate dateFrom;
}

interface MyMapper {
    EntityDto map(Entity e);
}

And I would expect mapping to fail and attract my attention to it, but it did not

MapStruct Version

main branch (commit id=a7ba12676d237957be0753076f436d7974c9f7f4)

Reacted by Sirttas

Activity

1. belkin1667

   added

   bug

   on Mar 15, 2023
2. belkin1667

   closed this as completedon Mar 15, 2023
3. belkin1667

   reopened this on Mar 15, 2023
4. belkin1667 commented on Mar 15, 2023

   

   belkin1667

   on Mar 15, 2023

   Author

   More actions

   Workaround is using @Mapper( mappingControl = NoComplexMapping.class ), however I do not consider current default behaviour is correct to be default

   The reason this workaround works is we disable 2 step mappings:
   allow2step defaults to true if no mappingControl set

   mapstruct/processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/SelectionCriteria.java

   Line 65 in 8894cd5

   this.allow2Steps = true;
5. belkin1667 commented on Mar 15, 2023

   

   belkin1667

   on Mar 15, 2023

   Author

   More actions

   Found this comment regarding wrong matches using 2 step mappings:

   mapstruct/processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java

   Line 183 in 8894cd5

   // resolving via 2 steps creates the possibility of wrong matches, first builtin method matches,
6. belkin1667 commented on Mar 15, 2023

   

   belkin1667

   on Mar 15, 2023

   Author

   More actions

   I believe MapStruct (regarding 2 step mappings) operates assuming if methods signatures match, then there is a correct mapping, however it is not always true and depends on implementation of found methods, as we can see. Therefore I suggest to introduce some mechanics to exclude known incorrect mapping sequences

   Would appreciate some help if it is good solution and where in source code I should implement it as I'm new to MapStruct source code.

   I've already implemented a fix with default mapping to date == null ? null : LocalDateTime.of(date, LocalTime.MIN), however I'm considering it is not correct to have LocalDate to LocalDateTime mapping at all, so I had not created PR with such fix
7. belkin1667

   changed the title [-]java.time.LocalDateTime to java.time.LocalDate default mapping always results in null[/-] [+]java.time.LocalDate to java.time.LocalDateTime default mapping always results in null[/+] on Mar 15, 2023
8. filiphr commented on Apr 16, 2023

   

   filiphr

   on Apr 16, 2023

   Member

   More actions

   MapStruct actually does not have an out of the box support for mapping between LocalDate and LocalDateTime. This works by accident via the 2-step mappings.

   I would rename this issue to support implicit mapping between LocalDate and LocalDateTime. The mapping should be in the following manner:

   • LocalDate -> LocalDateTime: date.atStartOfDay()
   • LocalDateTime -> LocalDate: date.toLocalDate()

   The JavaZonedDateTimeToDateConversion can be used as an example.

   This is a good issue for someone from the community that is interested in contributing to MapStruct. Please let us know if you would like to work on this. @belkin1667 you have the priority in case you are interested in adding this functionality.

   ---

   A workaround for this would be to define your own custom mapping for LocalDate -> LocalDateTime

   e.g.

   public class CustomLocalDateTimeMapper {
   
       public LocalDateTime toLocalDateTime(LocalDate localDate) {
           return localDate != null ? localDate.atStartOfDay() : null;
       }
       
       public LocalDate toLocalDate(LocalDateTime localDateTime) {
           return localDateTime != null ? localDateTime.toLocalDate() : null;
       }
   
   }

   and then put CustomLocalDateTimeMapper in Mapper#uses

   Reacted by Belkin Michael
9. filiphr

   changed the title [-]java.time.LocalDate to java.time.LocalDateTime default mapping always results in null[/-] [+]Support for implicit conversion between java.time.LocalDate and java.time.LocalDateTime[/+] on Apr 16, 2023
10. filiphr

    added

    enhancement

    and removed

    bug

    on Apr 16, 2023
11. filiphr

    added this to the 1.6.0 milestone on Apr 16, 2023
12. Bragolgirith

    added 4 commits that reference this issue on Apr 25, 2023

    mapstruct#3199 Add support for implicit conversion between java.time.…

    04bdf22

    mapstruct#3199 Add support for implicit conversion between java.time.…

    ece5767

    mapstruct#3199 Add support for implicit conversion between java.time.…

    d2cab51

    mapstruct#3199 Minor comment fix

    6cda3a3
13. filiphr

    closed this as completedin #3254on May 1, 2023
14. filiphr

    added a commit that references this issue on May 1, 2023

    #3199 Add support for implicit conversion between java.time.LocalDate…

    d3b4a16

Issue actions