Merge pull request #200 from Microsoft/PerthCharern/DebugRuleTests

Re-enable the default rule count test case (disabled in #176)

The problem was this:

ValidateCustomExtension test calls ValidationRuleSet.DefaultRuleSet and makes changes to it directly. Since the DefaultRuleSet is cached, the underlying private _defaultRuleSet is actually modified.

The reason we did not see the issue locally might be that the order of local test run and the order on AppVeyor are different. We see the issue only if the "Count" test is run after the custom extension test.

Author: PerthCharern

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

@@ -24,7 +24,7 @@ public class OpenApiValidator : OpenApiVisitorBase, IValidationContext
         /// <param name="ruleSet"></param>
         public OpenApiValidator(ValidationRuleSet ruleSet = null) 
         {
-            _ruleSet = ruleSet ?? ValidationRuleSet.DefaultRuleSet;
+            _ruleSet = ruleSet ?? ValidationRuleSet.GetDefaultRuleSet();
         }
 
         /// <summary>

src/Microsoft.OpenApi/Validations/ValidationRuleSet.cs

@@ -24,17 +24,22 @@ public sealed class ValidationRuleSet : IEnumerable<ValidationRule>
         /// <summary>
         /// Gets the default validation rule sets.
         /// </summary>
-        public static ValidationRuleSet DefaultRuleSet
+        /// <remarks>
+        /// This is a method instead of a property to signal that a new default ruleset object is created
+        /// per call. Making this a property may be misleading callers to think the returned rulesets from multiple calls
+        /// are the same objects.
+        /// </remarks>
+        public static ValidationRuleSet GetDefaultRuleSet()
         {
-            get
+            // Reflection can be an expensive operation, so we cache the default rule set that has already been built.
+            if (_defaultRuleSet == null)
             {
-                if (_defaultRuleSet == null)
-                {
-                    _defaultRuleSet = BuildDefaultRuleSet();
-                }
-
-                return _defaultRuleSet;
+                _defaultRuleSet = BuildDefaultRuleSet();
             }
+
+            // We create a new instance of ValidationRuleSet per call as a safeguard
+            // against unintentional modification of the private _defaultRuleSet.
+            return new ValidationRuleSet(_defaultRuleSet);
         }
 
         /// <summary>
@@ -44,51 +49,68 @@ public ValidationRuleSet()
         {
         }
 
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ValidationRuleSet"/> class.
+        /// </summary>
+        /// <param name="ruleSet">Rule set to be copied from.</param>
+        public ValidationRuleSet(ValidationRuleSet ruleSet)
+        {
+            if (ruleSet == null)
+            {
+                return;
+            }
+
+            foreach (ValidationRule rule in ruleSet)
+            {
+                Add(rule);
+            }
+        }
+
         /// <summary>
         /// Initializes a new instance of the <see cref="ValidationRuleSet"/> class.
         /// </summary>
         /// <param name="rules">Rules to be contained in this ruleset.</param>
         public ValidationRuleSet(IEnumerable<ValidationRule> rules)
         {
-            if (rules != null)
+            if (rules == null)
             {
-                foreach (ValidationRule rule in rules)
-                {
-                    Add(rule);
-                }
+                return;
+            }
+
+            foreach (ValidationRule rule in rules)
+            {
+                Add(rule);
             }
         }
 
         /// <summary>
         /// Gets the rules in this rule set.
         /// </summary>
-        public IEnumerable<ValidationRule> Rules
+        public IList<ValidationRule> Rules
         {
             get
             {
-                return _rules.Values.SelectMany(v => v);
+                return _rules.Values.SelectMany(v => v).ToList();
             }
         }
 
         /// <summary>
-        /// Add the new rule into rule set.
+        /// Add the new rule into the rule set.
         /// </summary>
         /// <param name="rule">The rule.</param>
         public void Add(ValidationRule rule)
         {
-            IList<ValidationRule> typeRules;
-            if (!_rules.TryGetValue(rule.ElementType, out typeRules))
+            if (!_rules.ContainsKey(rule.ElementType))
             {
-                typeRules = new List<ValidationRule>();
-                _rules[rule.ElementType] = typeRules;
+                _rules[rule.ElementType] = new List<ValidationRule>();
             }
 
-            if (typeRules.Contains(rule))
+            if (_rules[rule.ElementType].Contains(rule))
             {
                 throw new OpenApiException(SRResource.Validation_RuleAddTwice);
             }
 
-            typeRules.Add(rule);
+            _rules[rule.ElementType].Add(rule);
         }
 
         /// <summary>
@@ -97,7 +119,7 @@ public void Add(ValidationRule rule)
         /// <returns>The enumerator.</returns>
         public IEnumerator<ValidationRule> GetEnumerator()
         {
-            foreach (List<ValidationRule> ruleList in _rules.Values)
+            foreach (var ruleList in _rules.Values)
             {
                 foreach (var rule in ruleList)
                 {

test/Microsoft.OpenApi.Tests/Services/OpenApiValidatorTests.cs

@@ -11,12 +11,20 @@
 using Microsoft.OpenApi.Validations;
 using Microsoft.OpenApi.Writers;
 using Xunit;
+using Xunit.Abstractions;
 
 namespace Microsoft.OpenApi.Tests.Services
 {
     [Collection("DefaultSettings")]
     public class OpenApiValidatorTests
     {
+        private readonly ITestOutputHelper _output;
+
+        public OpenApiValidatorTests(ITestOutputHelper output)
+        {
+            _output = output;
+        }
+
         [Fact]
         public void ResponseMustHaveADescription()
         {
@@ -90,8 +98,8 @@ public void ServersShouldBeReferencedByIndex()
         [Fact]
         public void ValidateCustomExtension()
         {
-
-            var ruleset = Validations.ValidationRuleSet.DefaultRuleSet;
+            var ruleset = ValidationRuleSet.GetDefaultRuleSet();
+            
             ruleset.Add(
              new ValidationRule<FooExtension>(
                  (context, item) =>
@@ -102,7 +110,6 @@ public void ValidateCustomExtension()
                      }
                  }));
 
-
             var openApiDocument = new OpenApiDocument();
             openApiDocument.Info = new OpenApiInfo()
             {

test/Microsoft.OpenApi.Tests/Validations/ValidationRuleSetTests.cs

@@ -1,13 +1,19 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
 
-using System.Linq;
 using Xunit;
+using Xunit.Abstractions;
 
 namespace Microsoft.OpenApi.Validations.Tests
 {
     public class ValidationRuleSetTests
     {
+        private readonly ITestOutputHelper _output;
+
+        public ValidationRuleSetTests(ITestOutputHelper output)
+        {
+            _output = output;
+        }
 
         [Fact]
         public void DefaultRuleSetReturnsTheCorrectRules()
@@ -27,7 +33,7 @@ public void DefaultRuleSetReturnsTheCorrectRules()
         public void DefaultRuleSetPropertyReturnsTheCorrectRules()
         {
             // Arrange & Act
-            var ruleSet = ValidationRuleSet.DefaultRuleSet;
+            var ruleSet = ValidationRuleSet.GetDefaultRuleSet();
             Assert.NotNull(ruleSet); // guard
 
             var rules = ruleSet.Rules;
@@ -36,9 +42,8 @@ public void DefaultRuleSetPropertyReturnsTheCorrectRules()
             Assert.NotNull(rules);
             Assert.NotEmpty(rules);
 
-            // Temporarily removing this test as we get inconsistent behaviour on AppVeyor
-            // This needs to be investigated but it is currently holding up other work.
-            // Assert.Equal(14, rules.ToList().Count); // please update the number if you add new rule.
+            // Update the number if you add new default rule(s).
+            Assert.Equal(14, rules.Count);
         }
     }
 }