-
Couldn't load subscription status.
- Fork 239
feat(compass-data-modeling): update field type COMPASS-9659 #7248
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
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 pull request implements field type change functionality for the data modeling component. Users can now modify field types through the UI, with proper validation and schema handling.
Key changes:
- Added
getSchemaWithNewTypes()function to handle complex schema transformations when field types change - Implemented
changeFieldTypeaction creator andChangeFieldTypeedit operation - Enhanced the field drawer UI to allow type editing with validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/compass-data-modeling/test/fixtures/data-model-with-relationships.json |
Updated field name from id to _id in test fixture |
packages/compass-data-modeling/src/utils/schema-traversal.tsx |
Added getSchemaWithNewTypes() function and schema transformation logic |
packages/compass-data-modeling/src/utils/schema-traversal.spec.tsx |
Added comprehensive tests for the new schema transformation functionality |
packages/compass-data-modeling/src/store/diagram.ts |
Implemented changeFieldType action creator |
packages/compass-data-modeling/src/store/apply-edit.ts |
Added handling for ChangeFieldType edit operation |
packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx |
Enhanced UI with type change functionality and validation |
packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx |
Added tests for field type change functionality and readonly behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ebf0647 to
7f6b5d2
Compare
packages/compass-data-modeling/src/components/drawer/field-drawer-content.tsx
Outdated
Show resolved
Hide resolved
| if (!collectionSchema) throw new Error('Collection not found in model'); | ||
| const field = getFieldFromSchema({ | ||
| jsonSchema: collectionSchema, | ||
| fieldPath: fieldPath, | ||
| }); | ||
| if (!field) throw new Error('Field not found in schema'); |
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.
For other edits like relationships we do validations like this in the applyEdit method, why are we breaking this pattern here?
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.
Actually for the name we do validation in the component and display the error next to the field. But this is a good question, we have these editErrors that are not displayed anywhere anymore. They must've fallen through the cracks when we were moving away from the placeholder editor. But I don't see the store validating relationships specifically, just checking the edit against the zod schema? Which is superfluous now, this was needed for the original editor where you'd submit a json edit so we couldn't depend on types. Or am I looking at something else than you found?
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.
Yeah, I'm not talking about form field validation here, keeping it near form UI makes sense. Specifically for apply edit we're checking whether or not the model or relationship is missing in the applyEdit method that generates the schema itself, not in the corresponding apply methods, so let's just figure out where we want it, otherwise we are ending up in a weird situation where similar types of errors in edits will have different effects on the application because validation is happening in different places 🙂 Right now the pattern seems to be to validate these in applyEdit and not when the action for adding the edit is dispached
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.
Ah, sorry, I didn't notice which line you were commenting on :D
The problem here is, we're storing the Edit with the new field jsonSchema - so these failures happen already when we're prepping the Edit to be stored.
To change that, and to move the failure to applyEdit (which gets repeated when you move around in the history or open the diagram), we'd have to change the approach and not store the jsonSchema in the Edit. Instead we'd just store the new types and recalc the field schema each time the Edit is applied.
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.
Hmmm, wouldn't we want to do that in the first place? This apply method is the source of truth for calculating the final schema, I don't think we want to spread this logic around too much
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.
(we can leave this for a follow-up, but should figure out and agree on a consistent way of handling this)
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.
My concern also is that if we're not keeping the edits as source of truth for final state and modify them like that it would make generating migrations harder as we're losing information about the actual edit replacing it with the final state of the edit instead
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 behavior itself right now looks good to me, but let's have a ticket for later to look at the edit shape and logic again
Description
This is the last functional part (e2e tests and clean up to follow).
Open questions:
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes