fix: re-use canonicalizeRepoUrl & insert package_repos in go and nuget#4331
Conversation
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
|
Your PR title doesn't contain a Jira issue key. Consider adding it for better traceability. Example:
Projects:
Please add a Jira issue key to your PR title. |
|
|
There was a problem hiding this comment.
Pull request overview
Canonicalizes Go and NuGet repository URLs and persists package-to-repository links.
Changes:
- Uses shared repository URL canonicalization.
- Adds transactional
package_reposlinking and audit logging. - Includes Go package IDs for repository associations.
Issues: Existing declared links are not removed when repositories change. The title also incorrectly references Cargo and lacks a JIRA key.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
nuget/types.ts |
Adds canonical repository typing. |
nuget/normalize.ts |
Canonicalizes repository metadata. |
nuget/runNuGetEnrichmentLoop.ts |
Persists NuGet repository links. |
go/activities.ts |
Canonicalizes and links Go repositories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mbani01
changed the title
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let nuspecXml: string | null = null | ||
| if (preliminary.latestVersion) { | ||
| const nuspecResult = await fetchNuspec(packageId, preliminary.latestVersion) | ||
| nuspecXml = isNuGetFetchError(nuspecResult) ? null : nuspecResult |
There was a problem hiding this comment.
Nuspec fetch failure blocks package sync
Medium Severity
After registration and search succeed, fetchNuspec can throw on transient HTTP or network errors. That aborts processPackage, marks the package in error, and skips the DB upsert even though enrichment used to complete using catalog and homepage fallbacks when nuspec was not fetched.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ac2223. Configure here.
| const repo = | ||
| (nuspecRepoUrl ? canonicalizeRepoUrl(nuspecRepoUrl) : null) ?? | ||
| (isScmUrl(homepage) ? canonicalizeRepoUrl(homepage) : null) |
There was a problem hiding this comment.
@mbani01 do you think it maybe worth toreview this ?
| const preliminary = normalizeNuGetPackage(packageId, searchItem, registrationResult) | ||
|
|
||
| let nuspecXml: string | null = null | ||
| if (preliminary.latestVersion) { | ||
| const nuspecResult = await fetchNuspec(packageId, preliminary.latestVersion) |
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
| const repositoryUrl = normalizeRepoUrl(repoCandidate ?? undefined) | ||
| const repo = | ||
| (nuspecRepoUrl ? canonicalizeRepoUrl(nuspecRepoUrl) : null) ?? | ||
| (isScmUrl(homepage) ? canonicalizeRepoUrl(homepage) : null) |
There was a problem hiding this comment.
Homepage used when declared repo fails
Medium Severity
normalizeNuGetPackage resolves repo with nullish coalescing after canonicalizeRepoUrl on nuspecRepoUrl. When a declared repository URL is present but cannot be canonicalized, the code still links via the SCM-shaped homepage. That contradicts the nearby comment and can create incorrect package_repos rows while declaredRepositoryUrl stays different.
Reviewed by Cursor Bugbot for commit 36f39e4. Configure here.
| const isKnownHost = hostname in HOST_ENUM | ||
| let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1) | ||
| let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '') |
| let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1) | ||
| let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '') | ||
| if (!name || ownerPath.length === 0 || ownerPath.some((seg) => !seg)) return null | ||
|
|
||
| if (CASE_INSENSITIVE_HOSTS.has(hostname)) { | ||
| owner = owner.toLowerCase() | ||
| ownerPath = ownerPath.map((seg) => seg.toLowerCase()) | ||
| name = name.toLowerCase() | ||
| } | ||
|
|
||
| return { | ||
| url: `https://${hostname}/${owner}/${name}`, | ||
| url: `https://${hostname}/${[...ownerPath, name].join('/')}`, | ||
| host: HOST_ENUM[hostname] ?? 'other', |
| const resp = await axios.get<string>( | ||
| `${FLAT_CONTAINER_BASE_URL}/${lowerId}/${lowerVersion}/${lowerId}.nuspec`, | ||
| { | ||
| responseType: 'text', | ||
| transformResponse: (data) => data, | ||
| timeout: 15000, | ||
| }, |
| const nuspecParser = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: '@_' }) | ||
|
|
||
| function normalizeRepoUrl(url: string | undefined): string | null { | ||
| if (!url) return null | ||
| return url | ||
| .trim() | ||
| .replace(/\.git$/, '') | ||
| .replace(/^git\+/, '') | ||
| .replace(/^git:\/\//, 'https://') | ||
| .replace(/^http:\/\/github\.com\//, 'https://github.com/') | ||
| export function parseNuspecRepositoryUrl(nuspecXml: string): string | null { | ||
| try { | ||
| const doc = nuspecParser.parse(nuspecXml) | ||
| const url = doc?.package?.metadata?.repository?.['@_url'] | ||
| return typeof url === 'string' && url.trim() !== '' ? url.trim() : null | ||
| } catch { | ||
| return null | ||
| } |
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
Signed-off-by: Mouad BANI <mouad-mb@outlook.com>
| let owner = segments[0] | ||
| let name = segments[1].replace(/\.git$/, '') | ||
| if (!owner || !name) return null | ||
| const isKnownHost = hostname in HOST_ENUM |
| let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1) | ||
| let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '') |
| const nuspecRepoUrl = fetchedNuspecRepoUrl ?? catalogRepoUrl | ||
| const declaredRepositoryUrl = nuspecRepoUrl ?? null | ||
| const repoCandidate = nuspecRepoUrl ?? (isScmUrl(homepage) ? homepage : null) | ||
| const repositoryUrl = normalizeRepoUrl(repoCandidate ?? undefined) | ||
| const repo = | ||
| (nuspecRepoUrl ? canonicalizeRepoUrl(nuspecRepoUrl) : null) ?? | ||
| (isScmUrl(homepage) ? canonicalizeRepoUrl(homepage) : null) |
| const linkChanged = await upsertPackageRepo( | ||
| t, | ||
| packageDbId.toString(), | ||
| repoId, | ||
| 'declared', |
| ) | ||
| changedFields.push(...repoChanged) | ||
|
|
||
| const linkChanged = await upsertPackageRepo(t, row.id, repoId, 'declared', 0.8) |
| const isKnownHost = hostname in HOST_ENUM | ||
| let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1) | ||
| let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '') |
| const repoToLink = repo ?? declaredRepo | ||
| if (repoToLink) { |
| }) | ||
| pkgChanged.forEach((f) => changed.add(f)) | ||
|
|
||
| if (normalized.repo) { |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit db5be1a. Configure here.
| } catch (err) { | ||
| const classified = classifyError(err) | ||
| if (classified) return classified | ||
| throw err |
There was a problem hiding this comment.
Nuspec fetch aborts enrichment
Medium Severity
fetchNuspec rethrows most HTTP and network errors after registration has already succeeded. processPackage does not treat those as optional, so a transient flat-container failure can mark the whole package as errored instead of continuing without nuspec data.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit db5be1a. Configure here.
| const isKnownHost = hostname in HOST_ENUM | ||
| let ownerPath = isKnownHost ? [segments[0]] : segments.slice(0, -1) | ||
| let name = (isKnownHost ? segments[1] : segments[segments.length - 1]).replace(/\.git$/, '') |
| const linkChanged = await upsertPackageRepo( | ||
| t, | ||
| packageDbId.toString(), | ||
| repoId, | ||
| 'declared', | ||
| 0.8, | ||
| ) |
| const repoToLink = repo ?? declaredRepo | ||
| if (repoToLink) { | ||
| const { id: repoId, changedFields: repoChanged } = await getOrCreateRepoByUrl( | ||
| t, | ||
| repoToLink.url, | ||
| repoToLink.host, | ||
| ) | ||
| changedFields.push(...repoChanged) | ||
|
|
||
| const linkChanged = await upsertPackageRepo(t, row.id, repoId, 'declared', 0.8) | ||
| changedFields.push(...linkChanged) |
| // The registration API never exposes the nuspec <repository> element — it must be read from the | ||
| // raw nuspec XML, served by the flat-container endpoint. |
This pull request adds improved repository handling for both Go and NuGet package enrichment. It introduces fetching and parsing of repository URLs from NuGet
.nuspecfiles, canonicalizes and links repositories to packages in the database, and updates the data models to store normalized repository information. The most important changes are outlined below.NuGet repository enrichment and normalization:
fetchNuspecfunction innuget/client.tsto retrieve raw.nuspecXML files for extracting repository URLs not available in the NuGet registration API.nuget/normalize.tsto parse and canonicalize repository URLs from.nuspecXML, and to store repository info as a newrepoobject instead of a plain URL. [1] [2] [3] [4] [5] [6]runNuGetEnrichmentLoop.tsto fetch and use.nuspecdata, upsert repository records, and link packages to repositories in the database. [1] [2] [3]Go package repository linking:
activities.ts) to canonicalize repository URLs, upsert repository records, and link packages to repositories within a transaction. Also, updated the GoRow type and related queries to include package IDs for linking. [1] [2] [3] [4] [5] [6] [7]General improvements:
These changes ensure that repository information is more reliably and consistently extracted, normalized, and linked for both Go and NuGet packages.
Note
Medium Risk
Touches ingestion transactions and shared URL canonicalization, so bad parsing or linking could mis-associate packages to repos across ecosystems; scope is limited to packages_worker enrichment paths.
Overview
Go and NuGet enrichment now follow the same repository linking pattern as npm: URLs go through shared
canonicalizeRepoUrl,getOrCreateRepoByUrlcreates/updatesrepos, andupsertPackageRepowritespackage_reposwith sourcedeclaredand confidence0.8, with audit fields included in the same DB transaction.Go loads package
idanddeclared_repository_url, prefers the proxy repo URL when present otherwise the declared URL, and runs package updates plus repo linking insideqx.tx.NuGet adds
fetchNuspec(flat-container.nuspecXML) because registration JSON often omits<repository>, parses that URL inparseNuspecRepositoryUrl, and re-normalizes with nuspec-first then catalog/homepage fallbacks. The normalized shape exposesrepo: CanonicalRepo | nullinstead of a barerepositoryUrlstring.canonicalizeRepoUrlnow builds paths with a multi-segment owner path on non–known-host URLs (last segment = repo name), not onlyowner/nameon GitHub/GitLab/Bitbucket.Reviewed by Cursor Bugbot for commit db5be1a. Bugbot is set up for automated code reviews on this repo. Configure here.