🐛 Ignore results outside of requested analysis scope (#162)

Additional duplicated results were accumulated in the UI. Improve input validation and logging in the partial analysis flow. Reference-Url: #156 --------- Signed-off-by: Radoslaw Szwajkowski <[email protected]>
konveyor · Dec 16, 2024 · c47bee7 · c47bee7
1 parent 622ba45
commit c47bee7
Show file tree

Hide file tree

Showing 10 changed files with 161 additions and 24 deletions.
diff --git a/vscode/src/analysis/__tests__/mergeRuleSets.test.ts b/vscode/src/analysis/__tests__/mergeRuleSets.test.ts
@@ -1,6 +1,6 @@
 import { RuleSet } from "@editor-extensions/shared";
 import { produce } from "immer";
-import { mergeRuleSets } from "../mergeRuleSets";
+import { mergeRuleSetsWithStringPaths as mergeRuleSets } from "../mergeRuleSets";
 import expect from "expect";
 import { FOO, BAR, DISCOVERY } from "./data";
 
@@ -74,3 +74,33 @@ describe("mergeRuleSets() adds new violations/incidents in the chosen files", ()
     expect(foo.violations?.["foo-01"].incidents[0].message).toBe("new message");
   });
 });
+
+describe("mergeRuleSets() workarounds", () => {
+  it("ignores files outside of included_paths received in analysis response", () => {
+    const responseWithUnknownFile = produce(FOO, (draft: RuleSet) => {
+      draft.violations!["foo-01"].incidents = [
+        {
+          uri: "file:///src/UnknownFile.java",
+          message: "new message",
+        },
+      ];
+    });
+    const merged: RuleSet[] = produce(BASE_STATE, (draft) =>
+      mergeRuleSets(draft, [responseWithUnknownFile], SAVED_FILES),
+    );
+    expect(merged).toHaveLength(3);
+    const [foo] = merged;
+
+    expect(foo.violations?.["foo-01"].incidents).toHaveLength(0);
+  });
+  it("takes the whole partial response if there is no base response in the state", () => {
+    const merged: RuleSet[] = produce([] as RuleSet[], (draft) =>
+      mergeRuleSets(draft, PARTIAL_SINGLE_INCIDENT, SAVED_FILES),
+    );
+    expect(merged).toHaveLength(1);
+    const [foo] = merged;
+
+    expect(foo.violations?.["foo-01"].incidents).toHaveLength(1);
+    expect(foo.violations?.["foo-01"].incidents[0].message).toBe("new message");
+  });
+});
diff --git a/vscode/src/analysis/mergeRuleSets.ts b/vscode/src/analysis/mergeRuleSets.ts
@@ -1,10 +1,28 @@
 import { RuleSet, Violation } from "@editor-extensions/shared";
-
+import { Immutable } from "immer";
+import { Uri } from "vscode";
 export const mergeRuleSets = (
+  draft: RuleSet[],
+  received: RuleSet[],
+  fileUris: Uri[],
+): RuleSet[] => {
+  // use the same path representation as in the response
+  // which is file:///some/path/File.java
+  const filePaths = fileUris.map((uri) => uri.toString());
+  return mergeRuleSetsWithStringPaths(draft, received, filePaths);
+};
+
+export const mergeRuleSetsWithStringPaths = (
   draft: RuleSet[],
   received: RuleSet[],
   filePaths: string[],
 ): RuleSet[] => {
+  if (draft.length === 0) {
+    // there were no full analysis yet or it's results were deleted
+    // nothing to merge - take the whole partial analysis response
+    draft.push(...received);
+    return draft;
+  }
   // remove old incidents in the files included in the partial analysis
   draft
     .flatMap((it) => [...Object.values(it.violations ?? {})])
@@ -21,7 +39,10 @@ export const mergeRuleSets = (
     ])
     .map(([name, violations]): [string, [string, Violation][]] => [
       name,
-      violations.filter(([, violation]) => violation.incidents?.length),
+      // reject incidents outside of requested scope
+      violations.filter(
+        ([, violation]) => violation.incidents?.filter((it) => filePaths.includes(it.uri))?.length,
+      ),
     ])
     .filter(([, violations]) => violations.length)
     // remaining violations contain incidents
@@ -57,3 +78,9 @@ export const mergeRuleSets = (
 
   return draft;
 };
