Chore/sp 4138 add status batch response#50
Merged
agustingroh merged 7 commits intomainfrom Mar 20, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
📝 WalkthroughWalkthroughModels migrated from per-request Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as "dependency_service"
participant Usecase as "DependencyUseCase"
participant ComponentHelper as "componenthelper"
participant AllURLs as "AllUrlsModel"
participant License as "LicenseModel"
participant Response as "pb.DependencyResponse"
Client->>Service: GetDependencies(request)
Service->>Usecase: GetDependencies(dtoRequest)
Usecase->>ComponentHelper: GetComponentsVersion(PURLs, MaxWorkers=5)
ComponentHelper-->>Usecase: processedComponents (purl,name,type,version,status)
loop per component
alt status == Success
Usecase->>AllURLs: GetURLsByPurlString(component)
AllURLs-->>Usecase: allUrls
Usecase->>License: resolveLicenses(allUrls)
License-->>Usecase: licenseEntries
Usecase->>Usecase: assemble DependencyOutput (Success, version, licenses, url)
else status != Success
Usecase->>Usecase: assemble DependencyOutput (status with error_code/error_message)
end
end
Usecase-->>Service: DependenciesOutput
Service->>Service: convertDependencyOutput(dto) -> pb.DependencyResponse
Service-->>Client: return pb.DependencyResponse
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/models/versions.go (1)
80-84:⚠️ Potential issue | 🔴 CriticalRemove the extra bind arguments from the INSERT statement.
The query has 2 placeholders (
$1, $2) but 4 arguments are passed (name, "", false, false). This will fail at runtime inStructScan()with "sql: expected 2 arguments, got 4".Suggested fix
err := m.db.QueryRowxContext(m.ctx, "INSERT INTO versions (version_name, semver) VALUES($1, $2)"+ " RETURNING id, version_name, semver", - name, "", false, false, + name, "", ).StructScan(&version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/versions.go` around lines 80 - 84, The INSERT call in m.db.QueryRowxContext inside pkg/models/versions.go is passing too many bind parameters — the SQL has two placeholders ($1, $2) but the call provides name, "", false, false; update the call in the function where QueryRowxContext and .StructScan(&version) are used so only the arguments matching the placeholders are passed (e.g., pass just name and the semver value or the appropriate two variables) and remove the extra boolean arguments; ensure the values correspond to the INSERT columns (version_name, semver) so StructScan(&version) receives the returned row correctly.pkg/service/dependency_service.go (1)
77-85:⚠️ Potential issue | 🟠 MajorRemove the now-unused dedicated DB connection.
After dropping
connfromusecase.NewDependencies, Lines 77-83 only reserve a pool connection and can fail or throttle requests for no functional benefit.Suggested cleanup
- conn, err := d.db.Connx(ctx) // Get a connection from the pool - if err != nil { - s.Errorf("Failed to get a database connection from the pool: %v", err) - statusResp := common.StatusResponse{Status: common.StatusCode_FAILED, Message: "Failed to get database pool connection"} - return &pb.DependencyResponse{Status: &statusResp}, errors.NewServiceUnavailableError("problem getting database pool connection", err) - } - defer gd.CloseSQLConnection(conn) // Search the KB for information about each dependency depUc := usecase.NewDependencies(ctx, s, d.db, d.config)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_service.go` around lines 77 - 85, Remove the unused dedicated DB connection acquisition: delete the call to d.db.Connx(ctx), the associated error handling that returns a StatusResponse, and the deferred gd.CloseSQLConnection(conn) since usecase.NewDependencies no longer needs a raw connection; keep calling usecase.NewDependencies(ctx, s, d.db, d.config) as before. Ensure any imports or variables only used for conn are cleaned up to avoid unused symbol warnings (e.g., remove conn and related error handling code paths).pkg/usecase/dependency_test.go (1)
44-54:⚠️ Potential issue | 🟠 MajorReplace
:memory:DSN with shared-cache DSN to fix fixture visibility.These tests load fixtures through a reserved
connbut query throughdb. With SQLite:memory:, each pooled connection is isolated, so queries throughdbwon't see fixtures loaded viaconn. Use the shared-cache DSN (file::memory:?cache=shared) instead, like thesqliteSetup()helper incommon.goalready does.Also applies to: 89, 151–161, 169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency_test.go` around lines 44 - 54, The test uses sqlx.Connect with the ":memory:" DSN so fixtures loaded via the reserved conn (conn from db.Connx) aren't visible to queries made on db; change the in-memory DSN to the shared-cache DSN ("file::memory:?cache=shared") or reuse the existing sqliteSetup() helper so the connection pool shares the same in-memory DB, and ensure the code path around sqlx.Connect, db, conn, and models.LoadTestSQLData uses that shared DSN so fixtures are visible across connections.pkg/service/dependency_support.go (1)
222-223:⚠️ Potential issue | 🟠 MajorRemove the unconditional stdout dump of component data.
Unlike the existing debug logger, Lines 222-223 always print request-derived components to stdout. That bypasses log-level/redaction controls and can leak dependency inventory in production logs.
🧹 Suggested fix
- fmt.Printf("Valid components: %v\n", validComponents) - fmt.Printf("Invalid components: %v\n", invalidPurls) + s.Debugf("Filtered transitive dependency components: valid=%d invalid=%d", len(validComponents), len(invalidPurls))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 222 - 223, Remove the unconditional stdout prints that leak component data by deleting the two fmt.Printf calls (the ones printing validComponents and invalidPurls) and replace them with a controlled debug log call using the package's logger (e.g., logger.Debugf or similar) so output respects log levels and redaction; locate the prints by searching for fmt.Printf that references validComponents and invalidPurls in dependency_support.go and emit the same information via the service's debug logger instead of stdout.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
16-17: Keep the changelog focused on external behavior.These bullets read like implementation notes, not release notes.
convertDependencyOutputand the*sqlx.DBconstructor migration will age quickly and are harder for consumers to map to impact. I'd summarize the observable effect instead and keep the code-level refactor in the PR description/internal notes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 16 - 17, The changelog entries are too implementation-focused; replace the two code-level bullets with consumer-facing descriptions of observable behavior changes: for the convertDependencyOutput change, describe the user-visible outcome (e.g., "Dependency scan responses now include dependency metadata, license info, and explicit error status in responses") instead of naming the function; for the DB constructor change, describe the impact (e.g., "Data models now use pooled DB connections for improved stability/performance") rather than mentioning `*sqlx.DB` or `*sqlx.Conn`; move the detailed notes about `convertDependencyOutput` and the constructor refactor into the PR description or internal changelog for developers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 3: go.mod now declares "go 1.25.0" but CI workflows still set
"go-version: 1.24.x", causing mismatched toolchains; update every CI workflow
entry that contains the key "go-version: 1.24.x" to "go-version: 1.25.x" so the
YAML actions use the same Go version as declared in go.mod (search for the
literal "go-version: 1.24.x" in workflow YAML files and replace with
"go-version: 1.25.x").
- Around line 18-19: The Go modules for OpenTelemetry are skewed (root requires
otel v1.42.0 while transitive go-grpc-helper pins otel/sdk and exporters at
v1.40.0); fix by explicitly adding require entries for
go.opentelemetry.io/otel/sdk, go.opentelemetry.io/otel/sdk/metric and the OTLP
exporter modules at v1.42.0 in go.mod to override the transitive constraint,
include a short comment stating this override is needed until go-grpc-helper
updates, and run `go mod tidy` and tests (noting
pkg/service/dependency_support.go and pkg/protocol/grpc/server.go depend on this
stack) to verify compatibility.
In `@pkg/dtos/dependency_output.go`:
- Line 47: The DependenciesOutput struct's Status field (type
domain.ComponentStatus) lacks a JSON tag and will marshal as "Status" instead of
the intended "status"; update the struct field declaration for Status to include
`json:"status"` so it matches the sibling fields and preserves the API response
shape.
In `@pkg/service/dependency_support_test.go`:
- Around line 37-38: The test currently only prints the result of
convertDependencyOutput(outputDto); replace the print with assertions that
validate the converted value for an empty dtos.DependencyOutput: assert the
returned value from convertDependencyOutput(outputDto) is non-nil, any string
fields are empty, numeric fields are zero, boolean fields are false, and any
slices/maps have length zero. Use the test helper (t or require/assert) to check
these conditions so a broken convertDependencyOutput implementation will fail
the test.
In `@pkg/service/dependency_support.go`:
- Around line 91-97: The mapping currently skips setting d.ErrorCode when
dep.Status.StatusCode != domain.Success but dep.Status.Message is empty and also
takes &dep.Status.Message which binds to the loop variable; update the logic in
the loop around domain.StatusCodeToErrorCode, d.ErrorCode, d.ErrorMessage and
deps append so that you always set d.ErrorCode when dep.Status.StatusCode !=
domain.Success (regardless of message presence) and if you need a pointer to the
message, allocate a new string variable (e.g. msg := dep.Status.Message) and
assign &msg to d.ErrorMessage to avoid reusing the loop variable's address
before appending to deps.
In `@pkg/usecase/dependency_test.go`:
- Around line 127-139: The test currently expects every dependency returned by
depUc.GetDependencies to have an error status, but the input includes a valid
purl (pkg:npm/isbinaryfile) so the new component-level status model allows mixed
success/error results; update the assertions in the test (in
pkg/usecase/dependency_test.go) to stop failing on successes — instead verify
that (1) at least one dependency has a success status (compare
dep.Status.StatusCode to domain.Success for the known valid purl or by checking
dep.Purl == "pkg:npm/isbinaryfile"), and (2) at least one dependency has an
error status, or that warn is set appropriately, rather than asserting every dep
in dependencies.Files has an error.
In `@pkg/usecase/dependency.go`:
- Around line 90-93: The code currently skips appending a result when
d.allUrls.GetURLsByPurlString fails; instead of just logging and continue,
construct and append a failed DependenciesOutput for the current
processedComponent (include identifying fields like
processedComponent.Purl/Version and file.File) with a non-success Status (e.g.,
FAILED or ERROR) and any error message/details, then continue; update the logic
around d.allUrls.GetURLsByPurlString and depOutput to always push a
DependenciesOutput entry on error so batch semantics remain one-entry-per-input.
- Around line 43-49: The constructor NewDependencies/DependencyUseCase was
migrated to store *sqlx.DB but the service layer still calls db.Conn (checkout)
per RPC and never uses the connection; this pins pool connections. Remove
per-request conn checkout in the dependency service (remove any db.Conn/db.Connx
or conn variable usage in dependency_service.go), use the stored *sqlx.DB on
DependencyUseCase or pass *sqlx.DB into the called methods instead, and ensure
all model/service calls accept/use *sqlx.DB directly; also delete any unused
conn.Close/Release calls and fix call sites to stop expecting *sqlx.Conn.
---
Outside diff comments:
In `@pkg/models/versions.go`:
- Around line 80-84: The INSERT call in m.db.QueryRowxContext inside
pkg/models/versions.go is passing too many bind parameters — the SQL has two
placeholders ($1, $2) but the call provides name, "", false, false; update the
call in the function where QueryRowxContext and .StructScan(&version) are used
so only the arguments matching the placeholders are passed (e.g., pass just name
and the semver value or the appropriate two variables) and remove the extra
boolean arguments; ensure the values correspond to the INSERT columns
(version_name, semver) so StructScan(&version) receives the returned row
correctly.
In `@pkg/service/dependency_service.go`:
- Around line 77-85: Remove the unused dedicated DB connection acquisition:
delete the call to d.db.Connx(ctx), the associated error handling that returns a
StatusResponse, and the deferred gd.CloseSQLConnection(conn) since
usecase.NewDependencies no longer needs a raw connection; keep calling
usecase.NewDependencies(ctx, s, d.db, d.config) as before. Ensure any imports or
variables only used for conn are cleaned up to avoid unused symbol warnings
(e.g., remove conn and related error handling code paths).
In `@pkg/service/dependency_support.go`:
- Around line 222-223: Remove the unconditional stdout prints that leak
component data by deleting the two fmt.Printf calls (the ones printing
validComponents and invalidPurls) and replace them with a controlled debug log
call using the package's logger (e.g., logger.Debugf or similar) so output
respects log levels and redaction; locate the prints by searching for fmt.Printf
that references validComponents and invalidPurls in dependency_support.go and
emit the same information via the service's debug logger instead of stdout.
In `@pkg/usecase/dependency_test.go`:
- Around line 44-54: The test uses sqlx.Connect with the ":memory:" DSN so
fixtures loaded via the reserved conn (conn from db.Connx) aren't visible to
queries made on db; change the in-memory DSN to the shared-cache DSN
("file::memory:?cache=shared") or reuse the existing sqliteSetup() helper so the
connection pool shares the same in-memory DB, and ensure the code path around
sqlx.Connect, db, conn, and models.LoadTestSQLData uses that shared DSN so
fixtures are visible across connections.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 16-17: The changelog entries are too implementation-focused;
replace the two code-level bullets with consumer-facing descriptions of
observable behavior changes: for the convertDependencyOutput change, describe
the user-visible outcome (e.g., "Dependency scan responses now include
dependency metadata, license info, and explicit error status in responses")
instead of naming the function; for the DB constructor change, describe the
impact (e.g., "Data models now use pooled DB connections for improved
stability/performance") rather than mentioning `*sqlx.DB` or `*sqlx.Conn`; move
the detailed notes about `convertDependencyOutput` and the constructor refactor
into the PR description or internal changelog for developers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37fd348f-1e95-41b2-93b6-cb2e76f6a751
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
CHANGELOG.mdgo.modpkg/dtos/dependency_input.gopkg/dtos/dependency_output.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/licenses.gopkg/models/licenses_test.gopkg/models/mines.gopkg/models/mines_test.gopkg/models/projects.gopkg/models/projects_test.gopkg/models/versions.gopkg/models/versions_test.gopkg/service/dependency_service.gopkg/service/dependency_support.gopkg/service/dependency_support_test.gopkg/usecase/dependency.gopkg/usecase/dependency_test.go
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
from
6813088 to
c185e4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/models/versions.go (1)
80-84:⚠️ Potential issue | 🟠 MajorParameter count mismatch in INSERT statement.
The SQL query has 2 placeholders (
$1, $2) but 4 arguments are passed (name, "", false, false). The extra arguments will be silently ignored, indicating either incomplete refactoring or accidental extra parameters.🐛 Suggested fix
err := m.db.QueryRowxContext(m.ctx, "INSERT INTO versions (version_name, semver) VALUES($1, $2)"+ " RETURNING id, version_name, semver", - name, "", false, false, + name, "", ).StructScan(&version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/versions.go` around lines 80 - 84, The INSERT call in m.db.QueryRowxContext within pkg/models/versions.go passes four arguments (name, "", false, false) but the SQL has only two placeholders ($1, $2), causing a parameter mismatch; update the SQL to include the additional columns if the booleans and empty semver are intended (e.g., add the appropriate column names and $3, $4 placeholders) or remove the extra arguments and only pass name and semver (""), ensuring the call to QueryRowxContext and the subsequent StructScan(&version) match the final column list and returned fields.
♻️ Duplicate comments (2)
pkg/service/dependency_support.go (1)
91-99:⚠️ Potential issue | 🟠 MajorError code not set when message is empty; pointer aliasing on loop variable.
Two issues persist:
The condition
dep.Status.StatusCode != domain.Success && dep.Status.Message != ""skips settingErrorCodewhen status indicates failure but message is empty.
&dep.Status.Messagetakes the address of a field within the loop variabledep. Sincedepis reused across iterations, all error items will share the same pointer, causing the last message to appear in all.🐛 Suggested fix
- if dep.Status.StatusCode != domain.Success && dep.Status.Message != "" { - errorCode := domain.StatusCodeToErrorCode(dep.Status.StatusCode) - d.ErrorCode = errorCode - if dep.Status.Message != "" { - d.ErrorMessage = &dep.Status.Message - } + if dep.Status.StatusCode != domain.Success { + d.ErrorCode = domain.StatusCodeToErrorCode(dep.Status.StatusCode) + if dep.Status.Message != "" { + msg := dep.Status.Message + d.ErrorMessage = &msg + } deps = append(deps, d) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 91 - 99, The code currently only sets d.ErrorCode when dep.Status.Message is non-empty and takes &dep.Status.Message which aliases the loop variable; change the logic to check failure solely by dep.Status.StatusCode != domain.Success, always set d.ErrorCode = domain.StatusCodeToErrorCode(dep.Status.StatusCode), and if dep.Status.Message != "" create a new local string variable (e.g., msg := dep.Status.Message) and set d.ErrorMessage = &msg to avoid pointer aliasing before appending d to deps; ensure these changes are applied around the existing dep, d, dep.Status.Message, ErrorCode, ErrorMessage, domain.StatusCodeToErrorCode and deps append usage.pkg/service/dependency_support_test.go (1)
37-38:⚠️ Potential issue | 🟡 MinorTest lacks assertions.
The test only prints the converted value without asserting expected properties. A broken
convertDependencyOutputimplementation would still pass.💚 Suggested fix
output := convertDependencyOutput(outputDto) - fmt.Printf("Output: %v\n", output) + if output == nil { + t.Error("expected non-nil output") + } + if len(output.Files) != 0 { + t.Errorf("expected empty Files slice, got %d", len(output.Files)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support_test.go` around lines 37 - 38, The test currently only prints the result of convertDependencyOutput(outputDto) without verifying behavior; replace or supplement the fmt.Printf with assertions that validate the converted output's expected fields and values (e.g., check non-nil result, expected length, specific fields match input from outputDto, and any error/status fields). Locate the call to convertDependencyOutput and use the test framework's assertion helpers (t.Fatalf/t.Errorf or testify/require as used elsewhere) to assert correctness of the returned structure and key properties so the test fails when conversion is incorrect.
🧹 Nitpick comments (4)
.golangci.yml (1)
84-86: Consider excluding test files more precisely.The
paths: - testsexclusion only matches a directory namedtests. If test files follow the Go convention of*_test.gowithin packages, they may not be excluded. Consider usinggenerated: laxor adding*_test.gopatterns if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 84 - 86, The current .golangci.yml exclusions block uses paths: - tests which only matches a directory named tests; update the exclusions to precisely exclude Go test files by adding a glob pattern for *_test.go (e.g., add a paths entry like **/*_test.go) or enable generated: lax as needed; modify the exclusions.paths list where 'paths' and 'tests' are defined so that it includes the test-file glob (and/or set generated: lax) to ensure package-level *_test.go files are excluded.pkg/dtos/dependency_input.go (1)
23-26: Minor: Import grouping convention.The
componenthelperimport is grouped with stdlib imports. Consider separating external imports from stdlib for consistency.♻️ Suggested grouping
import ( "encoding/json" "errors" "fmt" - "github.com/scanoss/go-component-helper/componenthelper" + "github.com/scanoss/go-component-helper/componenthelper" "go.uber.org/zap" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dtos/dependency_input.go` around lines 23 - 26, The import grouping in dependency_input.go incorrectly places the external package "github.com/scanoss/go-component-helper/componenthelper" with standard library imports; update the import block to follow conventional grouping: keep stdlib imports together, place third-party imports (e.g., "github.com/scanoss/go-component-helper/componenthelper" and "go.uber.org/zap") in a separate group, and then local/project imports last so functions/types referencing componenthelper and zap remain unchanged (no code changes to functions like any using componenthelper).pkg/usecase/dependency.go (2)
91-99: Components are now correctly retained on URL lookup failure.The error handling now appends a
DependenciesOutputwithComponentNotFoundstatus instead of silently dropping the component. This maintains one-entry-per-input semantics.Minor style note:
fmt.Sprintf("component not found")can be simplified to just"component not found"since there are no format arguments.🧹 Simplify string literal
depOutput.Status = domain.ComponentStatus{ - Message: fmt.Sprintf("component not found"), + Message: "component not found", StatusCode: domain.ComponentNotFound, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency.go` around lines 91 - 99, Replace the unnecessary use of fmt.Sprintf in the error branch of GetURLsByPurlString handling: in the block inside the loop where url, err := d.allUrls.GetURLsByPurlString(...) leads to setting depOutput.Status = domain.ComponentStatus{ Message: fmt.Sprintf("component not found"), ... }, simplify Message to the plain string "component not found" (update the assignment in the code that constructs depOutput.Status inside that error branch in pkg/usecase/dependency.go).
69-75: Consider makingMaxWorkersconfigurable.The
MaxWorkers: 5is hardcoded. If this service handles variable load or needs tuning for different deployment environments, exposing this viaconfigwould provide more flexibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency.go` around lines 69 - 75, The code hardcodes MaxWorkers: 5 in the call to componentHelper.GetComponentsVersion which reduces configurability; update the call to read MaxWorkers from configuration (e.g., a field on the usecase/dependency receiver such as d.cfg.MaxWorkers or a passed-in options value) and fall back to a sensible default if unset, so change the ComponentVersionCfg construction in the GetComponentsVersion call to use that config value instead of the literal 5 (refer to MaxWorkers, componentHelper.GetComponentsVersion, ComponentVersionCfg, and the receiver fields like d.db/d.ctx/d.s/file.Purls to locate the call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 65-68: Remove the unused G204 exclusion from .golangci.yml or add
a short justification comment explaining why it is present; specifically, delete
the G204 entry unless you intend to run shell commands in the future, or if you
want to keep it add a comment referencing the relevant code (e.g., loadSQLData
in pkg/models/common.go and current exec usage via db.Exec/ExecContext which are
SQL executions, not shell exec) stating the rationale and intended future
patterns that would require G204. Ensure the justification mentions there are no
shell exec calls today and points to db.Exec/ExecContext as examples so
reviewers know the exclusion is deliberate.
In `@CHANGELOG.md`:
- Line 18: Update the version format on the changelog line containing "Upgraded
`package-url/packageurl-go` to v.1.5" by removing the extra dot after the "v" so
it reads "v1.5" to match the project's other version references; edit that exact
string in CHANGELOG.md to "Upgraded `package-url/packageurl-go` to v1.5".
---
Outside diff comments:
In `@pkg/models/versions.go`:
- Around line 80-84: The INSERT call in m.db.QueryRowxContext within
pkg/models/versions.go passes four arguments (name, "", false, false) but the
SQL has only two placeholders ($1, $2), causing a parameter mismatch; update the
SQL to include the additional columns if the booleans and empty semver are
intended (e.g., add the appropriate column names and $3, $4 placeholders) or
remove the extra arguments and only pass name and semver (""), ensuring the call
to QueryRowxContext and the subsequent StructScan(&version) match the final
column list and returned fields.
---
Duplicate comments:
In `@pkg/service/dependency_support_test.go`:
- Around line 37-38: The test currently only prints the result of
convertDependencyOutput(outputDto) without verifying behavior; replace or
supplement the fmt.Printf with assertions that validate the converted output's
expected fields and values (e.g., check non-nil result, expected length,
specific fields match input from outputDto, and any error/status fields). Locate
the call to convertDependencyOutput and use the test framework's assertion
helpers (t.Fatalf/t.Errorf or testify/require as used elsewhere) to assert
correctness of the returned structure and key properties so the test fails when
conversion is incorrect.
In `@pkg/service/dependency_support.go`:
- Around line 91-99: The code currently only sets d.ErrorCode when
dep.Status.Message is non-empty and takes &dep.Status.Message which aliases the
loop variable; change the logic to check failure solely by dep.Status.StatusCode
!= domain.Success, always set d.ErrorCode =
domain.StatusCodeToErrorCode(dep.Status.StatusCode), and if dep.Status.Message
!= "" create a new local string variable (e.g., msg := dep.Status.Message) and
set d.ErrorMessage = &msg to avoid pointer aliasing before appending d to deps;
ensure these changes are applied around the existing dep, d, dep.Status.Message,
ErrorCode, ErrorMessage, domain.StatusCodeToErrorCode and deps append usage.
---
Nitpick comments:
In @.golangci.yml:
- Around line 84-86: The current .golangci.yml exclusions block uses paths: -
tests which only matches a directory named tests; update the exclusions to
precisely exclude Go test files by adding a glob pattern for *_test.go (e.g.,
add a paths entry like **/*_test.go) or enable generated: lax as needed; modify
the exclusions.paths list where 'paths' and 'tests' are defined so that it
includes the test-file glob (and/or set generated: lax) to ensure package-level
*_test.go files are excluded.
In `@pkg/dtos/dependency_input.go`:
- Around line 23-26: The import grouping in dependency_input.go incorrectly
places the external package
"github.com/scanoss/go-component-helper/componenthelper" with standard library
imports; update the import block to follow conventional grouping: keep stdlib
imports together, place third-party imports (e.g.,
"github.com/scanoss/go-component-helper/componenthelper" and "go.uber.org/zap")
in a separate group, and then local/project imports last so functions/types
referencing componenthelper and zap remain unchanged (no code changes to
functions like any using componenthelper).
In `@pkg/usecase/dependency.go`:
- Around line 91-99: Replace the unnecessary use of fmt.Sprintf in the error
branch of GetURLsByPurlString handling: in the block inside the loop where url,
err := d.allUrls.GetURLsByPurlString(...) leads to setting depOutput.Status =
domain.ComponentStatus{ Message: fmt.Sprintf("component not found"), ... },
simplify Message to the plain string "component not found" (update the
assignment in the code that constructs depOutput.Status inside that error branch
in pkg/usecase/dependency.go).
- Around line 69-75: The code hardcodes MaxWorkers: 5 in the call to
componentHelper.GetComponentsVersion which reduces configurability; update the
call to read MaxWorkers from configuration (e.g., a field on the
usecase/dependency receiver such as d.cfg.MaxWorkers or a passed-in options
value) and fall back to a sensible default if unset, so change the
ComponentVersionCfg construction in the GetComponentsVersion call to use that
config value instead of the literal 5 (refer to MaxWorkers,
componentHelper.GetComponentsVersion, ComponentVersionCfg, and the receiver
fields like d.db/d.ctx/d.s/file.Purls to locate the call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f6fc5cc-413a-4f7e-a9bb-854d45d152b5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/go-ci.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.golangci.ymlCHANGELOG.mdMakefilego.modpkg/dtos/dependency_input.gopkg/dtos/dependency_output.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/licenses.gopkg/models/licenses_test.gopkg/models/mines.gopkg/models/mines_test.gopkg/models/projects.gopkg/models/projects_test.gopkg/models/versions.gopkg/models/versions_test.gopkg/service/dependency_service.gopkg/service/dependency_support.gopkg/service/dependency_support_test.gopkg/usecase/dependency.gopkg/usecase/dependency_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/models/mines_test.go
- pkg/models/all_urls.go
- pkg/models/golang_projects_test.go
- pkg/models/projects_test.go
- pkg/models/versions_test.go
- pkg/dtos/dependency_output.go
- pkg/models/golang_projects.go
- Makefile
- .github/workflows/release.yml
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
2 times, most recently
from
68ca2d1 to
ae46224
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/transdep/dependency_graph.go (1)
77-88:⚠️ Potential issue | 🟠 MajorUse the sorted keys here; the current output is still non-deterministic.
Line 77 ranges over
dg.dependenciesOf, so the sort done above is effectively dead code.String()can still emit a different order for the same graph across runs.🛠️ Proposed fix
- for key, value := range dg.dependenciesOf { - children := value + for _, dep := range deps { + key := *dep + children := dg.dependenciesOf[key] if len(children) == 0 { _, _ = fmt.Fprintf(&result, "%s --> null\n", key.Purl) continue } for _, child := range children { _, _ = fmt.Fprintf(&result, "%s --> %s\n", key.Purl, child.Purl) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transdep/dependency_graph.go` around lines 77 - 88, The String() method currently ranges directly over dg.dependenciesOf which is non-deterministic; instead collect the map keys (the package keys used as loop variable), sort them deterministically (e.g., by key.Purl), then iterate over that sorted slice when building result; also consider sorting each children slice (the value used as children) before iterating to ensure fully deterministic output. Locate the loop referencing dg.dependenciesOf and replace it with iterating the sorted keys and sorted children while writing to result.pkg/service/dependency_support.go (1)
221-222:⚠️ Potential issue | 🟡 MinorRemove debug print statements before merging.
fmt.Printfcalls for debugging should be removed or replaced with structured logging via the logger parameter.🔧 Suggested fix
- fmt.Printf("Valid components: %v\n", validComponents) - fmt.Printf("Invalid components: %v\n", invalidPurls) + s.Debugf("Valid components: %v", validComponents) + s.Debugf("Invalid components: %v", invalidPurls)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 221 - 222, Remove the two debug fmt.Printf calls that print validComponents and invalidPurls in pkg/service/dependency_support.go and replace them with structured logging using the function's logger parameter (e.g., logger.Debug/Info/Error) to record the same information; locate the lines referencing validComponents and invalidPurls and change them to logger calls that include context (e.g., "valid components" and "invalid purls") and the respective variable values so debug output goes through the structured logger instead of fmt.Printf.
♻️ Duplicate comments (1)
pkg/service/dependency_support.go (1)
90-98:⚠️ Potential issue | 🟠 MajorTwo bugs remain in error-mapping logic.
Lost error codes: The condition
dep.Status.StatusCode != domain.Success && dep.Status.Message != ""skips the entire block when status is non-success but message is empty, soErrorCodeis never set for those cases.Pointer aliasing risk:
&dep.Status.Messagetakes the address of the loop variable's field. In Go versions prior to 1.22, all iterations share the samedepvariable address, causing all error messages to point to the last value. Copy the string first:🔧 Suggested fix
- if dep.Status.StatusCode != domain.Success && dep.Status.Message != "" { - errorCode := domain.StatusCodeToErrorCode(dep.Status.StatusCode) - d.ErrorCode = errorCode - if dep.Status.Message != "" { - d.ErrorMessage = &dep.Status.Message - } + if dep.Status.StatusCode != domain.Success { + d.ErrorCode = domain.StatusCodeToErrorCode(dep.Status.StatusCode) + if dep.Status.Message != "" { + msg := dep.Status.Message + d.ErrorMessage = &msg + } deps = append(deps, d) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 90 - 98, The error-mapping block currently skips setting ErrorCode when dep.Status.StatusCode != domain.Success but dep.Status.Message == "", and it takes the address of dep.Status.Message which can alias across loop iterations; update the condition to only check dep.Status.StatusCode != domain.Success so ErrorCode is always set (use domain.StatusCodeToErrorCode(dep.Status.StatusCode) and assign to d.ErrorCode), and when assigning ErrorMessage copy the string into a new local (e.g. msg := dep.Status.Message) and set d.ErrorMessage = &msg only if msg != "" to avoid pointer aliasing and keep nil for empty messages before appending d to deps.
🧹 Nitpick comments (5)
pkg/models/common_test.go (1)
28-42: Mark both setup functions as test helpers.Adding
t.Helper()at the top ofsqliteSetupandsqliteConnwill report setup failures at the calling test instead of insidecommon_test.go.Small cleanup
func sqliteSetup(t *testing.T) *sqlx.DB { + t.Helper() db, err := sqlx.Connect("sqlite", "file::memory:?cache=shared") @@ func sqliteConn(t *testing.T, ctx context.Context, db *sqlx.DB) *sqlx.Conn { + t.Helper() conn, err := db.Connx(ctx) // Get a connection from the pool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/common_test.go` around lines 28 - 42, The test helper functions sqliteSetup and sqliteConn should call t.Helper() so failures are reported at the caller; add t.Helper() as the first line inside both sqliteSetup(t *testing.T) and sqliteConn(t *testing.T, ctx context.Context, db *sqlx.DB) to mark them as test helpers and improve error reporting.pkg/service/dependency_service.go (1)
59-60: Defer request-time telemetry so failures are measured too.The early returns on Line 67, Line 72, and Line 83 bypass Line 88, so failed dependency requests disappear from the latency histogram.
GetTransitiveDependenciesalready uses the saferdeferpattern.📈 Proposed change
func (d dependencyServer) GetDependencies(ctx context.Context, request *pb.DependencyRequest) (*pb.DependencyResponse, error) { requestStartTime := time.Now() // Capture the scan start time + defer telemetryRequestTime(ctx, d.config, requestStartTime) s := ctxzap.Extract(ctx).Sugar() s.Info("Processing dependency request...") @@ - depResponse := convertDependencyOutput(dtoDependencies) // Convert the internal data into a response object - telemetryRequestTime(ctx, d.config, requestStartTime) // Record the request processing time + depResponse := convertDependencyOutput(dtoDependencies) // Convert the internal data into a response objectAlso applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_service.go` around lines 59 - 60, The latency observation for dependency requests is currently done after early returns so failures aren't recorded; instead, capture the start time (requestStartTime := time.Now()) as before and add a deferred function right after creating the logger (s := ctxzap.Extract(ctx).Sugar()) that computes elapsed since requestStartTime and records the latency/telemetry (same pattern used in GetTransitiveDependencies), ensuring the defer runs on all return paths; update or remove the existing non-deferred observe call so latency is only recorded by the new defer..github/workflows/golangci-lint.yml (1)
27-28: Pin the actual golangci-lint binary version to match local configuration.The
Makefilepins local Docker lint tov2.10.1, but this workflow only pins the action wrapper. Without an explicitversioninput, the action defaults tolatest, creating a drift risk between local and CI lint results. Sincego.moddoes not specify a golangci-lint dependency, the action cannot auto-detect the version.🧹 Proposed change
- name: golangci-lint uses: golangci/golangci-lint-action@v9 + with: + version: v2.10.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/golangci-lint.yml around lines 27 - 28, The workflow currently uses the golangci-lint action wrapper (uses: golangci/golangci-lint-action@v9) but doesn't pin the actual golangci-lint binary version, causing drift with the Makefile (which uses v2.10.1); update the golangci-lint step (name: golangci-lint / uses: golangci/golangci-lint-action@v9) to include a with: block that sets version: "v2.10.1" so the CI binary matches local configuration.pkg/usecase/dependency.go (2)
68-74: Consider makingMaxWorkersconfigurable.The worker pool size is hardcoded to 5. For production flexibility, consider exposing this via
ServerConfigto allow tuning based on deployment environment and database connection pool limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency.go` around lines 68 - 74, The call to componentHelper.GetComponentsVersion hardcodes ComponentVersionCfg.MaxWorkers = 5; make MaxWorkers configurable by adding a MaxWorkers (or WorkerPoolSize) field to your ServerConfig struct and use that value when constructing ComponentVersionCfg (e.g., replace the literal 5 with d.s.ServerConfig.MaxWorkers or similar), ensure sensible default/fallback if config is zero and document/update any config parsing where ServerConfig is populated; touch the ComponentVersionCfg type only if necessary to align naming.
108-113: Simplify version assignment.The conditional can be replaced with a direct assignment since an empty string is an acceptable default.
♻️ Suggested simplification
- // Avoids empty version - if len(url.Version) > 0 { - depOutput.Version = url.Version - } else { - depOutput.Version = "" - } + // Prefer URL version over component version when available + if url.Version != "" { + depOutput.Version = url.Version + }Note: This preserves
processedComponent.Version(set at line 80) whenurl.Versionis empty, which may be more useful than overwriting with an empty string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency.go` around lines 108 - 113, Replace the conditional block that sets depOutput.Version so it only assigns when url.Version is non-empty (i.e., if url.Version != "" { depOutput.Version = url.Version }), leaving depOutput.Version as previously set (e.g., from processedComponent.Version) when url.Version is empty; this avoids overwriting a useful default with an empty string and keeps the logic centered on depOutput, url.Version, and processedComponent.Version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/models/common_test.go`:
- Around line 27-33: sqliteSetup is using a global in-memory URI which leaks
state across tests; update sqliteSetup to call t.Helper() and construct a
per-test unique SQLite URI using the test name (e.g. "file:" +
url.PathEscape(t.Name()) + "?mode=memory&cache=shared") when calling
sqlx.Connect so each test gets its own named in-memory DB; also add t.Helper()
to sqliteConn so failures point to the test, not the helper, and keep the
existing cache=shared/mode=memory options to preserve connection pooling
semantics.
---
Outside diff comments:
In `@pkg/service/dependency_support.go`:
- Around line 221-222: Remove the two debug fmt.Printf calls that print
validComponents and invalidPurls in pkg/service/dependency_support.go and
replace them with structured logging using the function's logger parameter
(e.g., logger.Debug/Info/Error) to record the same information; locate the lines
referencing validComponents and invalidPurls and change them to logger calls
that include context (e.g., "valid components" and "invalid purls") and the
respective variable values so debug output goes through the structured logger
instead of fmt.Printf.
In `@pkg/transdep/dependency_graph.go`:
- Around line 77-88: The String() method currently ranges directly over
dg.dependenciesOf which is non-deterministic; instead collect the map keys (the
package keys used as loop variable), sort them deterministically (e.g., by
key.Purl), then iterate over that sorted slice when building result; also
consider sorting each children slice (the value used as children) before
iterating to ensure fully deterministic output. Locate the loop referencing
dg.dependenciesOf and replace it with iterating the sorted keys and sorted
children while writing to result.
---
Duplicate comments:
In `@pkg/service/dependency_support.go`:
- Around line 90-98: The error-mapping block currently skips setting ErrorCode
when dep.Status.StatusCode != domain.Success but dep.Status.Message == "", and
it takes the address of dep.Status.Message which can alias across loop
iterations; update the condition to only check dep.Status.StatusCode !=
domain.Success so ErrorCode is always set (use
domain.StatusCodeToErrorCode(dep.Status.StatusCode) and assign to d.ErrorCode),
and when assigning ErrorMessage copy the string into a new local (e.g. msg :=
dep.Status.Message) and set d.ErrorMessage = &msg only if msg != "" to avoid
pointer aliasing and keep nil for empty messages before appending d to deps.
---
Nitpick comments:
In @.github/workflows/golangci-lint.yml:
- Around line 27-28: The workflow currently uses the golangci-lint action
wrapper (uses: golangci/golangci-lint-action@v9) but doesn't pin the actual
golangci-lint binary version, causing drift with the Makefile (which uses
v2.10.1); update the golangci-lint step (name: golangci-lint / uses:
golangci/golangci-lint-action@v9) to include a with: block that sets version:
"v2.10.1" so the CI binary matches local configuration.
In `@pkg/models/common_test.go`:
- Around line 28-42: The test helper functions sqliteSetup and sqliteConn should
call t.Helper() so failures are reported at the caller; add t.Helper() as the
first line inside both sqliteSetup(t *testing.T) and sqliteConn(t *testing.T,
ctx context.Context, db *sqlx.DB) to mark them as test helpers and improve error
reporting.
In `@pkg/service/dependency_service.go`:
- Around line 59-60: The latency observation for dependency requests is
currently done after early returns so failures aren't recorded; instead, capture
the start time (requestStartTime := time.Now()) as before and add a deferred
function right after creating the logger (s := ctxzap.Extract(ctx).Sugar()) that
computes elapsed since requestStartTime and records the latency/telemetry (same
pattern used in GetTransitiveDependencies), ensuring the defer runs on all
return paths; update or remove the existing non-deferred observe call so latency
is only recorded by the new defer.
In `@pkg/usecase/dependency.go`:
- Around line 68-74: The call to componentHelper.GetComponentsVersion hardcodes
ComponentVersionCfg.MaxWorkers = 5; make MaxWorkers configurable by adding a
MaxWorkers (or WorkerPoolSize) field to your ServerConfig struct and use that
value when constructing ComponentVersionCfg (e.g., replace the literal 5 with
d.s.ServerConfig.MaxWorkers or similar), ensure sensible default/fallback if
config is zero and document/update any config parsing where ServerConfig is
populated; touch the ComponentVersionCfg type only if necessary to align naming.
- Around line 108-113: Replace the conditional block that sets depOutput.Version
so it only assigns when url.Version is non-empty (i.e., if url.Version != "" {
depOutput.Version = url.Version }), leaving depOutput.Version as previously set
(e.g., from processedComponent.Version) when url.Version is empty; this avoids
overwriting a useful default with an empty string and keeps the logic centered
on depOutput, url.Version, and processedComponent.Version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ba5ca02-40fa-4d1d-a6a1-a985836db9c1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/go-ci.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.golangci.ymlCHANGELOG.mdMakefilego.modpkg/dtos/dependency_input.gopkg/dtos/dependency_output.gopkg/errors/error.gopkg/models/all_urls.gopkg/models/common.gopkg/models/common_test.gopkg/models/golang_projects.gopkg/protocol/rest/server.gopkg/service/dependency_service.gopkg/service/dependency_support.gopkg/shared/index.gopkg/transdep/dependency_collector.gopkg/transdep/dependency_graph.gopkg/transdep/helper.gopkg/usecase/dependency.go
💤 Files with no reviewable changes (1)
- pkg/models/common.go
✅ Files skipped from review due to trivial changes (3)
- pkg/transdep/dependency_collector.go
- pkg/shared/index.go
- pkg/errors/error.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/dtos/dependency_output.go
- CHANGELOG.md
- pkg/models/all_urls.go
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
2 times, most recently
from
83c9436 to
18d42c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/models/versions.go (1)
80-84:⚠️ Potential issue | 🟡 MinorExcess arguments in INSERT statement.
The SQL statement has 2 placeholders (
$1, $2) but 4 arguments are passed (name, "", false, false). The extra arguments (false, false) are silently ignored by the database driver, which may mask an intent error or indicate leftover code from a previous schema version.Based on the
Versionstruct (which hasid,version_name,semver), onlynameand the empty string forsemvershould be passed.🔧 Proposed fix
err := m.db.QueryRowxContext(m.ctx, "INSERT INTO versions (version_name, semver) VALUES($1, $2)"+ " RETURNING id, version_name, semver", - name, "", false, false, + name, "", ).StructScan(&version)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/versions.go` around lines 80 - 84, The INSERT call in the method using m.db.QueryRowxContext in pkg/models/versions.go passes four arguments (name, "", false, false) while the SQL has only two placeholders ($1, $2); remove the extraneous false, false arguments so the call supplies exactly name and the empty semver string to match the SQL, keeping the RETURNING and StructScan(&version) unchanged.
♻️ Duplicate comments (4)
CHANGELOG.md (1)
18-18:⚠️ Potential issue | 🟡 MinorFix version format for consistency.
The version format
v.1.5includes an extra dot after the "v" prefix. For consistency with other version references in this changelog (e.g.,v1.79.2,v0.32.0), it should bev1.5.📝 Proposed fix
-- Upgraded `package-url/packageurl-go` to v.1.5 +- Upgraded `package-url/packageurl-go` to v1.5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 18, Update the version string for package-url/packageurl-go: replace the incorrect "v.1.5" with the consistent format "v1.5" in the CHANGELOG entry that currently reads `Upgraded package-url/packageurl-go to v.1.5`.pkg/service/dependency_support_test.go (1)
37-38:⚠️ Potential issue | 🟡 MinorAssert the converted value instead of printing it.
This still passes even if
convertDependencyOutputregresses, because nothing is asserted. At minimum, check that the result is non-nil and that an emptydtos.DependencyOutputproduces no files.🧪 Minimal assertion update
output := convertDependencyOutput(outputDto) - fmt.Printf("Output: %v\n", output) + if output == nil { + t.Fatal("expected non-nil response") + } + if len(output.Files) != 0 { + t.Fatalf("expected no files, got %d", len(output.Files)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support_test.go` around lines 37 - 38, Replace the debug print with real assertions: inside the test that calls convertDependencyOutput(outputDto) assert that the returned value (output) is non-nil, and add a case where you pass an empty dtos.DependencyOutput and assert that convertDependencyOutput returns an output with zero files (or empty slice/length 0). Use the test's *testing.T assertion helpers (t.Fatalf/t.Errorf or testify/require if available) to fail the test on regression; reference the existing variables outputDto and output and the convertDependencyOutput function to locate where to add these assertions.pkg/service/dependency_support.go (1)
90-95:⚠️ Potential issue | 🟠 MajorDon't drop
error_codeor reuse the loop variable's message pointer.Failed components with an empty message currently fall through as successes, and
&dep.Status.Messagealiases the reused range variable, so later iterations can overwrite earliererror_messagevalues.🔧 Suggested fix
- if dep.Status.StatusCode != domain.Success && dep.Status.Message != "" { - errorCode := domain.StatusCodeToErrorCode(dep.Status.StatusCode) - d.ErrorCode = errorCode - if dep.Status.Message != "" { - d.ErrorMessage = &dep.Status.Message - } + if dep.Status.StatusCode != domain.Success { + d.ErrorCode = domain.StatusCodeToErrorCode(dep.Status.StatusCode) + if dep.Status.Message != "" { + msg := dep.Status.Message + d.ErrorMessage = &msg + } deps = append(deps, d) continue }In Go, is it safe to take the address of a field on the range variable (for example `&dep.Status.Message`) inside a `for _, dep := range ...` loop, or is the range variable reused across iterations?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 90 - 95, The loop currently skips failed dependencies with empty dep.Status.Message and assigns &dep.Status.Message which aliases the reused range variable; change the logic so any non-success StatusCode sets d.ErrorCode using domain.StatusCodeToErrorCode(dep.Status.StatusCode) regardless of Message content, and when assigning d.ErrorMessage create a fresh string copy (e.g., tmp := dep.Status.Message; d.ErrorMessage = &tmp) instead of taking the address of the range variable; update the code paths in the loop that reference dep, d.ErrorCode, and d.ErrorMessage accordingly.pkg/usecase/dependency_test.go (1)
131-143:⚠️ Potential issue | 🟡 MinorAlso assert the known valid purl succeeds.
This only proves there is at least one error. A regression where every component is marked failed would still pass, which weakens the new mixed-batch contract for
pkg:npm/isbinaryfile.🧪 Suggested assertion
- errorCount := 0 + errorCount := 0 + successCount := 0 for _, file := range dependencies.Files { for _, dep := range file.Dependencies { + if dep.Purl == "pkg:npm/isbinaryfile" && dep.Status.StatusCode == domain.Success { + successCount++ + } if dep.Status.StatusCode != "" && dep.Status.StatusCode != domain.Success { errorCount++ } } } + if successCount == 0 { + t.Errorf("expected pkg:npm/isbinaryfile to succeed in mixed batch") + } if errorCount == 0 { t.Errorf("expected at least one component-level error in mixed batch, got none") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency_test.go` around lines 131 - 143, Add an assertion that the known-valid package URL "pkg:npm/isbinaryfile" is marked successful in the mixed-batch test: after computing errorCount over dependencies.Files -> file.Dependencies (checking dep.Status.StatusCode against domain.Success), locate the dependency whose PURL equals "pkg:npm/isbinaryfile" (use the existing dependency struct field for the package URL) and assert its dep.Status.StatusCode == domain.Success (fail the test if not), so the test ensures at least that that specific valid component succeeded as well as there being some failures.
🧹 Nitpick comments (3)
pkg/transdep/dependency_graph.go (1)
68-87: Use the sorted keys when rendering the graph.Line 77 still ranges over
dg.dependenciesOfdirectly, soString()remains nondeterministic even thoughdepsis sorted above. That makes output unstable for tests and diffs.Proposed fix
- deps := make([]*Dependency, 0, len(dg.dependenciesOf)) + deps := make([]Dependency, 0, len(dg.dependenciesOf)) for dep := range dg.dependenciesOf { - deps = append(deps, &dep) + deps = append(deps, dep) } sort.Slice(deps, func(i, j int) bool { return deps[i].Purl < deps[j].Purl }) - for key, value := range dg.dependenciesOf { - children := value + for _, key := range deps { + children := dg.dependenciesOf[key] if len(children) == 0 { _, _ = fmt.Fprintf(&result, "%s --> null\n", key.Purl) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/transdep/dependency_graph.go` around lines 68 - 87, The graph rendering currently ranges over dg.dependenciesOf (making output nondeterministic); change the second loop to iterate over the sorted deps slice instead. Build deps as a stable list (avoid taking &dep of the range variable — append a copy or use a newDep := dep then append(&newDep) or simply append dep values) and then replace "for key, value := range dg.dependenciesOf" with "for _, key := range deps" and fetch children via dg.dependenciesOf[key] (or dg.dependenciesOf[*key] if you keep pointers); ensure you use the same Purl/key used when sorting so String()/rendering is deterministic..golangci.yml (1)
87-89: Consider using a more specific exclusion pattern for test files.The current exclusion path
testsonly matches a directory literally named "tests". If test files follow the Go convention of*_test.gofiles alongside source files (as seen inpkg/models/golang_projects_test.go), this exclusion won't apply to them.Since
run.tests: falsealready excludes test files from linting, thisexclusions.pathsentry may be redundant or intended for a different purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 87 - 89, The exclusion entry under exclusions.paths currently lists a literal "tests" directory which won't match Go test files like pkg/models/golang_projects_test.go; either remove that redundant entry because run.tests: false already skips *_test.go files, or replace "tests" with a more specific pattern that matches Go test filenames (e.g., a glob/regex targeting *_test.go) so exclusions.paths actually excludes test files—look for the exclusions.paths key in .golangci.yml and update or delete it accordingly.pkg/service/dependency_service.go (1)
87-88: Record latency for failed requests too.
telemetryRequestTimeonly runs on the success/warning path here, so bad requests and hard failures disappear from the latency metric.GetTransitiveDependenciesalready solves this with adefer; doing the same here keeps observability consistent.📈 Suggested change
func (d dependencyServer) GetDependencies(ctx context.Context, request *pb.DependencyRequest) (*pb.DependencyResponse, error) { requestStartTime := time.Now() // Capture the scan start time + defer telemetryRequestTime(ctx, d.config, requestStartTime) s := ctxzap.Extract(ctx).Sugar() s.Info("Processing dependency request...") @@ } depResponse := convertDependencyOutput(dtoDependencies) // Convert the internal data into a response object - telemetryRequestTime(ctx, d.config, requestStartTime) // Record the request processing time // Set the status and respond with the data return &pb.DependencyResponse{Files: depResponse.Files, Status: &statusResp}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_service.go` around lines 87 - 88, Move the telemetryRequestTime call into a deferred call at the start of the function so latency is recorded for all outcomes (success, warning, and failures); specifically, add a defer telemetryRequestTime(ctx, d.config, requestStartTime) near the beginning of the function that contains convertDependencyOutput and depResponse (mirroring how GetTransitiveDependencies uses defer) so the request processing time is always captured even if an error is returned before depResponse is created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/models/all_urls_test.go`:
- Around line 53-54: Update the test constructors that build the DB select
context so they exercise the migrated nil-conn path: replace the active
connection argument with nil when calling database.NewDBSelectContext within the
NewAllURLModel (and other model) test setups (i.e., change
database.NewDBSelectContext(s, db, conn, myConfig.Database.Trace) to
database.NewDBSelectContext(s, db, nil, myConfig.Database.Trace)) in all
occurrences in this test file so the code paths used in production (which pass
nil) are exercised; look for NewAllURLModel and the other model constructor
calls that wrap database.NewDBSelectContext and adjust their third parameter to
nil.
In `@pkg/usecase/dependency_test.go`:
- Line 89: The test is hitting a different in-memory SQLite instance because
NewDependencies uses *sqlx.DB while fixtures were loaded via Connx; switch the
test to use a shared-memory DSN and a single connection so both sides see the
same DB. Update the test setup that calls LoadTestSQLData and NewDependencies to
open the DB with a shared-memory DSN (e.g. "file::memory:?cache=shared" or
"file:memdb?mode=memory&cache=shared") and call db.SetMaxOpenConns(1) (and
SetConnMaxIdleTime/SetConnMaxLifetime as needed) so the *sqlx.DB and Connx use
the same underlying in-memory database; ensure Connx and the *sqlx.DB use that
same DSN when creating connections so NewDependencies reads the fixture data.
In `@pkg/usecase/dependency.go`:
- Around line 90-97: The code currently treats any error from
GetURLsByPurlString(processedComponent.Purl, processedComponent.Version) as
domain.ComponentNotFound; instead, if err != nil this is an operational/db error
and should be handled separately (log with d.s.Warnf including err and either
return/propagate the error to caller or set depOutput.Status to an
internal/lookup-failure status rather than ComponentNotFound), and only when the
call succeeds but returns no URLs (nil/empty url result) should you set
depOutput.Status to domain.ComponentNotFound; update the logic around
GetURLsByPurlString, d.s.Warnf, depOutput and depOutputs to reflect this
distinction.
- Around line 68-74: GetComponentsVersion is being called with parallelism
(ComponentVersionCfg{MaxWorkers: 5, Input: file.Purls}), which yields results in
completion order and breaks positional stability; fix by either setting
MaxWorkers to 1 so processing is sequential (preserve input order) or modify
GetComponentsVersion to return a stable ordering: have workers emit results with
the original input index (or use a map keyed by component identity) and then
reconstruct the returned slice in the same order as file.Purls before returning;
update the call site around GetComponentsVersion/ComponentVersionCfg (and fields
Input, MaxWorkers) accordingly so callers receive results in input order.
---
Outside diff comments:
In `@pkg/models/versions.go`:
- Around line 80-84: The INSERT call in the method using m.db.QueryRowxContext
in pkg/models/versions.go passes four arguments (name, "", false, false) while
the SQL has only two placeholders ($1, $2); remove the extraneous false, false
arguments so the call supplies exactly name and the empty semver string to match
the SQL, keeping the RETURNING and StructScan(&version) unchanged.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 18: Update the version string for package-url/packageurl-go: replace the
incorrect "v.1.5" with the consistent format "v1.5" in the CHANGELOG entry that
currently reads `Upgraded package-url/packageurl-go to v.1.5`.
In `@pkg/service/dependency_support_test.go`:
- Around line 37-38: Replace the debug print with real assertions: inside the
test that calls convertDependencyOutput(outputDto) assert that the returned
value (output) is non-nil, and add a case where you pass an empty
dtos.DependencyOutput and assert that convertDependencyOutput returns an output
with zero files (or empty slice/length 0). Use the test's *testing.T assertion
helpers (t.Fatalf/t.Errorf or testify/require if available) to fail the test on
regression; reference the existing variables outputDto and output and the
convertDependencyOutput function to locate where to add these assertions.
In `@pkg/service/dependency_support.go`:
- Around line 90-95: The loop currently skips failed dependencies with empty
dep.Status.Message and assigns &dep.Status.Message which aliases the reused
range variable; change the logic so any non-success StatusCode sets d.ErrorCode
using domain.StatusCodeToErrorCode(dep.Status.StatusCode) regardless of Message
content, and when assigning d.ErrorMessage create a fresh string copy (e.g., tmp
:= dep.Status.Message; d.ErrorMessage = &tmp) instead of taking the address of
the range variable; update the code paths in the loop that reference dep,
d.ErrorCode, and d.ErrorMessage accordingly.
In `@pkg/usecase/dependency_test.go`:
- Around line 131-143: Add an assertion that the known-valid package URL
"pkg:npm/isbinaryfile" is marked successful in the mixed-batch test: after
computing errorCount over dependencies.Files -> file.Dependencies (checking
dep.Status.StatusCode against domain.Success), locate the dependency whose PURL
equals "pkg:npm/isbinaryfile" (use the existing dependency struct field for the
package URL) and assert its dep.Status.StatusCode == domain.Success (fail the
test if not), so the test ensures at least that that specific valid component
succeeded as well as there being some failures.
---
Nitpick comments:
In @.golangci.yml:
- Around line 87-89: The exclusion entry under exclusions.paths currently lists
a literal "tests" directory which won't match Go test files like
pkg/models/golang_projects_test.go; either remove that redundant entry because
run.tests: false already skips *_test.go files, or replace "tests" with a more
specific pattern that matches Go test filenames (e.g., a glob/regex targeting
*_test.go) so exclusions.paths actually excludes test files—look for the
exclusions.paths key in .golangci.yml and update or delete it accordingly.
In `@pkg/service/dependency_service.go`:
- Around line 87-88: Move the telemetryRequestTime call into a deferred call at
the start of the function so latency is recorded for all outcomes (success,
warning, and failures); specifically, add a defer telemetryRequestTime(ctx,
d.config, requestStartTime) near the beginning of the function that contains
convertDependencyOutput and depResponse (mirroring how GetTransitiveDependencies
uses defer) so the request processing time is always captured even if an error
is returned before depResponse is created.
In `@pkg/transdep/dependency_graph.go`:
- Around line 68-87: The graph rendering currently ranges over dg.dependenciesOf
(making output nondeterministic); change the second loop to iterate over the
sorted deps slice instead. Build deps as a stable list (avoid taking &dep of the
range variable — append a copy or use a newDep := dep then append(&newDep) or
simply append dep values) and then replace "for key, value := range
dg.dependenciesOf" with "for _, key := range deps" and fetch children via
dg.dependenciesOf[key] (or dg.dependenciesOf[*key] if you keep pointers); ensure
you use the same Purl/key used when sorting so String()/rendering is
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7baac4b0-cc33-4490-9c20-69d60be1ed16
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
.github/workflows/go-ci.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.golangci.ymlCHANGELOG.mdMakefilego.modpkg/dtos/dependency_input.gopkg/dtos/dependency_output.gopkg/errors/error.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/common.gopkg/models/common_test.gopkg/models/dependencies.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/licenses.gopkg/models/licenses_test.gopkg/models/mines.gopkg/models/mines_test.gopkg/models/projects.gopkg/models/projects_test.gopkg/models/versions.gopkg/models/versions_test.gopkg/protocol/rest/server.gopkg/service/dependency_service.gopkg/service/dependency_support.gopkg/service/dependency_support_test.gopkg/shared/index.gopkg/transdep/dependency_collector.gopkg/transdep/dependency_graph.gopkg/transdep/helper.gopkg/usecase/dependency.gopkg/usecase/dependency_test.go
💤 Files with no reviewable changes (1)
- pkg/models/common.go
✅ Files skipped from review due to trivial changes (1)
- pkg/errors/error.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/shared/index.go
- pkg/models/licenses_test.go
- pkg/models/versions_test.go
- pkg/transdep/helper.go
- pkg/dtos/dependency_output.go
- pkg/models/common_test.go
- pkg/transdep/dependency_collector.go
- Makefile
- .github/workflows/go-ci.yml
- pkg/models/projects.go
- pkg/models/mines_test.go
- pkg/protocol/rest/server.go
- .github/workflows/golangci-lint.yml
- pkg/models/projects_test.go
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
2 times, most recently
from
d6986ef to
7097286
Compare
eeisegn
previously approved these changes
Mar 10, 2026
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
3 times, most recently
from
5a157f4 to
ffe037b
Compare
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
4 times, most recently
from
df0d2e2 to
9595bb0
Compare
isasmendiagus
previously requested changes
Mar 19, 2026
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
2 times, most recently
from
18738fd to
0f0e094
Compare
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
2 times, most recently
from
b5cb6ea to
228b35b
Compare
…y for dependency response
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
from
228b35b to
45d52fe
Compare
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
6 times, most recently
from
09ab2e3 to
b0b1b7e
Compare
agustingroh
force-pushed
the
chore/SP-4138-add-status-batch-response
branch
from
b0b1b7e to
5fb99d1
Compare
Contributor
|
@coderabbitai resume |
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
eeisegn
approved these changes
Mar 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
pkg/dtos/dependency_output.go (1)
47-47:⚠️ Potential issue | 🟠 MajorAdd the
json:"status"tag to maintain API consistency.The
Statusfield lacks a JSON tag and will serialize as"Status"(capital S), while all sibling fields use lowercase tags ("component","purl", etc.). This inconsistency will break the API response shape.Suggested fix
- Status domain.ComponentStatus + Status domain.ComponentStatus `json:"status"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dtos/dependency_output.go` at line 47, The Status field in the DTO struct is missing a JSON tag causing inconsistent serialization; update the struct definition to add the JSON tag json:"status" to the Status field (e.g., the Status field in the DependencyOutput/DTO struct) so it serializes as "status" like its sibling fields.pkg/service/dependency_support_test.go (1)
37-38:⚠️ Potential issue | 🟡 MinorRe-add assertions to validate the conversion result.
This test only prints the converted value without any assertions. A broken
convertDependencyOutputimplementation would still pass. Assert the expected shape: non-nil result, emptyFilesslice for empty input.Suggested fix
output := convertDependencyOutput(outputDto) - fmt.Printf("Output: %v\n", output) + if output == nil { + t.Fatal("expected non-nil output") + } + if len(output.Files) != 0 { + t.Errorf("expected empty Files slice, got %d", len(output.Files)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support_test.go` around lines 37 - 38, The test currently only prints the result of convertDependencyOutput; update the test to assert the conversion result instead: after calling output := convertDependencyOutput(outputDto) add assertions that output is not nil (e.g., non-nil pointer or non-empty struct as appropriate) and that output.Files is an empty slice (len(output.Files) == 0 or equivalent) to validate the expected shape for empty input; reference the convertDependencyOutput function and the output variable in your assertions.pkg/service/dependency_support.go (1)
90-94:⚠️ Potential issue | 🔴 CriticalPointer to loop variable field causes shared pointer bug.
Line 93 takes
&dep.Status.Messagewheredepis the range loop variable. Since range variables are reused across iterations, alld.ErrorMessagepointers will reference the same memory location, causing all error messages to show the last iteration's value.🐛 Proposed fix
if dep.Status.StatusCode != domain.Success { errorCode := domain.StatusCodeToErrorCode(dep.Status.StatusCode) d.ErrorCode = errorCode - d.ErrorMessage = &dep.Status.Message + msg := dep.Status.Message + d.ErrorMessage = &msg }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/dependency_support.go` around lines 90 - 94, In the range loop where you inspect each dep (the range variable named dep) you currently assign d.ErrorMessage = &dep.Status.Message which takes the address of the reused loop variable causing all ErrorMessage pointers to alias the same memory; fix by creating a new local copy of the message inside the loop (e.g. msg := dep.Status.Message) and assign d.ErrorMessage = &msg (or assign the string value instead of a pointer) so each d.ErrorMessage points to its own distinct memory; update the code paths in the function containing this loop accordingly.pkg/usecase/dependency.go (1)
96-103:⚠️ Potential issue | 🟠 MajorDon't translate lookup errors into “component not found”.
A real miss does not hit this branch:
pkg/models/all_urls.goalready returns a fallbackAllURLwithnilerror for that case. An error here means invalid input or a lookup/query failure, so converting it toComponentWithoutInfomasks operational problems and makes the per-component status misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/dependency.go` around lines 96 - 103, The error from d.allUrls.GetURLsByPurlString is an operational/lookup failure and must not be translated to domain.ComponentWithoutInfo; update the error branch in the GetURLsByPurlString handling so you log the full error (use d.s.Errorf) and set depOutput.Status.Message = err.Error() and depOutput.Status.StatusCode = domain.ComponentLookupError (create this constant if it doesn't exist, or use an existing internal/error status code rather than ComponentWithoutInfo), then append depOutput and continue—do not treat this path as a legitimate "component not found" success case.
🧹 Nitpick comments (1)
pkg/models/dependencies_test.go (1)
27-31: Consider simplifying the deferred closure.The closure doesn't need to take
dbas a parameter since it can capture it directly from the enclosing scope. This would also make the explicit*sqlx.DBtype annotation unnecessary.Additionally, this cleanup pattern differs from other test files (e.g.,
licenses_test.gousesdefer CloseDB(db)). Consider using a consistent pattern across test files for maintainability.♻️ Optional: Simplified closure
- defer func(db *sqlx.DB) { - if errDB := db.Close(); errDB != nil { - t.Fatalf("failed to close db: %v", errDB) - } - }(db) + defer func() { + if errDB := db.Close(); errDB != nil { + t.Fatalf("failed to close db: %v", errDB) + } + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/dependencies_test.go` around lines 27 - 31, The deferred closure that closes the database (currently using defer func(db *sqlx.DB) { ... }(db)) is overly verbose; replace it with a simpler deferred call that captures db from the outer scope (e.g., defer func() { if err := db.Close(); err != nil { t.Fatalf(...) } }()) or, better, use the existing test helper CloseDB(db) used in licenses_test.go to keep patterns consistent—update the test to call CloseDB(db) (or the simplified anonymous defer) and remove the explicit *sqlx.DB parameter and type annotation from the closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 13: Update the module version to match the CHANGELOG: change the go.mod
dependency for github.com/scanoss/go-component-helper from v0.4.0 to v0.5.0 (or
alternatively, if v0.4.0 is correct, update the CHANGELOG entry to reflect
v0.4.0); locate the dependency line referencing
github.com/scanoss/go-component-helper in go.mod and set it to v0.5.0 so the
declared version and CHANGELOG.md are consistent.
- Line 21: Update the gRPC module from google.golang.org/grpc v1.79.2 to v1.79.3
or later to address the authorization bypass vulnerability; edit the dependency
version for google.golang.org/grpc in the go.mod entry (replace v1.79.2 with
v1.79.3 or a newer safe release), then run the module update commands (e.g., go
get google.golang.org/grpc@v1.79.3 and go mod tidy) and run your test
suite/build to ensure compatibility.
In `@pkg/models/all_urls.go`:
- Around line 90-100: Get rid of using PurlName == "" as the golang miss
sentinel: change GetURLsByPurlNameType to return an explicit "found" signal
(e.g., (AllURL, bool, error) or set a Found bool on AllURL) and update the call
sites in this function to check that explicit flag instead of PurlName when
deciding to call getURLsByGolangPurl; update any other callers (including the
golang_projects/GitHub-conversion paths) to honor the new found signal so
fallback results that populate PurlName no longer block the golang fallback.
In `@pkg/usecase/dependency.go`:
- Around line 125-135: The code treats processedComponent.Requirement as an
exact pin by comparing trimmed strings (using
purlutils.GetVersionFromReqOperator) which misflags ranged requirements; replace
the string equality check with a proper range evaluation (e.g., use a
semver/range check utility) to test whether url.Version satisfies
processedComponent.Requirement, and only set depOutput.Status to VersionNotFound
when that range check fails; do not overwrite depOutput.Version with the
requested requirement—leave depOutput.Version as url.Version and only modify
depOutput.Status (domain.ComponentStatus) and Message when the range check
indicates a real mismatch.
---
Duplicate comments:
In `@pkg/dtos/dependency_output.go`:
- Line 47: The Status field in the DTO struct is missing a JSON tag causing
inconsistent serialization; update the struct definition to add the JSON tag
json:"status" to the Status field (e.g., the Status field in the
DependencyOutput/DTO struct) so it serializes as "status" like its sibling
fields.
In `@pkg/service/dependency_support_test.go`:
- Around line 37-38: The test currently only prints the result of
convertDependencyOutput; update the test to assert the conversion result
instead: after calling output := convertDependencyOutput(outputDto) add
assertions that output is not nil (e.g., non-nil pointer or non-empty struct as
appropriate) and that output.Files is an empty slice (len(output.Files) == 0 or
equivalent) to validate the expected shape for empty input; reference the
convertDependencyOutput function and the output variable in your assertions.
In `@pkg/service/dependency_support.go`:
- Around line 90-94: In the range loop where you inspect each dep (the range
variable named dep) you currently assign d.ErrorMessage = &dep.Status.Message
which takes the address of the reused loop variable causing all ErrorMessage
pointers to alias the same memory; fix by creating a new local copy of the
message inside the loop (e.g. msg := dep.Status.Message) and assign
d.ErrorMessage = &msg (or assign the string value instead of a pointer) so each
d.ErrorMessage points to its own distinct memory; update the code paths in the
function containing this loop accordingly.
In `@pkg/usecase/dependency.go`:
- Around line 96-103: The error from d.allUrls.GetURLsByPurlString is an
operational/lookup failure and must not be translated to
domain.ComponentWithoutInfo; update the error branch in the GetURLsByPurlString
handling so you log the full error (use d.s.Errorf) and set
depOutput.Status.Message = err.Error() and depOutput.Status.StatusCode =
domain.ComponentLookupError (create this constant if it doesn't exist, or use an
existing internal/error status code rather than ComponentWithoutInfo), then
append depOutput and continue—do not treat this path as a legitimate "component
not found" success case.
---
Nitpick comments:
In `@pkg/models/dependencies_test.go`:
- Around line 27-31: The deferred closure that closes the database (currently
using defer func(db *sqlx.DB) { ... }(db)) is overly verbose; replace it with a
simpler deferred call that captures db from the outer scope (e.g., defer func()
{ if err := db.Close(); err != nil { t.Fatalf(...) } }()) or, better, use the
existing test helper CloseDB(db) used in licenses_test.go to keep patterns
consistent—update the test to call CloseDB(db) (or the simplified anonymous
defer) and remove the explicit *sqlx.DB parameter and type annotation from the
closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ccba456-ac1d-466c-8bc2-02961d70ec26
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
.github/workflows/go-ci.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.golangci.ymlCHANGELOG.mdMakefilego.modpkg/dtos/dependency_input.gopkg/dtos/dependency_input_test.gopkg/dtos/dependency_output.gopkg/dtos/dependency_output_test.gopkg/errors/error.gopkg/models/all_urls.gopkg/models/all_urls_test.gopkg/models/common.gopkg/models/common_test.gopkg/models/dependencies.gopkg/models/dependencies_test.gopkg/models/golang_projects.gopkg/models/golang_projects_test.gopkg/models/licenses.gopkg/models/licenses_test.gopkg/models/mines.gopkg/models/mines_test.gopkg/models/projects.gopkg/models/projects_test.gopkg/models/versions.gopkg/models/versions_test.gopkg/protocol/rest/server.gopkg/service/dependency_service.gopkg/service/dependency_service_test.gopkg/service/dependency_support.gopkg/service/dependency_support_test.gopkg/shared/index.gopkg/transdep/dependency_collector.gopkg/transdep/dependency_collector_test.gopkg/transdep/dependency_graph.gopkg/transdep/helper.gopkg/transdep/utils_test.gopkg/usecase/dependency.gopkg/usecase/dependency_test.go
💤 Files with no reviewable changes (1)
- pkg/models/common.go
✅ Files skipped from review due to trivial changes (9)
- pkg/shared/index.go
- pkg/transdep/utils_test.go
- pkg/dtos/dependency_input_test.go
- pkg/protocol/rest/server.go
- pkg/errors/error.go
- pkg/dtos/dependency_output_test.go
- pkg/transdep/dependency_collector_test.go
- pkg/transdep/dependency_collector.go
- pkg/transdep/helper.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/transdep/dependency_graph.go
- .github/workflows/golangci-lint.yml
- pkg/models/common_test.go
- Makefile
- pkg/models/versions_test.go
- .github/workflows/go-ci.yml
- pkg/models/dependencies.go
- pkg/models/projects_test.go
- pkg/models/mines_test.go
- pkg/models/versions.go
- .github/workflows/release.yml
- pkg/models/all_urls_test.go
- .golangci.yml
- pkg/models/golang_projects_test.go
- pkg/models/licenses.go
- pkg/usecase/dependency_test.go
- pkg/models/projects.go
- pkg/service/dependency_service.go
- CHANGELOG.md
Comment on lines
+90
to
+100
| if result.PurlName == "" && component.PurlType == "golang" { | ||
| return m.getURLsByGolangPurl(component) | ||
| } | ||
| return result, nil | ||
| } | ||
| result, err := m.GetURLsByPurlNameType(component.Name, component.PurlType) | ||
| if err != nil { | ||
| return AllURL{}, err | ||
| } | ||
| if result.PurlName == "" && component.PurlType == "golang" { | ||
| return m.getURLsByGolangPurl(component) |
There was a problem hiding this comment.
Don't use PurlName as the golang miss sentinel anymore.
Line 216 now fills PurlName on fallback results, so after a normal all_urls miss this branch no longer calls getURLsByGolangPurl. Most Go components will return the generic fallback immediately and never reach golang_projects or the GitHub-conversion path. Please switch this to an explicit “found” signal instead of inferring it from PurlName.
Also applies to: 216-216
🤖 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 90 - 100, Get rid of using PurlName ==
"" as the golang miss sentinel: change GetURLsByPurlNameType to return an
explicit "found" signal (e.g., (AllURL, bool, error) or set a Found bool on
AllURL) and update the call sites in this function to check that explicit flag
instead of PurlName when deciding to call getURLsByGolangPurl; update any other
callers (including the golang_projects/GitHub-conversion paths) to honor the new
found signal so fallback results that populate PurlName no longer block the
golang fallback.
Comment on lines
+125
to
135
| if processedComponent.Requirement != "" { | ||
| v := purlutils.GetVersionFromReqOperator(processedComponent.Requirement) | ||
| // Compare versions ignoring the "v" prefix (e.g., "v1.2.3" == "1.2.3") | ||
| // If the version does not satisfy the requirement, mark the status accordingly. | ||
| if strings.TrimPrefix(url.Version, "v") != strings.TrimPrefix(v, "v") { | ||
| depOutput.Status = domain.ComponentStatus{ | ||
| StatusCode: domain.VersionNotFound, | ||
| Message: fmt.Sprintf("Requirement not met, showing information for version '%s'", url.Version), | ||
| } | ||
| depOutput.Version = v | ||
| } |
There was a problem hiding this comment.
Don't treat ranged requirements as exact pins, and keep the resolved version.
This exact compare will flag valid ranges like the PR's ^1.5.5 -> 1.14.3 example as VERSION_NOT_FOUND. On real mismatches, Line 134 then replaces the resolved version with the requested one, which also conflicts with the PR's boost 0.1 does not satisfy requirement 1.83.0 response shape. Evaluate the requirement with proper range semantics and leave depOutput.Version set to url.Version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/usecase/dependency.go` around lines 125 - 135, The code treats
processedComponent.Requirement as an exact pin by comparing trimmed strings
(using purlutils.GetVersionFromReqOperator) which misflags ranged requirements;
replace the string equality check with a proper range evaluation (e.g., use a
semver/range check utility) to test whether url.Version satisfies
processedComponent.Requirement, and only set depOutput.Status to VersionNotFound
when that range check fails; do not overwrite depOutput.Version with the
requested requirement—leave depOutput.Version as url.Version and only modify
depOutput.Status (domain.ComponentStatus) and Message when the range check
indicates a real mismatch.
✅ Actions performedReview triggered.
|
✅ Actions performedReviews resumed. |
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
4 participants
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
New Features
Changed
Chores
Documentation
scanoss.js
scanoss.py