Skip to content

Conversation

@shimonewman
Copy link
Contributor

@shimonewman shimonewman commented Aug 31, 2025

This commit introduces a new test case in MongoTest.php to validate the functionality of the count method in the database. The test covers various scenarios including total count, filtered counts, counts with limits, and aggregation counts, ensuring comprehensive coverage of the counting capabilities.

Summary by CodeRabbit

  • New Features
    • Counting now uses the database's native count command and supports filters, limit/skip, and maxTime options for more accurate results.
  • Bug Fixes
    • Count errors are handled gracefully and return 0 instead of causing failures.
  • Tests
    • Added comprehensive tests covering simple, filtered, ranged, limited, aggregated, grouped, and complex OR counting scenarios.

This commit introduces a new test case in MongoTest.php to validate the functionality of the count method in the database. The test covers various scenarios including total count, filtered counts, counts with limits, and aggregation counts, ensuring comprehensive coverage of the counting capabilities.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 31, 2025

Walkthrough

Replaced cursor-based counting with a native MongoDB count command (supports optional limit/skip/maxTimeMS), returning an integer and returning 0 on exceptions. Added tests inserting 30 documents and asserting various count and aggregation scenarios, then dropping the test collection.

Changes

Cohort / File(s) Summary
Core client behavior
src/Client.php
Replaced in-memory cursor counting with a native count command built from cleaned filters (toObject), applying optional limit, skip, and maxTimeMS; executes via query() and returns integer result; exceptions return 0.
Test suite additions
tests/MongoTest.php
Added testCountMethod() which inserts 30 documents into count_test, asserts counts for various filters, limits, $or conditions and aggregation pipelines ($count, $group), then drops the collection.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller as Caller
  participant Client as Client::count()
  participant Server as MongoDB Server

  Caller->>Client: count(collection, filters, opts)
  Client->>Client: cleanFilters -> toObject(filters)\nbuild count command { query, limit?, skip?, maxTimeMS? }
  alt success
    Client->>Server: query(count command)
    Server-->>Client: { n: <int> }
    Client-->>Caller: <int>
  else exception
    Client->>Client: catch Exception
    Client-->>Caller: 0
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title is concise, single-sentence, and accurately describes the primary change—adding a test for the count method in the MongoDB integration—as reflected by the PR description and the new test in tests/MongoTest.php; it is specific and free of noise.

Poem

I counted docs with twitching nose,
From cursors’ strolls to command that knows—
Thirty carrots lined in rows,
Filters nibble what each one shows.
If errors hop into the meadow,
I thump and return a cheerful zero. 🐇


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eebbd5b and 2138409.

📒 Files selected for processing (1)
  • tests/MongoTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/MongoTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Tests (8.3)
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-count

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Client.php (1)

1-1: Auto-format code to fix PSR-12 whitespace and brace violations
CI is failing on no_whitespace_in_blank_line and braces in src/Client.php and tests/MongoTest.php. Run the PSR-12 formatter and commit the fixes, for example:

# auto-format all files
vendor/bin/pint
# or via Composer
composer exec --verbose=false vendor/bin/pint
🧹 Nitpick comments (4)
src/Client.php (2)

711-711: Typo in comment: “instad” → “instead”

-        // Use MongoDB's native count command with the working format instad of running find and count the results
+        // Use MongoDB's native count command with the working format instead of running find and counting the results

718-730: Optional: pass through more supported count options

If needed, consider supporting hint and collation via explicit mapping to the count command.

tests/MongoTest.php (2)

295-301: Add tests for skip and skip+limit options

Since count() supports skip/limit, assert both individually and together.

         $total = $this->getDatabase()->count($collectionName, [], ['limit' => 5]);
         self::assertEquals(5, $total);
 
        // Test count with filter and limit (should be 3 for first 3 documents with number <= 10)
         $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['limit' => 3]);
         self::assertEquals(3, $total);
+
+        // Test count with skip (should be 5 remaining after skipping 25)
+        $total = $this->getDatabase()->count($collectionName, [], ['skip' => 25]);
+        self::assertEquals(5, $total);
+
+        // Test count with skip + limit (should be 10)
+        $total = $this->getDatabase()->count($collectionName, [], ['skip' => 5, 'limit' => 10]);
+        self::assertEquals(10, $total);

