Please introduce NullValuePropertyMappingStrategy.CLEAR for Maps and Collections (Iterables) #3381

drahkrub · 2023-09-11T14:32:52Z

drahkrub
Sep 11, 2023

When working with Hibernate and/or JPA its "difficult" to map @OneToMany relations with orphanRemoval=true (How does orphanRemoval work with JPA and Hibernate), see e.g. issue #1830.

If a collection or a map of some bean which is connected to a Hibernate session (or EntityManager) is

set to null or
replaced with a new/different instance of some collection or map

then an exception is thrown when the session gets closed: A collection with cascade=”all-delete-orphan” was no longer referenced by the owning entity instance.

I think that should be easy to avoid by introducing a new NullValuePropertyMappingStrategy.CLEAR for Maps and Collections (Iterables, see discussion #3380) such that instead of

        if ( bean.getList() != null ) {
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.getList().clear();
                bean.getList().addAll( list );
            }
            else {
                bean.setList( null ); // gives exception later on
            }
        }
        else {
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.setList( list );
            }
        }

the following code is created:

        if ( bean.getList() != null ) {
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.getList().clear();
                bean.getList().addAll( list );
            }
            else {
                bean.getList().clear();
            }
        }
        else {
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.setList( list );
            }
        }

Or optimized:

        if ( bean.getList() != null ) {
            bean.getList().clear();
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.getList().addAll( list );
            }
        }
        else {
            bean.setList( somethingDtoListToSomethingList( beanDto.getList() ) );
        }

Would that be easy to realise or is there something against it?

