Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(NODE-5019): add runCursorCommand API #3655

Merged
merged 20 commits into from
May 24, 2023
Merged

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Apr 25, 2023

Description

What is changing?

  • Adds a new Db API: await db.runCursorCommand(command, options)
  • A new RunCommandCursor subclass of AbstractCursor
    • The subclass overrides methods that do not apply to this kind of cursor throwing errors
    • Adds getMore configurations as required by the spec
    • overrides getMore execution in order to pass through the configurations correctly
Is there new documentation needed for these changes?

At least API docs for the new command, possibly new docs site page.

What is the motivation for this change?

DRIVERS-2533: mongodb/specifications#1412

Release Highlight

runCursorCommand API

We have added the Db#runCursorCommand method which can be used to execute generic cursor commands. This API complements the generic Db#command method.

const cursor = db.runCursorCommand({ checkMetadataConsistency: 1 });
for await (const metadata of cursor) {
  console.log(metadata);
}

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • [/] New TODOs have a related JIRA ticket

@nbbeeken nbbeeken force-pushed the NODE-5019-runCommandCursor branch from fc57254 to 514f8b3 Compare May 16, 2023 20:13
@nbbeeken nbbeeken changed the title [WIP, DNM] feat(NODE-5019): add runCursorCommand API feat(NODE-5019): add runCursorCommand API May 17, 2023
@nbbeeken nbbeeken removed the wip label May 17, 2023
@nbbeeken nbbeeken marked this pull request as ready for review May 17, 2023 15:40
@dariakp dariakp self-assigned this May 18, 2023
@dariakp dariakp self-requested a review May 18, 2023 18:38
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 18, 2023
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Partial feedback, need to circle back to finish review

test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
test/spec/run-command/runCommand.json Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from dariakp May 22, 2023 14:38
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
etc/sync-wip-spec.sh Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
test/integration/run-command/run_command.spec.test.ts Outdated Show resolved Hide resolved
test/integration/run-command/run_cursor_command.test.ts Outdated Show resolved Hide resolved
test/unit/cursor/run_command_cursor.test.ts Outdated Show resolved Hide resolved
test/unit/cursor/run_command_cursor.test.ts Outdated Show resolved Hide resolved
test/unit/cursor/run_command_cursor.test.ts Outdated Show resolved Hide resolved
src/cursor/run_command_cursor.ts Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from dariakp May 22, 2023 19:27
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Reviewed & resolved the addressed comments

@nbbeeken nbbeeken requested a review from dariakp May 22, 2023 21:11
src/cursor/run_command_cursor.ts Outdated Show resolved Hide resolved
dariakp
dariakp previously approved these changes May 24, 2023
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels May 24, 2023
await client.close();
});

it('returns each document only once across multiple iterators', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this test be on AbstractCursor instead of runCommandCursor, since the behavior comes from the abstract class? This test is not specific to our RunCommandCursor.

Why was it added now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The functionality is in the AbstractCursor but as an integration test I'm using the public API to reproduce the expected behavior. I was curious what the behavior of multiple iterators (or nested for-await loops) from one cursor is. Returning a document only once regardless of how many iterators there are is supported by nature of our dequeuing from a List. Since it is a data structure we have changed in the past it is possible that we could modify this behavior if, say, we used an offset into a List that is specific to each iterator.

} catch (e) {
return null;
}
function getChangeStream({ entities, operation }): UnifiedChangeStream | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this potentially be a method on the entities object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, moved to entities

Comment on lines 375 to 383
const changeStream = getChangeStream({ entities, operation });
if (changeStream == null) {
// iterateOnce is used for changes streams and regular cursors.
// we have no other way to distinguish which scenario we are testing when we run an
// iterateOnce operation, so we first try to get the changeStream and
// if that fails, we know we need to get a cursor
const cursor = entities.getEntity('cursor', operation.object);
return cursor.tryNext();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const changeStream = getChangeStream({ entities, operation });
if (changeStream == null) {
// iterateOnce is used for changes streams and regular cursors.
// we have no other way to distinguish which scenario we are testing when we run an
// iterateOnce operation, so we first try to get the changeStream and
// if that fails, we know we need to get a cursor
const cursor = entities.getEntity('cursor', operation.object);
return cursor.tryNext();
}
const cursorlike = getChangeStream(..) ?? entities.getEntity('cursor');
return cursorLike.tryNext();

Copy link
Contributor

Choose a reason for hiding this comment

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

same suggestion above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to a helper on entities

test/tools/unified-spec-runner/operations.ts Outdated Show resolved Hide resolved
@dariakp dariakp merged commit 4da926e into main May 24, 2023
@dariakp dariakp deleted the NODE-5019-runCommandCursor branch May 24, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants