From 5ee916630c0272655212a6709797923cfaec3f28 Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sat, 25 May 2024 14:48:09 -0700 Subject: [PATCH 01/25] add destroyColumn migration step --- CHANGELOG-Unreleased.md | 1 + src/RawRecord/index.d.ts | 3 +- src/RawRecord/index.js | 2 +- src/Schema/migrations/index.d.ts | 29 +++++++++++++-- src/Schema/migrations/index.js | 37 ++++++++++++++++++-- src/Schema/migrations/test.js | 36 +++++++++++++++++-- src/adapters/lokijs/worker/DatabaseDriver.js | 10 ++++++ src/adapters/sqlite/encodeSchema/index.js | 31 ++++++++++++++-- src/adapters/sqlite/index.js | 2 +- 9 files changed, 137 insertions(+), 14 deletions(-) diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index c00677656..5e485b29f 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -7,6 +7,7 @@ ### New features - Added `Database#experimentalIsVerbose` option +- Added destroyColumn migration step ### Fixes diff --git a/src/RawRecord/index.d.ts b/src/RawRecord/index.d.ts index 54ce69bad..83b15bee0 100644 --- a/src/RawRecord/index.d.ts +++ b/src/RawRecord/index.d.ts @@ -1,7 +1,6 @@ import type { ColumnName, ColumnSchema, TableSchema } from '../Schema' import type { RecordId, SyncStatus } from '../Model' - // Raw object representing a model record, coming from an untrusted source // (disk, sync, user data). Before it can be used to create a Model instance // it must be sanitized (with `sanitizedRaw`) into a RawRecord @@ -15,7 +14,7 @@ type _RawRecord = { } // Raw object representing a model record. A RawRecord is guaranteed by the type system -// to be safe to use (sanitied with `sanitizedRaw`): +// to be safe to use (sanitized with `sanitizedRaw`): // - it has exactly the fields described by TableSchema (+ standard fields) // - every field is exactly the type described by ColumnSchema (string, number, or boolean) // - … and the same optionality (will not be null unless isOptional: true) diff --git a/src/RawRecord/index.js b/src/RawRecord/index.js index fd35b4401..933d0d207 100644 --- a/src/RawRecord/index.js +++ b/src/RawRecord/index.js @@ -20,7 +20,7 @@ type _RawRecord = { } // Raw object representing a model record. A RawRecord is guaranteed by the type system -// to be safe to use (sanitied with `sanitizedRaw`): +// to be safe to use (sanitized with `sanitizedRaw`): // - it has exactly the fields described by TableSchema (+ standard fields) // - every field is exactly the type described by ColumnSchema (string, number, or boolean) // - … and the same optionality (will not be null unless isOptional: true) diff --git a/src/Schema/migrations/index.d.ts b/src/Schema/migrations/index.d.ts index 35f27f669..da99df8aa 100644 --- a/src/Schema/migrations/index.d.ts +++ b/src/Schema/migrations/index.d.ts @@ -1,5 +1,12 @@ import type { $RE, $Exact } from '../../types' -import type { ColumnSchema, TableName, TableSchema, TableSchemaSpec, SchemaVersion } from '../index' +import type { + ColumnName, + ColumnSchema, + TableName, + TableSchema, + TableSchemaSpec, + SchemaVersion, +} from '../index' export type CreateTableMigrationStep = $RE<{ type: 'create_table' @@ -13,12 +20,22 @@ export type AddColumnsMigrationStep = $RE<{ unsafeSql?: (_: string) => string }> +export type DestroyColumnMigrationStep = $RE<{ + type: 'destroy_column' + table: TableName + column: ColumnName +}> + export type SqlMigrationStep = $RE<{ type: 'sql' sql: string }> -export type MigrationStep = CreateTableMigrationStep | AddColumnsMigrationStep | SqlMigrationStep +export type MigrationStep = + | CreateTableMigrationStep + | AddColumnsMigrationStep + | SqlMigrationStep + | DestroyColumnMigrationStep type Migration = $RE<{ toVersion: SchemaVersion @@ -50,4 +67,12 @@ export function addColumns({ unsafeSql?: (_: string) => string }>): AddColumnsMigrationStep +export function destroyColumn({ + table, + column, +}: $Exact<{ + table: TableName + column: ColumnName +}>): DestroyColumnMigrationStep + export function unsafeExecuteSql(sql: string): SqlMigrationStep diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index d6b8cd8ac..4a6824081 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -6,7 +6,14 @@ import invariant from '../../utils/common/invariant' import isObj from '../../utils/fp/isObj' import type { $RE } from '../../types' -import type { ColumnSchema, TableName, TableSchema, TableSchemaSpec, SchemaVersion } from '../index' +import type { + columnName, + ColumnSchema, + TableName, + TableSchema, + TableSchemaSpec, + SchemaVersion, +} from '../index' import { tableSchema, validateColumnSchema } from '../index' export type CreateTableMigrationStep = $RE<{ @@ -21,12 +28,22 @@ export type AddColumnsMigrationStep = $RE<{ unsafeSql?: (string) => string, }> +export type DestroyColumnMigrationStep = $RE<{ + type: 'destroy_column', + table: TableName, + column: ColumnName, +}> + export type SqlMigrationStep = $RE<{ type: 'sql', sql: string, }> -export type MigrationStep = CreateTableMigrationStep | AddColumnsMigrationStep | SqlMigrationStep +export type MigrationStep = + | CreateTableMigrationStep + | AddColumnsMigrationStep + | SqlMigrationStep + | DestroyColumnMigrationStep type Migration = $RE<{ toVersion: SchemaVersion, @@ -159,6 +176,21 @@ export function addColumns({ return { type: 'add_columns', table, columns, unsafeSql } } +export function destroyColumn({ + table, + column, +}: $Exact<{ + table: TableName, + column: ColumnName, +}>): DestroyColumnMigrationStep { + if (process.env.NODE_ENV !== 'production') { + invariant(table, `Missing 'table' in destroyColumn()`) + invariant(column, `Missing 'column' in destroyColumn()`) + } + + return { type: 'destroy_column', table, column } +} + export function unsafeExecuteSql(sql: string): SqlMigrationStep { if (process.env.NODE_ENV !== 'production') { invariant(typeof sql === 'string', `SQL passed to unsafeExecuteSql is not a string`) @@ -176,7 +208,6 @@ renameTable({ from: 'old_table_name', to: 'new_table_name' }) // column operations renameColumn({ table: 'table_name', from: 'old_column_name', to: 'new_column_name' }) -destroyColumn({ table: 'table_name', column: 'column_name' }) // indexing addColumnIndex({ table: 'table_name', column: 'column_name' }) diff --git a/src/Schema/migrations/test.js b/src/Schema/migrations/test.js index b47d24b65..924eb88a6 100644 --- a/src/Schema/migrations/test.js +++ b/src/Schema/migrations/test.js @@ -1,4 +1,4 @@ -import { createTable, addColumns, schemaMigrations } from './index' +import { createTable, addColumns, destroyColumn, schemaMigrations } from './index' import { stepsForMigration } from './stepsForMigration' describe('schemaMigrations()', () => { @@ -30,7 +30,19 @@ describe('schemaMigrations()', () => { it('returns a complex schema migrations spec', () => { const migrations = schemaMigrations({ migrations: [ - { toVersion: 4, steps: [] }, + { + toVersion: 4, + steps: [ + addColumns({ + table: 'comments', + columns: [{ name: 'text', type: 'string' }], + }), + destroyColumn({ + table: 'comments', + column: 'body', + }), + ], + }, { toVersion: 3, steps: [ @@ -103,7 +115,21 @@ describe('schemaMigrations()', () => { }, ], }, - { toVersion: 4, steps: [] }, + { + toVersion: 4, + steps: [ + { + type: 'add_columns', + table: 'comments', + columns: [{ name: 'text', type: 'string' }], + }, + { + type: 'destroy_column', + table: 'comments', + column: 'body', + }, + ], + }, ], }) }) @@ -181,6 +207,10 @@ describe('migration step functions', () => { 'type', ) }) + it('throws if destroyColumn() is malformed', () => { + expect(() => destroyColumn({ column: 'foo' })).toThrow('table') + expect(() => destroyColumn({ table: 'foo' })).toThrow('column') + }) }) describe('stepsForMigration', () => { diff --git a/src/adapters/lokijs/worker/DatabaseDriver.js b/src/adapters/lokijs/worker/DatabaseDriver.js index f9731e575..9c3150a4f 100644 --- a/src/adapters/lokijs/worker/DatabaseDriver.js +++ b/src/adapters/lokijs/worker/DatabaseDriver.js @@ -390,6 +390,8 @@ export default class DatabaseDriver { this._executeCreateTableMigration(step) } else if (step.type === 'add_columns') { this._executeAddColumnsMigration(step) + } else if (step.type === 'destroy_column') { + this._executeDestroyColumnMigration(step) } else if (step.type === 'sql') { // ignore } else { @@ -424,6 +426,14 @@ export default class DatabaseDriver { } }) } + _executeDestroyColumnMigration({ table, column }: DestroyColumnMigrationStep): void { + const collection = this.loki.getCollection(table) + + // update ALL records in the collection, removing a field + collection.findAndUpdate({}, (record) => { + delete record[column] + }) + } // Maps records to their IDs if the record is already cached on JS side _compactQueryResults(records: DirtyRaw[], table: TableName): CachedQueryResult { diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index dec177712..9dbfd65a8 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -2,7 +2,11 @@ import type { TableSchema, AppSchema, ColumnSchema, TableName } from '../../../Schema' import { nullValue } from '../../../RawRecord' -import type { MigrationStep, AddColumnsMigrationStep } from '../../../Schema/migrations' +import type { + MigrationStep, + AddColumnsMigrationStep, + DestroyColumnMigrationStep, +} from '../../../Schema/migrations' import type { SQL } from '../index' import encodeValue from '../encodeValue' @@ -85,13 +89,36 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ }) .join('') -export const encodeMigrationSteps: (MigrationStep[]) => SQL = (steps) => +const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ( + { table, column }, + tableSchema, +) => { + const newTempTable = { ...tableSchema, name: `${table}Temp` } + const newColumns = [ + ...standardColumns, + ...Object.keys(tableSchema.columns) + .filter((c) => c !== column) + .map((c) => `"${c}`), + ] + return ` + ${encodeTable(newTempTable)} + INSERT INTO ${table}Temp(${newColumns.join(',')}) SELECT ${newColumns.join( + ',', + )} FROM ${table}; + DROP TABLE ${table}; + ALTER TABLE ${table}Temp RENAME TO ${table}; + ` +} + +export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, schema) => steps .map((step) => { if (step.type === 'create_table') { return encodeTable(step.schema) } else if (step.type === 'add_columns') { return encodeAddColumnsMigrationStep(step) + } else if (step.type === 'destroy_column') { + return encodeDestroyColumnMigrationStep(step, schema.tables[step.table]) } else if (step.type === 'sql') { return step.sql } diff --git a/src/adapters/sqlite/index.js b/src/adapters/sqlite/index.js index 72a5af9f5..d28da976e 100644 --- a/src/adapters/sqlite/index.js +++ b/src/adapters/sqlite/index.js @@ -169,7 +169,7 @@ export default class SQLiteAdapter implements DatabaseAdapter { 'setUpWithMigrations', [ this.dbName, - require('./encodeSchema').encodeMigrationSteps(migrationSteps), + require('./encodeSchema').encodeMigrationSteps(migrationSteps, this.schema), databaseVersion, this.schema.version, ], From c6883c4d0e96636280c85381202166a5ecbfeb48 Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sat, 25 May 2024 15:51:27 -0700 Subject: [PATCH 02/25] add renameColumn migration step --- CHANGELOG-Unreleased.md | 1 + src/Schema/migrations/getSyncChanges/index.js | 22 +++++- src/Schema/migrations/getSyncChanges/test.js | 68 +++++++++++++++---- src/Schema/migrations/index.d.ts | 18 +++++ src/Schema/migrations/index.js | 29 ++++++-- src/Schema/migrations/test.js | 24 ++++++- src/adapters/sqlite/encodeSchema/index.js | 31 +++++++-- .../__tests__/synchronize-migration.test.js | 5 +- 8 files changed, 172 insertions(+), 26 deletions(-) diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 5e485b29f..0aec181fc 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -8,6 +8,7 @@ - Added `Database#experimentalIsVerbose` option - Added destroyColumn migration step +- Added renameColumn migration step ### Fixes diff --git a/src/Schema/migrations/getSyncChanges/index.js b/src/Schema/migrations/getSyncChanges/index.js index 91ed64a5a..0c6e74321 100644 --- a/src/Schema/migrations/getSyncChanges/index.js +++ b/src/Schema/migrations/getSyncChanges/index.js @@ -15,6 +15,13 @@ export type MigrationSyncChanges = $Exact<{ table: TableName, columns: ColumnName[], }>[], + +renamedColumns: $Exact<{ + table: TableName, + columns: $Exact<{ + from: ColumnName, + to: ColumnName, + }>[], + }>[], }> | null export default function getSyncChanges( @@ -34,7 +41,7 @@ export default function getSyncChanges( steps.forEach((step) => { invariant( - ['create_table', 'add_columns', 'sql'].includes(step.type), + ['create_table', 'add_columns', 'sql', 'rename_column'].includes(step.type), `Unknown migration step type ${step.type}. Can not perform migration sync. This most likely means your migrations are defined incorrectly. It could also be a WatermelonDB bug.`, ) }) @@ -63,9 +70,22 @@ export default function getSyncChanges( columns: unique(columnDefs.map(({ name }) => name)), })) + const allRenamedColumns = steps.filter( + (step) => step.type === 'rename_column' && !createdTables.includes(step.table), + ) + + const renamedColumns = pipe( + groupBy(({ table }) => table), + toPairs, + )(allRenamedColumns).map(([table, columns]) => ({ + table: tableName(table), + columns: columns.map(({ from, to }) => ({ from, to })), + })) + return { from: fromVersion, tables: unique(createdTables), columns: addedColumns, + renamedColumns, } } diff --git a/src/Schema/migrations/getSyncChanges/test.js b/src/Schema/migrations/getSyncChanges/test.js index ce0e87da2..61c353ec0 100644 --- a/src/Schema/migrations/getSyncChanges/test.js +++ b/src/Schema/migrations/getSyncChanges/test.js @@ -1,5 +1,5 @@ import getSyncChanges from './index' -import { schemaMigrations, createTable, addColumns, unsafeExecuteSql } from '../index' +import { schemaMigrations, createTable, addColumns, unsafeExecuteSql, renameColumn } from '../index' const createCommentsTable = createTable({ name: 'comments', @@ -8,6 +8,13 @@ const createCommentsTable = createTable({ { name: 'body', type: 'string' }, ], }) +const addColumnsToPostsTable = addColumns({ + table: 'posts', + columns: [ + { name: 'subtitle', type: 'string', isOptional: true }, + { name: 'is_pinned', type: 'boolean' }, + ], +}) const test = (migrations, from, to) => getSyncChanges(schemaMigrations({ migrations }), from, to) const testSteps = (steps) => @@ -18,26 +25,51 @@ describe('getSyncChanges', () => { expect(test([{ toVersion: 2, steps: [createCommentsTable] }], 2, 2)).toEqual(null) }) it('returns empty changes for empty steps', () => { - expect(testSteps([])).toEqual({ from: 1, tables: [], columns: [] }) + expect(testSteps([])).toEqual({ from: 1, tables: [], columns: [], renamedColumns: [] }) }) it('returns created tables', () => { - expect(testSteps([createCommentsTable])).toEqual({ from: 1, tables: ['comments'], columns: [] }) + expect(testSteps([createCommentsTable])).toEqual({ + from: 1, + tables: ['comments'], + columns: [], + renamedColumns: [], + }) }) it('returns added columns', () => { + expect(testSteps([addColumnsToPostsTable])).toEqual({ + from: 1, + tables: [], + columns: [{ table: 'posts', columns: ['subtitle', 'is_pinned'] }], + renamedColumns: [], + }) + }) + it('returns renamed columns', () => { + expect(testSteps([renameColumn({ table: 'posts', from: 'body', to: 'text' })])).toEqual({ + from: 1, + tables: [], + columns: [], + renamedColumns: [{ table: 'posts', columns: [{ from: 'body', to: 'text' }] }], + }) + }) + it('combines renamed columns from multiple migration steps', () => { expect( testSteps([ - addColumns({ - table: 'posts', - columns: [ - { name: 'subtitle', type: 'string', isOptional: true }, - { name: 'is_pinned', type: 'boolean' }, - ], - }), + renameColumn({ table: 'posts', from: 'body', to: 'text' }), + renameColumn({ table: 'posts', from: 'favorite', to: 'saved' }), ]), ).toEqual({ from: 1, tables: [], - columns: [{ table: 'posts', columns: ['subtitle', 'is_pinned'] }], + columns: [], + renamedColumns: [ + { + table: 'posts', + columns: [ + { from: 'body', to: 'text' }, + { from: 'favorite', to: 'saved' }, + ], + }, + ], }) }) it('combines added columns from multiple migration steps', () => { @@ -60,6 +92,7 @@ describe('getSyncChanges', () => { from: 1, tables: [], columns: [{ table: 'posts', columns: ['subtitle', 'is_pinned', 'author_id'] }], + renamedColumns: [], }) }) it('skips added columns for a table if it is also added', () => { @@ -75,6 +108,7 @@ describe('getSyncChanges', () => { from: 1, tables: ['comments'], columns: [], + renamedColumns: [], }) }) it('skips duplicates', () => { @@ -97,6 +131,7 @@ describe('getSyncChanges', () => { from: 1, tables: ['comments'], columns: [{ table: 'posts', columns: ['subtitle'] }], + renamedColumns: [], }) }) const bigMigrations = [ @@ -198,6 +233,7 @@ describe('getSyncChanges', () => { { table: 'comments', columns: ['is_pinned', 'extra'] }, { table: 'workspaces', columns: ['plan_info', 'limits'] }, ], + renamedColumns: [], }) }) it(`returns only the necessary range of migrations`, () => { @@ -208,13 +244,20 @@ describe('getSyncChanges', () => { { table: 'workspaces', columns: ['plan_info', 'limits'] }, { table: 'attachment_versions', columns: ['reactions'] }, ], + renamedColumns: [], }) expect(test(bigMigrations, 8, 10)).toEqual({ from: 8, tables: [], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], + renamedColumns: [], + }) + expect(test(bigMigrations, 9, 10)).toEqual({ + from: 9, + tables: [], + columns: [], + renamedColumns: [], }) - expect(test(bigMigrations, 9, 10)).toEqual({ from: 9, tables: [], columns: [] }) expect(test(bigMigrations, 10, 10)).toEqual(null) }) it(`fails on incorrect migrations`, () => { @@ -225,7 +268,6 @@ describe('getSyncChanges', () => { const possibleFutureTypes = [ 'broken', 'rename_table', - 'rename_column', 'add_column_index', 'make_column_optional', 'make_column_required', diff --git a/src/Schema/migrations/index.d.ts b/src/Schema/migrations/index.d.ts index da99df8aa..593da059c 100644 --- a/src/Schema/migrations/index.d.ts +++ b/src/Schema/migrations/index.d.ts @@ -26,6 +26,13 @@ export type DestroyColumnMigrationStep = $RE<{ column: ColumnName }> +export type RenameColumnMigrationStep = $RE<{ + type: 'rename_column' + table: TableName + from: ColumnName + to: ColumnName +}> + export type SqlMigrationStep = $RE<{ type: 'sql' sql: string @@ -36,6 +43,7 @@ export type MigrationStep = | AddColumnsMigrationStep | SqlMigrationStep | DestroyColumnMigrationStep + | RenameColumnMigrationStep type Migration = $RE<{ toVersion: SchemaVersion @@ -75,4 +83,14 @@ export function destroyColumn({ column: ColumnName }>): DestroyColumnMigrationStep +export function renameColumn({ + table, + from, + to, +}: $Exact<{ + table: TableName + from: string + to: string +}>): RenameColumnMigrationStep + export function unsafeExecuteSql(sql: string): SqlMigrationStep diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 4a6824081..52a51e090 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -7,7 +7,7 @@ import isObj from '../../utils/fp/isObj' import type { $RE } from '../../types' import type { - columnName, + ColumnName, ColumnSchema, TableName, TableSchema, @@ -34,6 +34,13 @@ export type DestroyColumnMigrationStep = $RE<{ column: ColumnName, }> +export type RenameColumnMigrationStep = $RE<{ + type: 'rename_column', + table: TableName, + from: ColumnName, + to: ColumnName, +}> + export type SqlMigrationStep = $RE<{ type: 'sql', sql: string, @@ -44,6 +51,7 @@ export type MigrationStep = | AddColumnsMigrationStep | SqlMigrationStep | DestroyColumnMigrationStep + | RenameColumnMigrationStep type Migration = $RE<{ toVersion: SchemaVersion, @@ -198,6 +206,22 @@ export function unsafeExecuteSql(sql: string): SqlMigrationStep { return { type: 'sql', sql } } +export function renameColumn({ + table, + from, + to, +}: $Exact<{ + table: TableName, + from: ColumnName, + to: ColumnName, +}>): RenameColumnMigrationStep { + if (process.env.NODE_ENV !== 'production') { + invariant(table, `Missing table name in renameColumn()`) + invariant(from, `Missing 'from' in renameColumn()`) + invariant(to, `Missing 'to' in renameColumn()`) + } + return { type: 'rename_column', table, from, to } +} /* TODO: Those types of migrations are currently not implemented. If you need them, feel free to contribute! @@ -206,9 +230,6 @@ TODO: Those types of migrations are currently not implemented. If you need them, destroyTable('table_name') renameTable({ from: 'old_table_name', to: 'new_table_name' }) -// column operations -renameColumn({ table: 'table_name', from: 'old_column_name', to: 'new_column_name' }) - // indexing addColumnIndex({ table: 'table_name', column: 'column_name' }) removeColumnIndex({ table: 'table_name', column: 'column_name' }) diff --git a/src/Schema/migrations/test.js b/src/Schema/migrations/test.js index 924eb88a6..c2533358d 100644 --- a/src/Schema/migrations/test.js +++ b/src/Schema/migrations/test.js @@ -1,4 +1,4 @@ -import { createTable, addColumns, destroyColumn, schemaMigrations } from './index' +import { createTable, addColumns, destroyColumn, renameColumn, schemaMigrations } from './index' import { stepsForMigration } from './stepsForMigration' describe('schemaMigrations()', () => { @@ -30,6 +30,10 @@ describe('schemaMigrations()', () => { it('returns a complex schema migrations spec', () => { const migrations = schemaMigrations({ migrations: [ + { + toVersion: 5, + steps: [renameColumn({ table: 'comments', from: 'text', to: 'body' })], + }, { toVersion: 4, steps: [ @@ -76,7 +80,7 @@ describe('schemaMigrations()', () => { expect(migrations).toEqual({ validated: true, minVersion: 1, - maxVersion: 4, + maxVersion: 5, sortedMigrations: [ { toVersion: 2, @@ -130,6 +134,17 @@ describe('schemaMigrations()', () => { }, ], }, + { + toVersion: 5, + steps: [ + { + type: 'rename_column', + table: 'comments', + from: 'text', + to: 'body', + }, + ], + }, ], }) }) @@ -211,6 +226,11 @@ describe('migration step functions', () => { expect(() => destroyColumn({ column: 'foo' })).toThrow('table') expect(() => destroyColumn({ table: 'foo' })).toThrow('column') }) + it('throws if renameColumn() is malformed', () => { + expect(() => renameColumn({ from: 'text', to: 'body' })).toThrow('table') + expect(() => renameColumn({ table: 'foo', from: 'text' })).toThrow('to') + expect(() => renameColumn({ table: 'foo', to: 'body' })).toThrow('from') + }) }) describe('stepsForMigration', () => { diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 9dbfd65a8..07ad537c7 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -6,18 +6,22 @@ import type { MigrationStep, AddColumnsMigrationStep, DestroyColumnMigrationStep, + RenameColumnMigrationStep, } from '../../../Schema/migrations' import type { SQL } from '../index' import encodeValue from '../encodeValue' -const standardColumns = `"id" primary key, "_changed", "_status"` +const standardColumns = ['id', '_changed', '_status'] +const standardColumnsSQL = standardColumns + .map((column) => (column === 'id' ? `"${column}" primary key` : `"${column}"`)) + .join(', ') const commonSchema = 'create table "local_storage" ("key" varchar(16) primary key not null, "value" text not null);' + 'create index "local_storage_key_index" on "local_storage" ("key");' const encodeCreateTable = ({ name, columns }: TableSchema): SQL => { - const columnsSQL = [standardColumns] + const columnsSQL = [standardColumnsSQL] .concat(Object.keys(columns).map((column) => `"${column}"`)) .join(', ') return `create table "${name}" (${columnsSQL});` @@ -96,10 +100,9 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema const newTempTable = { ...tableSchema, name: `${table}Temp` } const newColumns = [ ...standardColumns, - ...Object.keys(tableSchema.columns) - .filter((c) => c !== column) - .map((c) => `"${c}`), + ...Object.keys(tableSchema.columns).filter((c) => c !== column), ] + return ` ${encodeTable(newTempTable)} INSERT INTO ${table}Temp(${newColumns.join(',')}) SELECT ${newColumns.join( @@ -109,6 +112,22 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema ALTER TABLE ${table}Temp RENAME TO ${table}; ` } +const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ( + { table, from, to }, + tableSchema, +) => { + const newTempTable = { ...tableSchema, name: `${table}Temp` } + const newColumns = [...standardColumns, ...Object.keys(tableSchema.columns)] + const prevColumns = newColumns.map((c) => (c === to ? from : c)) + return ` + ${encodeTable(newTempTable)} + INSERT INTO ${table}Temp(${newColumns.join(',')}) SELECT ${prevColumns.join( + ',', + )} FROM ${table}; + DROP TABLE ${table}; + ALTER TABLE ${table}Temp RENAME TO ${table}; + ` +} export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, schema) => steps @@ -119,6 +138,8 @@ export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, return encodeAddColumnsMigrationStep(step) } else if (step.type === 'destroy_column') { return encodeDestroyColumnMigrationStep(step, schema.tables[step.table]) + } else if (step.type === 'rename_column') { + return encodeRenameColumnMigrationStep(step, schema.tables[step.table]) } else if (step.type === 'sql') { return step.sql } diff --git a/src/sync/impl/__tests__/synchronize-migration.test.js b/src/sync/impl/__tests__/synchronize-migration.test.js index a29fd8f61..05d7f7098 100644 --- a/src/sync/impl/__tests__/synchronize-migration.test.js +++ b/src/sync/impl/__tests__/synchronize-migration.test.js @@ -1,6 +1,6 @@ import { mockDatabase, testSchema } from '../../../__tests__/testModels' import { expectToRejectWithMessage } from '../../../__tests__/utils' -import { schemaMigrations, createTable, addColumns } from '../../../Schema/migrations' +import { schemaMigrations, createTable, addColumns, renameColumn } from '../../../Schema/migrations' import { emptyPull } from './helpers' import { synchronize } from '../../index' @@ -17,6 +17,7 @@ describe('synchronize - migration syncs', () => { table: 'attachment_versions', columns: [{ name: 'reactions', type: 'string' }], }), + renameColumn({ table: 'post', from: 'body', to: 'text' }), ], }, { @@ -113,6 +114,7 @@ describe('synchronize - migration syncs', () => { from: 9, tables: [], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], + renamedColumns: [{ table: 'post', columns: [{ from: 'body', to: 'text' }] }], }, }) expect(await getLastPulledSchemaVersion(database)).toBe(10) @@ -135,6 +137,7 @@ describe('synchronize - migration syncs', () => { from: 8, tables: ['attachments'], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], + renamedColumns: [{ table: 'post', columns: [{ from: 'body', to: 'text' }] }], }, }) expect(await getLastPulledSchemaVersion(database)).toBe(10) From 0674a7be59b7321b923750273957a77a11d6c20a Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sun, 26 May 2024 12:20:46 -0700 Subject: [PATCH 03/25] add loki rename_column to loki adapter --- src/adapters/lokijs/worker/DatabaseDriver.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/adapters/lokijs/worker/DatabaseDriver.js b/src/adapters/lokijs/worker/DatabaseDriver.js index 9c3150a4f..4eecb8626 100644 --- a/src/adapters/lokijs/worker/DatabaseDriver.js +++ b/src/adapters/lokijs/worker/DatabaseDriver.js @@ -15,6 +15,8 @@ import type { SchemaMigrations, CreateTableMigrationStep, AddColumnsMigrationStep, + DestroyColumnMigrationStep, + RenameColumnMigrationStep, MigrationStep, } from '../../../Schema/migrations' import type { SerializedQuery } from '../../../Query' @@ -392,6 +394,8 @@ export default class DatabaseDriver { this._executeAddColumnsMigration(step) } else if (step.type === 'destroy_column') { this._executeDestroyColumnMigration(step) + } else if (step.type === 'rename_column') { + this._executeRenameColumnMigration(step) } else if (step.type === 'sql') { // ignore } else { @@ -435,6 +439,15 @@ export default class DatabaseDriver { }) } + _executeRenameColumnMigration({ table, from, to }: RenameColumnMigrationStep): void { + const collection = this.loki.getCollection(table) + collection.findAndUpdate({}, (record) => { + if (record[from] !== undefined) { + record[to] = record[from] + } + delete record[from] + }) + } // Maps records to their IDs if the record is already cached on JS side _compactQueryResults(records: DirtyRaw[], table: TableName): CachedQueryResult { const cache = this.getCache(table) From 0cb7980c96faa1c8cfaafc2a04e23425442f9762 Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sun, 26 May 2024 14:19:05 -0700 Subject: [PATCH 04/25] add destroy table migration step --- CHANGELOG-Unreleased.md | 1 + src/Schema/migrations/index.d.ts | 12 ++++ src/Schema/migrations/index.js | 33 +++++++--- src/Schema/migrations/test.js | 24 +++++++- src/adapters/lokijs/worker/DatabaseDriver.js | 10 ++++ src/adapters/sqlite/encodeSchema/index.js | 42 +++++++------ src/adapters/sqlite/encodeSchema/test.js | 63 +++++++++++++++++++- 7 files changed, 155 insertions(+), 30 deletions(-) diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 0aec181fc..e44299e8f 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -9,6 +9,7 @@ - Added `Database#experimentalIsVerbose` option - Added destroyColumn migration step - Added renameColumn migration step +- Added destroyTable migration step ### Fixes diff --git a/src/Schema/migrations/index.d.ts b/src/Schema/migrations/index.d.ts index 593da059c..28251560e 100644 --- a/src/Schema/migrations/index.d.ts +++ b/src/Schema/migrations/index.d.ts @@ -33,6 +33,11 @@ export type RenameColumnMigrationStep = $RE<{ to: ColumnName }> +export type DestroyTableMigrationStep = $RE<{ + type: 'destroy_table' + table: TableName +}> + export type SqlMigrationStep = $RE<{ type: 'sql' sql: string @@ -44,6 +49,7 @@ export type MigrationStep = | SqlMigrationStep | DestroyColumnMigrationStep | RenameColumnMigrationStep + | DestroyTableMigrationStep type Migration = $RE<{ toVersion: SchemaVersion @@ -93,4 +99,10 @@ export function renameColumn({ to: string }>): RenameColumnMigrationStep +export function destroyTable({ + table, +}: $Exact<{ + table: TableName +}>): DestroyTableMigrationStep + export function unsafeExecuteSql(sql: string): SqlMigrationStep diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 52a51e090..7795119fb 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -41,6 +41,11 @@ export type RenameColumnMigrationStep = $RE<{ to: ColumnName, }> +export type DestroyTableMigrationStep = $RE<{ + type: 'destroy_table', + table: TableName, +}> + export type SqlMigrationStep = $RE<{ type: 'sql', sql: string, @@ -199,13 +204,6 @@ export function destroyColumn({ return { type: 'destroy_column', table, column } } -export function unsafeExecuteSql(sql: string): SqlMigrationStep { - if (process.env.NODE_ENV !== 'production') { - invariant(typeof sql === 'string', `SQL passed to unsafeExecuteSql is not a string`) - } - return { type: 'sql', sql } -} - export function renameColumn({ table, from, @@ -222,12 +220,31 @@ export function renameColumn({ } return { type: 'rename_column', table, from, to } } + +export function destroyTable({ + table, +}: $Exact<{ + table: TableName, +}>): DestroyTableMigrationStep { + if (process.env.NODE_ENV !== 'production') { + invariant(table, `Missing 'table' in destroyTable()`) + } + + return { type: 'destroy_table', table } +} + +export function unsafeExecuteSql(sql: string): SqlMigrationStep { + if (process.env.NODE_ENV !== 'production') { + invariant(typeof sql === 'string', `SQL passed to unsafeExecuteSql is not a string`) + } + return { type: 'sql', sql } +} + /* TODO: Those types of migrations are currently not implemented. If you need them, feel free to contribute! // table operations -destroyTable('table_name') renameTable({ from: 'old_table_name', to: 'new_table_name' }) // indexing diff --git a/src/Schema/migrations/test.js b/src/Schema/migrations/test.js index c2533358d..d3eaf546a 100644 --- a/src/Schema/migrations/test.js +++ b/src/Schema/migrations/test.js @@ -1,4 +1,11 @@ -import { createTable, addColumns, destroyColumn, renameColumn, schemaMigrations } from './index' +import { + createTable, + addColumns, + destroyColumn, + renameColumn, + schemaMigrations, + destroyTable, +} from './index' import { stepsForMigration } from './stepsForMigration' describe('schemaMigrations()', () => { @@ -30,6 +37,7 @@ describe('schemaMigrations()', () => { it('returns a complex schema migrations spec', () => { const migrations = schemaMigrations({ migrations: [ + { toVersion: 6, steps: [destroyTable({ table: 'comments' })] }, { toVersion: 5, steps: [renameColumn({ table: 'comments', from: 'text', to: 'body' })], @@ -80,7 +88,7 @@ describe('schemaMigrations()', () => { expect(migrations).toEqual({ validated: true, minVersion: 1, - maxVersion: 5, + maxVersion: 6, sortedMigrations: [ { toVersion: 2, @@ -145,6 +153,15 @@ describe('schemaMigrations()', () => { }, ], }, + { + toVersion: 6, + steps: [ + { + type: 'destroy_table', + table: 'comments', + }, + ], + }, ], }) }) @@ -231,6 +248,9 @@ describe('migration step functions', () => { expect(() => renameColumn({ table: 'foo', from: 'text' })).toThrow('to') expect(() => renameColumn({ table: 'foo', to: 'body' })).toThrow('from') }) + it('throws if destroyTable() is malformed', () => { + expect(() => destroyTable()).toThrow('table') + }) }) describe('stepsForMigration', () => { diff --git a/src/adapters/lokijs/worker/DatabaseDriver.js b/src/adapters/lokijs/worker/DatabaseDriver.js index 4eecb8626..e75cc3ef0 100644 --- a/src/adapters/lokijs/worker/DatabaseDriver.js +++ b/src/adapters/lokijs/worker/DatabaseDriver.js @@ -17,6 +17,7 @@ import type { AddColumnsMigrationStep, DestroyColumnMigrationStep, RenameColumnMigrationStep, + DestroyTableMigrationStep, MigrationStep, } from '../../../Schema/migrations' import type { SerializedQuery } from '../../../Query' @@ -396,6 +397,8 @@ export default class DatabaseDriver { this._executeDestroyColumnMigration(step) } else if (step.type === 'rename_column') { this._executeRenameColumnMigration(step) + } else if (step.type === 'destroy_table') { + this._executeDestroyTableMigration(step) } else if (step.type === 'sql') { // ignore } else { @@ -448,6 +451,13 @@ export default class DatabaseDriver { delete record[from] }) } + _executeDestroyTableMigration({ table }: DestroyTableMigrationStep): void { + const collection = this.loki.getCollection(table) + if (collection) { + this.loki.removeCollection(table) + } + } + // Maps records to their IDs if the record is already cached on JS side _compactQueryResults(records: DirtyRaw[], table: TableName): CachedQueryResult { const cache = this.getCache(table) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 07ad537c7..5015cf6f6 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -7,6 +7,7 @@ import type { AddColumnsMigrationStep, DestroyColumnMigrationStep, RenameColumnMigrationStep, + DestroyTableMigrationStep, } from '../../../Schema/migrations' import type { SQL } from '../index' @@ -102,31 +103,36 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema ...standardColumns, ...Object.keys(tableSchema.columns).filter((c) => c !== column), ] - - return ` - ${encodeTable(newTempTable)} - INSERT INTO ${table}Temp(${newColumns.join(',')}) SELECT ${newColumns.join( + const createTempTableSQL = `${encodeTable(newTempTable)}` + const injectValuesSQL = `INSERT INTO ${table}Temp(${newColumns.join( ',', - )} FROM ${table}; - DROP TABLE ${table}; - ALTER TABLE ${table}Temp RENAME TO ${table}; - ` + )}) SELECT ${newColumns.join(',')} FROM ${table};` + const dropOldTable = `DROP TABLE ${table};` + const renameTempTableSQL = `ALTER TABLE ${table}Temp RENAME TO ${table};` + return `${createTempTableSQL}${injectValuesSQL}${dropOldTable}${renameTempTableSQL}` } const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ( { table, from, to }, tableSchema, ) => { const newTempTable = { ...tableSchema, name: `${table}Temp` } - const newColumns = [...standardColumns, ...Object.keys(tableSchema.columns)] - const prevColumns = newColumns.map((c) => (c === to ? from : c)) - return ` - ${encodeTable(newTempTable)} - INSERT INTO ${table}Temp(${newColumns.join(',')}) SELECT ${prevColumns.join( + const newColumnNames = [...standardColumns, ...Object.keys(tableSchema.columns)] + const oldColumnNames = newColumnNames.map((c) => (c === to ? from : c)) + + const createTempTableSQL = `${encodeTable(newTempTable)}` + const injectValuesSQL = `INSERT INTO ${table}Temp(${newColumnNames.join( ',', - )} FROM ${table}; - DROP TABLE ${table}; - ALTER TABLE ${table}Temp RENAME TO ${table}; - ` + )}) SELECT ${oldColumnNames.join(',')} FROM ${table};` + + const dropOldTableSQL = `DROP TABLE ${table};` + const renameTempTableSQL = `ALTER TABLE ${table}Temp RENAME TO ${table};` + return `${createTempTableSQL}${injectValuesSQL}${dropOldTableSQL}${renameTempTableSQL}` +} + +const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ table }) => { + return ` + DROP TABLE IF EXISTS ${table}; + ` } export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, schema) => @@ -140,6 +146,8 @@ export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, return encodeDestroyColumnMigrationStep(step, schema.tables[step.table]) } else if (step.type === 'rename_column') { return encodeRenameColumnMigrationStep(step, schema.tables[step.table]) + } else if (step.type === 'destroy_table') { + return encodeDestroyTableMigrationStep(step) } else if (step.type === 'sql') { return step.sql } diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 7fb5b618d..2d53ae4f4 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -1,6 +1,12 @@ /* eslint-disable prefer-template */ import { appSchema, tableSchema } from '../../../Schema' -import { addColumns, createTable, unsafeExecuteSql } from '../../../Schema/migrations' +import { + addColumns, + createTable, + destroyColumn, + renameColumn, + unsafeExecuteSql, +} from '../../../Schema/migrations' import { encodeSchema, encodeMigrationSteps, encodeCreateIndices, encodeDropIndices } from './index' @@ -129,6 +135,34 @@ describe('encodeIndices', () => { }) describe('encodeMigrationSteps', () => { + const migrationSchema = appSchema({ + version: 5, + tables: [ + tableSchema({ + name: 'posts', + columns: [ + { name: 'reactions', type: 'number' }, + { name: 'author_id', type: 'string', isIndexed: true }, + { name: 'is_pinned', type: 'boolean', isIndexed: true }, + { name: 'subtitle', type: 'string', isOptional: true }, + ], + }), + tableSchema({ + name: 'comments', + columns: [ + { name: 'post_id', type: 'string', isIndexed: true }, + { name: 'description', type: 'string' }, + ], + }), + tableSchema({ + name: 'authors', + columns: [ + { name: 'created_at', type: 'number' }, + { name: 'updated_at', type: 'number' }, + ], + }), + ], + }) it('encodes migrations', () => { const migrationSteps = [ addColumns({ @@ -149,9 +183,18 @@ describe('encodeMigrationSteps', () => { { name: 'is_pinned', type: 'boolean', isIndexed: true }, ], }), + destroyColumn({ + table: 'posts', + column: 'subtitle', + }), + renameColumn({ + table: 'comments', + from: 'body', + to: 'description', + }), ] - expect(encodeMigrationSteps(migrationSteps)).toBe( + expect(encodeMigrationSteps(migrationSteps, migrationSchema)).toBe( '' + `alter table "posts" add "subtitle";` + `update "posts" set "subtitle" = null;` + @@ -163,7 +206,21 @@ describe('encodeMigrationSteps', () => { `create index if not exists "posts_author_id" on "posts" ("author_id");` + `alter table "posts" add "is_pinned";` + `update "posts" set "is_pinned" = 0;` + - `create index if not exists "posts_is_pinned" on "posts" ("is_pinned");`, + `create index if not exists "posts_is_pinned" on "posts" ("is_pinned");` + + // destroy column + `create table "postsTemp" ("id" primary key, "_changed", "_status", "reactions", "author_id", "is_pinned", "subtitle");` + + `create index if not exists "postsTemp_author_id" on "postsTemp" ("author_id");` + + `create index if not exists "postsTemp_is_pinned" on "postsTemp" ("is_pinned");` + + `create index if not exists "postsTemp__status" on "postsTemp" ("_status");` + + `INSERT INTO postsTemp(id,_changed,_status,reactions,author_id,is_pinned) SELECT id,_changed,_status,reactions,author_id,is_pinned FROM posts;` + + `DROP TABLE posts;ALTER TABLE postsTemp RENAME TO posts;` + + // rename column + `create table "commentsTemp" ("id" primary key, "_changed", "_status", "post_id", "description");` + + `create index if not exists "commentsTemp_post_id" on "commentsTemp" ("post_id");` + + `create index if not exists "commentsTemp__status" on "commentsTemp" ("_status");` + + `INSERT INTO commentsTemp(id,_changed,_status,post_id,description) SELECT id,_changed,_status,post_id,body FROM comments;` + + `DROP TABLE comments;` + + `ALTER TABLE commentsTemp RENAME TO comments;`, ) }) it(`encodes migrations with unsafe SQL`, () => { From b5b642a6ea0c5432168a84f110ca0184a20e9164 Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sun, 26 May 2024 17:43:18 -0700 Subject: [PATCH 05/25] add missing type --- src/Schema/migrations/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 7795119fb..9d7ddd37c 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -57,6 +57,7 @@ export type MigrationStep = | SqlMigrationStep | DestroyColumnMigrationStep | RenameColumnMigrationStep + | DestroyTableMigrationStep type Migration = $RE<{ toVersion: SchemaVersion, From 0b0bdf1baadbfbf5d17a49192be3d548df436aaa Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Sun, 2 Jun 2024 09:59:17 -0700 Subject: [PATCH 06/25] add migration step labels to getSyncChanges --- src/Schema/migrations/getSyncChanges/index.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Schema/migrations/getSyncChanges/index.js b/src/Schema/migrations/getSyncChanges/index.js index 0c6e74321..7c4eb81b2 100644 --- a/src/Schema/migrations/getSyncChanges/index.js +++ b/src/Schema/migrations/getSyncChanges/index.js @@ -41,7 +41,14 @@ export default function getSyncChanges( steps.forEach((step) => { invariant( - ['create_table', 'add_columns', 'sql', 'rename_column'].includes(step.type), + [ + 'create_table', + 'add_columns', + 'destroy_column', + 'rename_column', + 'destroy_table', + 'sql', + ].includes(step.type), `Unknown migration step type ${step.type}. Can not perform migration sync. This most likely means your migrations are defined incorrectly. It could also be a WatermelonDB bug.`, ) }) From d91e6d8362206c9ce4df01dfff1c22edebb3463d Mon Sep 17 00:00:00 2001 From: Spencer Kam Date: Tue, 4 Jun 2024 09:35:04 -0700 Subject: [PATCH 07/25] remove destroy_column and destroy_table from possible future types test --- src/Schema/migrations/getSyncChanges/test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Schema/migrations/getSyncChanges/test.js b/src/Schema/migrations/getSyncChanges/test.js index 61c353ec0..a0ae24461 100644 --- a/src/Schema/migrations/getSyncChanges/test.js +++ b/src/Schema/migrations/getSyncChanges/test.js @@ -271,8 +271,6 @@ describe('getSyncChanges', () => { 'add_column_index', 'make_column_optional', 'make_column_required', - 'destroy_table', - 'destroy_column', ] possibleFutureTypes.forEach((type) => { expect(() => testSteps([{ type }])).toThrow('Unknown migration step type') From 50d9be3c410d81330e80ca5eea9d0e639de74440 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 15:08:33 +0200 Subject: [PATCH 08/25] encodeSchema: consistent sql style, escape column/table names --- src/adapters/sqlite/encodeSchema/index.js | 28 +++++++++++------------ src/adapters/sqlite/encodeSchema/test.js | 11 +++++---- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 5015cf6f6..9725918fd 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -98,34 +98,34 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema { table, column }, tableSchema, ) => { - const newTempTable = { ...tableSchema, name: `${table}Temp` } + const tempName = `${table}Temp` + const newTempTable = { ...tableSchema, name: tempName } const newColumns = [ ...standardColumns, ...Object.keys(tableSchema.columns).filter((c) => c !== column), - ] + ].map(c => `"${c}"`).join(', ') const createTempTableSQL = `${encodeTable(newTempTable)}` - const injectValuesSQL = `INSERT INTO ${table}Temp(${newColumns.join( - ',', - )}) SELECT ${newColumns.join(',')} FROM ${table};` - const dropOldTable = `DROP TABLE ${table};` - const renameTempTableSQL = `ALTER TABLE ${table}Temp RENAME TO ${table};` + const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${newColumns} from "${table}";` + const dropOldTable = `drop table "${table}";` + const renameTempTableSQL = `alter table "${tempName}" rename to "${table}";` return `${createTempTableSQL}${injectValuesSQL}${dropOldTable}${renameTempTableSQL}` } + const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ( { table, from, to }, tableSchema, ) => { - const newTempTable = { ...tableSchema, name: `${table}Temp` } + const tempName = `${table}Temp` + const newTempTable = { ...tableSchema, name: tempName } const newColumnNames = [...standardColumns, ...Object.keys(tableSchema.columns)] - const oldColumnNames = newColumnNames.map((c) => (c === to ? from : c)) + const newColumns = newColumnNames.map(c => `"${c}"`).join(", ") + const oldColumns = newColumnNames.map((c) => (c === to ? from : c)).map(c => `"${c}"`).join(", ") const createTempTableSQL = `${encodeTable(newTempTable)}` - const injectValuesSQL = `INSERT INTO ${table}Temp(${newColumnNames.join( - ',', - )}) SELECT ${oldColumnNames.join(',')} FROM ${table};` + const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${oldColumns} from "${table}";` - const dropOldTableSQL = `DROP TABLE ${table};` - const renameTempTableSQL = `ALTER TABLE ${table}Temp RENAME TO ${table};` + const dropOldTableSQL = `drop table "${table}";` + const renameTempTableSQL = `alter table "${tempName}" rename to "${table}";` return `${createTempTableSQL}${injectValuesSQL}${dropOldTableSQL}${renameTempTableSQL}` } diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 2d53ae4f4..7d395b04b 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -212,15 +212,16 @@ describe('encodeMigrationSteps', () => { `create index if not exists "postsTemp_author_id" on "postsTemp" ("author_id");` + `create index if not exists "postsTemp_is_pinned" on "postsTemp" ("is_pinned");` + `create index if not exists "postsTemp__status" on "postsTemp" ("_status");` + - `INSERT INTO postsTemp(id,_changed,_status,reactions,author_id,is_pinned) SELECT id,_changed,_status,reactions,author_id,is_pinned FROM posts;` + - `DROP TABLE posts;ALTER TABLE postsTemp RENAME TO posts;` + + `insert into "postsTemp" ("id", "_changed", "_status", "reactions", "author_id", "is_pinned") select "id", "_changed", "_status", "reactions", "author_id", "is_pinned" from "posts";` + + `drop table "posts";` + + `alter table "postsTemp" rename to "posts";` + // rename column `create table "commentsTemp" ("id" primary key, "_changed", "_status", "post_id", "description");` + `create index if not exists "commentsTemp_post_id" on "commentsTemp" ("post_id");` + `create index if not exists "commentsTemp__status" on "commentsTemp" ("_status");` + - `INSERT INTO commentsTemp(id,_changed,_status,post_id,description) SELECT id,_changed,_status,post_id,body FROM comments;` + - `DROP TABLE comments;` + - `ALTER TABLE commentsTemp RENAME TO comments;`, + `insert into "commentsTemp" ("id", "_changed", "_status", "post_id", "description") select "id", "_changed", "_status", "post_id", "body" from "comments";` + + `drop table "comments";` + + `alter table "commentsTemp" rename to "comments";`, ) }) it(`encodes migrations with unsafe SQL`, () => { From 493907e63c118bd96f15fa50d172d469dc517e73 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 15:21:28 +0200 Subject: [PATCH 09/25] encodeSchema: clean up --- src/adapters/sqlite/encodeSchema/index.js | 54 +++++++++++------------ 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 9725918fd..2e8dbdbec 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -14,15 +14,13 @@ import type { SQL } from '../index' import encodeValue from '../encodeValue' const standardColumns = ['id', '_changed', '_status'] -const standardColumnsSQL = standardColumns - .map((column) => (column === 'id' ? `"${column}" primary key` : `"${column}"`)) - .join(', ') +const standardColumnsInsertSQL = `"id" primary key, "_changed", "_status"` const commonSchema = 'create table "local_storage" ("key" varchar(16) primary key not null, "value" text not null);' + 'create index "local_storage_key_index" on "local_storage" ("key");' const encodeCreateTable = ({ name, columns }: TableSchema): SQL => { - const columnsSQL = [standardColumnsSQL] + const columnsSQL = [standardColumnsInsertSQL] .concat(Object.keys(columns).map((column) => `"${column}"`)) .join(', ') return `create table "${name}" (${columnsSQL});` @@ -94,40 +92,40 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ }) .join('') -const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ( - { table, column }, +const encodeChangeColumnMigrationStep: (TableName, ColumnName, ?ColumnName, TableSchema) => SQL = ( + table, + oldColumn, + newColumn, // null = destroy column tableSchema, ) => { const tempName = `${table}Temp` - const newTempTable = { ...tableSchema, name: tempName } - const newColumns = [ - ...standardColumns, - ...Object.keys(tableSchema.columns).filter((c) => c !== column), - ].map(c => `"${c}"`).join(', ') - const createTempTableSQL = `${encodeTable(newTempTable)}` - const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${newColumns} from "${table}";` + const tempTable = { ...tableSchema, name: tempName } + + const newColumnNames = [...standardColumns, ...Object.keys(tableSchema.columns)].filter( + newColumn ? Boolean : (c) => c !== oldColumn, + ) + const newColumns = newColumnNames.map((c) => `"${c}"`).join(', ') + const oldColumns = newColumnNames + .map((c) => (c === newColumn ? oldColumn : c)) + .map((c) => `"${c}"`) + .join(', ') + + const createTempTableSQL = encodeTable(tempTable) + const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${oldColumns} from "${table}";` const dropOldTable = `drop table "${table}";` const renameTempTableSQL = `alter table "${tempName}" rename to "${table}";` - return `${createTempTableSQL}${injectValuesSQL}${dropOldTable}${renameTempTableSQL}` + return createTempTableSQL + injectValuesSQL + dropOldTable + renameTempTableSQL } +const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ( + { table, column }, + tableSchema, +) => encodeChangeColumnMigrationStep(table, column, null, tableSchema) + const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ( { table, from, to }, tableSchema, -) => { - const tempName = `${table}Temp` - const newTempTable = { ...tableSchema, name: tempName } - const newColumnNames = [...standardColumns, ...Object.keys(tableSchema.columns)] - const newColumns = newColumnNames.map(c => `"${c}"`).join(", ") - const oldColumns = newColumnNames.map((c) => (c === to ? from : c)).map(c => `"${c}"`).join(", ") - - const createTempTableSQL = `${encodeTable(newTempTable)}` - const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${oldColumns} from "${table}";` - - const dropOldTableSQL = `drop table "${table}";` - const renameTempTableSQL = `alter table "${tempName}" rename to "${table}";` - return `${createTempTableSQL}${injectValuesSQL}${dropOldTableSQL}${renameTempTableSQL}` -} +) => encodeChangeColumnMigrationStep(table, from, to, tableSchema) const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ table }) => { return ` From 637737a100f2a4c7bb1e60529c7827a4042154c0 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 15:21:43 +0200 Subject: [PATCH 10/25] encodeSchema: clean up and test table destroy --- src/adapters/sqlite/encodeSchema/index.js | 4 +--- src/adapters/sqlite/encodeSchema/test.js | 9 ++++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 2e8dbdbec..25b49e8ca 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -128,9 +128,7 @@ const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) ) => encodeChangeColumnMigrationStep(table, from, to, tableSchema) const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ table }) => { - return ` - DROP TABLE IF EXISTS ${table}; - ` + return `drop table if exists "${table}";` } export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, schema) => diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 7d395b04b..2c38c12ab 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -4,6 +4,7 @@ import { addColumns, createTable, destroyColumn, + destroyTable, renameColumn, unsafeExecuteSql, } from '../../../Schema/migrations' @@ -192,6 +193,9 @@ describe('encodeMigrationSteps', () => { from: 'body', to: 'description', }), + destroyTable({ + table: 'authors', + }), ] expect(encodeMigrationSteps(migrationSteps, migrationSchema)).toBe( @@ -221,7 +225,10 @@ describe('encodeMigrationSteps', () => { `create index if not exists "commentsTemp__status" on "commentsTemp" ("_status");` + `insert into "commentsTemp" ("id", "_changed", "_status", "post_id", "description") select "id", "_changed", "_status", "post_id", "body" from "comments";` + `drop table "comments";` + - `alter table "commentsTemp" rename to "comments";`, + `alter table "commentsTemp" rename to "comments";` + + // destroy table + `drop table if exists "authors";` + + '', ) }) it(`encodes migrations with unsafe SQL`, () => { From f7b17b63b94d686fa1e2e11a34b1852f2ac118c5 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 15:38:21 +0200 Subject: [PATCH 11/25] migrations: validate column name in renameColumn, add test --- src/Schema/index.js | 2 +- src/Schema/migrations/index.js | 3 ++- src/Schema/migrations/test.js | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/Schema/index.js b/src/Schema/index.js index a91d0eacd..4dd6db2a2 100644 --- a/src/Schema/index.js +++ b/src/Schema/index.js @@ -98,7 +98,7 @@ export function appSchema({ version, tables: tableList, unsafeSql }: AppSchemaSp return { version, tables, unsafeSql } } -const validateName = (name: string) => { +export const validateName = (name: string) => { if (process.env.NODE_ENV !== 'production') { invariant( !['id', '_changed', '_status', 'local_storage'].includes(name.toLowerCase()), diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 9d7ddd37c..b9b6dc89a 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -14,7 +14,7 @@ import type { TableSchemaSpec, SchemaVersion, } from '../index' -import { tableSchema, validateColumnSchema } from '../index' +import { tableSchema, validateName, validateColumnSchema } from '../index' export type CreateTableMigrationStep = $RE<{ type: 'create_table', @@ -218,6 +218,7 @@ export function renameColumn({ invariant(table, `Missing table name in renameColumn()`) invariant(from, `Missing 'from' in renameColumn()`) invariant(to, `Missing 'to' in renameColumn()`) + validateName(to) } return { type: 'rename_column', table, from, to } } diff --git a/src/Schema/migrations/test.js b/src/Schema/migrations/test.js index d3eaf546a..d66f438c2 100644 --- a/src/Schema/migrations/test.js +++ b/src/Schema/migrations/test.js @@ -251,6 +251,39 @@ describe('migration step functions', () => { it('throws if destroyTable() is malformed', () => { expect(() => destroyTable()).toThrow('table') }) + it('does not allow unsafe names', () => { + // TODO: Move to a common location with Schema/test + ;[ + '"hey"', + "'hey'", + '`hey`', + "foo' and delete * from users --", + 'id', + '_changed', + '_status', + 'local_storage', + '$loki', + '__foo', + '__proto__', + 'toString', + 'valueOf', + 'oid', + '_rowid_', + 'ROWID', + ].forEach((name) => { + // console.log(name) + expect(() => createTable({ name: 'foo', columns: [{ name, type: 'string' }] })).toThrow( + 'name', + ) + expect(() => createTable({ name, columns: [{ name: 'hey', type: 'string' }] })).toThrow( + 'name', + ) + expect(() => addColumns({ table: 'foo', columns: [{ name, type: 'string' }] })).toThrow( + 'name', + ) + expect(() => renameColumn({ table: 'foo', from: 'hey', to: name })).toThrow('name') + }) + }) }) describe('stepsForMigration', () => { From b2a5e1a349a214197add1fca3162b681134a6df3 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 15:41:35 +0200 Subject: [PATCH 12/25] loki: clean up new migrations --- src/adapters/lokijs/worker/DatabaseDriver.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/adapters/lokijs/worker/DatabaseDriver.js b/src/adapters/lokijs/worker/DatabaseDriver.js index e75cc3ef0..ae826cc55 100644 --- a/src/adapters/lokijs/worker/DatabaseDriver.js +++ b/src/adapters/lokijs/worker/DatabaseDriver.js @@ -444,10 +444,15 @@ export default class DatabaseDriver { _executeRenameColumnMigration({ table, from, to }: RenameColumnMigrationStep): void { const collection = this.loki.getCollection(table) + // NOTE: Seems a bit safer to copy first, then delete old ones collection.findAndUpdate({}, (record) => { if (record[from] !== undefined) { record[to] = record[from] + } else { + delete record[to] } + }) + collection.findAndUpdate({}, (record) => { delete record[from] }) } From 1b8f69bb8144881105b455e48e5ca20a2ae475ac Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 22:35:46 +0200 Subject: [PATCH 13/25] commonTests: test column rename --- src/adapters/__tests__/commonTests.js | 28 +++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index f4ce5fa2f..3be4c228c 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -10,7 +10,7 @@ import Query from '../../Query' import { sanitizedRaw } from '../../RawRecord' import * as Q from '../../QueryDescription' import { appSchema, tableSchema } from '../../Schema' -import { schemaMigrations, createTable, addColumns } from '../../Schema/migrations' +import { schemaMigrations, createTable, addColumns, renameColumn } from '../../Schema/migrations' import { matchTests, naughtyMatchTests, joinTests } from '../../__tests__/databaseTests' import DatabaseAdapterCompat from '../compat' @@ -949,7 +949,7 @@ export default () => { ).rejects.toBeInstanceOf(Error) // migrate to new version - const taskColumnsV5 = [ + const taskColumnsV5_new = [ { name: 'test_string', type: 'string' }, { name: 'test_string_optional', type: 'string', isOptional: true }, { name: 'test_number', type: 'number' }, @@ -957,6 +957,12 @@ export default () => { { name: 'test_boolean', type: 'boolean' }, { name: 'test_boolean_optional', type: 'boolean', isOptional: true }, ] + + const taskColumnsV5 = [ + ...taskColumnsV5_new, + // renamed columns + { name: 'num1_renamed', type: 'number' }, + ] const projectColumnsV5 = [{ name: 'text2', type: 'string', isIndexed: true }] const tagAssignmentSchema = { name: 'tag_assignments', @@ -968,7 +974,7 @@ export default () => { tables: [ tableSchema({ name: 'tasks', - columns: [...taskColumnsV3, ...taskColumnsV5], + columns: taskColumnsV5, }), tableSchema({ name: 'projects', @@ -981,7 +987,10 @@ export default () => { migrations: [ { toVersion: 5, - steps: [addColumns({ table: 'tasks', columns: taskColumnsV5 })], + steps: [ + addColumns({ table: 'tasks', columns: taskColumnsV5_new }), + renameColumn({ table: 'tasks', from: 'num1', to: 'num1_renamed' }), + ], }, { toVersion: 4, @@ -1009,6 +1018,17 @@ export default () => { // check that the data is still there expect(await adapter.count(new Query({ modelClass: MockTask }, []))).toBe(2) + // check that column was renamed + { + const t1 = await adapter.find('tasks', 't1') + expect(t1.num1).toBe(undefined) + expect(t1.num1_renamed).toBe(10) + + const t2 = await adapter.find('tasks', 't2') + expect(t2.num1).toBe(undefined) + expect(t2.num1_renamed).toBe(20) + } + // check if new columns were populated with appropriate default values const checkTaskColumn = (columnName, expectedValue) => new Query({ modelClass: MockTask }, [Q.where(columnName, expectedValue)]).serialize() From 5a27ec18e299721198353d632c81057482e91e5f Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 22:43:59 +0200 Subject: [PATCH 14/25] commonTests: migrations: destroyTable --- src/adapters/__tests__/commonTests.js | 57 ++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 3be4c228c..65dfed55d 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -10,7 +10,13 @@ import Query from '../../Query' import { sanitizedRaw } from '../../RawRecord' import * as Q from '../../QueryDescription' import { appSchema, tableSchema } from '../../Schema' -import { schemaMigrations, createTable, addColumns, renameColumn } from '../../Schema/migrations' +import { + schemaMigrations, + createTable, + addColumns, + renameColumn, + destroyTable, +} from '../../Schema/migrations' import { matchTests, naughtyMatchTests, joinTests } from '../../__tests__/databaseTests' import DatabaseAdapterCompat from '../compat' @@ -1070,6 +1076,55 @@ export default () => { const tt1 = await adapter.find('tag_assignments', 'tt2') expect(tt1.text1).toBe('hello') }) + it('migrations: destroyTable', async (_adapter, AdapterClass, extraAdapterOptions) => { + // initial schema + const taskColumns_v1 = [{ name: 'num1', type: 'number' }] + const projectColumns_v1 = [{ name: 'text1', type: 'string' }] + const testSchema_v1 = appSchema({ + version: 1, + tables: [ + tableSchema({ name: 'tasks', columns: taskColumns_v1 }), + tableSchema({ name: 'projects', columns: projectColumns_v1 }), + ], + }) + + let adapter = new DatabaseAdapterCompat( + new AdapterClass({ + schema: testSchema_v1, + migrations: schemaMigrations({ migrations: [] }), + ...extraAdapterOptions, + }), + ) + + // add data + await adapter.batch([ + ['create', 'tasks', { id: 't1', num1: 10 }], + ['create', 'projects', { id: 'p1', text1: 'hi' }], + ]) + + // apply changes + // NOTE: This is incorrect, as the projects table should be gone. However, we want to test that + // the migration really was applied, and not just that table being queried is not in the schema + const testSchema_v2 = { ...testSchema_v1, version: 2 } + const migrations_v2 = schemaMigrations({ + migrations: [ + { + toVersion: 2, + steps: [destroyTable({ table: 'projects' })], + }, + ], + }) + adapter = await adapter.testClone({ + schema: testSchema_v2, + migrations: migrations_v2, + }) + + // check that unrelated table is still there + expect(await adapter.find('tasks', 't1')).toMatchObject({ id: 't1', num1: 10 }) + + // check that deleted table is gone + await expect(adapter.find('projects', 'p1')).rejects.toBeInstanceOf(Error) + }) it(`can perform empty migrations (regression test)`, async (_adapter, AdapterClass, extraAdapterOptions) => { let adapter = new DatabaseAdapterCompat( new AdapterClass({ From be4349d9c9c5274d5bbc5b4443f97381ccee5baf Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 22:53:25 +0200 Subject: [PATCH 15/25] commonTests: migrations: destroyColumn --- src/adapters/__tests__/commonTests.js | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 65dfed55d..339a16c02 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -16,6 +16,7 @@ import { addColumns, renameColumn, destroyTable, + destroyColumn, } from '../../Schema/migrations' import { matchTests, naughtyMatchTests, joinTests } from '../../__tests__/databaseTests' @@ -1076,6 +1077,63 @@ export default () => { const tt1 = await adapter.find('tag_assignments', 'tt2') expect(tt1.text1).toBe('hello') }) + it('migrations: destroyColumn', async (_adapter, AdapterClass, extraAdapterOptions) => { + // initial schema + const taskColumns_v1 = [ + { name: 'num1', type: 'number' }, + { name: 'num2', type: 'number' }, + ] + const testSchema_v1 = appSchema({ + version: 1, + tables: [tableSchema({ name: 'tasks', columns: taskColumns_v1 })], + }) + + let adapter = new DatabaseAdapterCompat( + new AdapterClass({ + schema: testSchema_v1, + migrations: schemaMigrations({ migrations: [] }), + ...extraAdapterOptions, + }), + ) + + // add data + await adapter.batch([ + ['create', 'tasks', { id: 't1', num1: 10, num2: 1337 }], + ['create', 'tasks', { id: 't2', num1: 20, num2: 2137 }], + ]) + + // apply changes - remove num2 column + const taskColumns_v2 = [{ name: 'num1', type: 'number' }] + const testSchema_v2 = appSchema({ + version: 2, + tables: [tableSchema({ name: 'tasks', columns: taskColumns_v2 })], + }) + const migrations_v2 = schemaMigrations({ + migrations: [{ toVersion: 2, steps: [destroyColumn({ table: 'tasks', column: 'num2' })] }], + }) + + adapter = await adapter.testClone({ schema: testSchema_v2, migrations: migrations_v2 }) + + // check that data was transformed correctly + const t1 = await adapter.find('tasks', 't1') + expect(t1.num1).toBe(10) + expect(t1.num2).toBe(undefined) + + const t2 = await adapter.find('tasks', 't2') + expect(t2.num1).toBe(20) + expect(t2.num2).toBe(undefined) + + // check that it's no longer possible to insert data into removed column + // NOTE: batch() expects sanitized raws, and Loki will take anything if it's not; we're just checking + // SQL which has an actual schema + if (AdapterClass.name === 'SQLiteAdapter') { + await adapter.batch([['create', 'tasks', { id: 't3', num1: 30, num2: 3333 }]]) + adapter = await adapter.testClone() + const t3 = await adapter.find('tasks', 't3') + expect(t3.num1).toBe(30) + expect(t3.num2).toBe(undefined) + } + }) it('migrations: destroyTable', async (_adapter, AdapterClass, extraAdapterOptions) => { // initial schema const taskColumns_v1 = [{ name: 'num1', type: 'number' }] From 46d9773b406802e62cf21a16a0c31a3ff322f696 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 23:05:23 +0200 Subject: [PATCH 16/25] commonTests: migrations: clean up --- src/adapters/__tests__/commonTests.js | 105 ++++++++++---------------- 1 file changed, 39 insertions(+), 66 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 339a16c02..982603763 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -924,6 +924,10 @@ export default () => { await expectToRejectWithMessage(adapter.getDeletedRecords(table), msg) await expectToRejectWithMessage(adapter.destroyDeletedRecords(table, []), msg) }) + + const migrationsAdapter = (AdapterClass, extraAdapterOptions, options) => + new DatabaseAdapterCompat(new AdapterClass({ ...options, ...extraAdapterOptions })) + it('migrates database between versions', async (_adapter, AdapterClass, extraAdapterOptions) => { // launch app in one version const taskColumnsV3 = [{ name: 'num1', type: 'number' }] @@ -935,14 +939,12 @@ export default () => { tableSchema({ name: 'projects', columns: projectColumnsV3 }), ], }) + const migrationsV3 = schemaMigrations({ migrations: [{ toVersion: 3, steps: [] }] }) - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: testSchemaV3, - migrations: schemaMigrations({ migrations: [{ toVersion: 3, steps: [] }] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: testSchemaV3, + migrations: migrationsV3, + }) // add data await adapter.batch([ @@ -1079,22 +1081,20 @@ export default () => { }) it('migrations: destroyColumn', async (_adapter, AdapterClass, extraAdapterOptions) => { // initial schema - const taskColumns_v1 = [ - { name: 'num1', type: 'number' }, - { name: 'num2', type: 'number' }, - ] const testSchema_v1 = appSchema({ version: 1, - tables: [tableSchema({ name: 'tasks', columns: taskColumns_v1 })], + tables: [ + tableSchema({ + name: 'tasks', + columns: [ + { name: 'num1', type: 'number' }, + { name: 'num2', type: 'number' }, + ], + }), + ], }) - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: testSchema_v1, - migrations: schemaMigrations({ migrations: [] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { schema: testSchema_v1 }) // add data await adapter.batch([ @@ -1103,10 +1103,9 @@ export default () => { ]) // apply changes - remove num2 column - const taskColumns_v2 = [{ name: 'num1', type: 'number' }] const testSchema_v2 = appSchema({ version: 2, - tables: [tableSchema({ name: 'tasks', columns: taskColumns_v2 })], + tables: [tableSchema({ name: 'tasks', columns: [{ name: 'num1', type: 'number' }] })], }) const migrations_v2 = schemaMigrations({ migrations: [{ toVersion: 2, steps: [destroyColumn({ table: 'tasks', column: 'num2' })] }], @@ -1146,13 +1145,7 @@ export default () => { ], }) - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: testSchema_v1, - migrations: schemaMigrations({ migrations: [] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { schema: testSchema_v1 }) // add data await adapter.batch([ @@ -1165,17 +1158,9 @@ export default () => { // the migration really was applied, and not just that table being queried is not in the schema const testSchema_v2 = { ...testSchema_v1, version: 2 } const migrations_v2 = schemaMigrations({ - migrations: [ - { - toVersion: 2, - steps: [destroyTable({ table: 'projects' })], - }, - ], - }) - adapter = await adapter.testClone({ - schema: testSchema_v2, - migrations: migrations_v2, + migrations: [{ toVersion: 2, steps: [destroyTable({ table: 'projects' })] }], }) + adapter = await adapter.testClone({ schema: testSchema_v2, migrations: migrations_v2 }) // check that unrelated table is still there expect(await adapter.find('tasks', 't1')).toMatchObject({ id: 't1', num1: 10 }) @@ -1184,13 +1169,10 @@ export default () => { await expect(adapter.find('projects', 'p1')).rejects.toBeInstanceOf(Error) }) it(`can perform empty migrations (regression test)`, async (_adapter, AdapterClass, extraAdapterOptions) => { - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: { ...testSchema, version: 1 }, - migrations: schemaMigrations({ migrations: [] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: { ...testSchema, version: 1 }, + migrations: schemaMigrations({ migrations: [] }), + }) await adapter.batch([['create', 'tasks', mockTaskRaw({ id: 't1', text1: 'foo' })]]) expect(await adapter.count(taskQuery())).toBe(1) @@ -1207,13 +1189,10 @@ export default () => { }) it(`resets database when it's newer than app schema`, async (_adapter, AdapterClass, extraAdapterOptions) => { // launch newer version of the app - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: { ...testSchema, version: 3 }, - migrations: schemaMigrations({ migrations: [{ toVersion: 3, steps: [] }] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: { ...testSchema, version: 3 }, + migrations: schemaMigrations({ migrations: [{ toVersion: 3, steps: [] }] }), + }) await adapter.batch([['create', 'tasks', mockTaskRaw({})]]) expect(await adapter.count(taskQuery())).toBe(1) @@ -1230,13 +1209,10 @@ export default () => { }) it('resets database when there are no available migrations', async (_adapter, AdapterClass, extraAdapterOptions) => { // launch older version of the app - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: { ...testSchema, version: 1 }, - migrations: schemaMigrations({ migrations: [] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: { ...testSchema, version: 1 }, + migrations: schemaMigrations({ migrations: [] }), + }) await adapter.batch([['create', 'tasks', mockTaskRaw({})]]) expect(await adapter.count(taskQuery())).toBe(1) @@ -1253,13 +1229,10 @@ export default () => { }) it('errors when migration fails', async (_adapter, AdapterClass, extraAdapterOptions) => { // launch older version of the app - let adapter = new DatabaseAdapterCompat( - new AdapterClass({ - schema: { ...testSchema, version: 1 }, - migrations: schemaMigrations({ migrations: [] }), - ...extraAdapterOptions, - }), - ) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: { ...testSchema, version: 1 }, + migrations: schemaMigrations({ migrations: [] }), + }) await adapter.batch([['create', 'tasks', mockTaskRaw({})]]) expect(await adapter.count(taskQuery())).toBe(1) From 9c029edafc8a0887ea126ce1de755fdd052bd162 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Thu, 13 Jun 2024 23:11:38 +0200 Subject: [PATCH 17/25] commonTests: migrations: renameColumn --- src/adapters/__tests__/commonTests.js | 92 +++++++++++++++++++++------ 1 file changed, 72 insertions(+), 20 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 982603763..667378ece 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -967,11 +967,7 @@ export default () => { { name: 'test_boolean_optional', type: 'boolean', isOptional: true }, ] - const taskColumnsV5 = [ - ...taskColumnsV5_new, - // renamed columns - { name: 'num1_renamed', type: 'number' }, - ] + const taskColumnsV5 = [...taskColumnsV5_new] const projectColumnsV5 = [{ name: 'text2', type: 'string', isIndexed: true }] const tagAssignmentSchema = { name: 'tag_assignments', @@ -996,10 +992,7 @@ export default () => { migrations: [ { toVersion: 5, - steps: [ - addColumns({ table: 'tasks', columns: taskColumnsV5_new }), - renameColumn({ table: 'tasks', from: 'num1', to: 'num1_renamed' }), - ], + steps: [addColumns({ table: 'tasks', columns: taskColumnsV5_new })], }, { toVersion: 4, @@ -1027,17 +1020,6 @@ export default () => { // check that the data is still there expect(await adapter.count(new Query({ modelClass: MockTask }, []))).toBe(2) - // check that column was renamed - { - const t1 = await adapter.find('tasks', 't1') - expect(t1.num1).toBe(undefined) - expect(t1.num1_renamed).toBe(10) - - const t2 = await adapter.find('tasks', 't2') - expect(t2.num1).toBe(undefined) - expect(t2.num1_renamed).toBe(20) - } - // check if new columns were populated with appropriate default values const checkTaskColumn = (columnName, expectedValue) => new Query({ modelClass: MockTask }, [Q.where(columnName, expectedValue)]).serialize() @@ -1079,6 +1061,76 @@ export default () => { const tt1 = await adapter.find('tag_assignments', 'tt2') expect(tt1.text1).toBe('hello') }) + it('migrations: renameColumn', async (_adapter, AdapterClass, extraAdapterOptions) => { + // initial schema + const testSchema_v1 = appSchema({ + version: 1, + tables: [ + tableSchema({ + name: 'tasks', + columns: [ + { name: 'num1', type: 'number' }, + { name: 'num2', type: 'number', isOptional: true }, + ], + }), + ], + }) + + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { schema: testSchema_v1 }) + + // add data + await adapter.batch([ + ['create', 'tasks', { id: 't1', num1: 10, num2: 1337 }], + ['create', 'tasks', { id: 't2', num1: 20 }], + ]) + + // apply changes - rename num2 column + const testSchema_v2 = appSchema({ + version: 2, + tables: [ + tableSchema({ + name: 'tasks', + columns: [ + { name: 'num1', type: 'number' }, + { name: 'num2_renamed', type: 'number', isOptional: true }, + ], + }), + ], + }) + const migrations_v2 = schemaMigrations({ + migrations: [ + { + toVersion: 2, + steps: [renameColumn({ table: 'tasks', from: 'num2', to: 'num2_renamed' })], + }, + ], + }) + + adapter = await adapter.testClone({ schema: testSchema_v2, migrations: migrations_v2 }) + + // check that data was transformed correctly + const t1 = await adapter.find('tasks', 't1') + expect(t1.num1).toBe(10) + expect(t1.num2).toBe(undefined) + expect(t1.num2_renamed).toBe(1337) + + const t2 = await adapter.find('tasks', 't2') + expect(t2.num1).toBe(20) + expect(t2.num2).toBe(undefined) + expect(t2.num2_renamed).toBe(null) + + // check that it's no longer possible to insert data into removed column + // NOTE: batch() expects sanitized raws, and Loki will take anything if it's not; we're just checking + // SQL which has an actual schema + if (AdapterClass.name === 'SQLiteAdapter') { + await adapter.batch([['create', 'tasks', { id: 't3', num1: 30, num2: 3333 }]]) + adapter = await adapter.testClone() + const t3 = await adapter.find('tasks', 't3') + expect(t3.num1).toBe(30) + expect(t3.num2).toBe(undefined) + expect(t3.num2_renamed).toBe(null) + } + }) it('migrations: destroyColumn', async (_adapter, AdapterClass, extraAdapterOptions) => { // initial schema const testSchema_v1 = appSchema({ From c7c9c2db8201134b96596513be69af583ba29c7b Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Fri, 14 Jun 2024 23:10:30 +0200 Subject: [PATCH 18/25] commonTests: migrations: add more variants of column rename/delete --- src/adapters/__tests__/commonTests.js | 33 +++++++++++++++++---------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index 667378ece..ebd173e2f 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -1085,7 +1085,7 @@ export default () => { ]) // apply changes - rename num2 column - const testSchema_v2 = appSchema({ + const testSchema_after = appSchema({ version: 2, tables: [ tableSchema({ @@ -1097,16 +1097,19 @@ export default () => { }), ], }) - const migrations_v2 = schemaMigrations({ + const migrations_after = schemaMigrations({ migrations: [ { toVersion: 2, - steps: [renameColumn({ table: 'tasks', from: 'num2', to: 'num2_renamed' })], + steps: [ + renameColumn({ table: 'tasks', from: 'num2', to: 'num2_interim' }), + renameColumn({ table: 'tasks', from: 'num2_interim', to: 'num2_renamed' }), + ], }, ], }) - adapter = await adapter.testClone({ schema: testSchema_v2, migrations: migrations_v2 }) + adapter = await adapter.testClone({ schema: testSchema_after, migrations: migrations_after }) // check that data was transformed correctly const t1 = await adapter.find('tasks', 't1') @@ -1141,6 +1144,7 @@ export default () => { columns: [ { name: 'num1', type: 'number' }, { name: 'num2', type: 'number' }, + { name: 'num3', type: 'number' }, ], }), ], @@ -1150,29 +1154,34 @@ export default () => { // add data await adapter.batch([ - ['create', 'tasks', { id: 't1', num1: 10, num2: 1337 }], - ['create', 'tasks', { id: 't2', num1: 20, num2: 2137 }], + ['create', 'tasks', { id: 't1', num1: 10, num2: 1337, num3: 3 }], + ['create', 'tasks', { id: 't2', num1: 20, num2: 2137, num3: 3 }], ]) - // apply changes - remove num2 column - const testSchema_v2 = appSchema({ - version: 2, + // apply changes - remove columns + const testSchema_after = appSchema({ + version: 3, tables: [tableSchema({ name: 'tasks', columns: [{ name: 'num1', type: 'number' }] })], }) - const migrations_v2 = schemaMigrations({ - migrations: [{ toVersion: 2, steps: [destroyColumn({ table: 'tasks', column: 'num2' })] }], + const migrations_after = schemaMigrations({ + migrations: [ + { toVersion: 2, steps: [destroyColumn({ table: 'tasks', column: 'num2' })] }, + { toVersion: 3, steps: [destroyColumn({ table: 'tasks', column: 'num3' })] }, + ], }) - adapter = await adapter.testClone({ schema: testSchema_v2, migrations: migrations_v2 }) + adapter = await adapter.testClone({ schema: testSchema_after, migrations: migrations_after }) // check that data was transformed correctly const t1 = await adapter.find('tasks', 't1') expect(t1.num1).toBe(10) expect(t1.num2).toBe(undefined) + expect(t1.num3).toBe(undefined) const t2 = await adapter.find('tasks', 't2') expect(t2.num1).toBe(20) expect(t2.num2).toBe(undefined) + expect(t2.num3).toBe(undefined) // check that it's no longer possible to insert data into removed column // NOTE: batch() expects sanitized raws, and Loki will take anything if it's not; we're just checking From e095586fe3bea0cf8c45a63ad839ab6a4f1a265f Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Fri, 14 Jun 2024 23:25:46 +0200 Subject: [PATCH 19/25] sqlite: simplify new migrations / fix bugs --- src/adapters/sqlite/encodeSchema/index.js | 51 +++++++---------------- src/adapters/sqlite/encodeSchema/test.js | 45 ++------------------ src/adapters/sqlite/index.js | 2 +- 3 files changed, 20 insertions(+), 78 deletions(-) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 25b49e8ca..208c03c39 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -13,7 +13,6 @@ import type { SQL } from '../index' import encodeValue from '../encodeValue' -const standardColumns = ['id', '_changed', '_status'] const standardColumnsInsertSQL = `"id" primary key, "_changed", "_status"` const commonSchema = 'create table "local_storage" ("key" varchar(16) primary key not null, "value" text not null);' + @@ -92,46 +91,28 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ }) .join('') -const encodeChangeColumnMigrationStep: (TableName, ColumnName, ?ColumnName, TableSchema) => SQL = ( +// Requires sqlite 3.35.0 / iOS 15 / Android 14 +const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ({ table, - oldColumn, - newColumn, // null = destroy column - tableSchema, -) => { - const tempName = `${table}Temp` - const tempTable = { ...tableSchema, name: tempName } - - const newColumnNames = [...standardColumns, ...Object.keys(tableSchema.columns)].filter( - newColumn ? Boolean : (c) => c !== oldColumn, - ) - const newColumns = newColumnNames.map((c) => `"${c}"`).join(', ') - const oldColumns = newColumnNames - .map((c) => (c === newColumn ? oldColumn : c)) - .map((c) => `"${c}"`) - .join(', ') - - const createTempTableSQL = encodeTable(tempTable) - const injectValuesSQL = `insert into "${tempName}" (${newColumns}) select ${oldColumns} from "${table}";` - const dropOldTable = `drop table "${table}";` - const renameTempTableSQL = `alter table "${tempName}" rename to "${table}";` - return createTempTableSQL + injectValuesSQL + dropOldTable + renameTempTableSQL + column, +}) => { + return `alter table "${table}" drop column "${column}";` } -const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ( - { table, column }, - tableSchema, -) => encodeChangeColumnMigrationStep(table, column, null, tableSchema) - -const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ( - { table, from, to }, - tableSchema, -) => encodeChangeColumnMigrationStep(table, from, to, tableSchema) +// Requires sqlite 3.25.0 / iOS 13 / Android 11 +const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ({ + table, + from, + to, +}) => { + return `alter table "${table}" rename column "${from}" to "${to}";` +} const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ table }) => { return `drop table if exists "${table}";` } -export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, schema) => +export const encodeMigrationSteps: (MigrationStep[]) => SQL = (steps) => steps .map((step) => { if (step.type === 'create_table') { @@ -139,9 +120,9 @@ export const encodeMigrationSteps: (MigrationStep[], AppSchema) => SQL = (steps, } else if (step.type === 'add_columns') { return encodeAddColumnsMigrationStep(step) } else if (step.type === 'destroy_column') { - return encodeDestroyColumnMigrationStep(step, schema.tables[step.table]) + return encodeDestroyColumnMigrationStep(step) } else if (step.type === 'rename_column') { - return encodeRenameColumnMigrationStep(step, schema.tables[step.table]) + return encodeRenameColumnMigrationStep(step) } else if (step.type === 'destroy_table') { return encodeDestroyTableMigrationStep(step) } else if (step.type === 'sql') { diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 2c38c12ab..87e9ad8f1 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -136,34 +136,6 @@ describe('encodeIndices', () => { }) describe('encodeMigrationSteps', () => { - const migrationSchema = appSchema({ - version: 5, - tables: [ - tableSchema({ - name: 'posts', - columns: [ - { name: 'reactions', type: 'number' }, - { name: 'author_id', type: 'string', isIndexed: true }, - { name: 'is_pinned', type: 'boolean', isIndexed: true }, - { name: 'subtitle', type: 'string', isOptional: true }, - ], - }), - tableSchema({ - name: 'comments', - columns: [ - { name: 'post_id', type: 'string', isIndexed: true }, - { name: 'description', type: 'string' }, - ], - }), - tableSchema({ - name: 'authors', - columns: [ - { name: 'created_at', type: 'number' }, - { name: 'updated_at', type: 'number' }, - ], - }), - ], - }) it('encodes migrations', () => { const migrationSteps = [ addColumns({ @@ -198,7 +170,7 @@ describe('encodeMigrationSteps', () => { }), ] - expect(encodeMigrationSteps(migrationSteps, migrationSchema)).toBe( + expect(encodeMigrationSteps(migrationSteps)).toBe( '' + `alter table "posts" add "subtitle";` + `update "posts" set "subtitle" = null;` + @@ -212,20 +184,9 @@ describe('encodeMigrationSteps', () => { `update "posts" set "is_pinned" = 0;` + `create index if not exists "posts_is_pinned" on "posts" ("is_pinned");` + // destroy column - `create table "postsTemp" ("id" primary key, "_changed", "_status", "reactions", "author_id", "is_pinned", "subtitle");` + - `create index if not exists "postsTemp_author_id" on "postsTemp" ("author_id");` + - `create index if not exists "postsTemp_is_pinned" on "postsTemp" ("is_pinned");` + - `create index if not exists "postsTemp__status" on "postsTemp" ("_status");` + - `insert into "postsTemp" ("id", "_changed", "_status", "reactions", "author_id", "is_pinned") select "id", "_changed", "_status", "reactions", "author_id", "is_pinned" from "posts";` + - `drop table "posts";` + - `alter table "postsTemp" rename to "posts";` + + `alter table "posts" drop column "subtitle";` + // rename column - `create table "commentsTemp" ("id" primary key, "_changed", "_status", "post_id", "description");` + - `create index if not exists "commentsTemp_post_id" on "commentsTemp" ("post_id");` + - `create index if not exists "commentsTemp__status" on "commentsTemp" ("_status");` + - `insert into "commentsTemp" ("id", "_changed", "_status", "post_id", "description") select "id", "_changed", "_status", "post_id", "body" from "comments";` + - `drop table "comments";` + - `alter table "commentsTemp" rename to "comments";` + + `alter table "comments" rename column "body" to "description";` + // destroy table `drop table if exists "authors";` + '', diff --git a/src/adapters/sqlite/index.js b/src/adapters/sqlite/index.js index d28da976e..72a5af9f5 100644 --- a/src/adapters/sqlite/index.js +++ b/src/adapters/sqlite/index.js @@ -169,7 +169,7 @@ export default class SQLiteAdapter implements DatabaseAdapter { 'setUpWithMigrations', [ this.dbName, - require('./encodeSchema').encodeMigrationSteps(migrationSteps, this.schema), + require('./encodeSchema').encodeMigrationSteps(migrationSteps), databaseVersion, this.schema.version, ], From 1cafb687d072b577c1af8cc257feacdfa8e703ab Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Sat, 15 Jun 2024 12:57:10 +0200 Subject: [PATCH 20/25] common: migrations: check table re-creation migration --- src/adapters/__tests__/commonTests.js | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index ebd173e2f..b0cf26599 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -1196,13 +1196,16 @@ export default () => { }) it('migrations: destroyTable', async (_adapter, AdapterClass, extraAdapterOptions) => { // initial schema - const taskColumns_v1 = [{ name: 'num1', type: 'number' }] - const projectColumns_v1 = [{ name: 'text1', type: 'string' }] + const projectsSchema = { + name: 'projects', + // NOTE: isIndexed is important as we want to check that the index won't conflict + columns: [{ name: 'text1', type: 'string', isIndexed: true }], + } const testSchema_v1 = appSchema({ version: 1, tables: [ - tableSchema({ name: 'tasks', columns: taskColumns_v1 }), - tableSchema({ name: 'projects', columns: projectColumns_v1 }), + tableSchema({ name: 'tasks', columns: [{ name: 'num1', type: 'number' }] }), + tableSchema(projectsSchema), ], }) @@ -1228,6 +1231,22 @@ export default () => { // check that deleted table is gone await expect(adapter.find('projects', 'p1')).rejects.toBeInstanceOf(Error) + + // apply changes + // create a new table (with the name of the old one) to see that it's not the same table + const testSchema_v3 = { ...testSchema_v1, version: 3 } + const migrations_v3 = schemaMigrations({ + migrations: [{ toVersion: 3, steps: [createTable(projectsSchema)] }], + }) + adapter = await adapter.testClone({ schema: testSchema_v3, migrations: migrations_v3 }) + + // check that the deleted table don't have its old data + expect(await adapter.find('projects', 'p1')).toBe(null) + + // add data to recreated table + await adapter.batch([['create', 'projects', { id: 'p1', text1: 'hi_new' }]]) + adapter = await adapter.testClone() + expect(await adapter.find('projects', 'p1')).toMatchObject({ id: 'p1', text1: 'hi_new' }) }) it(`can perform empty migrations (regression test)`, async (_adapter, AdapterClass, extraAdapterOptions) => { let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { From 99af408692d73c1991e3b58602ac3aab3f598218 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Sat, 15 Jun 2024 13:38:56 +0200 Subject: [PATCH 21/25] commonTests: migrations: add complex rename/destroy column test; fix destroy indexed columns --- src/adapters/__tests__/commonTests.js | 139 +++++++++++++++++++++- src/adapters/sqlite/encodeSchema/index.js | 3 +- src/adapters/sqlite/encodeSchema/test.js | 1 + 3 files changed, 141 insertions(+), 2 deletions(-) diff --git a/src/adapters/__tests__/commonTests.js b/src/adapters/__tests__/commonTests.js index b0cf26599..fc5f3f07a 100644 --- a/src/adapters/__tests__/commonTests.js +++ b/src/adapters/__tests__/commonTests.js @@ -1076,7 +1076,9 @@ export default () => { ], }) - let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { schema: testSchema_v1 }) + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: testSchema_v1, + }) // add data await adapter.batch([ @@ -1194,6 +1196,141 @@ export default () => { expect(t3.num2).toBe(undefined) } }) + it('migrations: renameColumn&destroyColumn (complex)', async (_adapter, AdapterClass, extraAdapterOptions) => { + // initial schema + const testSchema_before = appSchema({ + version: 2, + tables: [ + tableSchema({ + name: 'tasks', + columns: [ + { name: 'num1', type: 'number' }, + { name: 'num2', type: 'number', isOptional: true }, + { name: 'text3', type: 'string', isIndexed: true }, + { name: 'num3', type: 'number', isIndexed: true, isOptional: true }, + { name: 'num4', type: 'number' }, + { name: 'text4', type: 'string', isOptional: true }, + { name: 'text5', type: 'string' }, + ], + }), + ], + }) + + let adapter = migrationsAdapter(AdapterClass, extraAdapterOptions, { + schema: testSchema_before, + }) + + // add data + await adapter.batch([ + [ + 'create', + 'tasks', + { + id: 't1', + num1: 110, + num2: 120, + text3: 'hi130', + num3: 130, + num4: 140, + text4: 'hi140', + text5: 'hi150', + }, + ], + ['create', 'tasks', { id: 't2', num1: 210, text3: 'hi230', num4: 240, text5: 'hi250' }], + ]) + + // apply changes - remove columns + const testSchema_after = appSchema({ + version: 6, + tables: [ + tableSchema({ + name: 'tasks', + columns: [ + { name: 'num1', type: 'number' }, + { name: 'num2_v3', type: 'number', isOptional: true }, + { name: 'text3_v3', type: 'string', isIndexed: true }, + { name: 'text6_v2', type: 'string', isIndexed: true }, + ], + }), + ], + }) + const migrations_after = schemaMigrations({ + migrations: [ + { + toVersion: 3, + steps: [ + // can rename twice in a single step + renameColumn({ table: 'tasks', from: 'num2', to: 'num2_v2' }), + renameColumn({ table: 'tasks', from: 'num2_v2', to: 'num2_v3' }), + // can rename twice in two steps + renameColumn({ table: 'tasks', from: 'text3', to: 'text3_v2' }), + ], + }, + { + toVersion: 4, + steps: [ + // can rename twice in two steps + renameColumn({ table: 'tasks', from: 'text3_v2', to: 'text3_v3' }), + // can delete multiple columns in a step + // can delete columns in the same step as renaming + // can delete column with index + destroyColumn({ table: 'tasks', column: 'num3' }), + // destroyColumn({ table: 'tasks', column: 'num4' }), + ], + }, + { + toVersion: 5, + steps: [ + // can delete just-renamed column (don't know why you'd want to, but oh well) + renameColumn({ table: 'tasks', from: 'text4', to: 'text4_v2' }), + destroyColumn({ table: 'tasks', column: 'text4_v2' }), + // can delete a previously-renamed column + renameColumn({ table: 'tasks', from: 'text5', to: 'text5_v2' }), + // can rename a previously added column + addColumns({ + table: 'tasks', + columns: [{ name: 'text6', type: 'string', isIndexed: true }], + }), + // can destroy a previously added column + addColumns({ table: 'tasks', columns: [{ name: 'text7', type: 'string' }] }), + ], + }, + { + toVersion: 6, + steps: [ + // can delete a previously-renamed column + destroyColumn({ table: 'tasks', column: 'text5_v2' }), + // can rename a previously added column + renameColumn({ table: 'tasks', from: 'text6', to: 'text6_v2' }), + // can destroy a previously added column + destroyColumn({ table: 'tasks', column: 'text7' }), + ], + }, + ], + }) + + adapter = await adapter.testClone({ schema: testSchema_after, migrations: migrations_after }) + + // check that data was transformed correctly + expect(await adapter.find('tasks', 't1')).toEqual({ + id: 't1', + _changed: '', + _status: 'created', + num1: 110, + num2_v3: 120, + text3_v3: 'hi130', + text6_v2: '', + }) + expect(await adapter.find('tasks', 't2')).toEqual({ + id: 't2', + _changed: '', + _status: 'created', + num1: 210, + num2_v3: null, + text3_v3: 'hi230', + text6_v2: '', + }) + }) it('migrations: destroyTable', async (_adapter, AdapterClass, extraAdapterOptions) => { // initial schema const projectsSchema = { diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 208c03c39..72df52dbc 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -96,7 +96,8 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema table, column, }) => { - return `alter table "${table}" drop column "${column}";` + // We don't know if column is indexed, but if it is, we need to drop it + return `drop index if exists "${table}_${column}";alter table "${table}" drop column "${column}";` } // Requires sqlite 3.25.0 / iOS 13 / Android 11 diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 87e9ad8f1..62032b104 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -184,6 +184,7 @@ describe('encodeMigrationSteps', () => { `update "posts" set "is_pinned" = 0;` + `create index if not exists "posts_is_pinned" on "posts" ("is_pinned");` + // destroy column + `drop index if exists "posts_subtitle";` + `alter table "posts" drop column "subtitle";` + // rename column `alter table "comments" rename column "body" to "description";` + From efb9ba3202956fb3abf030ca2db1c70dbe6b2ad2 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Sat, 15 Jun 2024 13:41:01 +0200 Subject: [PATCH 22/25] fix flow --- src/adapters/sqlite/encodeSchema/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 72df52dbc..4ea753dea 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -92,7 +92,7 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ .join('') // Requires sqlite 3.35.0 / iOS 15 / Android 14 -const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema) => SQL = ({ +const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep) => SQL = ({ table, column, }) => { @@ -101,7 +101,7 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep, TableSchema } // Requires sqlite 3.25.0 / iOS 13 / Android 11 -const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep, TableSchema) => SQL = ({ +const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep) => SQL = ({ table, from, to, From 826873ab2101a60dcbd8105d5e8d50aaff573eae Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Fri, 12 Jul 2024 17:13:01 +0200 Subject: [PATCH 23/25] migrations: add unsafeSql option to renameColumn, destroyColumn, destroyTable migrations --- src/Schema/migrations/index.d.ts | 9 ++++++++ src/Schema/migrations/index.js | 15 +++++++++--- src/adapters/sqlite/encodeSchema/index.js | 15 ++++++++---- src/adapters/sqlite/encodeSchema/test.js | 28 +++++++++++++++++++++++ 4 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/Schema/migrations/index.d.ts b/src/Schema/migrations/index.d.ts index 28251560e..e2ae83d2b 100644 --- a/src/Schema/migrations/index.d.ts +++ b/src/Schema/migrations/index.d.ts @@ -24,6 +24,7 @@ export type DestroyColumnMigrationStep = $RE<{ type: 'destroy_column' table: TableName column: ColumnName + unsafeSql?: (_: string) => string }> export type RenameColumnMigrationStep = $RE<{ @@ -31,11 +32,13 @@ export type RenameColumnMigrationStep = $RE<{ table: TableName from: ColumnName to: ColumnName + unsafeSql?: (_: string) => string }> export type DestroyTableMigrationStep = $RE<{ type: 'destroy_table' table: TableName + unsafeSql?: (_: string) => string }> export type SqlMigrationStep = $RE<{ @@ -84,25 +87,31 @@ export function addColumns({ export function destroyColumn({ table, column, + unsafeSql, }: $Exact<{ table: TableName column: ColumnName + unsafeSql?: (_: string) => string }>): DestroyColumnMigrationStep export function renameColumn({ table, from, to, + unsafeSql, }: $Exact<{ table: TableName from: string to: string + unsafeSql?: (_: string) => string }>): RenameColumnMigrationStep export function destroyTable({ table, + unsafeSql, }: $Exact<{ table: TableName + unsafeSql?: (_: string) => string }>): DestroyTableMigrationStep export function unsafeExecuteSql(sql: string): SqlMigrationStep diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 1aad9b627..3ffbf2b9f 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -32,6 +32,7 @@ export type DestroyColumnMigrationStep = $RE<{ type: 'destroy_column', table: TableName, column: ColumnName, + unsafeSql?: (string) => string, }> export type RenameColumnMigrationStep = $RE<{ @@ -39,11 +40,13 @@ export type RenameColumnMigrationStep = $RE<{ table: TableName, from: ColumnName, to: ColumnName, + unsafeSql?: (string) => string, }> export type DestroyTableMigrationStep = $RE<{ type: 'destroy_table', table: TableName, + unsafeSql?: (string) => string, }> export type SqlMigrationStep = $RE<{ @@ -193,26 +196,30 @@ export function addColumns({ export function destroyColumn({ table, column, + unsafeSql, }: $Exact<{ table: TableName, column: ColumnName, + unsafeSql?: (string) => string, }>): DestroyColumnMigrationStep { if (process.env.NODE_ENV !== 'production') { invariant(table, `Missing 'table' in destroyColumn()`) invariant(column, `Missing 'column' in destroyColumn()`) } - return { type: 'destroy_column', table, column } + return { type: 'destroy_column', table, column, unsafeSql } } export function renameColumn({ table, from, to, + unsafeSql, }: $Exact<{ table: TableName, from: ColumnName, to: ColumnName, + unsafeSql?: (string) => string, }>): RenameColumnMigrationStep { if (process.env.NODE_ENV !== 'production') { invariant(table, `Missing table name in renameColumn()`) @@ -220,19 +227,21 @@ export function renameColumn({ invariant(to, `Missing 'to' in renameColumn()`) validateName(to) } - return { type: 'rename_column', table, from, to } + return { type: 'rename_column', table, from, to, unsafeSql } } export function destroyTable({ table, + unsafeSql, }: $Exact<{ table: TableName, + unsafeSql?: (string) => string, }>): DestroyTableMigrationStep { if (process.env.NODE_ENV !== 'production') { invariant(table, `Missing 'table' in destroyTable()`) } - return { type: 'destroy_table', table } + return { type: 'destroy_table', table, unsafeSql } } export function unsafeExecuteSql(sql: string): SqlMigrationStep { diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index 4ea753dea..cb61b8389 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -95,9 +95,12 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep) => SQL = ({ table, column, + unsafeSql, }) => { // We don't know if column is indexed, but if it is, we need to drop it - return `drop index if exists "${table}_${column}";alter table "${table}" drop column "${column}";` + return (unsafeSql || identity)( + `drop index if exists "${table}_${column}";alter table "${table}" drop column "${column}";`, + ) } // Requires sqlite 3.25.0 / iOS 13 / Android 11 @@ -105,12 +108,16 @@ const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep) => SQL = ({ table, from, to, + unsafeSql, }) => { - return `alter table "${table}" rename column "${from}" to "${to}";` + return (unsafeSql || identity)(`alter table "${table}" rename column "${from}" to "${to}";`) } -const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ table }) => { - return `drop table if exists "${table}";` +const encodeDestroyTableMigrationStep: (DestroyTableMigrationStep) => SQL = ({ + table, + unsafeSql, +}) => { + return (unsafeSql || identity)(`drop table if exists "${table}";`) } export const encodeMigrationSteps: (MigrationStep[]) => SQL = (steps) => diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 62032b104..33bdf586e 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -205,16 +205,44 @@ describe('encodeMigrationSteps', () => { columns: [{ name: 'body', type: 'string' }], unsafeSql: (sql) => sql.replace(/create table [^)]+\)/, '$& without rowid'), }), + destroyColumn({ + table: 'posts', + column: 'subtitle', + unsafeSql: (sql) => `${sql}foo;`, + }), + renameColumn({ + table: 'comments', + from: 'body', + to: 'description', + unsafeSql: (sql) => `${sql}bar;`, + }), + destroyTable({ + table: 'authors', + unsafeSql: (sql) => `${sql}baz;`, + }), unsafeExecuteSql('boop;'), ] expect(encodeMigrationSteps(migrationSteps)).toBe( '' + + // add columns `alter table "posts" add "subtitle";` + `update "posts" set "subtitle" = null;` + 'bla;' + + // create table `create table "comments" ("id" primary key, "_changed", "_status", "body") without rowid;` + `create index if not exists "comments__status" on "comments" ("_status");` + + // destroy column + `drop index if exists "posts_subtitle";` + + `alter table "posts" drop column "subtitle";` + + 'foo;' + + // rename column + `alter table "comments" rename column "body" to "description";` + + 'bar;' + + // destroy table + `drop table if exists "authors";` + + 'baz;' + + // unsafeExecuteSql 'boop;', ) }) From 1e84a8912021078bd1b1b1f2097cad2920efeaa0 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Fri, 12 Jul 2024 17:13:20 +0200 Subject: [PATCH 24/25] migrations: improve documentation --- CHANGELOG-Unreleased.md | 7 ++++--- docs-website/docs/docs/Advanced/Migrations.md | 7 ++++++- src/Schema/migrations/index.d.ts | 2 ++ src/Schema/migrations/index.js | 2 ++ src/adapters/sqlite/encodeSchema/index.js | 2 -- src/adapters/sqlite/encodeSchema/test.js | 3 +++ 6 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CHANGELOG-Unreleased.md b/CHANGELOG-Unreleased.md index 0b8f687c4..99a176bbf 100644 --- a/CHANGELOG-Unreleased.md +++ b/CHANGELOG-Unreleased.md @@ -9,9 +9,10 @@ ### New features - Added `Database#experimentalIsVerbose` option -- Added destroyColumn migration step -- Added renameColumn migration step -- Added destroyTable migration step +- New migration steps available: + - `destroyColumn` (see docs for limitations) + - `renameColumn` (see docs for limitations) + - `destroyTable` ### Fixes diff --git a/docs-website/docs/docs/Advanced/Migrations.md b/docs-website/docs/docs/Advanced/Migrations.md index 1a425a552..dea3b0e2a 100644 --- a/docs-website/docs/docs/Advanced/Migrations.md +++ b/docs-website/docs/docs/Advanced/Migrations.md @@ -174,7 +174,12 @@ schemaMigrations({ - `createTable({ name: 'table_name', columns: [ ... ] })` - same API as `tableSchema()` - `addColumns({ table: 'table_name', columns: [ ... ] })` - you can add one or multiple columns to an existing table. The columns table has the same format as in schema definitions -- Other types of migrations (e.g. deleting or renaming tables and columns) are not yet implemented. See [`migrations/index.js`](https://github.com/Nozbe/WatermelonDB/blob/master/src/Schema/migrations/index.js). Please contribute! +- `destroyColumn({ table: 'table_name', column: 'column_name' })` - removes column from table + - ⚠️ Important! This migration requires sqlite 3.35.0, available since iOS 15 and Android 14 (in non-JSI mode). `lokijs` adapter, Android in JSI mode, and sqlite-node are not affected by this limitation. +- `renameColumn({ table: 'table_name', from: 'old_column_name', to: 'new_column_name' })` - renames column in table + - ⚠️ Important! This migration requires sqlite 3.25.0, available since iOS 13 and Android 11 (in non-JSI mode). `lokijs` adapter, Android in JSI mode, and sqlite-node are not affected by this limitation. +- `destroyTable({ table: 'table_name' })` - removes table +- Other types of migrations (e.g. renaming tables, changing indices) are not yet implemented. See [`migrations/index.js`](https://github.com/Nozbe/WatermelonDB/blob/master/src/Schema/migrations/index.js). Please contribute! ## Database reseting and other edge cases diff --git a/src/Schema/migrations/index.d.ts b/src/Schema/migrations/index.d.ts index e2ae83d2b..4276ed28f 100644 --- a/src/Schema/migrations/index.d.ts +++ b/src/Schema/migrations/index.d.ts @@ -84,6 +84,7 @@ export function addColumns({ unsafeSql?: (_: string) => string }>): AddColumnsMigrationStep +/** Requires sqlite 3.35.0 (iOS 15 / Android 14) */ export function destroyColumn({ table, column, @@ -94,6 +95,7 @@ export function destroyColumn({ unsafeSql?: (_: string) => string }>): DestroyColumnMigrationStep +/** Requires sqlite 3.25.0 (iOS 13 / Android 11) */ export function renameColumn({ table, from, diff --git a/src/Schema/migrations/index.js b/src/Schema/migrations/index.js index 3ffbf2b9f..d718a17cb 100644 --- a/src/Schema/migrations/index.js +++ b/src/Schema/migrations/index.js @@ -193,6 +193,7 @@ export function addColumns({ return { type: 'add_columns', table, columns, unsafeSql } } +/** Requires sqlite 3.35.0 (iOS 15 / Android 14) */ export function destroyColumn({ table, column, @@ -210,6 +211,7 @@ export function destroyColumn({ return { type: 'destroy_column', table, column, unsafeSql } } +/** Requires sqlite 3.25.0 (iOS 13 / Android 11) */ export function renameColumn({ table, from, diff --git a/src/adapters/sqlite/encodeSchema/index.js b/src/adapters/sqlite/encodeSchema/index.js index cb61b8389..a57d76bc9 100644 --- a/src/adapters/sqlite/encodeSchema/index.js +++ b/src/adapters/sqlite/encodeSchema/index.js @@ -91,7 +91,6 @@ const encodeAddColumnsMigrationStep: (AddColumnsMigrationStep) => SQL = ({ }) .join('') -// Requires sqlite 3.35.0 / iOS 15 / Android 14 const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep) => SQL = ({ table, column, @@ -103,7 +102,6 @@ const encodeDestroyColumnMigrationStep: (DestroyColumnMigrationStep) => SQL = ({ ) } -// Requires sqlite 3.25.0 / iOS 13 / Android 11 const encodeRenameColumnMigrationStep: (RenameColumnMigrationStep) => SQL = ({ table, from, diff --git a/src/adapters/sqlite/encodeSchema/test.js b/src/adapters/sqlite/encodeSchema/test.js index 33bdf586e..f370b3b65 100644 --- a/src/adapters/sqlite/encodeSchema/test.js +++ b/src/adapters/sqlite/encodeSchema/test.js @@ -172,11 +172,14 @@ describe('encodeMigrationSteps', () => { expect(encodeMigrationSteps(migrationSteps)).toBe( '' + + // add columns `alter table "posts" add "subtitle";` + `update "posts" set "subtitle" = null;` + + // create table `create table "comments" ("id" primary key, "_changed", "_status", "post_id", "body");` + `create index if not exists "comments_post_id" on "comments" ("post_id");` + `create index if not exists "comments__status" on "comments" ("_status");` + + // add columns `alter table "posts" add "author_id";` + `update "posts" set "author_id" = '';` + `create index if not exists "posts_author_id" on "posts" ("author_id");` + From 8bf179380120a7b9ddd42581aed319514e277737 Mon Sep 17 00:00:00 2001 From: Radek Pietruszewski Date: Fri, 12 Jul 2024 17:13:47 +0200 Subject: [PATCH 25/25] migrations: fix getSyncChanges for renamed columns, deleted columns/tables --- src/Schema/migrations/getSyncChanges/index.js | 81 ++++++------ src/Schema/migrations/getSyncChanges/test.js | 117 ++++++++++++------ .../__tests__/synchronize-migration.test.js | 5 +- 3 files changed, 113 insertions(+), 90 deletions(-) diff --git a/src/Schema/migrations/getSyncChanges/index.js b/src/Schema/migrations/getSyncChanges/index.js index 7c4eb81b2..7cdfdb332 100644 --- a/src/Schema/migrations/getSyncChanges/index.js +++ b/src/Schema/migrations/getSyncChanges/index.js @@ -1,7 +1,5 @@ // @flow - -import { unique, groupBy, toPairs, pipe, unnest } from '../../../utils/fp' -import type { CreateTableMigrationStep, AddColumnsMigrationStep, SchemaMigrations } from '../index' +import type { SchemaMigrations } from '../index' import type { TableName, ColumnName, SchemaVersion } from '../../index' import { tableName } from '../../index' import { stepsForMigration } from '../stepsForMigration' @@ -15,13 +13,6 @@ export type MigrationSyncChanges = $Exact<{ table: TableName, columns: ColumnName[], }>[], - +renamedColumns: $Exact<{ - table: TableName, - columns: $Exact<{ - from: ColumnName, - to: ColumnName, - }>[], - }>[], }> | null export default function getSyncChanges( @@ -39,6 +30,9 @@ export default function getSyncChanges( return null } + const createdTables: Set> = new Set() + const createdColumns: Map> = new Map() + steps.forEach((step) => { invariant( [ @@ -51,48 +45,43 @@ export default function getSyncChanges( ].includes(step.type), `Unknown migration step type ${step.type}. Can not perform migration sync. This most likely means your migrations are defined incorrectly. It could also be a WatermelonDB bug.`, ) - }) - - // $FlowFixMe - const createTableSteps: CreateTableMigrationStep[] = steps.filter( - (step) => step.type === 'create_table', - ) - const createdTables = createTableSteps.map((step) => step.schema.name) - // $FlowFixMe - const addColumnSteps: AddColumnsMigrationStep[] = steps.filter( - (step) => step.type === 'add_columns', - ) - const allAddedColumns = addColumnSteps - .filter((step) => !createdTables.includes(step.table)) - .map(({ table, columns }) => columns.map(({ name }) => ({ table, name }))) - - const columnsByTable = pipe( - unnest, - groupBy(({ table }) => table), - toPairs, - )(allAddedColumns) - const addedColumns = columnsByTable.map(([table, columnDefs]) => ({ - table: tableName(table), - columns: unique(columnDefs.map(({ name }) => name)), - })) - - const allRenamedColumns = steps.filter( - (step) => step.type === 'rename_column' && !createdTables.includes(step.table), - ) + if (step.type === 'create_table') { + createdTables.add(step.schema.name) + } else if (step.type === 'add_columns') { + if (createdTables.has(step.table)) { + return + } + const columns = createdColumns.get(step.table) || new Set() + step.columns.forEach((column) => { + columns.add(column.name) + }) + createdColumns.set(step.table, columns) + } else if (step.type === 'destroy_table') { + createdTables.delete(step.table) + createdColumns.delete(step.table) + } else if (step.type === 'destroy_column') { + const columns = createdColumns.get(step.table) + if (columns) { + columns.delete(step.column) + } + } else if (step.type === 'rename_column') { + const columns = createdColumns.get(step.table) + if (columns && columns.has(step.from)) { + columns.delete(step.from) + columns.add(step.to) + } + } + }) - const renamedColumns = pipe( - groupBy(({ table }) => table), - toPairs, - )(allRenamedColumns).map(([table, columns]) => ({ + const columnsByTable = Array.from(createdColumns.entries()).map(([table, columns]) => ({ table: tableName(table), - columns: columns.map(({ from, to }) => ({ from, to })), + columns: Array.from(columns), })) return { from: fromVersion, - tables: unique(createdTables), - columns: addedColumns, - renamedColumns, + tables: Array.from(createdTables), + columns: columnsByTable, } } diff --git a/src/Schema/migrations/getSyncChanges/test.js b/src/Schema/migrations/getSyncChanges/test.js index 818e5a679..128ff8889 100644 --- a/src/Schema/migrations/getSyncChanges/test.js +++ b/src/Schema/migrations/getSyncChanges/test.js @@ -1,5 +1,13 @@ import getSyncChanges from './index' -import { schemaMigrations, createTable, addColumns, unsafeExecuteSql, renameColumn } from '../index' +import { + schemaMigrations, + createTable, + addColumns, + unsafeExecuteSql, + renameColumn, + destroyColumn, + destroyTable, +} from '../index' const createCommentsTable = createTable({ name: 'comments', @@ -25,14 +33,13 @@ describe('getSyncChanges', () => { expect(test([{ toVersion: 2, steps: [createCommentsTable] }], 2, 2)).toEqual(null) }) it('returns empty changes for empty steps', () => { - expect(testSteps([])).toEqual({ from: 1, tables: [], columns: [], renamedColumns: [] }) + expect(testSteps([])).toEqual({ from: 1, tables: [], columns: [] }) }) it('returns created tables', () => { expect(testSteps([createCommentsTable])).toEqual({ from: 1, tables: ['comments'], columns: [], - renamedColumns: [], }) }) it('returns added columns', () => { @@ -40,36 +47,6 @@ describe('getSyncChanges', () => { from: 1, tables: [], columns: [{ table: 'posts', columns: ['subtitle', 'is_pinned'] }], - renamedColumns: [], - }) - }) - it('returns renamed columns', () => { - expect(testSteps([renameColumn({ table: 'posts', from: 'body', to: 'text' })])).toEqual({ - from: 1, - tables: [], - columns: [], - renamedColumns: [{ table: 'posts', columns: [{ from: 'body', to: 'text' }] }], - }) - }) - it('combines renamed columns from multiple migration steps', () => { - expect( - testSteps([ - renameColumn({ table: 'posts', from: 'body', to: 'text' }), - renameColumn({ table: 'posts', from: 'favorite', to: 'saved' }), - ]), - ).toEqual({ - from: 1, - tables: [], - columns: [], - renamedColumns: [ - { - table: 'posts', - columns: [ - { from: 'body', to: 'text' }, - { from: 'favorite', to: 'saved' }, - ], - }, - ], }) }) it('combines added columns from multiple migration steps', () => { @@ -92,7 +69,6 @@ describe('getSyncChanges', () => { from: 1, tables: [], columns: [{ table: 'posts', columns: ['subtitle', 'is_pinned', 'author_id'] }], - renamedColumns: [], }) }) it('skips added columns for a table if it is also added', () => { @@ -108,7 +84,73 @@ describe('getSyncChanges', () => { from: 1, tables: ['comments'], columns: [], - renamedColumns: [], + }) + }) + it('skips added tables if subsequently destroyed', () => { + expect( + testSteps([ + createCommentsTable, + destroyTable({ table: 'comments' }), + destroyTable({ table: 'unrelated' }), + ]), + ).toEqual({ + from: 1, + tables: [], + columns: [], + }) + }) + it('skips added columns if subsequently destroyed', () => { + expect( + testSteps([ + addColumnsToPostsTable, + destroyColumn({ table: 'posts', column: 'is_pinned' }), + destroyColumn({ table: 'posts', column: 'unrelated' }), + destroyColumn({ table: 'unrelated', column: 'is_pinned' }), + ]), + ).toEqual({ + from: 1, + tables: [], + columns: [{ table: 'posts', columns: ['subtitle'] }], + }) + }) + it('skips added columns if parent table subsequently destroyed', () => { + expect( + testSteps([ + addColumnsToPostsTable, + destroyTable({ table: 'posts' }), + destroyTable({ table: 'unrelated' }), + ]), + ).toEqual({ + from: 1, + tables: [], + columns: [], + }) + }) + it('renames added columns if needed', () => { + expect( + testSteps([ + addColumnsToPostsTable, + renameColumn({ table: 'posts', from: 'is_pinned', to: 'is_starred' }), + // not needed + renameColumn({ table: 'posts', from: 'foo', to: 'bar' }), + ]), + ).toEqual({ + from: 1, + tables: [], + columns: [{ table: 'posts', columns: ['subtitle', 'is_starred'] }], + }) + }) + it('skips added columns if subsequently destroyed, after renaming', () => { + expect( + testSteps([ + addColumnsToPostsTable, + renameColumn({ table: 'posts', from: 'is_pinned', to: 'is_starred' }), + destroyColumn({ table: 'posts', column: 'is_starred' }), + ]), + ).toEqual({ + from: 1, + tables: [], + columns: [{ table: 'posts', columns: ['subtitle'] }], }) }) it('skips duplicates', () => { @@ -131,7 +173,6 @@ describe('getSyncChanges', () => { from: 1, tables: ['comments'], columns: [{ table: 'posts', columns: ['subtitle'] }], - renamedColumns: [], }) }) const bigMigrations = [ @@ -233,7 +274,6 @@ describe('getSyncChanges', () => { { table: 'comments', columns: ['is_pinned', 'extra'] }, { table: 'workspaces', columns: ['plan_info', 'limits'] }, ], - renamedColumns: [], }) }) it(`returns only the necessary range of migrations`, () => { @@ -244,19 +284,16 @@ describe('getSyncChanges', () => { { table: 'workspaces', columns: ['plan_info', 'limits'] }, { table: 'attachment_versions', columns: ['reactions'] }, ], - renamedColumns: [], }) expect(test(bigMigrations, 8, 10)).toEqual({ from: 8, tables: [], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], - renamedColumns: [], }) expect(test(bigMigrations, 9, 10)).toEqual({ from: 9, tables: [], columns: [], - renamedColumns: [], }) expect(test(bigMigrations, 10, 10)).toEqual(null) }) diff --git a/src/sync/impl/__tests__/synchronize-migration.test.js b/src/sync/impl/__tests__/synchronize-migration.test.js index 05d7f7098..a29fd8f61 100644 --- a/src/sync/impl/__tests__/synchronize-migration.test.js +++ b/src/sync/impl/__tests__/synchronize-migration.test.js @@ -1,6 +1,6 @@ import { mockDatabase, testSchema } from '../../../__tests__/testModels' import { expectToRejectWithMessage } from '../../../__tests__/utils' -import { schemaMigrations, createTable, addColumns, renameColumn } from '../../../Schema/migrations' +import { schemaMigrations, createTable, addColumns } from '../../../Schema/migrations' import { emptyPull } from './helpers' import { synchronize } from '../../index' @@ -17,7 +17,6 @@ describe('synchronize - migration syncs', () => { table: 'attachment_versions', columns: [{ name: 'reactions', type: 'string' }], }), - renameColumn({ table: 'post', from: 'body', to: 'text' }), ], }, { @@ -114,7 +113,6 @@ describe('synchronize - migration syncs', () => { from: 9, tables: [], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], - renamedColumns: [{ table: 'post', columns: [{ from: 'body', to: 'text' }] }], }, }) expect(await getLastPulledSchemaVersion(database)).toBe(10) @@ -137,7 +135,6 @@ describe('synchronize - migration syncs', () => { from: 8, tables: ['attachments'], columns: [{ table: 'attachment_versions', columns: ['reactions'] }], - renamedColumns: [{ table: 'post', columns: [{ from: 'body', to: 'text' }] }], }, }) expect(await getLastPulledSchemaVersion(database)).toBe(10)