DRIVERS-3009, DRIVERS-1447: Updates to find operations #1691

alcaeus · 2024-10-29T08:23:59Z

Please complete the following before merging:

Update changelog.
Make sure there are generated JSON files from the YAML test files.
Test changes in at least one language driver (PHP)
Test these changes against all server versions and topologies (including standalone, replica set, sharded
clusters, and serverless).

This PR introduces changes to the CRUD spec, covering multiple tickets:

DRIVERS-3009: Add a formal definition for Collection::findOne, especially in order to specify the limit behaviour and define available options
DRIVERS-1447: Clarify how to handle limit and batchSize options when they are set to the same value to avoid an unnecessary cursor being created on the server.

alcaeus · 2024-10-29T08:25:24Z

source/crud/crud.md

@@ -702,6 +714,194 @@ class FindOptions {
   */
  let: Optional<Document>;
 }
+
+class FindOneOptions {


This duplicates a significant amount of options from FindOptions. There are two alternatives:

Have FindOptions extend FindOneOptions and add options not supported by findOne

Introduce CommonFindOptions, have FindOptions and FindOneOptions extend that class and add their own.

If we want to avoid duplication, I'd prefer option one.

I don't think this matters, but I know some languages actually base their own model classes after these definitions within the spec. Maybe better to ask them, although I suppose that only matters if they actually intend on implementing findOne() (IIRC, C# does not).

I hate that we define all of these in giant code blocks. Makes it impossible to deep link or add meaningful prose text. Logically, we wouldn't need to document anything here apart from saying it's the same as FindOptions with limit and batchSize removed (and maybe some other options specific to multiple results).

Having said all that, I agree with:

If we want to avoid duplication, I'd prefer option one.

Otherwise, CommonFindOptions and FindOneOptions would just be identical anyway.

I pray that no users of a driver ever have to see a FindOptions model extending FindOneOptions. That should be an implementation detail.

I really dislike the duplication here. Here's what pymongo's docs say for find_one:

All arguments to :meth:find are also valid arguments for :meth:find_one, although any limit argument will be ignored.

What if we say something similar here instead of duplicating?

Typescript has syntax for this that we use in our driver:

For example:

type FindOneOptions = Omit<FindOptions, 'timeoutMode'>

Not suggesting we use that language specific syntax here, but I think it would be fine to psuedo declare FindOneOptions are equal to FindOptions except for X Y Z

@alcaeus What is your thinking here? Seems like we have a preference for not duplicating the options, but as for exactly how to write it, there are a couple of choices here.

After thinking about a good solution, I figured that solving this for find/findOne is only half the story, as there's lots of other duplication (e.g. UpdateOptions and ReplaceOptions). I filed DRIVERS-3038 to solve the duplication issue and also remove the code blocks: they are unnecessarily prescriptive and prevent us from properly linking to various options in the document. As such, I'd defer making changes to this for the time being.

Something for next spec-fest perhaps :) fine by me

Can't we just go with one of the proposed simple solutions for now? We can fix the find/findOne duplication in this PR and then use DRIVERS-3038 to fix the remaining issues.

Changed to the Omit syntax suggested by @nbbeeken.

alcaeus · 2024-10-29T08:26:31Z

source/crud/crud.md

@@ -2474,6 +2677,8 @@ aforementioned allowance in the SemVer spec.

 ## Changelog

+- 2024-10-29: Defined `findOne` operation as optional.


"as optional" is tentative here pending an outcome of a poll across drivers. I'm considering making the operation required, but letting drivers choose their timeline for implementing this operation, as findOne is a common use case.

alcaeus · 2024-10-29T08:27:26Z

source/crud/tests/unified/findOne.yml

@@ -0,0 +1,60 @@
+description: "findOne"


These tests heavily borrow from the find tests. I've decided to only explicitly check for limit and the absence of batchSize in the outgoing command. I don't think there are any other special cases we need to test.

alcaeus · 2024-10-29T13:26:40Z

source/crud/crud.md

+When users specify both `limit` and `batchSize` options with the same value, the server returns all results in the
+first batch, but still leaves an open cursor that needs to be closed using the `killCursors` command. To avoid this,
+drivers MUST send a value of `limit + 1` for `batchSize` in the resulting `find` command. This eliminates the open
+cursor issue.


