Skip to content

Conversation

@agustingroh
Copy link
Contributor

@agustingroh agustingroh commented Jan 7, 2026

…ility use case

Summary by CodeRabbit

  • New Features

    • Added a configurable worker pool for local vulnerability scanning to improve concurrency and throughput.
    • Local and remote vulnerability lookups now run in parallel for faster results.
  • Bug Fixes / Reliability

    • Improved timeout and cancellation propagation so scans respect context cancellation and return partial results cleanly.
  • Documentation

    • CHANGELOG updated with the 0.8.0 entries.

✏️ Tip: You can customize this high-level summary in your review settings.

@agustingroh agustingroh self-assigned this Jan 7, 2026
@agustingroh agustingroh added the enhancement New feature or request label Jan 7, 2026
@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore:SP-3888 Implement multithreading support for the local vulnerab...' is truncated but clearly describes the main change: implementing multithreading support for the local vulnerability use case, which is the primary focus of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pkg/models/versions_test.go (1)

109-113: Remove unused connection acquisition.

A connection is acquired from the pool but never used in this test. Since VersionModel now works with *sqlx.DB directly, this connection acquisition and cleanup is unnecessary.

Proposed cleanup
 	defer CloseDB(db)
-	conn, err := db.Connx(ctx) // Get a connection from the pool
-	if err != nil {
-		t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
-	}
-	defer CloseConn(conn)
 	versionModel := NewVersionModel(ctx, db)
pkg/models/vulns_purl_test.go (1)

149-153: Test logic may no longer validate the intended behavior.

This test closes conn and then calls GetVulnsByPurlVersion, expecting an error. However, since VulnsForPurlModel now uses *sqlx.DB instead of *sqlx.Conn, closing the connection won't affect the model's ability to query. The test will likely fail or pass for the wrong reasons.

Consider either:

  1. Removing this test case if validating closed-connection behavior is no longer relevant
  2. Updating it to test a meaningful error scenario (e.g., closed db)
🤖 Fix all issues with AI agents
In @CHANGELOG.md:
- Line 15: Changelog lists the env var as VULN_SCANOSS_MAX_WORKERS but the
implementation uses VULN_SCANOSS_WORKERS; make them consistent by updating the
CHANGELOG entry to mention VULN_SCANOSS_WORKERS (or change the config symbol in
pkg/config/server_config.go from VULN_SCANOSS_WORKERS to
VULN_SCANOSS_MAX_WORKERS if you prefer the "MAX" naming), and ensure any
references in docs/tests match the chosen name so the environment variable
documented equals the one read by the code.

In @pkg/config/server_config.go:
- Around line 82-83: The MaxWorkers env tag in the struct is inconsistent with
the changelog: update the struct field tag for MaxWorkers from
`env:"VULN_SCANOSS_WORKERS"` to `env:"VULN_SCANOSS_MAX_WORKERS"` so the
environment variable name matches the documented `VULN_SCANOSS_MAX_WORKERS`;
alternatively, if you prefer the current variable name, update the CHANGELOG
entry to `VULN_SCANOSS_WORKERS` — adjust either the `MaxWorkers` env tag or the
changelog so both names match.

In @pkg/models/versions.go:
- Around line 33-34: VersionModel currently stores a ctx context.Context field
which is an anti-pattern; remove the ctx field from the VersionModel struct and
change methods that use it (e.g., GetVersionByName and saveVersion) to accept
ctx context.Context as their first parameter, then use that passed ctx in all DB
calls (replacing references to the struct field); finally update all call sites
to pass the appropriate context through the call chain so each operation can
provide its own context.
🧹 Nitpick comments (2)
pkg/config/server_config.go (1)

128-128: Consider adding validation for MaxWorkers.

The default value of 5 is reasonable, but there's no validation in IsValidConfig to ensure MaxWorkers is positive when SCANOSS source is enabled. A zero or negative value could cause runtime issues in the worker pool implementation.

Suggested validation

Add validation in the IsValidConfig function:

// Check SCANOSS source config
if cfg.Source.SCANOSS.Enabled {
	if cfg.Source.SCANOSS.MaxWorkers <= 0 {
		return errors.New("SCANOSS MaxWorkers must be greater than 0")
	}
}
pkg/usecase/vulnerability_use_case.go (1)

97-114: Use a local error variable within the goroutine to avoid capturing the outer err.

