Skip to content

Commit 9b9b120

Browse files
committed
fix(node): make the repo dedup key did:key-aware so distinct DID methods don't collapse
The dedup grouping key took the last ':' segment of owner_did (split_part / rsplit), so two repos owned by did:key:X and did:gitlawb:X with the same name collapsed into one logical repo on the list, paged, count, stats, and GraphQL surfaces. That is the exact cross-method collision the codebase already guards against in did_matches. Replace it with a did:key-aware key that strips a did:key: prefix only when the remainder is a bare id (no ':'), otherwise keeps the full DID, reproducing did_matches/key_id as an equivalence relation: did:key:X and a bare mirror X still collapse, while distinct methods never merge. Applied byte-identically across DEDUP_CTE (DISTINCT ON / PARTITION BY / ORDER BY), count_repos_deduped, the empty-page count fallback, and the in-memory dedupe_canonical_repos, so the SQL and Rust paths agree. The backing index lived in the already-released migration v1, so v1 is left untouched and a new migration v7 drops idx_repos_owner_short_name and builds idx_repos_owner_key_name on the matching expression; the CASE must stay byte-identical to the queries or Postgres stops using it. Tests cover both the in-memory and SQL paths: distinct methods stay separate, bare-id and did:key forms collapse, and the residual-colon guard keeps a malformed did:key:did:gitlawb:X distinct from the bare method DID. 216 pass.
1 parent 5721791 commit 9b9b120

2 files changed

Lines changed: 302 additions & 14 deletions

File tree

‎crates/gitlawb-node/src/api/repos.rs‎

Lines changed: 142 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1422,13 +1422,20 @@ fn dedupe_canonical_repos(rows: Vec<(RepoRecord, i64)>) -> Vec<(RepoRecord, i64)
14221422
let mut latest: HashMap<(String, String), DateTime<Utc>> = HashMap::new();
14231423

14241424
for (rec, stars) in rows {
1425-
let short = rec
1425+
// did:key-aware owner key: strip a `did:key:` prefix so the bare mirror id
1426+
// and its `did:key:` canonical collapse, but leave any other DID method
1427+
// whole so `did:key:X` and `did:gitlawb:X` never merge. The `!contains(':')`
1428+
// guard mirrors did_matches' `key_id` check: a stripped value that still
1429+
// holds a `:` is a non-key full DID (e.g. malformed `did:key:did:gitlawb:X`)
1430+
// and must keep its full form, not collapse onto the bare method DID. Stays
1431+
// byte-equivalent to the SQL CASE in Db::DEDUP_CTE / count_repos_deduped.
1432+
let owner_key = rec
14261433
.owner_did
1427-
.rsplit(':')
1428-
.next()
1434+
.strip_prefix("did:key:")
1435+
.filter(|rest| !rest.contains(':'))
14291436
.unwrap_or(&rec.owner_did)
14301437
.to_string();
1431-
let key = (short, rec.name.clone());
1438+
let key = (owner_key, rec.name.clone());
14321439

14331440
latest
14341441
.entry(key.clone())
@@ -1649,6 +1656,137 @@ mod tests {
16491656
);
16501657
}
16511658

