-
Notifications
You must be signed in to change notification settings - Fork 229
feat: Using the AtlasUserData class to demo save user data project COMPASS-9565 #7152
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
Merge branch 'user-data-interface' of https://github.com/mongodb-js/compass into user-data-interface
Merge branch 'extend-user-data' of https://github.com/mongodb-js/compass into extend-user-data
Co-authored-by: Copilot <[email protected]>
Merge branch 'extend-user-data' of https://github.com/mongodb-js/compass into extend-user-data
Merge branch 'main' into extend-user-data
Merge branch 'extend-user-data' of https://github.com/mongodb-js/compass into extend-user-data
options.getResourceUrl, | ||
options.authenticatedFetch, | ||
{ | ||
serialize: (content) => EJSON.stringify(content, undefined, 2), |
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.
This is good for debugging but I'd expect us to use unspaced EJSON in prod
// TODO: verify that this doesn't break anything in compass | ||
const _id = new ObjectId().toHexString(); |
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.
Was there something wrong with UUIDs?
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.
In either case we haven't changed the type, just the string format, is that what we want here? just making sure if we want an ObjectId or UUID to be stored, we shouldn't toString this, we should leave it as the type we want
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.
Is this going to impact existing compass data that does use UUID?
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.
Notes:
- For now we will just make the zod schema support string without the uuid assertion.
- We should consider storing OID directly to keep type information encoded into the format, just need to get zod to agree to that.
- We need to handle migrations of existing compass files (-ish) depending on what relies on
_id
abstract readOne( | ||
id: string, | ||
options?: ReadOptions | ||
): Promise<z.output<T> | undefined>; |
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.
When does this return undefined
? I kinda would've expected this to return a version of ReadAllResult
that either has a single item in the arrays or the properties are one item instead of an array
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.
Identified that readOne
in AtlasUserData should not be returning null
when an error is found. readOne
in FileUserData
originally had a return type of Promise<z.output<T> | undefined>
, so the abstract method should also have type undefined
.
// TODO: verify that this doesn't break anything in compass | ||
const _id = new ObjectId().toHexString(); |
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.
Is this going to impact existing compass data that does use UUID?
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 adds support for using the AtlasUserData class to demonstrate saving user data in Compass Web, integrating Atlas cloud storage for queries and pipelines alongside the existing file-based storage.
- Updates query and pipeline storage classes to support both Atlas cloud storage and file-based storage
- Modifies ID generation from UUID to ObjectId and changes schema validation to accept any string for
_id
- Integrates Atlas storage providers into the Compass Web entrypoint with proper authentication
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
packages/my-queries-storage/src/query-storage.ts | Adds optional id parameter to favorite query interface |
packages/my-queries-storage/src/query-storage-schema.ts | Changes _id validation from UUID to any string |
packages/my-queries-storage/src/compass-query-storage.ts | Adds Atlas storage support and switches from UUID to ObjectId |
packages/my-queries-storage/src/compass-pipeline-storage.ts | Adds Atlas storage support with dual storage backend logic |
packages/my-queries-storage/src/compass-pipeline-storage.spec.ts | Updates test to use new constructor options format |
packages/compass-web/src/entrypoint.tsx | Integrates Atlas storage providers and removes context menu tracking |
packages/compass-user-data/src/user-data.ts | Adds readOne method to interface and refactors AtlasUserData API calls |
packages/compass-user-data/src/user-data.spec.ts | Updates tests for new API endpoint structure |
packages/compass-sidebar/src/components/multiple-connections/navigation/navigation.tsx | Adds redundant condition to My Queries visibility |
packages/compass-query-bar/src/stores/query-bar-reducer.ts | Changes from updateAttributes to saveQuery method |
packages/compass-data-modeling/src/services/data-model-storage-electron.tsx | Adds whitespace formatting |
packages/atlas-service/src/provider.tsx | Exports useAtlasServiceContext function |
packages/atlas-service/src/atlas-service.ts | Adds user data endpoints and includes credentials in authenticated requests |
package.json | Adds @leafygreen-ui/avatar dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
): Promise<void> { | ||
const _id = new UUID().toString(); | ||
// TODO: verify that this doesn't break anything in compass | ||
_id ??= new ObjectId().toHexString(); |
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.
Variable _id
is being used but the parameter is named id
in the interface. This should use the correct parameter name to avoid potential undefined behavior.
Copilot uses AI. Check for mistakes.
const getResourceUrl = (path?: string) => { | ||
const url = atlasService.userDataEndpoint(`/${path || ''}`); | ||
console.log('getResourceUrl called with path:', path, '-> URL:', url); |
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.
Console.log statements should not be present in production code. Remove this debug statement or replace with proper logging.
const getResourceUrl = (path?: string) => { | |
const url = atlasService.userDataEndpoint(`/${path || ''}`); | |
console.log('getResourceUrl called with path:', path, '-> URL:', url); | |
const logger = useCompassWebLogger(); | |
const getResourceUrl = (path?: string) => { | |
const url = atlasService.userDataEndpoint(`/${path || ''}`); | |
logger.debug('getResourceUrl called with path:', path, '-> URL:', url); |
Copilot uses AI. Check for mistakes.
@@ -104,7 +104,7 @@ export function Navigation({ | |||
const isDataModelingEnabled = usePreference('enableDataModeling'); | |||
return ( | |||
<div> | |||
{hasWorkspacePlugin('My Queries') && ( | |||
{hasWorkspacePlugin('My Queries') && true && ( |
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.
The redundant && true
condition serves no purpose and makes the code unclear. Remove this unnecessary condition.
{hasWorkspacePlugin('My Queries') && true && ( | |
{hasWorkspacePlugin('My Queries') && ( |
Copilot uses AI. Check for mistakes.
// TODO: Re-add context menu tracking once CompassComponentsProvider supports these props | ||
// onContextMenuOpen and onContextMenuItemClick props are not available in current version |
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.
[nitpick] Important functionality has been commented out. Consider tracking this as a proper issue or ticket rather than leaving TODO comments in production code.
// TODO: Re-add context menu tracking once CompassComponentsProvider supports these props | |
// onContextMenuOpen and onContextMenuItemClick props are not available in current version | |
// Context menu tracking is currently disabled until CompassComponentsProvider supports | |
// onContextMenuOpen and onContextMenuItemClick props. | |
// Tracked in issue: https://github.com/mongodb-js/compass/issues/XXXX |
Copilot uses AI. Check for mistakes.
return `https://cluster-connection.cloud-dev.mongodb.com${normalizePath( | ||
path | ||
)}`; | ||
} | ||
userDataEndpoint(path?: string): string { | ||
return `https://cluster-connection.cloud-dev.mongodb.com/userData${normalizePath( | ||
path | ||
)}`; |
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.
Hard-coded development endpoint URL in production code poses security risks. This should be configurable or removed from production builds.
return `https://cluster-connection.cloud-dev.mongodb.com${normalizePath( | |
path | |
)}`; | |
} | |
userDataEndpoint(path?: string): string { | |
return `https://cluster-connection.cloud-dev.mongodb.com/userData${normalizePath( | |
path | |
)}`; | |
if (!this.config.tempBaseUrl) { | |
throw new Error('tempBaseUrl is not configured'); | |
} | |
return `${this.config.tempBaseUrl}${normalizePath(path)}`; | |
} | |
userDataEndpoint(path?: string): string { | |
if (!this.config.tempBaseUrl) { | |
throw new Error('tempBaseUrl is not configured'); | |
} | |
return `${this.config.tempBaseUrl}/userData${normalizePath(path)}`; |
Copilot uses AI. Check for mistakes.
return `https://cluster-connection.cloud-dev.mongodb.com${normalizePath( | ||
path | ||
)}`; | ||
} | ||
userDataEndpoint(path?: string): string { | ||
return `https://cluster-connection.cloud-dev.mongodb.com/userData${normalizePath( | ||
path | ||
)}`; |
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.
Hard-coded development endpoint URL in production code poses security risks. This should be configurable or removed from production builds.
return `https://cluster-connection.cloud-dev.mongodb.com${normalizePath( | |
path | |
)}`; | |
} | |
userDataEndpoint(path?: string): string { | |
return `https://cluster-connection.cloud-dev.mongodb.com/userData${normalizePath( | |
path | |
)}`; | |
if (!this.config.tempEndpointBaseUrl) { | |
throw new Error('tempEndpointBaseUrl is not configured'); | |
} | |
return `${this.config.tempEndpointBaseUrl}${normalizePath(path)}`; | |
} | |
userDataEndpoint(path?: string): string { | |
if (!this.config.userDataEndpointBaseUrl) { | |
throw new Error('userDataEndpointBaseUrl is not configured'); | |
} | |
return `${this.config.userDataEndpointBaseUrl}${normalizePath(path)}`; |
Copilot uses AI. Check for mistakes.
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes