Skip to content
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

Jump to bottom

PHPORM-180 Trigger events in Model::createOrFirst #2980

Closed
wants to merge 1 commit into from
Closed

PHPORM-180 Trigger events in Model::createOrFirst #2980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
* Add `mongodb` driver for Batching by @GromNaN in [#2904](https://github.com/mongodb/laravel-mongodb/pull/2904)
* Rename queue option `table` to `collection`
* Replace queue option `expire` with `retry_after`
* Trigger events in `Model::createOrFirst()` @GromNaN in [#2980](https://github.com/mongodb/laravel-mongodb/pull/2980)

## [4.3.1]

Expand Down
40 changes: 3 additions & 37 deletions src/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,13 @@
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
use InvalidArgumentException;
use MongoDB\Driver\Cursor;
use MongoDB\Laravel\Collection;
use MongoDB\Laravel\Helpers\QueriesRelationships;
use MongoDB\Laravel\Internal\FindAndModifyCommandSubscriber;
use MongoDB\Laravel\Query\AggregationBuilder;
use MongoDB\Model\BSONDocument;
use MongoDB\Operation\FindOneAndUpdate;

use function array_intersect_key;
use function array_key_exists;
use function array_merge;
use function assert;
use function collect;
use function is_array;
use function iterator_to_array;
Expand Down Expand Up @@ -218,40 +215,9 @@ public function createOrFirst(array $attributes = [], array $values = []): Model
// Apply casting and default values to the attributes
// In case of duplicate key between the attributes and the values, the values have priority
$instance = $this->newModelInstance($values + $attributes);
assert($instance instanceof Model);

/* @see \Illuminate\Database\Eloquent\Model::performInsert */
if ($instance->usesTimestamps()) {
$instance->updateTimestamps();
}

$values = $instance->getAttributes();
$attributes = array_intersect_key($attributes, $values);

return $this->raw(function (Collection $collection) use ($attributes, $values) {
$listener = new FindAndModifyCommandSubscriber();
$collection->getManager()->addSubscriber($listener);

try {
$document = $collection->findOneAndUpdate(
$attributes,
// Before MongoDB 5.0, $setOnInsert requires a non-empty document.
// This should not be an issue as $values includes the query filter.
['$setOnInsert' => (object) $values],
[
'upsert' => true,
'returnDocument' => FindOneAndUpdate::RETURN_DOCUMENT_AFTER,
'typeMap' => ['root' => 'array', 'document' => 'array'],
],
);
} finally {
$collection->getManager()->removeSubscriber($listener);
}

$model = $this->model->newFromBuilder($document);
$model->wasRecentlyCreated = $listener->created;

return $model;
});
return $instance->saveOrFirst($attributes);
Fixed Show fixed Hide fixed
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall the original reason for introducing createOrFirst() in #2742 (PHPORM-139). It looks like this was necessary to avoid some issue with transactions.

Just wanted to confirm that a custom implementation is still required at all, given that we'll no longer be using findAndModify.

Copy link
Member Author

@GromNaN GromNaN May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are still using findAndModify, but in Query\Builder::findAndModify.

The call chain is:

  • Model::createOrFirst
  • Eloquent\Query::createOrFirst
  • Model::saveOrFirst
  • Query\Builder::findAndModify
  • Collection::findAndModify

}

/**
Expand Down
67 changes: 67 additions & 0 deletions src/Eloquent/Model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace MongoDB\Laravel\Eloquent;

use BackedEnum;
use BadMethodCallException;
use Carbon\CarbonInterface;
use DateTimeInterface;
use DateTimeZone;
Expand All @@ -31,6 +32,7 @@
use function array_merge;
use function array_unique;
use function array_values;
use function assert;
use function class_basename;
use function count;
use function date_default_timezone_get;
Expand Down Expand Up @@ -759,6 +761,71 @@ public function save(array $options = [])
return $saved;
}

/** @internal Not part of Laravel Eloquent API. Use raw findOneAndModify if necessary */
public function saveOrFirst(array $criteria): ?static
{
$this->mergeAttributesFromCachedCasts();
Copy link
Member

Choose a reason for hiding this comment

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

I assume much of the contents for this method were lifted from an existing method in Eloquent. If so, can we reference that in the method doc block?

I'm a little worried that if anything changes upstream we'll probably need to find out the hard way that this needs to be updated, but that may be unavoidable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also concerned that we have a lot of duplicated code that is hard to understand and we are not sure we can maintain in the long-term.
Creating a simple alternative PR: #2984


$query = $this->newModelQuery();
assert($query instanceof Builder);

// If the "saving" event returns false we'll bail out of the save and return
// false, indicating that the save failed. This provides a chance for any
// listeners to cancel save operations if validations fail or whatever.
if ($this->fireModelEvent('saving') === false) {
return null;
}

if ($this->exists) {
throw new BadMethodCallException(sprintf('%s can be used on new model instances only', __FUNCTION__));
}

if ($this->usesUniqueIds()) {
$this->setUniqueIds();
}

if ($this->fireModelEvent('creating') === false) {
return null;
}

// First we'll need to create a fresh query instance and touch the creation and
// update timestamps on this model, which are maintained by us for developer
// convenience. After, we will just continue saving these model instances.
if ($this->usesTimestamps()) {
$this->updateTimestamps();
}

// If the model has an incrementing key, we can use the "insertGetId" method on
// the query builder, which will give us back the final inserted ID for this
// table from the database. Not all tables have to be incrementing though.
$attributes = $this->getAttributesForInsert();
Copy link
Member

Choose a reason for hiding this comment

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

Why does the comment above refer to insertGetId()? It's not clear to me how that's getting used here, unless it's through getAttributesForInsert().

Does getAttributesForInsert respect modifications from earlier model methods such as setUniqueIds() and updateTimestamps()? I assume this all pulls from the same state.


$document = $query->getQuery()
->where($criteria)
->insertOrFirst($attributes);

$connection = $query->getConnection();
if (! $this->getConnectionName() && $connection) {
$this->setConnection($connection->getName());
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for inheriting the query's connection? IS this necessary for dispatching the created event?

}

// If a document matching the criteria was found, it is returned. Nothing was saved.
if (! $document['wasRecentlyCreated']) {
return $this->newInstance($document);
}

// If the model is successfully saved, we need to do a few more things once
// that is done. We will call the "saved" method here to run any actions
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any reference to a saved method. Is this comment accurate?

// we need to happen after a model gets successfully saved right here.
$this->exists = true;
$this->wasRecentlyCreated = true;
$this->setAttribute($this->getKeyName(), $document[$this->getKeyName()]);
$this->fireModelEvent('created', false);
$this->finishSave([]);

return $this;
}

/**
* {@inheritDoc}
*/
Expand Down
28 changes: 28 additions & 0 deletions src/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
use MongoDB\BSON\UTCDateTime;
use MongoDB\Builder\Stage\FluentFactoryTrait;
use MongoDB\Driver\Cursor;
use MongoDB\Laravel\Internal\FindAndModifyCommandSubscriber;
use MongoDB\Operation\FindOneAndUpdate;
use Override;
use RuntimeException;

Expand Down Expand Up @@ -725,6 +727,32 @@ public function update(array $values, array $options = [])
return $this->performUpdate($values, $options);
}

