feat: fix maven repo gap (CM-1305)#4311
Open
ulemons wants to merge 10 commits into
Open
Conversation
![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.
Contributor
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.
Comment on lines
+67
to
+69
| if (updates.length > 0 && !dryRun) { | ||
| await updateMavenRepositoryUrls(qx, updates) | ||
| } |
Comment on lines
+526
to
+531
| 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 |
Comment on lines
+98
to
+114
| 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 | ||
| } |
Comment on lines
+105
to
+111
| 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) | ||
| } | ||
| }) |
Comment on lines
+471
to
+472
| * - ambiguous `git.*` hosts (git.iem.at, git.i-novus.ru, git.oschina.net) and the | ||
| * internal git.corp.adobe.com: pending path/reachability confirmation. |
Comment on lines
+105
to
+111
| 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) | ||
| } | ||
| }) |
Comment on lines
+111
to
+114
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
Comment on lines
+565
to
+566
| // ssh://git@host:owner/repo → https://host/owner/repo (SCP colon under ssh) | ||
| s = s.replace(/^ssh:\/\/git@([^:/]+):(?=\D)/, 'https://$1/') |
Comment on lines
+575
to
+577
| // 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/') |
Comment on lines
+580
to
+584
| if (!s.includes('://')) { | ||
| // Bare SCP form "host:owner/repo" → "host/owner/repo" before assuming https. | ||
| s = s.replace(/^([^/:]+):(?=\D)/, '$1/') | ||
| s = `https://${s}` | ||
| } |
Comment on lines
+571
to
+573
| // 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://') |
Comment on lines
282
to
286
| 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 |
Comment on lines
+98
to
+114
| 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 | ||
| } |
Comment on lines
+158
to
+176
| 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[]> { |
Comment on lines
+63
to
+81
| 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 }[] = [] |
| 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.
Comment on lines
+105
to
+111
| 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) | ||
| } | ||
| }) |
Comment on lines
+111
to
+114
| }) | ||
| totals.pruned += pruneTargets.length | ||
| totals.linked += linkTargets.length | ||
| } |
| // ─── repository_url backfill ────────────────────────────────────────────────── | ||
|
|
||
| export type MavenRepoUrlRow = { | ||
| id: number |
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.
Comment on lines
+158
to
+162
| export type MavenRepoUrlRow = { | ||
| id: number | ||
| declaredRepositoryUrl: string | null | ||
| repositoryUrl: string | null | ||
| } |
Comment on lines
+105
to
+111
| 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) | ||
| } | ||
| }) |
Comment on lines
+877
to
+881
| const segments = parsed.pathname.split('/').filter(Boolean) | ||
| if (segments.length < 2) return null | ||
|
|
||
| // Strip trailing .git | ||
| url = url.replace(/\.git$/, '') | ||
| let owner = segments[0] | ||
| let name = segments[1].replace(/\.git$/, '') |
Comment on lines
+85
to
+87
| if (desired === row.repositoryUrl) { | ||
| totals.unchanged++ | ||
| continue |
| // 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) => { |
Comment on lines
+158
to
+162
| export type MavenRepoUrlRow = { | ||
| id: number | ||
| declaredRepositoryUrl: string | null | ||
| repositoryUrl: string | null | ||
| } |
|
|
||
| // prettier-ignore | ||
| async function writeRepoLink(qx: QueryExecutor, packageId: number, repositoryUrl: string | null, changed: Set<string>): Promise<void> { | ||
| export async function writeRepoLink(qx: QueryExecutor, packageId: number, repositoryUrl: string | null, changed?: Set<string>): Promise<void> { |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
ulemons
force-pushed
the
fix/maven-repo-gap
branch
from
b700368 to
53455dd
Compare
Comment on lines
+880
to
+881
| let owner = segments[0] | ||
| let name = segments[1].replace(/\.git$/, '') |
Comment on lines
+283
to
+285
| // 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('${') |
Comment on lines
+85
to
+87
| if (desired === row.repositoryUrl) { | ||
| totals.unchanged++ | ||
| continue |
Comment on lines
+158
to
+160
| export type MavenRepoUrlRow = { | ||
| id: number | ||
| declaredRepositoryUrl: string | null |
Comment on lines
+492
to
+493
| * - ambiguous `git.*` hosts (git.iem.at, git.i-novus.ru, git.oschina.net) and the | ||
| * internal git.corp.adobe.com: pending path/reachability confirmation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 downstream consumers (e.g. security-contacts); host allowlist choices can mis-link or drop valid self-hosted SCM until product confirms the list.Overview
Fixes Maven
repository_urlso stored values are canonical HTTPS repo links orNULL, not homepages or raw registry strings.normalizeScmUrlis rewritten to parse withURL(), normalize Maven SCM shapes (prefixes, SCP/ssh/git://, schemeless hosts,http→https), and accept only known hosting patterns via allowlists plus dedicated handlers (Apache gitweb, GitHub Pages/CDN aliases, Eclipse cgit, Bitbucket Server, Azure DevOps, Aliyun Codeup, googlesource, etc.). Unresolved${...}in owner/repo paths and non-SCM URLs are rejected (Gap C); previously dropped recoverable inputs are filled (Gap B).POM extraction now merges
<properties>along the parent chain and runsinterpolatePropertieson SCM URLs before normalization; parent walk treats placeholder SCM as missing.backfill:maven --forceaddsrunMavenCriticalForceBackfill(keyset by package id, bigint-safe) to re-fetch every critical row after logic changes.A separate
backfill:maven-repo-urljob recomputesrepository_urlfrom storeddeclared_repository_urlwith no POM fetch, uses directUPDATE(including NULL clears), and in one transaction updatespackage_repos(declaredprune + relink via exportedwriteRepoLink). DAL adds list/update/delete helpers for these scans.Reviewed by Cursor Bugbot for commit 53455dd. Bugbot is set up for automated code reviews on this repo. Configure here.