From fb3d0b08b2dc34f7e7dab7da754a3522c52a9eb1 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:51:09 +0200 Subject: [PATCH] fix(events): make event_date/expires_at nullable on SQLite (#1029) (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing a gallery's expiration failed on every SQLite install with SQLITE_CONSTRAINT: NOT NULL constraint failed: events.expires_at surfacing in the admin UI as "Failed to update event". Migration 061 added the event_require_event_date / event_require_expiration settings and dropped the NOT NULL on both columns — but only for Postgres. It skipped SQLite on the premise that "SQLite doesn't enforce NOT NULL as strictly", which is untrue, so "never expires" was never reachable there. The #426 work that allows clearing the expiration on edit therefore never worked on SQLite either. Migration 174 finishes 061 for SQLite. Knex implements .alter() on SQLite by recreating the table; migration 073 already does that on `events`, so the path is well-trodden. Postgres is skipped — it was handled in 061 and .alter() there would needlessly rewrite a column that is already correct. The test asserts against the real engine (the Jest harness runs SQLite) and reproduces the reporter's exact error without the migration. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM Co-authored-by: Paul Nothaft --- .../utils/sqliteNullableEventDates.test.js | 106 ++++++++++++++++++ .../core/174_sqlite_nullable_event_dates.js | 42 +++++++ 2 files changed, 148 insertions(+) create mode 100644 backend/__tests__/utils/sqliteNullableEventDates.test.js create mode 100644 backend/migrations/core/174_sqlite_nullable_event_dates.js diff --git a/backend/__tests__/utils/sqliteNullableEventDates.test.js b/backend/__tests__/utils/sqliteNullableEventDates.test.js new file mode 100644 index 00000000..de71f7bb --- /dev/null +++ b/backend/__tests__/utils/sqliteNullableEventDates.test.js @@ -0,0 +1,106 @@ +/** + * Regression test for clearing an event's expiration on SQLite (#1029). + * + * Migration 061 dropped the NOT NULL on events.event_date / events.expires_at + * for Postgres only — it skipped SQLite on the (wrong) premise that SQLite + * doesn't enforce NOT NULL. It does, so every SQLite install answered + * + * SQLITE_CONSTRAINT: NOT NULL constraint failed: events.expires_at + * + * when an admin cleared the expiration, surfacing as "Failed to update event". + * Migration 174 finishes the job. The harness runs on SQLite, so this asserts + * the real engine behaviour rather than a mock. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-nullable-dates-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'nullable-dates-test-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +let db; +let cleanup; +let eventId; + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + const inserted = await db('events').insert({ + slug: 'nullable-dates-test', + event_type: 'wedding', + event_name: 'Nullable Dates Test', + event_date: '2026-06-22', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: '/gallery/nullable-dates-test/share', + share_token: 'nullable-dates-share', + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + eventId = inserted[0]?.id ?? inserted[0]; +}, 120000); + +afterAll(async () => { if (cleanup) await cleanup(); }); + +describe('events date columns are nullable on SQLite (#1029)', () => { + test('the engine under test really is SQLite', () => { + expect(['sqlite3', 'better-sqlite3']).toContain(db.client.config.client); + }); + + test('clearing expires_at succeeds — this threw SQLITE_CONSTRAINT before migration 174', async () => { + await db('events').where('id', eventId).update({ expires_at: null }); + const row = await db('events').where('id', eventId).first('expires_at'); + expect(row.expires_at).toBeNull(); + }); + + test('clearing event_date succeeds too (061 covered both columns on PG)', async () => { + await db('events').where('id', eventId).update({ event_date: null }); + const row = await db('events').where('id', eventId).first('event_date'); + expect(row.event_date).toBeNull(); + }); + + test('a gallery can be created with no expiration at all', async () => { + const inserted = await db('events').insert({ + slug: 'never-expires-test', + event_type: 'other', + event_name: 'Never Expires', + event_date: null, + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: '/gallery/never-expires-test/share', + share_token: 'never-expires-share', + expires_at: null, + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + const id = inserted[0]?.id ?? inserted[0]; + const row = await db('events').where('id', id).first('expires_at', 'event_date'); + expect(row.expires_at).toBeNull(); + expect(row.event_date).toBeNull(); + }); + + test('columns the events table depends on survived the table rebuild', async () => { + // Knex implements .alter() on SQLite by recreating the table; make sure the + // rebuild kept the row and the wider schema intact. + const row = await db('events').where('id', eventId).first(); + expect(row.slug).toBe('nullable-dates-test'); + expect(row.share_token).toBe('nullable-dates-share'); + expect(await db.schema.hasColumn('events', 'allow_downloads')).toBe(true); + expect(await db.schema.hasColumn('events', 'hero_photo_id')).toBe(true); + const photos = await db('photos').where('event_id', eventId); + expect(Array.isArray(photos)).toBe(true); + }); +}); diff --git a/backend/migrations/core/174_sqlite_nullable_event_dates.js b/backend/migrations/core/174_sqlite_nullable_event_dates.js new file mode 100644 index 00000000..63e014ce --- /dev/null +++ b/backend/migrations/core/174_sqlite_nullable_event_dates.js @@ -0,0 +1,42 @@ +/** + * Migration 174: make events.event_date / events.expires_at nullable on SQLite (#1029). + * + * Migration 061 introduced the `event_require_event_date` / + * `event_require_expiration` settings and dropped the NOT NULL on both columns + * — but only for Postgres. It skipped SQLite on the premise that "SQLite + * doesn't enforce NOT NULL as strictly", which is simply untrue: clearing the + * expiration on a SQLite install fails with + * + * SQLITE_CONSTRAINT: NOT NULL constraint failed: events.expires_at + * + * so "never expires" has never been reachable there. This finishes 061 for + * SQLite. Knex implements .alter() on SQLite by recreating the table; migration + * 073 already does exactly that on `events`, so the path is well-trodden here. + * + * Postgres is skipped — 061 already handled it, and knex's .alter() rewrites + * the whole column definition (type, default, nullability), which would be a + * needless rewrite of a column that is already correct. + */ + +function isSqlite(knex) { + const client = knex.client.config.client; + return client === 'sqlite3' || client === 'better-sqlite3'; +} + +exports.up = async function(knex) { + if (!isSqlite(knex)) return; + + const hasEvents = await knex.schema.hasTable('events'); + if (!hasEvents) return; + + await knex.schema.alterTable('events', (table) => { + table.datetime('event_date').nullable().alter(); + table.datetime('expires_at').nullable().alter(); + }); +}; + +exports.down = async function(knex) { + // Deliberately irreversible. Restoring NOT NULL would fail on any install + // that has since created a gallery without an expiration — exactly what this + // migration enables — and 061's down() takes the same position for Postgres. +};