SLCORE-2387 SLCORE-2394 SLCORE-2395 Trigger local SCA analysis#2088
SLCORE-2387 SLCORE-2394 SLCORE-2395 Trigger local SCA analysis#2088eray-felek-sonarsource wants to merge 4 commits into
Conversation
Agentic Analysis: Early ResultsAgentic Analysis and Context Augmentation are available on your project. Here are some issues that could have been prevented. Follow the links to learn how to put them into action. 3 issue(s) found across 2 file(s):
Analyzed by SonarQube Agentic Analysis in 9.1 s |
279d877 to
1162042
Compare
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
1162042 to
54ffef8
Compare
f500748 to
54ffef8
Compare
5728432 to
1e81ccc
Compare
1e81ccc to
d6c8610
Compare
The 2 remaining issues are addressed, I think this is a false positive |
d6c8610 to
2e60061
Compare
2e60061 to
d69d83c
Compare
| import static org.assertj.core.api.Assertions.catchThrowable; | ||
|
|
||
| @ExtendWith(SystemStubsExtension.class) | ||
| class AnalyzeDependencyRiskProjectMediumTests { |
There was a problem hiding this comment.
I think the medium tests are very thin, they do not test much
There was a problem hiding this comment.
This was also an issue on CLI implementation I think.
We would need real downloading of their CLI to add more meaningful tests which should be covered by this ticket.
WDYT? Should we still add more tests in the scope of this PR? If so please specify which paths you'd like to see covered
There was a problem hiding this comment.
Real downloading would likely be an integration test, here it's a medium test. We could mock interactions with CLI via the harness, similarly to what we do with withSonarQubeConnection
There was a problem hiding this comment.
Just to clarify the technical constraint here: the CLI interaction doesn't actually happen in our code. It is entirely encapsulated within the sca-scanner library. The library handles downloading the CLI, spawning it as a separate process, and reading its output. Because only the initial download is an HTTP call, the test harness doesn't have an interception point for the actual execution.
This is why we can't really compare this to withSonarQubeConnection, which works by returning canned responses to HTTP calls made directly by our backend. Stubbing the download step wouldn't solve this either—whatever we serve still gets executed as a real program. Serving anything other than a working executable just reproduces the failure case we already have covered.
To actually test the happy path in a medium test, we would have to choose between two approaches:
Option 1: Provide a fake CLI executable
We could provide a small fake CLI program that prints canned output and let it run normally (via the existing cliOverrideLocation option).
Trade-off: Because it’s a real executable, it is OS-specific (we'd need to handle Windows vs. Linux). This makes it feel much more like an integration test.
Option 2: Inject a fake scanner
We could change our backend dependency injection so tests can replace the real scanner with a fake one that simply returns canned results.
Trade-off: While this avoids the OS-specific execution problem, it requires adding test-only code to the production backend and entirely skips the download, run, and parse steps.
There was a problem hiding this comment.
Do you think we should go with one of these options or should we continue with my initial suggestion?
There was a problem hiding this comment.
I think we could have a trade off solution with a very lightweight executable that fakes SCA CLI and creates a fake .tar.gz. It could be used with a system property to override the location of the CLI. This way, we can stub SCA analysis APIs and have a medium tests that can simulate the whole end to end chain.
02e7e5a to
0970d09
Compare
0970d09 to
d4e1050
Compare
d4e1050 to
d5f34a3
Compare
| * </p> | ||
| */ | ||
| public class AnalyzeDependencyRiskProjectResponse { | ||
| private final List<DependencyRiskDto> dependencyRisks; |
There was a problem hiding this comment.
The current pattern for SLCore based analysis triggering is that explicit analyze methods DON'T return issues. If in the future we implement automatic analysis, then we'll have to change this response to fit that pattern. Should we already do that?
There was a problem hiding this comment.
Same as discussed below
| @@ -33,6 +33,13 @@ public interface DependencyRiskRpcService { | |||
| @JsonRequest | |||
| CompletableFuture<ListAllDependencyRisksResponse> listAll(ListAllParams params); | |||
There was a problem hiding this comment.
local scan results are not integrated into this method. What is the end vision for how the frontends would use/combine these methods?
There was a problem hiding this comment.
Same goes for the client notification didChangeDependencyRisks
There was a problem hiding this comment.
As discussed
We first call analyze project locally
Keep things in cache
After analysis is finished we stream withdidChangeDependencyRisksfollowing the general patterns
There was a problem hiding this comment.
@georgii-borovinskikh-sonarsource following up on our discussion here, since the local analysis can yield different results depending on the branch we are on, I planning to store the analysis results in cache based on configurationScopeId + branchId, WDYT?
| this.dependencyDetails = dependencyDetails; | ||
| } | ||
|
|
||
| public ReleaseDetailsDto getReleaseDetails() { |
There was a problem hiding this comment.
What will this be used for on the frontend?
There was a problem hiding this comment.
Currently we are not cutting any information that we return to frontend. Do you think this information would never be used? We can let the frontend decide if it's useful or not.
There was a problem hiding this comment.
Not sure if we should investigate all the information we return to frontends are useful or not at this stage
There was a problem hiding this comment.
We decided we can do this if we have extra time but not essential
There was a problem hiding this comment.
Does that mean we don't know what will be implemented on client side?
| public class LocalDependencyRiskAnalysisCache { | ||
| private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>(); | ||
|
|
||
| public Optional<AnalyzeProjectResponse> get(String configurationScopeId) { | ||
| return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId)); | ||
| } | ||
|
|
||
| public void put(String configurationScopeId, AnalyzeProjectResponse analysis) { | ||
| analysesByConfigurationScopeId.put(configurationScopeId, analysis); | ||
| } | ||
| } |
There was a problem hiding this comment.
⚠️ Performance: LocalDependencyRiskAnalysisCache never evicts entries on scope removal
The LocalDependencyRiskAnalysisCache stores AnalyzeProjectResponse objects keyed by configuration scope ID in a ConcurrentHashMap that only grows. When a configuration scope is removed (unbound, closed), the entry is never evicted. Other services in the codebase (e.g., ClientFileSystemService) listen for ConfigurationScopeRemovedEvent to clean up their per-scope state. Without similar cleanup, this cache will leak memory proportional to the number of scopes analyzed over the IDE session lifetime.
Add a remove method, then add a @EventListener for ConfigurationScopeRemovedEvent in DependencyRiskService (or create a dedicated listener) that calls localDependencyRiskAnalysisCache.remove(event.getRemovedConfigurationScopeId()).:
public class LocalDependencyRiskAnalysisCache {
private final ConcurrentHashMap<String, AnalyzeProjectResponse> analysesByConfigurationScopeId = new ConcurrentHashMap<>();
public Optional<AnalyzeProjectResponse> get(String configurationScopeId) {
return Optional.ofNullable(analysesByConfigurationScopeId.get(configurationScopeId));
}
public void put(String configurationScopeId, AnalyzeProjectResponse analysis) {
analysesByConfigurationScopeId.put(configurationScopeId, analysis);
}
public void remove(String configurationScopeId) {
analysesByConfigurationScopeId.remove(configurationScopeId);
}
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
0cd1958 to
137d4a2
Compare
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
nquinquenel
left a comment
There was a problem hiding this comment.
You'll need to update API_CHANGES as well with all the new APIs
| var delta = dependencyRiskMerger.computeDelta(newRisks); | ||
| if (!delta.addedRisks().isEmpty() || !delta.updatedRisks().isEmpty()) { | ||
| client.didChangeDependencyRisks(new DidChangeDependencyRisksParams(configurationScopeId, Set.of(), delta.addedRisks(), delta.updatedRisks())); | ||
| } |
There was a problem hiding this comment.
I'm not sure this work as expected, local analysis should compare previous merged snapshot vs new merged snapshot (by id where present, and a stable local key otherwise), not filter by Presence on the new list only
|
|
||
|
|
||
|
|
||
|
|
||
|
|
| public AnalyzeDependencyRiskProjectResponse analyzeProject(AnalyzeDependencyRiskProjectParams params, SonarLintCancelMonitor cancelMonitor) { | ||
| var configurationScopeId = params.getConfigurationScopeId(); | ||
| var context = contextResolver.resolve(configurationScopeId); | ||
| var localResponse = runLocalAnalysis(configurationScopeId, context); | ||
| var mergedRisks = dependencyRiskService.updateLocalAnalysisAndNotify(configurationScopeId, localResponse, cancelMonitor); | ||
| var errors = localResponse.errors().stream().map(dependencyRiskDtoMapper::toErrorDto).toList(); | ||
| return new AnalyzeDependencyRiskProjectResponse(mergedRisks, localResponse.parsedFiles(), errors); | ||
| } |
There was a problem hiding this comment.
I think we may want to check if SCA is supported there
| var downloadBaseUrl = StringUtils.firstNonBlank(System.getProperty(SCA_DOWNLOAD_BASE_URL_PROPERTY), toScaDownloadBaseUrl(endpointParams)); | ||
| if (downloadBaseUrl == null) { | ||
| throw invalidArgument("Missing SCA scanner download base URL. Provide it in the request or with system property '" + SCA_DOWNLOAD_BASE_URL_PROPERTY + "'"); | ||
| } |
There was a problem hiding this comment.
I think the message is misleading, you can't provide the download URL via the request




Summary by Gitar
ScaProjectAnalysisServicefor orchestrating on-demand SCA analysis.ScaAnalysisContextResolverto consolidate analysis configuration (binding, credentials, endpoint, properties).LocalDependencyRiskAnalysisCacheto store and merge local scanner findings.DependencyRiskMergerandDependencyRiskDtoMapperto reconcile and enrich server-tracked risks with local analysis results.analyzeProjectmethod toDependencyRiskRpcServicefor manual trigger of local analysis.ScaProjectAnalysisServiceTestsandDependencyRiskServiceTests.AnalyzeDependencyRiskProjectMediumTests.This will update automatically on new commits.