Skip to content

feat: add upsertWithCounts() with separate matched/upserted counts #37

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

Open
premtsd-code wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add upsertWithCounts() with separate matched/upserted counts #37

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
fcb886f
feat: add upsertWithCounts() returning matched/modified/upserted sepa…
premtsd-code Apr 13, 2026
25d0a8c
Address review feedback on upsertWithCounts
premtsd-code Apr 13, 2026
b373e95
Address review: validate response in upsertWithCounts, harden empty-b…
premtsd-code Apr 13, 2026
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
128 changes: 121 additions & 7 deletions src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,14 @@ public function createUuid(): string
*
* @param array<string, mixed> $command Command to execute
* @param string|null $db Database name
* @param bool $returnRaw When true, skip the int/extracted-field short-circuits
* in the response parser and return the full stdClass
* result — use when the caller needs fields beyond `n`
* (e.g. the `upserted` array from an update command).
* @return stdClass|array|int Query result
* @throws Exception
*/
public function query(array $command, ?string $db = null): stdClass|array|int
public function query(array $command, ?string $db = null, bool $returnRaw = false): stdClass|array|int
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In PHP 8.x, is a child method signature compatible with a parent if the parent adds a new optional parameter but the child override keeps the previous shorter signature?

💡 Result:

No, a child method signature is not compatible with a parent if the parent adds a new optional parameter but the child override keeps the previous shorter signature. This violates method signature compatibility rules in PHP 8.x, as the child method has fewer parameters than the parent, making it incompatible under Liskov Substitution Principle (LSP) checks. In PHP 8.0+, such incompatibilities always trigger a fatal error during inheritance validation, rather than just a warning as in earlier versions.

Citations:

🏁 Script executed:

# Check the current state of src/Client.php around lines 253 and 378
cat -n src/Client.php | sed -n '250,260p'

Repository: utopia-php/mongo

Length of output: 518

🏁 Script executed:

# Also check around line 378 for the send() method
cat -n src/Client.php | sed -n '375,385p'

Repository: utopia-php/mongo

Length of output: 480

🏁 Script executed:

# Check git history to see what changed in these methods
git diff HEAD~1 src/Client.php 2>/dev/null || echo "No git history available; checking file directly"

Repository: utopia-php/mongo

Length of output: 108

🏁 Script executed:

# Search for classes that extend Client
rg 'extends\s+Client|class\s+\w+.*extends' src/ tests/ --type php

Repository: utopia-php/mongo

Length of output: 482

🏁 Script executed:

# Check if there's any documentation about extending Client
rg -i 'extend|subclass|override' README.md LICENSE.md || find . -maxdepth 2 -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l -i 'extend\|subclass' {} \;

Repository: utopia-php/mongo

Length of output: 110

🏁 Script executed:

# Check the class definition and its context
cat -n src/Client.php | head -30

Repository: utopia-php/mongo

Length of output: 865

Preserve override compatibility for query() and send().

Widening public, non-final method signatures can break downstream subclasses. If a child class overrides the old signature without the new optional parameters, PHP 8.0+ enforces strict compatibility and will raise a fatal LSP error. Since Client is non-final and part of your public API surface, consider keeping these signatures stable or marking the class as final if extension is not intended. Alternatively, move the raw-response toggle behind a new helper method instead.

