CHANGE: @W-19053527@: Handle new ApexGuru responses (#273)

Author: stephen-carter-at-sf

src/lib/apexguru/apex-guru-run-action.ts

@@ -55,9 +55,9 @@ export class ApexGuruRunAction {
                 this.telemetryService.sendCommandEvent(Constants.TELEM_SUCCESSFUL_APEX_GURU_FILE_ANALYSIS, {
                     executedCommand: commandName,
                     duration: (Date.now() - startTime).toString(),
-                    violationCount: violations.length.toString(),
-                    // TODO: This telemetry might change in the near future so that we can track the number of suggestions and fixes
-                    violationsWithSuggestedCodeCount: violations.filter(v => v.suggestions?.length > 0).length.toString()
+                    numViolations: violations.length.toString(),
+                    numViolationsWithSuggestions: violations.filter(v => v.suggestions?.length > 0).length.toString(),
+                    numViolationsWithFixes: violations.filter(v => v.fixes?.length > 0).length.toString()
                 });
             } catch (err) {
                 this.display.displayError(messages.error.analysisFailedGenerator(getErrorMessage(err)));

src/lib/apexguru/apex-guru-service.ts

@@ -5,15 +5,16 @@
  * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 
-import * as Constants from '../constants';
-import {CodeLocation, Violation} from '../diagnostics';
+import {CodeLocation, Fix, Suggestion, Violation} from '../diagnostics';
 import {Logger} from "../logger";
 import {getErrorMessage, indent} from '../utils';
 import {HttpMethods, HttpRequest, OrgConnectionService} from '../external-services/org-connection-service';
 import {FileHandler} from '../fs-utils';
 import { messages } from '../messages';
 
 export const APEX_GURU_ENGINE_NAME: string = 'apexguru';
+const APEX_GURU_MAX_TIMEOUT_SECONDS = 60;
+const APEX_GURU_RETRY_INTERVAL_MILLIS = 1000;
 
 const RESPONSE_STATUS = {
     NEW: "new",
@@ -37,8 +38,8 @@ export class LiveApexGuruService implements ApexGuruService {
                 orgConnectionService: OrgConnectionService,
                 fileHandler: FileHandler,
                 logger: Logger,
-                maxTimeoutSeconds: number = Constants.APEX_GURU_MAX_TIMEOUT_SECONDS,
-                retryIntervalMillis: number = Constants.APEX_GURU_RETRY_INTERVAL_MILLIS) {
+                maxTimeoutSeconds: number = APEX_GURU_MAX_TIMEOUT_SECONDS,
+                retryIntervalMillis: number = APEX_GURU_RETRY_INTERVAL_MILLIS) {
         this.orgConnectionService = orgConnectionService;
         this.fileHandler = fileHandler;
         this.logger = logger;
@@ -50,7 +51,7 @@ export class LiveApexGuruService implements ApexGuruService {
         if (!this.orgConnectionService.isAuthed()) {
             return false;
         }
-        const response: ApexGuruResponse = await this.request('GET', Constants.APEX_GURU_VALIDATE_ENDPOINT);
+        const response: ApexGuruResponse = await this.request('GET', await this.getValidateEndpoint());
         return response.status === RESPONSE_STATUS.SUCCESS;
     }
 
@@ -59,16 +60,22 @@ export class LiveApexGuruService implements ApexGuruService {
         const requestId = await this.initiateRequest(fileContent);
         this.logger.debug(`Initialized ApexGuru Analysis with Request Id: ${requestId}`);
         const queryResponse: ApexGuruQueryResponse = await this.waitForResponse(requestId);
-        this.logger.debug(`ApexGuru Analysis completed for Request Id: ${requestId}`);
-        const reports: ApexGuruReport[] = toReportArray(queryResponse);
-        return reports.map(r => toViolation(r, absFileToScan));
+        const payloadStr: string = decodeFromBase64(queryResponse.report);
+        this.logger.debug(`ApexGuru Analysis completed for Request Id: ${requestId}\n\nDecoded Response Payload:\n${payloadStr}`);
+        const apexGuruViolations: ApexGuruViolation[] = parsePayload(payloadStr);
+        return apexGuruViolations.map(v => toViolation(v, absFileToScan));
     }
 
     private async initiateRequest(fileContent: string): Promise<string> {
-        const base64EncodedContent = Buffer.from(fileContent).toString('base64');
-        const response: ApexGuruInitialResponse = await this.request('POST', Constants.APEX_GURU_REQUEST_ENDPOINT,
-            JSON.stringify({classContent: base64EncodedContent}));
-        if (!response.requestId || response.status != RESPONSE_STATUS.NEW) {
+        const requestBody: ApexGuruRequestBody = {
+            classContent: encodeToBase64(fileContent)
+        };
+        const response: ApexGuruInitialResponse = await this.request('POST', await this.getRequestEndpoint(),
+            JSON.stringify(requestBody));
+        
+        if (response.status == RESPONSE_STATUS.FAILED) {
+            throw new Error(messages.apexGuru.errors.unableToAnalyzeFile(response.message ?? ''));
+        } else if (!response.requestId || response.status != RESPONSE_STATUS.NEW) {
             throw Error(messages.apexGuru.errors.returnedUnexpectedResponse(JSON.stringify(response, null, 2)));
         }
         return response.requestId;
@@ -81,11 +88,11 @@ export class LiveApexGuruService implements ApexGuruService {
             if (queryResponse) { // After the first attempt, we pause each time between requests
                 await new Promise(resolve => setTimeout(resolve, this.retryIntervalMillis));
             }
-            queryResponse = await this.request('GET', `${Constants.APEX_GURU_REQUEST_ENDPOINT}/${requestId}`);
+            queryResponse = await this.request('GET', await this.getRequestEndpoint(requestId));
             if (queryResponse.status === RESPONSE_STATUS.SUCCESS && queryResponse.report) {
                 return queryResponse;
-            } else if (queryResponse.status === RESPONSE_STATUS.FAILED) { // TODO: I would love a failure message - but the response's report just gives back the file content and nothing else.
-                throw new Error(messages.apexGuru.errors.unableToAnalyzeFile);
+            } else if (queryResponse.status === RESPONSE_STATUS.FAILED) {
+                throw new Error(messages.apexGuru.errors.unableToAnalyzeFile(queryResponse.message ?? ''));
             } else if (queryResponse.status === RESPONSE_STATUS.ERROR && queryResponse.message) {
                 throw new Error(messages.apexGuru.errors.returnedUnexpectedError(queryResponse.message));
             }
@@ -117,73 +124,73 @@ export class LiveApexGuruService implements ApexGuruService {
             this.logger.trace('Call to ApexGuru Service failed:' + getErrorMessage(err));
             return {
                 status: RESPONSE_STATUS.ERROR,
-                message: getErrorMessage(err)
+                message: getErrorMessage(err),
             } as T;
         }
     }
+
+    private async getValidateEndpoint(): Promise<string> {
+        const apiVersion: string = await this.orgConnectionService.getApiVersion();
+        return `/services/data/v${apiVersion}/apexguru/validate`;
+    }
+
+    private async getRequestEndpoint(requestId?: string): Promise<string> {
+        const apiVersion: string = await this.orgConnectionService.getApiVersion();
+        return `/services/data/v${apiVersion}/apexguru/request` + (requestId ? `/${requestId}` : '');
+    }
 }
 
 
-export function toReportArray(response: ApexGuruQueryResponse): ApexGuruReport[] {
-    // TODO: This will change soon enough - once we receive the actual response
-    const report: string = Buffer.from(response.report, 'base64').toString('utf-8');
+export function parsePayload(payloadStr: string): ApexGuruViolation[] {
     try {
-        return JSON.parse(report) as ApexGuruReport[];
+        return JSON.parse(payloadStr) as ApexGuruViolation[];
     } catch (err) {
-        throw new Error(`Unable to parse response from ApexGuru.\n\n` + 
-            `Error:\n${indent(getErrorMessage(err))}\n\nDecoded report:\n${indent(report)}`);
+        throw new Error(messages.apexGuru.errors.unableToParsePayload(indent(getErrorMessage(err))));
     }
 }
 
-function toViolation(parsed: ApexGuruReport, file: string): Violation {
-    // IMPORTANT: AS OF 08/13/2024 THIS ALL FAILS IN PRODUCTION BECAUSE THE NEW ApexGuru Service NOW HAS A NEW
-    //            RESPONSE. TODO: W-19053527
-    const encodedCodeAfter = parsed.properties.find((prop: ApexGuruProperty) => prop.name === 'code_after')?.value
-        ?? parsed.properties.find((prop: ApexGuruProperty) => prop.name === 'class_after')?.value
-        ?? '';
-    const suggestedCode: string = Buffer.from(encodedCodeAfter, 'base64').toString('utf8');
-
-    const lineNumber = parseInt(parsed.properties.find((prop: ApexGuruProperty) => prop.name === 'line_number')?.value);
-
-    const violationLocation: CodeLocation = {
-        file: file,
-        startLine: lineNumber,
-        startColumn: 1
+function toViolation(apexGuruViolation: ApexGuruViolation, file: string): Violation {
+    const codeAnalyzerViolation: Violation = {
+        rule: apexGuruViolation.rule,
+        engine: APEX_GURU_ENGINE_NAME,
+        message: apexGuruViolation.message,
+        severity: apexGuruViolation.severity,
+        locations: apexGuruViolation.locations.map(l => addFile(l, file)),
+        primaryLocationIndex: apexGuruViolation.primaryLocationIndex,
+        tags: [], // Currently not used
+        resources: apexGuruViolation.resources ?? [],
+        suggestions: apexGuruViolation.suggestions?.map(s => {
+            s.location = addFile(s.location, file);
+            return s;
+        }),
+        fixes: apexGuruViolation.fixes?.map(f => {
+            f.location = addFile(f.location, file);
+            return f;
+        })
     };
+    return codeAnalyzerViolation;
+}
 
-    const violation: Violation = {
-        rule: parsed.type,
-        engine: APEX_GURU_ENGINE_NAME,
-        message: parsed.value,
-        severity: 1, // TODO: Should this really be critical level violation? This seems off.
-        locations: [violationLocation],
-        primaryLocationIndex: 0,
-        tags: [],
-        resources: [
-            'https://help.salesforce.com/s/articleView?id=sf.apexguru_antipatterns.htm&type=5'
-        ]
+function addFile(apexGuruLocation: CodeLocation, filePath: string): CodeLocation {
+    return {
+        ...apexGuruLocation,
+        file: filePath
     };
+}
 
-    // TODO: Soon we'll be receiving a different looking payload which will help us differentiate between fixes and suggestions.
-    //       For now, we are going to treat suggestedCode as a fix and a suggestion (as the current pilot code does)
-    if (suggestedCode.length > 0) {
-        violation.fixes = [
-            {
-                location: violationLocation,
-                fixedCode: `/*\n//ApexGuru Suggestions: \n${suggestedCode}\n*/`
-            }
-        ]
-        violation.suggestions = [
-            {
-                location: violationLocation,
-                // This message is temporary and will be improved as we get a better response back and unify the suggestions experience
-                message: suggestedCode
-            }
-        ]
-    }
-    return violation;
+function encodeToBase64(value: string): string {
+    return Buffer.from(value).toString('base64');
 }
 
+function decodeFromBase64(value: string): string {
+    return Buffer.from(value, 'base64').toString('utf-8');
+}
+
+
+type ApexGuruRequestBody = {
+    // Must be base64 encoded
+    classContent: string
+}
 
 type ApexGuruResponse = {
     status: string;
@@ -195,17 +202,21 @@ type ApexGuruInitialResponse = ApexGuruResponse & {
 }
 
 type ApexGuruQueryResponse = ApexGuruResponse & {
+    // Is returned with base64 encoding
     report: string;
 }
 
-type ApexGuruProperty = {
-    name: string;
-    value: string;
-};
+type ApexGuruViolation = {
+    rule: string;
+    message: string;
+
+    // Note that none of these location objects from ApexGuru will have a "file" field on it
+    locations: CodeLocation[];
+    primaryLocationIndex: number;
+    severity: number;
+    resources: string[];
 
-type ApexGuruReport = {
-    id: string;
-    type: string;
-    value: string;
-    properties: ApexGuruProperty[];
+    // Note that each suggestion and fix location from ApexGuru will not have a "file" field on it
+    suggestions?: Suggestion[];
+    fixes?: Fix[];
 }

src/lib/constants.ts

@@ -55,12 +55,6 @@ export const MINIMUM_REQUIRED_VERSION_CORE_EXTENSION = '58.4.1';
 export const RECOMMENDED_MINIMUM_REQUIRED_CODE_ANALYZER_CLI_PLUGIN_VERSION = '5.0.0';
 export const ABSOLUTE_MINIMUM_REQUIRED_CODE_ANALYZER_CLI_PLUGIN_VERSION = '5.0.0-beta.0';
 
-// apex guru APIS
-export const APEX_GURU_VALIDATE_ENDPOINT = '/services/data/v62.0/apexguru/validate'
-export const APEX_GURU_REQUEST_ENDPOINT = '/services/data/v62.0/apexguru/request'
-export const APEX_GURU_MAX_TIMEOUT_SECONDS = 60;
-export const APEX_GURU_RETRY_INTERVAL_MILLIS = 1000;
-
 // Context variables (dynamically set but consumed by the "when" conditions in the package.json "contributes" sections)
 export const CONTEXT_VAR_EXTENSION_ACTIVATED = 'sfca.extensionActivated';
 export const CONTEXT_VAR_APEX_GURU_ENABLED = 'sfca.apexGuruEnabled';

src/lib/external-services/org-connection-service.ts

@@ -2,6 +2,7 @@ import * as vscode from "vscode";
 
 export interface OrgConnectionService {
     isAuthed(): boolean;
+    getApiVersion(): Promise<string>;
     onOrgChange(callback: () => void): void;
     request<T>(requestOptions: HttpRequest): Promise<T>;
 }
@@ -15,9 +16,15 @@ export class NoOpOrgConnectionService implements OrgConnectionService {
     isAuthed(): boolean {
         return false;
     }
+
+    getApiVersion(): Promise<string> {
+        throw new Error(`Cannot get the api verison because no org is authed.`);
+    }
+
     onOrgChange(_callback: () => void): void {
         // No-op
     }
+
     request<T>(requestOptions: HttpRequest): Promise<T> {
         throw new Error(`Cannot make the following request because no org is authed:\n${JSON.stringify(requestOptions, null, 2)}`);
     }
@@ -34,6 +41,15 @@ export class LiveOrgConnectionService implements OrgConnectionService {
         return this.workpaceContext.orgId?.length > 0;
     }
 
+    async getApiVersion(): Promise<string> {
+        if (!this.isAuthed()) {
+            throw new Error(`Cannot get the api verison because no org is authed.`);
+        }
+        const connection: Connection = await this.workpaceContext.getConnection();
+        return connection.getApiVersion();
+    }
+
+
     onOrgChange(callback: (event: OrgUserInfo) => void): void {
         this.workpaceContext.onOrgChange(callback);
     }

src/lib/messages.ts

@@ -29,15 +29,17 @@ export const messages = {
                 `ApexGuru can scan only one file at a time. Ignoring the other files in your multi-selection and scanning only this file: ${file}.`
         },
         errors: {
-            unableToAnalyzeFile: 'ApexGuru was unable to analyze the file.',
+            unableToAnalyzeFile: (reason: string) => `ApexGuru was unable to analyze the file. ${reason}`,
             returnedUnexpectedResponse: (responseStr: string) => 
                 `ApexGuru returned an unexpected response:\n${responseStr}`,
             returnedUnexpectedError: (errMsg: string) => `ApexGuru returned an unexpected error: ${errMsg}`,
             failedToGetResponseBeforeTimeout: (maxSeconds: number, lastResponse: string) => 
                 `Failed to get a successful response from ApexGuru after ${maxSeconds} seconds.\n` + 
                 `Last response:\n${lastResponse}`,
             expectedResponseToContainStatusField: (responseStr: string) => 
-                `ApexGuru returned a response without a 'status' field containing a string value. Instead received:\n${responseStr}`
+                `ApexGuru returned a response without a 'status' field containing a string value. Instead received:\n${responseStr}`,
+            unableToParsePayload: (errMsg: string) =>
+                `Unable to parse the payload from the response from ApexGuru. Error:\n${errMsg}`
         }
     },
     info: {