zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
669 stars 75 forks source link

How to make sure Entity association is valid from both sides? #531

Open janopae opened 12 months ago

janopae commented 12 months ago

If you have a bidirectional OneToMany/ManyToOne assoziation between two entities, you need to make sure it gets updated on both sides. Your code might look like this:

<?php

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity()]
class Category
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column(type: 'string')]
    private string $id;

    #[ORM\Column(type: 'string', length: 255)]
    private string $name;

    #[ORM\OneToMany(targetEntity: Post::class)]
    private Collection $posts;

    public function __construct(string $name)
    {
        $this->posts = new ArrayCollection();
        $this->name = $name;
    }

    public function addPost(Post $post): void
    {
        if ($this !== $post->getCategory()) {
            throw new InvalidArgumentException();
        }

        if ($this->posts->contains($post)) {
            return;
        }

        $this->posts->add($post);
    }
}

#[ORM\Entity()]
class Post
{
    #[ORM\Id]
    #[ORM\GeneratedValue]
    #[ORM\Column(type: 'string')]
    private $id;

    #[ORM\ManyToOne(targetEntity: Category::class)]
    #[ORM\JoinColumn]
    private $category;

    public function __construct(Category $category)
    {
        $this->category = $category;
        $category->addPost($this);
    }

    public function getCategory(): Category
    {
        return $this->category;
    }
}

If you want to create an instance, just do

$category = new Category('test');
$post = new Post($category);

And both sides of the association will be up to date.

Now if you want to create a category in Foundry, you'd probably like to do something like this:

CategoryFactory::create([
    'posts' => [
        PostFactory::create(),
        PostFactory::create(),
    ],
]);

How can you write the Factories in such a way that they pass the right Category into the Post's constructor?

janopae commented 12 months ago

I already thought about using the beforeInstantiate hook in the CategoryFactory, but you can't provide a reference to the Category there, as this is before the Category gets instantiated.

afterIntantiate is too late, as the Posts will also have already been instantiated at his point, using the wrong Category.

I think I'd need to hook in after the Category is created, but before its attributes get normalized.

As I think the attributes get normalized before the category gets created, maybe I need to prevent the posts attribute from being normalized, and normalise them myself in the afterIntantiate hook?

Sadly, setting them as extraAttributes does not prevent them from being normalised.

nikophil commented 12 months ago

Hello @janopae

indeed, unfortunately there is currently no easy solution to do it.

However, I'd thought you'de be able to do it with afterInstantiate and extraAttributes :thinking:

have you tried to use another name than posts? a name which is not a field in your entitiy?

nikophil commented 12 months ago

I recently had the case and resolved it like this:

class Program{}
class ProgramTranslation
{
    public function __construct(private Program $program)
    {
        $program->addProgramTranslation($program);
    }
}

// ProgramFactory
    public function withTranslations(Language ...$languages): static
    {
        return $this->addState([
            'programTranslationsFactory' => static fn (Program $program) => ProgramTranslationFactory::new()->sequence(
                \array_map(
                    static fn (Language $language) => ['language' => $language, 'program' => $program],
                    $languages
                )
            )->create(),
        ]);
    }

    public function initialize(): self
    {
        return $this->instantiateWith(
            (new Instantiator())
                ->alwaysForceProperties()
                ->allowExtraAttributes(['programTranslationsFactory'])
        )->afterInstantiate(
            static function (Program $p, array $attributes) {
                if (!isset($attributes['programTranslationsFactory'])) {
                    return;
                }

                ($attributes['programTranslationsFactory'])($p);
            }
        );
    }
OskarStark commented 12 months ago

This should be added to the docs I guess 🙏

nikophil commented 12 months ago

Hi Oskar,

I think you're right. However, I think it will be fixed in foundry 2.0

janopae commented 11 months ago

@nikophil Thanks for the solution/workaround! Yes, that works – by wrapping it in a callback, I can successfully keep the instance from being automatically created before afterInstantiate.

I generalised it a bit, so it handles every attempt to set the ‘translations' attribute correctly – the withTranslations method now only needs to

return $this->addState(['translations' => ProgramTranslationFactory::new()->sequence(
                \array_map(
                    static fn (Language $language) => ['language' => $language, 'program' => $program],
                    $languages
                )
            )
]);

And from the outside, you can assign any ProgramTranslationFactoryto 'translations'. It even works when setting it from getDefaults.

This version of the code looks something like this:


