Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ThrowOnRecursive to AnalyzerOptions #5872

Merged
merged 4 commits into from
Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions libraries/Microsoft.Bot.Builder.LanguageGeneration/Analyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,23 @@ public Analyzer(Templates templates, EvaluationOptions opt = null)
/// <returns>Analyze result including variables and template references.</returns>
public AnalyzerResult AnalyzeTemplate(string templateName)
{
if (!_templateMap.ContainsKey(templateName) || _evaluationTargetStack.Any(e => e.TemplateName == templateName))
var missingName = !_templateMap.ContainsKey(templateName);
var stackHasName = _evaluationTargetStack.Any(e => e.TemplateName == templateName);

if (Templates.LgOptions?.ThrowOnRecursive == true)
EricDahlvang marked this conversation as resolved.
Show resolved Hide resolved
{
if (missingName)
{
throw new ArgumentException(TemplateErrors.TemplateNotExist(templateName));
}

if (stackHasName)
{
throw new InvalidOperationException($"{TemplateErrors.LoopDetected} {string.Join(" => ", _evaluationTargetStack.Reverse().Select(e => e.TemplateName))} => {templateName}");
}
}

if (missingName || stackHasName)
{
return new AnalyzerResult();
}
Expand Down Expand Up @@ -210,7 +226,7 @@ private AnalyzerResult AnalyzeExpressionDirectly(Expression exp)
}
else
{
if (!result.TemplateReferences.Contains(templateName))
if (Templates.LgOptions?.ThrowOnRecursive == true || !result.TemplateReferences.Contains(templateName))
EricDahlvang marked this conversation as resolved.
Show resolved Hide resolved
{
// if template has parameters, just get the template ref without variables.
result.Union(new AnalyzerResult(templateReferences: this.AnalyzeTemplate(templateName).TemplateReferences));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class EvaluationOptions
private readonly string _replaceNullKey = "@replaceNull";
private readonly string _lineBreakKey = "@lineBreakStyle";
private readonly string _cacheScope = "@cacheScope";
private readonly string _throwOnRecursive = "@throwOnRecursive";

/// <summary>
/// Initializes a new instance of the <see cref="EvaluationOptions"/> class.
Expand All @@ -67,6 +68,7 @@ public EvaluationOptions()
Locale = null;
OnEvent = null;
CacheScope = null;
ThrowOnRecursive = null;
}

/// <summary>
Expand All @@ -81,6 +83,7 @@ public EvaluationOptions(EvaluationOptions opt)
Locale = opt.Locale ?? Thread.CurrentThread.CurrentCulture.Name;
OnEvent = opt.OnEvent;
CacheScope = opt.CacheScope;
ThrowOnRecursive = opt.ThrowOnRecursive;
}

/// <summary>
Expand Down Expand Up @@ -124,6 +127,13 @@ public EvaluationOptions(IList<string> optionsList)
}
}
}
else if (key.Equals(_throwOnRecursive, StringComparison.OrdinalIgnoreCase))
{
if (value.ToLowerInvariant() == "true")
{
ThrowOnRecursive = true;
}
}
}
}
}
Expand Down Expand Up @@ -178,6 +188,14 @@ public EvaluationOptions(IList<string> optionsList)
/// </value>
public LGCacheScope? CacheScope { get; set; } = null;

/// <summary>
/// Gets or sets a value determining if recursive calls throw an exception.
/// </summary>
/// <value>
/// When true, throw an exception if a recursive call is detected.
/// </value>
public bool? ThrowOnRecursive { get; set; } = null;

/// <summary>
/// Merge a incoming option to current option. If a property in incoming option is not null while it is null in current
/// option, then the value of this property will be overwritten.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ public IList<object> ExpandTemplate(string templateName, object scope = null, Ev
public AnalyzerResult AnalyzeTemplate(string templateName)
{
CheckErrors();
var analyzer = new Analyzer(this);
var analyzer = new Analyzer(this, this.LgOptions);
EricDahlvang marked this conversation as resolved.
Show resolved Hide resolved
return analyzer.AnalyzeTemplate(templateName);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,14 @@ public void TestLoopDetected()
var lgFile = GetTemplates("LoopDetected.lg");
var exception = Assert.Throws<InvalidOperationException>(() => lgFile.Evaluate("wPhrase"));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);

lgFile.Options = new List<string> { "@throwOnRecursive=true" };
EricDahlvang marked this conversation as resolved.
Show resolved Hide resolved

exception = Assert.Throws<InvalidOperationException>(() => lgFile.AnalyzeTemplate("wPhrase"));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);

exception = Assert.Throws<InvalidOperationException>(() => lgFile.AnalyzeTemplate("shouldFail"));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);
}

[Fact]
Expand Down