The goroutine at line 103 assigns to the outer err variable (declared at line 58), then copies it to localErr. While this works due to wg.Wait() synchronization, it's fragile and could lead to data races if future changes add concurrent writes to err.

♻️ Suggested refactor
 	var localVulnerabilities = dtos.VulnerabilityOutput{}
 	var localErr error
 	if us.config.Source.SCANOSS.Enabled {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			localVulUc := NewLocalVulnerabilitiesUseCase(ctx, us.s, us.config, us.db)
-			localVulnerabilities, err = localVulUc.GetVulnerabilities(ctx, components)
+			var err error
+			localVulnerabilities, err = localVulUc.GetVulnerabilities(ctx, components)
 			if err != nil {
 				us.s.Errorf("Failed to get Vulnerabilities: %v", err)
 				localErr = errors.New("problems encountered extracting vulnerability data")
 				return
 			}
 		}()
 	}
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1bb4b02 and 1e75d31.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • pkg/config/server_config.go
  • pkg/models/versions.go
  • pkg/models/versions_test.go
  • pkg/models/vulns_purl.go
  • pkg/models/vulns_purl_test.go
  • pkg/usecase/OSV_use_case.go
  • pkg/usecase/OSV_use_case_test.go
  • pkg/usecase/local_use_case.go
  • pkg/usecase/local_use_case_test.go
  • pkg/usecase/vulnerability_use_case.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/models/versions_test.go (1)
pkg/models/versions.go (1)
  • NewVersionModel (51-53)
pkg/usecase/OSV_use_case_test.go (1)
pkg/usecase/OSV_use_case.go (1)
  • OSVUseCase (47-53)
pkg/usecase/OSV_use_case.go (2)
pkg/dtos/component_dto.go (1)
  • ComponentDTO (19-23)
pkg/dtos/vulnerability_output.go (1)
  • VulnerabilityOutput (28-30)
pkg/usecase/local_use_case_test.go (2)
pkg/config/server_config.go (1)
  • NewServerConfig (89-103)
pkg/usecase/local_use_case.go (1)
  • NewLocalVulnerabilitiesUseCase (43-50)
pkg/usecase/vulnerability_use_case.go (3)
pkg/dtos/vulnerability_output.go (1)
  • VulnerabilityOutput (28-30)
pkg/usecase/OSV_use_case.go (1)
  • NewOSVUseCase (55-65)
pkg/usecase/local_use_case.go (1)
  • NewLocalVulnerabilitiesUseCase (43-50)
pkg/models/vulns_purl_test.go (2)
pkg/models/vulns_purl.go (1)
  • NewVulnsForPurlModel (51-53)
pkg/models/common.go (1)
  • LoadTestSQLData (49-65)
🔇 Additional comments (17)
pkg/models/versions.go (1)

51-52: Database handle migration looks correct.

The refactor from *sqlx.Conn to *sqlx.DB is appropriate for the multithreading use case. The *sqlx.DB type maintains a connection pool internally and is safe for concurrent use, which aligns well with the worker pool pattern being introduced in this PR.

pkg/models/versions_test.go (1)

41-41: Test updates align with model refactor.

The test correctly uses the new NewVersionModel(ctx, db) signature and passes nil for the connection parameter in loadTestSQLDataFiles, which is consistent with the DB handle refactor.

Also applies to: 45-45

CHANGELOG.md (1)

18-18: Clear documentation of multithreading changes.

The changelog entry accurately describes the refactoring work, mentioning both multithreading support and context cancellation handling, which aligns with the changes observed across the PR.

pkg/usecase/OSV_use_case_test.go (1)

64-64: Context propagation correctly implemented.

The test now passes ctx to Execute, which aligns with the context-aware refactoring described in the AI summary. This enables proper cancellation handling and deadline propagation through the OSV use case.

pkg/usecase/OSV_use_case.go (1)

84-114: LGTM! Context propagation and timeout handling are correctly implemented.

The changes properly accept and propagate context through the call chain. The 3-minute timeout is layered on top of the incoming context, which correctly allows external cancellation to still work while adding an internal deadline.

pkg/models/vulns_purl_test.go (2)

53-53: LGTM! Constructor call updated to use *sqlx.DB.

The test correctly passes the database handle to the updated constructor signature.


105-110: LGTM! Test setup simplified for DB-based model.