1659+
#[test]
1660+
fn distinct_did_methods_sharing_a_base58_id_do_not_collapse() {
1661+
// `did:key` and `did:gitlawb` share the base58 id space, so a trailing
1662+
// segment key would treat these as one repo. The did:key-aware key keeps
1663+
// them apart, matching crate::api::did_matches.
1664+
let keyed = record(
1665+
"id-keyed",
1666+
"did:key:z6Mkwbud",
1667+
"nipmod",
1668+
"owned via did:key",
1669+
"2026-01-01T00:00:00Z",
1670+
);
1671+
let gitlawb = record(
1672+
"id-gitlawb",
1673+
"did:gitlawb:z6Mkwbud",
1674+
"nipmod",
1675+
"owned via did:gitlawb",
1676+
"2026-01-01T00:00:00Z",
1677+
);
1678+
1679+
let out = dedupe_canonical_repos(vec![(keyed, 1), (gitlawb, 2)]);
1680+
1681+
assert_eq!(
1682+
out.len(),
1683+
2,
1684+
"same name and base58 id under different DID methods are distinct repos"
1685+
);
1686+
}
1687+
1688+
#[test]
1689+
fn bare_id_and_did_key_form_of_same_owner_collapse() {
1690+
// A bare mirror id and its did:key canonical are the same owner and must
1691+
// collapse, the mirror-vs-canonical case stated in owner-key terms.
1692+
let mirror = record(
1693+
"z6Mkwbud/nipmod",
1694+
"z6Mkwbud",
1695+
"nipmod",
1696+
"mirrored from peer",
1697+
"2026-02-01T00:00:00Z",
1698+
);
1699+
let canonical = record(
1700+
"canon-id",
1701+
"did:key:z6Mkwbud",
1702+
"nipmod",
1703+
"real",
1704+
"2026-01-15T00:00:00Z",
1705+
);
1706+
1707+
let out = dedupe_canonical_repos(vec![(mirror, 0), (canonical, 5)]);
1708+
1709+
assert_eq!(out.len(), 1, "bare id and its did:key form are one owner");
1710+
assert_eq!(out[0].0.owner_did, "did:key:z6Mkwbud", "canonical row wins");
1711+
}
1712+
1713+
#[test]
1714+
fn did_key_wrapping_a_full_did_does_not_collapse_onto_the_bare_method_did() {
1715+
// Residual-colon guard, mirroring did_matches' `!key_id().contains(':')`:
1716+
// a malformed `did:key:did:gitlawb:X` strips to `did:gitlawb:X`, which still
1717+
// holds a `:`, so it must keep its full form and NOT collapse with a real
1718+
// `did:gitlawb:X` repo of the same name.
1719+
let wrapped = record(
1720+
"id-wrapped",
1721+
"did:key:did:gitlawb:z6Mkwbud",
1722+
"nipmod",
1723+
"malformed nested DID",
1724+
"2026-01-01T00:00:00Z",
1725+
);
1726+
let method = record(
1727+
"id-method",
1728+
"did:gitlawb:z6Mkwbud",
1729+
"nipmod",
1730+
"real method DID",
1731+
"2026-01-02T00:00:00Z",
1732+
);
1733+
1734+
let out = dedupe_canonical_repos(vec![(wrapped, 1), (method, 2)]);
1735+
1736+
assert_eq!(
1737+
out.len(),
1738+
2,
1739+
"a did:key-wrapped full DID stays distinct from the bare method DID"
1740+
);
1741+
// Assert identity, not just count: each owner survives unmerged, so a
1742+
// regression that kept two rows but mis-keyed the survivor is also caught.
1743+
let mut owners: Vec<&str> = out.iter().map(|(r, _)| r.owner_did.as_str()).collect();
1744+
owners.sort_unstable();
1745+
assert_eq!(
1746+
owners,
1747+
vec!["did:gitlawb:z6Mkwbud", "did:key:did:gitlawb:z6Mkwbud"],
1748+
"both owner DIDs survive in their full form"
1749+
);
1750+
}
1751+
1752+
#[test]
1753+
fn empty_did_key_residual_keys_to_empty_string_consistently() {
1754+
// Degenerate boundary the reviewers flagged: `did:key:` with no id strips to
1755+
// an empty residual (no colon), so the key is "". A bare empty owner also
1756+
// keys to "", so the two collapse — proving the Rust strip path maps the
1757+
// empty residual exactly like the SQL `substr(owner_did, 9)` / `position`
1758+
// path (mirrored in the db-level test). A real did:key id keys separately.
1759+
let empty_did_key = record(
1760+
"id-empty-didkey",
1761+
"did:key:",
1762+
"nipmod",
1763+
"empty residual",
1764+
"2026-01-01T00:00:00Z",
1765+
);
1766+
let empty_bare = record(
1767+
"id-empty-bare",
1768+
"",
1769+
"nipmod",
1770+
"empty owner",
1771+
"2026-01-02T00:00:00Z",
1772+
);
1773+
let real = record(
1774+
"id-real",
1775+
"did:key:z6Mkwbud",
1776+
"nipmod",
1777+
"real id",
1778+
"2026-01-03T00:00:00Z",
1779+
);
1780+
1781+
let out = dedupe_canonical_repos(vec![(empty_did_key, 0), (empty_bare, 0), (real, 0)]);
1782+
1783+
assert_eq!(
1784+
out.len(),
1785+
2,
1786+
"`did:key:` and the empty owner share the empty key and collapse; the real id stays separate"
1787+
);
1788+
}
1789+
16521790
#[test]
16531791
fn two_mirror_rows_break_tie_by_earliest_created_at() {
16541792
// Both are mirror rows (slash-form ids); earliest created_at wins.

‎crates/gitlawb-node/src/db/mod.rs‎

Lines changed: 160 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -767,6 +767,20 @@ const MIGRATIONS: &[Migration] = &[
767767
"ALTER TABLE agents ADD COLUMN IF NOT EXISTS deactivated_at TEXT",
768768
],
769769
},
770+
Migration {
771+
version: 7,
772+
name: "repo_owner_dedup_key_didkey_aware",
773+
stmts: &[
774+
// The dedup grouping key moved from the last `:` segment to a
775+
// did:key-aware key (strip `did:key:`, leave any other DID method
776+
// whole) so `did:key:X` and `did:gitlawb:X` no longer collapse. Swap
777+
// the index that backs it: drop the last-segment one from v1 and build
778+
// the matching expression index. The CASE must stay byte-for-byte in
779+
// sync with DEDUP_CTE / count_repos_deduped or Postgres won't use it.
780+
"DROP INDEX IF EXISTS idx_repos_owner_short_name",
781+
"CREATE INDEX IF NOT EXISTS idx_repos_owner_key_name ON repos ((CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END), name)",
782+
],
783+
},
770784
];
771785

