-
Notifications
You must be signed in to change notification settings - Fork 229
feat(database-model,databases-collections,collection-model): hide system DBs COMPASS-9674 #7324
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
base: main
Are you sure you want to change the base?
Conversation
I'll fix the tests but open to any initial feedback on this overall approach of hiding and disallowing additions to the"special" collections |
@@ -379,6 +383,14 @@ export const createNamespace = ( | |||
dispatch(handleError(new Error(NO_DOT))); |
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.
I think I broke this flow ~2 years ago and we never noticed. This should return, otherwise we are continuing to create the namespace even if validation failed.
dispatch(handleError(new Error(NO_DOT))); | |
return dispatch(handleError(new Error(NO_DOT))); |
if (dbName && toNS(dbName).special) { | ||
dispatch(handleError(new Error(INTERNAL_DATABASE))); | ||
} | ||
|
||
if (toNS(namespace).special) { | ||
dispatch(handleError(new Error(INTERNAL_COLLECTION))); | ||
} |
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.
Same wrt returns
if (dbName && toNS(dbName).special) { | |
dispatch(handleError(new Error(INTERNAL_DATABASE))); | |
} | |
if (toNS(namespace).special) { | |
dispatch(handleError(new Error(INTERNAL_COLLECTION))); | |
} | |
if (dbName && toNS(dbName).special) { | |
return dispatch(handleError(new Error(INTERNAL_DATABASE))); | |
} | |
if (toNS(namespace).special) { | |
return dispatch(handleError(new Error(INTERNAL_COLLECTION))); | |
} |
.filter((db) => { | ||
return toNs(db.name).special === false; | ||
}) |
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.
From the top of my head I think that should be an okay change, but I don't know enough about these special collections to say with confidence that we are not breaking some power user use-cases in Compass. Would it make sense to gate this with some new preference setting and allow people to opt-in?
WRITING-33247 defines a new pattern of MongoDB owned internal DBs prefixed with
__mdb_internal_
Here I propose hiding all special dbs as determined by the mongodb-ns package with the reasoning that users should generally not be aware of these internal DBs, nor should they make changes to them
As of right now, this does not stop a user from creating a new collection in these DBs
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes