-
Notifications
You must be signed in to change notification settings - Fork 228
feat(compass-collection): Schema Analysis Redux Integration for Collection Plugin CLOUDP-333846 #7177
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
Conversation
|
||
interface SchemaAnalysisFinishedAction { | ||
type: CollectionActions.SchemaAnalysisFinished; | ||
schemaAnalysis: SchemaAnalysis; |
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.
Actions shouldn't be controlling the state, reducer should, passing the whole state and just assigning it in the reducer is an antipattern that should be avoided, don't do this. Clearly separate what is the action payload and what is the state that reducer will derive based on the action
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 updated this. Wondering if it's worth it to move this to its own reducer. Thoughts?
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.
Looks pretty self contained, so can be its own slice, yeah
@jcobis @gribnoysup Heads up - Adding In order to remove the cyclical dependency, I think we'd have to remove the Since we only need schema_depth from this function, I'm considering extracting/duplicating just part of this calculation in |
@ncarbon I don't think you can easily remove the dependency here, compass-collection is the place that keeps the source of truth for this type, you can't really move it somewhere else without creating an awkward intermediate place for it and I think I'd prefer we avoid doing that 🙂 I think if you want to just implement the depth counting somewhere near the code that does the check that's fine (in your case you don't even need to count the full depth, right? only that it doesn't hit a relatively small limit). You should also consider moving this code to mongodb-schema package that is already this one place where we accumulate all the shared code related to schema analysis. |
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 work Nataly!
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 introduces schema analysis functionality to the Compass Collection Tab, automatically analyzing a collection's schema when it loads (excluding read-only and time-series collections). The implementation includes new Redux state management for schema analysis with comprehensive status tracking and error handling.
- Adds new Redux state management for schema analysis with status tracking (initial, analyzing, completed, error)
- Implements automatic schema analysis on collection load with proper filtering for read-only and time-series collections
- Introduces schema depth calculation utility for analyzing nested document structures
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
collection-tab.ts | Adds schema analysis state initialization and triggers analysis on metadata load |
collection-tab.spec.ts | Updates tests to mock schema analysis and verify conditional execution |
schema-analysis-types.ts | Defines TypeScript types for schema analysis state management |
collection-tab.ts (modules) | Implements core schema analysis thunk action with document sampling and error handling |
calculate-schema-depth.ts | Utility function for calculating maximum nesting depth in schema structures |
calculate-schema-depth.spec.ts | Comprehensive test suite for schema depth calculation |
package.json | Adds required dependencies for schema analysis functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
diffs for my last feedback and additions around calculate-schema-depth
are good for me
Description
This pull request introduces schema analysis functionality to the Compass Collection Tab, enabling the application to automatically analyze a collection's schema when it is loaded (unless the collection is read-only or time-series). The changes include new Redux state and actions for schema analysis, and a thunk to perform the analysis.
Motivation
Schema Analysis Feature:
SchemaAnalysis
state toCollectionState
, with status tracking (INITIAL
,ANALYZING
,COMPLETED
,ERROR
), schema data, sample document, schema metadata, and error information.analyzeCollectionSchema
that samples documents, analyzes the schema, calculates metadata (like max nesting depth and validation rules), and updates the Redux state accordingly.Store Initialization and Integration:
schemaAnalysis
state and inject required dependencies (logger, preferences, abort controller) for schema analysis.Testing Enhancements:
Checklist
Motivation and Context
To extract schema information (types and validation rules) that will be used in the upcoming mock data generator feature.
Open Questions
Dependents
Types of changes