Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion src/Microsoft.OpenApi/Validations/OpenApiValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
68 changes: 45 additions & 23 deletions src/Microsoft.OpenApi/Validations/ValidationRuleSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

   return new ValidationRuleSet(_defaultRuleSet); [](start = 4, length = 54)

We could alternatively prescribe that the caller never modifies the DefaultRuleSet and always uses it like this if they need to use default rules + more rules:

new ValidationRuleSet(ValidationRuleSet.DefaultRuleSet)

But then that's kinda risky and people will not always follow the best practice.

Creating the new instance here safeguards against that scenario as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating the new collection on each call seems like the best approach. The rules don't need to get re-instantiated so the cost should be minimal. The other option would be to return a ReadOnlyCollection.

BTW, thanks for finding this. I was looking in the completely wrong direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.

A ReadOnlyCollection is something I wanted to do, but it will require more changes since we return the default rule set now as a class object, not as a list/dictionary of rules. I'll go with the current implementation for now, and we can revisit if this turns out not working well.

}

/// <summary>
Expand All @@ -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>
Expand All @@ -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)
{
Expand Down
13 changes: 10 additions & 3 deletions test/Microsoft.OpenApi.Tests/Services/OpenApiValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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) =>
Expand All @@ -102,7 +110,6 @@ public void ValidateCustomExtension()
}
}));


var openApiDocument = new OpenApiDocument();
openApiDocument.Info = new OpenApiInfo()
{
Expand Down
15 changes: 10 additions & 5 deletions test/Microsoft.OpenApi.Tests/Validations/ValidationRuleSetTests.cs
Original file line number Diff line number Diff line change
@@ -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()
Expand All @@ -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;
Expand All @@ -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);
}
}
}