Skip to content

Commit

Permalink
Improve performance of no-hardcoded-env-urls linter rule (#3833)
Browse files Browse the repository at this point in the history 
* Test replacing the regex with basic string matching

* Add checks for valid subdomain vs substring

* Refactor

Co-authored-by: jofleish <jofleish@microsoft.com>
  • Loading branch information
@anthony-c-martin @jfleisher
anthony-c-martin and jfleisher authored Jul 30, 2021
1 parent 86843ce commit 217fe34
Show file tree
Hide file tree
Showing 10 changed files with 141 additions and 142 deletions.
7 changes: 0 additions & 7 deletions docs/linter-rules/no-hardcoded-env-urls.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ The set of URL hosts to disallow may be customized using the `disallowedHosts` p
"no-hardcoded-env-urls": {
"level": "warning",
"disallowedHosts": [
"management.core.windows.net",
"gallery.azure.com",
"management.core.windows.net",
"management.azure.com",
Expand All @@ -45,17 +44,11 @@ The set of URL hosts to disallow may be customized using the `disallowedHosts` p
"login.microsoftonline.com",
"graph.windows.net",
"trafficmanager.net",
"vault.azure.net",
"datalake.azure.net",
"azuredatalakestore.net",
"azuredatalakeanalytics.net",
"vault.azure.net",
"api.loganalytics.io",
"api.loganalytics.iov1",
"asazure.windows.net",
"region.asazure.windows.net",
"api.loganalytics.iov1",
"api.loganalytics.io",
"asazure.windows.net",
"region.asazure.windows.net",
"batch.core.windows.net"
Expand Down
32 changes: 19 additions & 13 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/LinterRuleTestsBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,37 @@
using Bicep.Core.Analyzers.Linter;
using Bicep.Core.Analyzers.Linter.Rules;
using Bicep.Core.Diagnostics;
using Bicep.Core.UnitTests.Assertions;
using Bicep.Core.UnitTests.Utils;
using FluentAssertions;
using FluentAssertions.Execution;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Bicep.Core.UnitTests.Diagnostics.LinterRuleTests
{
public class LinterRuleTestsBase
{
virtual protected void CompileAndTest(string ruleCode, string text, int expectedDiagnosticCount)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(ruleCode, text);
errors.Should().HaveCount(expectedDiagnosticCount);
}
}
protected void CompileAndTest(string ruleCode, string text, int expectedDiagnosticCount)
=> AssertRuleCodeDiagnostics(ruleCode, text, diags => diags.Should().HaveCount(expectedDiagnosticCount));

protected IDiagnostic[] GetDiagnostics(string ruleCode, string text)
protected void AssertRuleCodeDiagnostics(string ruleCode, string text, Action<IEnumerable<IDiagnostic>> assertAction)
=> DoWithDiagnosticAnnotations(
text,
diag => diag.Code == ruleCode,
assertAction);

private void DoWithDiagnosticAnnotations(string text, Func<IDiagnostic, bool> filterFunc, Action<IEnumerable<IDiagnostic>> assertAction)
{
var compilationResult = CompilationHelper.Compile(text);
var internalRuleErrors = compilationResult.Diagnostics.Where(d => d.Code == LinterAnalyzer.FailedRuleCode).ToArray();
internalRuleErrors.Count().Should().Be(0, "There should never be linter FailedRuleCode errors");
var result = CompilationHelper.Compile(text);

result.Should().NotHaveDiagnosticsWithCodes(new [] { LinterAnalyzer.FailedRuleCode }, "There should never be linter FailedRuleCode errors");

return compilationResult.Diagnostics.OfType<IDiagnostic>().Where(d => d.Code == ruleCode).ToArray();
DiagnosticAssertions.DoWithDiagnosticAnnotations(
result.Compilation.SourceFileGrouping.EntryPoint,
result.Diagnostics.Where(filterFunc),
assertAction);
}
}
}
21 changes: 5 additions & 16 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoHardcodedEnvironmentUrlsRuleTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Linq;
using Azure.Deployments.Core.Extensions;
using Bicep.Core.Analyzers.Linter.Rules;
using Bicep.Core.Configuration;
using Bicep.Core.Text;
using Bicep.Core.Diagnostics;
using Bicep.Core.UnitTests.Assertions;
using Bicep.Core.UnitTests.Utils;
using FluentAssertions;
using FluentAssertions.Execution;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;

// TODO: Test with different configs
namespace Bicep.Core.UnitTests.Diagnostics.LinterRuleTests
Expand Down Expand Up @@ -162,14 +153,13 @@ public void InsideExpressions(int diagnosticCount, string text)
[DataRow("subdomain1.management.azzzure.com", false)]
[DataRow("http://subdomain1.manageeeement.azure.com", false)]
[DataRow("https://subdomain1.managemeeeent.azure.com", false)]
public void DisallowedHostsRegexTest(string host, bool isMatch)
public void DisallowedHostsMatchingTest(string testString, bool isMatch)
{
var rule = new NoHardcodedEnvironmentUrlsRule();
var configHelper = new ConfigHelper(); // this ensures configuration is loaded from resources
rule.Configure(configHelper.Config);

var regex = rule.CreateDisallowedHostRegex();
Assert.AreEqual(isMatch, regex.IsMatch(host));
Assert.AreEqual(isMatch, actual: rule.DisallowedHosts.Any(host => NoHardcodedEnvironmentUrlsRule.FindHostnameMatches(host, testString).Any()));
}

[DataTestMethod]
Expand All @@ -192,14 +182,13 @@ public void DisallowedHostsRegexTest(string host, bool isMatch)
[DataRow("https://subdomain1.management.azure.com", false)]
[DataRow("all the world is a stage, but subdomain1.management.azure.com should not be hardcoded", false)]
[DataRow("all the world is a stage, but subdomain1.schema.management.azure.com should not be hardcoded", true)]
public void ExcludedHostsRegexTest(string host, bool isMatch)
public void ExcludedHostsMatchingTest(string testString, bool isMatch)
{
var rule = new NoHardcodedEnvironmentUrlsRule();
var configHelper = new ConfigHelper(); // this ensures configuration is loaded from resources
rule.Configure(configHelper.Config);

var regex = rule.CreateExcludedHostsRegex();
Assert.AreEqual(isMatch, regex.IsMatch(host));
Assert.AreEqual(isMatch, rule.ExcludedHosts.Any(host => NoHardcodedEnvironmentUrlsRule.FindHostnameMatches(host, testString).Any()));
}
}
}
Expand Down
10 changes: 4 additions & 6 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnusedParametersRuleTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,18 @@ public void ParameterNameInFormattedMessage()