772786
// ── Repos ─────────────────────────────────────────────────────────────────────
@@ -896,26 +910,32 @@ impl Db {
896910

897911
/// Shared dedup CTE: collapses the mirror row and the canonical row of one
898912
/// logical repo into a single survivor. `$1` is an optional owner filter
899-
/// (NULL = all rows). Grouping is `(split_part(owner_did,':',-1), name)`; the
900-
/// canonical row wins (mirror rows carry a slash-form `id` written only by
913+
/// (NULL = all rows). Grouping collapses on a did:key-aware owner key: strip a
914+
/// `did:key:` prefix (8 chars, so `substr(owner_did, 9)`) only when the
915+
/// remainder is a bare id with no `:`, otherwise keep the full DID. That is the
916+
/// exact normalization in `crate::api::did_matches`, so `did:key:X` and a bare
917+
/// `X` collapse while distinct DID methods (`did:gitlawb:X`) never merge. The
918+
/// CASE is repeated verbatim in `count_repos_deduped` and the v7 index and must
919+
/// stay byte-identical or Postgres stops using the index.
920+
/// The canonical row wins (mirror rows carry a slash-form `id` written only by
901921
/// `upsert_mirror_repo`), ties broken by earliest `created_at` then `id` so a
902922
/// full tie still picks a deterministic survivor. `list_all_repos_paged`,
903923
/// `list_all_repos_deduped`, and the marker logic in
904924
/// `crate::api::repos::dedupe_canonical_repos` must stay in sync.
905925
const DEDUP_CTE: &'static str = "WITH deduped AS (
906-
SELECT DISTINCT ON (split_part(owner_did, ':', -1), name)
926+
SELECT DISTINCT ON (CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END, name)
907927
id, name, owner_did, description, is_public, default_branch,
908928
created_at,
909929
-- group MAX, not the canonical row's own value: pushes that
910930
-- arrive via gossip touch only the mirror row, so the
911931
-- canonical updated_at goes stale
912932
MAX(updated_at) OVER (
913-
PARTITION BY split_part(owner_did, ':', -1), name
933+
PARTITION BY CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END, name
914934
) AS updated_at,
915935
disk_path, forked_from, machine_id
916936
FROM repos
917937
WHERE ($1::text IS NULL OR owner_did = $1 OR owner_did LIKE '%:' || $1)
918-
ORDER BY split_part(owner_did, ':', -1), name,
938+
ORDER BY CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END, name,
919939
-- mirror rows carry a slash-form id (\"{owner_short}/{name}\"),
920940
-- written only by upsert_mirror_repo; canonical ids are UUIDs.
921941
-- Rank canonical (no slash) ahead of the mirror in each group,
@@ -967,7 +987,7 @@ impl Db {
967987

968988
let total = if out.is_empty() {
969989
let row = sqlx::query(
970-
"SELECT COUNT(DISTINCT (split_part(owner_did, ':', -1), name)) AS cnt
990+
"SELECT COUNT(DISTINCT (CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END, name)) AS cnt
971991
FROM repos
972992
WHERE ($1::text IS NULL OR owner_did = $1 OR owner_did LIKE '%:' || $1)",
973993
)
@@ -1005,12 +1025,12 @@ impl Db {
10051025
}
10061026

10071027
/// Count of distinct logical repos (mirror + canonical collapsed), for
1008-
/// `/api/v1/stats`. Uses the same `(split_part(owner_did,':',-1), name)`
1009-
/// grouping as `DEDUP_CTE`; the marker/tiebreak only decide which row would
1010-
/// survive, not the group count, so they are not needed here.
1028+
/// `/api/v1/stats`. Uses the same did:key-aware owner-key grouping as
1029+
/// `DEDUP_CTE` (the CASE must stay byte-identical); the marker/tiebreak only
1030+
/// decide which row would survive, not the group count, so they are not needed here.
10111031
pub async fn count_repos_deduped(&self) -> Result<i64> {
10121032
let row = sqlx::query(
1013-
"SELECT COUNT(DISTINCT (split_part(owner_did, ':', -1), name)) AS cnt FROM repos",
1033+
"SELECT COUNT(DISTINCT (CASE WHEN owner_did LIKE 'did:key:%' AND position(':' in substr(owner_did, 9)) = 0 THEN substr(owner_did, 9) ELSE owner_did END, name)) AS cnt FROM repos",
10141034
)
10151035
.fetch_one(&self.pool)
10161036
.await?;
@@ -3162,6 +3182,136 @@ mod dedup_db_tests {
31623182
assert_eq!(out[0].owner_did, "did:key:z6Mkwbud", "canonical row wins");
31633183
}
31643184

3185+
/// Same name and base58 id but different DID methods (`did:key` vs
3186+
/// `did:gitlawb`) must NOT collapse: the grouping key strips only `did:key:`
3187+
/// and leaves other methods whole, matching crate::api::did_matches. Both the
3188+
/// list (DEDUP_CTE) and count (count_repos_deduped) paths must agree.
3189+
#[sqlx::test]
3190+
async fn deduped_keeps_distinct_did_methods_apart(pool: PgPool) {
3191+
let db = db(pool).await;
3192+
db.create_repo(&rec(
3193+
"id-keyed",
3194+
"did:key:z6Mkwbud",
3195+
"nipmod",
3196+
"via did:key",
3197+
"2026-01-01T00:00:00Z",
3198+
"2026-01-01T00:00:00Z",
3199+
))
3200+
.await
3201+
.unwrap();
3202+
db.create_repo(&rec(
3203+
"id-gitlawb",
3204+
"did:gitlawb:z6Mkwbud",
3205+
"nipmod",
3206+
"via did:gitlawb",
3207+
"2026-01-02T00:00:00Z",
3208+
"2026-01-02T00:00:00Z",
3209+
))
3210+
.await
3211+
.unwrap();
3212+
3213+
let out = db.list_all_repos_deduped().await.unwrap();
3214+
assert_eq!(out.len(), 2, "distinct DID methods are distinct owners");
3215+
assert_eq!(
3216+
db.count_repos_deduped().await.unwrap(),
3217+
2,
3218+
"count path agrees with the list path",
3219+
);
3220+
}
3221+
3222+
/// SQL residual-colon guard: a malformed `did:key:did:gitlawb:X` strips to a
3223+
/// value that still holds a `:`, so the CASE keeps it whole and it does NOT
3224+
/// collapse with a real `did:gitlawb:X`. Proves the SQL key matches the Rust
3225+
/// `strip_prefix(...).filter(|r| !r.contains(':'))` and did_matches.
3226+
#[sqlx::test]
3227+
async fn deduped_did_key_wrapping_a_full_did_stays_distinct(pool: PgPool) {
3228+
let db = db(pool).await;
3229+
db.create_repo(&rec(
3230+
"id-wrapped",
3231+
"did:key:did:gitlawb:z6Mkwbud",
3232+
"nipmod",
3233+
"malformed nested DID",
3234+
"2026-01-01T00:00:00Z",
3235+
"2026-01-01T00:00:00Z",
3236+
))
3237+
.await
3238+
.unwrap();
3239+
db.create_repo(&rec(
3240+
"id-method",
3241+
"did:gitlawb:z6Mkwbud",
3242+
"nipmod",
3243+
"real method DID",
3244+
"2026-01-02T00:00:00Z",
3245+
"2026-01-02T00:00:00Z",
3246+
))
3247+
.await
3248+
.unwrap();
3249+
3250+
let out = db.list_all_repos_deduped().await.unwrap();
3251+
assert_eq!(
3252+
out.len(),
3253+
2,
3254+
"wrapped full DID stays distinct from the method DID"
3255+
);
3256+
assert_eq!(
3257+
db.count_repos_deduped().await.unwrap(),
3258+
2,
3259+
"count path agrees with the list path",
3260+
);
3261+
}
3262+
3263+
/// Empty-residual boundary: `did:key:` matches `LIKE 'did:key:%'`,
3264+
/// `substr(owner_did, 9)` is '', and `position(':' in '')` is 0, so the CASE
3265+
/// keys it to '' just like a bare empty owner, while a real `did:key:z…` keys
3266+
/// separately. Pins that the SQL empty-residual handling matches the Rust
3267+
/// `strip_prefix(...).filter(...)` path (mirrored in the api-level test).
3268+
#[sqlx::test]
3269+
async fn deduped_empty_did_key_residual_keys_to_empty_string(pool: PgPool) {
3270+
let db = db(pool).await;
3271+
db.create_repo(&rec(
3272+
"id-empty-didkey",
3273+
"did:key:",
3274+
"nipmod",
3275+
"empty residual",
3276+
"2026-01-01T00:00:00Z",
3277+
"2026-01-01T00:00:00Z",
3278+
))
3279+
.await
3280+
.unwrap();
3281+
db.create_repo(&rec(
3282+
"id-empty-bare",
3283+
"",
3284+
"nipmod",
3285+
"empty owner",
3286+
"2026-01-02T00:00:00Z",
3287+
"2026-01-02T00:00:00Z",
3288+
))
3289+
.await
3290+
.unwrap();
3291+
db.create_repo(&rec(
3292+
"id-real",
3293+
"did:key:z6Mkwbud",
3294+
"nipmod",
3295+
"real id",
3296+
"2026-01-03T00:00:00Z",
3297+
"2026-01-03T00:00:00Z",
3298+
))
3299+
.await
3300+
.unwrap();
3301+
3302+
let out = db.list_all_repos_deduped().await.unwrap();
3303+
assert_eq!(
3304+
out.len(),
3305+
2,
3306+
"`did:key:` and the empty owner collapse on the empty key; the real id is separate"
3307+
);
3308+
assert_eq!(
3309+
db.count_repos_deduped().await.unwrap(),
3310+
2,
3311+
"count path agrees with the list path",
3312+
);
3313+
}
3314+
31653315
/// Distinct repos are preserved and ordered by most recent activity.
31663316
#[sqlx::test]
31673317
async fn deduped_preserves_distinct_repos_ordered_by_updated(pool: PgPool) {

0 commit comments

Comments
 (0)