[PORT]Throw exception when circular reference is detected in LG imports

libraries/botbuilder-lg/src/templatesParser.ts

@@ -88,6 +88,22 @@ export class TemplatesParser {
         return TemplatesParser.parseResource(resource, importResolver, expressionParser);
     }
 
+    /**
+     * Parser to turn lg content into a Templates.
+     * @param resource LG resource.
+     * @param importResolver Resolver to resolve LG import id to template text.
+     * @param expressionParser Expression parser for evaluating expressions.
+     * @param cachedTemplates Give the file path and templates to avoid parsing and to improve performance.
+     * @returns Entity.
+     */
+    public static parseResource(
+        resource: LGResource,
+        importResolver?: ImportResolverDelegate,
+        expressionParser?: ExpressionParser
+    ): Templates {
+        return TemplatesParser.innerParseResource(resource, importResolver, expressionParser);
+    }
+
     /**
      * Parser to turn lg content into a Templates based on the original Templates.
      * @param content Text content contains lg templates.
@@ -151,18 +167,19 @@ export class TemplatesParser {
      * @param importResolver Resolver to resolve LG import id to template text.
      * @param expressionParser Expression parser for evaluating expressions.
      * @param cachedTemplates Give the file path and templates to avoid parsing and to improve performance.
+     * @param parentTemplates Parent visited Templates.
      * @returns Entity.
      */
