Merge pull request #1235 from github/aeisenberg/history-sort

Add query history sorting for remote queries

Author: aeisenberg

extensions/ql-vscode/CHANGELOG.md

@@ -3,6 +3,8 @@
 ## [UNRELEASED]
 
 - Fix a bug where the AST viewer was not synchronizing its selected node when the editor selection changes. [#1230](https://github.com/github/vscode-codeql/pull/1230)
+- Open the directory in the finder/explorer (instead of just highlighting it) when running the "Open query directory" command from the query history view. [#1235](https://github.com/github/vscode-codeql/pull/1235)
+- Ensure query label in the query history view changes are persisted across restarts. [#1235](https://github.com/github/vscode-codeql/pull/1235)
 
 ## 1.6.1 - 17 March 2022
 

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

@@ -1,4 +1,5 @@
 import * as path from 'path';
+import * as fs from 'fs-extra';
 import {
   commands,
   Disposable,
@@ -28,7 +29,7 @@ import { URLSearchParams } from 'url';
 import { QueryServerClient } from './queryserver-client';
 import { DisposableObject } from './pure/disposable-object';
 import { commandRunner } from './commandRunner';
-import { assertNever, ONE_HOUR_IN_MS, TWO_HOURS_IN_MS, getErrorMessage, getErrorStack  } from './pure/helpers-pure';
+import { assertNever, ONE_HOUR_IN_MS, TWO_HOURS_IN_MS, getErrorMessage, getErrorStack } from './pure/helpers-pure';
 import { CompletedLocalQueryInfo, LocalQueryInfo as LocalQueryInfo, QueryHistoryInfo } from './query-results';
 import { DatabaseManager } from './databases';
 import { registerQueryHistoryScubber } from './query-history-scrubber';
@@ -183,38 +184,48 @@ export class HistoryTreeDataProvider extends DisposableObject {
   ): ProviderResult<QueryHistoryInfo[]> {
     return element ? [] : this.history.sort((h1, h2) => {
 
-      // TODO remote queries are not implemented yet.
-      if (h1.t !== 'local' && h2.t !== 'local') {
-        return 0;
-      }
-      if (h1.t !== 'local') {
-        return -1;
-      }
-      if (h2.t !== 'local') {
-        return 1;
-      }
+      const h1Label = h1.label.toLowerCase();
+      const h2Label = h2.label.toLowerCase();
 
-      const resultCount1 = h1.completedQuery?.resultCount ?? -1;
-      const resultCount2 = h2.completedQuery?.resultCount ?? -1;
+      const h1Date = h1.t === 'local'
+        ? h1.initialInfo.start.getTime()
+        : h1.remoteQuery?.executionStartTime;
+
+      const h2Date = h2.t === 'local'
+        ? h2.initialInfo.start.getTime()
+        : h2.remoteQuery?.executionStartTime;
+
+      // result count for remote queries is not available here.
+      const resultCount1 = h1.t === 'local'
+        ? h1.completedQuery?.resultCount ?? -1
+        : -1;
+      const resultCount2 = h2.t === 'local'
+        ? h2.completedQuery?.resultCount ?? -1
+        : -1;
 
       switch (this.sortOrder) {
         case SortOrder.NameAsc:
-          return h1.label.localeCompare(h2.label, env.language);
+          return h1Label.localeCompare(h2Label, env.language);
+
         case SortOrder.NameDesc:
-          return h2.label.localeCompare(h1.label, env.language);
+          return h2Label.localeCompare(h1Label, env.language);
+
         case SortOrder.DateAsc:
-          return h1.initialInfo.start.getTime() - h2.initialInfo.start.getTime();
+          return h1Date - h2Date;
+
         case SortOrder.DateDesc:
-          return h2.initialInfo.start.getTime() - h1.initialInfo.start.getTime();
+          return h2Date - h1Date;
+
         case SortOrder.CountAsc:
           // If the result counts are equal, sort by name.
           return resultCount1 - resultCount2 === 0
-            ? h1.label.localeCompare(h2.label, env.language)
+            ? h1Label.localeCompare(h2Label, env.language)
             : resultCount1 - resultCount2;
+
         case SortOrder.CountDesc:
           // If the result counts are equal, sort by name.
           return resultCount2 - resultCount1 === 0
-            ? h2.label.localeCompare(h1.label, env.language)
+            ? h2Label.localeCompare(h1Label, env.language)
             : resultCount2 - resultCount1;
         default:
           assertNever(this.sortOrder);
@@ -650,7 +661,7 @@ export class QueryHistoryManager extends DisposableObject {
     if (response !== undefined) {
       // Interpret empty string response as 'go back to using default'
       finalSingleItem.initialInfo.userSpecifiedLabel = response === '' ? undefined : response;
-      this.treeDataProvider.refresh();
+      await this.refreshTreeView();
     }
   }
 
@@ -741,20 +752,28 @@ export class QueryHistoryManager extends DisposableObject {
       return;
     }
 
-    let p: string | undefined;
+    let externalFilePath: string | undefined;
     if (finalSingleItem.t === 'local') {
       if (finalSingleItem.completedQuery) {
-        p = finalSingleItem.completedQuery.query.querySaveDir;
+        externalFilePath = path.join(finalSingleItem.completedQuery.query.querySaveDir, 'timestamp');
       }
     } else if (finalSingleItem.t === 'remote') {
-      p = path.join(this.queryStorageDir, finalSingleItem.queryId);
+      externalFilePath = path.join(this.queryStorageDir, finalSingleItem.queryId, 'timestamp');
     }
 
-    if (p) {
+    if (externalFilePath) {
+      if (!(await fs.pathExists(externalFilePath))) {
+        // timestamp file is missing (manually deleted?) try selecting the parent folder.
+        // It's less nice, but at least it will work.
+        externalFilePath = path.dirname(externalFilePath);
+        if (!(await fs.pathExists(externalFilePath))) {
+          throw new Error(`Query directory does not exist: ${externalFilePath}`);
+        }
+      }
       try {
-        await commands.executeCommand('revealFileInOS', Uri.file(p));
+        await commands.executeCommand('revealFileInOS', Uri.file(externalFilePath));
       } catch (e) {
-        throw new Error(`Failed to open ${p}: ${getErrorMessage(e)}`);
+        throw new Error(`Failed to open ${externalFilePath}: ${getErrorMessage(e)}`);
       }
     }
   }

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

@@ -427,9 +427,11 @@ describe('query-history', () => {
 
   describe('getChildren', () => {
     const history = [
-      item('a', 10, 20),
-      item('b', 5, 30),
-      item('c', 1, 25),
+      item('a', 2, 'remote'),
+      item('b', 10, 'local', 20),
+      item('c', 5, 'local', 30),
+      item('d', 1, 'local', 25),
+      item('e', 6, 'remote'),
     ];
     let treeDataProvider: HistoryTreeDataProvider;
 
@@ -456,31 +458,31 @@ describe('query-history', () => {
     });
 
     it('should get children for date ascending', async () => {
-      const expected = [history[2], history[1], history[0]];
+      const expected = [history[3], history[0], history[2], history[4], history[1]];
       treeDataProvider.sortOrder = SortOrder.DateAsc;
 
       const children = await treeDataProvider.getChildren();
       expect(children).to.deep.eq(expected);
     });
 
     it('should get children for date descending', async () => {
-      const expected = [history[0], history[1], history[2]];
+      const expected = [history[3], history[0], history[2], history[4], history[1]].reverse();
       treeDataProvider.sortOrder = SortOrder.DateDesc;
 
       const children = await treeDataProvider.getChildren();
       expect(children).to.deep.eq(expected);
     });
 
     it('should get children for result count ascending', async () => {
-      const expected = [history[0], history[2], history[1]];
+      const expected = [history[0], history[4], history[1], history[3], history[2]];
       treeDataProvider.sortOrder = SortOrder.CountAsc;
 
       const children = await treeDataProvider.getChildren();
       expect(children).to.deep.eq(expected);
     });
 
     it('should get children for result count descending', async () => {
-      const expected = [history[1], history[2], history[0]];
+      const expected = [history[0], history[4], history[1], history[3], history[2]].reverse();
       treeDataProvider.sortOrder = SortOrder.CountDesc;
 
       const children = await treeDataProvider.getChildren();
@@ -509,17 +511,27 @@ describe('query-history', () => {
       expect(children).to.deep.eq(expected);
     });
 
-    function item(label: string, start: number, resultCount?: number) {
-      return {
-        label,
-        initialInfo: {
-          start: new Date(start),
-        },
-        completedQuery: {
-          resultCount,
-        },
-        t: 'local'
-      };
+    function item(label: string, start: number, t = 'local', resultCount?: number) {
+      if (t === 'local') {
+        return {
+          label,
+          initialInfo: {
+            start: new Date(start),
+          },
+          completedQuery: {
+            resultCount,
+          },
+          t
+        };
+      } else {
+        return {
+          label,
+          remoteQuery: {
+            executionStartTime: start,
+          },
+          t
+        };
+      }
     }
   });