chore:SP-3074 Implements 'GetSPDXLicenseDetails' model #2
Conversation
WalkthroughAdds SPDX license detail retrieval and SeeAlso JSON/array handling in models, introduces a SeeAlsoArray type with SQL scan/value support, and adds unit tests for Scan. Updates go.mod dependency structure, moving several deps to indirect, removing unused ones, and adding telemetry/testing-related indirect dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LM as LicenseModel
participant DB as Database
Caller->>LM: GetSPDXLicenseDetails(ctx, spdxId)
LM->>LM: Validate spdxId (non-empty, normalize case)
LM->>DB: SELECT ... WHERE id = spdxId
DB-->>LM: Row (includes seealso column)
note over LM: Scan seealso via SeeAlsoArray.Scan<br/>(JSON or PostgreSQL array)
LM-->>Caller: SPDXLicenseDetail or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.
WHAT
Added
Summary by CodeRabbit
New Features
Tests
Chores