#1830 NullValuePropertyMappingStrategy.CLEAR

[zyberzebra]

Fixes #1830

• adds CLEAR as a new strategy under NullValuePropertyMappingStrategy for handling null values
• clears collections or maps when the source property is null
• includes tests for functionality and error handling

[zyberzebra]

@filiphr or any Reviewer who wants to Review:

• I added some TODO where I need some clarification

I tried to come up with, what is wanted from all the discussion that point to the issue i try to fix here.

If I missed something some examples would be nice.

[zyberzebra]

It's still not clear to me what should be done for every case.
I need some input on the following:

Works with this PR:

clearing

@Mapper
public interface BeanMapper {
    @Mapping(source = "list", target = "list", nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR)
    BeanDTO map(Bean source, @MappingTarget BeanDTO target);

generates:

[...]
        if ( target.getList() != null ) {
            Collection<String> collection = source.getList();
            if ( collection != null ) {
                target.getList().clear();
                target.getList().addAll( collection );
            }
            else {
                target.getList().clear();
            }
        }    
[...]

Error on compile:

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR)
public interface MapperWithErrorInConfig {

-> "The strategy: "CLEAR" can't be used for: "nullValuePropertyMapping"."

    @Mapping(target = "id", source = "id", nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR) //id is an integer
    BeanDTOWithId map(BeanWithId source, @MappingTarget BeanDTOWithId target);

-> "Target property: "id" is not a Collection or Map and can't be used with NullValuePropertyMappingStrategy: 'CLEAR'"

Should be valid(?):

@Mapper(
//    nullValueMapMappingStrategy = NullValueMappingStrategy.CLEAR, // TODO: IS this feature wanted? (calling .clear())
//    nullValueIterableMappingStrategy = NullValueMappingStrategy.CLEAR // TODO: IS this feature wanted? an do we iterate and remove? 
)
public interface BeanMapperWithValidConfig {

[filiphr]

Thanks for your work on this @zyberzebra. I'm traveling the next month, so I'm not sure if I'll get the chance to review this properly. Just giving you a heads up.

Regarding your questions:

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR)
public interface MapperWithErrorInConfig {

I didn't understand, does this work or not? It should work, basically things like this are inherited based on what is defined. So if you define in @Mapping it is using from there, then it goes to @BeanMapping, then @Mapper, @MapperConfig etc.

    @Mapping(target = "id", source = "id", nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR) //id is an integer
    BeanDTOWithId map(BeanWithId source, @MappingTarget BeanDTOWithId target);

I'm not sure what to do in this case, I would say it need to behave the same as it is now without the clear.

@Mapper(
//    nullValueMapMappingStrategy = NullValueMappingStrategy.CLEAR, // TODO: IS this feature wanted? (calling .clear())
//    nullValueIterableMappingStrategy = NullValueMappingStrategy.CLEAR // TODO: IS this feature wanted? an do we iterate and remove? 
)
public interface BeanMapperWithValidConfig {

are we ever doing a clear for a regular mapping? I don't think so, so I would not go there right now.

[zyberzebra]

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR)
public interface MapperWithErrorInConfig {

I didn't understand, does this work or not? It should work, basically things like this are inherited based on what is defined. So if you define in @Mapping it is using from there, then it goes to @BeanMapping, then @Mapper, @MapperConfig etc.

• I implemented it to throw an compile Error.

I'm not sure what to do in this case, I would say it need to behave the same as it is now without the clear.

• I'm going to change the compile Error to your suggestions that is does nothing, if there is nothing "to clear"

@Mapper(
//    nullValueMapMappingStrategy = NullValueMappingStrategy.CLEAR, // TODO: IS this feature wanted? (calling .clear())
//    nullValueIterableMappingStrategy = NullValueMappingStrategy.CLEAR // TODO: IS this feature wanted? an do we iterate and remove? 
)
public interface BeanMapperWithValidConfig {

are we ever doing a clear for a regular mapping? I don't think so, so I would not go there right now.

• alright!

Thanks for answering the questions. I'm going to try to make some changes and write some good Tests and tasks here that help with understanding what's going on if we come back to this PR in a month. :)

[zyberzebra]

    @Mapping(target = "id", source = "id", nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.CLEAR) //id is an integer
    BeanDTOWithId map(BeanWithId source, @MappingTarget BeanDTOWithId target);

I'm not sure what to do in this case, I would say it need to behave the same as it is now without the clear.

In the reference Issue your opinion was:

#1830 (comment)

In the implementation if NullValuePropertyMappingStrategy.CLEAR is used for nullValuePropertyMapingStrategy then we should provide a compile error, since the strategy does not make sense for non iterable / map types.

I removed it for now, and it behaves like the default behavior.

• Compile Error or not?

[filiphr]

Sorry for the confusion @zyberzebra.

I think I was referring to the fact that the issue you are working on, would be done after #3387. Which means that the clear would be fine for the 2 new places, but not for the existing place.

[filiphr]

Sorry for the wait @zyberzebra. The implementation looks good to me. I only have some small comments around the tests.

Additionally, can we please move the tests to another package. This is not a bug, but a new feature. e.g. you can put them under org.mapstruct.ap.test.nullvaluepropertymapping or a new package under that.