Also applies to: 378-378

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Client.php` at line 253, The change widened the public method signatures
for query() (and send()) which can break subclasses; revert the signature of
Client::query() to its original parameter list, restore Client::send() to its
previous signature, and implement the raw-response toggle by adding a new public
method (e.g., queryRaw() or setReturnRaw(bool) plus a protected helper like
doQueryRaw()) or mark the Client class as final if you intend to forbid
subclassing; update callers to use the new helper method rather than adding
optional params to the existing public methods so backwards-compatible
overriding remains intact.

{
// Validate connection state before each operation
$this->validateConnection();
Expand Down Expand Up @@ -351,7 +355,7 @@ public function query(array $command, ?string $db = null): stdClass|array|int

$sections = Document::fromPHP($params);
$message = pack('V*', 21 + strlen($sections), $this->id, 0, 2013, 0) . "\0" . $sections;
$result = $this->send($message);
$result = $this->send($message, $returnRaw);

$this->updateCausalConsistency($result);

Expand All @@ -371,7 +375,7 @@ public function query(array $command, ?string $db = null): stdClass|array|int
* @return stdClass|array|int
* @throws Exception
*/
public function send(mixed $data): stdClass|array|int
public function send(mixed $data, bool $returnRaw = false): stdClass|array|int
{
// Check if connection is alive, connect if not
if (!$this->client->isConnected()) {
Expand All @@ -390,14 +394,14 @@ public function send(mixed $data): stdClass|array|int
}
}

return $this->receive();
return $this->receive($returnRaw);
}

/**
* Receive a message from connection.
* @throws Exception
*/
private function receive(): stdClass|array|int
private function receive(bool $returnRaw = false): stdClass|array|int
{
$chunks = [];
$receivedLength = 0;
Expand Down Expand Up @@ -461,7 +465,7 @@ private function receive(): stdClass|array|int

$res = \implode('', $chunks);

return $this->parseResponse($res, $responseLength);
return $this->parseResponse($res, $responseLength, $returnRaw);
}

/**
Expand Down Expand Up @@ -899,6 +903,110 @@ public function upsert(string $collection, array $operations, array $options = [
);
}

/**
* Execute a batch of upserts and return detailed counts from the response.
*
* Unlike {@see upsert()}, which returns the raw `n` field (matched + upserted),
* this method separates matched-existing from newly-upserted documents so the
* caller can tell exactly which operations produced new docs. Useful for
* `skipDuplicates`-style callers that need to report "actually inserted" counts.
*
* Counts are only meaningful for acknowledged writes. In practice mongod
* still returns `n` for `writeConcern: { w: 0 }` over OP_MSG, so this method
* normally works regardless — but if a future protocol change ever causes
* `n` to be omitted, the method raises an Exception rather than silently
* returning zeros for writes that may have applied.
*
* @param string $collection
* @param array<int, array{filter: array<string, mixed>, update: array<string, mixed>, multi?: bool}> $operations
* @param array<string, mixed> $options
*
* @return array{
* matched: int,
* modified: int,
* upserted: array<int, array{index: int, _id: mixed}>
* } Response counts. `matched` is the number of pre-existing docs matched
* by a filter (derived as `n - count(upserted)` since MongoDB's `n`
* includes upserts). `modified` is docs whose contents changed. `upserted`
* is the list of newly-created docs, each carrying the source operation's
* `index` and the assigned `_id`.
*
* @throws Exception When the response is missing the `n` field — typically
* the result of an unacknowledged write concern.
*/
public function upsertWithCounts(string $collection, array $operations, array $options = []): array
{
// MongoDB rejects an `update` command with an empty `updates` array, so
// short-circuit dynamically-built batches before they hit the wire.
if (empty($operations)) {
return [
'matched' => 0,
'modified' => 0,
'upserted' => [],
];
}

$updates = [];

foreach ($operations as $op) {
$updates[] = [
'q' => $op['filter'],
'u' => $this->toObject($op['update']),
'upsert' => true,
'multi' => $op['multi'] ?? false,
];
}

$result = $this->query(
array_merge(
[
self::COMMAND_UPDATE => $collection,
'updates' => $updates,
],
$options
),
Comment on lines +960 to +967
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For MongoDB updatecommands, mustwriteConcernandreadConcernbe documents/objects rather than scalar values like1or"majority"?

💡 Result:

Yes, for MongoDB update commands (including db.collection.updateOne, updateMany, and the update database command), both writeConcern and readConcern must be specified as documents/objects rather than scalar values like 1 or "majority". Official MongoDB documentation explicitly states: - For writeConcern: "writeConcern: " (e.g., { w: "majority" }), as shown in the syntax for updateOne [6,7,17], update command [10], and write concern specification [2,3,18]. - For readConcern: "readConcern: { level: }" [11,15], and update operations support read concern only within transactions at the session/transaction level, not directly on the command. Scalar values like { writeConcern: "majority" } or { writeConcern: 1 } are invalid; the server expects a document with fields like w, j, wtimeout for writeConcern, leading to BSON type errors if a non-document is provided [1]. Drivers may offer convenience constructors (e.g., WriteConcern.Majority), but the wire protocol parameter is always a document.

Citations:

🏁 Script executed:

#!/bin/bash

# Check file size and read the entire Client.php file
wc -l src/Client.php

Repository: utopia-php/mongo

Length of output: 79

🏁 Script executed:

#!/bin/bash

# Search for the update() method to see how it handles options
rg -n "public function update\(" src/Client.php -A 50

Repository: utopia-php/mongo

Length of output: 2266

🏁 Script executed:

#!/bin/bash

# Search for helper methods mentioned in the review
rg -n "createWriteConcern|createReadConcern|shouldSkipReadConcern" src/Client.php

Repository: utopia-php/mongo

Length of output: 1996

🏁 Script executed:

#!/bin/bash

# Search for the upsertWithCounts() method
rg -n "public function upsertWithCounts\(" src/Client.php -A 80

Repository: utopia-php/mongo

Length of output: 3479

Normalize writeConcern/readConcern the same way update() does.

upsertWithCounts() bypasses option normalization by merging $options directly into the command. Callers passing scalar convenience forms—e.g. ['writeConcern' => 1] or ['readConcern' => 'majority']—will send invalid command shapes to MongoDB, whereas update() properly normalizes these via createWriteConcern() and createReadConcern(). Apply the same normalization pattern here before calling query(...).

Suggested fix 
-        $result = $this->query(
-            array_merge(
-                [
-                    self::COMMAND_UPDATE => $collection,
-                    'updates' => $updates,
-                ],
-                $options
-            ),
+        $command = [
+            self::COMMAND_UPDATE => $collection,
+            'updates' => $updates,
+        ];
+
+        if (isset($options['session'])) {
+            $command['session'] = $options['session'];
+        }
+
+        if (isset($options['writeConcern'])) {
+            $command['writeConcern'] = $this->createWriteConcern($options['writeConcern']);
+        }
+
+        if (isset($options['readConcern']) && !$this->shouldSkipReadConcern($options)) {
+            $command['readConcern'] = $this->createReadConcern($options['readConcern']);
+        }
+
+        $otherOptions = array_diff_key($options, array_flip(['session', 'writeConcern', 'readConcern']));
+
+        $result = $this->query(
+            array_merge($command, $otherOptions),
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Client.php` around lines 960 - 967, The upsertWithCounts() path is
merging $options directly into the command and thus can pass scalar convenience
forms for writeConcern/readConcern; adjust it to normalize these the same way
update() does by calling createWriteConcern($options) and
createReadConcern($options) (or the existing normalization helpers) and
replacing any scalar entries in $options with the normalized objects before
merging into the array passed to $this->query(...); locate upsertWithCounts(),
replicate the normalization logic from update() for writeConcern/readConcern,
then merge the normalized $options when building the command for query().

