#1830 NullValuePropertyMappingStrategy.CLEAR #3592

zyberzebra · 2024-05-03T11:15:52Z

Feature as discussed in: #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

- 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 · 2024-05-03T11:17:53Z

@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 · 2024-05-04T08:14:19Z

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 {

- prevents the usage of "CLEAR" as a nullValuePropertyMapping strategy in config - new error message for the invalid use of mapping strategies - test case to verify the error handling.

filiphr · 2024-05-18T06:24:30Z

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 · 2024-05-18T09:59:03Z

@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 · 2024-05-19T07:59:06Z

    @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 · 2024-06-01T10:46:32Z

processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java

@@ -300,6 +293,19 @@ else if ( targetType.isArrayType() && sourceType.isArrayType() && assignment.get
            );
        }

+        private Assignment assignToTargetType(Assignment assignment, Type sourceType) {


Why is this new method needed? From what I can see this is identical to what we had before. I prefer to not have not needed changes in PRs

I guess I added a branch, extracted the method, deleted the branch and did not revert the extraction. gonna revert that

filiphr · 2024-06-01T10:54:04Z

...pstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java

+        return nullValueMappingStrategy == MAP_NULL_TO_DEFAULT || nullValueMappingStrategy == MAP_NULL_TO_CLEAR;
+    }
+
+    public enum NullValueMappingStrategy {


Does this need to be public?

No! You are Right.

filiphr · 2024-06-01T10:54:28Z

...pstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java

+    }
+
+    private boolean needsImport() {
+        return nullValueMappingStrategy == MAP_NULL_TO_DEFAULT || nullValueMappingStrategy == MAP_NULL_TO_CLEAR;


Why would we need the implementation class import if we are clearing?

Don't we need the import for the add afterwards?

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

I see... it leads to errors in Tests! Interesting, for example:

Extra content at line 8: ["import java.util.ArrayList;"]

The add doesn't need an import as well, as we call the getter and just do addAll

- removes the needsImport method - reduce the enum visibility

adds NullValuePropertyMappingStrategy.CLEAR strategy

5075f87

- 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 changed the title ~~[#1830] NullValuePropertyMappingStrategy.CLEAR~~ #1830 NullValuePropertyMappingStrategy.CLEAR May 3, 2024

Adds validation for nullValuePropertyMapping strategy in config

7482f5a

- prevents the usage of "CLEAR" as a nullValuePropertyMapping strategy in config - new error message for the invalid use of mapping strategies - test case to verify the error handling.

zyberzebra added 3 commits May 19, 2024 10:01

remove compile errors

255ee32

adss missing copyright headers

111038d

deletes unused mapper

1186c92

filiphr reviewed Jun 1, 2024

View reviewed changes

zyberzebra added 2 commits June 1, 2024 14:20

refactor: ExistingInstanceSetterWrapperForCollectionsAndMaps

65b20f6

- removes the needsImport method - reduce the enum visibility

revert method extraction

f8bc66f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1830 NullValuePropertyMappingStrategy.CLEAR #3592

#1830 NullValuePropertyMappingStrategy.CLEAR #3592

zyberzebra commented May 3, 2024 •

edited

zyberzebra commented May 3, 2024

zyberzebra commented May 4, 2024 •

edited

filiphr commented May 18, 2024

zyberzebra commented May 18, 2024

zyberzebra commented May 19, 2024 •

edited

filiphr left a comment

filiphr Jun 1, 2024

zyberzebra Jun 1, 2024

filiphr Jun 1, 2024

zyberzebra Jun 1, 2024

filiphr Jun 1, 2024

zyberzebra Jun 1, 2024 •

edited

filiphr Jun 1, 2024

#1830 NullValuePropertyMappingStrategy.CLEAR #3592

Are you sure you want to change the base?

#1830 NullValuePropertyMappingStrategy.CLEAR #3592

Conversation

zyberzebra commented May 3, 2024 • edited

zyberzebra commented May 3, 2024

zyberzebra commented May 4, 2024 • edited

Works with this PR:

clearing

Error on compile:

Should be valid(?):

filiphr commented May 18, 2024

zyberzebra commented May 18, 2024

zyberzebra commented May 19, 2024 • edited

filiphr left a comment

Choose a reason for hiding this comment

filiphr Jun 1, 2024

Choose a reason for hiding this comment

zyberzebra Jun 1, 2024

Choose a reason for hiding this comment

filiphr Jun 1, 2024

Choose a reason for hiding this comment

zyberzebra Jun 1, 2024

Choose a reason for hiding this comment

filiphr Jun 1, 2024

Choose a reason for hiding this comment

zyberzebra Jun 1, 2024 • edited

Choose a reason for hiding this comment

filiphr Jun 1, 2024

Choose a reason for hiding this comment

zyberzebra commented May 3, 2024 •

edited

zyberzebra commented May 4, 2024 •

edited

zyberzebra commented May 19, 2024 •

edited

zyberzebra Jun 1, 2024 •

edited