Mscasso/feat/sp 3838 update go.api to support the new scanning parameters #58
Conversation
📝 WalkthroughWalkthroughAdds new server-side scanning settings (ranking/snippet controls), introduces a ScanningServiceConfig type with defaulting and update-from-request logic (including base64 JSON header), and refactors scanning call sites to accept and propagate the new config and SBOM parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as "HTTP Server"
participant Service as "ScanningService"
participant Engine as "Scan Engine"
participant Storage as "SBOM / File Storage"
rect rgb(235,247,255)
Client->>HTTP: POST /scan/direct (body + optional header scanoss-settings)
HTTP->>Service: request + headers
note right of Service: getConfigFromRequest decodes\nbase64 JSON -> ScanningServiceConfig
end
rect rgb(245,255,235)
Service->>Service: DefaultScanningServiceConfig(server defaults)
Service->>Service: UpdateScanningServiceConfigDTO(JSON, legacy params)
alt SBOM provided
Service->>Storage: validate/create SBOM (config.sbomFile)
Storage-->>Service: sbomFile path
end
end
rect rgb(255,250,235)
Service->>Engine: start scan(s) with ScanningServiceConfig + sbomFile
Engine->>Engine: workerScan threads use config (flags, ranking, thresholds)
Engine-->>Service: scan results
end
Service->>HTTP: respond with results/status
HTTP->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 4
🧹 Nitpick comments (4)
tests/scanning_test.go (1)
136-222: Good test coverage for the new scan settings header feature.The test covers key scenarios: valid base64-encoded settings, invalid base64 handling, and integration with legacy flags. Consider adding a test case for malformed JSON (valid base64 but invalid JSON content) to verify that the server handles this gracefully and uses defaults.
🔎 Optional: Additional test case for malformed JSON
{ name: "Test Invalid ScanSettings - Malformed JSON", filename: "../pkg/service/tests/fingers.wfp", shortName: "fingers.wfp", // Base64 of "{invalid json}" scanSettingsB64: "e2ludmFsaWQganNvbn0=", extraFields: map[string]string{}, want: http.StatusOK, description: "Should handle malformed JSON gracefully and continue with defaults", },pkg/service/scanning_service_config.go (2)
74-74: Address the TODO or create an issue to track it.The TODO indicates a warning should be added when ranking settings are rejected due to
RankingAllowedbeing false. This would improve observability for API consumers.Would you like me to open an issue to track adding a warning log when ranking settings are rejected?
100-123: Simplify redundant empty string checks.The conditions
len(x) > 0 && x != ""are redundant since an empty string always has length 0. Either check is sufficient.🔎 Proposed fix
- if len(dbName) > 0 && dbName != "" { + if dbName != "" { currentConfig.dbName = dbName s.Debugf("Updated DbName to %s", currentConfig.dbName) } - if len(flags) > 0 && flags != "" { + if flags != "" { flagsInt, err := strconv.Atoi(flags) if err != nil { s.Errorf("Error converting flags to integer: %v", err) } else { currentConfig.flags = flagsInt s.Debugf("Updated Flags to %d", currentConfig.flags) } } - if len(scanType) > 0 && scanType != "" { + if scanType != "" { currentConfig.sbomType = scanType s.Debugf("Updated SbomType to %s", currentConfig.sbomType) } - if len(sbom) > 0 && sbom != "" { + if sbom != "" { currentConfig.sbomFile = sbom s.Debugf("Updated SbomFile to %s", currentConfig.sbomFile) }pkg/service/scanning_service.go (1)
93-95: Extract "identify" and "blacklist" as constants.Static analysis flagged that the string
"identify"appears 3 times. Extract these SBOM type strings as constants for maintainability.🔎 Proposed fix - Add constants at package level
+const ( + sbomTypeIdentify = "identify" + sbomTypeBlacklist = "blacklist" +) + // scanDirect handles WFP scanning requests from a client.Then update usages:
- if scanConfig.sbomType != "identify" && scanConfig.sbomType != "blacklist" { + if scanConfig.sbomType != sbomTypeIdentify && scanConfig.sbomType != sbomTypeBlacklist { ... - case "identify": + case sbomTypeIdentify: args = append(args, "-s") - case "blacklist": + case sbomTypeBlacklist: args = append(args, "-b") default: args = append(args, "-s") // Default to identifyAlso applies to: 414-421
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/config/server_config.gopkg/service/kb_details.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.gotests/scanning_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/service/kb_details.go (1)
pkg/service/scanning_service_config.go (1)
DefaultScanningServiceConfig(39-52)
pkg/service/scanning_service_config_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(37-93)pkg/service/scanning_service_config.go (3)
DefaultScanningServiceConfig(39-52)ScanningServiceConfig(26-37)UpdateScanningServiceConfigDTO(54-126)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(26-37)DefaultScanningServiceConfig(39-52)UpdateScanningServiceConfigDTO(54-126)
🪛 GitHub Check: build
pkg/config/server_config.go
[warning] 77-77:
comment-spacings: no space between comment delimiter and comment text (revive)
[warning] 73-73:
comment-spacings: no space between comment delimiter and comment text (revive)
[warning] 131-131:
comment-spacings: no space between comment delimiter and comment text (revive)
pkg/service/scanning_service.go
[failure] 94-94:
string identify has 3 occurrences, make it a constant (goconst)
[failure] 197-197:
elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
🔇 Additional comments (13)
pkg/service/kb_details.go (1)
88-89: LGTM!The refactor correctly uses
DefaultScanningServiceConfigto create a configuration object from server defaults before passing it toscanWfp. This aligns with the new unified configuration flow introduced across the scanning service.tests/scanning_test.go (2)
51-51: LGTM - Expected behavior change documented.The change from
http.StatusInternalServerErrortohttp.StatusOKreflects that the engine now handles invalid KB names gracefully with a fallback, as noted in the comment.
93-101: LGTM - Assets format updated to proper JSON structure.The assets field now uses proper JSON format with
componentsarray containing PURLs, which aligns with the expected SBOM structure.pkg/service/scanning_service_config_test.go (5)
27-64: LGTM - Comprehensive test for default config creation.The test properly validates that all fields from
ServerConfig.Scanningare correctly mapped toScanningServiceConfig.
66-123: LGTM - Good coverage for JSON-based settings updates.The test validates that all JSON settings fields are correctly parsed and applied when
rankingAllowedis true.
125-162: LGTM - Important test for the RankingAllowed guard.This test verifies the critical security/business logic that ranking settings are ignored when
RankingAllowedis false at the server level.
164-222: LGTM - Good coverage for legacy parameters and invalid input handling.Tests properly validate backward compatibility with legacy string parameters and graceful error handling for invalid inputs.
224-286: LGTM - Combined update test validates precedence.Good test ensuring both JSON settings and legacy parameters can be applied together in a single update.
pkg/service/scanning_service_config.go (2)
26-37: LGTM - Well-structured configuration type.The
ScanningServiceConfigstruct cleanly encapsulates all scanning parameters with appropriate types. Using unexported fields enforces controlled access through the provided functions.
39-52: LGTM - Clean mapping from server config to service config.
DefaultScanningServiceConfigcorrectly initializes all fields from the server configuration, providing sensible empty defaults forsbomTypeandsbomFile.pkg/service/scanning_service.go (3)
90-109: LGTM - Clean integration with new config flow.The scan direct handler now correctly uses
getConfigFromRequestto obtain configuration and accesses SBOM settings through the config object.
224-226: LGTM - Updated method signatures.The
singleScanmethod correctly accepts the newScanningServiceConfigparameter and passes it through toscanWfp.
425-443: LGTM - New scan arguments are properly conditionally added.The new ranking and snippet parameters are correctly added to the command arguments when their conditions are met. Good to log that
honourFileExtsis not yet implemented in the engine.
| // Scanning flags | ||
| if config.flags > 0 { | ||
| args = append(args, fmt.Sprintf("-F %v", config.flags)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other flags are formatted with args
echo "=== Searching for flag patterns with %v ==="
rg -n '\-[a-zA-Z].*%' --type=go pkg/service/
echo ""
echo "=== Searching for append with args and flags ==="
rg -n 'append.*args.*"-' --type=go pkg/service/ -A 1 -B 1
echo ""
echo "=== Show the specific lines in question ==="
sed -n '400,415p' pkg/service/scanning_service.goRepository: scanoss/api.go
Length of output: 5068
Change flag formatting from -F %v to -F%v for consistency.
The flag uses fmt.Sprintf("-F %v", config.flags) with a space, but similar single-character flags in the codebase consistently use no space: -n%s (line 404) and -r%d (line 427). This creates an inconsistent argument format. Change to -F%v to match the established pattern.
🤖 Prompt for AI Agents
In pkg/service/scanning_service.go around lines 407 to 410, the flag is
formatted with a space as fmt.Sprintf("-F %v", config.flags) which is
inconsistent with other single-character flags; change the format string to
"-F%v" so the argument is built without the space, keeping the same fmt.Sprintf
call and ensuring the resulting args append matches the established pattern.
pkg/service/scanning_service.go
Outdated
| if err != nil { | ||
| zs.Errorf("Error decoding scan settings from base64: %v", err) | ||
| decoded = nil | ||
| } else { |
There was a problem hiding this comment.
Should we use 'else if' instead the current else block?
| @@ -0,0 +1,126 @@ | |||
| // SPDX-License-Identifier: GPL-2.0-or-later | |||
| /* | |||
| * Copyright (C) 2018-2023 SCANOSS.COM | |||
There was a problem hiding this comment.
Should we change the date to 2025?
| @@ -0,0 +1,286 @@ | |||
| // SPDX-License-Identifier: GPL-2.0-or-later | |||
| /* | |||
| * Copyright (C) 2018-2023 SCANOSS.COM | |||
There was a problem hiding this comment.
Should we update the date to 2025?
agustingroh
left a comment
agustingroh
left a comment
There was a problem hiding this comment.
Please, add changes to CHANGELOG.md file
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/config/server_config.go (1)
131-138: Typo in comment on line 137.The comment for
MinSnippetLinessays "minimum snippet hits" but should say "minimum snippet lines".🔎 Proposed fix
- cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet hits on the file total lines + cfg.Scanning.MinSnippetLines = 0 // Lets the engine decide on minimum snippet lines based on the file total linespkg/service/scanning_service.go (1)
411-414: Flag format inconsistency.This was flagged in a previous review: the flag uses
"-F %v"with a space, but other single-character flags in this file use no space (e.g.,"-n%s"on line 408,"-r%d"on line 431).🔎 Proposed fix
- args = append(args, fmt.Sprintf("-F %v", config.flags)) + args = append(args, fmt.Sprintf("-F%v", config.flags))
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
108-131: Simplify redundant empty string checks.The checks
len(x) > 0 && x != ""are redundant since a non-empty string always has length > 0.🔎 Proposed fix
- if len(dbName) > 0 && dbName != "" { + if dbName != "" { currentConfig.dbName = dbName s.Debugf("Updated DbName to %s", currentConfig.dbName) } - if len(flags) > 0 && flags != "" { + if flags != "" { flagsInt, err := strconv.Atoi(flags) if err != nil { s.Errorf("Error converting flags to integer: %v", err) } else { currentConfig.flags = flagsInt s.Debugf("Updated Flags to %d", currentConfig.flags) } } - if len(scanType) > 0 && scanType != "" { + if scanType != "" { currentConfig.sbomType = scanType s.Debugf("Updated SbomType to %s", currentConfig.sbomType) } - if len(sbom) > 0 && sbom != "" { + if sbom != "" { currentConfig.sbomFile = sbom s.Debugf("Updated SbomFile to %s", currentConfig.sbomFile) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdpkg/config/server_config.gopkg/service/scanning_service.gopkg/service/scanning_service_config.gopkg/service/scanning_service_config_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-93)
pkg/service/scanning_service_config_test.go (2)
pkg/config/server_config.go (1)
ServerConfig(37-93)pkg/service/scanning_service_config.go (3)
DefaultScanningServiceConfig(39-52)ScanningServiceConfig(26-37)UpdateScanningServiceConfigDTO(54-134)
🔇 Additional comments (11)
pkg/config/server_config.go (1)
73-80: LGTM!The new scanning configuration fields are well-structured with appropriate environment variable tags and clear inline documentation.
pkg/service/scanning_service_config.go (3)
26-37: LGTM!The struct encapsulates all scanning configuration fields appropriately. Using unexported fields enforces controlled access through the provided functions.
39-52: LGTM!The default config factory correctly maps all server configuration fields to the service config.
54-73: Consider whether early return on JSON error is the desired behavior.When JSON unmarshalling fails, the function logs an error and returns the original config unchanged. This silently ignores potentially malformed input. Consider whether returning an error or setting a flag to indicate partial configuration would be more appropriate for debugging/observability.
pkg/service/scanning_service_config_test.go (3)
27-64: LGTM!Comprehensive test for default config creation, validating all fields are correctly mapped from server config.
125-162: LGTM!Good test ensuring the
RankingAllowedguard prevents ranking settings from being modified when not allowed.
197-222: LGTM!Important error handling tests that verify the config remains unchanged when given invalid input.
pkg/service/scanning_service.go (4)
40-43: LGTM!Good practice defining constants for SBOM types to avoid magic strings.
95-114: LGTM!Clean integration of the new config-based approach for SBOM validation and file handling.
164-209: LGTM!Well-structured function that consolidates config extraction from form values, headers, and base64-encoded settings. The else-if style issue from previous review has been addressed.
429-447: LGTM!Good handling of the new scanning parameters. The
honourFileExtslogging with a note about engine support is a pragmatic approach for future-proofing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/service/scanning_service.go (1)
411-414: Fix flag formatting inconsistency.The flag formatting uses
fmt.Sprintf("-F %v", config.flags)with a space, but other single-character flags in the same function consistently use no space:-n%s(line 408) and-r%d(line 431). Change to-F%vto maintain consistency.🔎 Proposed fix
// Scanning flags if config.flags > 0 { - args = append(args, fmt.Sprintf("-F %v", config.flags)) + args = append(args, fmt.Sprintf("-F%v", config.flags)) }
🧹 Nitpick comments (1)
pkg/service/scanning_service_config.go (1)
54-134: Consider simplifying the parameter/return pattern.The function takes a pointer parameter, modifies it in-place, and then returns the dereferenced value. Since callers use the return value (line 208 in scanning_service.go), the pointer modification is redundant. Consider either:
- Taking the config by value and returning the modified value (no pointer needed), OR
- Modifying the pointer and returning nothing (void)
The current pattern works but may confuse future maintainers about whether to use the modified pointer or the returned value.
🔎 Proposed refactor (pass by value)
-func UpdateScanningServiceConfigDTO(s *zap.SugaredLogger, currentConfig *ScanningServiceConfig, +func UpdateScanningServiceConfigDTO(s *zap.SugaredLogger, currentConfig ScanningServiceConfig, flags, scanType, sbom, dbName string, inputSettings []byte) ScanningServiceConfig { // ScanSettings represents the scanning parameters that can be configured type scanSettings struct { RankingEnabled *bool `json:"ranking_enabled,omitempty"` RankingThreshold *int `json:"ranking_threshold,omitempty"` MinSnippetHits *int `json:"min_snippet_hits,omitempty"` MinSnippetLines *int `json:"min_snippet_lines,omitempty"` HonourFileExts *bool `json:"honour_file_exts,omitempty"` } // Parse scan settings from JSON if provided var newSettings scanSettings if len(inputSettings) > 0 { err := json.Unmarshal(inputSettings, &newSettings) if err != nil { s.Errorf("Error unmarshalling scanning service config input: %v", err) - return *currentConfig + return currentConfig } } if newSettings.RankingEnabled != nil { if currentConfig.rankingAllowed { currentConfig.rankingEnabled = *newSettings.RankingEnabled s.Debugf("Updated RankingEnabled to %v", currentConfig.rankingEnabled) } else { s.Warnf("RankingEnabled setting ignored as RankingAllowed is false") } } // ... rest of the function remains the same ... - return *currentConfig + return currentConfig }And update the caller at line 208 in scanning_service.go:
- return UpdateScanningServiceConfigDTO(zs, &scanConfig, flags, scanType, sbom, dbName, decoded) + return UpdateScanningServiceConfigDTO(zs, scanConfig, flags, scanType, sbom, dbName, decoded)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdpkg/service/scanning_service.gopkg/service/scanning_service_config.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/service/scanning_service.go (2)
pkg/service/utils_service.go (1)
APIService(71-73)pkg/service/scanning_service_config.go (3)
ScanningServiceConfig(26-37)DefaultScanningServiceConfig(39-52)UpdateScanningServiceConfigDTO(54-134)
pkg/service/scanning_service_config.go (1)
pkg/config/server_config.go (1)
ServerConfig(37-93)
🔇 Additional comments (8)
CHANGELOG.md (1)
11-15: LGTM!The version entry is properly formatted with a recent release date and clear descriptions of the new scanning configuration features. The comparison link that was previously missing has been added at line 169.
pkg/service/scanning_service_config.go (1)
39-52: LGTM!The function correctly initializes default scanning configuration from server-wide settings, with appropriate defaults for SBOM fields.
pkg/service/scanning_service.go (6)
40-43: LGTM!The SBOM type constants are well-defined and improve maintainability by centralizing these string literals.
164-209: LGTM!The function cleanly extracts configuration from both form values and headers, with proper base64 decoding of the scan settings header. The fallback logic ensures flexibility in how clients can provide parameters.
95-115: LGTM!The config-driven approach is well-integrated here, with proper validation of SBOM types using the defined constants. The configuration flows correctly to the scanning functions.
228-228: LGTM!The function signatures have been consistently updated to accept the scanning configuration. Passing the config by value is appropriate since these functions only read from it.
Also applies to: 248-248, 340-340, 379-379
416-427: LGTM!The SBOM configuration logic correctly maps the SBOM type to the appropriate command-line flags using the defined constants, with a sensible default fallback.
444-447: Update the stale comment or remove the unsupported flag.The
--ignore-file-extflag is added to scan arguments whenhonourFileExtsis false (lines 445-446), but the comment states it's "not yet implemented in scanoss engine." The mock test engine does not recognize this flag, and no engine documentation confirms support. Either:
- Update the comment to reflect current engine support if now implemented, or
- Remove or gate this flag behind a feature flag until engine support is confirmed
Running scans with an unrecognized flag risks failures depending on the engine version in use.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.