Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Nested mapping, where collection element needs to have parent assigned to its property #16

Open
ostrolucky opened this issue Apr 10, 2018 · 6 comments

Comments

@ostrolucky
Copy link

ostrolucky commented Apr 10, 2018

In following example, we didn't find a way how to replicate following in Automapper. We don't think it's possible without very custom things.

$answerSetDTO = ...
$duplicatedAnswerSetDTO = $this->autoMapper->map($answerSetDTO, AnswerSetDTO::class);

$optionsDTO = [];
foreach ($answerSetDTO->options as $optionDTO) {
    $optionDTO = $this->autoMapper->map($optionDTO, AnswerSetOptionDTO::class);
    $optionDTO->answerSet = $duplicatedAnswerSetDTO; // <- here is a complication
    $optionsDTO[] = $optionDTO;
}

$duplicatedAnswerSetDTO->options = new ArrayCollection($optionsDTO);

Using Operation::mapTo like following is close, but $optionsDTO#answerSet will be null, or it will just copy same object over:

// cloning
$autoMapperConfig->registerMapping(AnswerSetDTO::class, AnswerSetDTO::class)
    ->forMember('id', Operation::ignore())
    ->forMember('options', Operation::mapTo(AnswerSetOptionDTO::class))
    ->forMember('questions', Operation::ignore());

$autoMapperConfig->registerMapping(AnswerSetOptionDTO::class, AnswerSetOptionDTO::class)
    ->forMember('id', Operation::ignore())
// here is second culprit. Causes answerSet to be null, 
// or same object instead of parent one if Operation::ignore() is removed
// how to assign parent object id instead?
    ->forMember('answerSet', Operation::ignore()); 

So, if $answerSetDTO#options[0]#answerSet#id = 1 and $duplicateAnswerSetDTO#id = 2, we want $duplicateAnswerSetDTO#options[0]#answerSet#id = 2. Currently, we can achieve it to be null or 1 only.

@mark-gerarts
Copy link
Owner

You are right, you can't do this with the Automapper (yet). It is, however, an interesting usecase. I have some ideas on how to resolve this, but I'm gonna need some time to test things out.

Thanks for bringing this under my attention, I'll keep you posted!

@ostrolucky
Copy link
Author

Hey mark, any news about this? Would love to hear about your ideas at least!

@mark-gerarts
Copy link
Owner

Hi @ostrolucky,

I've been playing around with this for a while, but I haven't found a good solution. My idea was the following:

  1. Create a cache for mapped items based on their object reference
  2. When mapping a property, check if it references an object that exists in this cache, and use the cached version
  3. Otherwise add the result to the cache

I was able to implement this, and it worked just fine for your use case (child properties referencing their parent). It was, however, far from perfect. Some problems encountered:

  • Child properties referencing another child property would be ignored by this method
  • It only worked if the child property was passed to MapTo
  • Code-wise, the solution was far from extensible

I think it is therefore best to not resolve references automagically, but instead we could rely on some mechanism to explicitly define references you want to keep. Maybe something using the Symfony property accessor. It could look something like this:

$config->registerMapping(Parent::class, ParentDto::class)
    ->forMember('child', Operation::mapTo(ChildDto::class)
    ->withReference('child.parent', 'this')

This is just an idea, I don't know if it would be possible to implement. I can see some problems with this approach:

  • We'd be restricted by the symfony property accessor, so no accessing private properties that don't have a getter
  • It'd be tough handling arrays (e.g. in your case options.answerset wouldn't work because options is an array).

So, as a conclusion, I'm kind of stuck on this. Automatically resolving references doesn't look like a good idea, and manually defining references might take a long while to implement, if it's even possible. I'm going to experiment a bit with this though. In the meanwhile I'm open to other suggestions :)

I'm sorry I can't help you with this for now.

@ostrolucky
Copy link
Author

it is therefore best to not resolve references automagically

Agreed 100%. I'm definitely not asking to try to guess this automatically, that would produce false positives. I just want some way to configure mapping for this use case.

Something I had in mind was to allow to use custom closures where we could do anything we want. In this example, it could be used following way:

$config->registerMapping(Child::class, Child::class)
    ->forMember('parent', Operation::custom(function(Child $child, Parent $parent) {
        return $parent;
    })

or even for arrays via parent

$config->registerMapping(Parent::class, Parent::class)
    ->forMember('children', Operation::custom(function(Parent $parent) {
        $out = [];
        foreach ($parent->getChildren() as $child) {
           $child = $this->automapper->map($child, Child::class);
           $child->setParent($parent);
           $out[] = $child;
        }

        return $out;
    })

I like PropertyAccess component usage idea as well, mainly because I would wish automapper used it in the first place, as that interface is more standard. You could just decorate it and use reflection as you do now. This is already done in some projects, e.g. nelmio/alice. Even better, use alternative Closure::bind approach, which is faster.

Ultimately, for use cases like in this issue, custom closure is most flexible though.

@mark-gerarts
Copy link
Owner

That's a really great suggestion. I'll definitely look into using a decorated PropertyAccess component. nelmio/alice is a great example as well.

For the problem with the references, however, I don't see how your example could be a solution. Normally, an operation would receive the object being mapped. So Parent $parent in your second snippet would be the source object, and $child->setParent($parent); would make the children of the mapped object have references to the original source object. Unless you want the custom operation to be passed the resulting object?

@ostrolucky
Copy link
Author

Yes, idea is to pass resulting object and its parent

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants