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}
| @@ -306,14 +310,14 @@ | |||
| // $args[0] was already validated above. Since DocumentCodec::encode will always return a Document | |||
| // 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
| } | ||
|
|
||
| break; | ||
|
|
||
| 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
|
|
||
| break; | ||
|
|
||
| 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 (! isset($args[1]) && ! array_key_exists(1, $args)) { | ||
| throw new InvalidArgumentException(sprintf('Missing second argument for $operations[%d]["%s"]', $i, $type)); | ||
| } | ||
|
|
||
| if ($codec) { | ||
| $operations[$i][$type][1] = $codec->encode($args[1]); | ||
| $operation[$type][1] = $codec->encode($args[1]); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument Note
|
|
||
| break; | ||
|
|
||
| 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
|
|
||
| if (! isset($args[1]) && ! array_key_exists(1, $args)) { | ||
| throw new InvalidArgumentException(sprintf('Missing second argument for $operations[%d]["%s"]', $i, $type)); | ||
| } | ||
|
|
||
| $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
|
|
||
| if (! isset($args[1]) && ! array_key_exists(1, $args)) { | ||
| throw new InvalidArgumentException(sprintf('Missing second argument for $operations[%d]["%s"]', $i, $type)); | ||
| } | ||
|
|
||
| $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
| @@ -45,6 +45,9 @@ | |||
| * Operation for executing multiple write operations. | |||
| * | |||
| * @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.
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.
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.