Passing nil for the connection parameter is consistent with the new pattern where models use *sqlx.DB directly and don't require a dedicated connection.

pkg/models/vulns_purl.go (3)

32-53: LGTM! Migration from *sqlx.Conn to *sqlx.DB is correct.

Using *sqlx.DB instead of a single connection enables connection pooling and is safe for concurrent access. The context is properly stored and passed to query methods.


85-93: LGTM! Query execution updated to use pooled database handle.


145-145: LGTM! Consistent update to use m.db.SelectContext.

pkg/usecase/vulnerability_use_case.go (1)

82-93: LGTM! OSV fetch runs concurrently with proper synchronization.

The WaitGroup correctly synchronizes the goroutine, and the shared osvVulnerabilities variable is only written by this single goroutine before being read after wg.Wait().

pkg/usecase/local_use_case_test.go (2)

42-43: LGTM! Context properly initialized with logger for request-scoped logging.

Using ctxzap.ToContext and extracting the sugared logger provides consistent logging context throughout the test.


58-74: LGTM! Test correctly updated for new constructor and method signatures.

The test properly loads server configuration and passes the required parameters to the updated NewLocalVulnerabilitiesUseCase constructor and GetVulnerabilities method.

pkg/usecase/local_use_case.go (4)

43-50: LGTM! Constructor properly wires dependencies.

The updated constructor correctly initializes the use case with logger, config, and database handle, and creates the required model instances.


55-98: Worker implementation is correct with proper cancellation handling.

The select-based worker pattern correctly handles context cancellation and channel closure. Error cases properly log and continue processing, which maintains the worker loop integrity.


66-70: Empty components produce empty output entries in the result.

When a component has an empty Purl, an empty VulnerabilityComponentOutput{} is sent to the results channel. This means the output slice will contain empty entries that callers may need to filter out. Consider whether this is the intended behavior, or if empty inputs should be excluded from results.


120-127: Output order is non-deterministic.

Results are collected in the order workers complete, not the order components were submitted. Since multiple workers process jobs concurrently, vulnOutputs[i] receives whichever result arrives next, not the result for components[i].

If order preservation is required, consider having workers return an index alongside the result, or process results into a map keyed by purl.

Copy link
Contributor

@isasmendiagus isasmendiagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@agustingroh agustingroh force-pushed the chore/SP-3888-refactor-local-vulnerability-usecase branch from 1e75d31 to 3e752be Compare January 7, 2026 14:43
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @pkg/models/versions.go:
- Around line 84-88: The INSERT call in QueryRowxContext inside
pkg/models/versions.go has a parameter count mismatch: the SQL uses two
placeholders ($1, $2) but the call passes four arguments (name, "", false,
false); update the call in the function that builds the version (the
QueryRowxContext(...).StructScan(&version) block) to either remove the extra
false, false arguments so only name and semver ("") are passed, or if additional
columns were intended, update the SQL to include matching placeholders and
column names; ensure the arguments and the INSERT column list/placeholders
align.

In @pkg/models/vulns_purl_test.go:
- Around line 149-153: The test currently closes a single connection via
CloseConn(conn) but the model method GetVulnsByPurlVersion uses the DB pool
(m.db), so closing one connection won't reliably cause an error; either change
the test to close the entire pool using CloseDB(db) before calling
GetVulnsByPurlVersion to assert error behavior when the DB is closed, or remove
the test case if simulating a closed DB is no longer relevant with pool-based
access—update references to CloseConn(conn) accordingly and keep the assertion
that expects an error only when you explicitly close the pool.

In @pkg/usecase/local_use_case.go:
- Around line 104-125: GetVulnerabilities can deadlock if
d.config.Source.SCANOSS.MaxWorkers is set to 0 (via VULN_SCANOSS_WORKERS)
because numWorkers becomes 0 and no vulnerabilityWorker goroutines are started;
add a guard so numWorkers = max(1, min(d.config.Source.SCANOSS.MaxWorkers,
numJobs)) or validate in IsValidConfig that when SCANOSS is enabled MaxWorkers
>= 1 (and return a config error otherwise), and update tests accordingly;
additionally decide and document behavior on context cancellation for
GetVulnerabilities—either return an empty VulnerabilityOutput on ctx.Done() or
explicitly document that partial vulnOutputs may be returned when ctx.Err() is
returned.
🧹 Nitpick comments (3)
pkg/models/versions_test.go (1)

