Skip to content

Commit 8184f3c

Browse files
committed
fix: code review
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
1 parent c918213 commit 8184f3c

2 files changed

Lines changed: 43 additions & 14 deletions

File tree

backend/src/osspckgs/migrations/V1784718693__merge_case_duplicate_github_repos.sql

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,18 +29,21 @@ WHERE group_size > 1;
2929
CREATE INDEX ON repo_merge_members (repo_id);
3030
ANALYZE repo_merge_members;
3131

32-
-- Keep one link per (package, group): the keeper's if it exists, else the most
33-
-- recently verified. ROW_NUMBER instead of an EXISTS check against the keeper
34-
-- because 3-variant groups exist: two losers linking the same package would
35-
-- collide on UNIQUE (package_id, repo_id) after the re-point below.
32+
-- Keep one link per (package, group), ranked the way consumers pick links
33+
-- (sqlFragments bestRepoLink: confidence DESC, declared-on-ties) so the merge
34+
-- preserves the strongest provenance; keeper status only breaks full ties.
35+
-- ROW_NUMBER instead of an EXISTS check against the keeper because 3-variant
36+
-- groups exist: two losers linking the same package would collide on
37+
-- UNIQUE (package_id, repo_id) after the re-point below.
3638
DELETE FROM package_repos
3739
WHERE id IN (
3840
SELECT id
3941
FROM (
4042
SELECT pr.id,
4143
ROW_NUMBER() OVER (
4244
PARTITION BY m.keeper_id, pr.package_id
43-
ORDER BY m.is_keeper DESC, pr.verified_at DESC, pr.id
45+
ORDER BY pr.confidence DESC, (pr.source = 'declared') DESC,
46+
m.is_keeper DESC, pr.verified_at DESC, pr.id
4447
) AS rn
4548
FROM package_repos pr
4649
JOIN repo_merge_members m ON m.repo_id = pr.repo_id
@@ -65,6 +68,18 @@ USING repo_merge_members m
6568
WHERE c.repo_id = m.repo_id
6669
AND NOT m.is_keeper;
6770

71+
-- packages.repository_url is denormalized and must keep matching the
72+
-- canonical repos.url (the maven backfill updates the two atomically for the
73+
-- same reason). Match against member urls while the loser rows still exist;
74+
-- last_synced_at is the packages watermark, bumping it ships the correction
75+
-- to Tinybird.
76+
UPDATE packages p
77+
SET repository_url = LOWER(p.repository_url), last_synced_at = NOW()
78+
FROM repo_merge_members m
79+
JOIN repos r ON r.id = m.repo_id
80+
WHERE p.repository_url = r.url
81+
AND p.repository_url <> LOWER(p.repository_url);
82+
6883
-- Cascades security_contacts and repo_activity_snapshot on losers.
6984
DELETE FROM repos r
7085
USING repo_merge_members m
@@ -81,12 +96,6 @@ WHERE r.id = m.repo_id
8196
AND m.is_keeper
8297
AND r.url <> LOWER(r.url);
8398

84-
-- Recurrence guard; also fails the migration if any duplicate survived the
85-
-- merge. GitHub-only: it is the only host with both confirmed duplicates and
86-
-- guaranteed lowercase-on-write today (CASE_INSENSITIVE_HOSTS in
87-
-- canonicalizeRepoUrl also covers gitlab.com, whose merge is a follow-up).
88-
-- On other hosts case can be significant and writers do not normalize, so a
89-
-- wider index could reject legitimately distinct repos.
90-
CREATE UNIQUE INDEX IF NOT EXISTS repos_github_lower_url_uq
91-
ON repos (LOWER(url))
92-
WHERE host = 'github';
99+
-- The recurrence-guard unique index lives in the next migration: it must be
100+
-- built CONCURRENTLY (repos takes continuous worker writes), which cannot run
101+
-- inside this transaction.
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
-- Recurrence guard for case-duplicate GitHub repos, split from the merge in
2+
-- V1784718693: CONCURRENTLY cannot run inside a transaction, and under
3+
-- flyway's mixed=true a shared file would demote the merge itself to
4+
-- autocommit, losing its atomicity. The concurrent build keeps repos writable
5+
-- for the workers during the scan, and doubles as verification of the merge —
6+
-- a surviving duplicate fails the build.
7+
--
8+
-- GitHub-only: the only host with both confirmed duplicates and guaranteed
9+
-- lowercase-on-write today (CASE_INSENSITIVE_HOSTS in canonicalizeRepoUrl
10+
-- also covers gitlab.com, whose merge is a follow-up). On other hosts case
11+
-- can be significant and writers do not normalize, so a wider index could
12+
-- reject legitimately distinct repos.
13+
--
14+
-- A failed concurrent build leaves an INVALID index behind; the DROP lets a
15+
-- re-run replace it, where IF NOT EXISTS would silently keep the broken one.
16+
DROP INDEX IF EXISTS repos_github_lower_url_uq;
17+
18+
CREATE UNIQUE INDEX CONCURRENTLY repos_github_lower_url_uq
19+
ON repos (LOWER(url))
20+
WHERE host = 'github';

0 commit comments

Comments
 (0)