fix: deduplicate jct_user_group and make membership writes unique

\`jct_user_group\` had no unique constraint on (user_id, group_id) and
\`GroupStore.addUsers\` had no conflict clause, so re-adding a member
inserted a second row. \`readUserGroupPerms\` joins the junction table on
group_id alone, so each duplicate returned another copy of every group
permission the user holds.

Deduplicate keeping the lowest id, add the unique pair index, and make
\`addUsers\` ignore conflicts via the existing \`insertIgnoreInto\` helpers --
without that last part the index turns a re-add into a raised error, which
five call sites would log as a failed signup step.

mysql cannot delete from a table it reads in a subquery (error 1093), so
it uses a self-join with the same lowest-id-wins semantics.
This commit is contained in:
Juan Castro
2026-09-03 14:32:30 -04:00
parent abb426a717
commit c1c1588939
7 changed files with 234 additions and 4 deletions
@@ -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
@@ -157,6 +157,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 +501,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 {
@@ -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 {
@@ -0,0 +1,49 @@
-- 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 <https://www.gnu.org/licenses/>.
-- Deduplicate then constrain. See sqlite/0072 for why duplicates exist and why
-- dropping the higher-id row is lossless.
-- Self-join rather than `id NOT IN (SELECT MIN(id) ... FROM jct_user_group)`,
-- which mysql refuses with error 1093 -- it will not read the table it deletes
-- from in a subquery. Keeps the lowest id of each pair.
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`;
-- Guarded because mysql tracks no applied-state per file, so this may re-run.
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
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;
@@ -0,0 +1,28 @@
-- 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 <https://www.gnu.org/licenses/>.
-- Deduplicate then constrain. See sqlite/0072 for why duplicates exist and why
-- dropping the higher-id row is lossless.
-- Idempotent: the delete is a no-op once unique, and the index guards itself.
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);
@@ -0,0 +1,36 @@
-- 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 <https://www.gnu.org/licenses/>.
-- `GroupStore.addUsers` is an INSERT ... SELECT with no conflict clause, so a
-- repeated call inserts a second row for the same (user_id, group_id).
--
-- Each duplicate multiplies every row `readUserGroupPerms` returns, because it
-- joins this table on group_id alone -- two membership rows means every group
-- permission is reported twice. The index below stops that recurring; the
-- delete clears what already accumulated.
--
-- Dropping the higher-id row discards nothing: `addUsers` writes only the two
-- id columns, so `extra` and `metadata` are NULL on every row here, nothing has
-- a foreign key to `jct_user_group.id`, and no code reads it.
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`);
@@ -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']);
+10 -3
View File
@@ -35,16 +35,23 @@ 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<void> {
if (usernames.length === 0) return;
const placeholders = `(${usernames.map(() => '?').join(', ')})`;
// Ignore conflicts on the (user_id, group_id) unique index added in
// 0072. Re-adding a member was previously a silent duplicate row, which
// is what that index exists to stop -- without this it becomes a raised
// error instead, and every caller here treats a throw as a failed
// signup step worth warning about.
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],
);
}