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.
| 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?
| 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.
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.
| `Failed to update data: ${response.status} ${response.statusText}` | ||
| ); | ||
| } | ||
| return this.validator.parse(data); |
There was a problem hiding this comment.
The method returns the parsed input data instead of the actual updated data from the server response. This could lead to inconsistencies if the server modifies the data during update.
| return this.validator.parse(data); | |
| const responseData = await response.json(); | |
| return this.validator.parse(this.deserialize(responseData)); |
Copilot uses AI. Check for mistakes.
| return `${this.BASE_URL}/${this.type}/${this.orgId}/${this.groupId}`; | ||
| } |
There was a problem hiding this comment.
The URL construction should include protocol (https://) and validate/sanitize the orgId, groupId, and type parameters to prevent URL injection attacks.
| return `${this.BASE_URL}/${this.type}/${this.orgId}/${this.groupId}`; | |
| } | |
| const sanitizedType = this.sanitizeInput(this.type); | |
| const sanitizedOrgId = this.sanitizeInput(this.orgId); | |
| const sanitizedGroupId = this.sanitizeInput(this.groupId); | |
| const baseUrl = this.BASE_URL.startsWith('https://') ? this.BASE_URL : `https://${this.BASE_URL}`; | |
| return `${baseUrl}/${sanitizedType}/${sanitizedOrgId}/${sanitizedGroupId}`; | |
| } | |
| private sanitizeInput(input: string): string { | |
| // Basic sanitization: remove any characters that are not alphanumeric, dashes, or underscores | |
| return input.replace(/[^a-zA-Z0-9-_]/g, ''); | |
| } |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Merge branch 'extend-user-data' of https://github.com/mongodb-js/compass 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