Avoid clobbering quick-query file when re-opened

Only recreate the qlpack.yml file.

Also, add an integration test for quick-query creation.

Author: aeisenberg

extensions/ql-vscode/CHANGELOG.md

@@ -6,6 +6,7 @@
 - Add better error messages when AST Viewer is unable to create an AST. [#753](https://github.com/github/vscode-codeql/pull/753)
 - Cache AST viewing operations so that subsequent calls to view the AST of a single file will be extremely fast. [#753](https://github.com/github/vscode-codeql/pull/753)
 - Ensure CodeQL version in status bar updates correctly when version changes. [#754](https://github.com/github/vscode-codeql/pull/754)
+- Avoid deleting the quick query file when it is re-opened. [#747](https://github.com/github/vscode-codeql/pull/747)
 
 ## 1.4.2 - 2 February 2021
 

extensions/ql-vscode/src/quick-query.ts

@@ -1,16 +1,20 @@
 import * as fs from 'fs-extra';
 import * as yaml from 'js-yaml';
 import * as path from 'path';
-import { CancellationToken, ExtensionContext, window as Window, workspace, Uri } from 'vscode';
+import {
+  CancellationToken,
+  ExtensionContext,
+  window as Window,
+  workspace,
+  Uri
+} from 'vscode';
 import { ErrorCodes, ResponseError } from 'vscode-languageclient';
 import { CodeQLCliServer } from './cli';
 import { DatabaseUI } from './databases-ui';
-import { logger } from './logging';
 import {
   getInitialQueryContents,
   getPrimaryDbscheme,
   getQlPackForDbscheme,
-  showAndLogErrorMessage,
   showBinaryChoiceDialog,
 } from './helpers';
 import {
@@ -21,23 +25,35 @@ import {
 const QUICK_QUERIES_DIR_NAME = 'quick-queries';
 const QUICK_QUERY_QUERY_NAME = 'quick-query.ql';
 const QUICK_QUERY_WORKSPACE_FOLDER_NAME = 'Quick Queries';
+const QLPACK_FILE_HEADER = '# This is an automatically generated file.\n\n';
 
 export function isQuickQueryPath(queryPath: string): boolean {
   return path.basename(queryPath) === QUICK_QUERY_QUERY_NAME;
 }
 
-function getQuickQueriesDir(ctx: ExtensionContext): string {
+async function getQuickQueriesDir(ctx: ExtensionContext): Promise<string> {
   const storagePath = ctx.storagePath;
   if (storagePath === undefined) {
     throw new Error('Workspace storage path is undefined');
   }
   const queriesPath = path.join(storagePath, QUICK_QUERIES_DIR_NAME);
-  fs.ensureDir(queriesPath, { mode: 0o700 });
+  await fs.ensureDir(queriesPath, { mode: 0o700 });
   return queriesPath;
 }
 
+function updateQuickQueryDir(queriesDir: string, index: number, len: number) {
+  workspace.updateWorkspaceFolders(
+    index,
+    len,
+    { uri: Uri.file(queriesDir), name: QUICK_QUERY_WORKSPACE_FOLDER_NAME }
+  );
+}
 
-
+function findExistingQuickQueryEditor() {
+  return Window.visibleTextEditors.find(editor =>
+    path.basename(editor.document.uri.fsPath) === QUICK_QUERY_QUERY_NAME
+  );
+}
 
 /**
  * Show a buffer the user can enter a simple query into.
@@ -50,26 +66,18 @@ export async function displayQuickQuery(
   token: CancellationToken
 ) {
 
-  function updateQuickQueryDir(queriesDir: string, index: number, len: number) {
-    workspace.updateWorkspaceFolders(
-      index,
-      len,
-      { uri: Uri.file(queriesDir), name: QUICK_QUERY_WORKSPACE_FOLDER_NAME }
-    );
-  }
-
   try {
-    const workspaceFolders = workspace.workspaceFolders || [];
-    const queriesDir = await getQuickQueriesDir(ctx);
-
     // If there is already a quick query open, don't clobber it, just
     // show it.
-    const existing = workspace.textDocuments.find(doc => path.basename(doc.uri.fsPath) === QUICK_QUERY_QUERY_NAME);
-    if (existing !== undefined) {
-      Window.showTextDocument(existing);
+    const existing = findExistingQuickQueryEditor();
+    if (existing) {
+      await Window.showTextDocument(existing.document);
       return;
     }
 
+    const workspaceFolders = workspace.workspaceFolders || [];
+    const queriesDir = await getQuickQueriesDir(ctx);
+
     // We need to have a multi-root workspace to make quick query work
     // at all. Changing the workspace from single-root to multi-root
     // causes a restart of the whole extension host environment, so we
@@ -88,10 +96,11 @@ export async function displayQuickQuery(
     }
 
     const index = workspaceFolders.findIndex(folder => folder.name === QUICK_QUERY_WORKSPACE_FOLDER_NAME);
-    if (index === -1)
+    if (index === -1) {
       updateQuickQueryDir(queriesDir, workspaceFolders.length, 0);
-    else
+    } else {
       updateQuickQueryDir(queriesDir, index, 1);
+    }
 
     // We're going to infer which qlpack to use from the current database
     const dbItem = await databaseUI.getDatabaseItem(progress, token);
@@ -102,31 +111,38 @@ export async function displayQuickQuery(
     const datasetFolder = await dbItem.getDatasetFolder(cliServer);
     const dbscheme = await getPrimaryDbscheme(datasetFolder);
     const qlpack = await getQlPackForDbscheme(cliServer, dbscheme);
-
-    const quickQueryQlpackYaml: any = {
-      name: 'quick-query',
-      version: '1.0.0',
-      libraryPathDependencies: [qlpack]
-    };
-
-    const qlFile = path.join(queriesDir, QUICK_QUERY_QUERY_NAME);
     const qlPackFile = path.join(queriesDir, 'qlpack.yml');
-    await fs.writeFile(qlFile, getInitialQueryContents(dbItem.language, dbscheme), 'utf8');
-    await fs.writeFile(qlPackFile, yaml.safeDump(quickQueryQlpackYaml), 'utf8');
-    Window.showTextDocument(await workspace.openTextDocument(qlFile));
-  }
-
-  // TODO: clean up error handling for top-level commands like this
-  catch (e) {
-    if (e instanceof UserCancellationException) {
-      logger.log(e.message);
+    const qlFile = path.join(queriesDir, QUICK_QUERY_QUERY_NAME);
+    const shouldRewrite = await checkShouldRewrite(qlPackFile, qlpack);
+
+    // Only rewrite the qlpack file if the database has changed
+    if (shouldRewrite) {
+      const quickQueryQlpackYaml: any = {
+        name: 'quick-query',
+        version: '1.0.0',
+        libraryPathDependencies: [qlpack]
+      };
+      await fs.writeFile(qlPackFile, QLPACK_FILE_HEADER + yaml.safeDump(quickQueryQlpackYaml), 'utf8');
     }
-    else if (e instanceof ResponseError && e.code == ErrorCodes.RequestCancelled) {
-      logger.log(e.message);
+
+    if (shouldRewrite || !(await fs.pathExists(qlFile))) {
+      await fs.writeFile(qlFile, getInitialQueryContents(dbItem.language, dbscheme), 'utf8');
     }
-    else if (e instanceof Error)
-      showAndLogErrorMessage(e.message);
-    else
+
+    await Window.showTextDocument(await workspace.openTextDocument(qlFile));
+  } catch (e) {
+    if (e instanceof ResponseError && e.code == ErrorCodes.RequestCancelled) {
+      throw new UserCancellationException(e.message);
+    } else {
       throw e;
+    }
+  }
+}
+
+async function checkShouldRewrite(qlPackFile: string, newDependency: string) {
+  if (!(await fs.pathExists(qlPackFile))) {
+    return true;
   }
+  const qlPackContents: any = yaml.safeLoad(await fs.readFile(qlPackFile, 'utf8'));
+  return qlPackContents.libraryPathDependencies?.[0] !== newDependency;
 }

extensions/ql-vscode/src/vscode-tests/cli-integration/queries.test.ts

@@ -1,10 +1,11 @@
 import { fail } from 'assert';
-import { CancellationToken, commands, extensions, Uri } from 'vscode';
+import { CancellationToken, commands, ExtensionContext, extensions, Uri } from 'vscode';
 import * as sinon from 'sinon';
 import * as path from 'path';
 import * as fs from 'fs-extra';
 import 'mocha';
 import { expect } from 'chai';
+import * as yaml from 'js-yaml';
 
 import { DatabaseItem, DatabaseManager } from '../../databases';
 import { CodeQLExtensionInterface } from '../../extension';
@@ -34,6 +35,11 @@ describe('Queries', function() {
   let sandbox: sinon.SinonSandbox;
   let progress: sinon.SinonSpy;
   let token: CancellationToken;
+  let ctx: ExtensionContext;
+
+  let qlpackFile: string;
+  let qlFile: string;
+
 
   beforeEach(async () => {
     sandbox = sinon.createSandbox();
@@ -45,6 +51,9 @@ describe('Queries', function() {
         cli = extension.cliServer;
         qs = extension.qs;
         cli.quiet = true;
+        ctx = extension.ctx;
+        qlpackFile = `${ctx.storagePath}/quick-queries/qlpack.yml`;
+        qlFile = `${ctx.storagePath}/quick-queries/quick-query.ql`;
       } else {
         throw new Error('Extension not initialized. Make sure cli is downloaded and installed properly.');
       }
@@ -126,4 +135,42 @@ describe('Queries', function() {
       fail(e);
     }
   });
+
+  it('should create a quick query', async () => {
+    safeDel(qlFile);
+    safeDel(qlpackFile);
+
+    await commands.executeCommand('codeQL.quickQuery');
+
+    // should have created the quick query file and query pack file
+    expect(fs.pathExistsSync(qlFile)).to.be.true;
+    expect(fs.pathExistsSync(qlpackFile)).to.be.true;
+
+    const qlpackContents: any = await yaml.safeLoad(
+      fs.readFileSync(qlpackFile, 'utf8')
+    );
+    // Should have chosen the js libraries
+    expect(qlpackContents.libraryPathDependencies[0]).to.eq('codeql-javascript');
+  });
+
+  it('should avoid creating a quick query', async () => {
+    fs.writeFileSync(qlpackFile, yaml.safeDump({
+      name: 'quick-query',
+      version: '1.0.0',
+      libraryPathDependencies: ['codeql-javascript']
+    }));
+    fs.writeFileSync(qlFile, 'xxx');
+    await commands.executeCommand('codeQL.quickQuery');
+
+    // should not have created the quick query file because database schema hasn't changed
+    expect(fs.readFileSync(qlFile, 'utf8')).to.eq('xxx');
+  });
+
+  function safeDel(file: string) {
+    try {
+      fs.unlinkSync(file);
+    } catch (e) {
+      // ignore
+    }
+  }
 });

extensions/ql-vscode/src/vscode-tests/minimal-workspace/config.test.ts

@@ -26,7 +26,7 @@ describe('config listeners', () => {
   });
 
   interface TestConfig<T> {
-    clazz: new() => {};
+    clazz: new () => {};
     settings: {
       name: string;
       property: string;
@@ -84,19 +84,31 @@ describe('config listeners', () => {
         beforeEach(async () => {
           origValue = workspace.getConfiguration().get(setting.name);
           await workspace.getConfiguration().update(setting.name, setting.values[0]);
+          await wait();
           spy.resetHistory();
         });
 
         afterEach(async () => {
           await workspace.getConfiguration().update(setting.name, origValue);
+          await wait();
         });
 
         it(`should listen for changes to '${setting.name}'`, async () => {
           await workspace.getConfiguration().update(setting.name, setting.values[1]);
-          expect(spy.calledOnce).to.be.true;
+          await wait();
           expect(listener[setting.property]).to.eq(setting.values[1]);
+          expect(spy).to.have.been.calledOnce;
         });
       });
     });
   });
+
+  // Need to wait some time since the onDidChangeConfiguration listeners fire
+  // asynchronously and we sometimes need to wait for them to complete in
+  // order to have as successful test.
+  async function wait(ms = 50) {
+    return new Promise(resolve =>
+      setTimeout(resolve, ms)
+    );
+  }
 });