null,
true // $returnRaw — we need the full response, not just $result->n
);
Comment on lines +937 to +970
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Short-circuit empty upsert batches.

insertMany() already treats [] as a no-op, but this new method will send an update command with an empty updates array and turn that into a server error. Returning zero counts here makes dynamic batch builders safe.

Suggested fix 
 public function upsertWithCounts(string $collection, array $operations, array $options = []): array
 {
+    if ($operations === []) {
+        return [
+            'matched' => 0,
+            'modified' => 0,
+            'upserted' => [],
+        ];
+    }
+
     $updates = [];
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Client.php` around lines 930 - 953, The upsertWithCounts method currently
sends an update command even when $operations is an empty array, causing a
server error; modify upsertWithCounts to short-circuit when $operations is empty
by returning the same zero-count shaped array that callers expect (e.g. zero
inserted/updated/matched/modified/removed and ok flag) instead of calling
$this->query; place the check near the start of upsertWithCounts (before
building $updates) and reference $operations, $updates and the eventual $result
handling so callers that build dynamic batches are safe.


if (!$result instanceof stdClass) {
throw new Exception('Unexpected upsertWithCounts response: expected stdClass, got ' . \gettype($result));
}

// Defense-in-depth: the counts in this method are only meaningful for
// acknowledged writes. In practice mongod always replies with `n` over
// OP_MSG (even for writeConcern: { w: 0 }) because send() does not set
// the moreToCome flag, so this check is rarely tripped today — but if
// a future protocol change ever causes `n` to go missing, refuse loudly
// instead of silently returning zeros for writes that may have applied.
if (!\property_exists($result, 'n')) {
throw new Exception(
'upsertWithCounts() requires acknowledged writes — the response did not include `n`. '
. 'Do not pass writeConcern: { w: 0 } when calling this method.'
);
}

$upserted = [];
if (\property_exists($result, 'upserted') && \is_iterable($result->upserted)) {
foreach ($result->upserted as $entry) {
$upserted[] = [
'index' => (int) $entry->index,
'_id' => $entry->_id,
];
}
}

// MongoDB's `n` in the update response = matched-existing + upserted-new.
// Subtract the upserts so `matched` reflects only pre-existing docs.
$matched = \max(0, (int) $result->n - \count($upserted));

return [
'matched' => $matched,
'modified' => (int) ($result->nModified ?? 0),
'upserted' => $upserted,
];
}


/**
* Find document(s) with full transaction and session support.
Expand Down Expand Up @@ -1584,7 +1692,7 @@ public function close(): void
* @return stdClass|array|int Parsed response
* @throws Exception
*/
private function parseResponse(string $response, int $responseLength): stdClass|array|int
private function parseResponse(string $response, int $responseLength, bool $returnRaw = false): stdClass|array|int
{
/*
* The first 21 bytes of the MongoDB wire protocol response consist of:
Expand Down Expand Up @@ -1648,6 +1756,12 @@ private function parseResponse(string $response, int $responseLength): stdClass|
);
}

// Callers that need the full response object (e.g. to read the
// `upserted` array from an update command) opt in with $returnRaw.
if ($returnRaw && $result->ok === 1.0) {
return $result;
}

// Check for operation success
if (\property_exists($result, 'n') && $result->ok === 1.0) {
return $result->n;
Expand Down
183 changes: 183 additions & 0 deletions tests/MongoTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,189 @@ public function testUpsert()
self::assertEquals('English', $documents[1]->language);
}

public function testUpsertWithCountsMixed()
{
$collection = 'upsert_counts_mixed';
$this->getDatabase()->createCollection($collection);
try {
// Seed one document — it will be matched (no-op) by the first op.
$this->getDatabase()->insert($collection, [
'_id' => 'existing',
'name' => 'Original',
]);

$result = $this->getDatabase()->upsertWithCounts($collection, [
[
'filter' => ['_id' => 'existing'],
// $setOnInsert is a no-op for existing docs — leaves 'Original' intact.
'update' => ['$setOnInsert' => ['name' => 'ShouldNotWrite']],
],
[
'filter' => ['_id' => 'fresh'],
'update' => ['$setOnInsert' => ['name' => 'Fresh']],
],
]);

// One pre-existing match + one upsert = n:2, matched (derived):1, upserted:1.
self::assertIsArray($result);
self::assertArrayHasKey('matched', $result);
self::assertArrayHasKey('modified', $result);
self::assertArrayHasKey('upserted', $result);

self::assertSame(1, $result['matched'], 'matched should exclude upserts');
self::assertSame(0, $result['modified']);
self::assertCount(1, $result['upserted']);
self::assertSame(1, $result['upserted'][0]['index']);
self::assertSame('fresh', $result['upserted'][0]['_id']);

// Existing doc untouched, fresh doc created.
$existing = $this->getDatabase()->find($collection, ['_id' => 'existing'])->cursor->firstBatch ?? [];
self::assertCount(1, $existing);
self::assertEquals('Original', $existing[0]->name);

$fresh = $this->getDatabase()->find($collection, ['_id' => 'fresh'])->cursor->firstBatch ?? [];
self::assertCount(1, $fresh);
self::assertEquals('Fresh', $fresh[0]->name);
} finally {
$this->getDatabase()->dropCollection($collection);
}
}

public function testUpsertWithCountsAllNew()
{
$collection = 'upsert_counts_new';
$this->getDatabase()->createCollection($collection);
try {
$result = $this->getDatabase()->upsertWithCounts($collection, [
['filter' => ['_id' => 'a'], 'update' => ['$setOnInsert' => ['name' => 'A']]],
['filter' => ['_id' => 'b'], 'update' => ['$setOnInsert' => ['name' => 'B']]],
['filter' => ['_id' => 'c'], 'update' => ['$setOnInsert' => ['name' => 'C']]],
]);

self::assertSame(0, $result['matched'], 'No docs matched — all were upserts');
self::assertSame(0, $result['modified']);
self::assertCount(3, $result['upserted']);

$ids = \array_column($result['upserted'], '_id');
\sort($ids);
self::assertSame(['a', 'b', 'c'], $ids);

$indexes = \array_column($result['upserted'], 'index');
\sort($indexes);
self::assertSame([0, 1, 2], $indexes);
} finally {
$this->getDatabase()->dropCollection($collection);
}
}

public function testUpsertWithCountsModifiesExisting()
{
$collection = 'upsert_counts_modify';
$this->getDatabase()->createCollection($collection);
try {
$this->getDatabase()->insert($collection, [
'_id' => 'exists',
'name' => 'Old',
'counter' => 1,
]);

// Use $set (not $setOnInsert) so the matched doc is actually modified.
$result = $this->getDatabase()->upsertWithCounts($collection, [
[
'filter' => ['_id' => 'exists'],
'update' => [
'$set' => ['name' => 'New'],
'$inc' => ['counter' => 1],
],
],
]);

self::assertSame(1, $result['matched']);
self::assertSame(1, $result['modified'], 'modified should reflect nModified from MongoDB');
self::assertSame([], $result['upserted']);

// Verify the doc was actually updated on the server.
$docs = $this->getDatabase()->find($collection, ['_id' => 'exists'])->cursor->firstBatch ?? [];
self::assertCount(1, $docs);
self::assertEquals('New', $docs[0]->name);
self::assertEquals(2, $docs[0]->counter);
} finally {
$this->getDatabase()->dropCollection($collection);
}
}

public function testUpsertWithCountsEmpty()
{
// The guard should short-circuit before any wire traffic. Indirect proof:
// if the guard regressed, this would build an `update` command with an
// empty `updates` array, which MongoDB rejects with
// "Failed to parse: updates: array is empty"
// — that error would propagate as an Exception and fail this assertion.
// We also use a non-existent collection name to ensure the guard runs
// before any collection lookup happens.
$result = $this->getDatabase()->upsertWithCounts('this_collection_does_not_exist', []);

self::assertSame(['matched' => 0, 'modified' => 0, 'upserted' => []], $result);
}

public function testUpsertWithCountsRejectsEmptyUpdatesAtServer()
{
// Documents the underlying MongoDB behavior the empty-batch guard
// relies on. If a future MongoDB version stops rejecting empty
// `updates` arrays, the empty-batch test above becomes a weaker check
// and we should add a stronger guarantee.
$collection = 'upsert_counts_empty_proof';
$this->getDatabase()->createCollection($collection);
try {
$this->expectException(Exception::class);

// Bypass upsertWithCounts() entirely and send an `update` command
// with an empty `updates` array directly through query().
$this->getDatabase()->query([
'update' => $collection,
'updates' => [],
]);
} finally {
$this->getDatabase()->dropCollection($collection);
}
}

// Note: there is no test for the "missing `n` in response" defensive check.
// Triggering it would require either a moreToCome OP_MSG flag (not used by
// this client) or a stubbed transport — mongod still replies with `n` for
// writeConcern: { w: 0 } in normal use, so the check is defense-in-depth
// against future protocol changes rather than something callers can hit.

public function testUpsertWithCountsAllExisting()
{
$collection = 'upsert_counts_existing';
$this->getDatabase()->createCollection($collection);
try {
$this->getDatabase()->insertMany($collection, [
['_id' => 'a', 'name' => 'A'],
['_id' => 'b', 'name' => 'B'],
]);

$result = $this->getDatabase()->upsertWithCounts($collection, [
['filter' => ['_id' => 'a'], 'update' => ['$setOnInsert' => ['name' => 'NopeA']]],
['filter' => ['_id' => 'b'], 'update' => ['$setOnInsert' => ['name' => 'NopeB']]],
]);

self::assertSame(2, $result['matched']);
self::assertSame(0, $result['modified']);
self::assertSame([], $result['upserted']);

// Existing values preserved.
$docs = $this->getDatabase()->find($collection)->cursor->firstBatch ?? [];
self::assertCount(2, $docs);
foreach ($docs as $doc) {
self::assertContains($doc->name, ['A', 'B']);
}
} finally {
$this->getDatabase()->dropCollection($collection);
}
}

public function testToArrayWithNestedDocumentFromMongo()
{
$client = $this->getDatabase();
Expand Down
Loading