refactor: ensure paths type safely passed around in ng-update

Schematics using the TypeScript compiler API have a significant issue
with paths. The problem is that the TypeScript compiler API intends to
rely fully on absolute paths, while ng-update and the devkit tree intend
to rely on project-scoped/relative paths.

This is problematic as it is straightforward to mix paths up, leading to
unexpected errors. For example, computing a relative path between a devkit
path and an absolute disk path results in a non-existent path.

To fix this ambiguity, we improve type safety of paths by using branded
primitives such as the `@angular-devkit/core` `Path` or an even more
strict type called `WorkspacePath`. A normal string is considered an
unresolved path and cannot be assigned to a `WorkspacePath`. Similarly a
`WorkspacePath` cannot be assigned to a `string`. This ensures that
resolved paths are not accidentally mixed with unresolved paths, and
that we can safely pass around unresolved and resolved paths without
loss of their semantics.

This supposedly fixes #19270,
where a devkit path and an absolute disk path result in an incorrectly
resolved resource. In CLI apps it's possible to reference resources
through absolute project-scoped paths. e.g. `/src/app/app.component.html`

Author: devversion

src/cdk/schematics/ng-update/devkit-file-system.ts

@@ -6,32 +6,45 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {normalize} from '@angular-devkit/core';
+import {normalize, Path, relative} from '@angular-devkit/core';
 import {Tree, UpdateRecorder} from '@angular-devkit/schematics';
-import {relative} from 'path';
+import * as path from 'path';
 import {FileSystem} from '../update-tool/file-system';
 
 /**
  * File system that leverages the virtual tree from the CLI devkit. This file
  * system is commonly used by `ng update` migrations that run as part of the
  * Angular CLI.
  */
