feat: fix maven repo gap (CM-1305)#4311
Conversation
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| return url.replace(/\/$/, '') | ||
| return `https://${host}/${owner}/${name}` |
There was a problem hiding this comment.
GitLab nested paths truncated wrongly
Medium Severity
The updated normalizeScmUrl always keeps only the first two URL path segments as owner and repo. GitLab projects under nested groups (three or more path segments) are shortened to the wrong repository URL, and the repo-url backfill will persist that incorrect value wherever the declared SCM URL was complete.
Reviewed by Cursor Bugbot for commit aa4ad3e. Configure here.
There was a problem hiding this comment.
Pull request overview
This PR closes a gap in how Maven package repository links are derived. It rewrites normalizeScmUrl into a stricter, host-validated canonicalizer that only emits https://<host>/<owner>/<repo> for known SCM hosts (returning null for homepages, placeholders, and non-SCM hosts), and adds a one-shot backfill that re-runs this normalizer over already-stored declared_repository_url values — without re-fetching POMs — to fill previously-dropped repository_url values (Gap B) and clear non-repository values (Gap C).
Changes:
- Reworked
normalizeScmUrlto canonicalize SCM URLs against an allow-list of hosts (github,gitlab,bitbucket,gitee,codeberg), handling scheme-less,git+, SCP, andssh://forms, with case-folding for GitHub/GitLab. - Added DAL helpers (
listMavenPackagesForRepoUrlRecompute,updateMavenRepositoryUrls) plus a resumable, dry-run-capable backfill (backfillMavenRepositoryUrls) and CLI entrypoint. - Extended unit tests for the new normalizer behavior and added
npmscripts for the backfill.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/apps/packages_worker/src/maven/extract.ts | Rewrites normalizeScmUrl into a host-validated canonicalizer; core of the fix. |
| services/libs/data-access-layer/src/osspckgs/packages.ts | Adds keyset scan + batched clear/set UPDATE helpers used by the backfill. |
| services/apps/packages_worker/src/maven/backfillRepositoryUrl.ts | New resumable/idempotent backfill loop recomputing repository_url from stored data. |
| services/apps/packages_worker/src/bin/maven-repo-url-backfill.ts | CLI entrypoint with arg parsing, graceful shutdown, and DB connect. |
| services/apps/packages_worker/src/maven/tests/normalize.test.ts | Adds Gap B/Gap C test cases for the new normalizer. |
| services/apps/packages_worker/package.json | Adds backfill:maven-repo-url npm scripts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (updates.length > 0 && !dryRun) { | ||
| await updateMavenRepositoryUrls(qx, updates) | ||
| } |
| const segments = parsed.pathname.split('/').filter(Boolean) | ||
| if (segments.length < 2) return null | ||
|
|
||
| let owner = segments[0] | ||
| let name = segments[1].replace(/\.git$/, '') | ||
| if (!owner || !name) return null |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
| if (updates.length > 0 && !dryRun) { | ||
| // Atomic per batch: the repository_url UPDATE, the stale-link prune, and the | ||
| // relink must commit together. Otherwise an interrupt between them leaves | ||
| // packages.repository_url updated but package_repos out of sync — and on a | ||
| // re-run the row is skipped (its repository_url already matches `desired`), | ||
| // so the inconsistency would never be repaired. On rollback the row stays | ||
| // unchanged and is reprocessed on the next run. | ||
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Backfill lacks deadlock retry
Medium Severity
The repo-url backfill wraps batch UPDATE/DELETE/writeRepoLink work in a single qx.tx without the withDeadlockRetry wrapper used by the Maven enrichment path for the same shared repos and package_repos upserts. If the Maven worker runs concurrently, Postgres 40P01 deadlocks can abort the whole batch and exit the job via process.exit(1) even though the writes are idempotent and safe to retry.
Reviewed by Cursor Bugbot for commit 4535615. Configure here.
| * - ambiguous `git.*` hosts (git.iem.at, git.i-novus.ru, git.oschina.net) and the | ||
| * internal git.corp.adobe.com: pending path/reachability confirmation. |
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) |
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
| // ssh://git@host:owner/repo → https://host/owner/repo (SCP colon under ssh) | ||
| s = s.replace(/^ssh:\/\/git@([^:/]+):(?=\D)/, 'https://$1/') |
| // scheme://host:owner/repo → scheme://host/owner/repo — the colon is an SCP path | ||
| // separator, not a port (guarded by \D so real numeric ports are left intact). | ||
| s = s.replace(/^(https?):\/\/([^:/]+):(?=\D)/, '$1://$2/') |
| if (!s.includes('://')) { | ||
| // Bare SCP form "host:owner/repo" → "host/owner/repo" before assuming https. | ||
| s = s.replace(/^([^/:]+):(?=\D)/, '$1/') | ||
| s = `https://${s}` | ||
| } |
| // git:// → https://, and upgrade http:// → https:// — done before the SCP-colon | ||
| // rule below so that git://host:owner/repo is normalised too. | ||
| s = s.replace(/^git:\/\//, 'https://').replace(/^http:\/\//, 'https://') |
| const missingLicense = licenses.length === 0 | ||
| const missingScm = !scmUrl | ||
| // An unresolved ${...} placeholder counts as missing: the property that defines it | ||
| // may live in a parent POM, so we still need to walk the chain to collect it. | ||
| const missingScm = !scmUrl || scmUrl.includes('${') | ||
| const missingDevelopers = developers.length === 0 || contributors.length === 0 |
| if (updates.length > 0 && !dryRun) { | ||
| // Atomic per batch: the repository_url UPDATE, the stale-link prune, and the | ||
| // relink must commit together. Otherwise an interrupt between them leaves | ||
| // packages.repository_url updated but package_repos out of sync — and on a | ||
| // re-run the row is skipped (its repository_url already matches `desired`), | ||
| // so the inconsistency would never be repaired. On rollback the row stays | ||
| // unchanged and is reprocessed on the next run. | ||
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
| export type MavenRepoUrlRow = { | ||
| id: number | ||
| declaredRepositoryUrl: string | null | ||
| repositoryUrl: string | null | ||
| } | ||
|
|
||
| /** | ||
| * Keyset-paginated scan of Maven rows that carry a repository link (declared or | ||
| * canonical). Rows with neither are skipped — there is nothing to recompute. | ||
| * Used by the repository_url backfill to re-run the normalizer over stored data | ||
| * without re-fetching POMs from the registry. | ||
| * | ||
| * `criticalOnly` restricts the scan to is_critical rows (index-backed by the | ||
| * partial index on is_critical) — used for a fast, consumer-facing first pass. | ||
| */ | ||
| export async function listMavenPackagesForRepoUrlRecompute( | ||
| qx: QueryExecutor, | ||
| options: { afterId: number; limit: number; criticalOnly?: boolean }, | ||
| ): Promise<MavenRepoUrlRow[]> { |
| let afterId = 0 | ||
| for (;;) { | ||
| if (isShuttingDown()) { | ||
| log.info('Shutdown requested — stopping after the current batch.') | ||
| break | ||
| } | ||
|
|
||
| const rows = await listMavenPackagesForRepoUrlRecompute(qx, { | ||
| afterId, | ||
| limit: batchSize, | ||
| criticalOnly, | ||
| }) | ||
| if (rows.length === 0) break | ||
|
|
||
| const updates: { id: number; repositoryUrl: string | null }[] = [] | ||
| // Rows that had a link and now change/clear — their stale 'declared' link is pruned. | ||
| const pruneTargets: number[] = [] | ||
| // Rows that gained a canonical URL — their repo link is (re)written after the update. | ||
| const linkTargets: { id: number; repositoryUrl: string }[] = [] |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
| confidence: 0.8, | ||
| }) | ||
| repoChanged.forEach((f) => changed.add(f)) | ||
| repoChanged.forEach((f) => changed?.add(f)) |
There was a problem hiding this comment.
Apache URLs misparsed for repo links
Medium Severity
New Apache gitweb canonical URLs like https://gitbox.apache.org/repos/asf/commons-lang are written to repository_url and passed through writeRepoLink, but parseRepoUrl only reads the first two path segments, so owner/name become repos/asf instead of the actual project. repos and package_repos end up inconsistent with the stored URL.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 970ad5e. Configure here.
| totals.linked += linkTargets.length | ||
| } | ||
|
|
||
| afterId = rows[rows.length - 1].id |
There was a problem hiding this comment.
Repo backfill uses unsafe id cursor
Low Severity
The repo-url backfill paginates with a numeric afterId and MavenRepoUrlRow.id typed as number, while the same PR’s force Maven backfill documents that package ids are Postgres bigint values that lose precision when coerced to JavaScript numbers, which can skip rows during keyset scans.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 970ad5e. Configure here.
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) |
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
| // ─── repository_url backfill ────────────────────────────────────────────────── | ||
|
|
||
| export type MavenRepoUrlRow = { | ||
| id: number |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f8c9e2f. Configure here.
| const missingScm = !scmUrl | ||
| // An unresolved ${...} placeholder counts as missing: the property that defines it | ||
| // may live in a parent POM, so we still need to walk the chain to collect it. | ||
| const missingScm = !scmUrl || scmUrl.includes('${') |
There was a problem hiding this comment.
Unresolved SCM keeps parent URL
Medium Severity
Parent-chain walks now treat a child SCM value containing ${...} as missing, but the merge still prefers any non-null child scmUrl over the parent’s concrete URL. When placeholders cannot be resolved from merged properties, enrichment keeps the literal template, normalizeScmUrl returns null, and a valid inherited parent SCM URL is discarded.
Reviewed by Cursor Bugbot for commit f8c9e2f. Configure here.
| export type MavenRepoUrlRow = { | ||
| id: number | ||
| declaredRepositoryUrl: string | null | ||
| repositoryUrl: string | null | ||
| } |
| await qx.tx(async (t) => { | ||
| await updateMavenRepositoryUrls(t, updates) | ||
| await deleteMavenPackageRepoLinks(t, pruneTargets) | ||
| for (const target of linkTargets) { | ||
| await writeRepoLink(t, target.id, target.repositoryUrl) | ||
| } | ||
| }) |
Summary
Fixes the Maven half of the
repository_urlnormalization problem:packages.repository_urlmust always hold a canonicalhttps://<host>/<owner>/<repo>link orNULL, never a website, placeholder, or free-form string. The Maven normalizer (normalizeScmUrl) had two defects:startsWith('https://')discarded inputs whose repo was clearly reconstructable:scm:git:prefixes without a scheme, barehost/owner/repovalues, andhttp://URLs that were never upgraded tohttps://. This leftrepository_urlNULL for ~18.8k critical Maven rows where the declared value was a valid GitHub URL.https://meson.ai/) passed straight through and were stored as if they were repository links.Since the public API falls back to
declared_repository_urlwhenrepository_urlis NULL, these gaps meant clients received either nothing or raw, non-normalized registry values.This PR rewrites
normalizeScmUrlto parse and validate instead of regex-guess, and adds a DB-only backfill to correct existing rows.Scope: Maven only. Cargo/npm/NuGet gaps, the shared normalizer + ADR, and the API fallback removal are tracked separately.
Changes
normalizeScmUrl(services/apps/packages_worker/src/maven/extract.ts) to normalize viaURL()parsing rather than a regex chain. It now: stripsscm:git:/scm:/git+prefixes and SCP/ssh:///git://forms; prependshttps://to schemeless inputs and upgradeshttp://→https://(Gap B); requires a known SCM host and anowner/repopath shape, returning NULL otherwise (Gap C); and lower-cases the path on case-insensitive hosts (github/gitlab).SCM_HOSTS/CASE_INSENSITIVE_HOSTSare markedTODO(CM)pending product confirmation of the final host list. The trade-off: an allowlist reliably rejects doc-sites but also drops self-hosted git (e.g.gitbox.apache.org); needs a decision before rollout.maven/__tests__/normalize.test.ts). All pre-existing cases still pass, includingsvn://→ NULL.backfill:maven. The existing backfill re-fetches every POM over the network (~18.8k+ HTTP calls) and — critically — cannot clear Gap C junk, because the enrichment upsertCOALESCEs and can never write NULL. The new path re-runs the normalizer over the already-storeddeclared_repository_url(zero network) and applies a direct UPDATE, so it both fills recoverable NULLs (Gap B) and clears non-repo values (Gap C).services/apps/packages_worker/src/maven/backfillRepositoryUrl.ts— keyset-paginated, idempotent, resumable orchestration; counts filled/cleared/rewritten/unchanged/linked/pruned; supports--dry-run.repository_url. This matters because consumers like the security-contacts pipeline read the repository throughrepos ⋈ package_repos, notpackages.repository_url— so a fill that only touchedpackageswould stay invisible to them. On rewrites and clears the stalesource='declared'link is pruned; on fills and rewrites the correct link is (re)written via the enrichment loop'swriteRepoLink(now exported fromrunMavenEnrichmentLoop.ts). This is deliberately stricter than the incremental enrichment path, which is upsert-only and never prunes — so the backfill also cleans up pre-existing stale links rather than leaving zombies.services/apps/packages_worker/src/bin/maven-repo-url-backfill.ts— bin entrypoint with graceful shutdown and positive-integer validation of the batch-size env var.services/libs/data-access-layer/src/osspckgs/packages.ts— newlistMavenPackagesForRepoUrlRecompute+updateMavenRepositoryUrls(splits clears from sets to avoid NULLs inside atext[]literal).services/libs/data-access-layer/src/osspckgs/repos.ts— newdeleteMavenPackageRepoLinks(removes onlysource='declared'links; never deletes sharedreposrows).package.json— newbackfill:maven-repo-url[:local]scripts.last_synced_at, so it doesn't disturb the enrichment freshness window.Type of change
JIRA ticket
CM-1305
Note
Medium Risk
Bulk updates to
packages.repository_urlandpackage_reposaffect security-contacts and other repo join consumers; host allowlist is marked provisional (TODO) and may drop valid self-hosted URLs until confirmed.Overview
Maven
repository_urlis tightened so stored values are canonicalhttps://…/owner/repolinks orNULL, fixing recoverable SCM strings that were nulled and junk URLs that were kept.normalizeScmUrlis rewritten aroundURL()parsing: SCM host allowlisting, owner/repo shape checks, Apache gitweb and Eclipse cgit handling, SCP colon forms, placeholder rejection, and http→https / schemeless recovery.interpolatePropertiesresolves${…}in SCM URLs during POM extraction using merged parent-chain<properties>and built-inproject.*coordinates; parent walks now treat unresolved${in SCM as missing.Operational backfills:
backfill:maven-repo-urlrecomputesrepository_urlfromdeclared_repository_urlwith no POM fetch, uses direct UPDATEs (including NULL clears), and in one transaction updatespackage_repos(deleteMavenPackageRepoLinks+ exportedwriteRepoLink).backfill:maven --forcerunsrunMavenCriticalForceBackfill(keyset by packageid, full POM re-extraction) when staleness-based backfill cannot drain the queue.DAL adds
listMavenCriticalPackagesById,listMavenPackagesForRepoUrlRecompute, andupdateMavenRepositoryUrls. Extensive tests cover Gap B/C normalization and interpolation.Reviewed by Cursor Bugbot for commit f8c9e2f. Bugbot is set up for automated code reviews on this repo. Configure here.