109-113: Remove unused connection acquisition.

The test acquires a connection from the pool but never uses it since VersionModel now operates directly on the DB pool. This connection acquisition can be removed.

♻️ Suggested cleanup
 	defer CloseDB(db)
-	conn, err := db.Connx(ctx) // Get a connection from the pool
-	if err != nil {
-		t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
-	}
-	defer CloseConn(conn)
 	versionModel := NewVersionModel(db)
pkg/models/vulns_purl_test.go (1)

43-47: Remove unnecessary connection acquisition at these locations.

The loadSQLData function handles nil connections gracefully by falling back to db.Exec(). Line 105 in the same file successfully calls LoadTestSQLData(db, ctx, nil), so the connection acquisition at lines 43-47 and 132-136 is redundant. These can be simplified by passing nil consistently.

pkg/usecase/vulnerability_use_case.go (1)

82-93: Consider error handling for OSV goroutine.

The OSV use case is invoked concurrently, but unlike the local vulnerabilities goroutine (lines 97-109), there's no error capture. If OSVUseCase.Execute encounters an error (e.g., network issues, API errors), it would silently populate an empty or partial osvVulnerabilities result without signaling the issue.

Consider capturing errors from the OSV goroutine similar to the local vulnerabilities pattern, or at minimum document the decision to treat OSV failures as non-fatal.

💡 Proposed pattern for OSV error capture
 	wg := sync.WaitGroup{}
 	// Gets OSV vulnerabilities only if enabled
 	var osvVulnerabilities = dtos.VulnerabilityOutput{}
+	var osvErr error
 	if us.config.Source.OSV.Enabled {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			us.s.Debugf("vulnerabilities: OSV enabled")
 			osvUseCase := NewOSVUseCase(us.s, us.config)
 			osvVulnerabilities = osvUseCase.Execute(ctx, components)
+			// Note: OSVUseCase.Execute currently doesn't return an error
+			// If that changes, capture it here: osvErr = err
 		}()
 	}

Note: The current OSVUseCase.Execute signature doesn't return an error, so you'd need to update that first if error propagation is desired.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e75d31 and 3e752be.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • pkg/config/server_config.go
  • pkg/models/versions.go
  • pkg/models/versions_test.go
  • pkg/models/vulns_purl.go
  • pkg/models/vulns_purl_test.go
  • pkg/usecase/OSV_use_case.go
  • pkg/usecase/OSV_use_case_test.go
  • pkg/usecase/local_use_case.go
  • pkg/usecase/local_use_case_test.go
  • pkg/usecase/vulnerability_use_case.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/config/server_config.go
  • CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/usecase/local_use_case_test.go (2)
pkg/config/server_config.go (1)
  • NewServerConfig (89-103)
pkg/usecase/local_use_case.go (1)
  • NewLocalVulnerabilitiesUseCase (43-51)
pkg/usecase/OSV_use_case_test.go (1)
pkg/usecase/OSV_use_case.go (1)
  • OSVUseCase (47-53)
pkg/usecase/local_use_case.go (4)
pkg/models/vulns_purl.go (2)
  • VulnsForPurlModel (32-34)
  • NewVulnsForPurlModel (50-52)
pkg/config/server_config.go (1)
  • ServerConfig (32-86)
pkg/dtos/component_dto.go (1)
  • ComponentDTO (19-23)
pkg/dtos/vulnerability_output.go (1)
  • VulnerabilitiesOutput (59-70)
pkg/usecase/vulnerability_use_case.go (3)
pkg/dtos/vulnerability_output.go (1)
  • VulnerabilityOutput (28-30)
pkg/usecase/OSV_use_case.go (1)
  • NewOSVUseCase (55-65)
pkg/usecase/local_use_case.go (1)
  • NewLocalVulnerabilitiesUseCase (43-51)
pkg/usecase/OSV_use_case.go (2)
pkg/dtos/component_dto.go (1)
  • ComponentDTO (19-23)
pkg/dtos/vulnerability_output.go (1)
  • VulnerabilityOutput (28-30)
pkg/models/versions_test.go (1)
pkg/models/versions.go (1)
  • NewVersionModel (50-52)
pkg/models/vulns_purl_test.go (2)
pkg/models/vulns_purl.go (1)
  • NewVulnsForPurlModel (50-52)
