feat(save-user-data): extend user data abstract class for api backend COMPASS-9558 #7114
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
| } | ||
|
|
||
| return true; | ||
| } catch (error) { |
There was a problem hiding this comment.
How does the logging look? Are there any changes that I should make to make this more consistent with Compass behavior?
| ) => Promise<Response>, | ||
| orgId: string, | ||
| groupId: string, | ||
| type: string, |
There was a problem hiding this comment.
Should type be more strictly typed? I originally created a AtlasUserDataType enum so that the inputted type would be more strictly limited to the declared values in the enum, but I thought this might pose issues later on when importing — could be totally wrong about this.
There was a problem hiding this comment.
Yeah, I think if you want an enum here (we prefer string unions in the codebase, but the idea is the same, it's stricter), you might have troubles injecting it easily. I do think though that you can share this property with the base UserData interface and then provide a stricter set of values there
There was a problem hiding this comment.
What would be the advantage of sharing this property with the base UserData interface?
| const json = await response.json(); | ||
| const data = Array.isArray(json) | ||
| ? json.map((item) => | ||
| this.validator.parse(this.deserialize(item.data as string)) |
There was a problem hiding this comment.
I wonder whether it should parse one by one.
For example, suppose 3 queries in the output are well-formatted, and 1 isn't. As it currently stands, readAll would not return any values in data, and simply returns an error in the errors array. If we put the try-catch inside the loop, then we could add to data and errors as necessary. In theory, the data should always be well-formatted, though, because we also parse when we call write (?) — maybe updateAttributes should also parse as well?
There was a problem hiding this comment.
Yeah, you can look at the file user data as an example here, we do parse one by one and don't fail the whole fetch if only one document is malformed for some reason, this should follow the pattern
| async updateAttributes( | ||
| id: string, | ||
| data: Partial<z.input<T>> | ||
| ): Promise<z.output<T>> { |
There was a problem hiding this comment.
Why does the file system have updateAttributes as returning the whole data item? From a look through the code, I didn't see any instances of where the output of updateAttributes was actually used. Would the backend endpoint need to change to reflect the file system, or the other way around?
There was a problem hiding this comment.
If it's not used anywhere, I think it would be fine to remove from the shared interface
There was a problem hiding this comment.
Pull Request Overview
This PR extends the user data abstract class system by implementing a new AtlasUserData class that communicates with Atlas backend API endpoints for cloud-based user data storage, as an alternative to the existing file-based storage.
- Adds
AtlasUserDataclass implementing theIUserDatainterface for API-based storage - Includes comprehensive test coverage for all CRUD operations and URL construction
- Updates exports to make the new class available to consumers
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/compass-user-data/src/user-data.ts | Implements AtlasUserData class with HTTP-based CRUD operations |
| packages/compass-user-data/src/user-data.spec.ts | Adds comprehensive test suite for AtlasUserData functionality |
| packages/compass-user-data/src/index.ts | Exports the new AtlasUserData class |
| private orgId: string = ''; | ||
| private groupId: string = ''; | ||
| private readonly type: string; | ||
| private readonly BASE_URL = 'cluster-connection.cloud-local.mongodb.com'; |
There was a problem hiding this comment.
The hardcoded BASE_URL should be configurable through constructor parameters or environment variables to support different deployment environments (development, staging, production).
| private readonly BASE_URL = 'cluster-connection.cloud-local.mongodb.com'; | |
| private readonly BASE_URL: string; |
Copilot uses AI. Check for mistakes.
Merge branch 'extend-user-data' of https://github.com/mongodb-js/compass into extend-user-data
| } | ||
|
|
||
| private getUrl() { | ||
| return `${this.BASE_URL}/${this.type}/${this.orgId}/${this.groupId}`; |
There was a problem hiding this comment.
The this.BASE_URL part should come from atlas service, this is where we currently keep all the logic for correctly building all atlas related urls (see cloudEndpoint or driverProxyEndpoint)
| export class AtlasUserData<T extends z.Schema> extends IUserData<T> { | ||
| private readonly authenticatedFetch; | ||
| private orgId: string = ''; | ||
| private groupId: string = ''; |
There was a problem hiding this comment.
We use projectId instead of groupId in Compass. "Groups" are a legacy name for projects:
Some more info: https://www.mongodb.com/docs/atlas/api/atlas-admin-api-ref/#project-id
Merge branch 'main' into extend-user-data
Description
Extend the IUserData abstract class to create an AtlasUserData class that uses the endpoints created in CCS. Contains unit testing.
Checklist
Motivation and Context
Open Questions
See comments
Dependents
Types of changes