chore:SP-3074 Implements 'GetSPDXLicenseDetails' model#2
Merged
agustingroh merged 1 commit intomainfrom Apr 1, 2026
Merged
Conversation
|
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 a SeeAlsoArray type with sql.Scanner/driver.Valuer, an SPDXLicenseDetail struct, and two LicenseModel query methods to read SPDX data; includes unit tests for SeeAlsoArray.Scan, changelog entries, and dependency version updates. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Model as LicenseModel
participant DB as Database
participant Parser as SeeAlsoArray
Client->>Model: GetSPDXLicenseDetails(ctx, spdxId)
Model->>Model: validate spdxId (non-empty) / lower-case
Model->>DB: SELECT ... FROM spdx_license_data WHERE LOWER(id)=?
DB-->>Model: row (includes seealso column)
Model->>Parser: (&SeeAlsoArray).Scan(seealso_value)
alt PostgreSQL array literal
Parser->>Parser: parsePostgreSQLArray("{...}")
else JSON array
Parser->>Parser: json.Unmarshal("[...]")
else empty / nil
Parser->>Parser: set empty or nil slice
end
Parser-->>Model: SeeAlsoArray
Model-->>Client: SPDXLicenseDetail (or zero value / nil error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 0
🧹 Nitpick comments (7)
go.mod (1)
18-29: Confirm necessity of newly added indirect deps and run a tidy sweepThe added indirects (cmp, testify, otel, zap, multierr, packageurl-go) look plausible given tests and logging. However, since none are imported directly in this module, please run a fresh go mod tidy with your target Go toolchain to prune any unused entries and ensure sum consistency.
Please verify locally (CI image should match toolchain go1.24.4):
- go mod tidy -v
- Commit any changes to go.mod/go.sum if produced.
pkg/models/licenses.go (5)
42-76: Scanner logic is sound for common cases; consider edge cases and input types
- Good handling of nil, empty, string vs []byte, and fallback to JSON.
- PostgreSQL array detection is simplistic. It won’t handle quoted members, commas within quotes, or escapes. If your data might contain such cases, bolster the parser.
- Consider handling []string input (rare from drivers, but possible in custom flows).
If you want to keep it lightweight, at least unquote simple quoted items in the PostgreSQL path; see suggested refactor in parsePostgreSQLArray below.
78-101: PostgreSQL array parsing is naive; add basic quote handling or robust parsingSplitting on commas will break for quoted items that contain commas. Short-term, unquote simple "value" elements to avoid persisting quotes in results.
Apply this diff to minimally improve element cleanup:
func (s *SeeAlsoArray) parsePostgreSQLArray(str string) error { // Remove curly braces content := str[1 : len(str)-1] if content == "" { *s = []string{} return nil } // Split by comma and clean up each element parts := strings.Split(content, ",") result := make([]string, 0, len(parts)) for _, part := range parts { - trimmed := strings.TrimSpace(part) - if trimmed != "" { - result = append(result, trimmed) - } + trimmed := strings.TrimSpace(part) + if trimmed == "" { + continue + } + // Handle simple quoted items: "value" + if strings.HasPrefix(trimmed, `"`) && strings.HasSuffix(trimmed, `"`) && len(trimmed) >= 2 { + if unq, err := strconv.Unquote(trimmed); err == nil { + trimmed = unq + } else { + trimmed = strings.Trim(trimmed, `"`) + } + } + result = append(result, trimmed) } *s = result return nil }Additional change required outside this hunk:
// at top of file with other imports import "strconv"If you expect fully general PostgreSQL arrays (with escapes and nested braces), consider implementing a small state machine parser or leveraging a driver that can decode arrays.
102-109: Return JSON as string to favor TEXT storage over BLOBjson.Marshal returns []byte. Many drivers (e.g., SQLite) will store that as BLOB unless coerced. Returning string keeps it as TEXT/JSON.
func (s SeeAlsoArray) Value() (driver.Value, error) { if s == nil { return nil, nil } - return json.Marshal(s) + b, err := json.Marshal(s) + if err != nil { + return nil, err + } + return string(b), nil }
117-128: Struct naming/style and JSON exposure
- Go acronym style: prefer ID/OSI (IsDeprecatedLicenseID, IsOSIApproved). Not mandatory, but improves consistency.
- If this model is serialized to JSON anywhere, consider adding json tags to match expected API schema.
Example (no behavior change unless you expose JSON):
type SPDXLicenseDetail struct { - ID string `db:"id"` - Type string `db:"type"` - Reference string `db:"reference"` - IsDeprecatedLicenseId bool `db:"isdeprecatedlicenseid"` - DetailsURL string `db:"detailsurl"` - ReferenceNumber string `db:"referencenumber"` - Name string `db:"name"` - SeeAlso SeeAlsoArray `db:"seealso"` - IsOsiApproved bool `db:"isosiapproved"` + ID string `db:"id" json:"licenseId"` + Type string `db:"type" json:"type"` + Reference string `db:"reference" json:"reference"` + IsDeprecatedLicenseID bool `db:"isdeprecatedlicenseid" json:"isDeprecatedLicenseId"` + DetailsURL string `db:"detailsurl" json:"detailsUrl"` + ReferenceNumber string `db:"referencenumber" json:"referenceNumber"` + Name string `db:"name" json:"name"` + SeeAlso SeeAlsoArray `db:"seealso" json:"seeAlso"` + IsOSIApproved bool `db:"isosiapproved" json:"isOsiApproved"` }Note: Adjust JSON keys to your contract; above are guesses based on common SPDX JSON naming.
Please confirm DB column types (e.g., referencenumber) align with chosen Go types; if it’s numeric in DB, string may cause scan conversion issues.
178-196: Tighten query, receiver formatting, and remove redundant lowercasing
- Style: receiver should be “(m *LicenseModel)”.
- Avoid SELECT * for stability and performance; explicitly list columns mapping your struct.
- You don’t need to lowercase the parameter in Go if you already call LOWER($1) in SQL.
-// GetSPDXLicenseDetails get spdx license details. -func (m* LicenseModel) GetSPDXLicenseDetails(ctx context.Context, spdxId string) (SPDXLicenseDetail, error) { +// GetSPDXLicenseDetails get SPDX license details. +func (m *LicenseModel) GetSPDXLicenseDetails(ctx context.Context, spdxId string) (SPDXLicenseDetail, error) { s := ctxzap.Extract(ctx).Sugar() if spdxId == "" { s.Error("Please specify a valid SPDX ID to query") return SPDXLicenseDetail{}, errors.New("please specify a valid SPDX ID to query") } s.Debugf("Getting SPDX License Details for %v", spdxId) - spdxIdToLower := strings.ToLower(spdxId) var license SPDXLicenseDetail - err := m.db.QueryRowxContext(ctx, - "SELECT * FROM spdx_license_data WHERE LOWER(id) = LOWER($1)", spdxIdToLower).StructScan(&license) + query := ` + SELECT id, type, reference, isdeprecatedlicenseid, detailsurl, referencenumber, name, seealso, isosiapproved + FROM spdx_license_data + WHERE LOWER(id) = LOWER($1) + LIMIT 1` + err := m.db.QueryRowxContext(ctx, query, spdxId).StructScan(&license) if err != nil && !errors.Is(err, sql.ErrNoRows) { s.Errorf("Error: Failed to query spdx_license_data table for %v: %#v", spdxId, err) return SPDXLicenseDetail{}, fmt.Errorf("failed to query the spdx_license_data table: %v", err) } return license, nil }Optional: If this path is hot and you rely on LOWER(id), consider a functional index on LOWER(id) in PostgreSQL.
Please confirm returning zero-value detail with nil error on not-found matches existing method semantics elsewhere (it does in GetLicenseByName); if consumers expect a not-found error, we should propagate sql.ErrNoRows instead.
pkg/models/licenses_test.go (1)
192-239: Add tests for []byte inputs and Value() serializationTo fully exercise SeeAlsoArray, add:
- []byte variants for PostgreSQL and JSON.
- Value() round-trip behavior (nil → NULL, empty slice → "[]", non-empty → JSON string).
Apply this diff to extend the Scan tests:
tests := []struct { name string input interface{} want SeeAlsoArray wantErr bool }{ + { + name: "PostgreSQL array as []byte", + input: []byte("{https://a.example,https://b.example}"), + want: SeeAlsoArray{"https://a.example", "https://b.example"}, + }, + { + name: "JSON array as []byte", + input: []byte(`["https://a.example","https://b.example"]`), + want: SeeAlsoArray{"https://a.example", "https://b.example"}, + }, }And add a new test for Value():
func TestSeeAlsoArray_Value(t *testing.T) { cases := []struct{ name string in SeeAlsoArray want interface{} // string or nil }{ {"nil slice", nil, nil}, {"empty slice", SeeAlsoArray{}, "[]"}, {"non-empty", SeeAlsoArray{"https://a","https://b"}, `["https://a","https://b"]`}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { v, err := tc.in.Value() if err != nil { t.Fatalf("Value() error = %v", err) } if v == nil && tc.want == nil { return } if vs, ok := v.(string); !ok || vs != tc.want { t.Fatalf("Value() = %#v, want %#v", v, tc.want) } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)pkg/models/licenses.go(4 hunks)pkg/models/licenses_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/models/licenses_test.go (1)
pkg/models/licenses.go (1)
SeeAlsoArray(40-40)
🔇 Additional comments (1)
pkg/models/licenses_test.go (1)
192-239: Good, table-driven coverage for Scan happy pathsCovers PostgreSQL-style, JSON, empty/empty-array, and nil. LGTM as a starting point.
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
from
8252abb to
adc0602
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/models/licenses.go (1)
21-33:⚠️ Potential issue | 🟡 MinorFix import order to resolve CI failure.
The imports are not properly formatted according to
gci. Standard library imports should be grouped and ordered alphabetically.🔧 Proposed fix
import ( "context" "database/sql" - "encoding/json" "database/sql/driver" + "encoding/json" "errors" "fmt" "regexp" "strings" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "github.com/jmoiron/sqlx" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 21 - 33, The import block in pkg/models/licenses.go is misordered and failing gci; reorder imports so standard library packages (context, database/sql, database/sql/driver, encoding/json, errors, fmt, regexp, strings) are grouped first and alphabetized, followed by third-party imports (github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap, github.com/jmoiron/sqlx) also alphabetized; update the import grouping in the file to match gci expectations so CI passes.
🧹 Nitpick comments (2)
pkg/models/licenses.go (2)
179-179: Minor: Fix receiver formatting.The receiver
(m* LicenseModel)has the pointer*in an unconventional position. Standard Go style places it adjacent to the type:(m *LicenseModel).🔧 Proposed fix
-func (m* LicenseModel) GetSPDXLicenseDetails(ctx context.Context, spdxId string) (SPDXLicenseDetail, error) { +func (m *LicenseModel) GetSPDXLicenseDetails(ctx context.Context, spdxId string) (SPDXLicenseDetail, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` at line 179, The method receiver on GetSPDXLicenseDetails is formatted unconventionally as `(m* LicenseModel)`; change it to the standard Go style `(m *LicenseModel)` by moving the `*` next to the type in the method signature for the GetSPDXLicenseDetails method on LicenseModel to adhere to idiomatic Go and avoid lint warnings.
117-127: Consider aligning struct field formatting for readability.The struct field alignment is inconsistent. While functional, aligning the tags improves readability.
✨ Suggested formatting
type SPDXLicenseDetail struct { - ID string `db:"id"` - Type string `db:"type"` - Reference string `db:"reference"` - IsDeprecatedLicenseId bool `db:"isdeprecatedlicenseid"` - DetailsURL string `db:"detailsurl"` - ReferenceNumber string `db:"referencenumber"` - Name string `db:"name"` - SeeAlso SeeAlsoArray `db:"seealso"` - IsOsiApproved bool `db:"isosiapproved"` + ID string `db:"id"` + Type string `db:"type"` + Reference string `db:"reference"` + IsDeprecatedLicenseId bool `db:"isdeprecatedlicenseid"` + DetailsURL string `db:"detailsurl"` + ReferenceNumber string `db:"referencenumber"` + Name string `db:"name"` + SeeAlso SeeAlsoArray `db:"seealso"` + IsOsiApproved bool `db:"isosiapproved"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 117 - 127, The SPDXLicenseDetail struct's field formatting is inconsistent; update the declaration of SPDXLicenseDetail to align field names, their types, and db tags vertically for readability (e.g., align ID, Type, Reference, IsDeprecatedLicenseId, DetailsURL, ReferenceNumber, Name, SeeAlso, IsOsiApproved), keeping the exact field names and struct tags unchanged but adjusting spacing/indentation so types and `db:"..."` tags line up consistently across all fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/models/licenses.go`:
- Around line 102-108: Update the SeeAlsoArray.Value comment to end with a
period and silence the nilnil linter by either returning a typed nil (e.g.,
driver.Value(nil), nil) or adding a nolint directive; specifically, change the
comment above func (s SeeAlsoArray) Value() to end with a period and then either
replace the return of nil, nil with return driver.Value(nil), nil or add a
`//nolint:nilnil` comment on the Value method to acknowledge the intentional SQL
NULL return.
- Around line 39-42: Update the godoc comments to end with periods: add a
trailing period to the comment for the SeeAlsoArray type and to the comment for
the Scan implementation that mentions sql.Scanner, so function/type comments
(SeeAlsoArray and Scan) comply with the linter's requirement for sentence
punctuation.
- Around line 186-189: The query uses SELECT * and redundantly calls LOWER($1)
despite creating spdxIdToLower; change the SQL to explicitly list the columns
that map to SPDXLicenseDetail (matching the column list used in
GetLicenseByID/GetLicenseByName) instead of SELECT *, and remove LOWER($1) so
the WHERE clause reads "WHERE LOWER(id) = $1" or simply "WHERE id = $1" if you
pass spdxIdToLower; keep the call to m.db.QueryRowxContext(ctx, ...) and
StructScan(&license) unchanged and reference the SPDXLicenseDetail struct and
spdxIdToLower variable when updating the query.
- Around line 78-100: Update the documentation comment for the
parsePostgreSQLArray method to end with a period; specifically edit the comment
above SeeAlsoArray.parsePostgreSQLArray so it becomes a complete sentence ending
with a period (e.g., change "parses PostgreSQL array format like
{val1,val2,val3}" to "parses PostgreSQL array format like {val1,val2,val3}.").
---
Outside diff comments:
In `@pkg/models/licenses.go`:
- Around line 21-33: The import block in pkg/models/licenses.go is misordered
and failing gci; reorder imports so standard library packages (context,
database/sql, database/sql/driver, encoding/json, errors, fmt, regexp, strings)
are grouped first and alphabetized, followed by third-party imports
(github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap,
github.com/jmoiron/sqlx) also alphabetized; update the import grouping in the
file to match gci expectations so CI passes.
---
Nitpick comments:
In `@pkg/models/licenses.go`:
- Line 179: The method receiver on GetSPDXLicenseDetails is formatted
unconventionally as `(m* LicenseModel)`; change it to the standard Go style `(m
*LicenseModel)` by moving the `*` next to the type in the method signature for
the GetSPDXLicenseDetails method on LicenseModel to adhere to idiomatic Go and
avoid lint warnings.
- Around line 117-127: The SPDXLicenseDetail struct's field formatting is
inconsistent; update the declaration of SPDXLicenseDetail to align field names,
their types, and db tags vertically for readability (e.g., align ID, Type,
Reference, IsDeprecatedLicenseId, DetailsURL, ReferenceNumber, Name, SeeAlso,
IsOsiApproved), keeping the exact field names and struct tags unchanged but
adjusting spacing/indentation so types and `db:"..."` tags line up consistently
across all fields.
🪄 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: 00acd7ee-9935-45b2-a4d7-dcb4a838e9a7
📒 Files selected for processing (2)
pkg/models/licenses.gopkg/models/licenses_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/models/licenses_test.go
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
from
adc0602 to
6134975
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
go.mod (1)
28-28: Update google.golang.org/genproto/googleapis/rpc to the latest available version.The current version
v0.0.0-20260209200024-4cfbd4190f57(February 9, 2026) is valid but outdated. A newer versionv0.0.0-20260401024825-9d38bb4040a9(April 1, 2026) is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 28, Update the google.genproto dependency in go.mod: replace the existing version string for the module google.golang.org/genproto/googleapis/rpc (currently v0.0.0-20260209200024-4cfbd4190f57) with the newer release v0.0.0-20260401024825-9d38bb4040a9 and then run the appropriate Go module commands (e.g., go get or go mod tidy) to refresh the lockfile and ensure build consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 29: The go.mod currently pins the vulnerable module
google.golang.org/grpc at v1.79.1; update that module to a fixed version
(minimum v1.79.3, recommended v1.80.0) by running a module update (e.g., go get
google.golang.org/grpc@v1.80.0), then run go mod tidy (and vendor if you vendor
dependencies) and re-run tests to ensure nothing breaks; search for the module
string "google.golang.org/grpc" in go.mod to locate and change the version entry
and verify all CI builds use the updated module.
- Around line 25-27: The go.mod currently pins golang.org/x/net at v0.50.0 which
has a critical HTTP/2 vulnerability; update the module version for
golang.org/x/net to v0.51.0 (replace the line referencing golang.org/x/net
v0.50.0 with v0.51.0), then run the module update commands (e.g., go get
golang.org/x/net@v0.51.0 and go mod tidy) to refresh go.sum and ensure the new
version is used; finally run project tests and any CI checks to confirm no
regressions.
In `@pkg/models/licenses.go`:
- Around line 198-210: The GetAllSPDXLicensesDetails function is using
QueryRowxContext and StructScan into a slice which is incorrect; replace the
QueryRowxContext/StructScan usage with m.db.SelectContext(ctx, &licenses,
"SELECT id, type, reference, isdeprecatedlicenseid, detailsurl, referencenumber,
name, seealso, isosiapproved FROM spdx_license_data") to load all rows into the
licenses slice, keep the existing ctxzap logger (s) and error handling (log the
error and return a formatted error) but adjust to check the SelectContext error
directly (no need to use sql.ErrNoRows handling for Select), and return the
licenses slice on success from GetAllSPDXLicensesDetails.
---
Nitpick comments:
In `@go.mod`:
- Line 28: Update the google.genproto dependency in go.mod: replace the existing
version string for the module google.golang.org/genproto/googleapis/rpc
(currently v0.0.0-20260209200024-4cfbd4190f57) with the newer release
v0.0.0-20260401024825-9d38bb4040a9 and then run the appropriate Go module
commands (e.g., go get or go mod tidy) to refresh the lockfile and ensure build
consistency.
🪄 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: 31ace2d3-13fb-4413-a55e-f553c3674f79
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.mdgo.modpkg/models/licenses.gopkg/models/licenses_test.go
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- pkg/models/licenses_test.go
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
2 times, most recently
from
9d2a110 to
ad9916c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/models/licenses.go (2)
179-190:⚠️ Potential issue | 🟡 MinorNormalize
spdxIdonce before querying.Inputs like
" MIT "currently pass the empty check, get lowercased to" mit ", and miss an otherwise valid record. Since Line 186 already normalizes case in Go, the extraLOWER($1)in the SQL is redundant too.🔧 Suggested change
func (m *LicenseModel) GetSPDXLicenseDetails(ctx context.Context, spdxId string) (SPDXLicenseDetail, error) { s := ctxzap.Extract(ctx).Sugar() + spdxId = strings.TrimSpace(spdxId) if spdxId == "" { s.Error("Please specify a valid SPDX ID to query") return SPDXLicenseDetail{}, errors.New("please specify a valid SPDX ID to query") } s.Debugf("Getting SPDX License Details for %v", spdxId) spdxIdToLower := strings.ToLower(spdxId) var license SPDXLicenseDetail err := m.db.QueryRowxContext(ctx, "SELECT id, type, reference, isdeprecatedlicenseid, detailsurl, referencenumber, name, seealso, isosiapproved"+ - " FROM spdx_license_data WHERE LOWER(id) = LOWER($1)", spdxIdToLower).StructScan(&license) + " FROM spdx_license_data WHERE LOWER(id) = $1", spdxIdToLower).StructScan(&license)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 179 - 190, The GetSPDXLicenseDetails function currently only lowercases spdxId (spdxIdToLower) but doesn’t trim whitespace so inputs like " MIT " fail; normalize spdxId once by trimming and lowercasing (update the empty check to use the trimmed value), then pass that normalized value (spdxIdToLower or renamed normalizedID) to the DB query and remove the redundant LOWER($1) from the SQL WHERE clause so the DB compares the already-normalized id directly.
39-42:⚠️ Potential issue | 🟡 MinorFinish the remaining
SeeAlsoArraylint fixes.Lines 39, 42, 78, and 102 still fail
godot, and Line 105 still tripsnilnil. The Line 39 docstring is also narrower than the implementation now thatScanaccepts PostgreSQL array text as well as JSON.🔧 Suggested change
-// SeeAlsoArray represents an array of strings that can be stored as JSON in the database +// SeeAlsoArray represents a string slice that can be deserialized from PostgreSQL arrays or JSON. type SeeAlsoArray []string -// Scan implements the sql.Scanner interface for database deserialization +// Scan implements the sql.Scanner interface for database deserialization. func (s *SeeAlsoArray) Scan(value interface{}) error { -// parsePostgreSQLArray parses PostgreSQL array format like {val1,val2,val3} +// parsePostgreSQLArray parses PostgreSQL array format like {val1,val2,val3}. func (s *SeeAlsoArray) parsePostgreSQLArray(str string) error { -// Value implements the driver.Valuer interface for database serialization +// Value implements the driver.Valuer interface for database serialization. func (s SeeAlsoArray) Value() (driver.Value, error) { if s == nil { - return nil, nil + return nil, nil //nolint:nilnil // nil represents SQL NULL here }Also applies to: 78-78, 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 39 - 42, Update the SeeAlsoArray doc comment to a complete sentence that reflects current behavior (e.g., "SeeAlsoArray represents a slice of strings that can be stored as JSON or PostgreSQL text arrays in the database.") and ensure it starts with the type name and ends with a period; in the SeeAlsoArray.Scan method detect and parse both JSON input and PostgreSQL-style array text (call out Scan by name) and return proper wrapped errors (do not return ambiguous nils), and in SeeAlsoArray.Value return a typed nil for the driver.Value when there is no value (use driver.Value(nil), nil) to avoid the nilnil lint; keep error handling consistent and make sure SeeAlsoArray.Value and Scan signatures remain unchanged.go.mod (1)
25-29:⚠️ Potential issue | 🔴 CriticalApply the previously flagged security bumps before merge.
Lines 25 and 29 still pin the same
golang.org/x/netandgoogle.golang.org/grpcversions that were already called out earlier in the review thread, so this PR keeps those unresolved security findings in the module graph. Update them and regenerate the module graph before merging.🔧 Suggested change
- golang.org/x/net v0.50.0 // indirect + golang.org/x/net v0.51.0 // indirect ... - google.golang.org/grpc v1.79.1 // indirect + google.golang.org/grpc v1.80.0 // indirect#!/bin/bash # Confirm the current pins are still present in go.mod. sed -n '24,30p' go.mod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 25 - 29, go.mod still pins vulnerable versions of the modules golang.org/x/net and google.golang.org/grpc; update those module versions to the recommended secure releases (e.g., bump to the latest patched tags), then regenerate the module graph by running go get <module>@<version> for each updated module and run go mod tidy to update go.sum and the module graph; after changes, verify the pins are removed by inspecting the go.mod entries for golang.org/x/net and google.golang.org/grpc and running go list -m all to confirm no older vulnerable versions remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Around line 25-29: go.mod still pins vulnerable versions of the modules
golang.org/x/net and google.golang.org/grpc; update those module versions to the
recommended secure releases (e.g., bump to the latest patched tags), then
regenerate the module graph by running go get <module>@<version> for each
updated module and run go mod tidy to update go.sum and the module graph; after
changes, verify the pins are removed by inspecting the go.mod entries for
golang.org/x/net and google.golang.org/grpc and running go list -m all to
confirm no older vulnerable versions remain.
In `@pkg/models/licenses.go`:
- Around line 179-190: The GetSPDXLicenseDetails function currently only
lowercases spdxId (spdxIdToLower) but doesn’t trim whitespace so inputs like "
MIT " fail; normalize spdxId once by trimming and lowercasing (update the empty
check to use the trimmed value), then pass that normalized value (spdxIdToLower
or renamed normalizedID) to the DB query and remove the redundant LOWER($1) from
the SQL WHERE clause so the DB compares the already-normalized id directly.
- Around line 39-42: Update the SeeAlsoArray doc comment to a complete sentence
that reflects current behavior (e.g., "SeeAlsoArray represents a slice of
strings that can be stored as JSON or PostgreSQL text arrays in the database.")
and ensure it starts with the type name and ends with a period; in the
SeeAlsoArray.Scan method detect and parse both JSON input and PostgreSQL-style
array text (call out Scan by name) and return proper wrapped errors (do not
return ambiguous nils), and in SeeAlsoArray.Value return a typed nil for the
driver.Value when there is no value (use driver.Value(nil), nil) to avoid the
nilnil lint; keep error handling consistent and make sure SeeAlsoArray.Value and
Scan signatures remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0f45744-e500-45d7-bd6b-40bac9d1828a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.mdgo.modpkg/models/licenses.gopkg/models/licenses_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/models/licenses_test.go
- CHANGELOG.md
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
from
ad9916c to
c1e3e62
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go.mod (1)
25-25:⚠️ Potential issue | 🔴 CriticalUpgrade vulnerable modules before release (still unresolved).
Line 25 and Line 29 keep versions previously flagged for known vulnerabilities (
golang.org/x/net v0.50.0,google.golang.org/grpc v1.79.1). Please bump to patched versions (minimumx/net v0.51.0,grpc v1.79.3, preferably latest stable).Proposed go.mod fix
- golang.org/x/net v0.50.0 // indirect + golang.org/x/net v0.51.0 // indirect ... - google.golang.org/grpc v1.79.1 // indirect + google.golang.org/grpc v1.80.0 // indirect#!/bin/bash # Read-only verification: confirm current pinned versions and check OSV entries for those modules. set -euo pipefail echo "== Current pinned versions in go.mod ==" rg -n 'golang\.org/x/net|google\.golang\.org/grpc' go.mod echo "== OSV check for golang.org/x/net ==" curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' -d '{ "package": {"name":"golang.org/x/net","ecosystem":"Go"}, "version":"0.50.0" }' | jq '.vulns[]?.id' echo "== OSV check for google.golang.org/grpc ==" curl -s https://api.osv.dev/v1/query -H 'Content-Type: application/json' -d '{ "package": {"name":"google.golang.org/grpc","ecosystem":"Go"}, "version":"1.79.1" }' | jq '.vulns[]?.id'Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 25, The go.mod pins for vulnerable modules need to be upgraded: update the module entries for golang.org/x/net (currently v0.50.0) to at least v0.51.0 (prefer latest stable) and google.golang.org/grpc (currently v1.79.1) to at least v1.79.3 (prefer latest stable); after changing the versions in go.mod for the identifiers golang.org/x/net and google.golang.org/grpc, run the module tidy/resolve step (go get or go mod tidy) to refresh go.sum and verify no other dependent constraints block the bump and then run your test/build to confirm compatibility.
🧹 Nitpick comments (2)
pkg/models/licenses.go (2)
186-190: Remove redundantLOWER($1)sincespdxIdToLoweris already lowercase.The variable
spdxIdToLoweris already converted to lowercase on line 186, makingLOWER($1)in the query redundant.♻️ Proposed fix
spdxIdToLower := strings.ToLower(spdxId) var license SPDXLicenseDetail err := m.db.QueryRowxContext(ctx, "SELECT id, type, reference, isdeprecatedlicenseid, detailsurl, referencenumber, name, seealso, isosiapproved"+ - " FROM spdx_license_data WHERE LOWER(id) = LOWER($1)", spdxIdToLower).StructScan(&license) + " FROM spdx_license_data WHERE LOWER(id) = $1", spdxIdToLower).StructScan(&license)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 186 - 190, The query passes spdxIdToLower (already lowercase) but still uses LOWER($1), which is redundant; in the QueryRowxContext call inside the method that queries SPDXLicenseDetail (the block using variable spdxIdToLower and calling m.db.QueryRowxContext), replace LOWER($1) with $1 (or alternatively use WHERE LOWER(id) = $1) so you compare the lowercased DB id against spdxIdToLower without calling LOWER on the bound parameter.
198-210: Simplify the unnecessarysql.ErrNoRowscheck withSelectContext.
sqlx.SelectContextreturns an empty slice with a nil error when no rows match—it never returnssql.ErrNoRows. The!errors.Is(err, sql.ErrNoRows)condition is redundant and can be removed.♻️ Proposed simplification
// GetAllSPDXLicensesDetails get all spdx licenses details. func (m *LicenseModel) GetAllSPDXLicensesDetails(ctx context.Context) ([]SPDXLicenseDetail, error) { s := ctxzap.Extract(ctx).Sugar() var licenses []SPDXLicenseDetail err := m.db.SelectContext(ctx, &licenses, "SELECT id, type, reference, isdeprecatedlicenseid, detailsurl, referencenumber, name, seealso, isosiapproved"+ " FROM spdx_license_data") - if err != nil && !errors.Is(err, sql.ErrNoRows) { + if err != nil { s.Errorf("Error: Failed to query spdx_license_data table %#v", err) return []SPDXLicenseDetail{}, fmt.Errorf("failed to query the spdx_license_data table: %v", err) } return licenses, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/licenses.go` around lines 198 - 210, The error handling in GetAllSPDXLicensesDetails is checking for sql.ErrNoRows even though m.db.SelectContext (sqlx.SelectContext) returns an empty slice and nil error when no rows match; remove the redundant check (!errors.Is(err, sql.ErrNoRows)) and simplify the error branch to only handle a non-nil err from m.db.SelectContext, logging via s.Errorf and returning the formatted error, leaving the successful return of licenses intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go.mod`:
- Line 25: The go.mod pins for vulnerable modules need to be upgraded: update
the module entries for golang.org/x/net (currently v0.50.0) to at least v0.51.0
(prefer latest stable) and google.golang.org/grpc (currently v1.79.1) to at
least v1.79.3 (prefer latest stable); after changing the versions in go.mod for
the identifiers golang.org/x/net and google.golang.org/grpc, run the module
tidy/resolve step (go get or go mod tidy) to refresh go.sum and verify no other
dependent constraints block the bump and then run your test/build to confirm
compatibility.
---
Nitpick comments:
In `@pkg/models/licenses.go`:
- Around line 186-190: The query passes spdxIdToLower (already lowercase) but
still uses LOWER($1), which is redundant; in the QueryRowxContext call inside
the method that queries SPDXLicenseDetail (the block using variable
spdxIdToLower and calling m.db.QueryRowxContext), replace LOWER($1) with $1 (or
alternatively use WHERE LOWER(id) = $1) so you compare the lowercased DB id
against spdxIdToLower without calling LOWER on the bound parameter.
- Around line 198-210: The error handling in GetAllSPDXLicensesDetails is
checking for sql.ErrNoRows even though m.db.SelectContext (sqlx.SelectContext)
returns an empty slice and nil error when no rows match; remove the redundant
check (!errors.Is(err, sql.ErrNoRows)) and simplify the error branch to only
handle a non-nil err from m.db.SelectContext, logging via s.Errorf and returning
the formatted error, leaving the successful return of licenses intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6062e6a9-a942-4ac1-bad1-560b632bc75c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.mdgo.modpkg/models/licenses.gopkg/models/licenses_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/models/licenses_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
from
c1e3e62 to
fd54317
Compare
agustingroh
force-pushed
the
chore/implement-get-spdx-license-details-model
branch
from
fd54317 to
88fefca
Compare
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
2 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.
WHAT
Added
Summary by CodeRabbit
New Features
Tests
Documentation
Chores