pkg/models/common.go (2)
  • LoadTestSQLData (49-65)
  • CloseConn (90-98)
🔇 Additional comments (14)
pkg/models/versions.go (2)

33-33: Excellent refactor - addresses context anti-pattern and enables multithreading.

The change from sqlx.Conn to sqlx.DB and removing the stored context field is the correct approach for multithreading support. sqlx.DB is a thread-safe connection pool, while sqlx.Conn represents a single connection that shouldn't be shared across goroutines.

This properly addresses the past review comment about storing context in structs being an anti-pattern.

Also applies to: 50-51


55-55: Context propagation follows Go best practices.

Passing context as the first parameter to methods is the idiomatic Go pattern and enables proper request cancellation, tracing, and timeouts across concurrent operations.

Also applies to: 77-77

pkg/models/versions_test.go (1)

45-48: Test updates correctly align with refactored API.

All test calls properly use the new constructor signature and pass context to methods.

Also applies to: 59-59, 67-67, 76-76, 87-87

pkg/models/vulns_purl.go (2)

33-33: Consistent refactor enables thread-safe operations.

The change from sqlx.Conn to sqlx.DB mirrors the pattern in versions.go and enables concurrent vulnerability lookups using the thread-safe connection pool.

Also applies to: 50-51


55-55: Proper context propagation throughout the call chain.

All public methods correctly accept context as the first parameter and propagate it through delegated calls and database operations.

Also applies to: 76-76, 103-103

pkg/models/vulns_purl_test.go (1)

53-53: Test updates properly reflect the new API.

All test instantiations and method calls correctly use the new constructor signature and pass context to methods.

Also applies to: 78-78, 110-110, 112-112, 142-142, 144-144, 150-150

pkg/usecase/OSV_use_case_test.go (1)

64-64: LGTM! Context propagation correctly implemented.

The test properly passes the context to Execute, aligning with the updated method signature that enables timeout and cancellation support.

pkg/usecase/OSV_use_case.go (2)

84-86: LGTM! Context-aware API signature.

The updated Execute method correctly accepts a context and propagates it to processRequests, enabling caller-controlled timeouts and cancellation.


89-92: Verify timeout behavior with parent context deadlines.

The 3-minute timeout is derived via context.WithTimeout(ctx, 3*time.Minute). If the parent ctx already has a shorter deadline, the effective timeout will be the shorter of the two. However, if the parent has a longer deadline, this creates a new 3-minute timeout as expected.

Ensure this behavior aligns with your requirements—specifically, that parent context deadlines are respected and that 3 minutes is an appropriate default for OSV API queries.

pkg/usecase/vulnerability_use_case.go (2)

97-109: LGTM! Proper error handling in concurrent local vulnerability fetch.

The goroutine correctly captures errors in localErr and returns early after the wait if an error occurred. The defer wg.Done() ensures proper synchronization even on error paths.


111-114: LGTM! Proper synchronization and error propagation.

The wg.Wait() ensures both goroutines complete before proceeding, and the error check correctly propagates local vulnerability errors to the caller.

pkg/usecase/local_use_case_test.go (1)

42-43: LGTM! Test correctly updated for new constructor and method signatures.

The test properly:

  • Creates a context with logger (lines 42-43)
  • Loads server configuration (lines 58-61)
  • Invokes the updated constructor with ctx, logger, config, and DB (line 73)
  • Calls GetVulnerabilities with context (line 74)

These changes align with the new context-aware and configurable API.

Also applies to: 58-61, 73-74

pkg/usecase/local_use_case.go (2)

43-50: LGTM! Constructor properly wired for concurrent processing.

The constructor accepts context, logger, config, and DB, enabling the worker pool pattern with configurable MaxWorkers from the server config.


53-99: LGTM! Well-structured worker with proper context cancellation.

The worker correctly:

  • Respects context cancellation via select with ctx.Done() (line 59)
  • Handles empty PURLs gracefully (line 67)
  • Logs errors and continues processing (lines 79-80)
  • Sends results even on errors to prevent deadlock

Comment on lines +84 to 88
err := m.db.QueryRowxContext(ctx,
"INSERT INTO versions (version_name, semver) VALUES($1, $2)"+
" RETURNING id, version_name, semver",
name, "", false, false,
).StructScan(&version)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, read the actual file content around the specified lines
cat -n pkg/models/versions.go | sed -n '80,95p'