class ProgramFactory
{
    protected function initialize(): self
    {
        return $this
            ->instantiateWith((new Instantiator())->allowExtraAttributes(['translations']))
            ->beforeInstantiate(function (array $attributes) {
                if (!isset($attributes['translations'])) {
                    return $attributes;
                }

                $translationFactories = $attributes['translations'];

                $attributes['translations'] = function (Program $program) use ($translationFactories): void {
                    if ($translationFactories instanceof FactoryCollection) {
                        $translationFactories = $translationFactories->all();
                    }

                    foreach ($translationFactories as $translationFactory) {
                        if (!$translationFactory instanceof TranslationFactory) {
                            throw new \InvalidArgumentException('Got ' . get_debug_type($translationFactory));
                        }

                        $translationFactory->create(['program' => $program]);
                    }
                };

                return $attributes;
            })
            ->afterInstantiate(function (Program $program, array $attributes): void {
                if (!isset($attributes['translations'])) {
                    return;
                }

                $attributes['translations']($program);
            });
    }
}

Edit: I generalised the implementation even further:

/**
 * @see https://github.com/zenstruck/foundry/issues/531
 *
 * Usage:
 *
 * In the `initialize()` function of your factory:
 *
 * ```
 * return $this
 *          ->instantiateWith((new Instantiator())->allowExtraAttributes(['children']))
 *          ->beforeInstantiate(ChildRelationHelper::prepareCreationOfChildEntitiesWithAReferenceToParent('children', 'parent'))
 *          ->afterInstantiate(ChildRelationHelper::createChildEntitiesWithAReferenceToParent('children'))
 *      ;
 *```
 *
 */
class ChildRelationHelper
{
    /**
     * Prevents objects of a child relation to be created without a reference to their parent in a factory, and prepares
     * the creation using {@see self::createChildEntitiesWithAReferenceToParent() } if passed to {@see ModelFactory::$beforeInstantiate}.
     *
     * Requires the instantiator passed to {@see ModelFactory::instantiateWith()} to have {@see Instantiator::allowExtraAttributes()}
     * set with $childRelationNameOnParent.
     */
    final public static function prepareCreationOfChildEntitiesWithAReferenceToParent(string $childRelationNameOnParent, string $parentRelationNameOnChild): callable
    {
        return static function (array $attributes) use ($childRelationNameOnParent, $parentRelationNameOnChild) {
            if (!isset($attributes[$childRelationNameOnParent])) {
                return $attributes;
            }

            $childFactories = $attributes[$childRelationNameOnParent];

            $attributes[$childRelationNameOnParent] = static function ($parent) use ($parentRelationNameOnChild, $childFactories): void {
                if ($childFactories instanceof FactoryCollection) {
                    $childFactories = $childFactories->all();
                }

                foreach ($childFactories as $childFactory) {
                    if (!$childFactory instanceof ModelFactory) {
                        throw new \InvalidArgumentException('Got '.get_debug_type($childFactory));
                    }

                    $childFactory->create([$parentRelationNameOnChild => $parent]);
                }
            };

            return $attributes;
        };
    }

    /**
     * Creates instances of a child relation with a reference to its parent if provided to { @see ModelFactory::afterInstantiate() }.
     * Requires creation to be prepared using {@see self::prepareCreationOfChildEntitiesWithAReferenceToParent() }.
     */
    final public static function createChildEntitiesWithAReferenceToParent(string $childRelationNameOnParent): callable
    {
        return function ($parent, array $attributes) use ($childRelationNameOnParent): void {
            if (!isset($attributes[$childRelationNameOnParent])) {
                return;
            }

            $attributes[$childRelationNameOnParent]($parent);
        };
    }
}
janopae commented 11 months ago

I think you're right. However, I think it will be fixed in foundry 2.0

Because I'm curious: How will this use case be addressed in foundry 2.0?

nikophil commented 11 months ago

I think the creation of children is delegated to a post persist callback.

nikophil commented 11 months ago

hey @janopae

it appears it is even easier to directly use a "after instantiate" callback without even adding an extra parameter:

// ProgramFactory
public function withTranslation(array $translationAttributes): static
{
    return $this->afterInstantiate(
        static function (Program $program) use ($translationAttributes) {
            ProgramTranslationFactory::new([
                'program' => $program,
                ...$translationAttributes
            ])
                ->create();
        }
    );
}

// in some test
ProgramFactory::new()
    ->withTranslation([/** some attributes for translation 1 */])
    ->withTranslation([/** some attributes for translation 2 */])
    ->create()

I like how it fits well with the fluent interfaces.

you can even test if array_is_list($translationAttributes) this way you can call ->sequence($translationAttributes)->create() over create()