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

Source parameter for @ObjectFactory method is not well defined #1131

Closed
44past4 opened this Issue Mar 10, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@44past4

44past4 commented Mar 10, 2017

It seems that the source parameter to object factory method has different meaning depending if it is used from method with @MappingTarget annotation or not. For methods with @MappingTarget parent of object which is going to be mapped into newly created object is passed as source and in case of methods without @MappingTarget annotation the object to be mapped is passed itself.
This can be seen in the example below where copyB() method implementation passes BDTO instance as source and in mergeA() method implementation passes ADTO instance.

@Mapper
public interface VerifyMappings {
	class AEntity {
		private BEntity b;
		public BEntity getB() {
			return b;
		}
		public void setB(BEntity b) {
			this.b = b;
		}
	}
	class BEntity {}
	class ADTO {
		private BDTO b;
		public BDTO getB() {
			return b;
		}
		public void setB(BDTO b) {
			this.b = b;
		}
	}
	class BDTO {}

	ADTO copyA(AEntity source);
	BDTO copyB(BEntity source);

	void mergeA(AEntity source, @MappingTarget ADTO target);
	void mergeB(BEntity source, @MappingTarget BDTO target);

	@ObjectFactory
	default BDTO createB(Object source) {
		return new BDTO();
	}
}
@44past4

This comment has been minimized.

Show comment
Hide comment
@44past4

44past4 Mar 10, 2017

This one is quite important because it means that it is not possible to implement cycle detection as it was described in #469 in case of merge being performed.

44past4 commented Mar 10, 2017

This one is quite important because it means that it is not possible to implement cycle detection as it was described in #469 in case of merge being performed.

@agudian agudian added this to the 1.2.0.Final milestone Mar 10, 2017

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Mar 10, 2017

Member

We should definitely align that.

But regarding the cycle handling, doesn't the following work for update-methods as well?
https://github.com/mapstruct/mapstruct-examples/tree/master/mapstruct-mapping-with-cycles

Member

agudian commented Mar 10, 2017

We should definitely align that.

But regarding the cycle handling, doesn't the following work for update-methods as well?
https://github.com/mapstruct/mapstruct-examples/tree/master/mapstruct-mapping-with-cycles

@44past4

This comment has been minimized.

Show comment
Hide comment
@44past4

44past4 Mar 10, 2017

Unfortunately this solution doesn't work for update/merge methods.
The code below in case of simple cycle of employee reporting to itself in case of create/copy method works fine (boss -> boss) but in case of update/merge method returns employee reporting to empty uninitialized employee (boss -> null).

This could be fixed by adding following method to the mapper:

@ObjectFactory
default <T> T createNewInstance(Object source, @TargetType Class<T> targetClass,
		@Context CycleAvoidingMappingContext context) {
	if (context.knownInstances.containsKey(source)) {
		return (T) context.knownInstances.get(source);
	}
	try {
		return targetClass.newInstance();
	} catch (InstantiationException | IllegalAccessException e) {
		throw new RuntimeException(e);
	}
}

but it does not work exactly because of the problem with source parameter to @ObjectFactory method.

package com.syncron.bpp.core.mapping.mapstruct;

import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;

import org.mapstruct.BeforeMapping;
import org.mapstruct.Context;
import org.mapstruct.InheritConfiguration;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.MappingTarget;
import org.mapstruct.TargetType;
import org.mapstruct.factory.Mappers;

@Mapper
public interface EmpoyeeMapper {
	public class Employee {

		private String name;
		private Employee reportsTo;

		public String getName() {
			return name;
		}

		public void setName(String name) {
			this.name = name;
		}

		public Employee getReportsTo() {
			return reportsTo;
		}

		public void setReportsTo(Employee reportsTo) {
			this.reportsTo = reportsTo;
		}

		@Override
		public String toString() {
			return name;
		}
	}

	public class EmployeeDto {

		private String employeeName;
		private EmployeeDto reportsTo;
		private List<EmployeeDto> team;

		public String getEmployeeName() {
			return employeeName;
		}

		public void setEmployeeName(String employeeName) {
			this.employeeName = employeeName;
		}

		public EmployeeDto getReportsTo() {
			return reportsTo;
		}

		public void setReportsTo(EmployeeDto reportsTo) {
			this.reportsTo = reportsTo;
		}

		@Override
		public String toString() {
			return employeeName;
		}
	}

	public class CycleAvoidingMappingContext {
		private Map<Object, Object> knownInstances = new IdentityHashMap<Object, Object>();

		@BeforeMapping
		public <T> T getMappedInstance(Object source, @TargetType Class<T> targetType) {
			return (T) knownInstances.get(source);
		}

		@BeforeMapping
		public void storeMappedInstance(Object source, @MappingTarget Object target) {
			knownInstances.put(source, target);
		}
	}

	EmpoyeeMapper MAPPER = Mappers.getMapper(EmpoyeeMapper.class);

	@Mapping(source = "employeeName", target = "name")
	Employee toEmployee(EmployeeDto employeeDto, @Context CycleAvoidingMappingContext context);

	@InheritConfiguration
	Employee updateEmployee(EmployeeDto employeeDto, @MappingTarget Employee employee,
			@Context CycleAvoidingMappingContext context);

	public static class Main {
		public static void main(String[] args) {
			EmployeeDto e = new EmployeeDto();
			e.setEmployeeName("boss");
			e.setReportsTo(e);
			Employee e1 = MAPPER.toEmployee(e, new CycleAvoidingMappingContext());
			Employee e2 = MAPPER.updateEmployee(e, new Employee(), new CycleAvoidingMappingContext());
			System.out.println(e1+" -> "+e1.getReportsTo());
			System.out.println(e2+" -> "+e2.getReportsTo());
		}
	}
}

44past4 commented Mar 10, 2017

Unfortunately this solution doesn't work for update/merge methods.
The code below in case of simple cycle of employee reporting to itself in case of create/copy method works fine (boss -> boss) but in case of update/merge method returns employee reporting to empty uninitialized employee (boss -> null).

This could be fixed by adding following method to the mapper:

@ObjectFactory
default <T> T createNewInstance(Object source, @TargetType Class<T> targetClass,
		@Context CycleAvoidingMappingContext context) {
	if (context.knownInstances.containsKey(source)) {
		return (T) context.knownInstances.get(source);
	}
	try {
		return targetClass.newInstance();
	} catch (InstantiationException | IllegalAccessException e) {
		throw new RuntimeException(e);
	}
}

but it does not work exactly because of the problem with source parameter to @ObjectFactory method.

package com.syncron.bpp.core.mapping.mapstruct;

import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;

import org.mapstruct.BeforeMapping;
import org.mapstruct.Context;
import org.mapstruct.InheritConfiguration;
import org.mapstruct.Mapper;
import org.mapstruct.Mapping;
import org.mapstruct.MappingTarget;
import org.mapstruct.TargetType;
import org.mapstruct.factory.Mappers;

@Mapper
public interface EmpoyeeMapper {
	public class Employee {

		private String name;
		private Employee reportsTo;

		public String getName() {
			return name;
		}

		public void setName(String name) {
			this.name = name;
		}

		public Employee getReportsTo() {
			return reportsTo;
		}

		public void setReportsTo(Employee reportsTo) {
			this.reportsTo = reportsTo;
		}

		@Override
		public String toString() {
			return name;
		}
	}

	public class EmployeeDto {

		private String employeeName;
		private EmployeeDto reportsTo;
		private List<EmployeeDto> team;

		public String getEmployeeName() {
			return employeeName;
		}

		public void setEmployeeName(String employeeName) {
			this.employeeName = employeeName;
		}

		public EmployeeDto getReportsTo() {
			return reportsTo;
		}

		public void setReportsTo(EmployeeDto reportsTo) {
			this.reportsTo = reportsTo;
		}

