diff --git a/server/db/purge.sql b/server/db/purge.sql index 7215d90..7b6cc48 100644 --- a/server/db/purge.sql +++ b/server/db/purge.sql @@ -21,7 +21,11 @@ -- `notification_subs` rows for `shard.*` streams and `announce_job_legs` rows -- with leg `towncrier` belong to core's tables, and a module does not delete -- from those — core prunes them when it drops the registrations, which it can --- do because it knows which registrant owned what. +-- do because it knows which registrant owned what. The two `settings` rows +-- schema.sql seeds (`game_account_signup`, `uo_link_protocol_3_migrated`) are +-- the same case with an extra reason: the second is a one-shot MIGRATION +-- marker, and deleting it would re-arm a protocol bump against tables this +-- file has just dropped. DROP TABLE IF EXISTS `shard_atlas_pending`; DROP TABLE IF EXISTS `shard_atlas_meta`; diff --git a/server/db/schema.sql b/server/db/schema.sql index 89d4bf5..ae263e4 100644 --- a/server/db/schema.sql +++ b/server/db/schema.sql @@ -614,4 +614,31 @@ ALTER TABLE uo_link_config MODIFY COLUMN protocol INT NOT NULL DEFAULT 3; -- uo_link_config row yet) it is simply written with nothing to update. UPDATE uo_link_config SET protocol = 3 WHERE id = 1 AND protocol < 3 - AND NOT EXISTS (SELECT 1 FROM settings WHERE `key` = 'uo_link_protocol_3_migrated'); \ No newline at end of file + AND NOT EXISTS (SELECT 1 FROM settings WHERE `key` = 'uo_link_protocol_3_migrated'); +-- **The marker must be written HERE, not in core.** These two statements were +-- adjacent in core's schema.sql before the extraction; slice 1 moved the UPDATE +-- and left the INSERT behind, and the two files do not run at the same time — +-- core's schema is replayed in full BEFORE any module fragment (MODULE_API.md +-- §2.6). So the marker existed before the UPDATE ever read it, the NOT EXISTS +-- was true on the first boot of a fresh install and false on every boot of an +-- upgraded one, and the one-shot could never fire. An install carrying a +-- protocol-2 row would have stayed pinned at 2 against a v3 sidecar — every +-- REST call 409, which is precisely the failure this migration exists to +-- prevent. Latent rather than live: it only bites an install that first boots a +-- post-slice-1 build while already holding a uo_link_config row, and `edge` has +-- not cut over yet. +INSERT IGNORE INTO settings (`key`, value) VALUES ('uo_link_protocol_3_migrated', '1'); + +-- ── Settings rows this module owns ───────────────────────────────────────── +-- +-- Both keys predate the module system and both name a game concept, so core +-- seeding them made core's schema declare a module's settings — the structural +-- half of what Phase 3 removes (MODULE_SYSTEM.md §2.7.1, slice 4). The KEYS are +-- deliberately unchanged: they are live rows on every existing install, and +-- renaming one would silently reset an operator's choice to the default. +-- +-- INSERT IGNORE, so an install that already carries the row keeps its value and +-- only a database that has never seen the key gets the default. Nothing in core +-- reads either one; `game_account_signup` is read through ctx.settings by +-- server/utils/gameSignup.js, which owns the policy. +INSERT IGNORE INTO settings (`key`, value) VALUES ('game_account_signup', 'disabled'); \ No newline at end of file diff --git a/server/test/schemaFragment.test.js b/server/test/schemaFragment.test.js new file mode 100644 index 0000000..cdf29c0 --- /dev/null +++ b/server/test/schemaFragment.test.js @@ -0,0 +1,157 @@ +// `schema.sql` is replayed by core on EVERY boot, and core validates it before +// anything mounts. The rules are core's (MODULE_API.md §2.6, loader.js), and are +// restated here for the same reason `manifest.test.js` restates the manifest +// rules: a mistake should fail in this repo's CI, which can say what is wrong, +// rather than on an install, where the symptom is a module that is simply absent. +// +// The test this file exists for is the ORDER one. Two statements that read each +// other were adjacent in core's schema.sql until slice 1 moved one of them here +// and left the other behind — and because core's schema is replayed in full +// before any module fragment, the marker was written before the migration that +// reads it and the one-shot could never fire. Nothing caught it: both files were +// individually valid SQL, both replayed cleanly, and the failure only shows on an +// upgraded install talking to a real sidecar. An assertion about order is the +// only thing that would have. + +const test = require('node:test') +const assert = require('node:assert') +const fs = require('node:fs') +const path = require('node:path') + +const SCHEMA = path.join(__dirname, '..', 'db', 'schema.sql') +const sql = fs.readFileSync(SCHEMA, 'utf8') + +/** + * Split into statements the way core's `utils/sqlStatements.js` does: a + * character walk, not a regexp. + * + * Comments are stripped before quotes are considered, because a comment may + * contain quotes — line 508 of this very file is `-- '' when randomised per + * activation`, and a stripper that opened a string there would swallow the rest + * of the file. The reverse case (a `--` inside a string literal) is handled by + * the same walk, since a quote opened outside a comment stays open. + */ +function splitStatements(text) { + const out = [] + let buf = '' + let quote = null + for (let i = 0; i < text.length; i++) { + const c = text[i] + if (quote) { + buf += c + if (c === '\\') { + buf += text[++i] ?? '' + } else if (c === quote) { + quote = null + } + continue + } + if (c === '-' && text[i + 1] === '-') { + while (i < text.length && text[i] !== '\n') i++ + buf += '\n' + continue + } + if (c === "'" || c === '"' || c === '`') { + quote = c + buf += c + continue + } + if (c === ';') { + if (buf.trim()) out.push(buf.trim()) + buf = '' + continue + } + buf += c + } + if (buf.trim()) out.push(buf.trim()) + return out +} + +const statements = splitStatements(sql) + +// Core's leading-verb allowlist. Not a DROP denylist: this file replays on every +// boot, so a TRUNCATE or DELETE would empty a table at each restart. +const ALLOWED = ['CREATE', 'ALTER', 'INSERT', 'UPDATE'] + +test('the splitter survives a comment that contains quotes', () => { + const parts = splitStatements("SELECT 1; -- '' a quote in a comment\nSELECT 2;") + assert.deepEqual( + parts.map((s) => s.trim().split('\n')[0].trim()), + ['SELECT 1', 'SELECT 2'], + ) +}) + +test('the splitter does not treat a -- inside a string as a comment', () => { + const parts = splitStatements("INSERT INTO t VALUES ('a--b');") + assert.equal(parts.length, 1) + assert.match(parts[0], /'a--b'/) +}) + +test('every statement leads with a verb core allows', () => { + for (const statement of statements) { + const verb = statement.trim().split(/\s+/)[0].toUpperCase() + assert.ok(ALLOWED.includes(verb), `statement leads with "${verb}": ${statement.slice(0, 70)}`) + } +}) + +test('every CREATE TABLE is IF NOT EXISTS', () => { + for (const statement of statements) { + if (!/^CREATE\s+TABLE/i.test(statement)) continue + assert.match(statement, /^CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS/i, statement.slice(0, 70)) + } +}) + +test('every table this fragment declares is prefixed shard_ or uo_link_', () => { + // The two prefixes core grandfathers to this module by name + // (loader.js LEGACY_TABLE_PREFIXES). A module written after this one prefixes + // with its own id instead. + const CREATE_TABLE = /CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\s+`?([A-Za-z0-9_]+)`?/gi + const tables = [] + for (const statement of statements) { + for (const m of statement.matchAll(CREATE_TABLE)) tables.push(m[1].toLowerCase()) + } + assert.ok(tables.length > 20, `expected the module's tables, found ${tables.length}`) + for (const table of tables) { + assert.ok( + table.startsWith('shard_') || table.startsWith('uo_link_'), + `table "${table}" carries neither grandfathered prefix`, + ) + } +}) + +// ── The settings rows this module owns ────────────────────────────────────── + +const SETTINGS_KEYS = ['game_account_signup', 'uo_link_protocol_3_migrated'] + +test('both settings seeds are INSERT IGNORE, so a replay never resets a value', () => { + for (const key of SETTINGS_KEYS) { + const seed = statements.find((s) => /^INSERT/i.test(s) && s.includes(`'${key}'`)) + assert.ok(seed, `no seed for "${key}"`) + assert.match(seed, /^INSERT\s+IGNORE\s+INTO\s+settings/i, key) + } +}) + +test('the protocol-3 marker is written AFTER the update that reads it', () => { + const update = statements.findIndex( + (s) => /^UPDATE\s+uo_link_config/i.test(s) && s.includes('uo_link_protocol_3_migrated'), + ) + const marker = statements.findIndex( + (s) => /^INSERT/i.test(s) && s.includes("'uo_link_protocol_3_migrated'"), + ) + assert.ok(update >= 0, 'the protocol-3 migration is gone') + assert.ok(marker >= 0, 'the one-shot marker is gone') + assert.ok( + marker > update, + 'the marker is written before the UPDATE reads it — the one-shot can never fire. ' + + 'This is the shape of the defect slice 1 introduced by leaving the marker in core, ' + + 'whose schema replays first.', + ) +}) + +test('the migration is guarded on the marker, not on the column value alone', () => { + const update = statements.find((s) => /^UPDATE\s+uo_link_config/i.test(s)) + assert.match(update, /NOT\s+EXISTS\s*\(\s*SELECT/i) + // Without the guard an operator who deliberately pins an older sidecar in + // Admin → Shard is silently re-bumped on the next restart. + assert.match(update, /uo_link_protocol_3_migrated/) +})