fix(migrations): cf migration - detect and error when result is invalid i18n nesting (#53638)

thePunderWoman · atscott · commit eb7c29c7b64a · 2023-12-19T11:01:58.000-08:00
This will gracefully error on templates when the resulting template would have invalid i18n nested structures. PR Close #53638
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts
@@ -13,7 +13,7 @@ import {migrateFor} from './fors';
 import {migrateIf} from './ifs';
 import {migrateSwitch} from './switches';
 import {AnalyzedFile, endI18nMarker, endMarker, MigrateError, startI18nMarker, startMarker} from './types';
-import {canRemoveCommonModule, formatTemplate, parseTemplate, processNgTemplates, removeImports} from './util';
+import {canRemoveCommonModule, formatTemplate, parseTemplate, processNgTemplates, removeImports, validateI18nStructure, validateMigratedTemplate} from './util';
 
 /**
  * Actually migrates a given template to the new syntax
@@ -42,15 +42,9 @@ export function migrateTemplate(
     if (changed) {
       // determine if migrated template is a valid structure
       // if it is not, fail out
-      const parsed = parseTemplate(migrated);
-      if (parsed.errors.length > 0) {
-        const parsingError = {
-          type: 'parse',
-          error: new Error(
-              `The migration resulted in invalid HTML for ${file.sourceFile.fileName}. ` +
-              `Please check the template for valid HTML structures and run the migration again.`)
-        };
-        return {migrated: template, errors: [parsingError]};
+      const errors = validateMigratedTemplate(migrated, file.sourceFile.fileName);
+      if (errors.length > 0) {
+        return {migrated: template, errors};
       }
     }
 
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -10,7 +10,7 @@ import {Attribute, Element, HtmlParser, Node, ParseTreeResult, visitAll} from '@
 import {dirname, join} from 'path';
 import ts from 'typescript';
 
-import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endI18nMarker, endMarker, i18nCollector, importRemovals, importWithCommonRemovals, ParseResult, startI18nMarker, startMarker, Template, TemplateCollector} from './types';
+import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endI18nMarker, endMarker, i18nCollector, importRemovals, importWithCommonRemovals, MigrateError, ParseResult, startI18nMarker, startMarker, Template, TemplateCollector} from './types';
 
 const startMarkerRegex = new RegExp(startMarker, 'gm');
 const endMarkerRegex = new RegExp(endMarker, 'gm');
@@ -233,6 +233,49 @@ export function parseTemplate(template: string): ParseResult {
   return {tree: parsed, errors: []};
 }
 
+export function validateMigratedTemplate(migrated: string, fileName: string): MigrateError[] {
+  const parsed = parseTemplate(migrated);
+  let errors: MigrateError[] = [];
+  if (parsed.errors.length > 0) {
+    errors.push({
+      type: 'parse',
+      error: new Error(
+          `The migration resulted in invalid HTML for ${fileName}. ` +
+          `Please check the template for valid HTML structures and run the migration again.`)
+    });
+  }
+  if (parsed.tree) {
+    const i18nError = validateI18nStructure(parsed.tree, fileName);
+    if (i18nError !== null) {
+      errors.push({type: 'i18n', error: i18nError});
+    }
+  }
+  return errors;
+}
+
+export function validateI18nStructure(parsed: ParseTreeResult, fileName: string): Error|null {
+  const visitor = new i18nCollector();
+  visitAll(visitor, parsed.rootNodes);
+  const parents = visitor.elements.filter(el => el.children.length > 0);
+  for (const p of parents) {
+    for (const el of visitor.elements) {
+      if (el === p) continue;
+      if (isChildOf(p, el)) {
+        return new Error(
+            `i18n Nesting error: The migration would result in invalid i18n nesting for ` +
+            `${fileName}. Element with i18n attribute "${p.name}" would result having a child of ` +
+            `element with i18n attribute "${el.name}". Please fix and re-run the migration.`);
+      }
+    }
+  }
+  return null;
+}
+
+function isChildOf(parent: Element, el: Element): boolean {
+  return parent.sourceSpan.start.offset < el.sourceSpan.start.offset &&
+      parent.sourceSpan.end.offset > el.sourceSpan.end.offset;
+}
+
 /**
  * calculates the level of nesting of the items in the collector
  */
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -5511,5 +5511,47 @@ describe('control flow migration', () => {
                  `Element node: "strong" would result in invalid migrated @switch block structure. ` +
                  `@switch can only have @case or @default as children.`);
        });
+
+    it('should not migrate a template that would result in invalid i18n nesting', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+
+        @Component({
+          templateUrl: './comp.html'
+        })
+        class Comp {
+          testOpts = 2;
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<ng-container i18n="messageid">`,
+        `  <div *ngIf="condition; else elseTmpl">`,
+        `    If content here`,
+        `  </div>`,
+        `</ng-container>`,
+        `<ng-template #elseTmpl i18n="elsemessageid">`,
+        `  <div>Else content here</div>`,
+        `</ng-template>`,
+      ].join('\n'));
+
+      await runMigration();
+      const content = tree.readContent('/comp.html');
+      expect(content).toBe([
+        `<ng-container i18n="messageid">`,
+        `  <div *ngIf="condition; else elseTmpl">`,
+        `    If content here`,
+        `  </div>`,
+        `</ng-container>`,
+        `<ng-template #elseTmpl i18n="elsemessageid">`,
+        `  <div>Else content here</div>`,
+        `</ng-template>`,
+      ].join('\n'));
+
+      expect(warnOutput.join(' '))
+          .toContain(
+              `Element with i18n attribute "ng-container" would result having a child of element with i18n attribute ` +
+              `"ng-container". Please fix and re-run the migration.`);
+    });
   });
 });