-export class DevkitFileSystem implements FileSystem {
+export class DevkitFileSystem extends FileSystem<Path> {
   private _updateRecorderCache = new Map<string, UpdateRecorder>();
+  private _workspaceFsPath: Path;
 
-  constructor(private _tree: Tree, private _workspaceFsPath: string) {}
+  constructor(private _tree: Tree, workspaceFsPath: string) {
+    super();
+    this._workspaceFsPath = normalize(workspaceFsPath);
+  }
 
-  resolve(fsFilePath: string) {
-    return normalize(relative(this._workspaceFsPath, fsFilePath)) as string;
+  resolve(...segments: string[]): Path {
+    // Note: We use `posix.resolve` as the devkit paths are using posix separators.
+    const resolvedPath = normalize(path.posix.resolve(...segments.map(normalize)));
+    // If the resolved path points to the workspace root, then this is an absolute disk
+    // path and we need to compute a devkit tree relative path.
+    if (resolvedPath.startsWith(this._workspaceFsPath)) {
+      return relative(this._workspaceFsPath, resolvedPath);
+    }
+    // Otherwise we know that the path is absolute (due to the resolve), and that it
+    // refers to an absolute devkit tree path (like `/angular.json`). We keep those
+    // unmodified as they are already resolved workspace paths.
+    return resolvedPath;
   }
 
-  edit(fsFilePath: string) {
-    const treeFilePath = this.resolve(fsFilePath);
-    if (this._updateRecorderCache.has(treeFilePath)) {
-      return this._updateRecorderCache.get(treeFilePath)!;
+  edit(filePath: Path) {
+    if (this._updateRecorderCache.has(filePath)) {
+      return this._updateRecorderCache.get(filePath)!;
     }
-    const recorder = this._tree.beginUpdate(treeFilePath);
-    this._updateRecorderCache.set(treeFilePath, recorder);
+    const recorder = this._tree.beginUpdate(filePath);
+    this._updateRecorderCache.set(filePath, recorder);
     return recorder;
   }
 
@@ -40,24 +53,24 @@ export class DevkitFileSystem implements FileSystem {
     this._updateRecorderCache.clear();
   }
 
-  exists(fsFilePath: string) {
-    return this._tree.exists(this.resolve(fsFilePath));
+  exists(filePath: Path) {
+    return this._tree.exists(filePath);
   }
 
-  overwrite(fsFilePath: string, content: string) {
-    this._tree.overwrite(this.resolve(fsFilePath), content);
+  overwrite(filePath: Path, content: string) {
+    this._tree.overwrite(filePath, content);
   }
 
-  create(fsFilePath: string, content: string) {
-    this._tree.create(this.resolve(fsFilePath), content);
+  create(filePath: Path, content: string) {
+    this._tree.create(filePath, content);
   }
 
-  delete(fsFilePath: string) {
-    this._tree.delete(this.resolve(fsFilePath));
+  delete(filePath: Path) {
+    this._tree.delete(filePath);
   }
 
-  read(fsFilePath: string) {
-    const buffer = this._tree.read(this.resolve(fsFilePath));
+  read(filePath: Path) {
+    const buffer = this._tree.read(filePath);
     return buffer !== null ? buffer.toString() : null;
   }
 }

src/cdk/schematics/ng-update/devkit-migration-rule.ts

@@ -15,6 +15,7 @@ import {join} from 'path';
 import {UpdateProject} from '../update-tool';
 import {MigrationCtor} from '../update-tool/migration';
 import {TargetVersion} from '../update-tool/target-version';
+import {WorkspacePath} from '../update-tool/file-system';
 import {getTargetTsconfigPath, getWorkspaceConfigGracefully} from '../utils/project-tsconfig-paths';
 
 import {DevkitFileSystem} from './devkit-file-system';
@@ -72,7 +73,7 @@ export function createMigrationSchematicRule(
     // Keep track of all project source files which have been checked/migrated. This is
     // necessary because multiple TypeScript projects can contain the same source file and
     // we don't want to check these again, as this would result in duplicated failure messages.
-    const analyzedFiles = new Set<string>();
+    const analyzedFiles = new Set<WorkspacePath>();
     // The CLI uses the working directory as the base directory for the virtual file system tree.
     const workspaceFsPath = process.cwd();
     const fileSystem = new DevkitFileSystem(tree, workspaceFsPath);

src/cdk/schematics/ng-update/migrations/attribute-selectors.ts

@@ -7,6 +7,7 @@
  */
 
 import * as ts from 'typescript';
+import {WorkspacePath} from '../../update-tool/file-system';
 import {ResolvedResource} from '../../update-tool/component-resource-collector';
 import {Migration} from '../../update-tool/migration';
 import {AttributeSelectorUpgradeData} from '../data/attribute-selectors';
@@ -63,11 +64,13 @@ export class AttributeSelectorsMigration extends Migration<UpgradeData> {
     this.data.forEach(selector => {
       findAllSubstringIndices(literalText, selector.replace)
           .map(offset => literal.getStart() + offset)
-          .forEach(start => this._replaceSelector(filePath, start, selector));
+          .forEach(start => this._replaceSelector(
+              this.fileSystem.resolve(filePath), start, selector));
     });
   }
 
-  private _replaceSelector(filePath: string, start: number, data: AttributeSelectorUpgradeData) {
+  private _replaceSelector(filePath: WorkspacePath, start: number,
+                           data: AttributeSelectorUpgradeData) {
     this.fileSystem.edit(filePath)
       .remove(start, data.replace.length)
       .insertRight(start, data.replaceWith);

src/cdk/schematics/ng-update/migrations/class-names.ts

@@ -97,8 +97,9 @@ export class ClassNamesMigration extends Migration<UpgradeData> {
   /** Creates a failure and replacement for the specified identifier. */
   private _createFailureWithReplacement(identifier: ts.Identifier) {
     const classData = this.data.find(data => data.replace === identifier.text)!;
+    const filePath = this.fileSystem.resolve(identifier.getSourceFile().fileName);
 
-    this.fileSystem.edit(identifier.getSourceFile().fileName)
+    this.fileSystem.edit(filePath)
       .remove(identifier.getStart(), identifier.getWidth())
       .insertRight(identifier.getStart(), classData.replaceWith);
   }

src/cdk/schematics/ng-update/migrations/css-selectors.ts

@@ -7,6 +7,7 @@
  */
 
 import * as ts from 'typescript';
+import {WorkspacePath} from '../../update-tool/file-system';
 import {ResolvedResource} from '../../update-tool/component-resource-collector';
 import {Migration} from '../../update-tool/migration';
 import {CssSelectorUpgradeData} from '../data/css-selectors';
@@ -69,11 +70,11 @@ export class CssSelectorsMigration extends Migration<UpgradeData> {
 
       findAllSubstringIndices(textContent, data.replace)
           .map(offset => node.getStart() + offset)
-          .forEach(start => this._replaceSelector(filePath, start, data));
+          .forEach(start => this._replaceSelector(this.fileSystem.resolve(filePath), start, data));
     });
   }
 
-  private _replaceSelector(filePath: string, start: number, data: CssSelectorUpgradeData) {
+  private _replaceSelector(filePath: WorkspacePath, start: number, data: CssSelectorUpgradeData) {
     this.fileSystem.edit(filePath)
       .remove(start, data.replace.length)
       .insertRight(start, data.replaceWith);

src/cdk/schematics/ng-update/migrations/element-selectors.ts

@@ -8,6 +8,7 @@
 
 import * as ts from 'typescript';
 import {ResolvedResource} from '../../update-tool/component-resource-collector';
+import {WorkspacePath} from '../../update-tool/file-system';
 import {Migration} from '../../update-tool/migration';
 import {ElementSelectorUpgradeData} from '../data/element-selectors';
 import {findAllSubstringIndices} from '../typescript/literal';
@@ -57,11 +58,13 @@ export class ElementSelectorsMigration extends Migration<UpgradeData> {
     this.data.forEach(selector => {
       findAllSubstringIndices(textContent, selector.replace)
           .map(offset => node.getStart() + offset)
-          .forEach(start => this._replaceSelector(filePath, start, selector));
+          .forEach(start => this._replaceSelector(
+              this.fileSystem.resolve(filePath), start, selector));
     });
   }
 
-  private _replaceSelector(filePath: string, start: number, data: ElementSelectorUpgradeData) {
+  private _replaceSelector(filePath: WorkspacePath, start: number,
+                           data: ElementSelectorUpgradeData) {
     this.fileSystem.edit(filePath)
       .remove(start, data.replace.length)
       .insertRight(start, data.replaceWith);

src/cdk/schematics/ng-update/migrations/input-names.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {WorkspacePath} from '../../update-tool/file-system';
 import {findInputsOnElementWithAttr, findInputsOnElementWithTag} from '../html-parsing/angular';
 import {ResolvedResource} from '../../update-tool/component-resource-collector';
 import {Migration} from '../../update-tool/migration';
@@ -63,7 +64,8 @@ export class InputNamesMigration extends Migration<UpgradeData> {
     });
   }
 
-  private _replaceInputName(filePath: string, start: number, width: number, newName: string) {
+  private _replaceInputName(filePath: WorkspacePath, start: number, width: number,
+                            newName: string) {
     this.fileSystem.edit(filePath)
       .remove(start, width)
       .insertRight(start, newName);

src/cdk/schematics/ng-update/migrations/output-names.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {WorkspacePath} from '../../update-tool/file-system';
 import {ResolvedResource} from '../../update-tool/component-resource-collector';
 import {Migration} from '../../update-tool/migration';
 
@@ -49,7 +50,8 @@ export class OutputNamesMigration extends Migration<UpgradeData> {
     });
   }
 
-  private _replaceOutputName(filePath: string, start: number, width: number, newName: string) {
+  private _replaceOutputName(filePath: WorkspacePath, start: number, width: number,
+                             newName: string) {
     this.fileSystem.edit(filePath)
       .remove(start, width)
       .insertRight(start, newName);

src/cdk/schematics/ng-update/migrations/property-names.ts

@@ -39,7 +39,7 @@ export class PropertyNamesMigration extends Migration<UpgradeData> {
       }
 
       if (!data.whitelist || data.whitelist.classes.includes(typeName)) {
-        this.fileSystem.edit(node.getSourceFile().fileName)
+        this.fileSystem.edit(this.fileSystem.resolve(node.getSourceFile().fileName))
           .remove(node.name.getStart(), node.name.getWidth())
           .insertRight(node.name.getStart(), data.replaceWith);
       }

src/cdk/schematics/ng-update/test-cases/misc/external-resource-resolution.spec.ts

@@ -0,0 +1,30 @@
+import {resolveBazelPath} from '@angular/cdk/schematics/testing';
+import {MIGRATION_PATH} from '../../../index.spec';
+import {createTestCaseSetup} from '../../../testing';
+
+describe('ng-update external resource resolution', () => {
+
+  it('should properly resolve referenced resources in components', async () => {
+    const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
+      'migration-v6', MIGRATION_PATH,
+      [resolveBazelPath(__dirname, './external-resource-resolution_input.ts')]);
+
+    const testContent = `<div cdk-connected-overlay [origin]="test"></div>`;
+    const expected = `<div cdk-connected-overlay [cdkConnectedOverlayOrigin]="test"></div>`;
+
+    writeFile('/projects/material/test.html', testContent);
+    writeFile('/projects/cdk-testing/src/some-tmpl.html', testContent);
+    writeFile('/projects/cdk-testing/src/test-cases/local.html', testContent);
+
+    await runFixers();
+
+    expect(appTree.readContent('/projects/material/test.html'))
+        .toBe(expected, 'Expected absolute devkit tree paths to work.');
+    expect(appTree.readContent('/projects/cdk-testing/src/some-tmpl.html'))
+        .toBe(expected, 'Expected relative paths with parent segments to work.');
+    expect(appTree.readContent('/projects/cdk-testing/src/test-cases/local.html'))
+        .toBe(expected, 'Expected relative paths without explicit dot to work.');
+
+    removeTempDir();
+  });
+});

src/cdk/schematics/ng-update/test-cases/misc/external-resource-resolution_input.ts

@@ -0,0 +1,19 @@
+import {Component} from '@angular/core';
+
+@Component({
+  selector: 'test-cmp',
+  templateUrl: '/projects/material/test.html',
+})
+export class MyTestComp {}
+
+@Component({
+  selector: 'test-cmp2',
+  templateUrl: '../some-tmpl.html',
+})
+export class MyTestComp2 {}
+
+@Component({
+  selector: 'test-cmp3',
+  templateUrl: 'local.html',
+})
+export class MyTestComp3 {}

src/cdk/schematics/testing/test-case-setup.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {getSystemPath, normalize} from '@angular-devkit/core';
+import {getSystemPath, normalize, Path} from '@angular-devkit/core';
 import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing';
 import * as virtualFs from '@angular-devkit/core/src/virtual-fs/host';
 import {HostTree, Tree} from '@angular-devkit/schematics';
@@ -224,13 +224,13 @@ export function defineJasmineTestCases(versionName: string, collectionFile: stri
  */
 export function _patchTypeScriptDefaultLib(tree: Tree) {
   const _originalRead = tree.read;
-  tree.read = function(filePath: string) {
+  tree.read = function(filePath: Path) {
     // In case a file within the TypeScript package is requested, we read the file from
     // the real file system. This is necessary because within unit tests, the "typeScript"
     // package from within the Bazel "@npm" repository  is used. The virtual tree can't be
     // used because the "@npm" repository directory is not part of the virtual file system.
     if (filePath.match(/node_modules[/\\]typescript/)) {
-      return readFileSync(filePath);
+      return readFileSync(getSystemPath(filePath));
     } else {
       return _originalRead.apply(this, arguments);
     }

src/cdk/schematics/update-tool/component-resource-collector.ts

@@ -6,9 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {dirname, resolve} from 'path';
+import {dirname} from 'path';
 import * as ts from 'typescript';
-import {FileSystem} from './file-system';
+import {FileSystem, WorkspacePath} from './file-system';
 import {getAngularDecorators} from './utils/decorators';
 import {unwrapExpression} from './utils/functions';
 import {
@@ -28,7 +28,7 @@ export interface ResolvedResource {
   /** Whether the given resource is inline or not. */
   inline: boolean;
   /** Path to the file that contains this resource. */
-  filePath: string;
+  filePath: WorkspacePath;
   /**
    * Gets the character and line of a given position index in the resource.
    * If the resource is declared inline within a TypeScript source file, the line and
@@ -81,7 +81,8 @@ export class ComponentResourceCollector {
     }
 
     const sourceFile = node.getSourceFile();
-    const sourceFileName = sourceFile.fileName;
+    const filePath = this._fileSystem.resolve(sourceFile.fileName);
+    const sourceFileDirPath = dirname(sourceFile.fileName);
 
     // Walk through all component metadata properties and determine the referenced
     // HTML templates (either external or inline)
@@ -91,7 +92,6 @@ export class ComponentResourceCollector {
       }
 
       const propertyName = getPropertyNameText(property.name);
-      const filePath = resolve(sourceFileName);
 
       if (propertyName === 'styles' && ts.isArrayLiteralExpression(property.initializer)) {
         property.initializer.elements.forEach(el => {
@@ -100,7 +100,7 @@ export class ComponentResourceCollector {
             // not part of the template content.
             const templateStartIdx = el.getStart() + 1;
             this.resolvedStylesheets.push({
-              filePath: filePath,
+              filePath,
               container: node,
               content: el.text,
               inline: true,
@@ -119,7 +119,7 @@ export class ComponentResourceCollector {
         // not part of the template content.
         const templateStartIdx = property.initializer.getStart() + 1;
         this.resolvedTemplates.push({
-          filePath: filePath,
+          filePath,
           container: node,
           content: property.initializer.text,
           inline: true,
@@ -132,7 +132,7 @@ export class ComponentResourceCollector {
       if (propertyName === 'styleUrls' && ts.isArrayLiteralExpression(property.initializer)) {
         property.initializer.elements.forEach(el => {
           if (ts.isStringLiteralLike(el)) {
-            const stylesheetPath = resolve(dirname(sourceFileName), el.text);
+            const stylesheetPath = this._fileSystem.resolve(sourceFileDirPath, el.text);
 
             // In case the stylesheet does not exist in the file system, skip it gracefully.
             if (!this._fileSystem.exists(stylesheetPath)) {
@@ -145,7 +145,8 @@ export class ComponentResourceCollector {
       }
 
       if (propertyName === 'templateUrl' && ts.isStringLiteralLike(property.initializer)) {
-        const templatePath = resolve(dirname(sourceFileName), property.initializer.text);
+        const templateUrl = property.initializer.text;
+        const templatePath = this._fileSystem.resolve(sourceFileDirPath, templateUrl);
 
         // In case the template does not exist in the file system, skip this
         // external template.
@@ -169,7 +170,7 @@ export class ComponentResourceCollector {
   }
 
   /** Resolves an external stylesheet by reading its content and computing line mappings. */
-  resolveExternalStylesheet(filePath: string, container: ts.ClassDeclaration|null):
+  resolveExternalStylesheet(filePath: WorkspacePath, container: ts.ClassDeclaration|null):
       ResolvedResource {
     const fileContent = this._fileSystem.read(filePath);
     const lineStartsMap = computeLineStartsMap(fileContent);