Merge remote-tracking branch 'origin' into dm/ref-overflow

Author: darrelmiller

src/Microsoft.OpenApi.Readers/Microsoft.OpenApi.Readers.csproj

@@ -10,7 +10,7 @@
         <Company>Microsoft</Company>
         <Title>Microsoft.OpenApi.Readers</Title>
         <PackageId>Microsoft.OpenApi.Readers</PackageId>
-        <Version>1.0.0-beta012</Version>
+        <Version>1.0.0-beta013</Version>
         <Description>OpenAPI.NET Readers for JSON and YAML documents</Description>
         <Copyright>© Microsoft Corporation. All rights reserved.</Copyright>
         <PackageTags>OpenAPI .NET</PackageTags>

src/Microsoft.OpenApi/Microsoft.OpenApi.csproj

@@ -10,7 +10,7 @@
         <Company>Microsoft</Company>
         <Title>Microsoft.OpenApi</Title>
         <PackageId>Microsoft.OpenApi</PackageId>
-        <Version>1.0.0-beta012</Version>
+        <Version>1.0.0-beta013</Version>
         <Description>.NET models with JSON and YAML writers for OpenAPI specification</Description>
         <Copyright>© Microsoft Corporation. All rights reserved.</Copyright>
         <PackageTags>OpenAPI .NET</PackageTags>

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);
         }
     }
 }