Skip to content

[10.x] Add insertWithCasts() to Eloquent Builder #46798

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[10.x] Add insertWithCasts() to Eloquent Builder #46798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8feef8e
adds Builder@insertWithCasts & tests
cosmastech Apr 16, 2023
d346693
unguard model before setting properties
cosmastech Apr 16, 2023
6109648
improve tests
cosmastech Apr 16, 2023
5cd2f21
style
cosmastech Apr 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/Illuminate/Database/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,6 +997,45 @@ public function create(array $attributes = [])
});
}

/**
* Insert new records into the database, casting properties and setting timestamps where applicable.
*
* @param iterable $values
* @return bool
*/
public function insertWithCasts(iterable $values): bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the following setup:

$models = collect();
foreach ($values as $key => $attributes) {
    $model = $this->newModelInstance($attributes);
    if ($model->usesTimestamps()) {
        $model->updateTimestamps();
    }
    $models[] = $model;
}
// Trigger pre-events
$this->toBase()->insert($models->map(fn ($m) => $m->getAttributes());
// Trigger post-events

This uses more of the Eloquent methods and is more of a createMany function (it isn't that much different from your code actually). Also getAttributes() is used internally to create insert statements:

framework/src/Illuminate/Database/Eloquent/Model.php

Line 1297 in 997218b

$attributes = $this->getAttributesForInsert();
)
(which routes to the public fn getAttributes())

There are some remaining questions with triggering the events though, e.g. what happens when one event returns false? In current situations, the update fails so it might be wise to use the same thing here.

Also the unguarded() wrapper can probably be removed since I believe its expected to be guarded by default. You can wrap it yourself in the outer function:

User::unguarded(function() {
    User::createMany($values);
});

Just spouting some ideas here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bert-w thanks for the thoughtful comments!

Timestamps

I wanted to avoid creating a new set of timestamps for something that would essentially be static across all of the models being inserted. Not a make or break, and would be amenable to making that change. Just seemed like unnecessary overhead.

Use of unguarded()

This is here so that the call to Model@fill() doesn't discard attributes that are being passed into newModelInstance(), even if they are guarded. If we were to change it to createMany then that'd be great and would happily implement your suggestion. 👍

The way Builder@insert() works is that it let's you pass anything to be inserted, guarded or not. I was just keeping with that convention.

An aside

There's also an issue I never noticed before, which is if you pass two arrays that have different keys, MySQL throws an exception.

Album::insert([
    ['title' => 'Kind of Blue', 'genre' => 'jazz'],
    ['title' => 'Highway 61 Revisited', 'artist' => 'Bob Dylan', 'genre' => 'rock']
]);
Illuminate\Database\QueryException  SQLSTATE[21S01]: Insert value list does not match column list: 1136 Column count doesn't match value count at row 2 (Connection: mysql, SQL: insert into `albums` (`genre`, `title`) values (jazz, Kind of Blue), (Bob Dylan, rock, Highway 61 Revisited)).

(I feel there should be a PR for adding a DEFAULT value to an insert/update/etc statement, but well beyond the scope of this PR).

I guess we'll wait to see what the framework maintainers say. I think this could also be improved by letting the caller pass an array or collection of models that need persisted (in case they need to set the state with their own Model methods or something).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized, I don't think you can get the last inserted ID for a for multiple records inserted in a single INSERT statement.

So we wouldn't be able to write createMany() (that emits events) unless we did them one by one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's actually an interesting workaround: you can use last-inserted-id to get the id of the first row of the last batch. Since the batch is one atomic query, the ids are bound to be sequential (assuming mysql+innodb) so the other rows can be simply calculated. But this might introduce inconsistencies across other db drivers since im not sure what happens there (however the integration test should cover it since it runs for all supported db drivers).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit too presumptuous I'm afraid. Within MySQL, you can actually change your auto-increment increment value to something other than 1.

I don't know a lot about PostgreSQL, but I think that engine allows for a SERIAL column type for specifying how you might want to auto-increment.

{
if (empty($values)) {
return true;
}

if (! is_array(reset($values))) {
$values = [$values];
}

$modelInstance = $this->newModelInstance();
$timestampColumns = [];

if ($modelInstance->usesTimestamps()) {
$now = $modelInstance->freshTimestamp();

if ($createdAtColumn = $modelInstance->getCreatedAtColumn()) {
$timestampColumns[$createdAtColumn] = $now;
}
if ($updatedAtColumn = $modelInstance->getUpdatedAtColumn()) {
$timestampColumns[$updatedAtColumn] = $now;
}
}

$this->model->unguarded(function () use (&$values, $timestampColumns) {
foreach ($values as $key => $value) {
$values[$key] = $this->newModelInstance(array_merge($timestampColumns, $value))->getAttributes();
}
});

return $this->toBase()->insert($values);
}

/**
* Save a new model and return the instance. Allow mass-assignment.
*
Expand Down
41 changes: 41 additions & 0 deletions tests/Database/DatabaseEloquentIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2101,6 +2101,47 @@ public function testMorphPivotsCanBeRefreshed()
$this->assertSame('primary', $pivot->taxonomy);
}

public function testCanCreateMany()
{
DB::enableQueryLog();

$this->assertTrue(EloquentTestUser::insertWithCasts([
['email' => 'taylorotwell@gmail.com', 'birthday' => null],
['email' => 'someperson@fake.com', 'birthday' => new Carbon('1980-01-01')],
['email' => 'adifferentpersonbutequally@fake.com', 'birthday' => '2001-12-29'],
]));

$this->assertCount(1, DB::getQueryLog());

$this->assertCount(3, $users = EloquentTestUser::get());

$users->each(function (EloquentTestUser $user) {
$this->assertInstanceOf(\DateTime::class, $user->created_at);
$this->assertInstanceOf(\DateTime::class, $user->updated_at);
});

$this->assertNull($users[0]->birthday);
$this->assertInstanceOf(\DateTime::class, $users[1]->birthday);
$this->assertInstanceOf(\DateTime::class, $users[2]->birthday);
$this->assertEquals('2001-12-29', $users[2]->birthday->format('Y-m-d'));

DB::flushQueryLog();

$this->assertTrue(EloquentTestWithJSON::insertWithCasts([
['id' => 1, 'json' => ['album' => 'Keep It Like a Secret', 'release_date' => '1999-02-02']],
['id' => 2, 'json' => (object) ['album' => 'You In Reverse', 'release_date' => '2006-04-11']],
]));

$this->assertCount(1, DB::getQueryLog());

$this->assertCount(2, $testsWithJson = EloquentTestWithJSON::get());

$testsWithJson->each(function (EloquentTestWithJSON $testWithJson) {
$this->assertIsArray($testWithJson->json);
$this->assertArrayHasKey('album', $testWithJson->json);
});
}

/**
* Helpers...
*/
Expand Down