diff --git a/src/backend/clients/database/SqliteDatabaseClient.test.ts b/src/backend/clients/database/SqliteDatabaseClient.test.ts index 647b468c0..715b3af8b 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.test.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.test.ts @@ -27,7 +27,7 @@ import { DatabaseClientFactory } from './index.js'; import { SqliteDatabaseClient } from './SqliteDatabaseClient.js'; /** Highest schema version the migration table can reach. */ -const CURRENT_SCHEMA_VERSION = 73; +const CURRENT_SCHEMA_VERSION = 74; /** * These suites migrate real files on disk. Idle they finish in well under a @@ -81,7 +81,23 @@ describe('SqliteDatabaseClient — boot and migrations', { timeout: DISK_MIGRATI expect(await userVersionOf(client)).toBe(CURRENT_SCHEMA_VERSION); }); - it('applies the team columns and indexes from 0077', async () => { + it('stamps a version whose migration did not run (known off-by-one)', async () => { + // Existing behaviour, not an endorsement: the stamp overshoots the + // applied work by one entry, so 70 is reported without 0074 running. + const partial = await bootClient({ targetVersion: 70 }); + try { + expect(await userVersionOf(partial)).toBe(70); + await expect( + partial.read( + "SELECT 1 FROM pragma_table_info('group') WHERE name = 'handle'", + ), + ).resolves.toEqual([]); + } finally { + partial.onServerShutdown(); + } + }); + + it('applies the team columns and indexes from 0076', async () => { const columnsOf = async (table: string) => ( (await client.read( @@ -157,6 +173,24 @@ describe('SqliteDatabaseClient — boot and migrations', { timeout: DISK_MIGRATI ).resolves.toEqual([{ handle: 'design-team' }]); }); + it('rejects a duplicate membership pair after 0078', async () => { + const [{ user_id, group_id }] = (await client.read( + 'SELECT (SELECT MIN(`id`) FROM `user`) AS user_id, ' + + '(SELECT MIN(`id`) FROM `group`) AS group_id', + )) as { user_id: number; group_id: number }[]; + + const insert = () => + client.write( + 'INSERT INTO `jct_user_group` (`user_id`, `group_id`) VALUES (?, ?)', + [user_id, group_id], + ); + + await insert(); + // `GroupStore.addUsers` has no conflict clause, so before 0075 this + // second call silently doubled every group permission the user reads. + await expect(insert()).rejects.toThrow(/UNIQUE/iu); + }); + it('runs the javascript migrations, not just the .sql ones', async () => { // The `system` user only exists because 0025 (a .dbmig.js file) ran // inside the migration VM. @@ -483,6 +517,66 @@ describe('SqliteDatabaseClient — boot and migrations', { timeout: DISK_MIGRATI } }); + it('deduplicates pre-existing membership rows when 0078 applies', async () => { + const dir = mkdtempSync(join(tmpdir(), 'puter-sqlite-dedup-')); + const path = join(dir, 'puter.sqlite'); + try { + // Stop at 67 so the duplicates exist the way a live database's do: + // written before the unique index, not after it. + const before = new SqliteDatabaseClient( + sqliteConfig({ inMemory: false, path, targetVersion: 67 }), + ); + await before.onServerStart(); + + const [{ user_id, group_id }] = (await before.read( + 'SELECT (SELECT MIN(`id`) FROM `user`) AS user_id, ' + + '(SELECT MIN(`id`) FROM `group`) AS group_id', + )) as { user_id: number; group_id: number }[]; + + for (const pair of [ + [user_id, group_id], + [user_id, group_id], + [user_id, group_id], + [user_id, group_id + 1], + ]) { + await before.write( + 'INSERT INTO `jct_user_group` (`user_id`, `group_id`) VALUES (?, ?)', + pair, + ); + } + const seeded = (await before.read( + 'SELECT `id`, `group_id` FROM `jct_user_group` ORDER BY `id`', + )) as { id: number; group_id: number }[]; + expect(seeded).toHaveLength(4); + before.onServerShutdown(); + + const after = new SqliteDatabaseClient( + sqliteConfig({ inMemory: false, path }), + ); + await after.onServerStart(); + expect(await userVersionOf(after)).toBe(CURRENT_SCHEMA_VERSION); + + // Three copies collapse to the lowest id; the distinct pair is left + // alone. Losing the higher ids costs nothing -- `addUsers` writes + // only the two id columns, so `extra` and `metadata` are NULL. + await expect( + after.read( + 'SELECT `id`, `group_id` FROM `jct_user_group` ORDER BY `id`', + ), + ).resolves.toEqual([seeded[0], seeded[3]]); + + await expect( + after.write( + 'INSERT INTO `jct_user_group` (`user_id`, `group_id`) VALUES (?, ?)', + [user_id, group_id], + ), + ).rejects.toThrow(/UNIQUE/iu); + after.onServerShutdown(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('stops early at a configured target version', async () => { const partial = await bootClient({ targetVersion: 5 }); try { diff --git a/src/backend/clients/database/SqliteDatabaseClient.ts b/src/backend/clients/database/SqliteDatabaseClient.ts index 164af0e04..338f78edd 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.ts @@ -107,6 +107,7 @@ const AVAILABLE_MIGRATIONS: [number, string[]][] = [ [70, ['0075_event-subscriptions.sql']], [71, ['0076_event-handlers.sql']], [72, ['0077_teams.sql']], + [73, ['0078_jct-user-group-pair-unique.sql']], ]; export class SqliteDatabaseClient extends AbstractDatabaseClient { diff --git a/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql b/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql new file mode 100644 index 000000000..cf4e644a0 --- /dev/null +++ b/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql @@ -0,0 +1,47 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Deduplicate then constrain. See sqlite/0075 for why this is lossless. + +-- Both steps sit behind the index check: mysql re-runs this file on every boot, and +-- guarding them together stops a rolling deploy deleting and indexing concurrently. +DROP PROCEDURE IF EXISTS _puter_add_membership_pair_index; + +DELIMITER // +CREATE PROCEDURE _puter_add_membership_pair_index() +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = 'jct_user_group' + AND INDEX_NAME = 'idx_jct_user_group_pair' + ) THEN + -- Self-join because mysql refuses `NOT IN (SELECT ... FROM jct_user_group)` (1093). + DELETE dup FROM `jct_user_group` dup + JOIN `jct_user_group` keep + ON dup.`user_id` = keep.`user_id` + AND dup.`group_id` = keep.`group_id` + AND dup.`id` > keep.`id`; + + ALTER TABLE `jct_user_group` + ADD UNIQUE INDEX `idx_jct_user_group_pair` (`user_id`, `group_id`); + END IF; +END// +DELIMITER ; + +CALL _puter_add_membership_pair_index(); +DROP PROCEDURE IF EXISTS _puter_add_membership_pair_index; diff --git a/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql b/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql new file mode 100644 index 000000000..e15c6e9f2 --- /dev/null +++ b/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql @@ -0,0 +1,34 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- Deduplicate then constrain. See sqlite/0075 for why this is lossless. + +-- Guarded because postgres re-runs every file on each boot; `to_regclass` follows +-- search_path, so it resolves under a test schema too. +DO $mig15$ +BEGIN + IF to_regclass('idx_jct_user_group_pair') IS NULL THEN + DELETE FROM jct_user_group + WHERE id NOT IN ( + SELECT MIN(id) FROM jct_user_group GROUP BY user_id, group_id + ); + END IF; +END +$mig15$; + +CREATE UNIQUE INDEX IF NOT EXISTS idx_jct_user_group_pair + ON jct_user_group (user_id, group_id); diff --git a/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql b/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql new file mode 100644 index 000000000..3d25db275 --- /dev/null +++ b/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql @@ -0,0 +1,30 @@ +-- Copyright (C) 2024-present Puter Technologies Inc. +-- +-- This file is part of Puter. +-- +-- Puter is free software: you can redistribute it and/or modify +-- it under the terms of the GNU Affero General Public License as published +-- by the Free Software Foundation, either version 3 of the License, or +-- (at your option) any later version. +-- +-- This program is distributed in the hope that it will be useful, +-- but WITHOUT ANY WARRANTY; without even the implied warranty of +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +-- GNU Affero General Public License for more details. +-- +-- You should have received a copy of the GNU Affero General Public License +-- along with this program. If not, see . + +-- `GroupStore.addUsers` had no conflict clause, so a repeat call duplicated the pair. +-- `readUserGroupPerms` joins this table on group_id alone, so each duplicate reported +-- every group permission an extra time. The delete clears what accumulated. +-- Dropping the higher id is lossless: `addUsers` writes only the two id columns, and +-- nothing references `jct_user_group.id`. + +DELETE FROM `jct_user_group` +WHERE `id` NOT IN ( + SELECT MIN(`id`) FROM `jct_user_group` GROUP BY `user_id`, `group_id` +); + +CREATE UNIQUE INDEX IF NOT EXISTS `idx_jct_user_group_pair` + ON `jct_user_group` (`user_id`, `group_id`); diff --git a/src/backend/stores/group/GroupStore.test.ts b/src/backend/stores/group/GroupStore.test.ts index 4fe04645f..e6b1e6ce4 100644 --- a/src/backend/stores/group/GroupStore.test.ts +++ b/src/backend/stores/group/GroupStore.test.ts @@ -92,6 +92,37 @@ describe('GroupStore', () => { expect(await memberUsernames(uid)).toEqual([member.username]); }); + it('treats re-adding an existing member as a no-op, not a conflict', async () => { + const uid = await seedGroup(owner.id); + + await store.addUsers(uid, [member.username]); + // Before the unique index this duplicated the row; now it would raise. + await expect( + store.addUsers(uid, [member.username]), + ).resolves.toBeUndefined(); + + expect(await memberUsernames(uid)).toEqual([member.username]); + const rows = await server.clients.db.read( + 'SELECT COUNT(*) AS n FROM `jct_user_group` ' + + 'WHERE `group_id` = (SELECT id FROM `group` WHERE uid = ?)', + [uid], + ); + expect(Number(rows[0].n)).toBe(1); + }); + + it('adds a new member alongside one that is already present', async () => { + const uid = await seedGroup(owner.id); + const second = await makeUser(); + + await store.addUsers(uid, [member.username]); + // The conflicting row must not discard the batch's other inserts. + await store.addUsers(uid, [member.username, second.username]); + + expect((await memberUsernames(uid)).sort()).toEqual( + [member.username, second.username].sort(), + ); + }); + it('ignores usernames that do not resolve to a user', async () => { const uid = await seedGroup(owner.id); await store.addUsers(uid, ['ghost-user-does-not-exist']); diff --git a/src/backend/stores/group/GroupStore.ts b/src/backend/stores/group/GroupStore.ts index de95fed43..5b8c9426c 100644 --- a/src/backend/stores/group/GroupStore.ts +++ b/src/backend/stores/group/GroupStore.ts @@ -35,16 +35,20 @@ import { PuterStore } from '../types'; export class GroupStore extends PuterStore { /** * Adds users (by username) to the group identified by `uid`. No-op if - * `usernames` is empty. + * `usernames` is empty, and for a user who is already a member. */ async addUsers(uid: string, usernames: string[]): Promise { if (usernames.length === 0) return; const placeholders = `(${usernames.map(() => '?').join(', ')})`; + // Ignore conflicts on the unique pair index from 0072; re-adding a member + // was a duplicate row before it, and would raise without this. await this.clients.db.write( - 'INSERT INTO `jct_user_group` (`user_id`, `group_id`) ' + + `${this.clients.db.insertIgnoreInto('jct_user_group')} ` + + '(`user_id`, `group_id`) ' + 'SELECT u.id, g.id FROM `user` u ' + 'JOIN (SELECT id FROM `group` WHERE uid = ?) g ON 1 = 1 ' + - `WHERE u.username IN ${placeholders}`, + `WHERE u.username IN ${placeholders}` + + this.clients.db.insertIgnoreSuffix(), [uid, ...usernames], ); }