Framework: Factory creates extra related model when saving via relation

[sebastiaanluca]

• Laravel Version: 5.4.23
• PHP Version: 7.1.2

Description:

When creating e.g. 10 models using a factory and saving one related child model for each of those 10 through their relation, it creates 10 + 10 parent models instead of just the initial 10. This is however not desired and not expected either (docs state otherwise if I read it correctly). The problem also scales exponentially when creating more models or having more similar relations.

The expected result would be that if one passes the parent ID to the child or uses a relation to save the child factory model, the parent ID should be set automatically. Resulting in an identical workflow when not using factories (e.g. saving a child via its parent model's relation sets the ID before persisting to the database).

The situation can be fixed manually for now by explicitly passing the parent model ID when creating the child models, thus overriding the attribute and the creation of a related parent model in the child factory.

Steps To Reproduce:

Production example (when testing and seeding):

// Let's create 10 challenges
$challenges = factory(Challenge::class, 10)->create();

// Make (not create) an order for each challenge and save it through its relation
$challenges->each(function (Challenge $challenge) {
    $challenge->order()->save(factory(Order::class)->make());

    // The following works as intended. We can even ditch saving
    // it via the relation too and just use the factory to create
    // the child models if we want.
    //    $challenge->order()->save(factory(Order::class)->make([
    //        // The situation can be fixed by explicitly passing the parent model ID,
    //        // thus overriding the attribute and the creation of a related parent
    //        // model in the child factory.
    //        'challenge_id' => $challenge->id,
    //    ]));
});

The factories:

$factory->define(Challenge::class, function (Generator $faker) {
    return [
        'user_id' => factory(User::class),
    ];
});

$factory->define(Order::class, function (Generator $faker) {
    return [
        // In order to have the ability to create orders without creating
        // challenges first, use a factory to create them automatically.
        'challenge_id' => factory(Challenge::class),
    ];
});

The related models:

class Challenge extends \Illuminate\Database\Eloquent\Model
{
    /**
     * @return \Illuminate\Database\Eloquent\Relations\HasOne
     */
    public function order() : HasOne
    {
        return $this->hasOne(Order::class);
    }

    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function user() : BelongsTo
    {
        return $this->belongsTo(User::class);
    }
}

class Order extends \Illuminate\Database\Eloquent\Model
{
    /**
     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
     */
    public function challenge() : BelongsTo
    {
        return $this->belongsTo(Challenge::class);
    }
}

[RikSomers]

Try changing

$factory->define(Order::class, function (Generator $faker) {
    return [
        'challenge_id' => factory(Challenge::class),
    ];
});

to

$factory->define(Order::class, function (Generator $faker) {
    return [
        'challenge_id' => function() { 
            return factory(Challenge::class)->create()->id;
         }
    ];
});

Otherwise the factory will always be called, even when overwriting the value. The second example will only execute the function when no challenge_id is provided.

[sebastiaanluca]

@RikSomers Same result (although it should be factory(Challenge::class)->create()->id IIRC). My example was for the latest version of Laravel where you can now simply set a factory and it'll automatically determine if it needs to create one or not.

[RikSomers]

Wait, you first create 10 challenges, then for each of those call the Order factory which then (as part of it's factory) creates a Challenge again. That's why your commented out example does work: it overrides the challenge_id, thus not calling the factory a second time. Or am I missing something? Looks like intended behaviour to me?

[sebastiaanluca]

Well yeah, that's my point :p Saving it via the relation of a model should set the challenge_id on each order, preventing it from creating another 10 for each order. This is how saving models using relations works without factories, so I wrongfully assumed it would be the same when using them.

I'm not even sure why it doesn't set the ID as it's just a model being saved by its parent.

Edit: probably because $challenge->order()->save(factory(Order::class)->make()); makes the order (and another challenge) before saving it via its parent. Would be great for it to do this automagically though.

[devcircus]

There's been a lot of work done in this area over the past few weeks. Search pull requests for "factory" and look at the recently closed PRs. It seems like I remember this being one of the issues that was addressed. May be for 5.5 though, I'm not sure.

[themsaid]

You'll need to override the challenge_id:

$challenge->order()->save(factory(Order::class)->make(['challenge_id' => null]));

Without the factory builder will create a challenge to use its id while building the Order as you instructed it to do that earlier.

Closing since it's not a bug.

[sebastiaanluca]

Ok, thanks for the response and confirmation, everyone. Would've been useful to have a 'solution' for it, but this'll work too.

[joeystandrew]

This is old, but ran into this issue recently and found a fix. If you specify only a single model within the factory function, it should work as expected. Below is an example.

factory(App\Models\Plan::class, 1)->create([])->each()

[georgeboot]

It would be ideal if the closures with default values, only evaluate when the model is persisted, not when it's created. Doing so, you allow the model some time to have it's properties set.

The current solution somewhat messes up my dev database.

Yes, I can override the properties in the make() method, but now I'm adding relational logic to my tests everywhere, whereas that is already handled by Eloquent + Factory. Not a huge problem tough, just somewhat annoying.

[eko3alpha]

Laravel v6.0.6
PHP v7.2.21

Yes, I know this thread is old...

I recently ran into this situation when trying to create my seeder file. I had my factory definitions set up as:

$factory->define(Reply::class, function (Faker $faker) {
    return [
        'user_id' => factory(User::class)->create()->id,
        'thread_id' => factory(Thread::class)->create()->id,
        'body' => $faker->paragraph
    ];
});

Even though I was passing the thread and user id, it was still generating an extra thread and user. After changing my factory definition as @RikSomers suggested:

$factory->define(Reply::class, function (Faker $faker) {
    return [
        'user_id' => function () {
            return factory(User::class)->create()->id;
        },
        'thread_id' => function () {
            return factory(Thread::class)->create()->id;
        },
        'body' => $faker->paragraph
    ];
});

I went ahead and put all the foreign key relations inside a closure and now things work as expected.

[jasonmccreary]

Wanted to chime in since I was directed here as an example of the code in my tweet not working (when it does).

All of the noted examples of this _not working_ (generating double records) suffer from the same issue of chaining the factory method. Doing so results in its immediate evaluation.

You may use the shorthand and it will not generate "double records", but you must set the field directly to the factory method as exemplified in the documentation.

Example:

$factory->define(Post::class, function (Faker $faker) {
    return [
        // shorthand:
        'user_id' => factory(User::class),
        // or longhand:
        'user_id' => function () {
            return factory(User::class)->create()->id;
        },

        // but not:
        'user_id' => factory(User::class)->create()->id,
        // or:
        'user_id' => factory(User::class)->create(),
    ];
});