Chore/sp 4306 add info message and info code to responses#50
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (49)
📝 WalkthroughWalkthroughThe pull request refactors status handling across response builders and use cases by migrating from local Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
9dcfd55 to
bf763a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/responsebuilder/algorithms_in_range_response_builder.go (1)
47-65:⚠️ Potential issue | 🟠 MajorMap non-success statuses to
InfoCode/InfoMessagein this builder too.The component builders below manually translate
StatusintoInfo*, butToAlgorithmsInRangeResponsestill relies on JSON conversion. Since the DTO emits astatusobject and the protobuf response expectsinfo_code/info_message, non-success per-PURL status details can be silently dropped on this response path.🐛 Proposed manual mapping
import ( "context" - "encoding/json" "errors" status "github.com/scanoss/go-grpc-helper/pkg/grpc/domain" "net/http" @@ func ToAlgorithmsInRangeResponse(ctx context.Context, s *zap.SugaredLogger, output domain.CryptoInRangeOutput) (*pb.AlgorithmsInRangeResponse, error) { - data, err := json.Marshal(output) - - if err != nil { - s.Errorf("Problem marshalling Cryptography request output: %v", err) - return &pb.AlgorithmsInRangeResponse{}, errors.New("problem marshalling Cryptography output") - } - var response pb.AlgorithmsInRangeResponse - err = json.Unmarshal(data, &response) - if err != nil { - s.Errorf("Problem unmarshalling Cryptography request output: %v", err) - return &pb.AlgorithmsInRangeResponse{}, errors.New("problem unmarshalling Cryptography output") + response := &pb.AlgorithmsInRangeResponse{ + Purls: make([]*pb.AlgorithmsInRangeResponse_Purl, 0, len(output.Cryptography)), + } + for _, c := range output.Cryptography { + algorithms := make([]*pb.Algorithm, 0, len(c.Algorithms)) + for _, alg := range c.Algorithms { + algorithms = append(algorithms, &pb.Algorithm{ + Algorithm: alg.Algorithm, + Strength: alg.Strength, + }) + } + purl := &pb.AlgorithmsInRangeResponse_Purl{ + Purl: c.Purl, + Versions: c.Versions, + Algorithms: algorithms, + } + if c.Status.StatusCode != status.Success { + code := c.Status.StatusCode.String() + msg := c.Status.Message + purl.InfoCode = &code + purl.InfoMessage = &msg + } + response.Purls = append(response.Purls, purl) } response.Status = &common.StatusResponse{ Status: common.StatusCode_SUCCESS, Message: "Algorithms in range retrieved successfully.", } httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusOK) - return &response, nil + return response, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/responsebuilder/algorithms_in_range_response_builder.go` around lines 47 - 65, ToAlgorithmsInRangeResponse currently uses JSON round-trip which drops per-PURL non-success status fields; change it to build pb.AlgorithmsInRangeResponse explicitly by iterating the domain.CryptoInRangeOutput entries and mapping each entry's status to the protobuf's info_code/info_message fields (e.g., translate domain status -> common.InfoCode and status message -> info_message) instead of relying on json.Unmarshal; preserve existing fields (algorithms, purl, etc.), set response.Status as now, and keep the httphelper.SetHTTPCodeOnTrailer(ctx, s, http.StatusOK) call.pkg/usecase/library_detections.go (1)
121-131:⚠️ Potential issue | 🟡 MinorTypo: double space in message, plus inconsistent format across branches.
Line 131 has
"Component without info: '%s'"with a double space betweenwithoutandinfo. Line 121 uses a single space. Line 246 inprocessSinglePurluses yet a third variant:"Component without info '%s'"(single space, no colon). These user-visible messages should be consistent.🔧 Proposed fix
@@ line 131 - Status: status.ComponentStatus{StatusCode: status.NoInfo, Message: fmt.Sprintf("Component without info: '%s'", component.Purl)}, + Status: status.ComponentStatus{StatusCode: status.NoInfo, Message: fmt.Sprintf("Component without info: '%s'", component.Purl)},Optionally align line 246 (
"Component without info '%s'") to also include the colon for parity with lines 121/131.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/library_detections.go` around lines 121 - 131, The user-visible status messages for missing component info are inconsistent and contain a double space; update all occurrences to a single, consistent format (e.g., "Component without info: '%s'") across the codebase—specifically fix the string in the block that appends to out.Hints (domain.HintsOutputItem) and the message in processSinglePurl so all use the same spelling, spacing, and punctuation when constructing status.ComponentStatus.Message.
🧹 Nitpick comments (6)
pkg/responsebuilder/hints_in_range_response_builder_test.go (2)
34-85: Fixture duplication betweenhintsInRangeFixturesand each test'swantsmap.The
aes/rsadetection items are re-declared in every test function (TestToHintsInRangeResponseline 91-92,TestToComponentsHintsInRangeResponseline 143-144, andTestComponentHintsInRangeResponseline 195) and also insidehintsInRangeFixtures. Consider hoisting them to package-level test variables (or a single helper) to reduce drift risk when updating test data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/responsebuilder/hints_in_range_response_builder_test.go` around lines 34 - 85, The fixtures duplicate the AES/RSA ECDetectedItem definitions across hintsInRangeFixtures and several tests (TestToHintsInRangeResponse, TestToComponentsHintsInRangeResponse, TestComponentHintsInRangeResponse); hoist the shared detections into package-level test variables (e.g., var testAES = domain.ECDetectedItem{...}, var testRSA = ... ) or a single helper function and update hintsInRangeFixtures and the tests to reference those variables (replace local aes/rsa declarations with the package-level symbols) so all tests use the same source of truth.
115-117: Use.String()for consistency with production code pattern.
StatusCodeis defined astype StatusCode string, sostring(status.ComponentWithoutInfo)andstatus.ComponentWithoutInfo.String()are functionally equivalent. However, the production builder uses.String(), and aligning the test with that pattern improves consistency:Changes needed
Line 115:
- InfoCode: ptr(string(status.ComponentWithoutInfo)), + InfoCode: ptr(status.ComponentWithoutInfo.String()),Line 168 in
TestToComponentsHintsInRangeResponse:- InfoCode: ptr(string(status.NoInfo)), + InfoCode: ptr(status.NoInfo.String()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/responsebuilder/hints_in_range_response_builder_test.go` around lines 115 - 117, The test uses string(status.ComponentWithoutInfo) for the InfoCode field but should use the production pattern status.ComponentWithoutInfo.String(); update the test in TestToComponentsHintsInRangeResponse (the struct setting InfoCode) to call .String() on status.ComponentWithoutInfo so the test matches production code style and consistency with the StatusCode type.pkg/usecase/cryptography_major.go (1)
55-67: Local variablestatusshadows the package alias inside the loop.
parseAndValidateComponentreturns a value being assigned to a localstatusat line 56, which shadows the importedstatuspackage alias within the body of thisforloop. It currently compiles because the package alias is only referenced outside this loop (line 71 and below), but it's fragile — any future edit adding astatus.Xreference inside this loop will silently break. Consider renaming the local variable (e.g.,compStatusorst) or the package alias.♻️ Proposed rename
for _, component := range components { - status, packageURL, purlName := parseAndValidateComponent(s, component) + compStatus, packageURL, purlName := parseAndValidateComponent(s, component) cryptoItem := domain.CryptoInRangeOutputItem{ - Status: status, + Status: compStatus, Purl: component.Purl,The same pattern exists in
pkg/usecase/cryptography_versions_using.goat line 57 and should be fixed there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cryptography_major.go` around lines 55 - 67, The local variable named status in the loop that assigns parseAndValidateComponent(...) should be renamed (e.g., compStatus or st) to avoid shadowing the imported package alias status; update the variable usage inside the loop when building domain.CryptoInRangeOutputItem (Status: ...) to use the new name, and apply the same rename in the corresponding loop in the cryptography_versions_using.go file to prevent future accidental package shadowing.pkg/usecase/cryptography_versions_using.go (1)
77-82: Pre-existing:errQfromGetUrlsByPurlNameTypeInRangeis discarded.Not changed in this PR, but worth noting since the status code was migrated here: when
GetUrlsByPurlNameTypeInRangereturns an error,errQis unused (line 82:_ = errQ) and the error path is indistinguishable from a legitimate empty result — both map toComponentNotFound. If the underlying call fails transiently, the caller will be told the component doesn't exist.Consider logging or surfacing
errQdistinctly (e.g.,ComponentWithoutInfofor DB errors vsComponentNotFoundfor genuine empty results), matching the pattern used at line 91-95 forcryptoUsage.GetCryptoUsageByURLHashes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/usecase/cryptography_versions_using.go` around lines 77 - 82, The call to GetUrlsByPurlNameTypeInRange returns an error (errQ) that is currently discarded which conflates DB errors with genuine "no results" and sets StatusCode to ComponentNotFound; update the loop handling the result from GetUrlsByPurlNameTypeInRange (variable errQ, res, component) to check if errQ != nil and surface or log it and set a distinct status (e.g., ComponentWithoutInfo or include the error message) instead of treating it as ComponentNotFound; mirror the pattern used around cryptoUsage.GetCryptoUsageByURLHashes (lines handling errors at 91-95) so DB/query failures are distinguishable from empty results.pkg/responsebuilder/encryption_hints_response_builder_test.go (1)
39-252: Test refactor LGTM.Inline fixtures + full-struct assertions are a nice improvement over the CSV/JSON-driven approach. One small observation:
aes/rsadetection items are redeclared insideTestToHintsResponseandTestToComponentsEncryptionHintsResponse(lines 119-120, 171-172) despite being declared inencryptionHintsFixtures(lines 40-55). Consider extracting them to package-level test vars or a small helper to avoid drift if any field changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/responsebuilder/encryption_hints_response_builder_test.go` around lines 39 - 252, Tests redeclare AES/RSA detection items in TestToHintsResponse and TestToComponentsEncryptionHintsResponse even though encryptionHintsFixtures defines them; extract the AES/RSA items to package-level test variables (or a small helper function) and update encryptionHintsFixtures, TestToHintsResponse, TestToComponentsEncryptionHintsResponse, and TestComponentEncryptionHintsResponse to reference those shared vars (look for symbols encryptionHintsFixtures, TestToHintsResponse, TestToComponentsEncryptionHintsResponse, and the local aes/rsa declarations) so the same canonical domain.ECDetectedItem instances are used across fixtures and tests to avoid drift.pkg/responsebuilder/algorithms_in_range_response_buider_test.go (1)
109-254: Consistency: preferStatusCode.String()overstring(StatusCode)in test expectations for alignment with production code.The response builders produce the
InfoCodevalue viav.Status.StatusCode.String(), but the tests construct expected values via direct type conversion:ptr(string(status.InvalidPurl)),ptr(string(status.ComponentNotFound)), etc.Currently,
StatusCodeis defined astype StatusCode stringwith a trivialString()method that returnsstring(s)unchanged, so both approaches produce identical results and tests pass. However, this inconsistency diverges from the production pattern. To maintain consistency and resilience against future changes to theString()method, update all test occurrences to use:InfoCode: ptr(status.ComponentNotFound.String()),instead of:
InfoCode: ptr(string(status.ComponentNotFound)),Applies to all
ptr(string(status.X))occurrences in this file and inalgorithms_response_builder_test.go,encryption_hints_response_builder_test.go, andversions_in_range_response_builder_test.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/responsebuilder/algorithms_in_range_response_buider_test.go` around lines 109 - 254, Tests use direct type conversion ptr(string(status.X)) for InfoCode while production uses v.Status.StatusCode.String(); update all occurrences of ptr(string(status.*)) in this test file (and the other listed tests: algorithms_response_builder_test.go, encryption_hints_response_builder_test.go, versions_in_range_response_builder_test.go) to call the String() method instead (e.g., ptr(status.ComponentNotFound.String())) so expectations match builders; locate uses by searching for "InfoCode: ptr(string(status." and replace with the corresponding "InfoCode: ptr(status.<StatusName>.String())" for each StatusCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 32-36: Update the five OpenTelemetry module entries in go.mod from
v1.40.0 to v1.43.0 to remediate the PATH hijacking vulnerability; specifically
change versions for go.opentelemetry.io/otel/sdk,
go.opentelemetry.io/otel/sdk/metric,
go.opentelemetry.io/otel/exporters/otlp/otlptrace,
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc, and
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc to v1.43.0,
then run `go mod tidy`/`go get` to update the lockfile and ensure all otel
packages resolve to the patched v1.43.0 release.
In `@pkg/service/cryptography_service.go`:
- Line 74: The stray fmt.Printf("GetAlgorithms, %v\n", request) in the
GetAlgorithms handler is a leftover debug print; remove it (and the fmt import
if now unused) or replace it with the structured logger used in this file
(ctxzap.FromContext(ctx).Debug or Info) so the request is logged with
context/correlation like the Echo handler; locate the GetAlgorithms method in
pkg/service/cryptography_service.go and either drop the print entirely or emit a
contextual debug log using ctxzap.FromContext(ctx).Debugw/Debugf with a
descriptive message and the request payload.
In `@pkg/usecase/cryptography_major.go`:
- Line 98: There is a typo in the status message string: change "withput" to
"without" in the assignment to c.Status (the ComponentStatus instantiation that
uses status.ComponentStatus, StatusCode: status.NoInfo, and fmt.Sprintf with
c.Purl); update the fmt.Sprintf message to read "Component without info %s" so
the log/ status text is correct.
In `@pkg/usecase/cryptography_versions_using_test.go`:
- Around line 160-162: The test assertion message has a typo: update the
t.Fatalf call that checks v.Status.StatusCode against status.InvalidSemver to
read "invalid semver" instead of "inavlid semver" (i.e., change the string
inside t.Fatalf to "Expected to get 'invalid semver' status, but get '%s'") so
the failure message is correct and clear; locate the assertion using
v.Status.StatusCode, status.InvalidSemver and t.Fatalf to apply the fix.
---
Outside diff comments:
In `@pkg/responsebuilder/algorithms_in_range_response_builder.go`:
- Around line 47-65: ToAlgorithmsInRangeResponse currently uses JSON round-trip
which drops per-PURL non-success status fields; change it to build
pb.AlgorithmsInRangeResponse explicitly by iterating the
domain.CryptoInRangeOutput entries and mapping each entry's status to the
protobuf's info_code/info_message fields (e.g., translate domain status ->
common.InfoCode and status message -> info_message) instead of relying on
json.Unmarshal; preserve existing fields (algorithms, purl, etc.), set
response.Status as now, and keep the httphelper.SetHTTPCodeOnTrailer(ctx, s,
http.StatusOK) call.
In `@pkg/usecase/library_detections.go`:
- Around line 121-131: The user-visible status messages for missing component
info are inconsistent and contain a double space; update all occurrences to a
single, consistent format (e.g., "Component without info: '%s'") across the
codebase—specifically fix the string in the block that appends to out.Hints
(domain.HintsOutputItem) and the message in processSinglePurl so all use the
same spelling, spacing, and punctuation when constructing
status.ComponentStatus.Message.
---
Nitpick comments:
In `@pkg/responsebuilder/algorithms_in_range_response_buider_test.go`:
- Around line 109-254: Tests use direct type conversion ptr(string(status.X))
for InfoCode while production uses v.Status.StatusCode.String(); update all
occurrences of ptr(string(status.*)) in this test file (and the other listed
tests: algorithms_response_builder_test.go,
encryption_hints_response_builder_test.go,
versions_in_range_response_builder_test.go) to call the String() method instead
(e.g., ptr(status.ComponentNotFound.String())) so expectations match builders;
locate uses by searching for "InfoCode: ptr(string(status." and replace with the
corresponding "InfoCode: ptr(status.<StatusName>.String())" for each StatusCode.
In `@pkg/responsebuilder/encryption_hints_response_builder_test.go`:
- Around line 39-252: Tests redeclare AES/RSA detection items in
TestToHintsResponse and TestToComponentsEncryptionHintsResponse even though
encryptionHintsFixtures defines them; extract the AES/RSA items to package-level
test variables (or a small helper function) and update encryptionHintsFixtures,
TestToHintsResponse, TestToComponentsEncryptionHintsResponse, and
TestComponentEncryptionHintsResponse to reference those shared vars (look for
symbols encryptionHintsFixtures, TestToHintsResponse,
TestToComponentsEncryptionHintsResponse, and the local aes/rsa declarations) so
the same canonical domain.ECDetectedItem instances are used across fixtures and
tests to avoid drift.
In `@pkg/responsebuilder/hints_in_range_response_builder_test.go`:
- Around line 34-85: The fixtures duplicate the AES/RSA ECDetectedItem
definitions across hintsInRangeFixtures and several tests
(TestToHintsInRangeResponse, TestToComponentsHintsInRangeResponse,
TestComponentHintsInRangeResponse); hoist the shared detections into
package-level test variables (e.g., var testAES = domain.ECDetectedItem{...},
var testRSA = ... ) or a single helper function and update hintsInRangeFixtures
and the tests to reference those variables (replace local aes/rsa declarations
with the package-level symbols) so all tests use the same source of truth.
- Around line 115-117: The test uses string(status.ComponentWithoutInfo) for the
InfoCode field but should use the production pattern
status.ComponentWithoutInfo.String(); update the test in
TestToComponentsHintsInRangeResponse (the struct setting InfoCode) to call
.String() on status.ComponentWithoutInfo so the test matches production code
style and consistency with the StatusCode type.
In `@pkg/usecase/cryptography_major.go`:
- Around line 55-67: The local variable named status in the loop that assigns
parseAndValidateComponent(...) should be renamed (e.g., compStatus or st) to
avoid shadowing the imported package alias status; update the variable usage
inside the loop when building domain.CryptoInRangeOutputItem (Status: ...) to
use the new name, and apply the same rename in the corresponding loop in the
cryptography_versions_using.go file to prevent future accidental package
shadowing.
In `@pkg/usecase/cryptography_versions_using.go`:
- Around line 77-82: The call to GetUrlsByPurlNameTypeInRange returns an error
(errQ) that is currently discarded which conflates DB errors with genuine "no
results" and sets StatusCode to ComponentNotFound; update the loop handling the
result from GetUrlsByPurlNameTypeInRange (variable errQ, res, component) to
check if errQ != nil and surface or log it and set a distinct status (e.g.,
ComponentWithoutInfo or include the error message) instead of treating it as
ComponentNotFound; mirror the pattern used around
cryptoUsage.GetCryptoUsageByURLHashes (lines handling errors at 91-95) so
DB/query failures are distinguishable from empty results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 167f3a7d-67bb-4952-ba23-79421f136968
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumpkg/responsebuilder/testdata/algorithms_in_range_test_cases.csvis excluded by!**/*.csvpkg/responsebuilder/testdata/algorithms_response_test_cases.csvis excluded by!**/*.csvpkg/responsebuilder/testdata/encryption_hints_response_test_cases.csvis excluded by!**/*.csvpkg/responsebuilder/testdata/hints_in_range_test_cases.csvis excluded by!**/*.csvpkg/responsebuilder/testdata/versions_in_range_test_cases.csvis excluded by!**/*.csv
📒 Files selected for processing (26)
CHANGELOG.mdgo.modpkg/domain/cryptography_output.gopkg/domain/ec_output.gopkg/responsebuilder/algorithms_in_range_response_buider_test.gopkg/responsebuilder/algorithms_in_range_response_builder.gopkg/responsebuilder/algorithms_response_builder.gopkg/responsebuilder/algorithms_response_builder_test.gopkg/responsebuilder/encryption_hints_response_builder.gopkg/responsebuilder/encryption_hints_response_builder_test.gopkg/responsebuilder/error_code_response_builder.gopkg/responsebuilder/error_code_response_builder_test.gopkg/responsebuilder/hints_in_range_response_builder.gopkg/responsebuilder/hints_in_range_response_builder_test.gopkg/responsebuilder/versions_in_range_response_builder.gopkg/responsebuilder/versions_in_range_response_builder_test.gopkg/service/cryptography_service.gopkg/usecase/component_validator.gopkg/usecase/cryptography_major.gopkg/usecase/cryptography_major_test.gopkg/usecase/cryptography_search.gopkg/usecase/cryptography_search_test.gopkg/usecase/cryptography_versions_using.gopkg/usecase/cryptography_versions_using_test.gopkg/usecase/library_detections.gopkg/usecase/library_detections_test.go
💤 Files with no reviewable changes (2)
- pkg/responsebuilder/error_code_response_builder.go
- pkg/responsebuilder/error_code_response_builder_test.go
c46243e to
b1e968d
Compare
b1e968d to
c825ffd
Compare
c825ffd to
ff9b217
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
Chores