Open
Conversation
… retrieving dev life-cycle. Added a mapping scheme for custom status names
📝 WalkthroughWalkthroughAdds configurable status mapping and component status retrieval (single and batch). Introduces StatusMapper, DTOs, model queries, use-case logic, gRPC handlers, SQL schema/seed updates, tests, and config/dependency updates. ServerConfig reads STATUS_MAPPING and exposes a mapper. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as "gRPC Service"
participant UseCase as "Component UseCase"
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 with 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
pkg/config/server_config.go
Outdated
Comment on lines
+93
to
+96
|
|
||
| // Initialize the status mapper once at startup | ||
| s := zap.S() // Get global sugared logger | ||
| cfg.statusMapper = NewStatusMapper(s, cfg.StatusMapping.Mapping) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how NewServerConfig is called and if logger is initialized beforehand
rg -n -B5 "NewServerConfig" --type go | head -60Repository: scanoss/components
Length of output: 3885
🏁 Script executed:
#!/bin/bash
# Find the NewServerConfig function implementation
rg -n -A 20 "func NewServerConfig" --type go pkg/config/Repository: scanoss/components
Length of output: 1188
🏁 Script executed:
#!/bin/bash
# Check for zap logger initialization in the codebase
rg -n "zap\." --type go | grep -E "(NewLogger|ReplaceGlobals|must\.New)" | head -20Repository: scanoss/components
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there are any logger setup calls in main or init functions
rg -n -B2 -A5 "func main\|func init" --type go | grep -A5 "zap"Repository: scanoss/components
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find NewStatusMapper implementation
rg -n -A 15 "func NewStatusMapper" --type go pkg/config/Repository: scanoss/components
Length of output: 1016
🏁 Script executed:
#!/bin/bash
# Search for main function in the repository
find . -name "*.go" -type f | xargs grep -l "func main" | head -5Repository: scanoss/components
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check if zap has a default no-op logger behavior
# Look at imports and how zap is used
rg -n "import" --type go pkg/config/server_config.goRepository: scanoss/components
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Check the server main.go for logger initialization
cat cmd/server/main.goRepository: scanoss/components
Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Get the full imports section from server_config.go
head -30 pkg/config/server_config.goRepository: scanoss/components
Length of output: 1028
🏁 Script executed:
#!/bin/bash
# Find and examine the RunServer function
rg -n -A 40 "func RunServer" --type go pkg/cmd/Repository: scanoss/components
Length of output: 2241
🏁 Script executed:
#!/bin/bash
# Find the getConfig function
rg -n -A 30 "func getConfig" --type go pkg/cmd/Repository: scanoss/components
Length of output: 1654
🏁 Script executed:
#!/bin/bash
# Check for any nil-checking or fallback handling of zap.S()
rg -n "zap\.S()" --type go pkg/config/ -A 2 -B 2Repository: scanoss/components
Length of output: 2129
Initialize logger before calling NewServerConfig or defer logger-dependent initialization.
zap.S() is called before logger initialization occurs. The control flow shows NewServerConfig() is invoked from getConfig() (line 75 in pkg/cmd/server.go) which is called before zlog.SetupAppLogger() (line 87). This means StatusMapper receives a no-op logger that silently discards any log calls from configuration parsing errors. Either initialize the logger before config loading or pass a logger parameter to NewServerConfig.
🤖 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 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.
| data, err := json.Marshal(output) | ||
| if err != nil { | ||
| s.Errorf("Parse failure: %v", err) | ||
| return nil, errors.New("failed to produce JSON ") |
There was a problem hiding this comment.
Trailing space in error messages.
The error messages at lines 25 and 47 have a trailing space: "failed to produce JSON ". This is likely unintentional.
Proposed fix
- return nil, errors.New("failed to produce JSON ")
+ return nil, errors.New("failed to produce JSON")Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/dtos/component_status_input.go` at 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.
Comment on lines
+48
to
+77
| func ExportComponentStatusOutput(s *zap.SugaredLogger, output ComponentStatusOutput) ([]byte, error) { | ||
| data, err := json.Marshal(output) | ||
| if err != nil { | ||
| s.Errorf("Parse failure: %v", err) | ||
| return nil, errors.New("failed to produce JSON ") | ||
| } | ||
| return data, nil | ||
| } | ||
|
|
||
| func ParseComponentStatusOutput(s *zap.SugaredLogger, output []byte) (ComponentStatusOutput, error) { | ||
| if len(output) == 0 { | ||
| return ComponentStatusOutput{}, errors.New("no data supplied to parse") | ||
| } | ||
| var data ComponentStatusOutput | ||
| err := json.Unmarshal(output, &data) | ||
| if err != nil { | ||
| s.Errorf("Parse failure: %v", err) | ||
| return ComponentStatusOutput{}, fmt.Errorf("failed to parse data: %v", err) | ||
| } | ||
| return data, nil | ||
| } | ||
|
|
||
| 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 ") | ||
| } | ||
| return data, nil | ||
| } |
There was a problem hiding this comment.
Minor typos and misleading log message.
- Trailing space in error string at lines 52 and 74:
"failed to produce JSON " - Log message at lines 51 and 73 says "Parse failure" but the operation is marshaling (export), not parsing
🔧 Proposed fix
func ExportComponentStatusOutput(s *zap.SugaredLogger, output ComponentStatusOutput) ([]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
}Apply similar fix to ExportComponentsStatusOutput.
🤖 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 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.
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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added
GetComponentStatus/GetComponentsStatusservices for getting single and multiple development life-cycle informationChanged
Summary by CodeRabbit
New Features
Documentation
Tests
Chores