fix: surface correct error message for ambiguous sourcemap matches (#971)

* fix: surface correct error message for ambiguous sourcemap matches

Fixes #961

* Update test cases to be more natural statements

Co-authored-by: Justin Beckwith <[email protected]>
Co-authored-by: James McTavish <[email protected]>

Author: 3 people

src/agent/io/sourcemapper.ts

@@ -26,12 +26,25 @@ const readFilep = promisify(fs.readFile);
 
 /** @define {string} */ const MAP_EXT = '.map';
 
+/** Represents one source map file. */
 export interface MapInfoInput {
+  // The path of generated output file in the source map. For example, if
+  // "src/index1.ts" and "src/index2.ts" are generated into "dist/index.js" and
+  // "dist/index.js.map", then this field is "dist/index.js (relative to the
+  // process's working directory).
   outputFile: string;
+
+  // The source map's path (relative to the process's working directory). For
+  // the same example above, this field is "dist/index.js.map".
   mapFile: string;
+
+  // The SourceMapConsumer object after parsing the content in the mapFile.
   mapConsumer: sourceMap.SourceMapConsumer;
-  // the sources are in ascending order from
-  // shortest to longest
+
+  // The original sources in the source map. Each value is relative to the
+  // source map file. For the same example above, this field is
+  // ['../src/index1.ts', '../src/index2.ts']. the sources are in ascending
+  // order from shortest to longest.
   sources: string[];
 }
 
@@ -41,6 +54,12 @@ export interface MapInfoOutput {
   column?: number;
 }
 
+export class MultiFileMatchError implements Error {
+  readonly name = 'MultiFileMatchError';
+  readonly message = 'Error: matching multiple files';
+  constructor(readonly files: string[]) {}
+}
+
 /**
  * @param {!Map} infoMap The map that maps input source files to
  *  SourceMapConsumer objects that are used to calculate mapping information
@@ -167,13 +186,22 @@ export class SourceMapper {
    * source file provided there isn't any ambiguity with associating the input
    * path to exactly one output transpiled file.
    *
-   * @param inputPath The (possibly relative) path to the original source file.
+   * If there are more than one matches, throw the error to include all the
+   * matched candidates.
+   *
+   * If there is no such mapping, it could be because the input file is not
+   * the input to a transpilation process or it is the input to a transpilation
+   * process but its corresponding .map file was not given to the constructor of
+   * this mapper.
+   *
+   * @param inputPath The path to an input file that could possibly be the input
+   *     to a transpilation process.
+   *  The path can be relative to the process's current working directory.
    * @return The `MapInfoInput` object that describes the transpiled file
-   *  associated with the specified input path.  `null` is returned if either
-   *  zero files are associated with the input path or if more than one file
-   *  could possibly be associated with the given input path.
+   *  associated with the specified input path. `null` is returned if there is
+   *  no files that are associated with the input path.
    */
-  private getMappingInfo(inputPath: string): MapInfoInput | null {
+  getMapInfoInput(inputPath: string): MapInfoInput | null {
     if (this.infoMap.has(path.normalize(inputPath))) {
       return this.infoMap.get(inputPath) as MapInfoInput;
     }
@@ -182,30 +210,17 @@ export class SourceMapper {
       inputPath,
       Array.from(this.infoMap.keys())
     );
+
     if (matches.length === 1) {
       return this.infoMap.get(matches[0]) as MapInfoInput;
     }
+    if (matches.length > 1) {
+      throw new MultiFileMatchError(matches);
+    }
 
     return null;
   }
 
-  /**
-   * Used to determine if the source file specified by the given path has
-   * a .map file and an output file associated with it.
-   *
-   * If there is no such mapping, it could be because the input file is not
-   * the input to a transpilation process or it is the input to a transpilation
-   * process but its corresponding .map file was not given to the constructor
-   * of this mapper.
-   *
-   * @param {string} inputPath The path to an input file that could
-   *  possibly be the input to a transpilation process.  The path should be
-   *  relative to the process's current working directory.
-   */
-  hasMappingInfo(inputPath: string): boolean {
-    return this.getMappingInfo(inputPath) !== null;
-  }
-
   /**
    * @param {string} inputPath The path to an input file that could possibly
    *  be the input to a transpilation process.  The path should be relative to
@@ -225,20 +240,18 @@ export class SourceMapper {
    *   If the given input file does not have mapping information associated
    *   with it then null is returned.
    */
-  mappingInfo(
+  getMapInfoOutput(
     inputPath: string,
     lineNumber: number,
-    colNumber: number
+    colNumber: number,
+    entry: MapInfoInput
   ): MapInfoOutput | null {
     inputPath = path.normalize(inputPath);
-    const entry = this.getMappingInfo(inputPath);
-    if (entry === null) {
-      return null;
-    }
 
     const relPath = path
       .relative(path.dirname(entry.mapFile), inputPath)
       .replace(/\\/g, '/');
+
     /**
      * Note: Since `entry.sources` is in ascending order from shortest
      *       to longest, the first source path that ends with the

src/agent/v8/inspector-debugapi.ts

@@ -23,7 +23,11 @@ import consoleLogLevel = require('console-log-level');
 import * as stackdriver from '../../types/stackdriver';
 import {ResolvedDebugAgentConfig} from '../config';
 import {FileStats, ScanStats} from '../io/scanner';
-import {MapInfoOutput, SourceMapper} from '../io/sourcemapper';
+import {
+  MapInfoOutput,
+  SourceMapper,
+  MultiFileMatchError,
+} from '../io/sourcemapper';
 import * as state from '../state/inspector-state';
 import * as utils from '../util/utils';
 
@@ -159,7 +163,26 @@ export class InspectorDebugApi implements debugapi.DebugApi {
       );
     }
     const baseScriptPath = path.normalize(breakpoint.location.path);
-    if (!this.sourcemapper.hasMappingInfo(baseScriptPath)) {
+    let mapInfoInput = null;
+    try {
+      mapInfoInput = this.sourcemapper.getMapInfoInput(baseScriptPath);
+    } catch (error) {
+      if (error instanceof MultiFileMatchError) {
+        this.logger.warn(
+          `Unable to unambiguously find ${baseScriptPath}. Multiple matches: ${error.files}`
+        );
+        return utils.setErrorStatusAndCallback(
+          cb,
+          breakpoint,
+          StatusMessage.BREAKPOINT_SOURCE_LOCATION,
+          utils.messages.SOURCE_FILE_AMBIGUOUS
+        );
+      } else {
+        throw error;
+      }
+    }
+
+    if (mapInfoInput === null) {
       const extension = path.extname(baseScriptPath);
       if (!this.config.javascriptFileExtensions.includes(extension)) {
         return utils.setErrorStatusAndCallback(
@@ -174,10 +197,11 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     } else {
       const line = breakpoint.location.line;
       const column = 0;
-      const mapInfo = this.sourcemapper.mappingInfo(
+      const mapInfo = this.sourcemapper.getMapInfoOutput(
         baseScriptPath,
         line,
-        column
+        column,
+        mapInfoInput
       );
 
       const compile = utils.getBreakpointCompiler(breakpoint);

src/agent/v8/legacy-debugapi.ts

@@ -129,7 +129,8 @@ export class V8DebugApi implements debugapi.DebugApi {
       );
     }
     const baseScriptPath = path.normalize(breakpoint.location.path);
-    if (!this.sourcemapper.hasMappingInfo(baseScriptPath)) {
+    const mapInfoInput = this.sourcemapper.getMapInfoInput(baseScriptPath);
+    if (mapInfoInput === null) {
       const extension = path.extname(baseScriptPath);
       if (!this.config.javascriptFileExtensions.includes(extension)) {
         return utils.setErrorStatusAndCallback(
@@ -143,10 +144,11 @@ export class V8DebugApi implements debugapi.DebugApi {
     } else {
       const line = breakpoint.location.line;
       const column = 0;
-      const mapInfo = this.sourcemapper.mappingInfo(
+      const mapInfo = this.sourcemapper.getMapInfoOutput(
         baseScriptPath,
         line,
-        column
+        column,
+        mapInfoInput
       );
       const compile = utils.getBreakpointCompiler(breakpoint);
       if (breakpoint.condition && compile) {
@@ -168,6 +170,7 @@ export class V8DebugApi implements debugapi.DebugApi {
       this.setInternal(breakpoint, mapInfo, compile, cb);
     }
   }
+
   clear(
     breakpoint: stackdriver.Breakpoint,
     cb: (err: Error | null) => void

test/test-sourcemapper.ts

@@ -68,9 +68,9 @@ function testTool(
         tool +
         ' it states that it has mapping info for files it knows about',
       done => {
-        assert.strictEqual(
-          sourcemapper.hasMappingInfo(inputFilePath),
-          true,
+        assert.notStrictEqual(
+          sourcemapper.getMapInfoInput(inputFilePath),
+          null,
           `The sourcemapper should have information about '${inputFilePath}'`
         );
         done();
@@ -83,17 +83,17 @@ function testTool(
         ' it states that it has mapping info for files with a path' +
         ' similar to a path it knows about',
       done => {
-        assert.strictEqual(
-          sourcemapper.hasMappingInfo(relativeInputFilePath),
-          true
+        assert.notStrictEqual(
+          sourcemapper.getMapInfoInput(inputFilePath),
+          null
         );
         const movedPath = path.join(
           '/some/other/base/dir/',
           relativeInputFilePath
         );
-        assert.strictEqual(
-          sourcemapper.hasMappingInfo(movedPath),
-          true,
+        assert.notStrictEqual(
+          sourcemapper.getMapInfoInput(inputFilePath),
+          null,
           `The sourcemapper should have information about paths similar to '${movedPath}'`
         );
         done();
@@ -108,16 +108,23 @@ function testTool(
       done => {
         const invalidPath = inputFilePath + '_INVALID';
         assert.strictEqual(
-          sourcemapper.hasMappingInfo(invalidPath),
-          false,
+          sourcemapper.getMapInfoInput(invalidPath),
+          null,
           `The source mapper should not have information the path '${invalidPath}' it doesn't recognize`
         );
         done();
       }
     );
 
     const testLineMapping = (inputLine: number, expectedOutputLine: number) => {
-      const info = sourcemapper.mappingInfo(inputFilePath, inputLine, 0);
+      const mapInfoInput = sourcemapper.getMapInfoInput(inputFilePath);
+      assert.notEqual(mapInfoInput, null);
+      const info = sourcemapper.getMapInfoOutput(
+        inputFilePath,
+        inputLine,
+        0,
+        mapInfoInput!
+      );
       assert.notStrictEqual(
         info,
         null,

test/test-v8debugapi.ts

@@ -412,7 +412,7 @@ describe('v8debugapi', () => {
       });
     });
 
-    it('should reject breakpoint when filename is ambiguous', done => {
+    it('should reject breakpoint when javascript file is ambiguous', done => {
       require('./fixtures/a/hello.js');
       require('./fixtures/b/hello.js');
       // TODO(dominickramer): Have this actually implement Breakpoint
@@ -458,6 +458,32 @@ describe('v8debugapi', () => {
       });
     });
 
+    it('should reject breakpoint when source mapping is ambiguous', done => {
+      const bp: stackdriver.Breakpoint = {
+        id: 'ambiguous',
+        location: {line: 1, path: 'in.ts'},
+      } as {} as stackdriver.Breakpoint;
+      api.set(bp, err => {
+        assert.ok(err);
+        assert.ok(bp.status);
+        assert.ok(bp.status instanceof StatusMessage);
+        assert.ok(bp.status!.isError);
+        assert(
+          bp.status!.description.format === utils.messages.SOURCE_FILE_AMBIGUOUS
+        );
+
+        // Verify that a warning log message is emitted.
+        assert.strictEqual(
+          logger.warns.length,
+          1,
+          `Expected 1 warning log message, got ${logger.allCalls.length}`
+        );
+        const message = logger.warns[0].args[0];
+        assert.notStrictEqual(message.indexOf('Multiple matches:'), -1);
+        done();
+      });
+    });
+
     it('should reject breakpoint on non-existent line', done => {
       require('./fixtures/foo.js');
       // TODO(dominickramer): Have this actually implement Breakpoint