public function insertOrFirst(array $document): array
{
$wheres = $this->compileWheres();
$listener = new FindAndModifyCommandSubscriber();

try {
$this->collection->getManager()->addSubscriber($listener);
$document = $this->collection->findOneAndUpdate(
$wheres,
// Before MongoDB 5.0, $setOnInsert requires a non-empty document.
['$setOnInsert' => (object) $document],
[
'upsert' => true,
'returnDocument' => FindOneAndUpdate::RETURN_DOCUMENT_AFTER,
'typeMap' => ['root' => 'array', 'document' => 'array'],
],
);
} finally {
$this->collection->getManager()->removeSubscriber($listener);
}

$document['wasRecentlyCreated'] = $listener->created;

return $document;
}

/** @inheritdoc */
public function increment($column, $amount = 1, array $extra = [], array $options = [])
{
Expand Down
33 changes: 33 additions & 0 deletions tests/ModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,15 +1052,20 @@ public function testCreateOrFirst()
{
Carbon::setTestNow('2010-06-22');
$createdAt = Carbon::now()->getTimestamp();
$events = [];
self::registerModelEvents(User::class, $events);

$user1 = User::createOrFirst(['email' => '[email protected]']);

$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);
$this->assertTrue($user1->wasRecentlyCreated);
$this->assertEquals($createdAt, $user1->created_at->getTimestamp());
$this->assertEquals($createdAt, $user1->updated_at->getTimestamp());
$this->assertEquals(['saving', 'creating', 'created', 'saved'], $events);

Carbon::setTestNow('2020-12-28');
$events = [];
$user2 = User::createOrFirst(
['email' => '[email protected]'],
['name' => 'John Doe', 'birthday' => new DateTime('1987-05-28')],
Expand All @@ -1073,7 +1078,9 @@ public function testCreateOrFirst()
$this->assertFalse($user2->wasRecentlyCreated);
$this->assertEquals($createdAt, $user1->created_at->getTimestamp());
$this->assertEquals($createdAt, $user1->updated_at->getTimestamp());
$this->assertEquals(['saving', 'creating'], $events);
jmikola marked this conversation as resolved.
Show resolved Hide resolved

$events = [];
$user3 = User::createOrFirst(
['email' => '[email protected]'],
['name' => 'Jane Doe', 'birthday' => new DateTime('1987-05-28')],
Expand All @@ -1086,14 +1093,17 @@ public function testCreateOrFirst()
$this->assertTrue($user3->wasRecentlyCreated);
$this->assertEquals($createdAt, $user1->created_at->getTimestamp());
$this->assertEquals($createdAt, $user1->updated_at->getTimestamp());
$this->assertEquals(['saving', 'creating', 'created', 'saved'], $events);

$events = [];
$user4 = User::createOrFirst(
['name' => 'Robert Doe'],
['name' => 'Maria Doe', 'email' => '[email protected]'],
);

$this->assertSame('Maria Doe', $user4->name);
$this->assertTrue($user4->wasRecentlyCreated);
$this->assertEquals(['saving', 'creating', 'created', 'saved'], $events);
}

public function testCreateOrFirstRequiresFilter()
Expand All @@ -1114,6 +1124,9 @@ public function testUpdateOrCreate(array $criteria)
['email' => '[email protected]'],
]);

