Added destroyColumn migration step #1211
Conversation
|
Nice! Thank you for your contribution! Some quick thoughts:
|
There was a problem hiding this comment.
Okay, I reviewed it, and have a few requests:
- Please update docs-master/Advanced/Migrations.md
- add an integration test to adapters/tests/commonTEsts.js - you'll notice that there are some migration tests there that actually test if new columns/tables are usable after the addition. You could modify them or add a new test that tests if destroyed columns really were destroyed
- please add a note to changelog-unreleased
Otherwise, it all seems reasonable, thanks for your work!
src/Schema/migrations/test.js
Outdated
| @@ -181,6 +181,10 @@ describe('migration step functions', () => { | |||
| 'type', | |||
| ) | |||
| }) | |||
| it('throws if destroyColumn() is malformed', () => { | |||
There was a problem hiding this comment.
pls update one of the tests above to test for the format of what destroyColumn returns
| ) | ||
| case 'destroy_column': | ||
| return addedColumns.filter( | ||
| ({ table, name }) => step.table !== table || step.column !== name, |
There was a problem hiding this comment.
this doesn't seem correct to me, and there are no tests for this. can you explain?
There was a problem hiding this comment.
I had wrong understanding of getSyncChanges. I still have.
- If there is an addColumns for 'table' and 'column' in an earlier step, a destroyColumn in a later step should cancel it out. That's why I unnested them and kept them as a single array.
|
It looks like ALTER TABLE DROP COLUMN is not supported although the docs say it does. However, ALTER TABLE RENAME COLUMN is supported. I am stuck right now :) |
on which platform are you testing that? |
|
I have just executed |
|
oh no https://sqlite.org/changes.html this was only added in 3.35.0 :( |
|
you could go with something like this : https://stackoverflow.com/questions/5938048/delete-column-from-sqlite-table but you probably have to drop and recreate indices for it to work |
|
That's what I was afraid of. I would need to pass also the whole table description in order to do that. It makes the whole migration step really ugly. |
|
@bmatasar Unfortunately, it's complicated. On iOS and Android we're using the system built-in version of sqlite, so we can't rely on newer sqlite features. On Android JSI we're bundling sqlite 3.36 https://github.com/Nozbe/WatermelonDB/blob/master/package.json#L48 because it's impossible to reuse system version of sqlite. We're also using I would prefer not to bundle sqlite3 on iOS due to codesize/performance concerns |
|
The problem is that in order to apply the solution with the duplication of the table I need the whole schema for the table. The statements are generated just from the migrations steps, so it will look more like a |
|
yeah that's fair. I forgot about this, but this difficulty is why I never implemented dropping tables and columns in migrations - it shouldn't be very hard but it's not trivial |
|
Let's close the PR for now. |
|
I'm gonna re-open so the work done already doesn't get lost |
|
@bmatasar read about the magic sqlite tables, I think you can get info about indices with pure SQL |
|
I looked into |
|
Any plans to add this in? Seems like it would be a pretty useful migration step |
|
just passing by, hope you don't mind the notification now with ios 17 being out, I believe requiring ios 15 won't be an issue 👍 |
|
I would rather start a new PR based on #1654 |
I don't know how to add info about dropped columns in getSyncChanges