mapstruct
mapstruct copied to clipboard
mapstruct
Nesting on null target property
When I use the following mapping configuration with a nested property:
@Mapping(target = "nestedId", source = "nested.id")
ObjectA map(ObjectB source);
@InheritInverseConfiguration
ObjectB map(ObjectA source);
Mapstruct generates the inverse mapping below:
ObjectB objectB = new ObjectB();
Nested nested = new Nested();
objectB.setNested(nested);
nested.setId(source.getNestedId());
But when nested is null I get an instance of ObjectB with a non-null instance as nested property containing a null id property.
I need to get an instance of ObjectB with null value on its nested property.
Can I configure MapStruct in this way?
@softdays You are not using the latest 1.2.0.Beta2. I think that your issue is similar as #1011.
However, I think that currently you are always going to get a non null instance of the nested property with a null id if the source.nestedId is null.
Do you have a suggestion how we could do what you need?
In order to do something like that we will have to check all the properties that are being mapped and if all of them are null then return null, otherwise create a new object and map. However, this can trigger a large chain of mappings if you are mapping multiple target properties in the same level
Yes you're right, I was using 1.1.0.Final. And yes I can confirm that this is the same using 1.2.0.Beta2.
Indeed I had not actually perceived all the potential complexity of my issue...
Do you think that it could be addressed with a new NullValue*Strategy? Or maybe with a specific property on the property mapping?
I am aware of not bringing enough proposal
@softdays maybe a new NullValue*Strategy could help. However, this will need quite a few changes in how we generate the methods.
Maybe you can use an @AfterMapping and set the property to null explicitly if source.nestedId is null
I guess that the proper solution would-be to implement multi-argument factory methods or constructor selection as proposed in pr #847. In that way the user could control which nested properties would lead to creating new objects and which should then be completed by mapstruct automapping.
@sjaakd you are right, theoretically multi-argument factory methods will solve the problem. Thinking a bit more about the linked PR constructors can also be seen as a way of factory methods. That means that if we introduce multi value factories, then we could use it there as well
@sjaakd you are right, theoretically multi-argument factory methods will solve the problem. Thinking a bit more about the linked PR constructors can also be seen as a way of factory methods. That means that if we introduce multi value factories, then we could use it there as well
At the time when we discussed this, it was the intention to first go via factory methods. Problem is: you run into problems when argument have the same type. Which parameter should then go in which constructor argument. Perhaps we should revisit this somehow. Seems there's more urgency in it now we have nesting.
I think that we can revisit the version after 1.2.0. I don't what it has been discussed in the constructor PR (I'll need to have a look at it), but I think that we could go in the direction of if they have the same name then use that.
Smart completion in IntelliJ works in that direction as well. If the name of the parameter matches a variable it will be suggested on the top.
@filiphr Except the issue mentioned above, there is another case: as the nested property is always created with new instance, the then the new instance will be mapped with value in source. Could you check the nested property in target is not null first? if it is null then create a new instance otherwise use the original one? for example:
Nested nested =objectB.getNested(); if(nested == null){ nested = new Nested(); } objectB.setNested(nested);
because in some usecases we only copy part of property from source to nested target. If the nested one is always new one, then we will lost some properties of nested instance.
@ZhangZhangYunfei I am sorry, but I am not following.
If I understood you correctly, I think that we are already doing that for update methods and when there are different sources for the target nested property. Can you please share your Mapper that would allow us to figure it out easier.
@filiphr Our mapping like this:
@Mappings({@Mapping(source = "orderStatus", target = "status"),
@Mapping(source = "jointOrderId", target = "orderExtension.jointOrderId"),
@Mapping(source = "groupCount", target = "orderExtension.groupCount"),
@Mapping(source = "merge", target = "orderExtension.merge"),
@Mapping(source = "isMaster", target = "orderExtension.isMaster"),
@Mapping(source = "groupId", target = "orderExtension.groupId"),
@Mapping(target = "updatedTime", expression = "java( java.time.LocalDateTime.now() )")})
void copy(OrderSyncedEvent event, @MappingTarget Order order);
following is the generated code :
public void copy(OrderSyncedEvent event, Order order) {
if ( event == null ) {
return;
}
**OrderExtension orderExtension = new OrderExtension();**
order.setOrderExtension( orderExtension );
if ( event.getJointOrderId() != null ) {
orderExtension.setJointOrderId( event.getJointOrderId() );
}
}
the generated code is always create a new instance, that is the issue I mentioned.
@ZhangZhangYunfei I think you are not using the latest 1.2.0.Final version. Have you tried it, we did some modifications in this area. I think that the generated code there would be that what you are looking for
As @filiphr says in the Jul 2 comment I think that for a future version should be a NullStrategy that prevents a lof of @AfterMappings to set properties to null.
I think that this is a very common case that should have a simple solution as a strategy. For example when you map from Hibernate models to ViewDTOS if you have something like:
ViewDTO {
Integer townId;
}
JPAModel {
Town town;
}
Mapper {
@Mapping(target="town.id", source="townId")
JPAModel toJPA(ViewDTO)
}
When townId is null you expect to let town null and not create an empty instance with Id set to null.
The lack of a strategy to do that ends with lot of aafterMappercode. And as I think that a mapping between JPA<->View is a very common case it will be nice to solve that easily with some SetRootNullOnNestedNullStrategy
I think that the solution for this one is quite complex (in the sense of the number of nested mappings that need to be checked). The number of checks before doing the mapping can blow up if you have a lot of properties.
In any case at the moment we are focused on the 1.3.0 release. If someone wants to contribute support for this we are more than welcome to help out
@inad9300 the issue if up-for-grabs, the MapStruct team is currently focused on other issues.
What @sjaakd mentioned in https://github.com/mapstruct/mapstruct/issues/1166#issuecomment-323262455 together with https://github.com/mapstruct/mapstruct/issues/1243 might be a way to achieve this as well.
Using @ZhangZhangYunfei 's example, is it possible to have generated code like this:
public void copy(OrderSyncedEvent event, Order order) {
if ( event == null ) {
return;
}
if ( event.getJointOrderId() != null ) {
order.setOrderExtension(Optional.ofNullable(order.getOrderExtension()).orElseGet(OrderExtension::new));
order.getOrderExtension().setJointOrderId( event.getJointOrderId() );
}
if ( event.groupCount() != null ) {
order.setOrderExtension(Optional.ofNullable(order.getOrderExtension()).orElseGet(OrderExtension::new));
order.getOrderExtension().setGroupCount( event.getGroupCount() );
}
...
}
I'd have thought:
@Mapping(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE, source = "a.b", target = "c.d")
Would mean if a.b were null, it would just return, and not do anything. But currently it just creates an empty c.
@Jojoooo1 have a look at https://github.com/mapstruct/mapstruct/issues/1166#issuecomment-489312622
Hi, I am ready to submit a pull request. Some comments needed from the community first please:
Expected case we want this (a):
protected NestedLeaf orderDTOToNestedLeaf(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
NestedLeaf nestedLeaf = new NestedLeaf();
nestedLeaf.setOid( orderDTO.getNestedLeafOid() );
return nestedLeaf;
}
to become this (a.target):
protected NestedLeaf orderDTOToNestedLeaf(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
if (orderDTO.getNestedLeafOid() == null)
return null;
NestedLeaf nestedLeaf = new NestedLeaf();
nestedLeaf.setOid( orderDTO.getNestedLeafOid() );
return nestedLeaf;
}
General case is (b):
protected Address orderDTOToAddress(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
Address address = new Address();
address.setLine1( orderDTO.getLine1() );
address.setLine2( orderDTO.getLine2() );
address.setLine3( orderDTO.getLine3() );
return address;
}
becomes (b.target.1):
protected Address orderDTOToAddress(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
if ( orderDTO.getLine1() == null
&& orderDTO.getLine2()== null
&& orderDTO.getLine3() == null)
return null;
Address address = new Address();
address.setLine1( orderDTO.getLine1() );
address.setLine2( orderDTO.getLine2() );
address.setLine3( orderDTO.getLine3() );
return address;
}
Note: Do we want multiple fields collapsing to null when all of their parameters are null? I think so but this needs an option to disable this if this is an undesired behaviour.
Problem middle case is (c):
protected NestedMiddle orderDTOToNestedMiddle(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
NestedMiddle nestedMiddle = new NestedMiddle();
nestedMiddle.setLeaf( orderDTOToNestedLeaf( orderDTO ) );
return nestedMiddle;
}
becomes (c.target.1):
protected NestedMiddle orderDTOToNestedMiddle(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
NestedMiddle nestedMiddle = new NestedMiddle();
nestedMiddle.setLeaf( orderDTOToNestedLeaf( orderDTO ) );
if (nestedMiddle.getLeaf() == null)
return null;
return nestedMiddle;
}
Note: Note the unnecessary memory allocation. If I unbundle the assignments it looks ugly. See below. Consider when there is more than one nesting mapping.
(c.target.2):
protected NestedMiddle orderDTOToNestedMiddle(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
NestedLeaf a1 = orderDTOToNestedLeaf( orderDTO );
if (a1 == null)
return null;
NestedMiddle nestedMiddle = new NestedMiddle();
nestedMiddle.setLeaf( a1 );
return nestedMiddle;
}
If we standardise on (c.target.1) then (b.target.2) becomes the following as the general case:
protected Address orderDTOToAddress(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
Address address = new Address();
address.setLine1( orderDTO.getLine1() );
address.setLine2( orderDTO.getLine2() );
address.setLine3( orderDTO.getLine3() );
if ( address.getLine1() == null
&& address.getLine2()== null
&& address.getLine3() == null)
return null;
return address;
}
I am creating one strategy setting on global, mapper, and mapping level but I am going to need some advice from some of the mapstruct devs on my issues.
CollapseAllNullValuesToNullStrategy.NEVER, // default disabled due to the ramifications CollapseAllNullValuesToNullStrategy.ALWAYS, // when enabled, use mapstruct defined settings CollapseAllNullValuesToNullStrategy.ALWAYS_ONE_PARAMETER_ONLY, // enable for classes with a single parameter only (e.g. ID fields) CollapseAllNullValuesToNullStrategy.ALWAYS_AFTER // when enabled, (b.target.2) will be the common pattern.
Any comments on this?
Note 1: Where any mapping field is a primitive I have to skip the collapsing of the result object to null because there is a valid assignment. Not sure if there are use cases where defaults get ignored but I think the scenarios above cover 95% unless someone has some more edge cases.
I need more test cases if anyone has some esoteric mapping considerations.
For reference in a discussion:
baseline (e):
protected Address orderDTOToAddress(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
Address address = new Address();
address.setLine1( orderDTO.getLine1() );
address.setLine2( orderDTO.getLine2() );
address.setLine3( orderDTO.getLine3() );
return address;
}
becomes (e.NullValueMappingStrategy.RETURN_DEFAULT):
protected Address orderDTOToAddress(OrderDTO orderDTO) {
Address address = new Address();
if ( orderDTO != null ) {
address.setLine1( orderDTO.getLine1() );
address.setLine2( orderDTO.getLine2() );
address.setLine3( orderDTO.getLine3() );
}
return address;
}
becomes (e.NullValueCheckStrategy.ALWAYS):
protected Address orderDTOToAddress(OrderDTO orderDTO) {
if ( orderDTO == null ) {
return null;
}
Address address = new Address();
if ( orderDTO.getLine1() != null ) {
address.setLine1( orderDTO.getLine1() );
}
if ( orderDTO.getLine2() != null ) {
address.setLine2( orderDTO.getLine2() );
}
if ( orderDTO.getLine3() != null ) {
address.setLine3( orderDTO.getLine3() );
}
return address;
}
baseline (f):
@Override
public OrderDTO convertToOrderDTO(Order order) {
if ( order == null ) {
return null;
}
OrderDTO orderDTO = new OrderDTO();
orderDTO.setCustomerName( orderCustomerName( order ) );
orderDTO.setNestedLeafOid( orderNestedMiddleLeafOid( order ) );
orderDTO.setLine1( orderCustomerAddressLine1( order ) );
orderDTO.setLine2( orderCustomerAddressLine2( order ) );
orderDTO.setLine3( orderCustomerAddressLine3( order ) );
orderDTO.setDoubleNestedLeafOid( orderDoubleNestedMiddleLeafOid( order ) );
orderDTO.setDoubleNestedLeafName( orderDoubleNestedMiddleLeafName( order ) );
return orderDTO;
}
becomes (f.NullValueCheckStrategy.ALWAYS):
@Override
public OrderDTO convertToOrderDTO(Order order) {
if ( order == null ) {
return null;
}
OrderDTO orderDTO = new OrderDTO();
String name = orderCustomerName( order );
if ( name != null ) {
orderDTO.setCustomerName( name );
}
Long oid = orderNestedMiddleLeafOid( order );
if ( oid != null ) {
orderDTO.setNestedLeafOid( oid );
}
String line1 = orderCustomerAddressLine1( order );
if ( line1 != null ) {
orderDTO.setLine1( line1 );
}
String line2 = orderCustomerAddressLine2( order );
if ( line2 != null ) {
orderDTO.setLine2( line2 );
}
String line3 = orderCustomerAddressLine3( order );
if ( line3 != null ) {
orderDTO.setLine3( line3 );
}
Long oid1 = orderDoubleNestedMiddleLeafOid( order );
if ( oid1 != null ) {
orderDTO.setDoubleNestedLeafOid( oid1 );
}
String name1 = orderDoubleNestedMiddleLeafName( order );
if ( name1 != null ) {
orderDTO.setDoubleNestedLeafName( name1 );
}
return orderDTO;
}
NullValueMappingStrategy should control whether to return Null (RETURN_NULL) or Default (RETURN_DEFAULT) object. If default object is returned then there is nothing to do. If it is RETURN_NULL then instead of creating a new strategy we could have NullValueMappingStrategy.RETURN_NULL_ON_ALL_VALUES_NULL. Less work on creating more strategies.
This is likely the best approach to configure this.
The below will be an example of (c.target.2) with NullValueCheckStrategy.ALWAYS and NullValueMappingStrategy.RETURN_NULL_ON_ALL_VALUES_NULL.
public OrderDTO convertToOrderDTO(Order order) {
if ( order == null ) {
return null;
}
String name = orderCustomerName( order );
Long oid = orderNestedMiddleLeafOid( order );
String line1 = orderCustomerAddressLine1( order );
String line2 = orderCustomerAddressLine2( order );
String line3 = orderCustomerAddressLine3( order );
Long oid1 = orderDoubleNestedMiddleLeafOid( order );
String name1 = orderDoubleNestedMiddleLeafName( order );
if (name == null
&& oid == null
&& line1 == null
&& line2 == null
&& line3 == null
&& oid1 == null
&& name1 == null)
return null;
OrderDTO orderDTO = new OrderDTO();
if ( name != null ) {
orderDTO.setCustomerName( name );
}
if ( oid != null ) {
orderDTO.setNestedLeafOid( oid );
}
if ( line1 != null ) {
orderDTO.setLine1( line1 );
}
if ( line2 != null ) {
orderDTO.setLine2( line2 );
}
if ( line3 != null ) {
orderDTO.setLine3( line3 );
}
if ( oid1 != null ) {
orderDTO.setDoubleNestedLeafOid( oid1 );
}
if ( name1 != null ) {
orderDTO.setDoubleNestedLeafName( name1 );
}
return orderDTO;
}
Thanks a lot for your efforts in this @tim12s. I was going through your examples and ideas and I believe that a lot of this could be solved through #2610. In 1.5.0.Beta1 we added support for conditional mappings on properties see #2051.
If we bring that support for source parameters then this can be entirely solved on the side of the user. The user can decide when a source parameter is deemed to be present.
Imho this issue is still valid and can't be fixed with the current MapStruct release. No matter what conditionExpression is added or what null*CheckStrategy is set - in the generated protected Mapping method, the instance of the target property class is always created and returned - even if all the properties are null
protected BankAccount capitalsourceDataToBankAccount(CapitalsourceData capitalsourceData) {
if ( capitalsourceData == null ) {
return null;
}
String accountNumber = null;
String bankCode = null;
accountNumber = capitalsourceData.getAccountNumber();
bankCode = capitalsourceData.getBankCode();
BankAccount bankAccount = new BankAccount( accountNumber, bankCode );
return bankAccount;
}
No matter what you try - the instantiation of BankAccount always takes place and is returned. Giving BankAccount a no-args-constructor changes nothing in the outcome.
Just for reference my Mapper
@Mapper(componentModel = "spring", unmappedTargetPolicy = ReportingPolicy.ERROR, uses = {
CapitalsourceIdMapper.class, CapitalsourceTypeMapper.class, CapitalsourceStateMapper.class,
CapitalsourceImportMapper.class })
public interface CapitalsourceDataMapper extends IMapper<Capitalsource, CapitalsourceData> {
@Override
@Mapping(target = "groupUse", source = "attGroupUse")
@Mapping(target = "bankAccount.accountNumber", source = "accountNumber")
@Mapping(target = "bankAccount.bankCode", source = "bankCode")
@Mapping(target = "user.id.id", source = "macIdCreator")
@Mapping(target = "access.id.id", source = "macIdAccessor")
Capitalsource mapBToA(final CapitalsourceData b);
@Override
@Mapping(target = "attGroupUse", source = "groupUse")
@Mapping(target = ".", source = "bankAccount")
@Mapping(target = "macIdCreator", source = "user.id.id")
@Mapping(target = "macIdAccessor", source = "access.id.id")
CapitalsourceData mapAToB(final Capitalsource a);
}
@filiphr not sure thats really the case. Imagine I have the following Mappings:
@Mapping(target = "bankAccount.accountNumber", source = "accountNumber")
@Mapping(target = "bankAccount.bankCode", source = "bankCode")
I currently can't imagine how #2610 is going to work to leave bankAccount null when accountNumber and bankCode are both null, but will initiate and map it if one or both of them are set. bankAccount would need to remain null and the deep mapping would need to be ignored depending on the conditions. To go along with #2610 it would be something like:
@Mapping(target = "bankAccount.accountNumber", source = "accountNumber", condition = "isNotEmpty")
@Mapping(target = "bankAccount.bankCode", source = "bankCode", condition = "isNotEmpty")
[...]
Capitalsource mapBToA(final CapitalsourceData b);
@Condition
default boolean isNotEmpty(String value) {
return value != null && value.length() > 0;
}
And the generated Mapper:
protected BankAccount capitalsourceDataToBankAccount(CapitalsourceData capitalsourceData) {
// no condition succeeded or source is null --> return null
if ( capitalsourceData == null || ( !isNotEmpty(capitalsourceData.getAccountNumber()) && !isNotEmpty(capitalsourceData.getBankCode()) ) {
return null;
}
String accountNumber = null;
String bankCode = null;
accountNumber = capitalsourceData.getAccountNumber();
bankCode = capitalsourceData.getBankCode();
BankAccount bankAccount = new BankAccount();
if(isNotEmpty(accountNumber))
bankAccount.setAccountNumber(accountNumber);
if(isNotEmpty(bankCode))
bankAccount.setBankCode(bankCode);
return bankAccount;
}
While the 2nd part of my generated example Mapper (calling of the Setter depending on the condition) seems fine, I cant imagine a propper solution for the first part on how to handle when to return null for BankAccount or not. Should null be returned when all conditions are failing for the target Object in question? What if one of the deep-mapping has no condition? For example accountNumber has a condition, but bankCode hasen't? In this case BankCode will not become null? Somehow it does not feel intuitive that conditions used together with deep mappings could trigger the whole object becoming null (even if thats what I want :smile:) Imho #2610 is no fit for #1166. Maybe you could elaborate a bit more on that?
@OlliL the idea with #2610 is that you would be able to do something like:
@Mapping(target = "bankAccount.accountNumber", source = "accountNumber")
@Mapping(target = "bankAccount.bankCode", source = "bankCode")
[...]
Capitalsource mapBToA(final CapitalsourceData b);
@Condition
default boolean isNotEmpty(String value) {
return value != null && value.length() > 0;
}
@Condition
default boolean isValid(CapitalsourceData data) {
return data != null && ( isNotEmpty( data.getAccountNumber() ) || isNotEmpty( data.getBankCode() ) );
}
and then in the generated code you should have something like:
protected BankAccount capitalsourceDataToBankAccount(CapitalsourceData capitalsourceData) {
// no condition succeeded or source is null --> return null
if ( !isValid( capitalsourceData ) ) {
return null;
}
String accountNumber = null;
String bankCode = null;
accountNumber = capitalsourceData.getAccountNumber();
bankCode = capitalsourceData.getBankCode();
BankAccount bankAccount = new BankAccount();
if(isNotEmpty(accountNumber))
bankAccount.setAccountNumber(accountNumber);
if(isNotEmpty(bankCode))
bankAccount.setBankCode(bankCode);
return bankAccount;
}
@filiphr I see, thanks for explaining it. Would the generator intelligent enough to handle multiple target Objects?
isValid() is checking the validity for creating BankAccount or leaving it null in that example.
What would be if the target object has more Objects like BankAccount? One single isValid() method wouldn't be enough in such a case.
Here is one example showing what I'm trying to say: MoneyflowTransportMapper.java
Multiple "isValid-like" methods would need to be implemented but then the Code which generates the mapper implementation would need to know which isValid-method has to be used for which target object....?
I am not sure that I understand @OlliL. In your examples you want to same method to be called for the different source properties. Irregardless of their type.
By the way, for the code you shared I would suggest you to change your after mapping do not return the entity itself. If you have multiple after mappings they might not be called because we short circuit if an after mapping returns a non null instance.