291-294: Optional: cover compound logical filters

Add a case for $and/$or to ensure count() handles cleanFilters() changes correctly.

Example:

$total = $this->getDatabase()->count($collectionName, [
    '$and' => [
        ['number' => ['$gte' => 5]],
        ['number' => ['$lte' => 10]],
    ],
], []);
self::assertEquals(6, $total);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ddfdd9d and 6d149c5.

📒 Files selected for processing (2)
  • src/Client.php (1 hunks)
  • tests/MongoTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/MongoTest.php (1)
src/Client.php (5)
  • createCollection (332-345)
  • insertMany (480-502)
  • count (708-738)
  • aggregate (749-756)
  • dropCollection (356-363)
src/Client.php (1)
src/Exception.php (1)
  • Exception (5-7)
🪛 GitHub Actions: Linter
tests/MongoTest.php

[error] 1-1: PSR-12 violation: braces, no_whitespace_in_blank_line in tests/MongoTest.php. Lint step 'vendor/bin/pint --test' failed.

src/Client.php

[error] 1-1: PSR-12 violation: braces, no_whitespace_in_blank_line in src/Client.php. Lint step 'vendor/bin/pint --test' failed.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Tests (8.3)
🔇 Additional comments (1)
tests/MongoTest.php (1)

265-334: Whitespace fixes applied via vendor/bin/pint; PSR-12 compliance restored and CI checks now pass.

Comment on lines 708 to 716
public function count(string $collection, array $filters, array $options): int
{
$result = $this->find($collection, $filters, $options);
$list = $result->cursor->firstBatch;

return \count($list);

// Use MongoDB's native count command with the working format instad of running find and count the results
$command = [
self::COMMAND_COUNT => $collection,
'query' => $this->toObject($filters),
];

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 31, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Align count() with find(): add defaults and cleanFilters()

Make filters/options optional and normalize filters like find() to correctly handle $and/$or/$nor. This prevents malformed queries and keeps API parity.

-    public function count(string $collection, array $filters, array $options): int
+    public function count(string $collection, array $filters = [], array $options = []): int
     {
-    
+        $filters = $this->cleanFilters($filters);
-        // Use MongoDB's native count command with the working format instad of running find and count the results
+        // Use MongoDB's native count command with the working format instead of running find and counting the results
         $command = [
             self::COMMAND_COUNT => $collection,
-            'query' => $this->toObject($filters),
+            'query' => $this->toObject($filters),
         ];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public function count(string $collection, array $filters, array $options): int
{
$result = $this->find($collection, $filters, $options);
$list = $result->cursor->firstBatch;
return \count($list);
// Use MongoDB's native count command with the working format instad of running find and count the results
$command = [
self::COMMAND_COUNT => $collection,
'query' => $this->toObject($filters),
];
public function count(string $collection, array $filters = [], array $options = []): int
{
$filters = $this->cleanFilters($filters);
// Use MongoDB's native count command with the working format instead of running find and counting the results
$command = [
self::COMMAND_COUNT => $collection,
'query' => $this->toObject($filters),
];
🤖 Prompt for AI Agents
In src/Client.php around lines 708 to 716, the count() method currently requires
filters and options and builds the command directly; update it to mirror find()
by making the $filters and $options parameters optional (default to empty
arrays), run the same filter normalization (call the existing
cleanFilters($filters) or equivalent to normalize $and/$or/$nor and other
structures) before converting to an object, and apply the same default options
used by find() (merge $options with defaults) so the MongoDB count command is
built from normalized filters and options to avoid malformed queries and
preserve API parity.

Copy link
Member

Choose a reason for hiding this comment

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

@shimonewman Do we need the cleanFilters since we don't use find anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to apply cleanFilters to the filters to be used by count, same as in find()
Added it back

Comment on lines 732 to 737
try {
$result = $this->query($command);
return (int)$result;
} catch (Exception $e) {
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Don’t swallow exceptions in count(); let them surface or gate by an option

Returning 0 on any error hides real issues and is inconsistent with other methods that throw. Prefer bubbling the exception, or make suppression opt-in (e.g., options['suppressExceptions']).

-        try {
-        $result = $this->query($command);
-          return (int)$result;
-        } catch (Exception $e) {
-            return 0;
-        }
+        if (!empty($options['suppressExceptions'])) {
+            try {
+                return (int) $this->query($command);
+            } catch (Exception $e) {
+                return 0;
+            }
+        }
+        return (int) $this->query($command);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
$result = $this->query($command);
return (int)$result;
} catch (Exception $e) {
return 0;
}
if (!empty($options['suppressExceptions'])) {
try {
return (int) $this->query($command);
} catch (Exception $e) {
return 0;
}
}
return (int) $this->query($command);
🤖 Prompt for AI Agents
In src/Client.php around lines 732 to 737, the count() implementation currently
catches all Exceptions and returns 0 which swallows real errors; change it to
either let exceptions bubble (remove the try/catch) so callers receive the
thrown Exception, or implement an options flag (e.g.,
$this->options['suppressExceptions'] or a method parameter) that, when true,
returns 0 on error but otherwise rethrows the caught Exception; ensure any added
flag default preserves current behavior only if intentionally desired and update
the method signature/docblock accordingly.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
tests/MongoTest.php (2)

301-329: Assert batch size before indexing aggregation results

Prevents undefined offset errors and improves failure messages.

         // Test aggregation count - total documents
         $aggregationResult = $this->getDatabase()->aggregate($collectionName, [
             ['$count' => 'total']
         ]);
+        self::assertCount(1, $aggregationResult->cursor->firstBatch);
         self::assertEquals(30, $aggregationResult->cursor->firstBatch[0]->total);
 
         // Test aggregation count with filter
         $filteredAggregationResult = $this->getDatabase()->aggregate($collectionName, [
             ['$match' => ['number' => ['$lte' => 10]]],
             ['$count' => 'total']
         ]);
+        self::assertCount(1, $filteredAggregationResult->cursor->firstBatch);
         self::assertEquals(10, $filteredAggregationResult->cursor->firstBatch[0]->total);
 
         // Test aggregation count with limit
         $limitedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
             ['$limit' => 7],
             ['$count' => 'total']
         ]);
+        self::assertCount(1, $limitedAggregationResult->cursor->firstBatch);
         self::assertEquals(7, $limitedAggregationResult->cursor->firstBatch[0]->total);
 
         // Test aggregation count with group by
         $groupedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
             ['$group' => [
                 '_id' => '$category', // Group by category
                 'count' => ['$sum' => 1] // Count of documents in the group
             ]]
         ]);