private void CompileAndTest(string text, params string[] unusedParams)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(NoUnusedParametersRule.Code, text);
AssertRuleCodeDiagnostics(NoUnusedParametersRule.Code, text, diags => {
if (unusedParams.Any())
{
var rule = new NoUnusedParametersRule();
string[] expectedMessages = unusedParams.Select(p => rule.GetMessage(p)).ToArray();
errors.Select(e => e.Message).Should().ContainInOrder(expectedMessages);
diags.Select(e => e.Message).Should().ContainInOrder(expectedMessages);
}
else
{
errors.Should().BeEmpty();
diags.Should().BeEmpty();
}
}
});
}

[DataRow(@"
Expand Down
10 changes: 4 additions & 6 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/NoUnusedVariablesRuleTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,18 @@ public void VariableNameInFormattedMessage()

private void CompileAndTest(string text, params string[] unusedVars)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(NoUnusedVariablesRule.Code, text);
AssertRuleCodeDiagnostics(NoUnusedVariablesRule.Code, text, diags => {
if (unusedVars.Any())
{
var rule = new NoUnusedVariablesRule();
string[] expectedMessages = unusedVars.Select(p => rule.GetMessage(p)).ToArray();
errors.Select(e => e.Message).Should().ContainInOrder(expectedMessages);
diags.Select(e => e.Message).Should().ContainInOrder(expectedMessages);
}
else
{
errors.Should().BeEmpty();
diags.Should().BeEmpty();
}
}
});
}

[DataRow(@"
Expand Down
22 changes: 9 additions & 13 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/PreferInterpolationRuleTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@ public class PreferInterpolationRuleTests : LinterRuleTestsBase
{
private void ExpectPass(string text)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(PreferInterpolationRule.Code, text);
errors.Should().HaveCount(0, $"expecting linter rule to pass");
}
AssertRuleCodeDiagnostics(PreferInterpolationRule.Code, text, diags => {
diags.Should().HaveCount(0, $"expecting linter rule to pass");
});
}

private void ExpectDiagnosticWithFix(string text, string expectedFix)
Expand All @@ -33,15 +31,13 @@ private void ExpectDiagnosticWithFix(string text, string expectedFix)

private void ExpectDiagnosticWithFix(string text, string[] expectedFixes)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(PreferInterpolationRule.Code, text);
errors.Should().HaveCount(expectedFixes.Length, $"expecting one fix per testcase");
AssertRuleCodeDiagnostics(PreferInterpolationRule.Code, text, diags => {
diags.Should().HaveCount(expectedFixes.Length, $"expecting one fix per testcase");

errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.Should().HaveCount(1);
errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.Should().HaveCount(1);
var a = errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.SelectMany(f => f.Replacements.SelectMany(r => r.Text));
}
diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.Should().HaveCount(1);
diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.Should().HaveCount(1);
var a = diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.SelectMany(f => f.Replacements.SelectMany(r => r.Text));
});
}

[DataRow(@"
Expand Down
22 changes: 9 additions & 13 deletions src/Bicep.Core.UnitTests/Diagnostics/LinterRuleTests/SimplifyInterpolationRuleTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,20 @@ public class SimplifyInterpolationRuleTests : LinterRuleTestsBase
{
private void ExpectPass(string text)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(SimplifyInterpolationRule.Code, text);
errors.Should().HaveCount(0, $"expecting linter rule to pass");
}
AssertRuleCodeDiagnostics(SimplifyInterpolationRule.Code, text, diags => {
diags.Should().HaveCount(0, $"expecting linter rule to pass");
});
}

private void ExpectDiagnosticWithFix(string text, string expectedFix)
{
using (new AssertionScope($"linter errors for this code:\n{text}\n"))
{
var errors = GetDiagnostics(SimplifyInterpolationRule.Code, text);
errors.Should().HaveCount(1, $"expected one fix per testcase");
AssertRuleCodeDiagnostics(SimplifyInterpolationRule.Code, text, diags => {
diags.Should().HaveCount(1, $"expected one fix per testcase");

errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.Should().HaveCount(1);
errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.Should().HaveCount(1);
errors.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.First().Text.Should().Be(expectedFix);
}
diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.Should().HaveCount(1);
diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.Should().HaveCount(1);
diags.First().As<IBicepAnalyerFixableDiagnostic>().Fixes.First().Replacements.First().Text.Should().Be(expectedFix);
});
}

[DataRow(
Expand Down
Loading

0 comments on commit 217fe34

Please sign in to comment.