fix: trigger folder scan when `.snyk` file changes [IDE-253] #563

cat2608 · 2024-06-26T14:18:17Z

Description

This PR triggers a workspace scan when the .snyk file was saved.

Checklist

Tests added and all succeed
Linted
README.md updated, if user-facing
License file updated, if new 3rd-party dependency is introduced

🚨After having merged, please update the CLI go.mod to pull in latest language server.

Screenshots / GIFs

auto-scan-snyj-ignore.mp4

application/server/server.go

bastiandoetsch · 2024-06-26T20:05:16Z

application/server/server.go

-		} else {
+
+		if isDotSnykFile(params.TextDocument.URI) {
+			dotSnykFileDidChangeScan(bgCtx, f)


I think this should be dependent on the auto-scan setting too. Although this scenario is right now not defined. I wonder though how we transmit the need to rescan to get new results. Maybe a notification? WDYT?

I wonder though how we transmit the need to rescan to get new results. Maybe a notification? WDYT?

Could you please elaborate a bit here? Are you saying we need to notify the user that after a local change to the .snyk file, the workspace will be re-scanned?

So far, I've been doing it in the background: when textDocumentDidSaveHandler the scan occurs.

From the customer perspective, if they have deactivated auto-scanning, they wouldn't notice a need to re-scan. On the other hand, they are the ones to update the .snyk file so it may be okay.

application/server/server.go

bastiandoetsch · 2024-06-26T20:07:34Z

application/server/server.go

 			} else {
 				logger.Warn().Msg("Not scanning, auto-scan is disabled")
 			}
+		} else if autoScanEnabled {


why is this dependent on the autoscan boolean?

This bit was just a small refactor to reduce one level of nesting. It's the first:

refactor: join condition checks for folder and auto-scan to reduce nesting

application/server/server_test.go

acke

Looks good to me.

bastiandoetsch · 2024-06-28T05:49:03Z

application/server/server.go

-		} else {
+		ws := workspace.Get()
+		if ws != nil && autoScanEnabled && uri.IsDotSnykFile(params.TextDocument.URI) {
+			go ws.ScanWorkspace(bgCtx)


Question: why not scan the workspaceFolder? Scanning the workspace could be too much - especially in Eclipse, corporate users often have 30+ projects (=workspace folders) open, and scanning all of them may slow down their workflow & computer. You could pull line 563 up, and then call f.Scan(). WDYT?

bastiandoetsch · 2024-06-28T05:50:55Z

application/server/server.go

 			if autoScanEnabled {
-				logger.Warn().Str("documentURI", filePath).Msg("Not scanning, file not part of workspace")
+				go f.ScanFile(bgCtx, filePath)


This may cause a double scan, right? First the scan above and afterwards the f.ScanFile. I'd think we can combine it :)

application/server/server_test.go

bastiandoetsch

Please have a look at my comment regarding double scans - else very nice 🚀

cat2608 · 2024-06-28T11:35:19Z

application/server/server.go


 		di.FileWatcher().SetFileAsChanged(params.TextDocument.URI)

+		filePath := uri.PathFromUri(params.TextDocument.URI)
+		autoScanEnabled := c.IsAutoScanEnabled()
+
+		f := workspace.Get().GetFolderContaining(filePath)
+
+		if f != nil && autoScanEnabled && uri.IsDotSnykFile(params.TextDocument.URI) {
+			go f.ScanFolder(ctx)
+			return nil, nil
+		}
+
 		for _, change := range params.ContentChanges {
 			if packageScanner, ok := di.Scanner().(snyk.PackageScanner); ok {
 				packageScanner.ScanPackages(ctx, c, uri.PathFromUri(params.TextDocument.URI), change.Text)


@bastiandoetsch I also added the trigger when a user runs the command snyk ignore --id='ISSUE_ID' --reason='REASON' in the terminal.

The Snyk ignore command triggers the textDocument/didChange

I need to commit the unit test for this branch and move this logic to a function so both handlers can use it

Oh yeah, probably when VSCode refreshes and the content changes, it is sending this.

Let me know, once you've pushed it, I'll re-review and merge.

For this case, I'm trying to make this test pass:

func Test_textDocumentDidChangeHandler_shouldTriggerScanForDotSnykFile(t *testing.T) { loc, jsonRPCRecorder := setupServer(t) config.CurrentConfig().SetSnykCodeEnabled(false) fakeAuthenticationProvider := di.AuthenticationService().Provider().(*snyk.FakeAuthenticationProvider) fakeAuthenticationProvider.IsAuthenticated = true _, err := loc.Client.Call(ctx, "initialize", nil) if err != nil { t.Fatalf("initialization failed: %v", err) } snykFilePath, folderPath := createTemporaryDirectoryWithSnykFile(t) // Simulate a "did change" event for the .snyk file sendFileChangedMessage(t, snykFilePath, folderPath, loc) // Wait for $/snyk.scan notification assert.Eventually( t, checkForSnykScan(t, jsonRPCRecorder), 15*time.Second, 50*time.Millisecond, ) } func sendFileChangedMessage(t *testing.T, filePath, fileDir string, loc server.Local) sglsp.DocumentURI { t.Helper() c := config.CurrentConfig() didChangeParams := sglsp.DidChangeTextDocumentParams{ TextDocument: sglsp.VersionedTextDocumentIdentifier{ TextDocumentIdentifier: sglsp.TextDocumentIdentifier{ URI: uri.PathToUri(filePath), }, Version: 1, }, ContentChanges: []sglsp.TextDocumentContentChangeEvent{ { Text: "SNYK-JS-QS-3153490", // simulates: `snyk ignore --id='SNYK-JS-QS-3153490'` }, }, } workspace.Get().AddFolder(workspace.NewFolder(c, fileDir, "Test", di.Scanner(), di.HoverService(), di.ScanNotifier(), di.Notifier())) _, err := loc.Client.Call(ctx, "textDocument/didChange", didChangeParams) if err != nil { t.Fatal(err) } return didChangeParams.TextDocument.URI }

I'm following a similar approach written for other cases but I wasn't able to solve this error yet:

024-06-28T16:07:55+01:00 INF - Fake Authentication - successful. 2024-06-28T16:07:55+01:00 DBG - IsAuthenticated: fake auth successful 2024-06-28T16:07:55+01:00 INF ide.workspace.folder.DelegatingConcurrentScanner.ScanFile - Scan was canceled err={} --- FAIL: Test_textDocumentDidChangeHandler_shouldTriggerScanForDotSnykFile (15.21s) server_test.go:1033: Error Trace: /Users/cata/git/snyk/hammerhead/snyk-ls/application/server/server_test.go:1033 Error: Condition never satisfied Test: Test_textDocumentDidChangeHandler_shouldTriggerScanForDotSnykFile FAIL

which is happening here: domain/snyk/scanner.go.

We could go for the textDocumentDidSaveHandler only and work on the textDocumentDidChangeHandler in another PR. WDYT?

@bastiandoetsch here is the initial change needed in IntelliJ snyk/snyk-intellij-plugin#562

bastiandoetsch · 2024-06-28T12:51:36Z

application/server/server_test.go

+	if err != nil {
+		t.Fatalf("initialization failed: %v", err)
+	}


same here - should use assert.NoError(t, err)

This is the case when there is an error and all the other blocks are using t.Fatal. In this case I'm only appending the error message.

…sting

cat2608 had a problem deploying to Standard June 26, 2024 14:18 — with GitHub Actions Failure

cat2608 had a problem deploying to MT1 June 26, 2024 14:18 — with GitHub Actions Failure

cat2608 had a problem deploying to MT3 June 26, 2024 14:18 — with GitHub Actions Error

cat2608 had a problem deploying to MT1 June 26, 2024 19:02 — with GitHub Actions Error

cat2608 had a problem deploying to Standard June 26, 2024 19:02 — with GitHub Actions Failure

cat2608 had a problem deploying to MT3 June 26, 2024 19:02 — with GitHub Actions Failure

cat2608 changed the title ~~fix: trigger folder scan when .snyk file changes~~ fix: trigger folder scan when .snyk file changes [IDE-253] Jun 26, 2024

cat2608 force-pushed the fix/IDE-253-scan-when-dot-snyk-file-changes branch from 75f9738 to cfd4baa Compare June 26, 2024 19:18

cat2608 had a problem deploying to MT1 June 26, 2024 19:18 — with GitHub Actions Error

cat2608 had a problem deploying to MT3 June 26, 2024 19:18 — with GitHub Actions Failure

cat2608 had a problem deploying to Standard June 26, 2024 19:18 — with GitHub Actions Failure