Skip to content

Commit 1d4100c

Browse files
Herklosclaude
andcommitted
Add the missing rem_lines foreign key, index every FK, gate migrations on pgTAP.
rem_lines.user_id was the only non-PK uuid column missing a foreign key that should have had one. It holds a company_members user id on a financial ledger, so a deleted auth user left dangling rows forever. Orphans are cleaned before the constraint is added, which is safe because rem_lines is fully derived: compute_rem_month deletes and rebuilds every line of a month. Many other uuid columns look unconstrained in the Supabase dashboard. They are not. 31 of the foreign keys target auth.users(id) and the schema visualizer only draws public to public edges. The migration header records this so the next person does not redo the investigation. The two remaining FK-less uuid columns are deliberate: companies.product_pool_project_id (circular dependency, trigger enforced) and audit_log.entity_id (polymorphic). Postgres does not index the referencing side of a foreign key. Deleting one auth.users row sequential-scanned 13 tables and deleting one companies row scanned 8 more, including a nested cascade into jungle_tjm_queue_settlements. This adds 25 indexes, partial on "col is not null" for the nullable actor columns so they stay small. EXPLAIN under force_generic_plan confirms the planner uses those for the referential-integrity probe. One case was an outright regression: 20260806000000 replaced the unique constraint backing revenue_entries.revenue_source_id with a partial index, which cannot serve an integrity check. CI now replays every migration into an empty database and runs the pgTAP suite, on pull requests as well as pushes, with the deploy job gated behind it. db push only applies what production is missing, so it never validated the schema as a whole, and no test had ever run in CI. That first run surfaced two stale assertions in 010-company_invites_accept. They used a "second user" that had already become a member earlier in the file, so it hit the already-a-member branch added in 20260812000000 instead of the invite-used guard. The happy-path redemptions now use their own throwaway users, and two added tests pin that a used admin invite cannot escalate an existing member. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 66dcdbc commit 1d4100c

3 files changed

Lines changed: 209 additions & 2 deletions

File tree

.github/workflows/supabase.yml

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,38 @@ on:
3131
default: false
3232