+
+export const countIncidentsOnPaths = (ruleSets: Immutable<RuleSet[]>, filePaths: string[]) =>
+  ruleSets
+    .flatMap((r) => Object.values(r.violations ?? {}))
+    .flatMap((v) => v?.incidents ?? [])
+    .filter((it) => filePaths.includes(it.uri)).length;
diff --git a/vscode/src/analysis/runAnalysis.ts b/vscode/src/analysis/runAnalysis.ts
@@ -1,12 +1,14 @@
 import { getConfigAnalyzeOnSave } from "../utilities";
 import { ExtensionState } from "../extensionState";
-import { TextDocument, commands, window } from "vscode";
+import { TextDocument, Uri, commands, window } from "vscode";
 
 export const partialAnalysisTrigger = (textDoc: TextDocument) => {
-  commands.executeCommand("konveyor.partialAnalysis", [textDoc.fileName]);
+  if (textDoc.uri.scheme === "file") {
+    commands.executeCommand("konveyor.partialAnalysis", [textDoc.uri]);
+  }
 };
 
-export const runPartialAnalysis = async (state: ExtensionState, filePaths: string[]) => {
+export const runPartialAnalysis = async (state: ExtensionState, filePaths: Uri[]) => {
   if (!getConfigAnalyzeOnSave()) {
     return;
   }

diff --git a/vscode/src/client/analyzerClient.ts b/vscode/src/client/analyzerClient.ts
@@ -24,7 +24,11 @@ import {
   getConfigMaxIterations,
   getConfigMaxPriority,
   getConfigKaiDemoMode,
+  isAnalysisResponse,
 } from "../utilities";
+import { allIncidents } from "../issueView";
+import { Immutable } from "immer";
+import { countIncidentsOnPaths } from "../analysis";
 
 export class AnalyzerClient {
   private kaiRpcServer: ChildProcessWithoutNullStreams | null = null;
@@ -42,6 +46,7 @@ export class AnalyzerClient {
   constructor(
     private extContext: vscode.ExtensionContext,
     mutateExtensionData: (recipe: (draft: ExtensionData) => void) => void,
+    private getExtStateData: () => Immutable<ExtensionData>,
   ) {
     this.fireStateChange = (state: ServerState) =>
       mutateExtensionData((draft) => {
@@ -365,7 +370,7 @@ export class AnalyzerClient {
    *
    * Will only run if the sever state is: `running`
    */
-  public async runAnalysis(filePaths?: string[]): Promise<void> {
+  public async runAnalysis(filePaths?: vscode.Uri[]): Promise<void> {
     // TODO: Ensure serverState is running
 
     if (!this.rpcConnection) {
@@ -386,7 +391,7 @@ export class AnalyzerClient {
 
           const requestParams = {
             label_selector: getConfigLabelSelector(),
-            included_paths: filePaths,
+            included_paths: filePaths?.map((uri) => uri.fsPath),
           };
 
           this.outputChannel.appendLine(
@@ -407,7 +412,7 @@ export class AnalyzerClient {
             });
           });
 
-          const { response, isCancelled }: any = await Promise.race([
+          const { response: rawResponse, isCancelled }: any = await Promise.race([
             this.rpcConnection!.sendRequest("analysis_engine.Analyze", requestParams).then(
               (response) => ({ response }),
             ),
@@ -420,17 +425,47 @@ export class AnalyzerClient {
             this.fireAnalysisStateChange(false);
             return;
           }
-          this.outputChannel.appendLine(`Response: ${JSON.stringify(response)}`);
+          const isResponseWellFormed = isAnalysisResponse(rawResponse?.Rulesets);
+          const ruleSets: RuleSet[] = isResponseWellFormed ? rawResponse?.Rulesets : [];
+          const summary = isResponseWellFormed
+            ? {
+                wellFormed: true,
+                rawIncidentCount: ruleSets
+                  .flatMap((r) => Object.values(r.violations ?? {}))
+                  .flatMap((v) => v.incidents ?? []).length,
+                incidentCount: allIncidents(ruleSets).length,
+                partialAnalysis: filePaths
+                  ? {
+                      incidentsBefore: countIncidentsOnPaths(
+                        this.getExtStateData().ruleSets,
+                        filePaths.map((uri) => uri.toString()),
+                      ),
+                      incidentsAfter: countIncidentsOnPaths(
+                        ruleSets,
+                        filePaths.map((uri) => uri.toString()),
+                      ),
+                    }
+                  : {},
+              }
+            : { wellFormed: false };
+
+          this.outputChannel.appendLine(`Response received. Summary: ${JSON.stringify(summary)}`);
 
           // Handle the result
-          const rulesets = response?.Rulesets as RuleSet[];
-          if (!rulesets || rulesets.length === 0) {
+          if (!isResponseWellFormed) {
+            vscode.window.showErrorMessage(
+              "Analysis completed, but received results are not well formed.",
+            );
+            this.fireAnalysisStateChange(false);
+            return;
+          }
+          if (ruleSets.length === 0) {
             vscode.window.showInformationMessage("Analysis completed, but no RuleSets were found.");
             this.fireAnalysisStateChange(false);
             return;
           }
 
-          vscode.commands.executeCommand("konveyor.loadRuleSets", rulesets, filePaths);
+          vscode.commands.executeCommand("konveyor.loadRuleSets", ruleSets, filePaths);
           progress.report({ message: "Results processed!" });
           vscode.window.showInformationMessage("Analysis completed successfully!");
         } catch (err: any) {

diff --git a/vscode/src/commands.ts b/vscode/src/commands.ts
@@ -371,7 +371,8 @@ const commandsMap: (state: ExtensionState) => {
       // Update the user settings
       await updateLabelSelector(modifiedLabelSelector);
     },
-    "konveyor.loadRuleSets": async (ruleSets: RuleSet[]) => loadRuleSets(state, ruleSets),
+    "konveyor.loadRuleSets": async (ruleSets: RuleSet[], filePaths: Uri[]) =>
+      loadRuleSets(state, ruleSets, filePaths),
     "konveyor.cleanRuleSets": () => cleanRuleSets(state),
     "konveyor.loadStaticResults": loadStaticResults,
     "konveyor.loadResultsFromDataFolder": loadResultsFromDataFolder,
@@ -403,7 +404,7 @@ const commandsMap: (state: ExtensionState) => {
     "konveyor.diffView.applyBlockInline": applyBlock,
     "konveyor.diffView.applySelection": applyBlock,
     "konveyor.diffView.applySelectionInline": applyBlock,
-    "konveyor.partialAnalysis": async (filePaths: string[]) => runPartialAnalysis(state, filePaths),
+    "konveyor.partialAnalysis": async (filePaths: Uri[]) => runPartialAnalysis(state, filePaths),
     "konveyor.configureGetSolutionParams": async () => {
       const maxPriorityInput = await window.showInputBox({
         prompt: "Enter max_priority for getSolution",

diff --git a/vscode/src/data/analyzerResults.ts b/vscode/src/data/analyzerResults.ts
@@ -36,7 +36,7 @@ function getSeverityFromCategory(category: Category | undefined): vscode.Diagnos
 
 export const processIncidents = (
   ruleSets: Immutable<RuleSet[]>,
-): ReadonlyArray<[vscode.Uri, vscode.Diagnostic[] | undefined]> =>
+): ReadonlyArray<[vscode.Uri, vscode.Diagnostic[]]> =>
   ruleSets
     .flatMap((ruleSet) => Object.values(ruleSet.violations ?? {}))
     .flatMap((violation): [vscode.DiagnosticSeverity, Incident][] => {

diff --git a/vscode/src/data/loadResults.ts b/vscode/src/data/loadResults.ts
@@ -9,7 +9,7 @@ import { processIncidents } from "./analyzerResults";
 import { ExtensionState } from "src/extensionState";
 import { deleteAllDataFilesByPrefix, writeDataFile } from "./storage";
 import { toLocalChanges, writeSolutionsToMemFs } from "./virtualStorage";
-import { window } from "vscode";
+import { Diagnostic, Uri, window } from "vscode";
 import {
   KONVEYOR_SCHEME,
   MERGED_RULE_SET_DATA_FILE_PREFIX,
@@ -20,10 +20,28 @@ import {
 import { castDraft, Immutable } from "immer";
 import { mergeRuleSets } from "../analysis";
 
+const diagnosticsForPaths = (
+  filePaths: Uri[],
+  diagnostics: Immutable<[Uri, Diagnostic[]][]>,
+): [Uri, Diagnostic[]][] => {
+  const paths = new Set(filePaths.map((it) => it.toString()));
+  return diagnostics.filter(([uri]) => paths.has(uri.toString())) as [Uri, Diagnostic[]][];
+};
+
+const countDiagnosticPerPath = (diagnostics: [Uri, readonly Diagnostic[] | undefined][]) =>
+  diagnostics.reduce(
+    (acc, [uri, items]) => {
+      const path = uri.toString();
+      acc[path] = (acc[path] ?? 0) + (items?.length ?? 0);
+      return acc;
+    },
+    {} as { [key: string]: number },
+  );
+
 export const loadRuleSets = async (
   state: ExtensionState,
   receivedRuleSets: RuleSet[],
-  filePaths?: string[],
+  filePaths?: Uri[],
 ) => {
   const isPartial = !!filePaths;
   if (isPartial) {
@@ -42,10 +60,28 @@ export const loadRuleSets = async (
       ? mergeRuleSets(draft.ruleSets, receivedRuleSets, filePaths)
       : receivedRuleSets;
   });
+  const diagnosticTuples = processIncidents(data.ruleSets);
   if (isPartial) {
     await writeDataFile(data.ruleSets, MERGED_RULE_SET_DATA_FILE_PREFIX);
+    console.log(
+      "Diagnostics on the selected paths before update",
+      countDiagnosticPerPath(filePaths.map((uri) => [uri, state.diagnosticCollection.get(uri)])),
+    );
+    const scopedChange = [
+      // remove existing markers by passing undefined first
+      ...filePaths.map((uri): [Uri, undefined] => [uri, undefined]),
+      // set new values
+      ...diagnosticsForPaths(filePaths, diagnosticTuples),
+    ];
+    state.diagnosticCollection.set(scopedChange);
+    console.log(
+      "Diagnostics on the selected paths after update",
+      countDiagnosticPerPath(filePaths.map((uri) => [uri, state.diagnosticCollection.get(uri)])),
+    );
+  } else {
+    state.diagnosticCollection.clear();
+    state.diagnosticCollection.set(diagnosticTuples);
   }
-  state.diagnosticCollection.set(processIncidents(data.ruleSets));
 };
 
 export const cleanRuleSets = (state: ExtensionState) => {

diff --git a/vscode/src/diffView/register.ts b/vscode/src/diffView/register.ts
@@ -60,11 +60,11 @@ export function registerDiffView({
         .map((change, index): [LocalChange, number] => [change, index])
         .filter(([change, index]) => hasChanged(change, index))
         .filter(([{ state }]) => state === "applied" || state === "discarded")
-        .map(([change, index]): [LocalChange, number, vscode.Uri[], string] => [
+        .map(([change, index]): [LocalChange, number, vscode.Uri[], vscode.Uri | undefined] => [
           change,
           index,
           copyFromTo(change),
-          change.state === "applied" ? change.originalUri.fsPath : "",
+          change.state === "applied" ? change.originalUri : undefined,
         ])
         .map(([change, index, [fromUri, toUri], filePath]) =>
           vscode.workspace.fs.copy(fromUri, toUri, { overwrite: true }).then(() => {
@@ -74,7 +74,7 @@ export function registerDiffView({
         ),
     );
 
-    const appliedPaths = allModifiedPaths.filter(Boolean);
+    const appliedPaths = allModifiedPaths.filter(Boolean).filter((uri) => uri?.scheme === "file");
     if (appliedPaths.length && getConfigAnalyzeOnSave()) {
       return vscode.commands.executeCommand("konveyor.partialAnalysis", appliedPaths);
     }

diff --git a/vscode/src/extension.ts b/vscode/src/extension.ts
@@ -45,7 +45,7 @@ class VsCodeExtension {
     };
 
     this.state = {
-      analyzerClient: new AnalyzerClient(context, mutateData),
+      analyzerClient: new AnalyzerClient(context, mutateData, getData),
       webviewProviders: new Map<string, KonveyorGUIWebviewViewProvider>(),
       extensionContext: context,
       diagnosticCollection: vscode.languages.createDiagnosticCollection("konveyor"),

diff --git a/vscode/src/utilities/typeGuards.ts b/vscode/src/utilities/typeGuards.ts
@@ -38,15 +38,21 @@ export function isAnalysis(obj: unknown): obj is RuleSet {
     skipped: "object",
   };
 
+  const knownKeysAsString = knownKeys as Record<string, string>;
+
   return (
     isObject(obj) &&
     !isEmpty(obj) &&
     Object.entries(obj).every(
-      ([key, value]) => typeof value === (knownKeys as Record<string, string>)[key],
+      ([key, value]) => !knownKeysAsString[key] || typeof value === knownKeysAsString[key],
     )
   );
 }
 
+export function isAnalysisResponse(obj: unknown[]): obj is RuleSet[] {
+  return Array.isArray(obj) && obj.every((item) => isAnalysis(item));
+}
+
 export function isUri(obj: unknown): obj is Uri {
   if (!isObject(obj)) {
     return false;