Refactor: Change renderLocation in webview

aeisenberg · aeisenberg · commit db6fc5d7f068 · 2020-09-24T07:54:50.000-07:00
* It is now more general and the logic is simplified
* Also, add more comments
* Rename `adaptBqrs` to `transformBqrsResultSet`
* Remove a react error for missing a key attribute in a list
diff --git a/extensions/ql-vscode/src/bqrs-cli-types.ts b/extensions/ql-vscode/src/bqrs-cli-types.ts
@@ -90,7 +90,13 @@ export interface RawResultSet {
   readonly rows: readonly ResultRow[];
 }
 
-export function adaptBqrs(schema: ResultSetSchema, page: DecodedBqrsChunk): RawResultSet {
+// TODO: This function is not necessary. It generates a tuple that is slightly easier
+// to handle than the ResultSetSchema and DecodedBqrsChunk. But perhaps it is unnecessary
+// boilerplate.
+export function transformBqrsResultSet(
+  schema: ResultSetSchema,
+  page: DecodedBqrsChunk
+): RawResultSet {
   return {
     schema,
     rows: Array.from(page.tuples),
diff --git a/extensions/ql-vscode/src/bqrs-utils.ts b/extensions/ql-vscode/src/bqrs-utils.ts
@@ -63,8 +63,7 @@ function isWholeFileMatch(matches: RegExpExecArray): boolean {
 }
 
 /**
- * Checks whether the file path is empty. For now, just check whether
- * the file path is empty. If so, we do not want to render this location
+ * Checks whether the file path is empty. If so, we do not want to render this location
  * as a link.
  *
  * @param uri A file uri
diff --git a/extensions/ql-vscode/src/compare/compare-interface.ts b/extensions/ql-vscode/src/compare/compare-interface.ts
@@ -19,7 +19,7 @@ import { Logger } from '../logging';
 import { CodeQLCliServer } from '../cli';
 import { DatabaseManager } from '../databases';
 import { getHtmlForWebview, jumpToLocation } from '../interface-utils';
-import { adaptBqrs, RawResultSet, BQRSInfo } from '../bqrs-cli-types';
+import { transformBqrsResultSet, RawResultSet, BQRSInfo } from '../bqrs-cli-types';
 import resultsDiff from './resultsDiff';
 
 interface ComparePair {
@@ -256,7 +256,7 @@ export class CompareInterfaceManager extends DisposableObject {
       resultsPath,
       resultSetName
     );
-    return adaptBqrs(schema, chunk);
+    return transformBqrsResultSet(schema, chunk);
   }
 
   private compareResults(
diff --git a/extensions/ql-vscode/src/interface.ts b/extensions/ql-vscode/src/interface.ts
@@ -46,7 +46,7 @@ import {
   jumpToLocation,
 } from './interface-utils';
 import { getDefaultResultSetName, ParsedResultSets } from './interface-types';
-import { RawResultSet, adaptBqrs, ResultSetSchema } from './bqrs-cli-types';
+import { RawResultSet, transformBqrsResultSet, ResultSetSchema } from './bqrs-cli-types';
 
 /**
  * interface.ts
@@ -372,7 +372,7 @@ export class InterfaceManager extends DisposableObject {
           pageSize: RAW_RESULTS_PAGE_SIZE
         }
       );
-      const resultSet = adaptBqrs(schema, chunk);
+      const resultSet = transformBqrsResultSet(schema, chunk);
       return {
         pageNumber: 0,
         numPages: numPagesOfResultSet(resultSet),
@@ -485,7 +485,7 @@ export class InterfaceManager extends DisposableObject {
         pageSize: RAW_RESULTS_PAGE_SIZE
       }
     );
-    const resultSet = adaptBqrs(schema, chunk);
+    const resultSet = transformBqrsResultSet(schema, chunk);
 
     const parsedResultSets: ParsedResultSets = {
       pageNumber,
diff --git a/extensions/ql-vscode/src/sarif-utils.ts b/extensions/ql-vscode/src/sarif-utils.ts
@@ -7,6 +7,9 @@ export interface SarifLink {
   text: string;
 }
 
+// The type of a result that has no associated location.
+// hint is a string intended for display to the user
+// that explains why there is no location.
 interface NoLocation {
   hint: string;
 }
@@ -85,7 +88,6 @@ export function parseSarifLocation(
   // file uri or a relative uri.
   const uri = physicalLocation.artifactLocation.uri;
 
-  // FIXME: This is probably wrong
   const fileUriRegex = /^file:/;
   const effectiveLocation = uri.match(fileUriRegex)
     ? uri
diff --git a/extensions/ql-vscode/src/view/RawTableValue.tsx b/extensions/ql-vscode/src/view/RawTableValue.tsx
@@ -2,7 +2,6 @@ import * as React from 'react';
 
 import { renderLocation } from './result-table-utils';
 import { ColumnValue } from '../bqrs-cli-types';
-import { isStringLoc, isWholeFileLoc, isLineColumnLoc } from '../bqrs-utils';
 
 interface Props {
   value: ColumnValue;
@@ -11,20 +10,13 @@ interface Props {
 
 export default function RawTableValue(props: Props): JSX.Element {
   const v = props.value;
-  if (typeof v === 'string'
+  if (
+    typeof v === 'string'
     || typeof v === 'number'
-    || typeof v === 'boolean') {
+    || typeof v === 'boolean'
+  ) {
     return <span>{v}</span>;
   }
 
-  const loc = v.url;
-  if (!loc) {
-    return <span />;
-  } else if (isStringLoc(loc)) {
-    return <a href={loc}>{loc}</a>;
-  } else if (isWholeFileLoc(loc) || isLineColumnLoc(loc)) {
-    return renderLocation(loc, v.label, props.databaseUri);
-  } else {
-    return <span />;
-  }
+  return renderLocation(v.url, v.label, props.databaseUri);
 }
diff --git a/extensions/ql-vscode/src/view/alert-table.tsx b/extensions/ql-vscode/src/view/alert-table.tsx
@@ -132,7 +132,13 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
       if ('hint' in parsedLoc) {
         return renderNonLocation(text, parsedLoc.hint);
       } else if (isWholeFileLoc(parsedLoc) || isLineColumnLoc(parsedLoc)) {
-        return renderLocation(parsedLoc, text, databaseUri, undefined, updateSelectionCallback(pathNodeKey));
+        return renderLocation(
+          parsedLoc,
+          text,
+          databaseUri,
+          undefined,
+          updateSelectionCallback(pathNodeKey)
+        );
       } else {
         return undefined;
       }
@@ -142,18 +148,33 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
      * Render sarif location as a link with the text being simply a
      * human-readable form of the location itself.
      */
-    function renderSarifLocation(loc: Sarif.Location, pathNodeKey: Keys.PathNode | undefined): JSX.Element | undefined {
+    function renderSarifLocation(
+      loc: Sarif.Location,
+      pathNodeKey: Keys.PathNode | undefined
+    ): JSX.Element | undefined {
       const parsedLoc = parseSarifLocation(loc, sourceLocationPrefix);
       if ('hint' in parsedLoc) {
         return renderNonLocation('[no location]', parsedLoc.hint);
       } else if (isWholeFileLoc(parsedLoc)) {
         const shortLocation = `${path.basename(parsedLoc.userVisibleFile)}`;
         const longLocation = `${parsedLoc.userVisibleFile}`;
-        return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey));
+        return renderLocation(
+          parsedLoc,
+          shortLocation,
+          databaseUri,
+          longLocation,
+          updateSelectionCallback(pathNodeKey)
+        );
       } else if (isLineColumnLoc(parsedLoc)) {
         const shortLocation = `${path.basename(parsedLoc.userVisibleFile)}:${parsedLoc.startLine}:${parsedLoc.startColumn}`;
         const longLocation = `${parsedLoc.userVisibleFile}`;
-        return renderLocation(parsedLoc, shortLocation, databaseUri, longLocation, updateSelectionCallback(pathNodeKey));
+        return renderLocation(
+          parsedLoc,
+          shortLocation,
+          databaseUri,
+          longLocation,
+          updateSelectionCallback(pathNodeKey)
+        );
       } else {
         return undefined;
       }
@@ -163,9 +184,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
       return (e) => this.toggle(e, indices);
     };
 
-    if (resultSet.sarif.runs.length === 0 ||
-      resultSet.sarif.runs[0].results === undefined ||
-      resultSet.sarif.runs[0].results.length === 0) {
+    if (!resultSet.sarif.runs?.[0]?.results?.length) {
       return this.renderNoResults();
     }
 
@@ -202,7 +221,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
           [expansionIndex];
 
         rows.push(
-          <tr {...zebraStripe(resultIndex)}>
+          <tr {...zebraStripe(resultIndex)} key={resultIndex}>
             <td className="vscode-codeql__icon-cell vscode-codeql__dropdown-cell" onMouseDown={toggler(indices)}>
               {indicator}
             </td>
@@ -223,7 +242,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
           if (currentResultExpanded) {
             const indicator = currentPathExpanded ? octicons.chevronDown : octicons.chevronRight;
             rows.push(
-              <tr {...zebraStripe(resultIndex)}>
+              <tr {...zebraStripe(resultIndex)} key={`${resultIndex}-${pathIndex}`}>
                 <td className="vscode-codeql__icon-cell"><span className="vscode-codeql__vertical-rule"></span></td>
                 <td className="vscode-codeql__icon-cell vscode-codeql__dropdown-cell" onMouseDown={toggler([expansionIndex])}>{indicator}</td>
                 <td className="vscode-codeql__text-center" colSpan={3}>
@@ -249,7 +268,7 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
               const stepIndex = pathNodeIndex + 1; // Convert to 1-based
               const zebraIndex = resultIndex + stepIndex;
               rows.push(
-                <tr className={isSelected ? 'vscode-codeql__selected-path-node' : undefined}>
+                <tr className={isSelected ? 'vscode-codeql__selected-path-node' : undefined} key={`${resultIndex}-${pathIndex}-${pathNodeIndex}`}>
                   <td className="vscode-codeql__icon-cell"><span className="vscode-codeql__vertical-rule"></span></td>
                   <td className="vscode-codeql__icon-cell"><span className="vscode-codeql__vertical-rule"></span></td>
                   <td {...selectableZebraStripe(isSelected, zebraIndex, 'vscode-codeql__path-index-cell')}>{stepIndex}</td>
@@ -264,9 +283,13 @@ export class PathTable extends React.Component<PathTableProps, PathTableState> {
     });
 
     if (numTruncatedResults > 0) {
-      rows.push(<tr><td colSpan={5} style={{ textAlign: 'center', fontStyle: 'italic' }}>
-        Too many results to show at once. {numTruncatedResults} result(s) omitted.
-      </td></tr>);
+      rows.push(
+        <tr key="truncatd-message">
+          <td colSpan={5} style={{ textAlign: 'center', fontStyle: 'italic' }}>
+            Too many results to show at once. {numTruncatedResults} result(s) omitted.
+          </td>
+        </tr>
+      );
     }
 
     return <table className={className}>
diff --git a/extensions/ql-vscode/src/view/result-table-utils.tsx b/extensions/ql-vscode/src/view/result-table-utils.tsx
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import { UrlValue, ResolvableLocationValue } from '../bqrs-cli-types';
-import { tryGetResolvableLocation } from '../bqrs-utils';
+import { isStringLoc, tryGetResolvableLocation } from '../bqrs-utils';
 import { RawResultsSortState, QueryMetadata, SortDirection } from '../interface-types';
 import { assertNever } from '../helpers-pure';
 import { ResultSet } from '../interface-types';
@@ -60,28 +60,41 @@ export function jumpToLocation(loc: ResolvableLocationValue, databaseUri: string
 /**
  * Render a location as a link which when clicked displays the original location.
  */
-export function renderLocation(loc: UrlValue | undefined, label: string | undefined,
-  databaseUri: string, title?: string, callback?: () => void): JSX.Element {
+export function renderLocation(
+  loc: UrlValue | undefined,
+  label: string | undefined,
+  databaseUri: string,
+  title?: string,
+  callback?: () => void
+): JSX.Element {
+
+  if (loc === undefined) {
+    return <span />;
+  } else if (isStringLoc(loc)) {
+    return <a href={loc}>{loc}</a>;
+  }
 
   // If the label was empty, use a placeholder instead, so the link is still clickable.
   let displayLabel = label;
-  if (label === undefined || label === '')
+  if (!label) {
     displayLabel = '[empty string]';
-  else if (label.match(/^\s+$/))
+  } else if (label.match(/^\s+$/)) {
     displayLabel = `[whitespace: "${label}"]`;
+  }
 
-  if (loc !== undefined) {
-    const resolvableLoc = tryGetResolvableLocation(loc);
-    if (resolvableLoc !== undefined) {
-      return <a href="#"
+  const resolvableLoc = tryGetResolvableLocation(loc);
+  if (resolvableLoc !== undefined) {
+    return (
+      <a href="#"
         className="vscode-codeql__result-table-location-link"
         title={title}
-        onClick={jumpToLocationHandler(resolvableLoc, databaseUri, callback)}>{displayLabel}</a>;
-    } else {
-      return <span title={title}>{displayLabel}</span>;
-    }
+        onClick={jumpToLocationHandler(resolvableLoc, databaseUri, callback)}>
+        {displayLabel}
+      </a>
+    );
+  } else {
+    return <span title={title}>{displayLabel}</span>;
   }
-  return <span />;
 }
 
 /**
diff --git a/extensions/ql-vscode/src/view/results.tsx b/extensions/ql-vscode/src/view/results.tsx
@@ -11,7 +11,7 @@ import {
   QueryMetadata,
   ResultsPaths,
   ALERTS_TABLE_NAME,
-  ParsedResultSets,
+  ParsedResultSets
 } from '../interface-types';
 import { EventHandlers as EventHandlerList } from './event-handler-list';
 import { ResultTables } from './result-tables';
@@ -172,14 +172,17 @@ class App extends React.Component<{}, ResultsViewState> {
     });
   }
 
-  private async getResultSets(
+  private getResultSets(
     resultsInfo: ResultsInfo
-  ): Promise<readonly ResultSet[]> {
+  ): readonly ResultSet[] {
     const parsedResultSets = resultsInfo.parsedResultSets;
-    return [{
-      ...parsedResultSets.resultSet,
-      t: (parsedResultSets.resultSet.t ?? 'RawResultSet') as any
-    }];
+    const resultSet = parsedResultSets.resultSet;
+    if (!resultSet.t) {
+      throw new Error(
+        'Missing result set type. Should be either "SarifResultSet" or "RawResultSet".'
+      );
+    }
+    return [resultSet];
   }
 
   private async loadResults(): Promise<void> {
@@ -191,7 +194,7 @@ class App extends React.Component<{}, ResultsViewState> {
     let results: Results | null = null;
     let statusText = '';
     try {
-      const resultSets = await this.getResultSets(resultsInfo);
+      const resultSets = this.getResultSets(resultsInfo);
       results = {
         resultSets,
         database: resultsInfo.database,
diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/interface-utils.test.ts
@@ -71,6 +71,23 @@ describe('interface-utils', () => {
 
   describe('resolveWholeFileLocation', () => {
     it('should resolve a whole file location', () => {
+      const mockDatabaseItem: DatabaseItem = ({
+        resolveSourceFile: sinon.stub().returns(vscode.Uri.file('abc')),
+      } as unknown) as DatabaseItem;
+      expect(
+        tryResolveLocation(
+          'file://hucairz:0:0:0:0',
+          mockDatabaseItem
+        )
+      ).to.deep.equal(
+        new vscode.Location(
+          vscode.Uri.file('abc'),
+          new vscode.Range(0, 0, 0, 0)
+        )
+      );
+    });
+
+    it('should resolve a five-part location edge case', () => {
       const mockDatabaseItem: DatabaseItem = ({
         resolveSourceFile: sinon.stub().returns(vscode.Uri.file('abc')),
       } as unknown) as DatabaseItem;

Original file line number	Diff line number	Diff line change
`@@ -63,8 +63,7 @@ function isWholeFileMatch(matches: RegExpExecArray): boolean {`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`/**`
`66`		`- * Checks whether the file path is empty. For now, just check whether`
`67`		`- * the file path is empty. If so, we do not want to render this location`
	`66`	`+ * Checks whether the file path is empty. If so, we do not want to render this location`
`68`	`67`	`* as a link.`
`69`	`68`	`*`
`70`	`69`	`* @param uri A file uri`