3333
jobs:
34+
# Gate: replay every migration into an empty database and run the pgTAP suite.
35+
# `db push` only applies what production is missing, so it never re-checks the
36+
# schema as a whole — a migration that is invalid from scratch (a foreign key
37+
# referencing a table created later, a constraint no existing row satisfies)
38+
# would otherwise be discovered in production. Runs on pull requests too, where
39+
# the deploy job is skipped, so a branch gets validated before it lands.
40+
test:
41+
runs-on: ubuntu-latest
42+
43+
steps:
44+
- uses: actions/checkout@v4
45+
46+
- uses: supabase/setup-cli@v1
47+
with:
48+
version: latest
49+
50+
# Boots the full local stack (the auth container creates the auth schema
51+
# that migrations and tests reference). Runners already have Docker.
52+
- name: Start local stack
53+
run: supabase --workdir backend start
54+
55+
- name: Replay migrations from scratch
56+
run: supabase --workdir backend db reset
57+
58+
# Same command as `pnpm test` in backend/supabase/package.json.
59+
- name: pgTAP
60+
run: supabase --workdir backend test db
61+
3462
deploy:
3563
# Run only on push / manual dispatch; on PRs the workflow reports a check but skips the deploy.
3664
if: github.event_name != 'pull_request'
65+
needs: test
3766
runs-on: ubuntu-latest
3867
environment:
3968
name: Production - Supabase
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
-- ============================================================================
2+
-- Foreign-key integrity gap + index coverage behind every foreign key
3+
--
4+
-- READ THIS BEFORE "FIXING" MISSING FOREIGN KEYS FROM THE DASHBOARD:
5+
-- the Supabase schema visualizer only draws public → public relationships.
6+
-- 31 of this schema's foreign keys target auth.users(id), so every user_id /
7+
-- created_by / approved_by / actor_id column *looks* unconstrained there while
8+
-- the constraint is in fact present. Check pg_constraint, not the diagram.
9+
--
10+
-- After this migration exactly two non-PK uuid columns have no FK, both on
11+
-- purpose:
12+
-- companies.product_pool_project_id — would re-create the circular
13+
-- companies ↔ projects dependency rejected in 20260803000000; enforced by
14+
-- the enforce_product_pool_project trigger instead.
15+
-- audit_log.entity_id — polymorphic, discriminated by entity_type.
16+
--
17+
-- Postgres does not index the referencing side of a foreign key. Without one,
18+
-- every parent delete sequential-scans the child: deleting one auth.users row
19+
-- scanned 13 tables and deleting one companies row scanned 8 more, including a
20+
-- nested cascade into jungle_tjm_queue_settlements. This adds the missing
21+
-- indexes.
22+
--
23+
-- Plain `create index` (not `concurrently`): db push runs each migration in a
24+
-- transaction, where concurrently is illegal. It takes a write lock for the
25+
-- duration, which is fine at current table sizes.
26+
-- ============================================================================
27+
28+
-- ----------------------------------------------------------------------------
29+
-- 1) rem_lines.user_id — the one real gap
30+
--
31+
-- Declared bare in 20260803000000 (`user_id uuid, -- null for company_fee
32+
-- bucket`) while its sibling project_id got a FK. It holds a company_members
33+
-- user id, i.e. an auth.users id, on a financial ledger.
34+
--
35+
-- Orphans are cleaned first so the constraint cannot block the push. This is
36+
-- safe: rem_lines is fully derived — compute_rem_month opens by deleting every
37+
-- line of the month and rebuilding it. A line whose user no longer exists in
38+
-- auth.users has already lost its company_members row (that FK cascades), so it
39+
-- can never be recomputed and carries nothing worth keeping. Same precedent as
40+
-- the zero-cent cleanup in 20260806000000.
41+
-- ----------------------------------------------------------------------------
42+
delete from public.rem_lines l
43+
where l.user_id is not null
44+
and not exists (select 1 from auth.users u where u.id = l.user_id);
45+
46+
alter table public.rem_lines
47+
drop constraint if exists rem_lines_user_id_fkey;
48+
49+
alter table public.rem_lines
50+
add constraint rem_lines_user_id_fkey
51+
foreign key (user_id) references auth.users (id) on delete cascade;
52+
53+
-- auth.users, not profiles(user_id): that matches every other person column
54+
-- that is not PostgREST-embedded (time_entries.user_id, notifications.user_id,
55+
-- time_off.user_id, project_costs.user_id). The four profiles mirrors added in
56+
-- 20260719000000 exist only to make `profiles(...)` embeds resolve; adding more
57+
-- of those to a table with several actor columns causes PGRST201 ambiguity.
58+
--
59+
-- The column stays nullable — null means the company_fee bucket, which has no
60+
-- person attached. A nullable FK ignores null rows.
61+
62+
-- ----------------------------------------------------------------------------
63+
-- 2) Index coverage: NOT NULL foreign-key columns
64+
--
65+
-- revenue_entries.revenue_source_id is a regression, not an oversight:
66+
-- 20260806000000 dropped `unique (revenue_source_id, period_month)` and
67+
-- replaced it with a partial unique index (where auto_generated and not
68+
-- deleted). A partial index cannot serve a referential-integrity check, so the
69+
-- column silently lost its coverage. The partial unique index stays — it still
70+
-- enforces one auto row per source-month.
71+
-- ----------------------------------------------------------------------------
72+
create index if not exists revenue_entries_company_idx
73+
on public.revenue_entries (company_id);
74+
create index if not exists revenue_entries_source_idx
75+
on public.revenue_entries (revenue_source_id);
76+
77+
create index if not exists revenue_sources_company_idx
78+
on public.revenue_sources (company_id);
79+
80+
create index if not exists referral_earnings_company_idx
81+
on public.referral_earnings (company_id);
82+
83+
create index if not exists project_referrals_company_idx
84+
on public.project_referrals (company_id);
85+
86+
create index if not exists notifications_company_idx
87+
on public.notifications (company_id);
88+
89+
create index if not exists time_off_company_idx
90+
on public.time_off (company_id);
91+
92+
-- The jungle queue's two existing indexes are both partial (where deleted =
93+
-- false) and neither leads with user_id, so all three of its FK columns are
94+
-- uncovered.
95+
create index if not exists jungle_queue_company_idx
96+
on public.jungle_tjm_queue_entries (company_id);
97+
create index if not exists jungle_queue_project_idx
98+
on public.jungle_tjm_queue_entries (project_id);
99+
create index if not exists jungle_queue_user_idx
100+
on public.jungle_tjm_queue_entries (user_id);
101+
102+
create index if not exists jungle_queue_settlements_company_idx
103+
on public.jungle_tjm_queue_settlements (company_id);
104+
create index if not exists jungle_queue_settlements_entry_idx
105+
on public.jungle_tjm_queue_settlements (queue_entry_id);
106+
107+
-- ----------------------------------------------------------------------------
108+
-- 3) Index coverage: nullable actor columns
109+
--
110+
-- Partial `where <col> is not null` keeps these small — most rows are null.
111+
-- The RI probe is `col = $1`, which implies `col is not null`, so the planner
112+
-- can still use them.
113+
-- ----------------------------------------------------------------------------
114+
create index if not exists projects_created_by_idx
115+
on public.projects (created_by) where created_by is not null;
116+
117+
create index if not exists time_entries_approved_by_idx
118+
on public.time_entries (approved_by) where approved_by is not null;
119+
120+
create index if not exists revenue_sources_created_by_idx
121+
on public.revenue_sources (created_by) where created_by is not null;
122+
123+
create index if not exists company_invites_invited_by_idx
124+
on public.company_invites (invited_by) where invited_by is not null;
125+
create index if not exists company_invites_accepted_by_idx
126+
on public.company_invites (accepted_by) where accepted_by is not null;
127+
128+
create index if not exists invoice_payments_recorded_by_idx
129+
on public.invoice_payments (recorded_by) where recorded_by is not null;
130+
131+
create index if not exists audit_log_actor_idx
132+
on public.audit_log (actor_id) where actor_id is not null;
133+
134+
-- project_costs.company_id is already covered by project_costs_company_status_idx
135+
-- (company_id leading). These four are not.
136+
create index if not exists project_costs_user_idx
137+
on public.project_costs (user_id) where user_id is not null;
138+
create index if not exists project_costs_approved_by_idx
139+
on public.project_costs (approved_by) where approved_by is not null;
140+
create index if not exists project_costs_reimbursed_by_idx
141+
on public.project_costs (reimbursed_by) where reimbursed_by is not null;
142+
create index if not exists project_costs_created_by_idx
143+
on public.project_costs (created_by) where created_by is not null;
144+
145+
-- rem_lines_user_idx is (company_id, user_id), so user_id is not leading and
146+
-- cannot serve the FK added above. rem_lines_month_idx does not cover
147+
-- project_id either.
148+
create index if not exists rem_lines_user_fk_idx
149+
on public.rem_lines (user_id) where user_id is not null;
150+
create index if not exists rem_lines_project_idx
151+
on public.rem_lines (project_id) where project_id is not null;

