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();
+  });
+});