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-5314): add search index helpers #3672

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 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

package.json Outdated Show resolved Hide resolved
@nbbeeken nbbeeken self-requested a review May 26, 2023 15:16
lerouxb
lerouxb previously approved these changes May 27, 2023
src/collection.ts Outdated Show resolved Hide resolved
@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
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
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.

src/collection.ts Outdated Show resolved Hide resolved
src/collection.ts Outdated Show resolved Hide resolved
src/operations/search_indexes/drop.ts Outdated Show resolved Hide resolved
src/operations/search_indexes/drop.ts Outdated Show resolved Hide resolved
src/operations/search_indexes/list.ts Outdated Show resolved Hide resolved
src/operations/search_indexes/update.ts Outdated Show resolved Hide resolved
* 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

src/db.ts Outdated Show resolved Hide resolved
test/unit/utils.test.ts Outdated Show resolved Hide resolved
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

src/cursor/list_search_indexes_cursor.ts Outdated Show resolved Hide resolved
src/collection.ts Show resolved Hide resolved
}

/**
* @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.

nbbeeken
nbbeeken previously approved these changes May 31, 2023
@durran durran merged commit f647542 into mongodb:main May 31, 2023
14 of 23 checks passed
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
5 participants