-    public static parseResource(
+    private static innerParseResource(
         resource: LGResource,
         importResolver?: ImportResolverDelegate,
         expressionParser?: ExpressionParser,
-        cachedTemplates?: Map<string, Templates>
+        cachedTemplates: Map<string, Templates> = new Map<string, Templates>(),
+        parentTemplates: Templates[] = []
     ): Templates {
         if (!resource) {
             throw new Error('lg resource is empty.');
         }
-        cachedTemplates = cachedTemplates || new Map<string, Templates>();
 
         if (cachedTemplates.has(resource.id)) {
             return cachedTemplates.get(resource.id);
@@ -181,7 +198,7 @@ export class TemplatesParser {
         try {
             // eslint-disable-next-line @typescript-eslint/no-use-before-define
             templates = new TemplatesTransformer(templates).transform(this.antlrParseTemplates(resource));
-            templates.references = this.getReferences(templates, cachedTemplates);
+            templates.references = this.getReferences(templates, cachedTemplates, parentTemplates);
             const semanticErrors = new StaticChecker(templates).check();
             templates.diagnostics.push(...semanticErrors);
         } catch (err) {
@@ -218,9 +235,13 @@ export class TemplatesParser {
     /**
      * @private
      */
-    private static getReferences(file: Templates, cachedTemplates?: Map<string, Templates>): Templates[] {
+    private static getReferences(
+        file: Templates,
+        cachedTemplates: Map<string, Templates> = new Map<string, Templates>(),
+        parentTemplates: Templates[] = []
+    ): Templates[] {
         const resourcesFound = new Set<Templates>();
-        this.resolveImportResources(file, resourcesFound, cachedTemplates || new Map<string, Templates>());
+        this.resolveImportResources(file, resourcesFound, cachedTemplates, parentTemplates);
 
         resourcesFound.delete(file);
         return Array.from(resourcesFound);
@@ -232,9 +253,11 @@ export class TemplatesParser {
     private static resolveImportResources(
         start: Templates,
         resourcesFound: Set<Templates>,
-        cachedTemplates?: Map<string, Templates>
+        cachedTemplates: Map<string, Templates>,
+        parentTemplates: Templates[]
     ): void {
         resourcesFound.add(start);
+        parentTemplates.push(start);
 
         for (const importItem of start.imports) {
             let resource: LGResource;
@@ -251,23 +274,39 @@ export class TemplatesParser {
                 throw new TemplateException(error.message, [diagnostic]);
             }
 
+            // Cycle reference would throw exception to avoid infinite Loop.
+            // Import self is allowed, and would ignore it.
+            const parentTemplate = parentTemplates[parentTemplates.length - 1];
+            if (parentTemplate.id !== resource.id && parentTemplates.some(u => u.id === resource.id)) {
+                const errorMsg = `${TemplateErrors.loopDetected} ${resource.id} => ${start.id}`;
+                const diagnostic = new Diagnostic(
+                    importItem.sourceRange.range,
+                    errorMsg,
+                    DiagnosticSeverity.Error,
+                    start.source
+                );
+                throw new TemplateException(errorMsg, [diagnostic]);
+            }
+
             if (Array.from(resourcesFound).every((u): boolean => u.id !== resource.id)) {
                 let childResource: Templates;
                 if (cachedTemplates.has(resource.id)) {
                     childResource = cachedTemplates.get(resource.id);
                 } else {
-                    childResource = TemplatesParser.parseResource(
+                    childResource = TemplatesParser.innerParseResource(
                         resource,
                         start.importResolver,
                         start.expressionParser,
-                        cachedTemplates
+                        cachedTemplates,
+                        parentTemplates
                     );
                     cachedTemplates.set(resource.id, childResource);
                 }
 
-                this.resolveImportResources(childResource, resourcesFound, cachedTemplates);
+                this.resolveImportResources(childResource, resourcesFound, cachedTemplates, parentTemplates);
             }
         }
+        parentTemplates.pop();
     }
 }
 

libraries/botbuilder-lg/tests/lgDiagnostic.test.js

@@ -41,7 +41,8 @@ describe(`LGExceptionTest`, function() {
         RunTimeErrors: GetLGFile('RunTimeErrors.lg'),
         ExpressionFormatError: GetDiagnostics(`ExpressionFormatError.lg`),
         MultiLineExprError: GetDiagnostics(`MultiLineExprError.lg`),
-        ErrorLine: GetDiagnostics(`ErrorLine.lg`)
+        ErrorLine: GetDiagnostics(`ErrorLine.lg`),
+        CycleRef1: GetDiagnostics(`CycleRef1.lg`),
     };
 
     it(`TestConditionFormatError`, function() {
@@ -247,9 +248,9 @@ describe(`LGExceptionTest`, function() {
 
     it(`TestLoopDetected`, function() {
         var templates = preloaded.LoopDetected;
-        
+
         assert.throws(() => templates.evaluate(`wPhrase`), Error(`Loop detected: welcome_user => wPhrase [wPhrase]  Error occurred when evaluating '-\${wPhrase()}'. [welcome_user]  Error occurred when evaluating '-\${welcome_user()}'.`));
-        
+
         assert.throws(() => templates.analyzeTemplate(`wPhrase`), Error('Loop detected: welcome_user => wPhrase'),);
 
         assert.throws(() => templates.analyzeTemplate(`shouldFail`), Error('Loop detected: shouldFail'),);
@@ -282,11 +283,11 @@ describe(`LGExceptionTest`, function() {
         assert.throws(() => templates.evaluate(`structured1`), Error(`'dialog.abc' evaluated to null. [structured1] Property 'Text': Error occurred when evaluating 'Text=I want \${dialog.abc}'.`));
 
         assert.throws(() => templates.evaluate(`structured2`), Error(`'dialog.abc' evaluated to null. [template1]  Error occurred when evaluating '-I want \${dialog.abc}'. [structured2] Property 'Text': Error occurred when evaluating 'Text=I want \${template1()}'.`));
-        
+
         assert.throws(() => templates.evaluate(`structured3`), Error(`'dialog.abc' evaluated to null. [template1]  Error occurred when evaluating '-I want \${dialog.abc}'. [structured2] Property 'Text': Error occurred when evaluating 'Text=I want \${template1()}'. [structured3]  Error occurred when evaluating '\${structured2()}'.`));
-        
+
         assert.throws(() => templates.evaluate(`switchcase1`, { turn : { testValue : 1 } }), Error(`'dialog.abc' evaluated to null. [switchcase1] Case '\${1}': Error occurred when evaluating '-I want \${dialog.abc}'.`));
-        
+
         assert.throws(() => templates.evaluate(`switchcase2`, { turn : { testValue : 0 } }), Error(`'dialog.abc' evaluated to null. [switchcase2] Case 'Default': Error occurred when evaluating '-I want \${dialog.abc}'.`));
     });
 
@@ -295,26 +296,26 @@ describe(`LGExceptionTest`, function() {
         assert.strictEqual(diagnostics.length, 1);
         assert.strictEqual(diagnostics[0].message.includes(`Close } is missing in Expression`), true);
     });
-    
+
     it(`TestMultiLineExpressionInLG`, function() {
         var diagnostics = preloaded.MultiLineExprError;
         assert.strictEqual(diagnostics.length, 1);
         assert.strictEqual(diagnostics[0].message.includes(`Close } is missing in Expression`), true);
-    
+
         diagnostics = Templates.parseResource(new LGResource('', '', '#Demo2\r\n- ${createArray(1,\r\n, 2,3)')).diagnostics;
         assert.strictEqual(diagnostics.length, 1);
         assert.strictEqual(diagnostics[0].message.includes(`Close } is missing in Expression`), true);
-    
+
         diagnostics = Templates.parseResource(new LGResource('', '', '#Demo4\r\n- ${createArray(1,\r\n2,3)\r\n> this is a comment')).diagnostics;
         assert.strictEqual(diagnostics.length, 1);
         assert.strictEqual(diagnostics[0].message.includes(`Close } is missing in Expression`), true);
     });
-    
+
     it(`TestErrorLine`, function() {
         var diagnostics = preloaded.ErrorLine;
         assert.strictEqual(diagnostics.length, 4);
-        
-    
+
+
         assert.strictEqual(DiagnosticSeverity.Error, diagnostics[0].severity);
         assert.strictEqual(diagnostics[0].message.includes(TemplateErrors.syntaxError('mismatched input \'-\' expecting <EOF>')), true);
         assert.strictEqual(DiagnosticSeverity.Error, diagnostics[1].severity);
@@ -324,4 +325,12 @@ describe(`LGExceptionTest`, function() {
         assert.strictEqual(DiagnosticSeverity.Error, diagnostics[3].severity);
         assert.strictEqual(diagnostics[3].message.includes(TemplateErrors.invalidStrucBody('- hi')), true);
     });
-});
+
+    it(`TestLoopReference`, function() {
+        var diagnostics = preloaded.CycleRef1;
+        assert.strictEqual(diagnostics.length, 1);
+
+        assert.strictEqual(DiagnosticSeverity.Error, diagnostics[0].severity);
+        assert.strictEqual(diagnostics[0].message.startsWith(TemplateErrors.loopDetected), true);
+    });
+});

libraries/botbuilder-lg/tests/testData/exceptionExamples/CycleRef1.lg

@@ -0,0 +1,5 @@
+> Import CycleRef2.lg
+[import](./ImportFile.lg)
+[import](./CycleRef2.lg)
+# a
+- a

libraries/botbuilder-lg/tests/testData/exceptionExamples/CycleRef2.lg

@@ -0,0 +1,4 @@
+> Import CycleRef1.lg
+[import](./CycleRef1.lg)
+# b
+- b