One more addition: The code above is generated for update mapping methods (i.e. methods using @MappingTarget), see e.g. SO: Updating existing bean instances for child collection](https://stackoverflow.com/a/77029004/194900).

Methods without @MappingTarget create the following code:

    @Override
    public Bean map(BeanDto beanDto) {
        if ( beanDto == null ) {
            return null;
        }

        Bean bean = /*  here create instance with the help of some @ObjectFactory using the ID contained in beanDto */

        bean.setList( somethingDtoListToSomethingList( beanDto.getList() ) ); // gives exception later on

        return bean;
    }

which again produces the exception A collection with cascade=”all-delete-orphan” was no longer referenced by the owning entity instance.

It would be nice if this could be changed to the (optimized) version above:

    @Override
    public Bean map(BeanDto beanDto) {
        if ( beanDto == null ) {
            return null;
        }

        Bean bean = /*  here create instance with the help of some @ObjectFactory using the ID contained in beanDto */

         if ( bean.getList() != null ) {
            bean.getList().clear();
            List<Something> list = somethingDtoListToSomethingList( beanDto.getList() );
            if ( list != null ) {
                bean.getList().addAll( list );
            }
        }
        else {
            bean.setList( somethingDtoListToSomethingList( beanDto.getList() ) );
        }

        return bean;
    }

filiphr · 2023-09-16T08:25:44Z

filiphr
Sep 16, 2023
Maintainer

This is indeed a good suggestions @drahkrub. This should be a continuation of #3380 I would say. Since using NullValuePropertyMappingStrategy.CLEAR for non iterable / map mappings doesn't make much sense. We should have an error for this in my opinion. I've created #3388 to track this

One more addition: The code above is generated for update mapping methods (i.e. methods using @MappingTarget),

NullValuePropertyMappingStrategy only applies for update methods. Your other example is an update method disguised as a normal mapping method. Why don't you use an update method instead there?

0 replies

drahkrub · 2023-09-18T11:31:13Z

drahkrub
Sep 18, 2023
Author

Your other example is an update method disguised as a normal mapping method.

I wouldn't call it a "disguise", I think it's a viable use case - if not the main use case - for an @ObjectFactory.

Why don't you use an update method instead there?

The problem is e.g. described in the mentioned SO link Updating existing bean instances for child collection, but I would like to give a complete and simplified example: (with getters/setters left out)

public class IdOnly {
    private Integer id;
}

public class IdOnlyDto {
    private Integer id;
}

public class Bean extends IdOnly {
    private List<Bean> list;
}

public class Dto extends IdOnlyDto {
    private List<Dto> list;
}

@Mapper
public abstract class MyMapper {

    public abstract Bean mapDtoOnBean(Dto dto, @MappingTarget Bean bean);
    
    @ObjectFactory
    public <T extends IdOnly> T resolve(final IdOnlyDto dto, final @TargetType Class<T> clazz) {
        return dto != null
                ? resolveById(dto.getId(), clazz)
                : null;
    }
    
    public <T extends IdOnly> T resolveById(final Integer id, final @TargetType Class<T> clazz) {
        try {
            // the way with Spring/Hibernate:
//            return id != null
//                    ? entityManager.getReference(clazz, id)
//                    : BeanUtils.instantiateClass(clazz);
            // "faked" without Spring/Hibernate:
            T newInstance = clazz.getDeclaredConstructor().newInstance();
            newInstance.setId(id);
            return newInstance;
        } catch (Exception ex) {
            throw new RuntimeException(ex);
        }
    }
}

This creates the following implementation:

public class MyMapperImpl extends MyMapper {

    @Override
    public Bean mapDtoOnBean(Dto dto, Bean bean) {
        if ( dto == null ) {
            return bean;
        }

        bean.setId( dto.getId() ); // redundant, no matter
        if ( bean.getList() != null ) {
            List<Bean> list = dtoListToBeanList( dto.getList() );
            if ( list != null ) {
                bean.getList().clear();
                bean.getList().addAll( list );
            }
            else {
                bean.setList( null );
            }
        }
        else {
            List<Bean> list = dtoListToBeanList( dto.getList() );
            if ( list != null ) {
                bean.setList( list );
            }
        }

        return bean;
    }

    protected List<Bean> dtoListToBeanList(List<Dto> list) {
        if ( list == null ) {
            return null;
        }

        List<Bean> list1 = new ArrayList<Bean>( list.size() );
        for ( Dto dto : list ) {
            list1.add( dtoToBean( dto ) );
        }

        return list1;
    }

    protected Bean dtoToBean(Dto dto) {
        if ( dto == null ) {
            return null;
        }

        Bean bean = resolve( dto, Bean.class );

        bean.setId( dto.getId() ); // redundant, no matter
        bean.setList( dtoListToBeanList( dto.getList() ) ); // oh, no!

        return bean;
    }
}

The update method public Bean mapDtoOnBean(Dto dto, Bean bean) { ... } internally calls the normal mapping method protected Bean dtoToBean(Dto dto) { ... } later on which again leads to the exception A collection with cascade=”all-delete-orphan” was no longer referenced by the owning entity instance - even if NullValuePropertyMappingStrategy.CLEAR would be introduced (for update methods).

Of course this problem can be solved by providing

public Bean dtoToBean(Dto dto) {
    return mapDtoOnBean(dto, resolve( dto, Bean.class ));
}

in MyMapper.

But why even make the distinction between update and normal mapping methods (at least here)?

Without an @ObjectFactory instead of Bean bean = resolve( dto, Bean.class ); we have Bean bean = new Bean();.
Everything that follows is also an update of this bean, isn't it? Why handle it different?

Or to put it another way: Why should

public Bean dtoToBean(Dto dto) {
    return mapDtoOnBean(dto, new Bean());
}

work different than some generated method Bean dtoToBean(Dto dto)?

0 replies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Please introduce NullValuePropertyMappingStrategy.CLEAR for Maps and Collections (Iterables) #3381

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{editor}}'s edit

{{editor}}'s edit

Uh oh!

Replies: 2 comments

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{title}}

Uh oh!

Uh oh!

{{editor}}'s edit

{{editor}}'s edit

Uh oh!

Select a reply

Uh oh!

Uh oh!

Please introduce NullValuePropertyMappingStrategy.CLEAR for Maps and Collections (Iterables) #3381

Uh oh!

Uh oh!

drahkrub Sep 11, 2023

Replies: 2 comments

Uh oh!

filiphr Sep 16, 2023 Maintainer

Uh oh!

Uh oh!

drahkrub Sep 18, 2023 Author

drahkrub
Sep 11, 2023

filiphr
Sep 16, 2023
Maintainer

drahkrub
Sep 18, 2023
Author