Skip to content

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

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

Merged
merged 3 commits into from
Sep 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8daac63
PHPLIB-1518 WriteResult::get*Count() always return an int on acknowle…
GromNaN Sep 25, 2024
6908c08
Propagation of type changes to GridFS
GromNaN Sep 25, 2024
49fa315
Format stubs
GromNaN Sep 25, 2024
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 psalm.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<file name="stubs/BSON/PackedArray.stub.php"/>
<file name="stubs/Driver/Cursor.stub.php"/>
<file name="stubs/Driver/CursorInterface.stub.php"/>
<file name="stubs/Driver/WriteResult.stub.php"/>
</stubs>

<issueHandlers>
Expand Down
65 changes: 19 additions & 46 deletions src/BulkWriteResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@

namespace MongoDB;

use MongoDB\Driver\Exception\LogicException;
use MongoDB\Driver\WriteResult;
use MongoDB\Exception\BadMethodCallException;

/**
* Result class for a bulk write operation.
*/
class BulkWriteResult
{
private bool $isAcknowledged;

public function __construct(private WriteResult $writeResult, private array $insertedIds)
{
$this->isAcknowledged = $writeResult->isAcknowledged();
}

/**
Expand All @@ -38,15 +35,11 @@ public function __construct(private WriteResult $writeResult, private array $ins
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getDeletedCount(): ?int
public function getDeletedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getDeletedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getDeletedCount();
}

/**
Expand All @@ -55,15 +48,11 @@ public function getDeletedCount(): ?int
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getInsertedCount(): ?int
public function getInsertedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getInsertedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getInsertedCount();
}

/**
Expand All @@ -86,15 +75,11 @@ public function getInsertedIds(): array
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getMatchedCount(): ?int
public function getMatchedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getMatchedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getMatchedCount();
}

/**
Expand All @@ -106,15 +91,11 @@ public function getMatchedCount(): ?int
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getModifiedCount(): ?int
public function getModifiedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getModifiedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getModifiedCount();
}

/**
Expand All @@ -123,15 +104,11 @@ public function getModifiedCount(): ?int
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getUpsertedCount(): ?int
public function getUpsertedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getUpsertedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getUpsertedCount();
}

/**
Expand All @@ -145,15 +122,11 @@ public function getUpsertedCount(): ?int
* This method should only be called if the write was acknowledged.
*
* @see BulkWriteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getUpsertedIds(): array
{
if ($this->isAcknowledged) {
return $this->writeResult->getUpsertedIds();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getUpsertedIds();
}

/**
Expand All @@ -164,6 +137,6 @@ public function getUpsertedIds(): array
*/
public function isAcknowledged(): bool
{
return $this->isAcknowledged;
return $this->writeResult->isAcknowledged();
}
}
17 changes: 5 additions & 12 deletions src/DeleteResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@

namespace MongoDB;

use MongoDB\Driver\Exception\LogicException;
use MongoDB\Driver\WriteResult;
use MongoDB\Exception\BadMethodCallException;

/**
* Result class for a delete operation.
*/
class DeleteResult
{
private bool $isAcknowledged;

public function __construct(private WriteResult $writeResult)
{
$this->isAcknowledged = $writeResult->isAcknowledged();
}

/**
Expand All @@ -38,15 +35,11 @@ public function __construct(private WriteResult $writeResult)
* This method should only be called if the write was acknowledged.
*
* @see DeleteResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getDeletedCount(): ?int
public function getDeletedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getDeletedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getDeletedCount();
}

/**
Expand All @@ -57,6 +50,6 @@ public function getDeletedCount(): ?int
*/
public function isAcknowledged(): bool
{
return $this->isAcknowledged;
return $this->writeResult->isAcknowledged();
}
}
11 changes: 0 additions & 11 deletions src/Exception/BadMethodCallException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,4 @@ public static function classIsImmutable(string $class): self
{
return new self(sprintf('%s is immutable', $class));
}

/**
* Thrown when accessing a result field on an unacknowledged write result.
*
* @param string $method Method name
* @internal
*/
public static function unacknowledgedWriteResultAccess(string $method): self
{
return new self(sprintf('%s should not be called for an unacknowledged write result', $method));
}
}
13 changes: 3 additions & 10 deletions src/GridFS/Bucket.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,16 +583,9 @@ public function rename(mixed $id, string $newFilename): void
return;
}

/* 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->getMatchedCount() !== null
? $updateResult->getMatchedCount() === 1
: $this->collectionWrapper->findFileById($id) !== null;

if (! $found) {
// 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) {
Comment on lines +586 to +588
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 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) {

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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).

throw FileNotFoundException::byId($id, $this->getFilesNamespace());
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/GridFS/CollectionWrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function deleteChunksByFilesId(mixed $id): void
/**
* Delete all GridFS files and chunks for a given filename.
*/
public function deleteFileAndChunksByFilename(string $filename): ?int
public function deleteFileAndChunksByFilename(string $filename): int
{
/** @var iterable<array{_id: mixed}> $files */
$files = $this->findFiles(['filename' => $filename], [
Expand Down Expand Up @@ -262,7 +262,7 @@ public function insertFile(array|object $file): void
/**
* Updates the filename field in the file document for all the files with a given filename.
*/
public function updateFilenameForFilename(string $filename, string $newFilename): ?int
public function updateFilenameForFilename(string $filename, string $newFilename): int
{
return $this->filesCollection->updateMany(
['filename' => $filename],
Expand Down
15 changes: 4 additions & 11 deletions src/InsertManyResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@

namespace MongoDB;

use MongoDB\Driver\Exception\LogicException;
use MongoDB\Driver\WriteResult;
use MongoDB\Exception\BadMethodCallException;

/**
* Result class for a multi-document insert operation.
*/
class InsertManyResult
{
private bool $isAcknowledged;

public function __construct(private WriteResult $writeResult, private array $insertedIds)
{
$this->isAcknowledged = $writeResult->isAcknowledged();
}

/**
Expand All @@ -38,15 +35,11 @@ public function __construct(private WriteResult $writeResult, private array $ins
* This method should only be called if the write was acknowledged.
*
* @see InsertManyResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getInsertedCount(): ?int
public function getInsertedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getInsertedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getInsertedCount();
}

/**
Expand Down
15 changes: 4 additions & 11 deletions src/InsertOneResult.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@

namespace MongoDB;

use MongoDB\Driver\Exception\LogicException;
use MongoDB\Driver\WriteResult;
use MongoDB\Exception\BadMethodCallException;

/**
* Result class for a single-document insert operation.
*/
class InsertOneResult
{
private bool $isAcknowledged;

public function __construct(private WriteResult $writeResult, private mixed $insertedId)
{
$this->isAcknowledged = $writeResult->isAcknowledged();
}

/**
Expand All @@ -38,15 +35,11 @@ public function __construct(private WriteResult $writeResult, private mixed $ins
* This method should only be called if the write was acknowledged.
*
* @see InsertOneResult::isAcknowledged()
* @throws BadMethodCallException if the write result is unacknowledged
* @throws LogicException if the write result is unacknowledged
*/
public function getInsertedCount(): ?int
public function getInsertedCount(): int
{
if ($this->isAcknowledged) {
return $this->writeResult->getInsertedCount();
}

throw BadMethodCallException::unacknowledgedWriteResultAccess(__METHOD__);
return $this->writeResult->getInsertedCount();
}

/**
Expand Down
Loading