Skip to content

PHPORM-139 Implement Model::createOrFirst() using findOneAndUpdate operation #2742

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.

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

Merged
merged 4 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.
## [unreleased]

* Add support for Laravel 11 by @GromNaN in [#2735](https://github.com/mongodb/laravel-mongodb/pull/2735)
* Implement Model::createOrFirst() using findOneAndUpdate operation by @GromNaN in [#2742](https://github.com/mongodb/laravel-mongodb/pull/2742)

## [4.1.3] - 2024-03-05

Expand Down
38 changes: 38 additions & 0 deletions src/Eloquent/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
use Illuminate\Database\ConnectionInterface;
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
use MongoDB\Driver\Cursor;
use MongoDB\Laravel\Collection;
use MongoDB\Laravel\Helpers\QueriesRelationships;
use MongoDB\Laravel\Internal\FindAndModifyCommandSubscriber;
use MongoDB\Model\BSONDocument;

use function array_intersect_key;
use function array_key_exists;
use function array_merge;
use function collect;
Expand Down Expand Up @@ -183,6 +186,41 @@ public function raw($value = null)
return $results;
}

/**
* Attempt to create the record if it does not exist with the matching attributes.
* If the record exists, it will be returned.
*
* @param array $attributes The attributes to check for duplicate records
* @param array $values The attributes to insert if no matching record is found
*/
public function createOrFirst(array $attributes = [], array $values = []): Model
{
// Apply casting and default values to the attributes
$instance = $this->newModelInstance($values + $attributes);
Copy link
Member

Choose a reason for hiding this comment

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

When doing $values + $attributes, preference is given to an existing key in $values. In that case, it seems possible that actual criteria in $attributes might be lost.

Is there a particular reason you didn't do $attributes + $values?

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'd say the behavior is not well defined if there is a key duplicate between $attributes and $values. If we look at the implementation in Laravel, it tries to create with an array_merge($attributes, $values), fails if there is a uniqueness constraint in DB and tries to find using $attributes.

I used this merge order $values + $attribute === array_merge($attributes, $values) to please the unit test that I imported.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a possible Laravel bug if you want to pursue that.

$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,
['$setOnInsert' => $values],
Copy link
Member

Choose a reason for hiding this comment

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

Since $values is over-written above, its value here includes the union of fields from the $attributes and $values parameters. This is a bit redundant, since upserts already apply the criteria to a new document, but I suppose it makes no functional difference.

Just to confirm, what prevents a user from using query syntax in the $attributes parameter? It'd logically work for executing findOneAndUpdate(), but I imagine it wouldn't play nice with Builder::newModelInstance().

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't know the $filter argument was used to create the document, good to know.

Just to confirm, what prevents a user from using query syntax in the $attributes parameter? It'd logically work for executing findOneAndUpdate(), but I imagine it wouldn't play nice with Builder::newModelInstance().

$parameters and $values are supposed to be used to create a Model instance. Using a query operator would be incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

If $values is empty, I get this server error:

MongoDB\Driver\Exception\CommandException: Modifiers operate on fields but we found type array instead. For example: {$mod: {: ...}} not {$setOnInsert: []}

If I try to use an empty update, the error is:

MongoDB\Exception\InvalidArgumentException: Expected update operator(s) or non-empty pipeline for $update

I could add a ternary condition to use $attribute as fallback if $values is empty, but I do prefer to be explicit and merge all values all the time.

Copy link
Member

Choose a reason for hiding this comment

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

Modifiers operate on fields but we found type array instead.

That could be fixed with an explicit (object) cast on the $setOnInsert value. Note that server versions prior to 5.0 would still error if that object is empty (this is noted in the $setOnInsert docs).

Expected update operator(s) or non-empty pipeline for $update

Also addressable with an explicit object cast on the value; however, it's secondary to the issue above so not worth looking into.

['upsert' => true, 'new' => true, 'typeMap' => ['root' => 'array', 'document' => 'array']],
Copy link
Member

Choose a reason for hiding this comment

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

new is an option on the internal FindAndModify operation. Since this is using FindOneAndUpdate, the correct option to use would be returnDocument with a value of FindOneAndUpdate::RETURN_DOCUMENT_AFTER.

);
} finally {
$collection->getManager()->removeSubscriber($listener);
}

$model = $this->model->newFromBuilder($document);
Copy link
Member

Choose a reason for hiding this comment

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

How does this method differ from Builder::newModelInstance()? It looks like both are used to construct new model instances from "raw" document data.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Eloquent\Builder::newModelInstance() applies cast on the attributes. Values comes from the developer.
  • Eloquent\Model::newFromBuilder() uses data from the database and doesn't apply changes.

$model->wasRecentlyCreated = $listener->created;

return $model;
});
}

/**
* Add the "updated at" column to an array of values.
* TODO Remove if https://github.com/laravel/framework/commit/6484744326531829341e1ff886cc9b628b20d73e
Expand Down
34 changes: 34 additions & 0 deletions src/Internal/FindAndModifyCommandSubscriber.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

namespace MongoDB\Laravel\Internal;

use MongoDB\Driver\Monitoring\CommandFailedEvent;
use MongoDB\Driver\Monitoring\CommandStartedEvent;
use MongoDB\Driver\Monitoring\CommandSubscriber;
use MongoDB\Driver\Monitoring\CommandSucceededEvent;

/**
* Track findAndModify command events to detect when a document is inserted or
* updated.
*
* @internal
*/
final class FindAndModifyCommandSubscriber implements CommandSubscriber
{
public bool $created;

public function commandFailed(CommandFailedEvent $event)
{
}

public function commandStarted(CommandStartedEvent $event)
{
}

public function commandSucceeded(CommandSucceededEvent $event)
{
$this->created = ! $event->getReply()->lastErrorObject->updatedExisting;
}
}
39 changes: 39 additions & 0 deletions tests/ModelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1044,4 +1044,43 @@ public function testNumericFieldName(): void
$this->assertInstanceOf(User::class, $found);
$this->assertEquals([3 => 'two.three'], $found[2]);
}

public function testCreateOrFirst()
{
$user1 = User::createOrFirst(['email' => '[email protected]']);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where these addresses came from but we'd do well to use @example.com in email data fixtures.

Copy link
Member Author

Choose a reason for hiding this comment

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


$this->assertSame('[email protected]', $user1->email);
$this->assertNull($user1->name);
$this->assertTrue($user1->wasRecentlyCreated);

$user2 = User::createOrFirst(
['email' => '[email protected]'],
['name' => 'Taylor Otwell', 'birthday' => new DateTime('1987-05-28')],
);

$this->assertEquals($user1->id, $user2->id);
$this->assertSame('[email protected]', $user2->email);
$this->assertNull($user2->name);
$this->assertNull($user2->birthday);
$this->assertFalse($user2->wasRecentlyCreated);

$user3 = User::createOrFirst(
['email' => '[email protected]'],
['name' => 'Abigail Otwell', 'birthday' => new DateTime('1987-05-28')],
);

$this->assertNotEquals($user3->id, $user1->id);
$this->assertSame('[email protected]', $user3->email);
$this->assertSame('Abigail Otwell', $user3->name);
$this->assertEquals(new DateTime('1987-05-28'), $user3->birthday);
$this->assertTrue($user3->wasRecentlyCreated);

$user4 = User::createOrFirst(
['name' => 'Dries Vints'],
['name' => 'Nuno Maduro', 'email' => '[email protected]'],
);

$this->assertSame('Nuno Maduro', $user4->name);
$this->assertTrue($user4->wasRecentlyCreated);
}
}