From db6fc5d7f068126ff9bc8ffd01c9c70cfc709ad2 Mon Sep 17 00:00:00 2001 From: Andrew Eisenberg Date: Wed, 16 Sep 2020 14:37:29 -0700 Subject: [PATCH] Refactor: Change renderLocation in webview * 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 --- extensions/ql-vscode/src/bqrs-cli-types.ts | 8 ++- extensions/ql-vscode/src/bqrs-utils.ts | 3 +- .../src/compare/compare-interface.ts | 4 +- extensions/ql-vscode/src/interface.ts | 6 +-- extensions/ql-vscode/src/sarif-utils.ts | 4 +- .../ql-vscode/src/view/RawTableValue.tsx | 18 ++----- extensions/ql-vscode/src/view/alert-table.tsx | 49 ++++++++++++++----- .../ql-vscode/src/view/result-table-utils.tsx | 41 ++++++++++------ extensions/ql-vscode/src/view/results.tsx | 19 ++++--- .../no-workspace/interface-utils.test.ts | 17 +++++++ 10 files changed, 112 insertions(+), 57 deletions(-) diff --git a/extensions/ql-vscode/src/bqrs-cli-types.ts b/extensions/ql-vscode/src/bqrs-cli-types.ts index 81dd54edc..024a9c53d 100644 --- 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 index f5936ceba..6004aa685 100644 --- 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 index 71877f3be..c9b2b6104 100644 --- 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 index 18b03ad9f..ce004d25f 100644 --- 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 index 4a8467da4..cbfc8675a 100644 --- 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 index b4ce7b341..59b150e04 100644 --- 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 {v}; } - const loc = v.url; - if (!loc) { - return ; - } else if (isStringLoc(loc)) { - return {loc}; - } else if (isWholeFileLoc(loc) || isLineColumnLoc(loc)) { - return renderLocation(loc, v.label, props.databaseUri); - } else { - return ; - } + 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 index 32cf6a348..0d3717315 100644 --- 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 { 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 { * 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 { 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 { [expansionIndex]; rows.push( - + {indicator} @@ -223,7 +242,7 @@ export class PathTable extends React.Component { if (currentResultExpanded) { const indicator = currentPathExpanded ? octicons.chevronDown : octicons.chevronRight; rows.push( - + {indicator} @@ -249,7 +268,7 @@ export class PathTable extends React.Component { const stepIndex = pathNodeIndex + 1; // Convert to 1-based const zebraIndex = resultIndex + stepIndex; rows.push( - + {stepIndex} @@ -264,9 +283,13 @@ export class PathTable extends React.Component { }); if (numTruncatedResults > 0) { - rows.push( - Too many results to show at once. {numTruncatedResults} result(s) omitted. - ); + rows.push( + + + Too many results to show at once. {numTruncatedResults} result(s) omitted. + + + ); } return diff --git a/extensions/ql-vscode/src/view/result-table-utils.tsx b/extensions/ql-vscode/src/view/result-table-utils.tsx index d92e0a5f1..dd0e4f93e 100644 --- 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 ; + } else if (isStringLoc(loc)) { + return {loc}; + } // 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 {displayLabel}; - } else { - return {displayLabel}; - } + onClick={jumpToLocationHandler(resolvableLoc, databaseUri, callback)}> + {displayLabel} + + ); + } else { + return {displayLabel}; } - return ; } /** diff --git a/extensions/ql-vscode/src/view/results.tsx b/extensions/ql-vscode/src/view/results.tsx index bd53eef07..045f57520 100644 --- 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[] { 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 { @@ -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 index fa9ca3a45..6231e4d4d 100644 --- 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;