+        self::assertCount(1, $groupedAggregationResult->cursor->firstBatch);
         self::assertEquals(30, $groupedAggregationResult->cursor->firstBatch[0]->count);
         self::assertEquals('test', $groupedAggregationResult->cursor->firstBatch[0]->_id);

267-333: Always clean up the collection via try/finally

If any assertion fails, the collection is left behind and may break subsequent runs with "Collection Exists".

     $collectionName = 'count_test';
     $this->getDatabase()->createCollection($collectionName);
-
-    $documents = [];
-    for ($i = 1; $i <= 30; $i++) {
-        $documents[] = [
-            'name' => "Document {$i}",
-            'number' => $i,
-            'category' => 'test',
-            'created_at' => new \DateTime()
-        ];
-    }
-
-    $this->getDatabase()->insertMany($collectionName, $documents);
-
-    $total = $this->getDatabase()->count($collectionName, [], []);
-    self::assertEquals(30, $total);
-
-    // Test count with filter (should be 1 for each specific number)
-    $total = $this->getDatabase()->count($collectionName, ['number' => 15], []);
-    self::assertEquals(1, $total);
-
-    // Test count with range filter (should be 10 for numbers 1-10)
-    $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], []);
-    self::assertEquals(10, $total);
-
-    // Test count with limit (should be 5 for first 5 documents)
-    $total = $this->getDatabase()->count($collectionName, [], ['limit' => 5]);
-    self::assertEquals(5, $total);
-
-    // Test count with filter and limit (should be 3 for first 3 documents with number <= 10)
-    $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['limit' => 3]);
-    self::assertEquals(3, $total);
-
-    // Test aggregation count - total documents
-    $aggregationResult = $this->getDatabase()->aggregate($collectionName, [
-        ['$count' => 'total']
-    ]);
-    self::assertEquals(30, $aggregationResult->cursor->firstBatch[0]->total);
-
-    // Test aggregation count with filter
-    $filteredAggregationResult = $this->getDatabase()->aggregate($collectionName, [
-        ['$match' => ['number' => ['$lte' => 10]]],
-        ['$count' => 'total']
-    ]);
-    self::assertEquals(10, $filteredAggregationResult->cursor->firstBatch[0]->total);
-
-    // Test aggregation count with limit
-    $limitedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
-        ['$limit' => 7],
-        ['$count' => 'total']
-    ]);
-    self::assertEquals(7, $limitedAggregationResult->cursor->firstBatch[0]->total);
-
-    // Test aggregation count with group by
-    $groupedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
-        ['$group' => [
-            '_id' => '$category', // Group by category
-            'count' => ['$sum' => 1] // Count of documents in the group
-        ]]
-    ]);
-    self::assertEquals(30, $groupedAggregationResult->cursor->firstBatch[0]->count);
-    self::assertEquals('test', $groupedAggregationResult->cursor->firstBatch[0]->_id);
-
-    $this->getDatabase()->dropCollection($collectionName);
+    try {
+        $documents = [];
+        for ($i = 1; $i <= 30; $i++) {
+            $documents[] = [
+                'name' => "Document {$i}",
+                'number' => $i,
+                'category' => 'test',
+                'created_at' => new \DateTime()
+            ];
+        }
+
+        $this->getDatabase()->insertMany($collectionName, $documents);
+
+        $total = $this->getDatabase()->count($collectionName, [], []);
+        self::assertEquals(30, $total);
+
+        // Test count with filter (should be 1 for each specific number)
+        $total = $this->getDatabase()->count($collectionName, ['number' => 15], []);
+        self::assertEquals(1, $total);
+
+        // Test count with range filter (should be 10 for numbers 1-10)
+        $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], []);
+        self::assertEquals(10, $total);
+
+        // Test count with limit (should be 5 for first 5 documents)
+        $total = $this->getDatabase()->count($collectionName, [], ['limit' => 5]);
+        self::assertEquals(5, $total);
+
+        // Test count with filter and limit (should be 3 for first 3 documents with number <= 10)
+        $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['limit' => 3]);
+        self::assertEquals(3, $total);
+
+        // Test aggregation count - total documents
+        $aggregationResult = $this->getDatabase()->aggregate($collectionName, [
+            ['$count' => 'total']
+        ]);
+        self::assertEquals(30, $aggregationResult->cursor->firstBatch[0]->total);
+
+        // Test aggregation count with filter
+        $filteredAggregationResult = $this->getDatabase()->aggregate($collectionName, [
+            ['$match' => ['number' => ['$lte' => 10]]],
+            ['$count' => 'total']
+        ]);
+        self::assertEquals(10, $filteredAggregationResult->cursor->firstBatch[0]->total);
+
+        // Test aggregation count with limit
+        $limitedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
+            ['$limit' => 7],
+            ['$count' => 'total']
+        ]);
+        self::assertEquals(7, $limitedAggregationResult->cursor->firstBatch[0]->total);
+
+        // Test aggregation count with group by
+        $groupedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
+            ['$group' => [
+                '_id' => '$category',
+                'count' => ['$sum' => 1]
+            ]]
+        ]);
+        self::assertEquals(30, $groupedAggregationResult->cursor->firstBatch[0]->count);
+        self::assertEquals('test', $groupedAggregationResult->cursor->firstBatch[0]->_id);
+    } finally {
+        $this->getDatabase()->dropCollection($collectionName);
+    }
🧹 Nitpick comments (3)
tests/MongoTest.php (3)

