Prevent duplicate app permission entries from being saved
Original task (#91): The `app_permissions` join table had no unique constraint, allowing the same `(app_id, permission_id)` pair to be inserted repeatedly. The Admin Panel restriction had grown to 24 duplicate rows. This change prevents that recurring at the DB level and verifies that the existing conflict-safe insert path keeps working. Schema change: - `lib/db/src/schema/apps.ts`: added a composite primary key on `(app_id, permission_id)` for `appPermissionsTable`, matching the pattern used by other join tables in this repo (rolePermissions, userRoles, groupApps, etc.). This is enforced at the database level. DB migration: - Cleaned up the 23 duplicate rows still present in the DB before applying the constraint (kept the earliest row per pair using `ctid`), then ran `pnpm --filter @workspace/db run push` to sync the schema. Verified the new primary key `app_permissions_app_id_permission_id_pk` rejects duplicate inserts. Graceful handling of duplicates: - The seed script (`scripts/src/seed.ts`) already uses `.onConflictDoNothing()` when inserting into `app_permissions`. With the new primary key, that call is now properly idempotent (no longer silently allowing duplicates). - There is currently no HTTP route or admin UI that POSTs into `app_permissions` (the task description listed `routes/apps.ts` as a relevant file, but no such handler exists today). The constraint itself is what prevents future regressions, and any future endpoint should follow the seed's `.onConflictDoNothing()` pattern. Tests: - Added `artifacts/api-server/tests/app-permissions-unique.test.mjs` with two cases that prove the new behavior: 1. A plain duplicate INSERT fails with SQLSTATE 23505 (unique violation) and only one row remains. 2. `INSERT ... ON CONFLICT DO NOTHING` (the pattern Drizzle's `.onConflictDoNothing()` emits) handles duplicates gracefully: no error thrown, `rowCount` is 0, and exactly one row remains. - All previously-passing api-server tests for apps/groups still pass after the schema change. Replit-Task-Id: 0589a4dc-5898-4c66-8feb-3cd48289fe89
This commit is contained in:
@@ -0,0 +1,83 @@
|
||||
import { test, before, after } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import pg from "pg";
|
||||
|
||||
const DATABASE_URL = process.env.DATABASE_URL;
|
||||
if (!DATABASE_URL) {
|
||||
throw new Error("DATABASE_URL must be set to run these tests");
|
||||
}
|
||||
|
||||
const pool = new pg.Pool({ connectionString: DATABASE_URL });
|
||||
|
||||
let testAppId;
|
||||
let testPermissionId;
|
||||
|
||||
before(async () => {
|
||||
const stamp = `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 6)}`;
|
||||
const slug = `apuq_app_${stamp}`;
|
||||
const permName = `apuq.perm.${stamp}`;
|
||||
|
||||
const route = `/${slug}`;
|
||||
const appRes = await pool.query(
|
||||
`INSERT INTO apps (slug, name_ar, name_en, route, is_active, sort_order)
|
||||
VALUES ($1, $2, $3, $4, true, 999)
|
||||
RETURNING id`,
|
||||
[slug, slug, slug, route],
|
||||
);
|
||||
testAppId = appRes.rows[0].id;
|
||||
|
||||
const permRes = await pool.query(
|
||||
`INSERT INTO permissions (name, description_en) VALUES ($1, $2) RETURNING id`,
|
||||
[permName, permName],
|
||||
);
|
||||
testPermissionId = permRes.rows[0].id;
|
||||
});
|
||||
|
||||
after(async () => {
|
||||
await pool.query(`DELETE FROM app_permissions WHERE app_id = $1`, [testAppId]);
|
||||
await pool.query(`DELETE FROM apps WHERE id = $1`, [testAppId]);
|
||||
await pool.query(`DELETE FROM permissions WHERE id = $1`, [testPermissionId]);
|
||||
await pool.end();
|
||||
});
|
||||
|
||||
test("app_permissions composite primary key prevents duplicate (app_id, permission_id)", async () => {
|
||||
// First insert succeeds
|
||||
await pool.query(
|
||||
`INSERT INTO app_permissions (app_id, permission_id) VALUES ($1, $2)`,
|
||||
[testAppId, testPermissionId],
|
||||
);
|
||||
|
||||
// Plain duplicate insert should fail with a unique-violation error (SQLSTATE 23505)
|
||||
await assert.rejects(
|
||||
pool.query(
|
||||
`INSERT INTO app_permissions (app_id, permission_id) VALUES ($1, $2)`,
|
||||
[testAppId, testPermissionId],
|
||||
),
|
||||
(err) => err.code === "23505",
|
||||
);
|
||||
|
||||
// Only one row should exist for the pair
|
||||
const { rows: countRows } = await pool.query(
|
||||
`SELECT COUNT(*)::int AS c FROM app_permissions WHERE app_id = $1 AND permission_id = $2`,
|
||||
[testAppId, testPermissionId],
|
||||
);
|
||||
assert.equal(countRows[0].c, 1);
|
||||
});
|
||||
|
||||
test("ON CONFLICT DO NOTHING handles duplicate inserts gracefully (no error, no extra row)", async () => {
|
||||
// Re-running the same insert with ON CONFLICT DO NOTHING should not throw
|
||||
// and should not add a second row. This mirrors what `seed.ts` and any
|
||||
// future admin permission management endpoint does via Drizzle's
|
||||
// `.onConflictDoNothing()`.
|
||||
const res = await pool.query(
|
||||
`INSERT INTO app_permissions (app_id, permission_id) VALUES ($1, $2) ON CONFLICT DO NOTHING`,
|
||||
[testAppId, testPermissionId],
|
||||
);
|
||||
assert.equal(res.rowCount, 0, "expected no row to be inserted on conflict");
|
||||
|
||||
const { rows: countRows } = await pool.query(
|
||||
`SELECT COUNT(*)::int AS c FROM app_permissions WHERE app_id = $1 AND permission_id = $2`,
|
||||
[testAppId, testPermissionId],
|
||||
);
|
||||
assert.equal(countRows[0].c, 1, "exactly one row should remain after conflict-safe insert");
|
||||
});
|
||||
Reference in New Issue
Block a user