Skip to content

#3931 Fix @DeepClone to clone Date and Calendar instead of copying references - #4092

Open
kamilkrzywanski wants to merge 1 commit into
mapstruct:mainfrom
kamilkrzywanski:issue-3931-deepclone-date
Open

#3931 Fix @DeepClone to clone Date and Calendar instead of copying references#4092
kamilkrzywanski wants to merge 1 commit into
mapstruct:mainfrom
kamilkrzywanski:issue-3931-deepclone-date

Conversation

@kamilkrzywanski

Copy link
Copy Markdown

Summary

  • Fixes #3931: with @Mapper(mappingControl = DeepClone.class), java.util.Date fields were assigned by reference (setDate(source.getDate())) instead of being cloned.
  • Root cause: when DIRECT is disabled, types from the java package are still treated as always-direct. That is correct for immutable types like String, but wrong for mutable Date / Calendar.
  • Processor-only fix in MappingResolverImpl: exclude java.util.Date and java.util.Calendar from the always-direct rule when DIRECT is off, and emit an inline clone:
    • Datenew Date(source.getTime())
    • Calendar((Calendar) source.clone())
  • Default mappers (with DIRECT enabled) are unchanged and still copy the reference.
  • No public API changes.

Generated code (after)

if ( source.getDate() != null ) {
    beanA.setDate( new Date( source.getDate().getTime() ) );
}
if ( source.getCalendar() != null ) {
    beanA.setCalendar( ((Calendar) source.getCalendar().clone()) );
}

Test plan

  • Issue3931Test — Date/Calendar independence under @DeepClone, null handling
  • MappingControlTest — existing deep-clone / mapping-control behaviour
  • DateConversionTest — normal Date conversion paths still pass

…opying references

When DIRECT mapping is disabled (e.g. via @deepClone), java.util.Date and
java.util.Calendar were still assigned by reference because types from the
java package are treated as always-direct. Clone them instead so mutations
on the source no longer affect the target.

@filiphr filiphr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @kamilkrzywanski. This might work for the use case. However, this is not the right solution for the problem at hand. There are other types that are mutable where we cannot do a direct assignment.

One can argue that we should perhaps always create a clone of the objects that are mutable. Are you interested in working on a right fix that would fix in the architecture of MapStruct? If not, then we'll have to reject the PR and work on a fix for this separately

@kamilkrzywanski

Copy link
Copy Markdown
Author

@filiphr thanks. Agreed this isn't the right approach. I won't be able to take on the broader fix, so please close if you prefer to handle it yourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DeepClone does not clone java.util.Date fields but perform a copy reference

2 participants