-
Notifications
You must be signed in to change notification settings - Fork 245
fix(compass-user-data): remove divergent user data types from atlas implementation CLOUDP-334901 #7543
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
ece76a1 to
78dd30c
Compare
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.
Nice, changes look good, left 2 questions, not blockers.
78dd30c to
b406724
Compare
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.
Pull Request Overview
This PR standardizes user data type strings across filesystem and Atlas implementations by migrating from camelCase to PascalCase naming (e.g., favoriteQueries → FavoriteQueries). This unification simplifies future additions of new data types by ensuring consistent handling between storage backends.
Key changes:
- Introduced a typed
UserDataTypeunion with validation to enforce valid data type strings - Added migration support in
FileUserDatato preserve existing user data during the naming transition - Updated all storage factory functions and Atlas service endpoints to use the new PascalCase naming
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/my-queries-storage/src/storage-factories.ts | Updated storage factory functions to use PascalCase data types |
| packages/compass-web/src/entrypoint.tsx | Replaced hardcoded type union with assertsUserDataType validation |
| packages/compass-web/package.json | Added compass-user-data dependency |
| packages/compass-user-data/src/user-data.ts | Added UserDataType enum, validation function, and migration support |
| packages/compass-user-data/src/user-data.spec.ts | Added comprehensive migration tests and updated existing tests for new naming |
| packages/compass-user-data/src/index.ts | Exported new UserDataType and assertsUserDataType |
| packages/compass-shell/src/modules/history-storage.ts | Migrated from app-name-based folder to ShellHistory with migration support |
| packages/compass-shell/src/modules/history-storage.spec.ts | Updated test file path to reflect new folder structure |
| packages/compass-data-modeling/src/services/data-model-storage-web.tsx | Updated to use PascalCase DataModelDescriptions |
| packages/atlas-service/src/atlas-service.ts | Replaced inline type union with UserDataType |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const oldAppName = getAppName(); | ||
| if (oldAppName && oldAppName !== 'ShellHistory') { | ||
| await this.userData.migrateFromOldFolder(oldAppName); | ||
| } |
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.
Not sure what this condition is checking. This will always be true, appName is never going to be ShellHistory
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.
Good catch, was thinking very birds eye view of the file rename process, you're correct!
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.
How do you feel about instead throwing if the appName isn't present and a variable rename? A nit, not a blocker
| const oldAppName = getAppName(); | |
| if (oldAppName && oldAppName !== 'ShellHistory') { | |
| await this.userData.migrateFromOldFolder(oldAppName); | |
| } | |
| const appName = getAppName(); | |
| if (!appName) { | |
| throw new Error('Cannot initialize shell history storage without app name'); | |
| } | |
| // We used to store the shell-history in a folder like `MongoDB Compass/shell-history.json`. | |
| await this.userData.migrateFromOldFolder(appName); |
Anemy
left a comment
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! nice.
One suggestion would be to also include the migration for the Import log path while we're at it so we can mark the COMPASS-7080 ticket done.
compass/packages/compass-import-export/src/utils/get-user-data-file-path.ts
|
TY all for the reviews. This is not mergable until we've put the new endpoints in prod, because I don't want compass-web to be in a state where updating it would break. I will merge this when we've shipped the BE changes. |
Description
We now use the same user data string for filesystem and atlas this makes it easier in the future to add data types that should "automatically" work with just additionally support added to the backend.
Checklist
Motivation and Context
Open Questions
I needed to do the work of https://jira.mongodb.org/browse/COMPASS-7080 and add a migration for the existing folder. This is a bit more than expected but didn't feel like the most complex addition to this work. I can break this out to its own change that we merge first if we'd like that.
Dependencies
CCS must support the new endpoints before we ship these changes
Types of changes