PHPLIB-810: Always use count command for estimatedDocumentCount

[aleksandr-rudo]

https://jira.mongodb.org/browse/PHPLIB-810

[aleksandr-rudo]

This PR already includes PHPLIB-749 (#925) merged inside.

[jmikola]

@aleksandr-rudo: I know we previously discussed the overlapping spec tests (e.g. comment tests assume estimatedDocumentCount always uses a count command), but I don't understand why #925 was merged into this PR. Can you revert those changes to reduce this PR to only the estimatedDocumentCount changes?

The two relevant commits from DRIVERS-2228 are:

• mongodb/specifications@c430376 (original)
• mongodb/specifications@021cbc8 (follow-up, linked in the downstream changes)

I don't want to waste your time adding skips for various comment tests in the CRUD spec, so when you sync spec tests, so I'd be OK with ignoring all of the other comment-related test files in the unified CRUD directory apart from estimatedDocumentCount-comment.json (since it's actually in the commit). I'd propose the following:

• Check out mongodb/specifications@021cbc8 in your local clone of the specifications repository
• Sync all spec tests in the legacy retryable reads directory
• Sync all spec tests in the unified versioned API directory
• Sync only the estimatedDocumentCount tests in the unified CRUD directory
• Add skips for the tests in estimatedDocumentCount-comment.json, and reference PHPLIB-749 in the skip message
• Add skips for any tests pertaining to view-related createCollection options, and reference PHPLIB-869 in the skip message

We can then wait on #925 to sync the remaining CRUD spec tests.

My reasoning for this suggestion is that PHPLIB-810 will be complete with this single PR (noting the future tickets needed to address skipped tests); however, PHPLIB-749 will likely sit in review until I can wrap up PHPC-2049. I'd rather we merge on PR while it's possible to do so.

---

On a separate note, I should point out that DRIVERS-2228 also has several documentation requirements for the estimatedDocumentCount method. See: mongodb/specifications@c430376#diff-2fe314a3da48fca10d3a170b09a5a393a93dbb4e9369d6741c99656a95dd7034R757. Specifically:

Drivers MUST document that, due to an oversight in versions 5.0.0-5.0.7 of MongoDB, the count command, which estimatedDocumentCount uses in its implementation, was not included in v1 of the Stable API, and so users of the Stable API with estimatedDocumentCount are recommended to upgrade their server version to 5.0.8+ or set apiStrict: false to avoid encountering errors.

Drivers MUST document that the count server command is used to implement estimatedDocumentCount and that users can find more information via Count: Behavior.

I originally missed this in my first PR for CDRIVER-4309 and had to re-open the ticket to ensure I came back to it. If you'd like to attempt to made those documentation changes in MongoDBCollection-estimatedDocumentCount.txt, feel free; otherwise, please add a documentation-needed label to PHPLIB-810 and I'll come back to this at a later point before the 1.13.0 release.

I think we can add the required information under the the existing Behavior section of the manual page.

[jmikola]

With this change, I believe createAggregate is no longer called and should be removed.

[aleksandr-rudo]

@jmikola The idea was to keep relevant changes in the corresponding branches and PRs. I've got two tickets:

• PHPLIB-749 - consists of
  • request to add "comment" option support for the following classes: EstimatedDocumentCount, CountDocuments (as the specifications tests update contains new tests for this operation), Count (as the operating class for EstimatedDocumentCount calls) and Aggregate (as the operating class for EstimatedDocumentCount calls)
  • request to update specifications tests (as it is said in the ticket description, sync with mongodb/specifications@938a402)
• PHPLIB-810 - consists of
  • request to change only estimatedDocumentCount logic to use only Count as the operating class (without Aggregate).
  • request to update specifications tests (as it is said in the ticket description, sync with mongodb/specifications@021cbc8), which contains retryable-reads tests update, estimatedDocumentsCount test update (for correct using Count without Aggregate), but doesn't contain new CRUD tests like countDocuments-comment.json, estimatedDocumentCount-comment.json, which exists in the specifications update from the previous ticket.

It seemed to me that it is a better way to avoid any mistake to store all changes corresponding to tickets requests in theirs branches and merge them in one PR to gain all changes in one place.

[jmikola]

It seemed to me that it is a better way to avoid any mistake to store all changes corresponding to tickets requests in theirs branches and merge them in one PR to gain all changes in one place.

I'm not sure what storing the changes in separate branches would accomplish.

The actual code changes for PHPLIB-810 should be much smaller than those for PHPLIB-749, and thus it should be much easier to review and merge its PR. I think that should only require adding skips for tests in estimatedDocumentCount-comment.json if you follow my instructions in #926 (comment).

DRIVERS-2228 included two commits, and the first (mongodb/specifications@c430376) did include changes to estimatedDocumentCount-comment.json. That is why I suggested copying both estimatedDocumentCount.json and estimatedDocumentCount-comment.json and skipping remaining CRUD files (if you'd rather not add skips for all of the other comment tests). I'm not worried about us missing those other files when PHPLIB-749 is implemented.

Regarding mistakes, I think those are more likely when the diffs themselves include changes from multiple PRs, as is currently the case. To further elaborate, looks at the current list of commits in this PR:

• Implementing new option 'comment' for the following helper methods: "…
• Merge branch 'mongodb:master' into phplib-749
• Filtering out failing tests.
• Making EstimatedDocumentCount helper works over Count helper.
• Code style.
• Failing tests fixes.
• Merge branch 'phplib-749' into phplib-810

Contrast that with what I did in #908. An earlier iteration of that PR did have commits from #909 in it, since there was a dependency. After #909 was merged, I rebased #908 on the master branch. At any given time, all commits associated with a specific ticket included its issue number in the commit message.

At it applies to these two tickets, I'd expect to see PHPLIB-810 implemented independently and then, if needed, have your PR for PHPLIB-749 rebased on top of this branch. If that doesn't sound manageable (given how comfortable you are with rebasing), let's just focus on getting this PR in a complete state (with just the count changes) so it can be merged to master and then you can proceed with PHPLIB-749 as if it had no dependency on an unmerged PR.

[jmikola]

@aleksandr-rudo: I didn't find this in any commit messages, so please leave a comment on the PR with the appropriate sync commit. For example:

Synced with mongodb/specifications@...

I'd like to review this locally and confirm that the test files and sync commit are accurate before merging, and also ensure we have the correct line to include in the squashed commit message.

[aleksandr-rudo]

Branch description:

• Making EstimatedDocumentCount works over operating class Count only (without using Aggregate)
• Spec tests and UnifiedSpec tests synced with mongodb/specifications@021cbc8
• Some tests are excluded as tests for new upcoming options

[jmikola]

Note: earlier today I merged documentation changes for the corresponding libmongoc ticket (mongodb/mongo-c-driver@22a8696 for CDRIVER-4309). Feel free to adapt that text for documentation in the estimatedDocumentCount() API reference; otherwise, I can do so myself after this gets merged.

[jmikola]

Feel free to adapt that text for documentation in the estimatedDocumentCount() API reference; otherwise, I can do so myself after this gets merged.

Done in 8f2e921.