Repository: scanoss/vulnerabilities

Length of output: 795


🏁 Script executed:

#!/bin/bash
# Search for other INSERT statements into versions table to understand schema
rg -n "INSERT INTO versions" --type=go -B 2 -A 3

Repository: scanoss/vulnerabilities

Length of output: 434


🏁 Script executed:

#!/bin/bash
# Search for versions table schema definition (likely in migrations or schema files)
fd -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | head -20

Repository: scanoss/vulnerabilities

Length of output: 237


🏁 Script executed:

#!/bin/bash
# Search for Version struct definition
rg -n "type Version struct" --type=go -A 10

Repository: scanoss/vulnerabilities

Length of output: 575


🏁 Script executed:

#!/bin/bash
# Search for other methods that interact with versions table
rg -n "versions" pkg/models/versions.go | head -20

Repository: scanoss/vulnerabilities

Length of output: 783


Fix parameter count mismatch in SQL statement.

The INSERT statement has 2 placeholders ($1, $2) but the call passes 4 arguments. The SQL will reject the extra false, false parameters.

Fix
 	err := m.db.QueryRowxContext(ctx,
 		"INSERT INTO versions (version_name, semver) VALUES($1, $2)"+
 			" RETURNING id, version_name, semver",
-		name, "", false, false,
+		name, "",
 	).StructScan(&version)
📝 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
err := m.db.QueryRowxContext(ctx,
"INSERT INTO versions (version_name, semver) VALUES($1, $2)"+
" RETURNING id, version_name, semver",
name, "", false, false,
).StructScan(&version)
err := m.db.QueryRowxContext(ctx,
"INSERT INTO versions (version_name, semver) VALUES($1, $2)"+
" RETURNING id, version_name, semver",
name, "",
).StructScan(&version)
🤖 Prompt for AI Agents
In @pkg/models/versions.go around lines 84 - 88, The INSERT call in
QueryRowxContext inside pkg/models/versions.go has a parameter count mismatch:
the SQL uses two placeholders ($1, $2) but the call passes four arguments (name,
"", false, false); update the call in the function that builds the version (the
QueryRowxContext(...).StructScan(&version) block) to either remove the extra
false, false arguments so only name and semver ("") are passed, or if additional
columns were intended, update the SQL to include matching placeholders and
column names; ensure the arguments and the INSERT column list/placeholders
align.

Comment on lines 149 to 153
CloseConn(conn)
_, err = cpeModel.GetVulnsByPurlVersion("pkg:github/hapijs/call", "1.0.0")
_, err = cpeModel.GetVulnsByPurlVersion(ctx, "pkg:github/hapijs/call", "1.0.0")
if err == nil {
t.Errorf("Error was expected because purl is empty in cpeModel.GetVulnsByPurlVersion()")
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Intentional connection close tests error handling.

The test intentionally closes the connection at line 149 before calling GetVulnsByPurlVersion at line 150 to verify error handling. However, since the model now uses the DB pool (m.db) rather than a specific connection, closing a single connection from the pool won't necessarily cause the subsequent call to fail as intended.

The test logic may need adjustment since:

  • The model uses m.db (the connection pool), not a specific connection
  • Closing one connection from the pool doesn't prevent other calls from getting a new connection
  • This test might not actually verify what it intends to verify

Consider either:

  1. Closing the entire DB pool with CloseDB(db) if testing DB closure scenarios, or
  2. Removing this specific test case if it's no longer relevant with the pool-based approach
🤖 Prompt for AI Agents
In @pkg/models/vulns_purl_test.go around lines 149 - 153, The test currently
closes a single connection via CloseConn(conn) but the model method
GetVulnsByPurlVersion uses the DB pool (m.db), so closing one connection won't
reliably cause an error; either change the test to close the entire pool using
CloseDB(db) before calling GetVulnsByPurlVersion to assert error behavior when
the DB is closed, or remove the test case if simulating a closed DB is no longer
relevant with pool-based access—update references to CloseConn(conn) accordingly
and keep the assertion that expects an error only when you explicitly close the
pool.

@agustingroh agustingroh merged commit c60e434 into main Jan 7, 2026
3 checks passed
@agustingroh agustingroh deleted the chore/SP-3888-refactor-local-vulnerability-usecase branch January 7, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants