-
-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#1830 NullValuePropertyMappingStrategy.CLEAR #3592
base: main
Are you sure you want to change the base?
#1830 NullValuePropertyMappingStrategy.CLEAR #3592
Conversation
- 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
@filiphr or any Reviewer who wants to Review:
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. |
It's still not clear to me what should be done for every case. 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.
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(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. |
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. :) |
In the reference Issue your opinion was:
I removed it for now, and it behaves like the default behavior.
|
Feature as discussed in: #1830