267-268: Avoid “Collection Exists” by making the test collection name unique

Prevents cross-run/test collisions and allows parallelism.

-        $collectionName = 'count_test';
+        $collectionName = 'count_test_' . uniqid();

297-300: Also exercise the skip path in count()

Covers options supported by the new count command implementation.

         // Test count with filter and limit (should be 3 for first 3 documents with number <= 10)
         $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['limit' => 3]);
         self::assertEquals(3, $total);
+
+        // Test count with filter and skip (numbers <= 10, skip 5 -> 5)
+        $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['skip' => 5]);
+        self::assertEquals(5, $total);
+
+        // Test count with skip + limit
+        $total = $this->getDatabase()->count($collectionName, [], ['skip' => 10, 'limit' => 5]);
+        self::assertEquals(5, $total);

282-299: Prefer strict assertions for integers

Use assertSame for type-safe equality on counts.

-        self::assertEquals(30, $total);
+        self::assertSame(30, $total);
@@
-        self::assertEquals(1, $total);
+        self::assertSame(1, $total);
@@
-        self::assertEquals(10, $total);
+        self::assertSame(10, $total);
@@
-        self::assertEquals(5, $total);
+        self::assertSame(5, $total);
@@
-        self::assertEquals(3, $total);
+        self::assertSame(3, $total);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d149c5 and f2b48fa.