$events = [];
self::registerModelEvents(User::class, $events);

Carbon::setTestNow('2010-01-01');
$createdAt = Carbon::now()->getTimestamp();

Expand All @@ -1127,6 +1140,8 @@ public function testUpdateOrCreate(array $criteria)
$this->assertEquals(new DateTime('1987-05-28'), $user->birthday);
$this->assertEquals($createdAt, $user->created_at->getTimestamp());
$this->assertEquals($createdAt, $user->updated_at->getTimestamp());
$this->assertEquals(['saving', 'creating', 'created', 'saved'], $events);
$events = [];

Carbon::setTestNow('2010-02-01');
$updatedAt = Carbon::now()->getTimestamp();
Expand All @@ -1142,6 +1157,7 @@ public function testUpdateOrCreate(array $criteria)
$this->assertEquals(new DateTime('1990-01-12'), $user->birthday);
$this->assertEquals($createdAt, $user->created_at->getTimestamp());
$this->assertEquals($updatedAt, $user->updated_at->getTimestamp());
$this->assertEquals(['saving', 'saved'], $events);

// Stored data
$checkUser = User::where($criteria)->first();
Expand All @@ -1168,4 +1184,21 @@ public function testUpdateOrCreateWithNullId()
['email' => '[email protected]'],
);
}

/** @param class-string<Model> $modelClass */
private static function registerModelEvents(string $modelClass, array &$events): void
{
$modelClass::creating(function () use (&$events) {
$events[] = 'creating';
});
$modelClass::created(function () use (&$events) {
$events[] = 'created';
});
$modelClass::saving(function () use (&$events) {
$events[] = 'saving';
});
$modelClass::saved(function () use (&$events) {
$events[] = 'saved';
});
}
}
Loading