Return undefined for finding file ranges on empty URI

Also, refactor resolveSourceFile to make it easier to read.
And add unit tests for resolveSourceFile.

This commit fixes a bug in resolveSourceFile where the 
`pathWithinSourceArchive` was being removed and appended to the
`sourceArchiveZipPath`. In normal situations, we don't hit this bug
because most database source archive uris have an empty path for the
`pathWithinSourceArchive`.

Author: aeisenberg

extensions/ql-vscode/src/bqrs-utils.ts

@@ -68,7 +68,7 @@ function isWholeFileMatch(matches: RegExpExecArray): boolean {
  *
  * @param uri A file uri
  */
-function isEmptyPath(uriStr: string) {
+export function isEmptyPath(uriStr: string) {
   return !uriStr || uriStr === 'file:/';
 }
 

extensions/ql-vscode/src/contextual/fileRangeFromURI.ts

@@ -1,6 +1,7 @@
 import * as vscode from 'vscode';
 
 import { UrlValue, LineColumnLocation } from '../bqrs-cli-types';
+import { isEmptyPath } from '../bqrs-utils';
 import { DatabaseItem } from '../databases';
 
 
@@ -11,6 +12,9 @@ export default function fileRangeFromURI(uri: UrlValue | undefined, db: Database
     return undefined;
   } else {
     const loc = uri as LineColumnLocation;
+    if (isEmptyPath(loc.uri)) {
+      return undefined;
+    }
     const range = new vscode.Range(Math.max(0, (loc.startLine || 0) - 1),
       Math.max(0, (loc.startColumn || 0) - 1),
       Math.max(0, (loc.endLine || 0) - 1),

extensions/ql-vscode/src/databases.ts

@@ -267,7 +267,8 @@ export interface DatabaseChangedEvent {
   item: DatabaseItem | undefined;
 }
 
-class DatabaseItemImpl implements DatabaseItem {
+// Exported for testing
+export class DatabaseItemImpl implements DatabaseItem {
   private _error: Error | undefined = undefined;
   private _contents: DatabaseContents | undefined;
   /** A cache of database info */
@@ -301,8 +302,7 @@ class DatabaseItemImpl implements DatabaseItem {
   public get sourceArchive(): vscode.Uri | undefined {
     if (this.options.ignoreSourceArchive || (this._contents === undefined)) {
       return undefined;
-    }
-    else {
+    } else {
       return this._contents.sourceArchiveUri;
     }
   }
@@ -341,42 +341,42 @@ class DatabaseItemImpl implements DatabaseItem {
 
   public resolveSourceFile(uri: string | undefined): vscode.Uri {
     const sourceArchive = this.sourceArchive;
-    // FIXME: This is wrong. Should parse the uri properly first
+    // Sometimes, we are passed a path, sometimes a file URI.
+    // We need to convert this to a file path that is probably inside of a zip file.
     const file = uri?.replace(/file:/, '');
-    if (sourceArchive === undefined) {
-      if (file !== undefined) {
+    if (!sourceArchive) {
+      if (file) {
         // Treat it as an absolute path.
         return vscode.Uri.file(file);
-      }
-      else {
+      } else {
         return this.databaseUri;
       }
     }
-    else {
-      if (file !== undefined) {
-        const absoluteFilePath = file.replace(':', '_');
-        // Strip any leading slashes from the file path, and replace `:` with `_`.
-        const relativeFilePath = absoluteFilePath.replace(/^\/*/, '').replace(':', '_');
-        if (sourceArchive.scheme == zipArchiveScheme) {
-          return encodeSourceArchiveUri({
-            pathWithinSourceArchive: absoluteFilePath,
-            sourceArchiveZipPath: sourceArchive.fsPath,
-          });
-        }
-        else {
-          let newPath = sourceArchive.path;
-          if (!newPath.endsWith('/')) {
-            // Ensure a trailing slash.
-            newPath += '/';
-          }
-          newPath += relativeFilePath;
 
-          return sourceArchive.with({ path: newPath });
+    if (file) {
+      const absoluteFilePath = file.replace(':', '_');
+      // Strip any leading slashes from the file path, and replace `:` with `_`.
+      const relativeFilePath = absoluteFilePath.replace(/^\/*/, '').replace(':', '_');
+      if (sourceArchive.scheme === zipArchiveScheme) {
+        const zipRef = decodeSourceArchiveUri(sourceArchive);
+        return encodeSourceArchiveUri({
+          pathWithinSourceArchive: zipRef.pathWithinSourceArchive + '/' + absoluteFilePath,
+          sourceArchiveZipPath: zipRef.sourceArchiveZipPath,
+        });
+
+      } else {
+        let newPath = sourceArchive.path;
+        if (!newPath.endsWith('/')) {
+          // Ensure a trailing slash.
+          newPath += '/';
         }
+        newPath += relativeFilePath;
+
+        return sourceArchive.with({ path: newPath });
       }
-      else {
-        return sourceArchive;
-      }
+
+    } else {
+      return sourceArchive;
     }
   }
 

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

@@ -12,6 +12,16 @@ describe('fileRangeFromURI', () => {
     expect(fileRangeFromURI('hucairz', createMockDatabaseItem())).to.be.undefined;
   });
 
+  it('should return undefined when value is an empty uri', () => {
+    expect(fileRangeFromURI({
+      uri: 'file:/',
+      startLine: 1,
+      startColumn: 2,
+      endLine: 3,
+      endColumn: 4,
+    } as LineColumnLocation, createMockDatabaseItem())).to.be.undefined;
+  });
+
   it('should return a range for a WholeFileLocation', () => {
     expect(fileRangeFromURI({
       uri: 'file:///hucairz',

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

@@ -2,11 +2,18 @@ import 'vscode-test';
 import 'mocha';
 import * as sinon from 'sinon';
 import { expect } from 'chai';
-import { ExtensionContext } from 'vscode';
+import { ExtensionContext, Uri } from 'vscode';
 
-import { DatabaseEventKind, DatabaseItem, DatabaseManager } from '../../databases';
+import {
+  DatabaseEventKind,
+  DatabaseItem,
+  DatabaseManager,
+  DatabaseItemImpl,
+  DatabaseContents
+} from '../../databases';
 import { QueryServerConfig } from '../../config';
 import { Logger } from '../../logging';
+import { encodeSourceArchiveUri } from '../../archive-filesystem-provider';
 
 describe('databases', () => {
   let databaseManager: DatabaseManager;
@@ -78,4 +85,105 @@ describe('databases', () => {
       kind: DatabaseEventKind.Rename
     });
   });
+
+  describe('resolveSourceFile', () => {
+    describe('unzipped source archive', () => {
+      it('should resolve a source file in an unzipped database', () => {
+        const db = createMockDB();
+        const resolved = db.resolveSourceFile('abc');
+        expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc');
+      });
+
+      it('should resolve a source file in an unzipped database with trailing slash', () => {
+        const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+        const resolved = db.resolveSourceFile('abc');
+        expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc');
+      });
+
+      it('should resolve a source uri in an unzipped database with trailing slash', () => {
+        const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+        const resolved = db.resolveSourceFile('file:/abc');
+        expect(resolved.toString()).to.eq('file:///sourceArchive-uri/abc');
+      });
+    });
+
+    describe('no source archive', () => {
+      it('should resolve a file', () => {
+        const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+        (db as any)._contents.sourceArchiveUri = undefined;
+        const resolved = db.resolveSourceFile('abc');
+        expect(resolved.toString()).to.eq('file:///abc');
+      });
+
+      it('should resolve an empty file', () => {
+        const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+        (db as any)._contents.sourceArchiveUri = undefined;
+        const resolved = db.resolveSourceFile('file:');
+        expect(resolved.toString()).to.eq('file:///database-uri');
+      });
+    });
+
+    describe('zipped source archive', () => {
+      it('should encode a source archive url', () => {
+        const db = createMockDB(encodeSourceArchiveUri({
+          sourceArchiveZipPath: 'sourceArchive-uri',
+          pathWithinSourceArchive: 'def'
+        }));
+        const resolved = db.resolveSourceFile(Uri.file('abc').toString());
+
+        // must recreate an encoded archive uri instead of typing out the
+        // text since the uris will be different on windows and ubuntu.
+        expect(resolved.toString()).to.eq(encodeSourceArchiveUri({
+          sourceArchiveZipPath: 'sourceArchive-uri',
+          pathWithinSourceArchive: 'def/abc'
+        }).toString());
+      });
+
+      it('should encode a source archive url with trailing slash', () => {
+        const db = createMockDB(encodeSourceArchiveUri({
+          sourceArchiveZipPath: 'sourceArchive-uri',
+          pathWithinSourceArchive: 'def/'
+        }));
+        const resolved = db.resolveSourceFile(Uri.file('abc').toString());
+
+        // must recreate an encoded archive uri instead of typing out the
+        // text since the uris will be different on windows and ubuntu.
+        expect(resolved.toString()).to.eq(encodeSourceArchiveUri({
+          sourceArchiveZipPath: 'sourceArchive-uri',
+          pathWithinSourceArchive: 'def/abc'
+        }).toString());
+      });
+
+      it('should encode an empty source archive url', () => {
+        const db = createMockDB(encodeSourceArchiveUri({
+          sourceArchiveZipPath: 'sourceArchive-uri',
+          pathWithinSourceArchive: 'def'
+        }));
+        const resolved = db.resolveSourceFile('file:');
+        expect(resolved.toString()).to.eq('codeql-zip-archive://1-18/sourceArchive-uri/def');
+      });
+    });
+
+    it('should handle an empty file', () => {
+      const db = createMockDB(Uri.parse('file:/sourceArchive-uri/'));
+      const resolved = db.resolveSourceFile('');
+      expect(resolved.toString()).to.eq('file:///sourceArchive-uri/');
+    });
+    function createMockDB(
+      sourceArchiveUri = Uri.parse('file:/sourceArchive-uri'),
+      databaseUri = Uri.parse('file:/database-uri')
+    ) {
+      return new DatabaseItemImpl(
+        databaseUri,
+        {
+          sourceArchiveUri
+        } as DatabaseContents,
+        {
+          dateAdded: 123,
+          ignoreSourceArchive: false
+        },
+        () => { /**/ }
+      );
+    }
+  });
 });