From 7bbba535445799f4d49016221257817f73a39fea Mon Sep 17 00:00:00 2001 From: Kevin Date: Tue, 18 Jun 2024 16:28:15 +0200 Subject: [PATCH] feat (client): renamed disableFKs flag and modified default to disable FKs on SQLite (#1376) Decided to go for an enumeration as this is more explicit than the `boolean | undefined`: ```ts enum ForeignKeyChecks { enabled = 'enabled', disabled = 'disabled', inherit = 'inherit', } ``` --- .changeset/fast-fishes-hunt.md | 5 +++ clients/typescript/src/config/index.ts | 22 ++++++++--- clients/typescript/src/migrators/bundle.ts | 9 +++-- clients/typescript/src/migrators/index.ts | 5 ++- clients/typescript/src/migrators/mock.ts | 5 ++- clients/typescript/src/satellite/config.ts | 12 +++--- clients/typescript/src/satellite/process.ts | 17 +++++---- clients/typescript/src/satellite/registry.ts | 2 +- clients/typescript/src/util/transactions.ts | 16 ++++---- clients/typescript/test/satellite/process.ts | 8 ++++ .../test/satellite/registry.test.ts | 6 ++- .../typescript/test/util/transactions.test.ts | 38 +++++++++---------- 12 files changed, 89 insertions(+), 56 deletions(-) create mode 100644 .changeset/fast-fishes-hunt.md diff --git a/.changeset/fast-fishes-hunt.md b/.changeset/fast-fishes-hunt.md new file mode 100644 index 0000000000..07526d0a0c --- /dev/null +++ b/.changeset/fast-fishes-hunt.md @@ -0,0 +1,5 @@ +--- +"electric-sql": patch +--- + +Modify FK flag option to default to disabling FK checks on SQLite. diff --git a/clients/typescript/src/config/index.ts b/clients/typescript/src/config/index.ts index 43ba5cfe18..49cea81af1 100644 --- a/clients/typescript/src/config/index.ts +++ b/clients/typescript/src/config/index.ts @@ -43,10 +43,17 @@ export interface ElectricConfig { */ connectionBackOffOptions?: ConnectionBackOffOptions /** - * Whether to disable FK checks when applying downstream (i.e. incoming) transactions to the local SQLite database. - * When using Postgres, this is the default behavior and can't be changed. + * Whether to check foreign keys when applying downstream (i.e. incoming) transactions to the local SQLite database. + * Defaults to `disabled`, meaning that FKs are not checked. + * When using Postgres, this option cannot be changed. */ - disableForeignKeysDownstream?: boolean + foreignKeyChecksDownstream?: ForeignKeyChecks +} + +export enum ForeignKeyChecks { + enabled = 'enabled', + disabled = 'disabled', + inherit = 'inherit', } export type ElectricConfigWithDialect = ElectricConfig & { @@ -66,7 +73,7 @@ export type HydratedConfig = { debug: boolean connectionBackOffOptions: ConnectionBackOffOptions namespace: string - disableFKs: boolean | undefined + fkChecks: ForeignKeyChecks } export type InternalElectricConfig = { @@ -79,7 +86,7 @@ export type InternalElectricConfig = { } debug?: boolean connectionBackOffOptions?: ConnectionBackOffOptions - disableFKs?: boolean + fkChecks: ForeignKeyChecks } export const hydrateConfig = ( @@ -99,6 +106,9 @@ export const hydrateConfig = ( const defaultNamespace = config.dialect === 'Postgres' ? 'public' : 'main' + const fkChecks = + config.foreignKeyChecksDownstream ?? ForeignKeyChecks.disabled + const replication = { host: url.hostname, port: port, @@ -133,6 +143,6 @@ export const hydrateConfig = ( debug, connectionBackOffOptions, namespace: defaultNamespace, - disableFKs: config.disableForeignKeysDownstream, + fkChecks, } } diff --git a/clients/typescript/src/migrators/bundle.ts b/clients/typescript/src/migrators/bundle.ts index a2aca721d5..b56468e6ea 100644 --- a/clients/typescript/src/migrators/bundle.ts +++ b/clients/typescript/src/migrators/bundle.ts @@ -13,6 +13,7 @@ import { _electric_migrations } from '../satellite/config' import { pgBuilder, QueryBuilder, sqliteBuilder } from './query-builder' import { dedent } from 'ts-dedent' import { runInTransaction } from '../util/transactions' +import { ForeignKeyChecks } from '../config' export const SCHEMA_VSN_ERROR_MSG = `Local schema doesn't match server's. Clear local state through developer tools and retry connection manually. If error persists, re-generate the client. Check documentation (https://electric-sql.com/docs/reference/roadmap) to learn more.` @@ -133,7 +134,7 @@ export abstract class BundleMigratorBase implements Migrator { async apply( { statements, version }: StmtMigration, - disableFKs?: boolean + fkChecks: ForeignKeyChecks = ForeignKeyChecks.inherit ): Promise { if (!VALID_VERSION_EXP.test(version)) { throw new Error( @@ -141,7 +142,7 @@ export abstract class BundleMigratorBase implements Migrator { ) } - await runInTransaction(this.adapter, disableFKs, ...statements, { + await runInTransaction(this.adapter, fkChecks, ...statements, { sql: dedent` INSERT INTO ${this.migrationsTable} (version, applied_at) VALUES (${this.queryBuilder.makePositionalParam( @@ -160,7 +161,7 @@ export abstract class BundleMigratorBase implements Migrator { */ async applyIfNotAlready( migration: StmtMigration, - disableFKs: boolean | undefined + fkChecks: ForeignKeyChecks = ForeignKeyChecks.inherit ): Promise { const rows = await this.adapter.query({ sql: dedent` @@ -175,7 +176,7 @@ export abstract class BundleMigratorBase implements Migrator { if (shouldApply) { // This is a new migration because its version number // is not in our migrations table. - await this.apply(migration, disableFKs) + await this.apply(migration, fkChecks) } return shouldApply diff --git a/clients/typescript/src/migrators/index.ts b/clients/typescript/src/migrators/index.ts index 61df58bf75..4ce059f2d5 100644 --- a/clients/typescript/src/migrators/index.ts +++ b/clients/typescript/src/migrators/index.ts @@ -1,3 +1,4 @@ +import { ForeignKeyChecks } from '../config' import { Statement } from '../util' import { QueryBuilder } from './query-builder' @@ -29,10 +30,10 @@ export function makeStmtMigration(migration: Migration): StmtMigration { export interface Migrator { up(): Promise - apply(migration: StmtMigration, disableFKs?: boolean): Promise + apply(migration: StmtMigration, fkChecks: ForeignKeyChecks): Promise applyIfNotAlready( migration: StmtMigration, - disableFKs: boolean | undefined + fkChecks: ForeignKeyChecks ): Promise querySchemaVersion(): Promise queryBuilder: QueryBuilder diff --git a/clients/typescript/src/migrators/mock.ts b/clients/typescript/src/migrators/mock.ts index 029b0c33c0..2f5237f628 100644 --- a/clients/typescript/src/migrators/mock.ts +++ b/clients/typescript/src/migrators/mock.ts @@ -1,3 +1,4 @@ +import { ForeignKeyChecks } from '../config' import { Migrator, StmtMigration } from './index' import { QueryBuilder } from './query-builder' @@ -8,13 +9,13 @@ export class MockMigrator implements Migrator { return 1 } - async apply(_: StmtMigration, _disableFks?: boolean): Promise { + async apply(_: StmtMigration, _fkChecks: ForeignKeyChecks): Promise { return } async applyIfNotAlready( _: StmtMigration, - _disableFks: boolean | undefined + _fkChecks: ForeignKeyChecks ): Promise { return true } diff --git a/clients/typescript/src/satellite/config.ts b/clients/typescript/src/satellite/config.ts index 5a90946636..a6f903d51a 100644 --- a/clients/typescript/src/satellite/config.ts +++ b/clients/typescript/src/satellite/config.ts @@ -1,5 +1,6 @@ import { IBackOffOptions } from 'exponential-backoff' import { QualifiedTablename } from '../util/tablename' +import { ForeignKeyChecks } from '../config' export type ConnectionBackoffOptions = Omit export interface SatelliteOpts { @@ -25,11 +26,12 @@ export interface SatelliteOpts { /** Backoff options for connecting with Electric*/ connectionBackOffOptions: ConnectionBackoffOptions /** - * Whether to disable FK checks when applying incoming (i.e. remote) transactions to the local SQLite database. - * When using Postgres, this is the default behavior and can't be changed. - * If this flag is undefined and we're running on SQLite, then the FK pragma is left untouched. + * Whether to enable or disable FK checks when applying incoming (i.e. remote) transactions to the local SQLite database. + * When set to `inherit` the FK pragma is left untouched. + * This option defaults to `disable` which disables FK checks on incoming transactions. + * This option only affects FK checks on SQLite databases and should not be modified when using Postgres. */ - disableFKs: boolean | undefined + fkChecks: ForeignKeyChecks /** With debug mode enabled, Satellite can show additional logs. */ debug: boolean } @@ -76,7 +78,7 @@ export const satelliteDefaults: (namespace: string) => SatelliteOpts = ( numOfAttempts: 50, timeMultiple: 2, }, - disableFKs: undefined, + fkChecks: ForeignKeyChecks.disabled, debug: false, } } diff --git a/clients/typescript/src/satellite/process.ts b/clients/typescript/src/satellite/process.ts index 6ab6250c15..6b4737f2a0 100644 --- a/clients/typescript/src/satellite/process.ts +++ b/clients/typescript/src/satellite/process.ts @@ -79,6 +79,7 @@ import groupBy from 'lodash.groupby' import { ShapeManager } from './shapes/shapeManager' import { SyncStatus } from '../client/model/shapes' import { runInTransaction } from '../util/transactions' +import { ForeignKeyChecks } from '../config' type ChangeAccumulator = { [key: string]: Change @@ -125,7 +126,7 @@ export class SatelliteProcess implements Satellite { builder: QueryBuilder opts: SatelliteOpts - disableFKs: boolean | undefined + fkChecks: ForeignKeyChecks _authState?: AuthState _unsubscribeFromAuthState?: UnsubscribeFunction @@ -175,8 +176,10 @@ export class SatelliteProcess implements Satellite { this.builder = this.migrator.queryBuilder this.opts = opts - this.disableFKs = - this.builder.dialect === 'SQLite' ? this.opts.disableFKs : undefined + this.fkChecks = + this.builder.dialect === 'SQLite' + ? this.opts.fkChecks + : ForeignKeyChecks.inherit this.relations = {} this.previousShapeSubscriptions = [] @@ -695,11 +698,11 @@ export class SatelliteProcess implements Satellite { } /** - * Runs the provided statements in a transaction and disables FK checks if `disableFKs` is true. - * `disableFKs` should only be set to true when using SQLite as we already disable FK checks for incoming TXs when using Postgres + * Runs the provided statements in a transaction and disables FK checks if `this.fkChecks` is set to `disabled`. + * `this.fkChecks` should only be set to true when using SQLite as we already disable FK checks for incoming TXs when using Postgres */ async runInTransaction(...stmts: Statement[]) { - return runInTransaction(this.adapter, this.disableFKs, ...stmts) + return runInTransaction(this.adapter, this.fkChecks, ...stmts) } _resetClientState(opts?: { keepSubscribedShapes: boolean }): Promise { @@ -1515,7 +1518,7 @@ export class SatelliteProcess implements Satellite { statements: allStatements, version: transaction.migrationVersion, }, - this.disableFKs + this.fkChecks ) } else { await this.runInTransaction(...allStatements) diff --git a/clients/typescript/src/satellite/registry.ts b/clients/typescript/src/satellite/registry.ts index 91ff7c6089..331e7956c6 100644 --- a/clients/typescript/src/satellite/registry.ts +++ b/clients/typescript/src/satellite/registry.ts @@ -220,7 +220,7 @@ export class GlobalRegistry extends BaseRegistry { const satelliteOpts: SatelliteOpts = { ...satelliteDefaults(config.namespace), connectionBackOffOptions: config.connectionBackOffOptions, - disableFKs: config.disableFKs, + fkChecks: config.fkChecks, debug: config.debug, } diff --git a/clients/typescript/src/util/transactions.ts b/clients/typescript/src/util/transactions.ts index f80202ce7b..6da4518cf2 100644 --- a/clients/typescript/src/util/transactions.ts +++ b/clients/typescript/src/util/transactions.ts @@ -1,24 +1,26 @@ import { Statement } from '.' +import { ForeignKeyChecks } from '../config' import { DatabaseAdapter, RunResult } from '../electric' /** - * Runs the provided statements in a transaction and disables FK checks if `disableFKs` is true. - * FK checks are enabled if `disableFKs` is false. - * FK checks are left untouched if `disableFKs` is undefined. - * `disableFKs` should only be set to true when using SQLite as we already disable FK checks for incoming TXs when using Postgres, + * Runs the provided statements in a transaction and sets the `foreign_keys` pragma based on the `fkChecks` flag. + * FK checks are enabled if `fkChecks` is `ForeignKeyChecks.enabled`. + * FK checks are disabled if `fkChecks` is `ForeignKeyChecks.disabled`. + * FK checks are left untouched if `fkChecks` is `ForeignKeyChecks.inherit`. + * `fkChecks` should only be set to `ForeignKeyChecks.disabled` when using SQLite as we already disable FK checks for incoming TXs when using Postgres, * so the executed SQL code to disable FKs is for SQLite dialect only. */ export async function runInTransaction( adapter: DatabaseAdapter, - disableFKs: boolean | undefined, + fkChecks: ForeignKeyChecks, ...stmts: Statement[] ): Promise { - if (disableFKs === undefined) { + if (fkChecks === ForeignKeyChecks.inherit) { // don't touch the FK pragma return adapter.runInTransaction(...stmts) } - const desiredPragma = disableFKs ? 0 : 1 + const desiredPragma = fkChecks === ForeignKeyChecks.disabled ? 0 : 1 return adapter.runExclusively(async (uncoordinatedAdapter) => { const [{ foreign_keys: originalPragma }] = await uncoordinatedAdapter.query( diff --git a/clients/typescript/test/satellite/process.ts b/clients/typescript/test/satellite/process.ts index 05adb245cc..e65cb289ff 100644 --- a/clients/typescript/test/satellite/process.ts +++ b/clients/typescript/test/satellite/process.ts @@ -53,6 +53,7 @@ import { } from '../../src/notifiers' import { QueryBuilder } from '../../src/migrators/query-builder' import { SatelliteOpts } from '../../src/satellite/config' +import { ForeignKeyChecks } from '../../src/config' export type ContextType = CommonContextType & { builder: QueryBuilder @@ -1164,6 +1165,7 @@ export const processTests = (test: TestFn) => { await runMigrations() if (builder.dialect === 'SQLite') { + satellite.fkChecks = ForeignKeyChecks.inherit // set FK checks to inherit because by default they are disabled await adapter.run({ sql: `PRAGMA foreign_keys = ON` }) } await satellite._setMeta('compensations', 0) @@ -1297,6 +1299,7 @@ export const processTests = (test: TestFn) => { await runMigrations() if (builder.dialect === 'SQLite') { + satellite.fkChecks = ForeignKeyChecks.inherit // set FK checks to inherit because by default they are disabled await adapter.run({ sql: `PRAGMA foreign_keys = ON` }) } await satellite._setMeta('compensations', 0) @@ -1966,6 +1969,11 @@ export const processTests = (test: TestFn) => { await runMigrations() + if (builder.dialect === 'SQLite') { + satellite.fkChecks = ForeignKeyChecks.inherit // set FK checks to inherit because by default they are disabled + await adapter.run({ sql: `PRAGMA foreign_keys = ON` }) + } + const tablename = 'child' // relations must be present at subscription delivery diff --git a/clients/typescript/test/satellite/registry.test.ts b/clients/typescript/test/satellite/registry.test.ts index 7cf0e8019e..f8c5d1c8df 100644 --- a/clients/typescript/test/satellite/registry.test.ts +++ b/clients/typescript/test/satellite/registry.test.ts @@ -1,6 +1,9 @@ import test from 'ava' -import { InternalElectricConfig } from '../../src/config/index' +import { + ForeignKeyChecks, + InternalElectricConfig, +} from '../../src/config/index' import { DatabaseAdapter } from '../../src/electric/adapter' import { Migrator } from '../../src/migrators/index' import { Notifier } from '../../src/notifiers/index' @@ -18,6 +21,7 @@ const notifier = {} as Notifier const socketFactory = {} as SocketFactory const config: InternalElectricConfig = { auth: {}, + fkChecks: ForeignKeyChecks.inherit, } const args = [ dbName, diff --git a/clients/typescript/test/util/transactions.test.ts b/clients/typescript/test/util/transactions.test.ts index 17c597d63b..d4714fa195 100644 --- a/clients/typescript/test/util/transactions.test.ts +++ b/clients/typescript/test/util/transactions.test.ts @@ -4,6 +4,7 @@ import type { Database as DB } from 'better-sqlite3' import { DatabaseAdapter } from '../../src/drivers/better-sqlite3/adapter' import { DatabaseAdapter as DatabaseAdapterInterface } from '../../src/electric' import { runInTransaction } from '../../src/util/transactions' +import { ForeignKeyChecks } from '../../src/config' interface Context { adapter: DatabaseAdapterInterface @@ -36,14 +37,13 @@ test.afterEach.always(async (t) => { db.close() }) -test('runInTransaction disables FK checks when flag is set to true', async (t) => { +test('runInTransaction disables FK checks when flag is set to disabled', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = ON;' }) // Should succeed even though the FK is not valid - // because we pass `true` for the `disableFKs` flag - const res = await runInTransaction(adapter, true, { + const res = await runInTransaction(adapter, ForeignKeyChecks.disabled, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", }) @@ -61,14 +61,13 @@ test('runInTransaction disables FK checks when flag is set to true', async (t) = t.is(foreign_keys, 1) }) -test('runInTransaction disables FK checks when flag is set to true and FK pragma is already disabled', async (t) => { +test('runInTransaction disables FK checks when flag is set to disabled and FK pragma is already disabled', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = OFF;' }) // Should succeed even though the FK is not valid - // because we pass `true` for the `disableFKs` flag - const res = await runInTransaction(adapter, true, { + const res = await runInTransaction(adapter, ForeignKeyChecks.disabled, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", }) @@ -86,15 +85,14 @@ test('runInTransaction disables FK checks when flag is set to true and FK pragma t.is(foreign_keys, 0) }) -test('runInTransaction enables FK checks when flag is set to false', async (t) => { +test('runInTransaction enables FK checks when flag is set to enabled', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = OFF;' }) // Should fail because the FK is not valid - // because we pass `false` for the `disableFKs` flag await t.throwsAsync( - runInTransaction(adapter, false, { + runInTransaction(adapter, ForeignKeyChecks.enabled, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", }), { message: /FOREIGN KEY constraint failed/ } @@ -106,7 +104,7 @@ test('runInTransaction enables FK checks when flag is set to false', async (t) = // Now insert a parent row and a child row pointing to the parent await runInTransaction( adapter, - false, + ForeignKeyChecks.enabled, { sql: "INSERT INTO parent (pid) VALUES ('p1');" }, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');" } ) @@ -129,15 +127,14 @@ test('runInTransaction enables FK checks when flag is set to false', async (t) = t.is(foreign_keys, 0) }) -test('runInTransaction enables FK checks when flag is set to false and pragma is already enabled', async (t) => { +test('runInTransaction enables FK checks when flag is set to enabled and pragma is already enabled', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = ON;' }) // Should fail because the FK is not valid - // because we pass `false` for the `disableFKs` flag await t.throwsAsync( - runInTransaction(adapter, false, { + runInTransaction(adapter, ForeignKeyChecks.enabled, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", }), { message: /FOREIGN KEY constraint failed/ } @@ -149,7 +146,7 @@ test('runInTransaction enables FK checks when flag is set to false and pragma is // Now insert a parent row and a child row pointing to the parent await runInTransaction( adapter, - false, + ForeignKeyChecks.enabled, { sql: "INSERT INTO parent (pid) VALUES ('p1');" }, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');" } ) @@ -172,15 +169,14 @@ test('runInTransaction enables FK checks when flag is set to false and pragma is t.is(foreign_keys, 1) }) -test('runInTransaction does not touch enabled FK pragma when flag is undefined', async (t) => { +test('runInTransaction does not touch enabled FK pragma when flag is inherit', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = ON;' }) // Should fail because the FK is not valid - // because we pass `false` for the `disableFKs` flag await t.throwsAsync( - runInTransaction(adapter, undefined, { + runInTransaction(adapter, ForeignKeyChecks.inherit, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", }), { message: /FOREIGN KEY constraint failed/ } @@ -198,7 +194,7 @@ test('runInTransaction does not touch enabled FK pragma when flag is undefined', // Now insert a parent row and a child row pointing to the parent await runInTransaction( adapter, - undefined, + ForeignKeyChecks.inherit, { sql: "INSERT INTO parent (pid) VALUES ('p1');" }, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');" } ) @@ -221,16 +217,16 @@ test('runInTransaction does not touch enabled FK pragma when flag is undefined', t.is(fk2, 1) }) -test('runInTransaction does not touch disabled FK pragma when flag is undefined', async (t) => { +test('runInTransaction does not touch disabled FK pragma when flag is inherit', async (t) => { const { adapter } = t.context adapter.run({ sql: 'PRAGMA foreign_keys = OFF;' }) // Should succeed even though the FK is not valid // because we disabled the FK pragma - // and passed `undefined` for the `disableFKs` flag + // and passed `inherit` for the `fkChecks` flag // which means the FK pragma is used as is - const res = await runInTransaction(adapter, undefined, { + const res = await runInTransaction(adapter, ForeignKeyChecks.inherit, { sql: "INSERT INTO child (cid, p) VALUES ('c1', 'p1');", })