fix(migrations): cf migration validate structure of ngswitch before migrating (#53530)

thePunderWoman · alxhub · commit e88a12d5b3cf · 2023-12-13T09:23:17.000-08:00
This fix handles the common case where an ngswitch might have invalid syntax post migration. This is likely due to using elements other than case or default underneath the ngswitchcase. This will fail out of the migration for that file when these cases are detected with a useful console message. fixes: #53234 PR Close #53530
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/cases.ts b/packages/core/schematics/ng-generate/control-flow-migration/cases.ts
@@ -17,7 +17,7 @@ export const nakedcase = 'ngSwitchCase';
 export const switchdefault = '*ngSwitchDefault';
 export const nakeddefault = 'ngSwitchDefault';
 
-const cases = [
+export const cases = [
   boundcase,
   switchcase,
   nakedcase,
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts
@@ -28,6 +28,9 @@ export function migrateTemplate(
     const ifResult = migrateIf(template);
     const forResult = migrateFor(ifResult.migrated);
     const switchResult = migrateSwitch(forResult.migrated);
+    if (switchResult.errors.length > 0) {
+      return {migrated: template, errors: switchResult.errors};
+    }
     const caseResult = migrateCase(switchResult.migrated);
     const templateResult = processNgTemplates(caseResult.migrated);
     if (templateResult.err !== undefined) {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/switches.ts b/packages/core/schematics/ng-generate/control-flow-migration/switches.ts
@@ -6,8 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {visitAll} from '@angular/compiler';
+import {Element, Node, Text, visitAll} from '@angular/compiler';
 
+import {cases} from './cases';
 import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
@@ -65,11 +66,35 @@ export function migrateSwitch(template: string):
   return {migrated: result, errors, changed};
 }
 
+function assertValidSwitchStructure(children: Node[]): void {
+  for (const child of children) {
+    if (child instanceof Text && child.value.trim() !== '') {
+      throw new Error(
+          `Text node: "${child.value}" would result in invalid migrated @switch block structure. ` +
+          `@switch can only have @case or @default as children.`);
+    } else if (child instanceof Element) {
+      let hasCase = false;
+      for (const attr of child.attrs) {
+        if (cases.includes(attr.name)) {
+          hasCase = true;
+        }
+      }
+      if (!hasCase) {
+        throw new Error(
+            `Element node: "${
+                child.name}" would result in invalid migrated @switch block structure. ` +
+            `@switch can only have @case or @default as children.`);
+      }
+    }
+  }
+}
+
 function migrateNgSwitch(etm: ElementToMigrate, tmpl: string, offset: number): Result {
   const lbString = etm.hasLineBreaks ? '\n' : '';
   const condition = etm.attr.value;
 
   const originals = getOriginals(etm, tmpl, offset);
+  assertValidSwitchStructure(originals.childNodes);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
   const startBlock = `${startMarker}${start}${lbString}@switch (${condition}) {`;
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Attribute, Element, ParseError, ParseTreeResult, RecursiveVisitor, Text} from '@angular/compiler';
+import {Attribute, Element, ParseTreeResult, RecursiveVisitor, Text} from '@angular/compiler';
 import ts from 'typescript';
 
 export const ngtemplate = 'ng-template';
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -6,11 +6,11 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Attribute, HtmlParser, ParseTreeResult, visitAll} from '@angular/compiler';
+import {Attribute, HtmlParser, Node, ParseTreeResult, visitAll} from '@angular/compiler';
 import {dirname, join} from 'path';
 import ts from 'typescript';
 
-import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endMarker, startMarker, MigrateError, ParseResult, Template, TemplateCollector} from './types';
+import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endMarker, ParseResult, startMarker, Template, TemplateCollector} from './types';
 
 const importRemovals = [
   'NgIf', 'NgIfElse', 'NgIfThenElse', 'NgFor', 'NgForOf', 'NgForTrackBy', 'NgSwitch',
@@ -415,7 +415,7 @@ export function removeImports(
  * processing
  */
 export function getOriginals(etm: ElementToMigrate, tmpl: string, offset: number):
-    {start: string, end: string, childLength: number} {
+    {start: string, end: string, childLength: number, children: string[], childNodes: Node[]} {
   // original opening block
   if (etm.el.children.length > 0) {
     const childStart = etm.el.children[0].sourceSpan.start.offset - offset;
@@ -428,13 +428,25 @@ export function getOriginals(etm: ElementToMigrate, tmpl: string, offset: number
         etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset,
         etm.el.sourceSpan.end.offset - offset);
     const childLength = childEnd - childStart;
-    return {start, end, childLength};
+    return {
+      start,
+      end,
+      childLength,
+      children: getOriginalChildren(etm.el.children, tmpl, offset),
+      childNodes: etm.el.children
+    };
   }
   // self closing or no children
   const start =
       tmpl.slice(etm.el.sourceSpan.start.offset - offset, etm.el.sourceSpan.end.offset - offset);
   // original closing block
-  return {start, end: '', childLength: 0};
+  return {start, end: '', childLength: 0, children: [], childNodes: []};
+}
+
+function getOriginalChildren(children: Node[], tmpl: string, offset: number) {
+  return children.map(child => {
+    return tmpl.slice(child.sourceSpan.start.offset - offset, child.sourceSpan.end.offset - offset);
+  });
 }
 
 function isI18nTemplate(etm: ElementToMigrate, i18nAttr: Attribute|undefined): boolean {
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -5082,5 +5082,46 @@ describe('control flow migration', () => {
       expect(warnings).toContain(
           'Please check the template for valid HTML structures and run the migration again.');
     });
+
+    it('should not migrate a template that would result in invalid switch block contents',
+       async () => {
+         writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+
+        @Component({
+          templateUrl: './comp.html'
+        })
+        class Comp {
+          testOpts = 2;
+        }
+      `);
+
+         writeFile('/comp.html', [
+           `<div [ngSwitch]="testOpts">`,
+           `<strong>`,
+           `<p *ngSwitchCase="1">Option 1</p>`,
+           `<p *ngSwitchCase="2">Option 2</p>`,
+           `<p *ngSwitchDefault>Option 3</p>`,
+           `</strong>`,
+           `</div>`,
+         ].join('\n'));
+
+         await runMigration();
+         const content = tree.readContent('/comp.html');
+         expect(content).toBe([
+           `<div [ngSwitch]="testOpts">`,
+           `<strong>`,
+           `<p *ngSwitchCase="1">Option 1</p>`,
+           `<p *ngSwitchCase="2">Option 2</p>`,
+           `<p *ngSwitchDefault>Option 3</p>`,
+           `</strong>`,
+           `</div>`,
+         ].join('\n'));
+
+         expect(warnOutput.join(' '))
+             .toContain(
+                 `Element node: "strong" would result in invalid migrated @switch block structure. ` +
+                 `@switch can only have @case or @default as children.`);
+       });
   });
 });
