Improve type of Collection Bulk Write operations#1694
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // instance, there is no need to re-validate the returned value here. | ||
| if ($codec) { | ||
| $operations[$i][$type][0] = $codec->encode($args[0]); | ||
| $operation[$type][0] = $codec->encode($args[0]); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument Note
| case self::DELETE_MANY: | ||
| case self::DELETE_ONE: | ||
| $operations[$i][$type][0] = $builderEncoder->encodeIfSupported($args[0]); | ||
| $operation[$type][0] = $builderEncoder->encodeIfSupported($args[0]); |
Check notice
Code scanning / Psalm
MixedAssignment Note
|
|
||
| case self::REPLACE_ONE: | ||
| $operations[$i][$type][0] = $builderEncoder->encodeIfSupported($args[0]); | ||
| $operation[$type][0] = $builderEncoder->encodeIfSupported($args[0]); |
Check notice
Code scanning / Psalm
MixedAssignment Note
|
|
||
| if ($codec) { | ||
| $operations[$i][$type][1] = $codec->encode($args[1]); | ||
| $operation[$type][1] = $codec->encode($args[1]); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument Note
| case self::UPDATE_MANY: | ||
| case self::UPDATE_ONE: | ||
| $operations[$i][$type][0] = $builderEncoder->encodeIfSupported($args[0]); | ||
| $operation[$type][0] = $builderEncoder->encodeIfSupported($args[0]); |
Check notice
Code scanning / Psalm
MixedAssignment Note
| } | ||
|
|
||
| $operations[$i][$type][1] = $args[1] = $builderEncoder->encodeIfSupported($args[1]); | ||
| $operation[$type][1] = $args[1] = $builderEncoder->encodeIfSupported($args[1]); |
Check notice
Code scanning / Psalm
MixedAssignment Note
| } | ||
|
|
||
| $operations[$i][$type][1] = $args[1] = $builderEncoder->encodeIfSupported($args[1]); | ||
| $operation[$type][1] = $args[1] = $builderEncoder->encodeIfSupported($args[1]); |
Check notice
Code scanning / Psalm
MixedAssignment Note
| * @see \MongoDB\Collection::bulkWrite() | ||
| * | ||
| * @psalm-type Document = object|array | ||
| * @psalm-type OperationType = array{deleteMany: array{0: Document, 1?: array}}|array{deleteOne: array{0: Document, 1?: array}}|array{insertOne: array{0: Document}}|array{replaceOne: array{0: Document, 1: Document, 2?: array}}|array{updateMany: array{0: Document, 1: Document, 2?: array}}|array{updateOne: array{0: Document, 1: Document, 2?: array}} |
There was a problem hiding this comment.
Per our conversation yesterday, I think we still need confirmation on whether this can be utilized by the IDE. If not, is this worth adding at all?
There was a problem hiding this comment.
Looking at the baseline, it definitely improves the psalm analysis.
There was a problem hiding this comment.
PHPStorm has supports autocompletion of array keys thanks to the generic @param annotation on Collection::bulkWrite. The psalm annotation is here to improve static analysis.
There was a problem hiding this comment.
Pull Request Overview
This PR improves type safety for MongoDB Collection bulk write operations by introducing a custom Psalm type definition for the complex array structure required by the bulkWrite() method. The change adds static analysis support to help catch type-related issues at development time.
- Introduces a detailed
OperationTypecustom type that describes all supported bulk write operation formats - Updates method signatures to use the new type annotations for better static analysis
- Refactors the validation loop to use references instead of array indexing for improved performance
- Adds a test case for empty operation validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Operation/BulkWrite.php | Defines the OperationType custom type and updates method signatures and validation logic |
| src/Collection.php | Imports and uses the OperationType in the bulkWrite method signature |
| tests/Operation/BulkWriteTest.php | Adds test coverage for empty operation validation |
| psalm-baseline.xml | Updates baseline to reflect improved type analysis with fewer mixed type issues |
Comments suppressed due to low confidence (1)
src/Operation/BulkWrite.php:1
- The
Documenttype is defined twice (lines 49 and 51). Remove the duplicate definition to avoid confusion and maintain clean code.
<?php
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
FWIW, I think this is a good intermediate step until we provide a better solution (e.g. by allowing to pass a |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #1694 +/- ##
============================================
+ Coverage 87.80% 87.91% +0.11%
+ Complexity 3209 3208 -1
============================================
Files 428 428
Lines 6402 6390 -12
============================================
- Hits 5621 5618 -3
+ Misses 781 772 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* v2.2: (138 commits) Improve flakey index version test (#1844) Require ext-mongodb 2.2 (#1840) Bump tests/specifications from `9e2acc6` to `bb9dddd` (#1842) Bump tests/drivers-evergreen-tools from `c7ec372` to `bd10517` (#1843) Improve type of Collection Bulk Write operations (#1694) Fix atlas search exception support detection for MongoDB 5 shared cluster (#1836) Skip CRUD prose test 13 with ext-mongodb 2.2+ (#1838) PHPLIB-1647: Add Database::getGridFSBucket (#1835) Bump tests/specifications from `08d7132` to `9e2acc6` (#1830) PHPLIB-1698: Remove reference to test-atlas-data-lake task (#1834) PHPLIB-1770 The unknown command name is not quoted in 5.0.32-enterprise (#1829) Bump tests/specifications from `28fdaf9` to `08d7132` (#1828) Bump tests/drivers-evergreen-tools from `5514d6a` to `c7ec372` (#1827) PHPLIB-1752 Upgrade to psalm 6.14 (#1814) Support encrypted colls in FunctionalTestCase::createCollection (#1820) Bump tests/drivers-evergreen-tools from `61cb4e9` to `5514d6a` (#1823) Bump tests/specifications from `192976b` to `28fdaf9` (#1824) PHPLIB-1736: Add `$scoreFusion` stage (#1822) Bump tests/drivers-evergreen-tools from `514927f` to `61cb4e9` (#1818) PHPLIB-1689 Make the Atlas Search exception more specific (#1794) ...
The
Collection::bulkWrite($operations)method requires a complex array structure for the operations list. Describing this type using psalm custom type feature to helps static analysis.PHPStorm is able to make the completion only with the full
@paramannotation, not@phpstan-paramnor@psaml-param. And only when I start typing the first letter.Both Psalm and PHPStan detect when the argument is invalid. The PHPStorm inspection does not detect issues when the array does not match the expected
@paramtype.