📒 Files selected for processing (2)
  • src/Client.php (1 hunks)
  • tests/MongoTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Client.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/MongoTest.php (1)
src/Client.php (5)
  • createCollection (332-345)
  • insertMany (480-502)
  • count (708-737)
  • aggregate (748-755)
  • dropCollection (356-363)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Tests (8.3)

This commit updates the count method in the Client class to include a call to cleanFilters for improved filter handling. Additionally, it adds new test cases in MongoTest.php to validate counting with the $or operator and complex conditions, ensuring robust testing of the counting functionality.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/Client.php (2)

708-709: Align signature defaults with find(): make $filters and $options optional.

Apply this diff (backward compatible with current calls):

-    public function count(string $collection, array $filters, array $options): int
+    public function count(string $collection, array $filters = [], array $options = []): int

733-738: Don’t swallow exceptions; bubble by default (or gate via an option).

One-line change to bubble:

-        try {
-            $result = $this->query($command);
-            return (int)$result;
-        } catch (Exception $e) {
-            return 0;
-        }
+        return (int) $this->query($command);

Alternatively, make suppression opt-in:

-        try {
-            $result = $this->query($command);
-            return (int)$result;
-        } catch (Exception $e) {
-            return 0;
-        }
+        if (!empty($options['suppressExceptions'])) {
+            try {
+                return (int) $this->query($command);
+            } catch (Exception $e) {
+                return 0;
+            }
+        }
+        return (int) $this->query($command);
tests/MongoTest.php (1)

265-352: Guarantee cleanup with try/finally around the test body.

If any assertion fails, the collection isn’t dropped and pollutes later tests.

Apply this diff:

     public function testCountMethod()
     {
         $collectionName = 'count_test';
-        $this->getDatabase()->createCollection($collectionName);
-
-        $documents = [];
-        for ($i = 1; $i <= 30; $i++) {
-            $documents[] = [
-                'name' => "Document {$i}",
-                'number' => $i,
-                'category' => 'test',
-                'created_at' => new \DateTime()
-            ];
-        }
-
-        $this->getDatabase()->insertMany($collectionName, $documents);
-
-        $total = $this->getDatabase()->count($collectionName, [], []);
-        self::assertEquals(30, $total);
+        $this->getDatabase()->createCollection($collectionName);
+        try {
+            $documents = [];
+            for ($i = 1; $i <= 30; $i++) {
+                $documents[] = [
+                    'name' => "Document {$i}",
+                    'number' => $i,
+                    'category' => 'test',
+                    'created_at' => new \DateTime()
+                ];
+            }
+            $this->getDatabase()->insertMany($collectionName, $documents);
+            $total = $this->getDatabase()->count($collectionName, [], []);
+            self::assertEquals(30, $total);
@@
-        $this->getDatabase()->dropCollection($collectionName);
+        } finally {
+            $this->getDatabase()->dropCollection($collectionName);
+        }
     }
🧹 Nitpick comments (6)
src/Client.php (3)

712-712: Fix typo in comment (“instad” → “instead”).

Apply this diff:

-        // Use MongoDB's native count command with the working format instad of running find and count the results
+        // Use MongoDB's native count command with the working format instead of running find and counting the results

718-731: Validate option bounds and whitelist keys (limit/skip/maxTimeMS).

Negative limit/skip are invalid; also consider allowing hint/collation/comment when present, while rejecting unknown keys to avoid server errors.

Apply this diff:

-        // Add limit if specified
-        if (isset($options['limit'])) {
-            $command['limit'] = (int)$options['limit'];
-        }
-
-        // Add skip if specified
-        if (isset($options['skip'])) {
-            $command['skip'] = (int)$options['skip'];
-        }
-
-        // Add maxTimeMS if specified
-        if (isset($options['maxTimeMS'])) {
-            $command['maxTimeMS'] = (int)$options['maxTimeMS'];
-        }
+        // Whitelist supported options
+        $allowed = ['limit', 'skip', 'maxTimeMS', 'hint', 'collation', 'comment'];
+        foreach ($allowed as $key) {
+            if (!array_key_exists($key, $options)) continue;
+            $value = $options[$key];
+            if ($key === 'limit' || $key === 'skip' || $key === 'maxTimeMS') {
+                $value = (int) $value;
+                if ($value < 0) {
+                    throw new Exception("Option '{$key}' must be >= 0");
+                }
+            }
+            $command[$key] = $value;
+        }

705-707: Docblock and behavior are inconsistent regarding exceptions.

The method is annotated with @throws Exception but currently catches and returns 0 on error. Update the behavior (bubble) or the docblock to reflect suppression semantics.

tests/MongoTest.php (3)

302-305: Fix misleading inline comment (should be 4, not 2).

Numbers 1, 2, 29, 30 satisfy the predicate.

Apply this diff:

-        // Test count with $or operator and comparison (should be 2 documents with number <= 2 OR number >= 29)
+        // Test count with $or operator and comparison (should be 4 documents: number <= 2 OR number >= 29)

326-334: Assert the number of groups to harden the test.

Ensure only one group (‘test’) is returned.

Apply this diff:

         $groupedAggregationResult = $this->getDatabase()->aggregate($collectionName, [
             ['$group' => [
                 '_id' => '$category', // Group by category
                 'count' => ['$sum' => 1] // Count of documents in the group
             ]]
         ]);
+        self::assertCount(1, $groupedAggregationResult->cursor->firstBatch);
         self::assertEquals(30, $groupedAggregationResult->cursor->firstBatch[0]->count);
         self::assertEquals('test', $groupedAggregationResult->cursor->firstBatch[0]->_id);

293-299: Add a skip-only count case to exercise ‘skip’ handling.

Currently limit is covered; skip isn’t.

Apply this diff:

         // Test count with filter and limit (should be 3 for first 3 documents with number <= 10)
         $total = $this->getDatabase()->count($collectionName, ['number' => ['$lte' => 10]], ['limit' => 3]);
         self::assertEquals(3, $total);
 
+        // Test count with skip (should be 5 for last 5 documents after skipping 25)
+        $total = $this->getDatabase()->count($collectionName, [], ['skip' => 25]);
+        self::assertEquals(5, $total);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2b48fa and eebbd5b.

📒 Files selected for processing (2)
  • src/Client.php (1 hunks)
  • tests/MongoTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Client.php (1)
src/Exception.php (1)
  • Exception (5-7)
tests/MongoTest.php (1)
src/Client.php (5)
  • createCollection (332-345)
  • insertMany (480-502)
  • count (708-739)
  • aggregate (750-757)
  • dropCollection (356-363)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Tests (8.3)
🔇 Additional comments (2)
src/Client.php (1)

710-716: Good call: normalize filters before building the count command.

Using cleanFilters() + toObject() keeps parity with find() and prevents malformed $and/$or/$nor structures.

tests/MongoTest.php (1)

282-349: Overall: solid coverage of count and aggregation paths.

Good spread: unfiltered, filtered, range, limit, $or, and multiple aggregation patterns.

This commit modifies the MongoTest class to wrap the collection drop operation in a try-finally block, ensuring that the collection is always dropped after the test execution, even if an exception occurs during the test. This enhances the reliability of the test environment.
@abnegate abnegate merged commit 589e329 into main Sep 11, 2025
4 checks passed
@abnegate abnegate deleted the refactor-count branch September 11, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants