chore(go-packages):SP-4143 add support for golang packages#9
Open
agustingroh wants to merge 1 commit intomainfrom
Open
chore(go-packages):SP-4143 add support for golang packages#9agustingroh wants to merge 1 commit intomainfrom
agustingroh wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds Golang package lookup via a new GolangProjects model (DB-backed golang_projects), pkg.go.dev integration, semver-aware URL selection, service-layer Golang PURL handling with GitHub fallback, dependency updates, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ComponentService
participant GolangProjects
participant Database
participant PkgGoDev
participant GitHub
Client->>ComponentService: GetComponent(purlString)
ComponentService->>GolangProjects: GetGoLangURLByPurlString(purlString)
GolangProjects->>Database: Query golang_projects by name/type/version
alt Database has matching, valid record
Database-->>GolangProjects: AllURL (version, license, repo)
GolangProjects-->>ComponentService: Return AllURL
else Database missing or incomplete
GolangProjects->>PkgGoDev: queryPkgGoDev / getLatestPkgGoDev
PkgGoDev-->>GolangProjects: Package metadata
GolangProjects->>Database: savePkg(package data)
GolangProjects-->>ComponentService: AllURL from pkg.go.dev
end
alt Golang resolution incomplete or not fully resolved
ComponentService->>ComponentService: convertGolangToGithubPurl()
ComponentService->>ComponentService: CheckPurl(GitHub PURL) / fallback lookup
ComponentService-->>Client: Return component from GitHub lookup or not found
else Fully resolved
ComponentService-->>Client: Return Golang AllURL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
agustingroh
force-pushed
the
chore/SP-4143-handle-golang-packages
branch
from
e1bde95 to
8f06c40
Compare
agustingroh
force-pushed
the
chore/SP-4143-handle-golang-packages
branch
from
8f06c40 to
69de0d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
pkg/models/all_urls.go (1)
140-143: PreferPickOneURLbefore this export becomes sticky.The package already uses
AllURLandGetURLs...; keeping theURLinitialism uppercase would make this new public helper consistent with the rest of the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/all_urls.go` around lines 140 - 143, Rename the exported helper from PickOneUrl to PickOneURL to match the package's existing initialism style (e.g., AllURL, GetURLs); update the function declaration name (PickOneUrl -> PickOneURL) and all internal and external references/call sites, plus any tests or documentation that reference PickOneUrl, ensuring the signature and behavior remain unchanged.pkg/models/models.go (1)
38-45: Consider reusing the model instances you already construct here.
NewModelsnow buildsVersions,Licenses,Mines, andProjects, thenNewGolangProjectModel(db)creates another copy of those wrappers insidepkg/models/golang_projects.goat Lines 32-46. Reusing the existing instances would keep dependency wiring centralized and avoid drift if any of those models later gain state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/models.go` around lines 38 - 45, The Models initialization creates Versions, Licenses, Mines and Projects then NewGolangProjectModel(db) reconstructs those same wrapper instances in pkg/models/golang_projects.go; change wiring so the GolangProjects model reuses the already-created instances instead of creating new ones — e.g., modify NewGolangProjectModel to accept the existing Models (or the individual dependencies Versions, Licenses, Mines, Projects) and update the Models constructor to pass those instances (refer to Models struct initialization, NewGolangProjectModel, and the Versions/Licenses/Mines/Projects constructors) so all wrappers share the same instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-12: The changelog entry for version header "## [0.7.0] -
2026-03-11" is pre-dated; keep the 0.7.0 changes under an "[Unreleased]" section
until release day by moving the "Added" bullet referencing the GolangProjects
model and pkg.go.dev integration into the "## [Unreleased]" section (or rename
the existing header to "## [Unreleased]") and remove the release date, so the
file no longer presents 0.7.0 as already shipped.
In `@pkg/models/all_urls.go`:
- Around line 151-158: The version-constraint parsing error from
semver.NewConstraint is currently only logged and ignored (leaving c nil), which
allows invalid requirement strings to be silently bypassed; change the error
handling in the block where semver.NewConstraint(purlReq) is called so that on
err you immediately return an error (e.g., return empty AllURL and fmt.Errorf
with context including purlReq, purlName, purlType and err) instead of just
s.Warnf; this makes
GetGolangUrlsByPurlNameType/GetGolangUrlsByPurlNameTypeVersion propagate the
parsing failure to callers and prevents skipping the constraint check that
relies on c.
In `@pkg/models/golang_projects_test.go`:
- Around line 110-115: Each test that calls testutils.SqliteSetup(t) in
pkg/models/golang_projects_test.go should immediately defer testutils.CloseDB(t,
db) to avoid leaking DB connections—add defer testutils.CloseDB(t, db) right
after the db := testutils.SqliteSetup(t) call in the setup blocks (e.g., the
blocks that create db and call NewGolangProjectModel) the same way
TestGolangProjectUrlsSearch does so the sqlite handle is closed for each test.
- Around line 234-249: The test golang_projects_test.go currently performs live
pkg.go.dev lookups via getLatestPkgGoDev() which calls queryPkgGoDev() and
creates a real pkggodevclient that calls DescribePackage() using
context.Background(), making tests flaky and preventing deterministic DB error
testing; fix by either gating these assertions behind an integration flag (e.g.,
only run when integration tests enabled) or refactor
queryPkgGoDev/getLatestPkgGoDev to accept an injected pkgGoDev client interface
(mockable) and a context with timeout so unit tests can supply a fake client
that simulates HTTP responses and DB error paths deterministically (ensure the
bad-SQL/drop-table path is exercised by the mock, not by real network calls).
In `@pkg/models/golang_projects.go`:
- Around line 327-350: CheckPurlByNameType currently returns count(*) and omits
the is_indexed predicate, which diverges from ProjectModel.CheckPurlByNameType
and breaks consistency with GetGolangUrlsByPurlNameType /
GetGolangUrlsByPurlNameTypeVersion and ComponentService.CheckPurl; modify
GolangProjects.CheckPurlByNameType to mirror ProjectModel.CheckPurlByNameType by
querying for a single matching row id (SELECT id ... LIMIT 1), include the
u.is_indexed = TRUE condition alongside u.purl_name and m.purl_type, scan the id
into an int and return that id (or -1 on not found) and keep error
logging/returns consistent with other CheckPurlByNameType implementations.
- Around line 303-316: The code retries a versioned DescribePackage call without
the version but then unconditionally sets version = purlVersion, which can
mislabel metadata from the latest release as the requested (nonexistent)
version; update the logic in the DescribePackage/comp handling so that you only
force version = purlVersion when the successful DescribePackage response
actually came from the versioned query. Concretely, record which package string
was used for the successful call (e.g., store queriedPkg before calling
client.DescribePackage or track a boolean like usedRequestedVersion), and after
a successful response set version = purlVersion only if the successful query was
for the versioned package (purlName+purlVersion or however the versioned pkg was
formed); otherwise leave version = comp.Version so fallback results aren’t
falsely labeled with purlVersion.
In `@pkg/models/projects_test.go`:
- Around line 111-112: TestProjectsSearchBadSql currently drops the projects
table but does not close the sqlite handle returned by testutils.SqliteSetup,
causing a leaked DB connection; update the test to call testutils.CloseDB(t, db)
via defer immediately after db is created (i.e., after testutils.SqliteSetup(t))
so the connection is released even when the test modifies schema, referencing
the db variable and the TestProjectsSearchBadSql test function.
In `@pkg/services/component.go`:
- Around line 109-113: The conversion for Golang GitHub PURLs clears an existing
version parsed from Requirement when convertGolangToGithubPurl(req.Purl) returns
an empty version; change the assignment logic so that after calling
convertGolangToGithubPurl you only overwrite purl.Version if the returned
version is non-empty (i.e., preserve any pre-existing purl.Version derived from
Requirement), and still update purl.Type and purlName as before; locate the
callsite around req.Purl handling and adjust the assignment to pick the
non-empty version (existing purl.Version or the converted one).
---
Nitpick comments:
In `@pkg/models/all_urls.go`:
- Around line 140-143: Rename the exported helper from PickOneUrl to PickOneURL
to match the package's existing initialism style (e.g., AllURL, GetURLs); update
the function declaration name (PickOneUrl -> PickOneURL) and all internal and
external references/call sites, plus any tests or documentation that reference
PickOneUrl, ensuring the signature and behavior remain unchanged.
In `@pkg/models/models.go`:
- Around line 38-45: The Models initialization creates Versions, Licenses, Mines
and Projects then NewGolangProjectModel(db) reconstructs those same wrapper
instances in pkg/models/golang_projects.go; change wiring so the GolangProjects
model reuses the already-created instances instead of creating new ones — e.g.,
modify NewGolangProjectModel to accept the existing Models (or the individual
dependencies Versions, Licenses, Mines, Projects) and update the Models
constructor to pass those instances (refer to Models struct initialization,
NewGolangProjectModel, and the Versions/Licenses/Mines/Projects constructors) so
all wrappers share the same instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e716862d-1306-4386-af71-e9e05102836c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
CHANGELOG.mdgo.modinternal/testutils/mock/golang_projects.sqlpkg/models/all_urls.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/models.gopkg/models/projects_test.gopkg/models/versions_test.gopkg/services/component.gopkg/services/component_test.go
Comment on lines
+10
to
+12
| ## [0.7.0] - 2026-03-11 | ||
| ### Added | ||
| - Added support for Golang packages lookup via `GolangProjects` model and `pkg.go.dev` integration |
There was a problem hiding this comment.
Keep 0.7.0 under [Unreleased] until release day.
This entry is dated March 11, 2026, but the PR is still open on March 10, 2026. Pre-dating the section makes the changelog read as if 0.7.0 has already shipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 10 - 12, The changelog entry for version header
"## [0.7.0] - 2026-03-11" is pre-dated; keep the 0.7.0 changes under an
"[Unreleased]" section until release day by moving the "Added" bullet
referencing the GolangProjects model and pkg.go.dev integration into the "##
[Unreleased]" section (or rename the existing header to "## [Unreleased]") and
remove the release date, so the file no longer presents 0.7.0 as already
shipped.
Comment on lines
+151
to
+158
| var c *semver.Constraints | ||
| if len(purlReq) > 0 { | ||
| s.Debugf("Building version constraint for %v: %v", purlName, purlReq) | ||
| var err error | ||
| c, err = semver.NewConstraint(purlReq) | ||
| if err != nil { | ||
| s.Warnf("Encountered an issue parsing version constraint string '%v' (%v,%v): %v", purlReq, purlName, purlType, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bPickOneUrl\s*\('Repository: scanoss/go-models
Length of output: 2047
🏁 Script executed:
cat -n pkg/models/all_urls.go | sed -n '143,200p'Repository: scanoss/go-models
Length of output: 2301
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '115,125p'Repository: scanoss/go-models
Length of output: 557
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '170,180p'Repository: scanoss/go-models
Length of output: 466
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '95,120p'Repository: scanoss/go-models
Length of output: 1377
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '139,176p'Repository: scanoss/go-models
Length of output: 2047
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '80,95p'Repository: scanoss/go-models
Length of output: 1028
🏁 Script executed:
cat -n pkg/models/golang_projects.go | sed -n '122,130p'Repository: scanoss/go-models
Length of output: 635
Return error when version constraint parsing fails.
If semver.NewConstraint fails at line 155, c remains nil and line 182's constraint check is skipped, allowing the function to return the highest available version without validating it against the requirement. This silently masks malformed requirement strings. Since the calling functions GetGolangUrlsByPurlNameType() (line 119) and GetGolangUrlsByPurlNameTypeVersion() (line 175) both return (AllURL, error) and other callers like component.go:130 already handle PickOneUrl errors, the error should propagate up instead.
Proposed fix
c, err = semver.NewConstraint(purlReq)
if err != nil {
s.Warnf("Encountered an issue parsing version constraint string '%v' (%v,%v): %v", purlReq, purlName, purlType, err)
+ return AllURL{}, fmt.Errorf("invalid version constraint %q: %w", purlReq, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/all_urls.go` around lines 151 - 158, The version-constraint
parsing error from semver.NewConstraint is currently only logged and ignored
(leaving c nil), which allows invalid requirement strings to be silently
bypassed; change the error handling in the block where
semver.NewConstraint(purlReq) is called so that on err you immediately return an
error (e.g., return empty AllURL and fmt.Errorf with context including purlReq,
purlName, purlType and err) instead of just s.Warnf; this makes
GetGolangUrlsByPurlNameType/GetGolangUrlsByPurlNameTypeVersion propagate the
parsing failure to callers and prevents skipping the constraint check that
relies on c.
Comment on lines
+110
to
+115
| db := testutils.SqliteSetup(t) // Setup SQL Lite DB | ||
| testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/golang_projects.sql") | ||
| testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/mines.sql") | ||
| testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/licenses.sql") | ||
| testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/versions.sql") | ||
| golangProjModel := NewGolangProjectModel(db) |
There was a problem hiding this comment.
Add DB cleanup to the remaining Golang tests.
Only TestGolangProjectUrlsSearch closes its sqlite handle. The setup blocks in the other four tests should also defer testutils.CloseDB(t, db) to avoid leaking DB connections across the package test run.
♻️ Proposed fix for each setup block
db := testutils.SqliteSetup(t) // Setup SQL Lite DB
+defer testutils.CloseDB(t, db)
testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/golang_projects.sql")Also applies to: 189-194, 222-227, 314-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/golang_projects_test.go` around lines 110 - 115, Each test that
calls testutils.SqliteSetup(t) in pkg/models/golang_projects_test.go should
immediately defer testutils.CloseDB(t, db) to avoid leaking DB connections—add
defer testutils.CloseDB(t, db) right after the db := testutils.SqliteSetup(t)
call in the setup blocks (e.g., the blocks that create db and call
NewGolangProjectModel) the same way TestGolangProjectUrlsSearch does so the
sqlite handle is closed for each test.
Comment on lines
+234
to
+249
| url, err := golangProjModel.getLatestPkgGoDev(ctx, "google.golang.org/grpc", "golang", "v0.0.0-201910101010-s3333") | ||
| if err != nil { | ||
| t.Errorf("FAILED: golang_projects.getLatestPkgGoDev() error = %v", err) | ||
| } | ||
| if len(url.PurlName) == 0 { | ||
| t.Errorf("FAILED: golang_projects.getLatestPkgGoDev() No URLs returned from query") | ||
| } | ||
| fmt.Printf("Golang URL Version: %#v\n", url) | ||
|
|
||
| url, err = golangProjModel.getLatestPkgGoDev(ctx, "github.com/scanoss/papi", "golang", "v0.0.3") | ||
| if err != nil { | ||
| t.Errorf("FAILED: golang_projects.getLatestPkgGoDev() error = %v", err) | ||
| } | ||
| if len(url.PurlName) == 0 { | ||
| t.Errorf("FAILED: golang_projects.getLatestPkgGoDev() No URLs returned from query") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the model/test code path for direct pkg.go.dev calls.
rg -n -C3 'getLatestPkgGoDev|queryPkgGoDev|pkggodevclient.New|DescribePackage' pkg/models/golang_projects.go pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 6694
🏁 Script executed:
# Check for integration flags, build tags, or test skip mechanisms
rg -n 'build tag|skip|Skip|integration|Integration|+build' pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 230
🏁 Script executed:
# Check for timeout configuration on the pkg.go.dev client
rg -n -A10 'pkggodevclient.New|timeout|Timeout|WithTimeout' pkg/models/golang_projects.goRepository: scanoss/go-models
Length of output: 675
🏁 Script executed:
# Get the full range of queryPkgGoDev implementation to verify line count claim
sed -n '291,330p' pkg/models/golang_projects.goRepository: scanoss/go-models
Length of output: 1734
🏁 Script executed:
# Check for integration flags, build tags, or test skip mechanisms (corrected regex)
rg -n 'build|skip|Skip|integration|Integration' pkg/models/golang_projects_test.go | head -20Repository: scanoss/go-models
Length of output: 43
🏁 Script executed:
# Check test file for any mock setup or test isolation
rg -n 'mock|Mock|stub|Stub|fake|Fake' pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 1416
🏁 Script executed:
# Check if there are context timeouts passed anywhere
rg -n 'context.WithTimeout|context.WithCancel|WithDeadline' pkg/models/golang_projects.go pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 43
🏁 Script executed:
# Check the test function context around the problematic lines
sed -n '210,250p' pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 1652
🏁 Script executed:
# Check if there's any timeout or context cancellation in the test setup
sed -n '215,260p' pkg/models/golang_projects_test.goRepository: scanoss/go-models
Length of output: 1956
🏁 Script executed:
# Check the full extent of queryPkgGoDev to verify line range claim
sed -n '290,320p' pkg/models/golang_projects.go | cat -nRepository: scanoss/go-models
Length of output: 1657
Remove live pkg.go.dev lookups from the default test suite.
Lines 233 and 242 in the test call getLatestPkgGoDev(), which invokes queryPkgGoDev() at lines 291–319. This function creates a real pkggodevclient (line 297) and makes unguarded HTTP calls via client.DescribePackage() (lines 303, 307) without mocks or timeouts. The test uses context.Background() with no deadline, making it flaky and offline-hostile.
Additionally, the bad-SQL check at line 332 can pass before dropped table errors are exercised, since the nonexistent package fails at the HTTP layer first.
Gate these assertions behind an integration flag or inject a mock client so DB failure paths can be asserted deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/golang_projects_test.go` around lines 234 - 249, The test
golang_projects_test.go currently performs live pkg.go.dev lookups via
getLatestPkgGoDev() which calls queryPkgGoDev() and creates a real
pkggodevclient that calls DescribePackage() using context.Background(), making
tests flaky and preventing deterministic DB error testing; fix by either gating
these assertions behind an integration flag (e.g., only run when integration
tests enabled) or refactor queryPkgGoDev/getLatestPkgGoDev to accept an injected
pkgGoDev client interface (mockable) and a context with timeout so unit tests
can supply a fake client that simulates HTTP responses and DB error paths
deterministically (ensure the bad-SQL/drop-table path is exercised by the mock,
not by real network calls).
Comment on lines
+303
to
+316
| comp, err := client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: pkg}) | ||
| if err != nil && len(purlVersion) > 0 { | ||
| // We have a version zero search, so look for the latest one | ||
| s.Debugf("Failed to query pkg.go.dev for %v: %v. Trying without version...", pkg, err) | ||
| comp, err = client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: purlName}) | ||
| } | ||
| if err != nil { | ||
| s.Warnf("Failed to query pkg.go.dev for %v: %v", pkg, err) | ||
| return AllURL{}, fmt.Errorf("failed to query pkg.go.dev: %v", err) | ||
| } | ||
| var version = comp.Version | ||
| if len(purlVersion) > 0 { | ||
| version = purlVersion // Force the requested version if specified (the returned value can be concatenated) | ||
| } |
There was a problem hiding this comment.
Don't rename the latest pkg.go.dev response to the requested version.
On any versioned lookup error, this code retries without the version and then forces version = purlVersion. That can make a nonexistent release look resolvable and return metadata from the latest release under the wrong version string.
Suggested fix
comp, err := client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: pkg})
if err != nil && len(purlVersion) > 0 {
- // We have a version zero search, so look for the latest one
- s.Debugf("Failed to query pkg.go.dev for %v: %v. Trying without version...", pkg, err)
- comp, err = client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: purlName})
+ return AllURL{}, fmt.Errorf("failed to query pkg.go.dev for requested version %s: %w", purlVersion, err)
}
if err != nil {
s.Warnf("Failed to query pkg.go.dev for %v: %v", pkg, err)
return AllURL{}, fmt.Errorf("failed to query pkg.go.dev: %v", err)
}
- var version = comp.Version
- if len(purlVersion) > 0 {
- version = purlVersion // Force the requested version if specified (the returned value can be concatenated)
- }
+ version := comp.Version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| comp, err := client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: pkg}) | |
| if err != nil && len(purlVersion) > 0 { | |
| // We have a version zero search, so look for the latest one | |
| s.Debugf("Failed to query pkg.go.dev for %v: %v. Trying without version...", pkg, err) | |
| comp, err = client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: purlName}) | |
| } | |
| if err != nil { | |
| s.Warnf("Failed to query pkg.go.dev for %v: %v", pkg, err) | |
| return AllURL{}, fmt.Errorf("failed to query pkg.go.dev: %v", err) | |
| } | |
| var version = comp.Version | |
| if len(purlVersion) > 0 { | |
| version = purlVersion // Force the requested version if specified (the returned value can be concatenated) | |
| } | |
| comp, err := client.DescribePackage(pkggodevclient.DescribePackageRequest{Package: pkg}) | |
| if err != nil && len(purlVersion) > 0 { | |
| return AllURL{}, fmt.Errorf("failed to query pkg.go.dev for requested version %s: %w", purlVersion, err) | |
| } | |
| if err != nil { | |
| s.Warnf("Failed to query pkg.go.dev for %v: %v", pkg, err) | |
| return AllURL{}, fmt.Errorf("failed to query pkg.go.dev: %v", err) | |
| } | |
| version := comp.Version |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/golang_projects.go` around lines 303 - 316, The code retries a
versioned DescribePackage call without the version but then unconditionally sets
version = purlVersion, which can mislabel metadata from the latest release as
the requested (nonexistent) version; update the logic in the
DescribePackage/comp handling so that you only force version = purlVersion when
the successful DescribePackage response actually came from the versioned query.
Concretely, record which package string was used for the successful call (e.g.,
store queriedPkg before calling client.DescribePackage or track a boolean like
usedRequestedVersion), and after a successful response set version = purlVersion
only if the successful query was for the versioned package (purlName+purlVersion
or however the versioned pkg was formed); otherwise leave version = comp.Version
so fallback results aren’t falsely labeled with purlVersion.
Comment on lines
+327
to
+350
| // CheckPurlByNameType checks the golang project table for the count of entries matching a Purl Name and Type. | ||
| func (m *GolangProjects) CheckPurlByNameType(ctx context.Context, purlName string, purlType string) (int, error) { | ||
| s := ctxzap.Extract(ctx).Sugar() | ||
| if len(purlName) == 0 { | ||
| s.Error("Please specify a valid Purl Name to query") | ||
| return -1, errors.New("please specify a valid Purl Name to query") | ||
| } | ||
| if len(purlType) == 0 { | ||
| s.Error("Please specify a valid Purl Type to query") | ||
| return -1, errors.New("please specify a valid Purl Type to query") | ||
| } | ||
| var count int | ||
| fmt.Printf("Checking golang projects table for %v, %v...\n", purlName, purlType) | ||
| err := m.db.QueryRowxContext(ctx, | ||
| "SELECT count(*)"+ | ||
| " FROM golang_projects u"+ | ||
| " INNER JOIN mines m ON u.mine_id = m.id"+ | ||
| " WHERE u.purl_name = $1 AND m.purl_type = $2", | ||
| purlName, purlType).Scan(&count) | ||
| if err != nil { | ||
| s.Errorf("Error: Failed to query projects table for %v, %v: %v", purlName, purlType, err) | ||
| return -1, fmt.Errorf("failed to query the projects table: %v", err) | ||
| } | ||
| return count, nil |
There was a problem hiding this comment.
Keep CheckPurlByNameType consistent with ProjectModel.CheckPurlByNameType.
The existing pkg/models/projects.go implementation returns a matching row ID, but this variant returns count(*) and also skips the is_indexed = True predicate used by GetGolangUrlsByPurlNameType and GetGolangUrlsByPurlNameTypeVersion. ComponentService.CheckPurl will therefore change meaning for Golang PURLs and can report hits that GetComponent won't resolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/golang_projects.go` around lines 327 - 350, CheckPurlByNameType
currently returns count(*) and omits the is_indexed predicate, which diverges
from ProjectModel.CheckPurlByNameType and breaks consistency with
GetGolangUrlsByPurlNameType / GetGolangUrlsByPurlNameTypeVersion and
ComponentService.CheckPurl; modify GolangProjects.CheckPurlByNameType to mirror
ProjectModel.CheckPurlByNameType by querying for a single matching row id
(SELECT id ... LIMIT 1), include the u.is_indexed = TRUE condition alongside
u.purl_name and m.purl_type, scan the id into an int and return that id (or -1
on not found) and keep error logging/returns consistent with other
CheckPurlByNameType implementations.
Comment on lines
111
to
+112
| db := testutils.SqliteSetup(t) // Setup SQL Lite DB | ||
| defer testutils.CloseDB(t, db) | ||
| _, _ = db.Exec("DROP TABLE IF EXISTS projects") |
There was a problem hiding this comment.
Restore CloseDB in TestProjectsSearchBadSql.
Dropping projects changes the schema, but it does not release the sqlite handle. This test no longer defers testutils.CloseDB(t, db), so it leaks the DB connection during the package test run.
♻️ Proposed fix
db := testutils.SqliteSetup(t) // Setup SQL Lite DB
+defer testutils.CloseDB(t, db)
_, _ = db.Exec("DROP TABLE IF EXISTS projects")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| db := testutils.SqliteSetup(t) // Setup SQL Lite DB | |
| defer testutils.CloseDB(t, db) | |
| _, _ = db.Exec("DROP TABLE IF EXISTS projects") | |
| db := testutils.SqliteSetup(t) // Setup SQL Lite DB | |
| defer testutils.CloseDB(t, db) | |
| _, _ = db.Exec("DROP TABLE IF EXISTS projects") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/projects_test.go` around lines 111 - 112, TestProjectsSearchBadSql
currently drops the projects table but does not close the sqlite handle returned
by testutils.SqliteSetup, causing a leaked DB connection; update the test to
call testutils.CloseDB(t, db) via defer immediately after db is created (i.e.,
after testutils.SqliteSetup(t)) so the connection is released even when the test
modifies schema, referencing the db variable and the TestProjectsSearchBadSql
test function.
Comment on lines
+109
to
+113
| if strings.HasPrefix(req.Purl, "pkg:golang/github.com/") { | ||
| purl.Type, purlName, purl.Version, err = convertGolangToGithubPurl(req.Purl) | ||
| if err != nil { | ||
| return types.ComponentResponse{}, err | ||
| } |
There was a problem hiding this comment.
Preserve the requirement-derived version through the GitHub fallback.
If an exact version was already pulled out of Requirement, re-parsing req.Purl here clears purl.Version back to empty for pkg:golang/github.com/... inputs without an inline version. The fallback then resolves the latest GitHub record instead of the requested release.
Suggested fix
// No component/license found — if it's a GitHub component, try GitHub lookup
if strings.HasPrefix(req.Purl, "pkg:golang/github.com/") {
- purl.Type, purlName, purl.Version, err = convertGolangToGithubPurl(req.Purl)
- if err != nil {
- return types.ComponentResponse{}, err
+ convertedType, convertedName, convertedVersion, convErr := convertGolangToGithubPurl(req.Purl)
+ if convErr != nil {
+ return types.ComponentResponse{}, convErr
}
+ purl.Type, purlName = convertedType, convertedName
+ if len(convertedVersion) > 0 {
+ purl.Version = convertedVersion
+ }
}A regression case with a GitHub-backed Golang PURL plus an exact Requirement would catch this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if strings.HasPrefix(req.Purl, "pkg:golang/github.com/") { | |
| purl.Type, purlName, purl.Version, err = convertGolangToGithubPurl(req.Purl) | |
| if err != nil { | |
| return types.ComponentResponse{}, err | |
| } | |
| if strings.HasPrefix(req.Purl, "pkg:golang/github.com/") { | |
| convertedType, convertedName, convertedVersion, convErr := convertGolangToGithubPurl(req.Purl) | |
| if convErr != nil { | |
| return types.ComponentResponse{}, convErr | |
| } | |
| purl.Type, purlName = convertedType, convertedName | |
| if len(convertedVersion) > 0 { | |
| purl.Version = convertedVersion | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/services/component.go` around lines 109 - 113, The conversion for Golang
GitHub PURLs clears an existing version parsed from Requirement when
convertGolangToGithubPurl(req.Purl) returns an empty version; change the
assignment logic so that after calling convertGolangToGithubPurl you only
overwrite purl.Version if the returned version is non-empty (i.e., preserve any
pre-existing purl.Version derived from Requirement), and still update purl.Type
and purlName as before; locate the callsite around req.Purl handling and adjust
the assignment to pick the non-empty version (existing purl.Version or the
converted one).
There was a problem hiding this comment.
♻️ Duplicate comments (5)
pkg/models/projects_test.go (1)
111-112:⚠️ Potential issue | 🟡 MinorRestore
defer testutils.CloseDB(t, db)to prevent connection leak.The removal of
defer testutils.CloseDB(t, db)leaves the sqlite handle open after the test completes, leaking the DB connection during the package test run.Proposed fix
db := testutils.SqliteSetup(t) // Setup SQL Lite DB +defer testutils.CloseDB(t, db) _, _ = db.Exec("DROP TABLE IF EXISTS projects")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/projects_test.go` around lines 111 - 112, The test opens an SQLite handle with testutils.SqliteSetup(t) but removed the defer that closes it; restore a deferred call to testutils.CloseDB(t, db) immediately after db := testutils.SqliteSetup(t) so the database connection is closed when the test exits (use the existing db variable and testutils.CloseDB function in projects_test.go).pkg/models/golang_projects_test.go (2)
109-114:⚠️ Potential issue | 🟡 MinorAdd
defer testutils.CloseDB(t, db)to prevent DB connection leak.This test and several others (
TestGolangProjectsSearchVersionRequirement,TestGolangPkgGoDev,TestGolangProjectsSearchBadSql) are missing the deferred close call present inTestGolangProjectUrlsSearch.Proposed fix
db := testutils.SqliteSetup(t) // Setup SQL Lite DB +defer testutils.CloseDB(t, db) testutils.LoadSQLDataFile(t, db, "../../internal/testutils/mock/golang_projects.sql")Also applies to lines 188, 221, and 313.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/golang_projects_test.go` around lines 109 - 114, The Sqlite DB opened with testutils.SqliteSetup(t) in these tests is not closed; add a deferred close immediately after creating the DB by inserting defer testutils.CloseDB(t, db) right after the db := testutils.SqliteSetup(t) call (where db is used to load fixtures and construct NewGolangProjectModel). Apply this change in the tests TestGolangProjectsSearchVersionRequirement, TestGolangPkgGoDev, TestGolangProjectsSearchBadSql and the existing TestGolangProjectUrlsSearch pattern so the DB connection is properly cleaned up.
233-249:⚠️ Potential issue | 🟠 MajorRemove live
pkg.go.devlookups from the default test suite.These tests call
getLatestPkgGoDev()which makes real HTTP calls topkg.go.devviapkggodevclient.New()andclient.DescribePackage()without mocks or timeouts. The tests usecontext.Background()with no deadline, making them flaky and offline-hostile.Consider gating these behind an integration build tag or injecting a mock client interface.
Also applies to line 332.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/golang_projects_test.go` around lines 233 - 249, Tests in golang_projects_test.go call golangProjModel.getLatestPkgGoDev which performs real network calls to pkg.go.dev (via pkggodevclient.New and client.DescribePackage) using context.Background() and no timeout; remove or disable these live lookups from the default unit test suite by either (a) moving the test cases that call getLatestPkgGoDev into a separate integration test file gated by a build tag (e.g., // +build integration) or (b) refactoring golangProjModel to accept an interface for the pkg.go.dev client and injecting a mock implementation in the tests so the unit tests do not perform real HTTP requests; apply the same change to the duplicate at the later test (around line ~332) so no default tests hit the network.pkg/services/component.go (1)
111-117:⚠️ Potential issue | 🟠 MajorPreserve the requirement-derived version through the GitHub fallback.
If an exact version was extracted from
Requirement(lines 93-99), re-parsingreq.PurlviaconvertGolangToGithubPurlclearspurl.Versionwhen the original PURL has no inline version. The fallback then resolves the latest GitHub record instead of the requested release.Proposed fix
// No component/license found — if it's a GitHub component, try GitHub lookup if strings.HasPrefix(req.Purl, "pkg:golang/github.com/") { - purl.Type, purlName, purl.Version, err = convertGolangToGithubPurl(req.Purl) - if err != nil { - return types.ComponentResponse{}, err + convertedType, convertedName, convertedVersion, convErr := convertGolangToGithubPurl(req.Purl) + if convErr != nil { + return types.ComponentResponse{}, convErr } + purl.Type, purlName = convertedType, convertedName + if len(convertedVersion) > 0 { + purl.Version = convertedVersion + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/component.go` around lines 111 - 117, When falling back to GitHub for golang purls, convertGolangToGithubPurl can clear purl.Version causing the earlier-extracted version from req.Requirement to be lost; before calling convertGolangToGithubPurl (inside the if strings.HasPrefix(req.Purl...) block) save the requirement-derived version (e.g., req.Requirement or the variable set earlier), call convertGolangToGithubPurl, and if the returned purl.Version is empty then restore it from the saved requirement-derived version so the lookup uses the requested release rather than the latest.pkg/models/all_urls.go (1)
151-158:⚠️ Potential issue | 🟠 MajorReturn error when version constraint parsing fails.
If
semver.NewConstraintfails,cremainsniland the constraint check at line 182 is skipped, allowing the function to return the highest available version without validating it against the requirement. This silently masks malformed requirement strings.Proposed fix
c, err = semver.NewConstraint(purlReq) if err != nil { s.Warnf("Encountered an issue parsing version constraint string '%v' (%v,%v): %v", purlReq, purlName, purlType, err) + return AllURL{}, fmt.Errorf("invalid version constraint %q: %w", purlReq, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/all_urls.go` around lines 151 - 158, The code logs but ignores errors from semver.NewConstraint (called when purlReq is non-empty), leaving c nil and skipping constraint checks; change the handling in the function in pkg/models/all_urls.go so that if semver.NewConstraint(purlReq) returns an error you return that error (wrap with context including purlReq, purlName, purlType) instead of only calling s.Warnf; adjust the function signature to propagate the error upward (e.g., return (..., error)) and update callers accordingly so malformed version requirement strings fail fast rather than silently returning an unvalidated highest version.
🧹 Nitpick comments (2)
pkg/services/component_test.go (1)
408-471: Good test coverage for Golang component resolution.Consider adding a test case for the scenario where a component is found but lacks license data (should trigger
errGolangNotResolvedfallback). This would exercise the license validation path at lines 158-161 in component.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/services/component_test.go` around lines 408 - 471, Add a test case to TestResolveGolangComponent that covers the path where a Golang component exists in the DB but has no license data so resolveGolangComponent returns the errGolangNotResolved fallback; call service.resolveGolangComponent with the purl of an existing golang project from your fixture that lacks license entries, and assert the result matches the expected fallback (nil result and shouldError true / wantNil true) so the license-validation branch in component.go (errGolangNotResolved) is exercised.pkg/models/all_urls.go (1)
197-200: Consider logging or handling the ProjectUrl error.The error from
purlutils.ProjectUrlis silently discarded. While the URL is a computed field and the primary match data is valid, logging this error would aid debugging when URL generation fails unexpectedly.Optional fix
s.Debugf("Selected highest version: %v", bestVersion) - bestURL.URL, _ = purlutils.ProjectUrl(purlName, purlType) + url, urlErr := purlutils.ProjectUrl(purlName, purlType) + if urlErr != nil { + s.Warnf("Failed to generate project URL for %v/%v: %v", purlType, purlName, urlErr) + } + bestURL.URL = url s.Debugf("Selected version: %#v", bestURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/all_urls.go` around lines 197 - 200, The call to purlutils.ProjectUrl in the all_urls.go snippet discards its error; update the code around bestURL.URL, _ = purlutils.ProjectUrl(purlName, purlType) (used with bestURL, purlName, purlType and s.Debugf) to capture the returned error and log it (e.g., s.Debugf/s.Errorf) with context including purlName/purlType and bestVersion; do not change the primary return behavior unless you want to escalate the failure—just record the error so URL generation failures are visible in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/models/all_urls.go`:
- Around line 151-158: The code logs but ignores errors from
semver.NewConstraint (called when purlReq is non-empty), leaving c nil and
skipping constraint checks; change the handling in the function in
pkg/models/all_urls.go so that if semver.NewConstraint(purlReq) returns an error
you return that error (wrap with context including purlReq, purlName, purlType)
instead of only calling s.Warnf; adjust the function signature to propagate the
error upward (e.g., return (..., error)) and update callers accordingly so
malformed version requirement strings fail fast rather than silently returning
an unvalidated highest version.
In `@pkg/models/golang_projects_test.go`:
- Around line 109-114: The Sqlite DB opened with testutils.SqliteSetup(t) in
these tests is not closed; add a deferred close immediately after creating the
DB by inserting defer testutils.CloseDB(t, db) right after the db :=
testutils.SqliteSetup(t) call (where db is used to load fixtures and construct
NewGolangProjectModel). Apply this change in the tests
TestGolangProjectsSearchVersionRequirement, TestGolangPkgGoDev,
TestGolangProjectsSearchBadSql and the existing TestGolangProjectUrlsSearch
pattern so the DB connection is properly cleaned up.
- Around line 233-249: Tests in golang_projects_test.go call
golangProjModel.getLatestPkgGoDev which performs real network calls to
pkg.go.dev (via pkggodevclient.New and client.DescribePackage) using
context.Background() and no timeout; remove or disable these live lookups from
the default unit test suite by either (a) moving the test cases that call
getLatestPkgGoDev into a separate integration test file gated by a build tag
(e.g., // +build integration) or (b) refactoring golangProjModel to accept an
interface for the pkg.go.dev client and injecting a mock implementation in the
tests so the unit tests do not perform real HTTP requests; apply the same change
to the duplicate at the later test (around line ~332) so no default tests hit
the network.
In `@pkg/models/projects_test.go`:
- Around line 111-112: The test opens an SQLite handle with
testutils.SqliteSetup(t) but removed the defer that closes it; restore a
deferred call to testutils.CloseDB(t, db) immediately after db :=
testutils.SqliteSetup(t) so the database connection is closed when the test
exits (use the existing db variable and testutils.CloseDB function in
projects_test.go).
In `@pkg/services/component.go`:
- Around line 111-117: When falling back to GitHub for golang purls,
convertGolangToGithubPurl can clear purl.Version causing the earlier-extracted
version from req.Requirement to be lost; before calling
convertGolangToGithubPurl (inside the if strings.HasPrefix(req.Purl...) block)
save the requirement-derived version (e.g., req.Requirement or the variable set
earlier), call convertGolangToGithubPurl, and if the returned purl.Version is
empty then restore it from the saved requirement-derived version so the lookup
uses the requested release rather than the latest.
---
Nitpick comments:
In `@pkg/models/all_urls.go`:
- Around line 197-200: The call to purlutils.ProjectUrl in the all_urls.go
snippet discards its error; update the code around bestURL.URL, _ =
purlutils.ProjectUrl(purlName, purlType) (used with bestURL, purlName, purlType
and s.Debugf) to capture the returned error and log it (e.g., s.Debugf/s.Errorf)
with context including purlName/purlType and bestVersion; do not change the
primary return behavior unless you want to escalate the failure—just record the
error so URL generation failures are visible in logs.
In `@pkg/services/component_test.go`:
- Around line 408-471: Add a test case to TestResolveGolangComponent that covers
the path where a Golang component exists in the DB but has no license data so
resolveGolangComponent returns the errGolangNotResolved fallback; call
service.resolveGolangComponent with the purl of an existing golang project from
your fixture that lacks license entries, and assert the result matches the
expected fallback (nil result and shouldError true / wantNil true) so the
license-validation branch in component.go (errGolangNotResolved) is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 273db9f3-5ed6-476c-bca8-9f714115e06b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
CHANGELOG.mdgo.modinternal/testutils/mock/golang_projects.sqlpkg/models/all_urls.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/models.gopkg/models/projects_test.gopkg/models/versions_test.gopkg/services/component.gopkg/services/component_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- internal/testutils/mock/golang_projects.sql
- pkg/models/versions_test.go
- pkg/models/golang_projects.go
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
1 participant
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 by CodeRabbit