-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Model::createOrFirst()
using findOneAndUpdate operationModel::createOrFirst()
using findOneAndUpdate
operation
1ca7d46
to
0eec67b
Compare
|
||
public function testCreateOrFirst() | ||
{ | ||
$user1 = User::createOrFirst(['email' => '[email protected]']); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from Laravel's test suite. https://github.com/laravel/framework/blob/a518ef66bebd04c97fa1f04cb3a450795cc3ca11/tests/Database/DatabaseEloquentIntegrationTest.php#L527
public function createOrFirst(array $attributes = [], array $values = []): Model | ||
{ | ||
// Apply casting and default values to the attributes | ||
$instance = $this->newModelInstance($values + $attributes); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
try { | ||
$document = $collection->findOneAndUpdate( | ||
$attributes, | ||
['$setOnInsert' => $values], |
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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 executingfindOneAndUpdate()
, but I imagine it wouldn't play nice withBuilder::newModelInstance()
.
$parameters
and $values
are supposed to be used to create a Model instance. Using a query operator would be incorrect.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
$document = $collection->findOneAndUpdate( | ||
$attributes, | ||
['$setOnInsert' => $values], | ||
['upsert' => true, 'new' => true, 'typeMap' => ['root' => 'array', 'document' => 'array']], |
There was a problem hiding this comment.
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
.
$collection->getManager()->removeSubscriber($listener); | ||
} | ||
|
||
$model = $this->model->newFromBuilder($document); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* 4.2: PHPORM-139 Implement `Model::createOrFirst()` using `findOneAndUpdate` operation (#2742) Test Laravel 10 and 11 (#2746) PHPORM-150 Run CI on Laravel 11 (#2735) PHPORM-152 Fix tests for Carbon 3 (#2733)
Fix PHPORM-139
Fix #2720
This may be considered as a new feature and merged into 4.2 instead of 4.1
The
createOfFirst
algorithm is:This is precisely what we can do in a single command with
findOneAndUpdate
.Unlike Eloquent's implementation, we don't rely on the "unique" index constraint, and just use the
$attribute
array as criteria.Checklist