Skip to content

Commit 277869e

Browse files
committed
Ensure graph queries with ids that have slashes work
Do this by actually walking the interpretation directory. Move the directory walker from tests to prod and make it async. Also add tests for it. And add a warning on graph views to let users know that it is not production quality. Finally, change the interpreted directory to be `graphResults` instead of `interpretedResults.sarif`.
1 parent cdb9506 commit 277869e

9 files changed

Lines changed: 127 additions & 37 deletions

File tree

extensions/ql-vscode/src/cli.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { CancellationToken, Disposable, Uri } from 'vscode';
1313
import { BQRSInfo, DecodedBqrsChunk } from './pure/bqrs-cli-types';
1414
import { CliConfig } from './config';
1515
import { DistributionProvider, FindDistributionResultKind } from './distribution';
16-
import { assertNever } from './pure/helpers-pure';
16+
import { assertNever, walkDirectory } from './pure/helpers-pure';
1717
import { QueryMetadata, SortDirection } from './pure/interface-types';
1818
import { Logger, ProgressReporter } from './logging';
1919
import { CompilationMessage } from './pure/messages';
@@ -729,11 +729,15 @@ export class CodeQLCliServer implements Disposable {
729729
return await sarifParser(interpretedResultsPath);
730730
}
731731

732+
// Warning: this function is untenable for large dot files,
732733
async readDotFiles(dir: string): Promise<string[]> {
733-
return Promise.all((await fs.readdir(dir))
734-
.filter(name => path.extname(name).toLowerCase() === '.dot')
735-
.map(file => fs.readFile(path.join(dir, file), 'utf8'))
736-
);
734+
const dotFiles: Promise<string>[] = [];
735+
for await (const file of walkDirectory(dir)) {
736+
if (file.endsWith('.dot')) {
737+
dotFiles.push(fs.readFile(file, 'utf8'));
738+
}
739+
}
740+
return Promise.all(dotFiles);
737741
}
738742

