Skip to content

feat(NODE-5314): add search index helpers #3672

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

Merged
merged 25 commits into from
May 31, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented May 17, 2023

Description

POC for mongodb/specifications#1423.

What is changing?

  • 5 new operations are added to the unified test runner
  • new collection helpers are added to enable search index management
Is there new documentation needed for these changes?

No.

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

@baileympearson baileympearson requested a review from durran May 17, 2023 20:39
@baileympearson baileympearson marked this pull request as ready for review May 18, 2023 19:24
@baileympearson baileympearson changed the title add search index helpers feat(NODE-5194): add search index helpers May 18, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 19, 2023
@durran durran self-assigned this May 19, 2023
* the `collection` portion of the namespace is defined and should only be
* used in scenarios where this can be guaranteed.
*/
export class MongoDBCollectionNamespace extends MongoDBNamespace {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is not necessary but without it, the operations added in this PR must either assert on the namespace's collection string being truthy or provide a default value, but in all cases the namespace has a collection defined (they're all operations defined on the collection).

Copy link
Contributor

Choose a reason for hiding this comment

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

I have another thread where I mention we do not need this if we pass through the namespace instead of the collection instance, so let's address that first. But following up here let's unit test withCollection & this class

@baileympearson baileympearson requested a review from durran May 24, 2023 15:47
@nbbeeken nbbeeken self-requested a review May 26, 2023 15:16
@baileympearson baileympearson requested review from durran and lerouxb May 26, 2023 16:54
lerouxb
lerouxb previously approved these changes May 27, 2023
@durran durran 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 30, 2023
@baileympearson baileympearson changed the title feat(NODE-5194): add search index helpers feat(NODE-5314): add search index helpers May 30, 2023
durran
durran previously approved these changes May 30, 2023
Comment on lines +1018 to +1030
listSearchIndexes(
indexNameOrOptions?: string | ListSearchIndexesOptions,
options?: ListSearchIndexesOptions
): ListSearchIndexesCursor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the return value is the same for all these methods so we should just have the union type argument and optional options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not quite the same. Collapsing them into a single overload implies that listSearchIndexes(AggregateOptions, AggregateOptions) is a valid signature. Overloading the method clarifies which combinations of arguments are valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Calling the method with listSearchIndexes(options, options) will work though, so I don't know if there's much risk to that implication. I think that the UX is reasonably clear without it

But if we do keep the overload the one after the first one doesn't have documentation, overloads each need their own tsdoc block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-05-30 at 1 00 49 PM Screenshot 2023-05-30 at 1 00 30 PM

Maybe this is my particular workflow, but I often peruse the overloads for methods by scrolling through the modal that pops up in vscode. I think it's beneficial to provide clear intended usages. I added an additional tsdoc comment for the new overload.

Also, listSearchIndexes(options, options) won't work because the actual pipeline stage sent will be:

{ $listSearchIndexes: { name: <aggregation options > } }

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that's fair, I think the union of types is equally clear, the possible inputs are listed in one popup, no scrolling needed.

Also, listSearchIndexes(options, options) won't work because the actual pipeline stage sent will be: { $listSearchIndexes: { name: <aggregation options > } }

From the option handling:

   : typeof indexNameOrOptions === 'object'
        ? null

This case should set the name to null and then the pipeline will be constructed correctly in the cursor since we test for name == null.

* the `collection` portion of the namespace is defined and should only be
* used in scenarios where this can be guaranteed.
*/
export class MongoDBCollectionNamespace extends MongoDBNamespace {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have another thread where I mention we do not need this if we pass through the namespace instead of the collection instance, so let's address that first. But following up here let's unit test withCollection & this class

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.

Just some clarifying questions

}

/**
* @internal
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be @internal? Once released we're going to have to use them from mongosh, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to release an untested API as public yet, so they're internal for now until we can manually e2e test them.

@internal only strips things out of the Typescript definitions, everything will still be accessible. It might be slightly more inconvenient to work with in mongosh, but it should still work.

Is that alright with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, I guess. I forked this branch so I can work with it locally in the meantime anyway.

@baileympearson baileympearson force-pushed the NODE-5194-node-poc-search-indexes branch from c5fcc23 to b66854c Compare May 31, 2023 17:15
nbbeeken
nbbeeken previously approved these changes May 31, 2023
@durran durran merged commit f647542 into mongodb:main May 31, 2023
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.

5 participants