Merged
Conversation
… retrieving dev life-cycle. Added a mapping scheme for custom status names
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable status mapping and component-status APIs. Introduces StatusMapper, DTOs, model queries, use-case logic, gRPC handlers, SQL schema/seed updates, tests, and dependency/tooling updates. ServerConfig reads Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as "gRPC Service"
participant UseCase as "Component Use Case"
participant Helper as "go-component-helper"
participant Model as "ComponentStatusModel"
participant Mapper as "StatusMapper"
participant DB as "Database"
Client->>Service: GetComponentStatus(purl, requirement)
Service->>Service: validate & convert request -> DTO
Service->>UseCase: GetComponentStatus(dto)
UseCase->>Helper: GetComponentsVersion(purl, requirement)
Helper->>DB: query version data
DB-->>Helper: version info
Helper-->>UseCase: results + StatusCode
UseCase->>Model: GetComponentStatusByPurl(purl)
Model->>DB: query project/mines
DB-->>Model: project status
Model-->>UseCase: ComponentProjectStatus
UseCase->>Mapper: MapStatus(repositoryStatus)
Mapper-->>UseCase: normalized status
UseCase-->>Service: ComponentStatusOutput
Service->>Service: convert output -> protobuf
Service-->>Client: ComponentStatusResponse
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: 10
🧹 Nitpick comments (9)
pkg/dtos/component_status_input.go (1)
21-28: Consider nil-check for logger parameter.The
Export*andParse*functions accept a logger but don't guard against a nil logger, which would panic ons.Errorf(...)if called withnil. If these functions may be invoked in contexts where the logger isn't guaranteed, add a nil check or document that a non-nil logger is required.Also applies to: 43-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dtos/component_status_input.go` around lines 21 - 28, The ExportComponentStatusInput (and sibling ParseComponentStatusInput) functions currently call s.Errorf on a possibly nil s *zap.SugaredLogger; add a nil-check at the top of each function and if s is nil substitute a no-op logger (e.g., zap.NewNop().Sugar()) or skip logging to avoid a panic. Locate ExportComponentStatusInput and the corresponding ParseComponentStatusInput functions and ensure all uses of s.Errorf/sugar logging are guarded or use the fallback no-op sugared logger before any logging occurs.pkg/service/component_service_test.go (1)
296-300: Consider adding more specific assertions for the success case.The current check only verifies the response is non-nil. Consider asserting on the status code, status message, or actual status data to ensure the response content is correct.
Example enhanced assertions
checkFn: func(t *testing.T, resp *pb.ComponentStatusResponse) { if resp == nil { t.Error("Expected non-nil response") + return + } + if resp.Status == nil { + t.Error("Expected non-nil status in response") + return + } + if resp.Status.Status != common.StatusCode_SUCCESS { + t.Errorf("Expected SUCCESS status, got %v", resp.Status.Status) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/component_service_test.go` around lines 296 - 300, The test's checkFn currently only ensures resp (pb.ComponentStatusResponse) is non-nil; enhance it to assert the actual response content by checking expected status fields (e.g., resp.Status.Code or resp.Code), status message (resp.Status.Message or resp.Message), and any relevant payload/data (e.g., resp.Data, resp.Components or length/values) against the expected values for the success case; update the checkFn in the test table to compare those fields (using t.Errorf/t.Fatalf or testify/require helpers) so the test verifies both presence and correctness of ComponentStatusResponse.pkg/models/tests/all_urls.sql (1)
22-22: Make one fixture row exercise the non-happy path.All of the explicit status rows here use
version_status = 'active', andversion_status_change_datealways matchesindexed_date. That only covers the happy path, so a regression in the custom status-mapping branch or a column mix-up between those two dates would still pass. Seed at least one row with a raw/non-default status and a different change date.Also applies to: 24-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/tests/all_urls.sql` at line 22, One fixture row in the all_urls seed only exercises the happy path (version_status='active' and version_status_change_date == indexed_date); update or add a row in the all_urls inserts (e.g., the row with package_hash '4d66775f503b1e76582e7e5b2ea54d92' or add a new package_hash) that uses a non-default/raw version_status (e.g., 'deprecated' or 'yanked') and set version_status_change_date to a different date than indexed_date so the non-happy-path mapping and date handling are exercised.pkg/models/component_status_test.go (1)
71-80: Consider adding assertions for success cases.The pass test cases print results but don't assert on them when successful. While this may be intentional for exploratory testing with limited test data, adding assertions (e.g., checking that returned fields are non-empty) would make the tests more robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/component_status_test.go` around lines 71 - 80, The loop over passTestTable currently only prints results from componentStatusModel.GetComponentStatusByPurlAndVersion and lacks assertions for successful fetches; update the test to assert expected success conditions by checking that err is nil and that the returned status fields (e.g., status.ID or other non-empty fields on the status struct) meet basic expectations for success. For each test in passTestTable, replace or augment the fmt.Printf branches with assertions such as require.NoError(t, err) (or t.Fatalf/t.Errorf) and require.NotEmpty(t, status.ID) (or equivalent checks on status fields) so successful cases fail the test when data is unexpected. Ensure you reference the same function GetComponentStatusByPurlAndVersion and variable names (test.purl, test.version, status) when adding the assertions.pkg/service/component_support.go (1)
156-165: Silently discarding conversion errors may hide issues.The error from
convertComponentStatusOutputis discarded with_. If individual component conversion fails, the loop continues and may append a nil or incomplete response to the slice.Consider logging the error or tracking failed conversions:
♻️ Proposed improvement to handle conversion errors
func convertComponentsStatusOutput(s *zap.SugaredLogger, output dtos.ComponentsStatusOutput) (*pb.ComponentsStatusResponse, error) { var statusResp pb.ComponentsStatusResponse for _, c := range output.Components { - cs, _ := convertComponentStatusOutput(s, c) + cs, err := convertComponentStatusOutput(s, c) + if err != nil { + s.Warnf("Failed to convert component status for purl %s: %v", c.Purl, err) + } statusResp.Components = append(statusResp.Components, cs) } return &statusResp, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/component_support.go` around lines 156 - 165, The loop in convertComponentsStatusOutput swallows errors from convertComponentStatusOutput by using `_`, which hides failures; change the code to capture the error (e.g., cs, err := convertComponentStatusOutput(s, c)) and handle it rather than discarding it — either log the error via the provided logger s (s.Errorf or s.With(...).Errorf) and skip appending that component, or return a wrapped error immediately to the caller so conversion failures are visible; ensure you only append cs to statusResp.Components when err == nil and cs != nil.pkg/service/component_service.go (1)
174-208: Inconsistent error handling compared to other endpoints.
GetComponentStatusreturns errors directly (lines 182-183, 189, 197, 204) without wrapping them in aStatusResponselike other endpoints (SearchComponents,GetComponentVersions,GetComponentsStatus). This inconsistency may cause different client behavior for error handling.Consider aligning with the pattern used in
GetComponentsStatus:♻️ Proposed fix for consistent error handling
func (d componentServer) GetComponentStatus(ctx context.Context, request *common.ComponentRequest) (*pb.ComponentStatusResponse, error) { s := ctxzap.Extract(ctx).Sugar() s.Info("Processing component status request...") // Verify the input request if len(request.Purl) == 0 { - s.Error("No purl supplied") - return &pb.ComponentStatusResponse{}, se.NewBadRequestError("No purl supplied", nil) + status := se.HandleServiceError(ctx, s, se.NewBadRequestError("No purl supplied", nil)) + return &pb.ComponentStatusResponse{ + ComponentStatus: &pb.ComponentStatusResponse_ComponentStatus{ + ErrorMessage: &status.Message, + }, + }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/service/component_service.go` around lines 174 - 208, GetComponentStatus currently returns raw errors directly; make its error handling consistent with other endpoints (e.g., GetComponentsStatus) by returning a pb.ComponentStatusResponse that contains a populated StatusResponse instead of returning the error directly. Specifically, for failures in convertComponentStatusInput, compUc.GetComponentStatus, and convertComponentStatusOutput, construct and return a &pb.ComponentStatusResponse{Status: <mapped status response with error message and code>} (use the same status mapping helper used elsewhere), keep logging the error via s.Errorf, and only return a non-nil error when truly needed by the RPC framework; ensure the final successful path still returns the converted statusResponse and nil error. Reference functions/types: GetComponentStatus, convertComponentStatusInput, usecase.NewComponents / GetComponentStatus, and convertComponentStatusOutput.pkg/models/tests/projects.sql (1)
1-592: Excessive repetitive comments should be cleaned up.There are nearly 600 identical
-- noinspection SpellCheckingInspectionForFilecomments at the start of this file. A single instance at the top of the file is sufficient for IDE spell-check suppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/tests/projects.sql` around lines 1 - 592, The file contains nearly 600 duplicated suppression comments '-- noinspection SpellCheckingInspectionForFile'; remove all but a single instance at the very top of pkg/models/tests/projects.sql so only one suppression comment remains and the rest of the repeated lines are deleted, leaving the rest of the SQL untouched.pkg/usecase/component.go (1)
240-263: Consider consolidating duplicate error handling branches.The
InvalidPurlandComponentNotFoundcases produce identical output structures. These could be combined using a fallthrough or shared handling to reduce duplication.♻️ Proposed consolidation
- case domain.InvalidPurl: // The requested purl is invalid, minimun data retrieved - errorStatus := dtos.ComponentStatusOutput{ - Purl: results[0].Purl, - Name: "", - Requirement: results[0].Requirement, - ComponentStatus: &dtos.ComponentStatusInfo{ - ErrorMessage: &results[0].Status.Message, - ErrorCode: &results[0].Status.StatusCode, - }, - } - return errorStatus, nil - - case domain.ComponentNotFound: // Component not found on DB, minimun data retrieved + case domain.InvalidPurl, domain.ComponentNotFound: errorStatus := dtos.ComponentStatusOutput{ Purl: results[0].Purl, Name: "", Requirement: results[0].Requirement, ComponentStatus: &dtos.ComponentStatusInfo{ ErrorMessage: &results[0].Status.Message, ErrorCode: &results[0].Status.StatusCode, }, } return errorStatus, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/component.go` around lines 240 - 263, The two switch branches handling domain.InvalidPurl and domain.ComponentNotFound in pkg/usecase/component.go produce identical dtos.ComponentStatusOutput; consolidate them by merging the cases (e.g., use domain.InvalidPurl, domain.ComponentNotFound: or use fallthrough) and route both to a single handler that builds the errorStatus (referencing results[0].Purl, results[0].Requirement and results[0].Status.Message/StatusCode) and returns it, so you remove the duplicated construction logic while preserving the returned value and behavior.pkg/models/component_status.go (1)
85-99: Consider using explicit JOIN syntax for consistency.The query uses comma-join syntax (
from all_urls au, mines m where au.mine_id = m.id) whileGetProjectStatusByPurluses explicitJOINsyntax. Consider using explicitJOINfor consistency and clarity across all queries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/component_status.go` around lines 85 - 99, The SQL uses old comma-style joins; update the query in the block that populates []ComponentVersionStatus (the code calling m.q.SelectContext and logging "Failed to query component status...") to use explicit JOIN syntax (e.g., FROM all_urls au JOIN mines m ON au.mine_id = m.id) and move the join condition out of the WHERE clause so the WHERE only contains filters (purl_name, m.purl_type, au."version"); keep the same parameter order (purlName, purl.Type, version) and ensure the query string is updated wherever ComponentVersionStatus is queried so it matches the explicit JOIN style used by GetProjectStatusByPurl.
🤖 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`:
- Line 16: Change the compound adjective "registry specific" to
"registry-specific" in the changelog line containing "Added support for custom
status mapping for _retrieved_ and _registry_ specific status" so it reads
"Added support for custom status mapping for _retrieved_ and _registry-specific_
status".
In `@go.mod`:
- Line 22: Update the grpc dependency entry for google.golang.org/grpc in go.mod
from v1.79.1 to v1.79.3 to remediate the authorization bypass
(GHSA-p77j-4mvh-x3m3); after editing the google.golang.org/grpc version string
run the module tooling (e.g., go get google.golang.org/grpc@v1.79.3 and go mod
tidy) to refresh go.sum and ensure the lockfile is updated and builds still
pass, and verify any code referencing grpc (imports of google.golang.org/grpc)
compiles against v1.79.3.
In `@pkg/config/server_config.go`:
- Around line 93-96: The code calls zap.S() inside NewServerConfig to create the
StatusMapper (cfg.statusMapper = NewStatusMapper(s, cfg.StatusMapping.Mapping))
before the application logger is initialized in zlog.SetupAppLogger(); change
the initialization so the StatusMapper receives a real logger: either (A)
refactor NewServerConfig to accept a logger parameter and pass the initialized
logger from the caller (update callers such as getConfig() to provide the logger
after zlog.SetupAppLogger()), or (B) ensure zlog.SetupAppLogger() runs before
getConfig()/NewServerConfig so zap.S() is valid; update references to
NewStatusMapper and zap.S() accordingly to remove use of a no-op logger.
In `@pkg/dtos/component_status_input.go`:
- Line 25: Remove the unintended trailing space in the error string used when
creating the error (the errors.New call that returns "failed to produce JSON "),
update that literal to "failed to produce JSON" in
pkg/dtos/component_status_input.go (both occurrences, including the one at the
second instance noted), and run tests/lint to ensure no other similar
trailing-space error literals remain.
In `@pkg/dtos/component_status_output.go`:
- Around line 48-77: Update the export functions to correct the log text and
error strings: in ExportComponentStatusOutput and ExportComponentsStatusOutput
replace the log message "Parse failure" with a more accurate message like
"Marshal failure" (or "Export failure") and remove the trailing space in the
returned error string ("failed to produce JSON "). Also verify
ParseComponentStatusOutput's log message remains "Parse failure" (since it
unmarshals) so only change the two Export* functions referenced above.
In `@pkg/models/component_status.go`:
- Around line 129-156: The SQL query in the query variable used in the method
that fills ComponentProjectStatus (via m.q.SelectContext into results) is not
guaranteed to return the "latest" row because it lacks ordering; update the
query to deterministically pick the latest version (e.g., add ORDER BY
last_indexed_date DESC and LIMIT 1 or ORDER BY last_indexed_date DESC and remove
DISTINCT if appropriate) so that results[0] is the most recent component status
for the given purlName and purl.Type.
In `@pkg/models/tests/all_urls.sql`:
- Around line 9124-9128: The backfill uses '2020-01-01' as a sentinel which can
falsely make old fixture rows appear newer; update the UPDATE statement that
touches table all_urls and the COALESCE expressions for indexed_date and
version_status_change_date to use a clearly old sentinel (e.g., '1900-01-01') or
else handle rows with date IS NULL explicitly (set indexed_date and
version_status_change_date to the old sentinel only when date IS NULL) and keep
version_status defaulting to 'active' via the existing COALESCE on
version_status so chronology/assertions are not skewed.
In `@pkg/usecase/component.go`:
- Around line 265-267: The fallback return incorrectly reports "purl is
required" even though purl was validated earlier; update the fallback in this
function to return a meaningful error that reflects GetComponentsVersion failure
(e.g., "failed to retrieve component versions" or include status/empty results),
referencing the GetComponentsVersion call and replacing the
se.NewBadRequestError("purl is required", ...) usage with a new descriptive
message (and consider a different error type if appropriate) while still
returning dtos.ComponentStatusOutput{}.
- Around line 181-201: The code dereferences statusVersion when building
dtos.ComponentStatusOutput -> VersionStatusOutput without checking for nil;
update the logic around the calls to
c.componentStatus.GetComponentStatusByPurlAndVersion to check if statusVersion
is nil (or errVersion != nil) before using its fields and, if nil, set
VersionStatus to nil or populate it with safe defaults (e.g., empty strings and
an Unknown status via c.statusMapper) and log the condition; ensure all accesses
to statusVersion.Version, statusVersion.VersionStatus.String and
statusVersion.IndexedDate.String are guarded so no nil pointer dereference
occurs.
In `@README.md`:
- Line 32: Fix the typographical error in the README section heading by changing
the heading text "Satus mapping" to "Status mapping" (update the exact header
string "Satus mapping" in the README.md file).
---
Nitpick comments:
In `@pkg/dtos/component_status_input.go`:
- Around line 21-28: The ExportComponentStatusInput (and sibling
ParseComponentStatusInput) functions currently call s.Errorf on a possibly nil s
*zap.SugaredLogger; add a nil-check at the top of each function and if s is nil
substitute a no-op logger (e.g., zap.NewNop().Sugar()) or skip logging to avoid
a panic. Locate ExportComponentStatusInput and the corresponding
ParseComponentStatusInput functions and ensure all uses of s.Errorf/sugar
logging are guarded or use the fallback no-op sugared logger before any logging
occurs.
In `@pkg/models/component_status_test.go`:
- Around line 71-80: The loop over passTestTable currently only prints results
from componentStatusModel.GetComponentStatusByPurlAndVersion and lacks
assertions for successful fetches; update the test to assert expected success
conditions by checking that err is nil and that the returned status fields
(e.g., status.ID or other non-empty fields on the status struct) meet basic
expectations for success. For each test in passTestTable, replace or augment the
fmt.Printf branches with assertions such as require.NoError(t, err) (or
t.Fatalf/t.Errorf) and require.NotEmpty(t, status.ID) (or equivalent checks on
status fields) so successful cases fail the test when data is unexpected. Ensure
you reference the same function GetComponentStatusByPurlAndVersion and variable
names (test.purl, test.version, status) when adding the assertions.
In `@pkg/models/component_status.go`:
- Around line 85-99: The SQL uses old comma-style joins; update the query in the
block that populates []ComponentVersionStatus (the code calling
m.q.SelectContext and logging "Failed to query component status...") to use
explicit JOIN syntax (e.g., FROM all_urls au JOIN mines m ON au.mine_id = m.id)
and move the join condition out of the WHERE clause so the WHERE only contains
filters (purl_name, m.purl_type, au."version"); keep the same parameter order
(purlName, purl.Type, version) and ensure the query string is updated wherever
ComponentVersionStatus is queried so it matches the explicit JOIN style used by
GetProjectStatusByPurl.
In `@pkg/models/tests/all_urls.sql`:
- Line 22: One fixture row in the all_urls seed only exercises the happy path
(version_status='active' and version_status_change_date == indexed_date); update
or add a row in the all_urls inserts (e.g., the row with package_hash
'4d66775f503b1e76582e7e5b2ea54d92' or add a new package_hash) that uses a
non-default/raw version_status (e.g., 'deprecated' or 'yanked') and set
version_status_change_date to a different date than indexed_date so the
non-happy-path mapping and date handling are exercised.
In `@pkg/models/tests/projects.sql`:
- Around line 1-592: The file contains nearly 600 duplicated suppression
comments '-- noinspection SpellCheckingInspectionForFile'; remove all but a
single instance at the very top of pkg/models/tests/projects.sql so only one
suppression comment remains and the rest of the repeated lines are deleted,
leaving the rest of the SQL untouched.
In `@pkg/service/component_service_test.go`:
- Around line 296-300: The test's checkFn currently only ensures resp
(pb.ComponentStatusResponse) is non-nil; enhance it to assert the actual
response content by checking expected status fields (e.g., resp.Status.Code or
resp.Code), status message (resp.Status.Message or resp.Message), and any
relevant payload/data (e.g., resp.Data, resp.Components or length/values)
against the expected values for the success case; update the checkFn in the test
table to compare those fields (using t.Errorf/t.Fatalf or testify/require
helpers) so the test verifies both presence and correctness of
ComponentStatusResponse.
In `@pkg/service/component_service.go`:
- Around line 174-208: GetComponentStatus currently returns raw errors directly;
make its error handling consistent with other endpoints (e.g.,
GetComponentsStatus) by returning a pb.ComponentStatusResponse that contains a
populated StatusResponse instead of returning the error directly. Specifically,
for failures in convertComponentStatusInput, compUc.GetComponentStatus, and
convertComponentStatusOutput, construct and return a
&pb.ComponentStatusResponse{Status: <mapped status response with error message
and code>} (use the same status mapping helper used elsewhere), keep logging the
error via s.Errorf, and only return a non-nil error when truly needed by the RPC
framework; ensure the final successful path still returns the converted
statusResponse and nil error. Reference functions/types: GetComponentStatus,
convertComponentStatusInput, usecase.NewComponents / GetComponentStatus, and
convertComponentStatusOutput.
In `@pkg/service/component_support.go`:
- Around line 156-165: The loop in convertComponentsStatusOutput swallows errors
from convertComponentStatusOutput by using `_`, which hides failures; change the
code to capture the error (e.g., cs, err := convertComponentStatusOutput(s, c))
and handle it rather than discarding it — either log the error via the provided
logger s (s.Errorf or s.With(...).Errorf) and skip appending that component, or
return a wrapped error immediately to the caller so conversion failures are
visible; ensure you only append cs to statusResp.Components when err == nil and
cs != nil.
In `@pkg/usecase/component.go`:
- Around line 240-263: The two switch branches handling domain.InvalidPurl and
domain.ComponentNotFound in pkg/usecase/component.go produce identical
dtos.ComponentStatusOutput; consolidate them by merging the cases (e.g., use
domain.InvalidPurl, domain.ComponentNotFound: or use fallthrough) and route both
to a single handler that builds the errorStatus (referencing results[0].Purl,
results[0].Requirement and results[0].Status.Message/StatusCode) and returns it,
so you remove the duplicated construction logic while preserving the returned
value and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55e09a86-7ebf-4e84-8f5a-e33df5b51032
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.env.exampleCHANGELOG.mdREADME.mdconfig/app-config-dev.jsongo.modpkg/config/server_config.gopkg/config/status_mapper.gopkg/config/status_mapper_test.gopkg/dtos/component_status_input.gopkg/dtos/component_status_output.gopkg/models/component_status.gopkg/models/component_status_test.gopkg/models/tests/all_urls.sqlpkg/models/tests/projects.sqlpkg/models/tests/versions.sqlpkg/service/component_service.gopkg/service/component_service_test.gopkg/service/component_support.gopkg/usecase/component.gopkg/usecase/component_test.go
Comment on lines
+129
to
+156
| // Query to get both version and component status for the latest version | ||
| query := ` | ||
| select distinct | ||
| p.component, | ||
| p.first_indexed_date, | ||
| p.last_indexed_date , | ||
| p.status, | ||
| p.status_change_date | ||
| from | ||
| projects p, | ||
| mines m | ||
| where | ||
| p.mine_id = m.id | ||
| and p.purl_name = $1 | ||
| and m.purl_type = $2; | ||
| ` | ||
|
|
||
| var results []ComponentProjectStatus | ||
| err = m.q.SelectContext(m.ctx, &results, query, purlName, purl.Type) | ||
| if err != nil { | ||
| m.s.Errorf("Failed to query component status for %v: %v", purlName, err) | ||
| return nil, fmt.Errorf("failed to query component status: %v", err) | ||
| } | ||
| if len(results) == 0 { | ||
| m.s.Warnf("No status found for %v", purlName) | ||
| return nil, fmt.Errorf("component not found") | ||
| } | ||
| status = results[0] |
There was a problem hiding this comment.
Query may not return "latest version" as documented.
The comment says "gets status information for the latest version" but the query has no ORDER BY clause. With DISTINCT, the returned row is arbitrary. If deterministic results are needed, add ORDER BY last_indexed_date DESC or similar.
🔧 Proposed fix if latest is intended
query := `
select distinct
p.component,
p.first_indexed_date,
p.last_indexed_date ,
p.status,
p.status_change_date
from
projects p,
mines m
where
p.mine_id = m.id
and p.purl_name = $1
- and m.purl_type = $2;
+ and m.purl_type = $2
+ ORDER BY p.last_indexed_date DESC
+ LIMIT 1;
`📝 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
| // Query to get both version and component status for the latest version | |
| query := ` | |
| select distinct | |
| p.component, | |
| p.first_indexed_date, | |
| p.last_indexed_date , | |
| p.status, | |
| p.status_change_date | |
| from | |
| projects p, | |
| mines m | |
| where | |
| p.mine_id = m.id | |
| and p.purl_name = $1 | |
| and m.purl_type = $2; | |
| ` | |
| var results []ComponentProjectStatus | |
| err = m.q.SelectContext(m.ctx, &results, query, purlName, purl.Type) | |
| if err != nil { | |
| m.s.Errorf("Failed to query component status for %v: %v", purlName, err) | |
| return nil, fmt.Errorf("failed to query component status: %v", err) | |
| } | |
| if len(results) == 0 { | |
| m.s.Warnf("No status found for %v", purlName) | |
| return nil, fmt.Errorf("component not found") | |
| } | |
| status = results[0] | |
| // Query to get both version and component status for the latest version | |
| query := ` | |
| select distinct | |
| p.component, | |
| p.first_indexed_date, | |
| p.last_indexed_date , | |
| p.status, | |
| p.status_change_date | |
| from | |
| projects p, | |
| mines m | |
| where | |
| p.mine_id = m.id | |
| and p.purl_name = $1 | |
| and m.purl_type = $2 | |
| ORDER BY p.last_indexed_date DESC | |
| LIMIT 1; | |
| ` | |
| var results []ComponentProjectStatus | |
| err = m.q.SelectContext(m.ctx, &results, query, purlName, purl.Type) | |
| if err != nil { | |
| m.s.Errorf("Failed to query component status for %v: %v", purlName, err) | |
| return nil, fmt.Errorf("failed to query component status: %v", err) | |
| } | |
| if len(results) == 0 { | |
| m.s.Warnf("No status found for %v", purlName) | |
| return nil, fmt.Errorf("component not found") | |
| } | |
| status = results[0] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/models/component_status.go` around lines 129 - 156, The SQL query in the
query variable used in the method that fills ComponentProjectStatus (via
m.q.SelectContext into results) is not guaranteed to return the "latest" row
because it lacks ordering; update the query to deterministically pick the latest
version (e.g., add ORDER BY last_indexed_date DESC and LIMIT 1 or ORDER BY
last_indexed_date DESC and remove DISTINCT if appropriate) so that results[0] is
the most recent component status for the given purlName and purl.Type.
Contributor
Author
There was a problem hiding this comment.
Date does not affect the query
There was a problem hiding this comment.
@scanoss-qg, understood — since projects holds component-level metadata with a single row per purl_name/purl_type combination, ordering by date would indeed have no effect on the result. My concern was misplaced. Thanks for the clarification!
✏️ Learnings added
Learnt from: scanoss-qg
Repo: scanoss/components PR: 36
File: pkg/models/component_status.go:129-156
Timestamp: 2026-03-19T19:26:57.815Z
Learning: In `pkg/models/component_status.go`, the `GetComponentStatusByPurl` method queries the `projects` table which stores component-level (project-level) metadata — there is only one row per `purl_name` + `purl_type` combination, so adding `ORDER BY` on date columns has no effect on the result. Do not suggest date ordering for this query.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/config/server_config.go (1)
115-117:⚠️ Potential issue | 🟡 Minor
zap.S()at config bootstrap still risks a no-op logger.On Line 116,
zap.S()is captured duringNewServerConfig. If app logger setup runs later, mapper parse warnings/errors are silently dropped. Prefer passing an initialized logger intoNewServerConfig(or initialize logger before config creation).#!/bin/bash # Verify initialization order between config loading and logger setup. # Expected: logger setup should occur before NewServerConfig call sites. rg -n -C3 --type go 'func getConfig|NewServerConfig\(|SetupAppLogger\(|NewSugaredDevLogger\(' rg -n -C2 --type go 'zap\.S\(' pkg/config/server_config.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/server_config.go` around lines 115 - 117, The code captures a global sugared logger via zap.S() during NewServerConfig which may be a no-op if the app logger is initialized later; modify the call sites to ensure an initialized logger is provided to config initialization: change NewServerConfig to accept a *zap.SugaredLogger (or accept an interface) and update NewServerConfig implementation to use the injected logger when calling NewStatusMapper and parseStatusMappingString so cfg.statusMapper is created with a real logger, or alternatively move logger setup to run before any NewServerConfig calls so zap.S() is valid; update all constructors/callers accordingly (refer to NewServerConfig, zap.S(), NewStatusMapper, parseStatusMappingString, and cfg.statusMapper).
🧹 Nitpick comments (1)
pkg/config/server_config_integration_test.go (1)
46-48: Avoid asserting private field state when public getter already exists.The check on Line 46 couples the test to internals; asserting through
GetStatusMapper()is enough and more refactor-safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/server_config_integration_test.go` around lines 46 - 48, The test directly asserts the private field cfg.statusMapper; replace this internal check with the public accessor by calling cfg.GetStatusMapper() and asserting its non-nil result (or using existing test helpers/assertions) so the test validates behavior via the public API rather than coupling to the private field; update references to cfg.statusMapper to use GetStatusMapper() in the test containing that assertion.
🤖 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/config/server_config.go`:
- Around line 96-97: The unused struct field rawConfig is causing golangci-lint
to fail; remove the rawConfig map[string]interface{} field from the struct
declaration (e.g., remove rawConfig from the ServerConfig/type where it’s
declared) and delete any related references or comments so the field is fully
removed; alternatively, if you prefer to keep it, wire it by capturing the raw
parsed config (e.g., from Viper/Unmarshal into a map) in the
constructor/initializer function (e.g., NewServerConfig or LoadServerConfig) and
store that map in rawConfig for later post-processing, ensuring any postProcess
methods reference that field.
---
Duplicate comments:
In `@pkg/config/server_config.go`:
- Around line 115-117: The code captures a global sugared logger via zap.S()
during NewServerConfig which may be a no-op if the app logger is initialized
later; modify the call sites to ensure an initialized logger is provided to
config initialization: change NewServerConfig to accept a *zap.SugaredLogger (or
accept an interface) and update NewServerConfig implementation to use the
injected logger when calling NewStatusMapper and parseStatusMappingString so
cfg.statusMapper is created with a real logger, or alternatively move logger
setup to run before any NewServerConfig calls so zap.S() is valid; update all
constructors/callers accordingly (refer to NewServerConfig, zap.S(),
NewStatusMapper, parseStatusMappingString, and cfg.statusMapper).
---
Nitpick comments:
In `@pkg/config/server_config_integration_test.go`:
- Around line 46-48: The test directly asserts the private field
cfg.statusMapper; replace this internal check with the public accessor by
calling cfg.GetStatusMapper() and asserting its non-nil result (or using
existing test helpers/assertions) so the test validates behavior via the public
API rather than coupling to the private field; update references to
cfg.statusMapper to use GetStatusMapper() in the test containing that assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f6bbce9-9790-42e7-b034-eed5ff3b8ebf
📒 Files selected for processing (2)
pkg/config/server_config.gopkg/config/server_config_integration_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/dtos/component_status_output.go (1)
70-77:⚠️ Potential issue | 🟡 MinorFix log message and trailing space (previously flagged, still present).
The issues from the previous review remain:
- Line 73: Log says "Parse failure" but this is a marshal/export operation
- Line 74: Error string has trailing space
"failed to produce JSON "🐛 Proposed fix
func ExportComponentsStatusOutput(s *zap.SugaredLogger, output ComponentsStatusOutput) ([]byte, error) { data, err := json.Marshal(output) if err != nil { - s.Errorf("Parse failure: %v", err) - return nil, errors.New("failed to produce JSON ") + s.Errorf("Marshal failure: %v", err) + return nil, errors.New("failed to produce JSON") } return data, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dtos/component_status_output.go` around lines 70 - 77, In ExportComponentsStatusOutput, update the error logging and returned error string: change the s.Errorf call to reflect a marshal/export failure (e.g. "Failed to marshal ComponentsStatusOutput: %v") instead of "Parse failure", and remove the trailing space from the errors.New message so it returns e.g. errors.New("failed to produce JSON"); keep referencing ExportComponentsStatusOutput and the s.Errorf invocation when making the edits.pkg/usecase/component.go (1)
266-268:⚠️ Potential issue | 🟡 MinorFix misleading fallback error message (previously flagged, still present).
This fallback returns "purl is required" but purl was already validated at line 157. This code path is reached when
resultsis empty or an unhandled status code is returned. The error should reflect the actual condition.🐛 Proposed fix
- return dtos.ComponentStatusOutput{}, se.NewBadRequestError("purl is required", errors.New("purl is required")) + return dtos.ComponentStatusOutput{}, se.NewNotFoundError("no results returned for component status")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/component.go` around lines 266 - 268, The fallback currently returns se.NewBadRequestError("purl is required", ...) which is misleading because purl was validated earlier; update the logic that returns dtos.ComponentStatusOutput{} and the se.NewBadRequestError call so the error reflects the real condition (e.g., empty results or unexpected response) — use the existing results variable and, if available, the purl or response status to create a clear error like se.NewNotFoundError("no results returned for purl: <purl>") or se.NewInternalError("unexpected response status: <status>") instead of "purl is required".
🧹 Nitpick comments (1)
pkg/usecase/component.go (1)
270-303: LGTM! Batch processing with graceful per-component error handling.The implementation correctly continues processing when individual components fail and includes error information in the response. Consider adding an
ErrorCodeto the error entry (lines 292-294) for consistency with other error responses in this file, though this is optional since the error originates from a different code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/component.go` around lines 270 - 303, Add an ErrorCode field to the per-component error entry created in GetComponentsStatus: when building the fallback errorStatus (the dtos.ComponentStatusOutput returned on individual failures), populate ComponentStatusInfo.ErrorCode with an appropriate code (e.g., map from the returned error or a generic "component_error") so it matches other error responses; update the construction of errorStatus to set ComponentStatusInfo.ErrorCode alongside ErrorMessage so callers can consistently inspect an error code.
🤖 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/dtos/component_status_output.go`:
- Around line 70-77: In ExportComponentsStatusOutput, update the error logging
and returned error string: change the s.Errorf call to reflect a marshal/export
failure (e.g. "Failed to marshal ComponentsStatusOutput: %v") instead of "Parse
failure", and remove the trailing space from the errors.New message so it
returns e.g. errors.New("failed to produce JSON"); keep referencing
ExportComponentsStatusOutput and the s.Errorf invocation when making the edits.
In `@pkg/usecase/component.go`:
- Around line 266-268: The fallback currently returns
se.NewBadRequestError("purl is required", ...) which is misleading because purl
was validated earlier; update the logic that returns
dtos.ComponentStatusOutput{} and the se.NewBadRequestError call so the error
reflects the real condition (e.g., empty results or unexpected response) — use
the existing results variable and, if available, the purl or response status to
create a clear error like se.NewNotFoundError("no results returned for purl:
<purl>") or se.NewInternalError("unexpected response status: <status>") instead
of "purl is required".
---
Nitpick comments:
In `@pkg/usecase/component.go`:
- Around line 270-303: Add an ErrorCode field to the per-component error entry
created in GetComponentsStatus: when building the fallback errorStatus (the
dtos.ComponentStatusOutput returned on individual failures), populate
ComponentStatusInfo.ErrorCode with an appropriate code (e.g., map from the
returned error or a generic "component_error") so it matches other error
responses; update the construction of errorStatus to set
ComponentStatusInfo.ErrorCode alongside ErrorMessage so callers can consistently
inspect an error code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efdf5826-06d6-4a6c-a54f-f48f2da0ee77
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
CHANGELOG.mdREADME.mdgo.modpkg/config/server_config.gopkg/dtos/component_status_output.gopkg/models/tests/all_urls.sqlpkg/usecase/component.go
✅ Files skipped from review due to trivial changes (2)
- README.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/models/tests/all_urls.sql
- go.mod
eeisegn
requested changes
Mar 24, 2026
Contributor
eeisegn
left a comment
eeisegn
left a comment
There was a problem hiding this comment.
Please address linter issues, increase unit test coverage (above 80%), update to golang 1.25, and other comments inline
pkg/config/status_mapper.go
Outdated
| if dbStatus == "" { | ||
| return "" | ||
| } | ||
|
|
pkg/models/component_status.go
Outdated
|
|
||
| // Query to get both version and component status | ||
| query := ` | ||
| select distinct au.purl_name, au."version", au.indexed_date, au.version_status, au.version_status_change_date |
Contributor
There was a problem hiding this comment.
put SQL 'words' in UPPERCASE please
pkg/usecase/component.go
Outdated
| c.s.Errorf("The request does not contain purl to retrieve component status") | ||
| return dtos.ComponentStatusOutput{}, se.NewBadRequestError("purl is required", errors.New("purl is required")) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
remove blank lines or add comments
| return dtos.ComponentVersionsOutput{Component: output}, nil | ||
| } | ||
|
|
||
| func (c ComponentUseCase) GetComponentStatus(request dtos.ComponentStatusInput) (dtos.ComponentStatusOutput, error) { |
| return dtos.ComponentStatusOutput{}, se.NewBadRequestError("purl is required", errors.New("purl is required")) | ||
| } | ||
|
|
||
| func (c ComponentUseCase) GetComponentsStatus(request dtos.ComponentsStatusInput) (dtos.ComponentsStatusOutput, error) { |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/golangci-lint.yml (1)
27-28: Pin golangci-lint version for consistency between CI and local development.The
golangci-lint-action@v9defaults to the latest golangci-lint release when no version is specified. This can cause inconsistent linting results between CI (which would run the latest) and local development (which usesv2.10.1via the Makefile'slint_dockertarget).♻️ Recommended: Explicitly pin the version
- 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 golangci/golangci-lint-action@v9 which allows the action to install the latest golangci-lint and can diverge from the locally pinned version; update the workflow step that references golangci/golangci-lint-action@v9 to explicitly pin the golangci-lint tool by adding a "with: version: v2.10.1" (matching the Makefile's lint_docker target) or otherwise specify the exact golangci-lint release you want to enforce so CI and local linting produce identical results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 46-47: The Makefile target lint_docker_fix is using a hardcoded
golangci-lint image tag and the wrong pwd variable; update the target
(lint_docker_fix) to reference the project LINT_VERSION variable instead of
v1.50.1 and replace $(pwd) with the Makefile PWD variable; also update any cache
mount paths that include the golangci-lint version to use $(LINT_VERSION) so the
docker image, cache directory, and command remain consistent with the
lint_docker target and the new v2 config format.
---
Nitpick comments:
In @.github/workflows/golangci-lint.yml:
- Around line 27-28: The workflow currently uses
golangci/golangci-lint-action@v9 which allows the action to install the latest
golangci-lint and can diverge from the locally pinned version; update the
workflow step that references golangci/golangci-lint-action@v9 to explicitly pin
the golangci-lint tool by adding a "with: version: v2.10.1" (matching the
Makefile's lint_docker target) or otherwise specify the exact golangci-lint
release you want to enforce so CI and local linting produce identical results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce4eacd-044a-40f7-a068-c3595dcd699b
📒 Files selected for processing (5)
.github/workflows/go-ci.yml.github/workflows/golangci-lint.yml.github/workflows/release.yml.golangci.ymlMakefile
agustingroh
reviewed
Mar 24, 2026
eeisegn
approved these changes
Mar 26, 2026
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
3 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.
Added
GetComponentStatus/GetComponentsStatusservices for getting single and multiple development life-cycle informationChanged
Summary by CodeRabbit
New Features
Documentation
Tests
Chores