-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(NODE-5496): remove client-side collection and database name check validation #3873
Conversation
There was a problem hiding this 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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
There was a problem hiding this 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?
@baileympearson @nbbeeken @W-A-James 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") ? |
I notice the database name does not encounter an error whereas the collection name will trigger a https://github.com/mongodb/node-mongodb-native/blob/main/src/cmap/connection.ts#L542 |
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? |
Description for NODE-5496
The previous internal Node Driver
checkCollectionName
functions was more narrow with name check guidelines than server. This ledcheckCollectionName
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
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript