PHPLIB-1563 and PHPLIB-1564: batchSize and singleBatch fixes for find operations #1532
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
Bumps [tests/specifications](https://github.com/mongodb/specifications) from `a32d445` to `11022ca`. - [Release notes](https://github.com/mongodb/specifications/releases) - [Commits](mongodb/specifications@a32d445...11022ca) --- updated-dependencies: - dependency-name: tests/specifications dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
dependabot
bot
added
dependencies
alcaeus
force-pushed
the
dependabot/submodules/tests/specifications-11022ca
branch
from
dc0650e to
48376d0
Compare
alcaeus
force-pushed
the
dependabot/submodules/tests/specifications-11022ca
branch
from
48376d0 to
ff31e58
Compare
|
@jmikola This PR automatically fixes PHPLIB-1562 and PHPLIB-1591. I've included changes necessary for PHPLIB-1563 and PHPLIB-1564. Since our |
There was a problem hiding this comment.
Code changes LGTM but I'll wait for feedback.
I'm not a fan of adding actual commits for tickets in these dependabot PRs, as that makes the PR related to even more JIRA issues than it would have originally via just spec test updates. There's also a risk of issue references getting dropped at merge time (I've caught GitHub's UI doing that before when suggesting a single commit message).
| @@ -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'])); | |||
There was a problem hiding this comment.
Is this necessary for static analysis? The constructor should already enforce that batchSize is an integer if set.
There was a problem hiding this comment.
Yep, this is for static analysis only.
| } | ||
|
|
||
| if (isset($options['limit']) && $options['limit'] === 1) { | ||
| $options['singleBatch'] = true; |
There was a problem hiding this comment.
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
singleBatchas an explicit option in PHPLIB.
There was a problem hiding this comment.
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.
jmikola
deleted the
dependabot/submodules/tests/specifications-11022ca
branch
jmikola
changed the title
a32d445 to 11022ca
Note: this was originally a spec sync PR but was amended with fixes for the following tickets:
https://jira.mongodb.org/browse/PHPLIB-1563
https://jira.mongodb.org/browse/PHPLIB-1564
Bumps tests/specifications from
a32d445to11022ca.Commits
11022caDRIVERS-943 Convert retryable writes command construction tests to unified fo...ffe8b9eTypo fix in logging spec, max document length environment var (#1728)2362d1aDRIVERS-3009, DRIVERS-1447: Updates to find operations (#1691)d795d49DRIVERS-3033 SDAM logging tests should allow durationMS as a float (#1723)376b98aDRIVERS-1785: clarify ordering of CMAP events when pool is cleared (#1690)152257bDRIVERS-3040: Split updateWithPipelines test file by operation (#1727)ccb9816DRIVERS-2091: remove references to getLastError (#1698)Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)