refactor: ensure paths are safely passed around in ng-update (#19354)

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);