739743
async interpretBqrsGraph(metadata: QueryMetadata, resultsPath: string, interpretedResultsPath: string, sourceInfo?: SourceInfo): Promise<string[]> {
@@ -1246,7 +1250,7 @@ export class CliVersionConstraint {
12461250

12471251
/**
12481252
* CLI version where the `--evaluator-log` and related options to the query server were introduced,
1249-
* on a per-query server basis.
1253+
* on a per-query server basis.
12501254
*/
12511255
public static CLI_VERSION_WITH_STRUCTURED_EVAL_LOG = new SemVer('2.8.2');
12521256

extensions/ql-vscode/src/helpers.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,3 +558,28 @@ export async function createTimestampFile(storagePath: string) {
558558
await fs.ensureDir(storagePath);
559559
await fs.writeFile(timestampPath, Date.now().toString(), 'utf8');
560560
}
561+
562+
563+
/**
564+
* Recursively walk a directory and return the full path to all files found.
565+
* Symbolic links are ignored.
566+
*
567+
* @param dir the directory to walk
568+
*
569+
* @return An iterator of the full path to all files recursively found in the directory.
570+
*/
571+
export async function* walkDirectory(dir: string): AsyncIterableIterator<string> {
572+
const seenFiles = new Set<string>();
573+
for await (const d of await fs.opendir(dir)) {
574+
const entry = path.join(dir, d.name);
575+
if (seenFiles.has(entry)) {
576+
continue;
577+
}
578+
seenFiles.add(entry);
579+
if (d.isDirectory()) {
580+
yield* walkDirectory(entry);
581+
} else if (d.isFile()) {
582+
yield entry;
583+
}
584+
}
585+
}

extensions/ql-vscode/src/pure/helpers-pure.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
12
/**
23
* helpers-pure.ts
34
* ------------

extensions/ql-vscode/src/run-queries.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,11 @@ export class QueryEvaluationInfo {
8686
get resultsPaths() {
8787
return {
8888
resultsPath: path.join(this.querySaveDir, 'results.bqrs'),
89-
interpretedResultsPath: path.join(this.querySaveDir, 'interpretedResults.sarif'),
89+
interpretedResultsPath: path.join(this.querySaveDir,
90+
this.metadata?.kind === 'graph'
91+
? 'graphResults'
92+
: 'interpretedResults.sarif'
93+
),
9094
};
9195
}
9296

extensions/ql-vscode/src/view/graph.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ export class Graph extends React.Component<GraphProps> {
2525
}
2626

2727
return <>
28+
<div className={graphClassName}>
29+
<strong>Warning:</strong> The Graph Viewer is not a publicly released feature and will crash on large graphs.
30+
</div>
2831
<div id={graphId} className={graphClassName}><span>Rendering graph...</span></div>
2932
</>;
3033
};

extensions/ql-vscode/src/vscode-tests/directory-walker.ts

Lines changed: 0 additions & 23 deletions
This file was deleted.

extensions/ql-vscode/src/vscode-tests/no-workspace/helpers.test.ts

Lines changed: 77 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,32 @@
11
import { expect } from 'chai';
22
import 'mocha';
3-
import { EnvironmentVariableCollection, EnvironmentVariableMutator, Event, ExtensionContext, ExtensionMode, Memento, SecretStorage, SecretStorageChangeEvent, Uri, window } from 'vscode';
3+
import {
4+
EnvironmentVariableCollection,
5+
EnvironmentVariableMutator,
6+
Event,
7+
ExtensionContext,
8+
ExtensionMode,
9+
Memento,
10+
SecretStorage,
11+
SecretStorageChangeEvent,
12+
Uri,
13+
window
14+
} from 'vscode';
415
import * as yaml from 'js-yaml';
516
import * as tmp from 'tmp';
617
import * as path from 'path';
718
import * as fs from 'fs-extra';
819
import * as sinon from 'sinon';
20+
import { DirResult } from 'tmp';
921

1022
import {
1123
getInitialQueryContents,
1224
InvocationRateLimiter,
1325
isLikelyDbLanguageFolder,
1426
showBinaryChoiceDialog,
1527
showBinaryChoiceWithUrlDialog,
16-
showInformationMessageWithAction
28+
showInformationMessageWithAction,
29+
walkDirectory
1730
} from '../../helpers';
1831
import { reportStreamProgress } from '../../commandRunner';
1932
import Sinon = require('sinon');
@@ -377,3 +390,65 @@ describe('helpers', () => {
377390
});
378391
});
379392
});
393+
394+
describe('walkDirectory', () => {
395+
let tmpDir: DirResult;
396+
let dir: string;
397+
let dir2: string;
398+
399+
beforeEach(() => {
400+
tmpDir = tmp.dirSync({ unsafeCleanup: true });
401+
dir = path.join(tmpDir.name, 'dir');
402+
fs.ensureDirSync(dir);
403+
dir2 = path.join(tmpDir.name, 'dir2');
404+
});
405+
406+
afterEach(() => {
407+
tmpDir.removeCallback();
408+
});
409+
410+
it('should walk a directory', async () => {
411+
const file1 = path.join(dir, 'file1');
412+
const file2 = path.join(dir, 'file2');
413+
const file3 = path.join(dir, 'file3');
414+
const dir3 = path.join(dir, 'dir3');
415+
const file4 = path.join(dir, 'file4');
416+
const file5 = path.join(dir, 'file5');
417+
const file6 = path.join(dir, 'file6');
418+
419+
// These symlinks link back to paths that are already existing, so ignore.
420+
const symLinkFile7 = path.join(dir, 'symlink0');
421+
const symlinkDir = path.join(dir2, 'symlink1');
422+
423+
// some symlinks that point outside of the base dir.
424+
const file8 = path.join(tmpDir.name, 'file8');
425+
const file9 = path.join(dir2, 'file8');
426+
const symlinkDir2 = path.join(dir2, 'symlink2');
427+
const symlinkFile2 = path.join(dir2, 'symlinkFile3');
428+
429+
fs.ensureDirSync(dir2);
430+
fs.ensureDirSync(dir3);
431+
432+
fs.writeFileSync(file1, 'file1');
433+
fs.writeFileSync(file2, 'file2');
434+
fs.writeFileSync(file3, 'file3');
435+
fs.writeFileSync(file4, 'file4');
436+
fs.writeFileSync(file5, 'file5');
437+
fs.writeFileSync(file6, 'file6');
438+
fs.writeFileSync(file8, 'file8');
439+
fs.writeFileSync(file9, 'file9');
440+
441+
fs.symlinkSync(file6, symLinkFile7, 'file');
442+
fs.symlinkSync(dir3, symlinkDir, 'dir');
443+
fs.symlinkSync(file8, symlinkFile2, 'file');
444+
fs.symlinkSync(dir2, symlinkDir2, 'dir');
445+
446+
const files = [];
447+
for await (const file of walkDirectory(dir)) {
448+
files.push(file);
449+
}
450+
451+
// Only real files should be returned.
452+
expect(files.sort()).to.deep.eq([file1, file2, file3, file4, file5, file6]);
453+
});
454+
});

extensions/ql-vscode/src/vscode-tests/no-workspace/remote-query-history.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { AnalysesResultsManager } from '../../remote-queries/analyses-results-ma
1717
import { RemoteQueryResult } from '../../remote-queries/shared/remote-query-result';
1818
import { DisposableBucket } from '../disposable-bucket';
1919
import { testDisposeHandler } from '../test-dispose-handler';
20-
import { walk } from '../directory-walker';
20+
import { walkDirectory } from '../../pure/helpers-pure';
2121

2222
chai.use(chaiAsPromised);
2323
const expect = chai.expect;
@@ -41,10 +41,10 @@ describe('Remote queries and query history manager', function() {
4141
let showTextDocumentSpy: sinon.SinonSpy;
4242
let openTextDocumentSpy: sinon.SinonSpy;
4343

44-
beforeEach(() => {
44+
beforeEach(async () => {
4545
// Since these tests change the state of the query history manager, we need to copy the original
4646
// to a temporary folder where we can manipulate it for tests
47-
copyHistoryState();
47+
await copyHistoryState();
4848
});
4949

5050
afterEach(() => {
@@ -321,12 +321,12 @@ describe('Remote queries and query history manager', function() {
321321
});
322322
});
323323

324-
function copyHistoryState() {
324+
async function copyHistoryState() {
325325
fs.ensureDirSync(STORAGE_DIR);
326326
fs.copySync(path.join(__dirname, 'data/remote-queries/'), path.join(tmpDir.name, 'remote-queries'));
327327

328328
// also, replace the files with "PLACEHOLDER" so that they have the correct directory
329-
for (const p of walk(STORAGE_DIR)) {
329+
for await (const p of walkDirectory(STORAGE_DIR)) {
330330
replacePlaceholder(path.join(p));
331331
}
332332
}

extensions/ql-vscode/test/pure-tests/helpers-pure.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { fail } from 'assert';
22
import { expect } from 'chai';
3+
34
import { asyncFilter } from '../../src/pure/helpers-pure';
45

56
describe('helpers-pure', () => {

0 commit comments

Comments
 (0)