backend/supabase/tests/database/010-company_invites_accept.test.sql

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
begin;
2-
select plan(22);
2+
select plan(24);
33

44
-- Happy path + lifecycle for accept_company_invite
55
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa1', 'admin1@test.local');
@@ -8,6 +8,13 @@ select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa3', 'invitee-m
88
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa4', 'invitee-free@test.local');
99
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa5', 'other-co-admin@test.local');
1010
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa6', 'second-redeemer@test.local');
11+
-- aaa6 must never join the company: it is the "outsider" probe for the
12+
-- used-invite guards. Redeeming an invite as aaa6 would make it a member, and
13+
-- accept_company_invite treats an active member re-redeeming a used invite as
14+
-- success (20260812000000), so the guard would stop being exercised. The
15+
-- happy-path redemptions below therefore get their own throwaway users.
16+
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa7', 'pad-redeemer@test.local');
17+
select tests.create_auth_user('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa8', 'boundary-redeemer@test.local');
1118

1219
select tests.clear_auth();
1320

@@ -131,12 +138,14 @@ select throws_ok(
131138
);
132139

133140
-- 9 padded token is accepted after server trim
141+
select tests.authenticate_as('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa7');
134142
select lives_ok(
135143
format($f$select public.accept_company_invite(' %s ')$f$, (select tok_pad from inv_ctx)),
136144
'padded token accepted via server trim'
137145
);
138146

139-
-- 10–15 lifecycle guards
147+
-- 10–15 lifecycle guards, run as the outsider so `invite-used` is reachable
148+
select tests.authenticate_as('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa6');
140149
select throws_ok(
141150
format($f$select public.accept_company_invite('%s')$f$, (select tok_revoked from inv_ctx)),
142151
'P0001',
@@ -163,6 +172,7 @@ select throws_ok(
163172
);
164173

165174
-- 13 boundary: expires_at = now() is still valid (< now() required to expire)
175+
select tests.authenticate_as('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa8');
166176
select lives_ok(
167177
format($f$select public.accept_company_invite('%s')$f$, (select tok_boundary from inv_ctx)),
168178
'expires_at = now() still redeemable'
@@ -200,5 +210,22 @@ select is(
200210
'same acceptor redeem is idempotent'
201211
);
202212

213+
-- 20–21 a different active member may re-redeem a used invite (the branch added
214+
-- in 20260812000000), but it must not inherit that invite's role. aaa7 joined as
215+
-- a freelancer via tok_pad; tok_admin is a used ADMIN invite.
216+
select tests.authenticate_as('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa7');
217+
select is(
218+
public.accept_company_invite((select tok_admin from inv_ctx)),
219+
(select company_id from inv_ctx),
220+
'active member re-redeeming a used invite succeeds'
221+
);
222+
select is(
223+
(select role::text from public.company_members
224+
where company_id = (select company_id from inv_ctx)
225+
and user_id = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa7'),
226+
'freelancer',
227+
'a used admin invite cannot escalate an existing member'
228+
);
229+
203230
select * from finish();
204231
rollback;

0 commit comments

Comments
 (0)