PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result by GromNaN · Pull Request #1454 · mongodb/mongo-php-library

GromNaN · 2024-09-25T09:15:59Z

I chose to rely on the driver exception thrown by mongodb/mongo-php-driver#1687

src/UpdateResult.php

-            }
-
-            return null;
+        foreach ($this->writeResult->getUpsertedIds() as $id) {


alcaeus

LGTM, but we may want to consider unacknowledged writes in rename.

alcaeus · 2024-09-25T12:13:17Z

src/GridFS/Bucket.php

+        // If the update resulted in no modification, it's possible that the
+        // file did not exist, in which case we must raise an error.
+        if ($updateResult->getMatchedCount() !== 1) {


I'm not sure we should consider using unacknowledged writes a valid use case, but in case we do there's a functional change here. If we want to preserve the option to use unacknowledged writes with GridFS, we should check isAcknowledged instead of relying on the null value from getMatchedCount:

Suggested change

// If the update resulted in no modification, it's possible that the

// file did not exist, in which case we must raise an error.

if ($updateResult->getMatchedCount() !== 1) {

/* If the update resulted in no modification, it's possible that the

* file did not exist, in which case we must raise an error. Checking

* the write result's matched count will be most efficient, but fall

* back to a findOne operation if necessary (i.e. legacy writes).

*/

$found = $updateResult->isAcknowledged()

? $updateResult->getMatchedCount() === 1

: $this->collectionWrapper->findFileById($id) !== null;

if (! $found) {

In that case, that's a fix to backport to v1.x, as it already throws an exception when there is an unacknowledged operation.

In that case, let's defer to a separate PR, as this does PR not change existing behaviour.

let's defer to a separate PR

I assume a ticket needs to be created for this. Please cross-reference it here after doing so.

I just realized that this second if ($updateResult->getMatchedCount() !== 1) is always true after if ($updateResult->getModifiedCount() === 1) return;.

Do you know what are the "legacy writes" that are mentioned in the comment?

Legacy opcodes (e.g. OP_UPDATE), which were removed in MongoDB 6.0. At this point, drivers may still use them for unacknowledged writes but all acknowledged writes are guaranteed to use OP_MSG and the corresponding write commands (e.g. update).

…dged result

GromNaN · 2024-09-26T16:15:09Z

I did not update the UPGRADE file, there is nothing to upgrade.

* v2.x: PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

* v2.x: PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478) PHPLIB-1227 Use void return types for operations without meaningful result document (#1468) Remove deprecated functionality (#1439) PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

* v2.x: Regenerate evergreen configuration (#1503) PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478) PHPLIB-1227 Use void return types for operations without meaningful result document (#1468) Remove deprecated functionality (#1439) PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

* v2.x: Add return type hint for Encoder::encode() implementation Reverse pipeline init from an array (#1596) PHPLIB-1617 Accept a Pipeline instance in aggregate and watch methods (#1580) Fix CS Require latest python version (#1564) (#1565) Update src/Operation/Find.php Ignore `disableMD5` option as `md5` field is removed from the spec (#1502) Remove obsolete baseline entries Regenerate evergreen configuration (#1503) PHPLIB-1546 and PHPLIB-1159: Remove CreateCollection flags and autoIndexId options (#1478) PHPLIB-1227 Use void return types for operations without meaningful result document (#1468) Remove deprecated functionality (#1439) PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result (#1454) PHPLIB-954: Add return types to all methods (#1391) PHPLIB-797: Remove unused methods in UnsupportedException (#1436) Revert "Add final annotations to non-internal Operation classes (#1410)" PHPLIB-953 Make internal classes and Operation classes final (#1392) PHPLIB-1218 Remove deprecated fields from GridFS files (#1398)

GromNaN requested a review from a team as a code owner September 25, 2024 09:16

GromNaN requested a review from alcaeus September 25, 2024 09:16

GromNaN changed the title ~~PHPLIB-1518 WriteResult::get*Count() always return an int on acknowledged result~~ PHPLIB-1518 WriteResult::get*Count() always return an int on acknowledged result Sep 25, 2024

GromNaN force-pushed the PHPLIB-1518 branch from ef66a68 to 17c183d Compare September 25, 2024 10:11

github-advanced-security bot found potential problems Sep 25, 2024

View reviewed changes

src/UpdateResult.php

}

return null;

foreach ($this->writeResult->getUpsertedIds() as $id) {

Check notice

Code scanning / Psalm

MixedAssignment

Unable to determine the type that $id is being assigned to

alcaeus reviewed Sep 25, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1518 branch from 01a600d to 4495a6f Compare September 25, 2024 14:00

alcaeus approved these changes Sep 26, 2024

View reviewed changes

GromNaN added 3 commits September 26, 2024 16:50

PHPLIB-1518 WriteResult::get*Count() always return an int on acknowle…

8daac63

…dged result

Propagation of type changes to GridFS

6908c08

Format stubs

49fa315

GromNaN force-pushed the PHPLIB-1518 branch from 4495a6f to 49fa315 Compare September 26, 2024 14:50

GromNaN enabled auto-merge (squash) September 26, 2024 16:14

GromNaN merged commit ea818c8 into mongodb:v2.x Sep 26, 2024

GromNaN deleted the PHPLIB-1518 branch September 30, 2024 13:48

GromNaN mentioned this pull request Oct 29, 2024

PHPLIB-1568 Add GridFS\Bucket::deleteByName(filename) and renameByName(filename, newFilename) #1504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result#1454

PHPLIB-1518 `WriteResult::get*Count()` always return an int on acknowledged result#1454
GromNaN merged 3 commits intomongodb:v2.xfrom
GromNaN:PHPLIB-1518

GromNaN commented Sep 25, 2024 •

edited

Loading

Uh oh!

Check notice

alcaeus left a comment

Uh oh!

alcaeus Sep 25, 2024

Uh oh!

GromNaN Sep 25, 2024

Uh oh!

alcaeus Sep 26, 2024

Uh oh!

jmikola Sep 27, 2024

Uh oh!

GromNaN Sep 27, 2024

Uh oh!

jmikola Sep 30, 2024

Uh oh!

GromNaN commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-        // If the update resulted in no modification, it's possible that the
-        // file did not exist, in which case we must raise an error.
-        if ($updateResult->getMatchedCount() !== 1) {
+        /* If the update resulted in no modification, it's possible that the
+         * file did not exist, in which case we must raise an error. Checking
+         * the write result's matched count will be most efficient, but fall
+         * back to a findOne operation if necessary (i.e. legacy writes).
+         */
+        $found = $updateResult->isAcknowledged()
+            ? $updateResult->getMatchedCount() === 1
+            : $this->collectionWrapper->findFileById($id) !== null;
+        if (! $found) {

Conversation

GromNaN commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check notice

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

alcaeus Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

GromNaN Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

alcaeus Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

jmikola Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

GromNaN Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

jmikola Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GromNaN commented Sep 25, 2024 •

edited

Loading