Unlike suggested in the ticket, we can't just omit the batchSize option when limit <= batchSize. Omitting the batch size makes us fall back to the default batch size, which may be less than the limit requested by the user, defeating the expectation that all results are returned in a single batch. The only workaround for this is to set a batch size higher than limit to get the server to close the cursor immediately before returning results.

Ah, this is what you mentioned in Slack about limit + 1. I missed that it only applied to find and that you weren't discussing findOne (which can omit batchSize entirely).

jmikola · 2024-10-29T15:50:58Z

source/crud/crud.md

+   * as it would be impossible to be forwards and backwards compatible. Let the server
+   * handle the validation.
+   *
+   * @see https://www.mongodb.com/docs/manual/core/read-operations-introduction/


FYI, this redirects to https://www.mongodb.com/docs/manual/tutorial/query-documents/

Changed the link to this one - I believe this works fine as an introduction to querying

source/crud/crud.md

jmikola · 2024-10-29T15:55:58Z

source/crud/crud.md

@@ -702,6 +714,194 @@ class FindOptions {
   */
  let: Optional<Document>;
 }
+
+class FindOneOptions {


I don't think this matters, but I know some languages actually base their own model classes after these definitions within the spec. Maybe better to ask them, although I suppose that only matters if they actually intend on implementing findOne() (IIRC, C# does not).

I hate that we define all of these in giant code blocks. Makes it impossible to deep link or add meaningful prose text. Logically, we wouldn't need to document anything here apart from saying it's the same as FindOptions with limit and batchSize removed (and maybe some other options specific to multiple results).

Having said all that, I agree with:

If we want to avoid duplication, I'd prefer option one.

Otherwise, CommonFindOptions and FindOneOptions would just be identical anyway.

I pray that no users of a driver ever have to see a FindOptions model extending FindOneOptions. That should be an implementation detail.

jmikola · 2024-10-29T15:58:26Z

source/crud/crud.md

+When users specify both `limit` and `batchSize` options with the same value, the server returns all results in the
+first batch, but still leaves an open cursor that needs to be closed using the `killCursors` command. To avoid this,
+drivers MUST send a value of `limit + 1` for `batchSize` in the resulting `find` command. This eliminates the open
+cursor issue.


Ah, this is what you mentioned in Slack about limit + 1. I missed that it only applied to find and that you weren't discussing findOne (which can omit batchSize entirely).

source/crud/crud.md

source/crud/tests/unified/find.yml

source/crud/tests/unified/findOne.yml

source/crud/crud.md

nbbeeken · 2024-10-30T14:15:41Z

source/crud/crud.md

@@ -702,6 +714,194 @@ class FindOptions {
   */
  let: Optional<Document>;
 }
+
+class FindOneOptions {


Typescript has syntax for this that we use in our driver:

For example:

type FindOneOptions = Omit<FindOptions, 'timeoutMode'>

Not suggesting we use that language specific syntax here, but I think it would be fine to psuedo declare FindOneOptions are equal to FindOptions except for X Y Z

source/crud/crud.md

nbbeeken · 2024-11-05T14:42:19Z

source/crud/crud.md

@@ -702,6 +714,194 @@ class FindOptions {
   */
  let: Optional<Document>;
 }
+
+class FindOneOptions {


@alcaeus What is your thinking here? Seems like we have a preference for not duplicating the options, but as for exactly how to write it, there are a couple of choices here.

source/crud/crud.md

jmikola · 2024-11-11T14:13:28Z

source/crud/crud.md

@@ -2496,6 +2513,8 @@ aforementioned allowance in the SemVer spec.

 ## Changelog

+- 2024-11-05: Define `findOne` operation as optional.


You can also note that you're adding guidance on limit and batchSize for find operations.

jmikola

LGTM with @see and changelog updates.

Co-authored-by: Jeremy Mikola <[email protected]>

alcaeus requested a review from jmikola October 29, 2024 08:23

alcaeus requested a review from a team as a code owner October 29, 2024 08:24

alcaeus commented Oct 29, 2024

View reviewed changes

alcaeus force-pushed the drivers-3009-findone-updates branch 3 times, most recently from b25faeb to 8594353 Compare October 29, 2024 13:22

alcaeus changed the title ~~DRIVERS-3009: Define findOne operation~~ DRIVERS-3009, DRIVERS-1447: Updates to find operations Oct 29, 2024

alcaeus commented Oct 29, 2024

View reviewed changes

jmikola reviewed Oct 29, 2024

View reviewed changes

nbbeeken requested changes Oct 30, 2024

View reviewed changes

alcaeus force-pushed the drivers-3009-findone-updates branch from 041345a to b4c8e3d Compare November 5, 2024 10:22

alcaeus requested review from nbbeeken and jmikola November 5, 2024 12:42

nbbeeken requested changes Nov 5, 2024

View reviewed changes

alcaeus requested a review from nbbeeken November 6, 2024 07:41

nbbeeken approved these changes Nov 6, 2024

View reviewed changes

nbbeeken requested a review from ShaneHarvey November 6, 2024 15:21

alcaeus force-pushed the drivers-3009-findone-updates branch from b4c8e3d to 734e40a Compare November 7, 2024 12:30

jmikola reviewed Nov 11, 2024

View reviewed changes

ShaneHarvey approved these changes Nov 11, 2024

View reviewed changes

jmikola approved these changes Nov 12, 2024

View reviewed changes

alcaeus added 9 commits November 13, 2024 13:01

DRIVERS-3009: Define findOne operation

0a81dc9

DRIVERS-1447: Specify batchSize behavior when limit is set

e51dd1a

Update link for query filters

80792af

Remove obsolete note about query operators

91fff2d

Add comment to explain computed batchSize value

80e71ac

Add expectEvents for findOne test

0ebef94

Fix linter issue

4461132

Make findOne result optional

cc49a0e

Require singleBatch to be set for findOne

f59c18a

alcaeus added 2 commits November 13, 2024 13:01

Remove duplication in FindOneOptions

b5f3768

Fix changelog entry

8780c53

alcaeus force-pushed the drivers-3009-findone-updates branch from 734e40a to 8780c53 Compare November 13, 2024 12:01

Apply suggestions from code review

a6e9ab0

Co-authored-by: Jeremy Mikola <[email protected]>

alcaeus merged commit 2362d1a into mongodb:master Nov 13, 2024
5 checks passed

alcaeus deleted the drivers-3009-findone-updates branch November 13, 2024 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRIVERS-3009, DRIVERS-1447: Updates to find operations #1691

DRIVERS-3009, DRIVERS-1447: Updates to find operations #1691

alcaeus commented Oct 29, 2024 •

edited

Loading

alcaeus Oct 29, 2024

jmikola Oct 29, 2024

ShaneHarvey Oct 29, 2024 •

edited

Loading

nbbeeken Oct 30, 2024

nbbeeken Nov 5, 2024

alcaeus Nov 6, 2024

nbbeeken Nov 6, 2024

ShaneHarvey Nov 6, 2024

alcaeus Nov 7, 2024

alcaeus Oct 29, 2024

alcaeus Oct 29, 2024

alcaeus Oct 29, 2024

jmikola Oct 29, 2024

jmikola Oct 29, 2024

alcaeus Oct 30, 2024

jmikola Oct 29, 2024

jmikola Oct 29, 2024

nbbeeken Oct 30, 2024

nbbeeken Nov 5, 2024

jmikola Nov 11, 2024

jmikola left a comment

		@@ -2474,6 +2677,8 @@ aforementioned allowance in the SemVer spec.

		## Changelog

		- 2024-10-29: Defined `findOne` operation as optional.

		@@ -2496,6 +2513,8 @@ aforementioned allowance in the SemVer spec.

		## Changelog

		- 2024-11-05: Define `findOne` operation as optional.

DRIVERS-3009, DRIVERS-1447: Updates to find operations #1691

DRIVERS-3009, DRIVERS-1447: Updates to find operations #1691

Conversation

alcaeus commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

alcaeus commented Oct 29, 2024 •

edited

Loading

ShaneHarvey Oct 29, 2024 •

edited

Loading