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

fix(NODE-5496): remove client-side collection and database name check validation #3873

Merged
merged 15 commits into from
Oct 4, 2023

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Sep 21, 2023

Description for NODE-5496

The previous internal Node Driver checkCollectionName functions was more narrow with name check guidelines than server. This led checkCollectionName to invalidate legal collection names. This affects Mongosh and Compass since they rely on the Node Driver and need to be able to access all valid collection names.

The Node Driver has made the decision to only rely on server validation for collection name checking, and for database name checking. This is because collections and databases are so closely related.

In addition, string coercion for database names is removed for the Msg and Query class.

What is changing?

The Node Driver will no longer error-check for database naming and collection naming, and leave it to the server to validate.
Namely, checkCollectionName used to incorrectly invalidate collection names that started or ended with a '.' character.

However, for the case of the dot character in a database name, the Node Driver will not remove client-side validation for this case. This is because the dot character in a database name will cause ambiguous behavior before it is sent to the server.

Is there new documentation needed for these changes?

Yes, the API now allows for more collection names.

What is the motivation for this change?

The motivation is to ensure the driver is adjusted to match server naming restrictions for collections and databases.

Bug Description

Current Behavior

The following code leads to an error.
db.createCollection('spider.');

Expected Behavior

No error

Release Highlight

Database and collection name checking will be in sync with the MongoDB server's name requirements.
Specifically, users can now create collections that start or end with the '.' character.

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

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title Node 5496/collection name check does not match docs fix(Node 5496): remove client-side collection and database name check validation Sep 21, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review September 22, 2023 14:46
@nbbeeken nbbeeken changed the title fix(Node 5496): remove client-side collection and database name check validation fix(NODE-5496): remove client-side collection and database name check validation Sep 22, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Some fixups, we also wanted to introduce API docs to MongoClient .db() and Db .collection() that mention the new behavior, something like: "namespace validation occurs at operation time"

test/integration/node-specific/db.test.js Outdated Show resolved Hide resolved
test/integration/node-specific/db.test.js Outdated Show resolved Hide resolved
test/integration/node-specific/db.test.js Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken self-assigned this Sep 25, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 25, 2023
src/db.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
test/integration/collection-management/collection.test.ts Outdated Show resolved Hide resolved
test/integration/node-specific/db.test.js Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 27, 2023
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM

src/db.ts Show resolved Hide resolved
@W-A-James W-A-James self-requested a review September 28, 2023 13:46
Copy link
Contributor

@W-A-James W-A-James left a comment

Choose a reason for hiding this comment

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

LGTM pending some API docs changes and a question

src/db.ts Outdated Show resolved Hide resolved
src/db.ts Outdated Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes Sep 28, 2023
nbbeeken
nbbeeken previously approved these changes Sep 29, 2023
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

question: should we at least check that the value passed to new Db() and new Collection() are of type string?

@aditi-khare-mongoDB
Copy link
Contributor Author

aditi-khare-mongoDB commented Oct 2, 2023

question: should we at least check that the value passed to new Db() and new Collection() are of type string?

@baileympearson @nbbeeken @W-A-James
If we use a number as a database name, no error is thrown. I think we should definitely check for type on client-side since this is the case.

Since we now have to client-side error-check for 1) type checking, 2) ensuring no '.' character in db, it seems like edge cases might keep arising. Should we stick with our current release notes ("The Node Driver will no longer error-check for database naming and collection naming, except edge cases x and y") ?

@nbbeeken
Copy link
Contributor

nbbeeken commented Oct 2, 2023

I notice the database name does not encounter an error whereas the collection name will trigger a BadValue server error stating the collection name is the incorrect type. I think it is important to consider that our BSON library is the point at which types sent to the server matter, javascript strings are not the only input that can be serialized as a BSON string, and if our driver does not need access to the input then it should allow it to reach the BSON layer so it can be serialized to the correct value.

https://github.com/mongodb/node-mongodb-native/blob/main/src/cmap/connection.ts#L542
The reason database names do not error is because of this line that implicitly stringifies whatever value the db name is. I vaguely recall us discussing correcting this line to just pass in the db name directly without building the dotted string, should we consider correcting that instead?

@aditi-khare-mongoDB
Copy link
Contributor Author

aditi-khare-mongoDB commented Oct 2, 2023

https://github.com/mongodb/node-mongodb-native/blob/main/src/cmap/connection.ts#L542 Would correcting this line be part of this ticket? If so, how would we go about correcting this line / would it be more straight-forward to just error on non-string input for databases?

nbbeeken
nbbeeken previously approved these changes Oct 2, 2023
nbbeeken
nbbeeken previously approved these changes Oct 3, 2023
src/cmap/commands.ts Outdated Show resolved Hide resolved
src/cmap/command_monitoring_events.ts Show resolved Hide resolved
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.

4 participants