Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions src/Operation/Find.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use MongoDB\Exception\UnsupportedException;
use MongoDB\Model\CodecCursor;

use function assert;
use function is_array;
use function is_bool;
use function is_integer;
Expand Down Expand Up @@ -418,6 +419,16 @@ private function createQueryOptions(): array
$options['modifiers'] = is_object($modifiers) ? document_to_array($modifiers) : $modifiers;
}

// Ensure no cursor is left behind when limit == batchSize by increasing batchSize
if (isset($options['limit'], $options['batchSize']) && $options['limit'] === $options['batchSize']) {
assert(is_integer($options['batchSize']));
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary for static analysis? The constructor should already enforce that batchSize is an integer if set.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, this is for static analysis only.

$options['batchSize']++;
}

if (isset($options['limit']) && $options['limit'] === 1) {
$options['singleBatch'] = true;
Copy link
Member

Choose a reason for hiding this comment

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

I took the step of automatically setting singleBatch when limit is set to 1, even though this can affect find operations where users themself set limit to 1. Let me know if you see any potential issues with that approach, the alternative would be to introduce singleBatch as an undocumented option to Find.

@alcaeus: Is that any different than what already happens in PHPC? Read on.


The CRUD spec's Q&A says this about why singleBatch was omitted from FindOptions in the API:

Rather than introduce a singleBatch option to FindOptions, the spec preserves the existing API for limit and instructs drivers to convert negative values accordingly for servers >= 3.2.

That said, the option exists in PHPC. PHPC also supports a negative limit, which implies singleBatch: true. That logic is implemented in php_phongo_query_init_limit_and_singlebatch.

I think there are two possible actions to take here:

  • Acknowledge the CRUD spec and leave support for negative limits in place. If so, we should update the docs.
  • Deprecate negative limits in 1.x and remove it in 2.0 (to be on par with libmongoc's API). In that case we should introduce singleBatch as an explicit option in PHPLIB.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I believe given that we no longer support 3.2 and older, we may want to consider introducing singleBatch as an option for Find. Reading the spec, it never says that a driver may not provide singleBatch as an option, so we could certainly add it for PHP. We can always decide to leave it undocumented until the spec is updated. I can file a drivers ticket for adding singleBatch.

That said, singleBatch has scenarios where not all expected results are returned (so neither limit nor batchSize is reached). This is not relevant for findOne, but I feel like that should be documented in the spec.

}

return $options;
}
}
2 changes: 1 addition & 1 deletion tests/specifications
Submodule specifications updated 65 files
+4 −4 source/connection-monitoring-and-pooling/tests/cmap-format/pool-create-min-size-error.json
+2 −2 source/connection-monitoring-and-pooling/tests/cmap-format/pool-create-min-size-error.yml
+32 −13 source/crud/crud.md
+148 −0 source/crud/tests/unified/bulkWrite-updateMany-pipeline.json
+67 −0 source/crud/tests/unified/bulkWrite-updateMany-pipeline.yml
+156 −0 source/crud/tests/unified/bulkWrite-updateOne-pipeline.json
+66 −0 source/crud/tests/unified/bulkWrite-updateOne-pipeline.yml
+62 −0 source/crud/tests/unified/find.json
+28 −0 source/crud/tests/unified/find.yml
+158 −0 source/crud/tests/unified/findOne.json
+75 −0 source/crud/tests/unified/findOne.yml
+130 −0 source/crud/tests/unified/findOneAndUpdate-pipeline.json
+56 −0 source/crud/tests/unified/findOneAndUpdate-pipeline.yml
+142 −0 source/crud/tests/unified/updateMany-pipeline.json
+64 −0 source/crud/tests/unified/updateMany-pipeline.yml
+150 −0 source/crud/tests/unified/updateOne-pipeline.json
+64 −0 source/crud/tests/unified/updateOne-pipeline.yml
+0 −494 source/crud/tests/unified/updateWithPipelines.json
+0 −296 source/crud/tests/unified/updateWithPipelines.yml
+1 −1 source/logging/logging.md
+5 −20 source/read-write-concern/read-write-concern.md
+5 −36 source/retryable-writes/tests/README.md
+144 −0 source/retryable-writes/tests/unified/aggregate-out-merge.json
+65 −0 source/retryable-writes/tests/unified/aggregate-out-merge.yml
+153 −1 source/retryable-writes/tests/unified/bulkWrite.json
+64 −0 source/retryable-writes/tests/unified/bulkWrite.yml
+12 −3 source/retryable-writes/tests/unified/client-bulkWrite-serverErrors.json
+3 −0 source/retryable-writes/tests/unified/client-bulkWrite-serverErrors.yml
+21 −1 source/retryable-writes/tests/unified/deleteMany.json
+8 −0 source/retryable-writes/tests/unified/deleteMany.yml
+31 −1 source/retryable-writes/tests/unified/deleteOne.json
+12 −0 source/retryable-writes/tests/unified/deleteOne.yml
+31 −1 source/retryable-writes/tests/unified/findOneAndDelete.json
+12 −0 source/retryable-writes/tests/unified/findOneAndDelete.yml
+31 −1 source/retryable-writes/tests/unified/findOneAndReplace.json
+12 −0 source/retryable-writes/tests/unified/findOneAndReplace.yml
+31 −1 source/retryable-writes/tests/unified/findOneAndUpdate.json
+12 −0 source/retryable-writes/tests/unified/findOneAndUpdate.yml
+58 −1 source/retryable-writes/tests/unified/insertMany.json
+23 −0 source/retryable-writes/tests/unified/insertMany.yml
+31 −1 source/retryable-writes/tests/unified/insertOne.json
+12 −0 source/retryable-writes/tests/unified/insertOne.yml
+31 −1 source/retryable-writes/tests/unified/replaceOne.json
+12 −0 source/retryable-writes/tests/unified/replaceOne.yml
+77 −0 source/retryable-writes/tests/unified/unacknowledged-write-concern.json
+40 −0 source/retryable-writes/tests/unified/unacknowledged-write-concern.yml
+21 −1 source/retryable-writes/tests/unified/updateMany.json
+8 −0 source/retryable-writes/tests/unified/updateMany.yml
+31 −1 source/retryable-writes/tests/unified/updateOne.json
+12 −0 source/retryable-writes/tests/unified/updateOne.yml
+0 −12 source/server-discovery-and-monitoring/server-discovery-and-monitoring-tests.md
+9 −7 source/server-discovery-and-monitoring/server-discovery-and-monitoring.md
+4 −0 source/server-discovery-and-monitoring/tests/unified/logging-replicaset.json
+4 −4 source/server-discovery-and-monitoring/tests/unified/logging-replicaset.yml
+2 −0 source/server-discovery-and-monitoring/tests/unified/logging-sharded.json
+2 −2 source/server-discovery-and-monitoring/tests/unified/logging-sharded.yml
+2 −0 source/server-discovery-and-monitoring/tests/unified/logging-standalone.json
+2 −2 source/server-discovery-and-monitoring/tests/unified/logging-standalone.yml
+149 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-application-error.json
+88 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-application-error.yml
+296 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-checkout-error.json
+176 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-checkout-error.yml
+230 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-min-pool-size-error.json
+144 −0 source/server-discovery-and-monitoring/tests/unified/pool-clear-min-pool-size-error.yml
+4 −1 source/server_write_commands/server_write_commands.md