		@Override
		public String toString() {
			return employeeName;
		}
	}

	public class CycleAvoidingMappingContext {
		private Map<Object, Object> knownInstances = new IdentityHashMap<Object, Object>();

		@BeforeMapping
		public <T> T getMappedInstance(Object source, @TargetType Class<T> targetType) {
			return (T) knownInstances.get(source);
		}

		@BeforeMapping
		public void storeMappedInstance(Object source, @MappingTarget Object target) {
			knownInstances.put(source, target);
		}
	}

	EmpoyeeMapper MAPPER = Mappers.getMapper(EmpoyeeMapper.class);

	@Mapping(source = "employeeName", target = "name")
	Employee toEmployee(EmployeeDto employeeDto, @Context CycleAvoidingMappingContext context);

	@InheritConfiguration
	Employee updateEmployee(EmployeeDto employeeDto, @MappingTarget Employee employee,
			@Context CycleAvoidingMappingContext context);

	public static class Main {
		public static void main(String[] args) {
			EmployeeDto e = new EmployeeDto();
			e.setEmployeeName("boss");
			e.setReportsTo(e);
			Employee e1 = MAPPER.toEmployee(e, new CycleAvoidingMappingContext());
			Employee e2 = MAPPER.updateEmployee(e, new Employee(), new CycleAvoidingMappingContext());
			System.out.println(e1+" -> "+e1.getReportsTo());
			System.out.println(e2+" -> "+e2.getReportsTo());
		}
	}
}
@44past4

This comment has been minimized.

Show comment
Hide comment
@44past4

44past4 Mar 10, 2017

There wouldn't be a problem if the updateEmployee() method implementation instead of this:

if ( employeeDto.getReportsTo() != null ) {
    if ( employee.getReportsTo() == null ) {
        employee.setReportsTo( new Employee() );
    }
    updateEmployee( employeeDto.getReportsTo(), employee.getReportsTo(), context );
}
else {
    employee.setReportsTo( null );
}

would do something like this:

if ( employeeDto.getReportsTo() != null ) {
    if ( employee.getReportsTo() == null ) {
        employee.setReportsTo( toEmployee( employeeDto.getReportsTo(), context ) );
    } else {
        employee.setReportsTo( updateEmployee( employeeDto.getReportsTo(), employee.getReportsTo(), context ) );
    }
}
else {
    employee.setReportsTo( null );
}

44past4 commented Mar 10, 2017

There wouldn't be a problem if the updateEmployee() method implementation instead of this:

if ( employeeDto.getReportsTo() != null ) {
    if ( employee.getReportsTo() == null ) {
        employee.setReportsTo( new Employee() );
    }
    updateEmployee( employeeDto.getReportsTo(), employee.getReportsTo(), context );
}
else {
    employee.setReportsTo( null );
}

would do something like this:

if ( employeeDto.getReportsTo() != null ) {
    if ( employee.getReportsTo() == null ) {
        employee.setReportsTo( toEmployee( employeeDto.getReportsTo(), context ) );
    } else {
        employee.setReportsTo( updateEmployee( employeeDto.getReportsTo(), employee.getReportsTo(), context ) );
    }
}
else {
    employee.setReportsTo( null );
}

@agudian agudian added the bug label Mar 12, 2017

@agudian agudian changed the title from Source paramter for @ObjectFactory method is not well defined to Wrong source paramter for @ObjectFactory method is passed when creating property instances for update methods Mar 12, 2017

@agudian agudian changed the title from Wrong source paramter for @ObjectFactory method is passed when creating property instances for update methods to Source paramter for @ObjectFactory method is not well defined Mar 12, 2017

@agudian agudian removed the bug label Mar 12, 2017

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Mar 12, 2017

Member

Hmm, I just added the bug label and rephrased the issue title to fix this.

But when I looked at the code, it doomed on me that the current behavior is as it was coded on purpose, and it matches the documentation of @ObjectFactory.

So I've changed it back to your original title, as I too think that this behavior is something that we might need to rethink. I must admit I didn't have this possible invocation point for factory methods in my mind when we discussed this feature.

But I guess we still can change this, as we didn't have a 1.2.0 Final, yet and the source parameter of the object factory methods is something new.

So I propose that if an @ObjectFactory method with a non-annotated parameter (what we call the "source" parameter), we don't check if any of the mapping methods source parameters is assignable, but if the "source" of the target object to create is assignable. For factory-method invocations at the start of the mapping method, that can be any of the mapping methods source params as it is now. But for the case where we want to create a new instance to pass into an update method as in this example, we would only try to type-match and pass the source property value.

For the very first mapper example from above, this would mean we generate this:

    @Override
    BDTO copyB(BEntity source) {
        if ( source == null ) {
            return null;
        }

        BDTO bDTO = createB( source );

        return bDTO;
    }

    @Override
    void mergeA(AEntity source, ADTO target) {
        if ( source == null ) {
            return;
        }

        if ( source.getB() != null ) {
            if ( target.getB() == null ) {
               target.setB( createB( source.getB() ) ); // is currently generated as: target.setB( createB( source ) );
            }
            mergeB( source.getB(), target.getB() );
        }
        else {
            target.setB( null );
        }
    }

@gunnarmorling, @filiphr, @sjaakd: what do you guys think?

Member

agudian commented Mar 12, 2017

Hmm, I just added the bug label and rephrased the issue title to fix this.

But when I looked at the code, it doomed on me that the current behavior is as it was coded on purpose, and it matches the documentation of @ObjectFactory.

So I've changed it back to your original title, as I too think that this behavior is something that we might need to rethink. I must admit I didn't have this possible invocation point for factory methods in my mind when we discussed this feature.

But I guess we still can change this, as we didn't have a 1.2.0 Final, yet and the source parameter of the object factory methods is something new.

So I propose that if an @ObjectFactory method with a non-annotated parameter (what we call the "source" parameter), we don't check if any of the mapping methods source parameters is assignable, but if the "source" of the target object to create is assignable. For factory-method invocations at the start of the mapping method, that can be any of the mapping methods source params as it is now. But for the case where we want to create a new instance to pass into an update method as in this example, we would only try to type-match and pass the source property value.

For the very first mapper example from above, this would mean we generate this:

    @Override
    BDTO copyB(BEntity source) {
        if ( source == null ) {
            return null;
        }

        BDTO bDTO = createB( source );

        return bDTO;
    }

    @Override
    void mergeA(AEntity source, ADTO target) {
        if ( source == null ) {
            return;
        }

        if ( source.getB() != null ) {
            if ( target.getB() == null ) {
               target.setB( createB( source.getB() ) ); // is currently generated as: target.setB( createB( source ) );
            }
            mergeB( source.getB(), target.getB() );
        }
        else {
            target.setB( null );
        }
    }

@gunnarmorling, @filiphr, @sjaakd: what do you guys think?

agudian added a commit to agudian/mapstruct that referenced this issue Mar 12, 2017

@44past4 44past4 changed the title from Source paramter for @ObjectFactory method is not well defined to Source parameter for @ObjectFactory method is not well defined Mar 13, 2017

@filiphr filiphr modified the milestones: 1.2.0.CR1, 1.2.0.Final Mar 18, 2017

@agudian agudian self-assigned this Apr 20, 2017

@filiphr filiphr modified the milestones: 1.2.0.CR1, 1.2.0.Beta3 May 17, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue Jun 30, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue Jun 30, 2017

filiphr added a commit to filiphr/mapstruct that referenced this issue Jun 30, 2017

@filiphr filiphr closed this in #1237 Jul 11, 2017

filiphr added a commit that referenced this issue Jul 11, 2017

#1131 Use SourceRHS source type for update methods factories
If a SourceRHS is present then the source type of the SourceRHS and the MappingContext parameters are considered for the factory method selection, i.e. the other source parameters are ignored
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment