-
Notifications
You must be signed in to change notification settings - Fork 215
feat(dbml/core): adds support for exporting to sqlite #752
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: master
Are you sure you want to change the base?
Conversation
|
Just curious if I missed something in this PR? I'm happy to fix if something is missing. Can someone on this project provide some guidance? |
|
Hey @danielgary, sorry for the late reply. Thanks for the contribution! We'll definitely take a look at your PR when we have time. Thank you for your patience. |
|
Thank you for resolving all my comments, I'll continue reviewing when I have time! |
H-DNA
left a comment
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 think the implementation most of the part is good 💯 please help consider some suggestions and add more testcases. Then I'll perform one final round of review. Thank you!
| return indexArr; | ||
| } | ||
|
|
||
| static export(model) { |
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.
Since dbml v5.0.0, we have supported check constraints in model_structure. The model parameter now can contain information about check constraints.
You can look at the definition here for reference:
- Table: https://github.com/holistics/dbml/blob/master/packages/dbml-core/src/model_structure/table.js
- Field: https://github.com/holistics/dbml/blob/master/packages/dbml-core/src/model_structure/field.js
- Check: https://github.com/holistics/dbml/blob/master/packages/dbml-core/src/model_structure/check.js
Please help us exporting Check constraints as well!
| const allRefIds = _.flatten(database.schemaIds.map(sid => model.schemas[sid].refIds || [])); | ||
| const { fksByTableId, junctionCreates, dependencyEdges } = SqliteExporter.collectForeignKeysByTable(allRefIds, model); | ||
|
|
||
| const allTableIds = _.flatten(database.schemaIds.map(sid => model.schemas[sid].tableIds || [])); |
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.
Can you use flatMap instead of map + flatten?
| map.set(fq, vals); | ||
| map.set(local, vals); |
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 wonder if this approach would work if there are two enums in two different schemas but have the same name? For example:
Enum S1.E {
}
Enum S2.E {
}
By performing map.set(local, vals), E would be set two times and the latter would override.
|
|
||
| const enumMap = SqliteExporter.buildEnumMap(model); | ||
|
|
||
| const allRefIds = _.flatten(database.schemaIds.map(sid => model.schemas[sid].refIds || [])); |
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.
Can you use flatMap instead of map + flatten?
|
|
||
| const tableCreates = SqliteExporter.exportTables(orderedTableIds, model, enumMap, fksByTableId); | ||
|
|
||
| const allIndexIds = _.flatten( |
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.
Can you use flatMap instead of map + flatten?
| const allIndexIds = _.flatten( | ||
| (database.schemaIds || []).map(sid => { | ||
| const tIds = model.schemas[sid].tableIds || []; | ||
| return _.flatten(tIds.map(tid => model.tables[tid].indexIds || [])); |
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.
Can you use flatMap instead of map + flatten?
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 testcases are failing. Please help update the tests!
Also, can you verify that the following cases are covered in the test cases:
- Fields that have types in the form of
name(args)such asVARCHAR(255). - Various index types: unique, primary, functional, multi-column (unique and primary).
- Unique constraints.
- Default
- Autoincrement.
- Various referential actions.
- Primary constraints.
- Check constraints.
- Enums
- Multi-column, many-to-many, one-to-one, one-to-many, zero-to-one, zero-to-many Refs.
- Not null constraints
- Column types that have spaces in their names.
- Table and column names that contain spaces in them
You can look up the input for the test cases by reusing the other input files in the other exporters.
Summary
Implemented SQLite exporter with many-to-many relations and proper index exporting support. Included tests.
Issue
#14
#286
Lasting Changes (Technical)
Checklist
Please check directly on the box once each of these are done