Implementation for the Open API validation by xuzhg · Pull Request #152 · microsoft/OpenAPI.NET

xuzhg · 2017-12-15T21:39:34Z

I want to implement the validation logic for the Open API element. Here's my design (it's not finished).
General design:
Can validation each part individually. for example, Developer create a object, he can call the validate api to validate the created object.
Validate rule can be customized. For example, developer can create their own validation rules.
Detail:
OpenApiElementValidator.cs has the public apis
.\Validations\Visitors has the visitor classes for each element.
.\Validations\Rules has the rule classes for each element.
Please have a look and give me some feedback.

PerthCharern · 2017-12-15T23:27:57Z

+
+            OpenApiExtensibleRules.ExtensionNameMustStartWithXDash,
+            // add more default rules.
+        };


This will be difficult to track. People will forget to put the new rules in here since this list is in a different file than the rules themselves.

I am thinking of this:

Add DefaultRuleSet for every OpenApiXXXRules file

This main DefaultRuleSet is a concatenation of all DefaultRuleSet's

What do you think? #Closed

In fact, we might not need this concatenated list at all since it will be broken down in Add() below anyway.

In reply to: 157320269 [](ancestors = 157320269)

Basically, I want to use the attribute for each "DOM" classes same as Add the validator attribute, validation rule and validator #133.
Let me think it more.

Why do you think it will break the Add() method? That's in deed will call "Add()" method. #Resolved

If you wanted to create an OpenApiValidationRuleAttribute and annotate each rule with that attribute and then have a discovery mechanism that searched the assembly to find all classes with that attribute, then I would have no objection to that.

In reply to: 157321750 [](ancestors = 157321750)

Overall I like the direction of this PR. I think by adding a simple marker attribute to each rule, you can reflect over the rules and built a default ruleset. You should be able to determine the target type from the generic return type of the rule.

This should address the concern brought up by @PerthCharern that someone might forget to add the rule to the ruleset. #Resolved

@xuzhg I didn't mean it will break the Add() method. I meant the list didn't have to be created because it would be enumerated in the Add() method anyway, so we can find a way to make it less complicated.

Anyway, I like @darrelmiller 's suggestion here. That would put the concern of when each rule should be used back to the rule itself. #Closed

Changed the design to use reflect.

In reply to: 157320436 [](ancestors = 157320436,157320269)

Changed.

In reply to: 157326026 [](ancestors = 157326026,157321750)

Changed.

In reply to: 157327739 [](ancestors = 157327739)

darrelmiller · 2017-12-15T23:51:40Z

Can I make a suggestion for doing these kind of design reviews? It would be helpful to see the code changes for a couple of examples rather than a complete implementation. Trying to review changes to 60 files in the GitHub UI is harder than it should be. #WontFix

darrelmiller · 2017-12-16T00:13:43Z

+
+        /// <summary>
+        ///  Validate the Open API element.
+        /// </summary>


Ok, so here's the deal! If I have to put up with these horrible comment blocks, I would like to see comments that actually justify their existence. So let's make the comment explain why I would pick one overload over the other so that I don't have to guess the reason based on what parameters are there. #Resolved

Great point. I will modify it.

In reply to: 157324617 [](ancestors = 157324617)

modified.

In reply to: 157324939 [](ancestors = 157324939,157324617)

darrelmiller · 2017-12-16T00:18:25Z

+        public ValidationContext(ValidationRuleSet ruleSet)
+        {
+            RuleSet = ruleSet ?? throw Error.ArgumentNull(nameof(ruleSet));
+        }


This is a personal preference, but I really don't like this syntax. I'd much rather see a guard clause and a separate assignment. Could we even use the Code Contracts library that would do static analysis? #WontFix

Will we have to enforce this though? It's a valid and acceptable C# syntax as far as I know, and I'm not sure if we should enforce it one way or another when someone submits a PR.

In reply to: 157325008 [](ancestors = 157325008)

It's ok. And it's VS recommendation.

In reply to: 157626597 [](ancestors = 157626597,157325008)

It's also VS recommendation to sort System. namespaces after Microsoft. namespaces. There be dragons with that argument ;-)

And nobody likes the idea of using Code Contracts so we actually get static analysis for these issues too?

darrelmiller · 2017-12-16T00:42:28Z

+    /// The base visitor class for Open Api elements.
+    /// </summary>
+    internal abstract class VisitorBase<T> : IVisitor where T : IOpenApiElement
+    {


This class has no state and we have an IVisitor interface. Is there any reason these two methods couldn't just be library functions? I don't think we need a base class for all the visitors. #Pending

Each visitor has two parts:

Visit itself's properties

Go to visit it's Children.

Where:
a. Visit() method is for the #1, it has the same logic for all elements. So, put its at the base class.

b. Next() method is for the #2, it has different logic for all elements. So, each element visitor class should override Next() method to list the children.

In reply to: 157326815 [](ancestors = 157326815)

Part of the reason I initially used the visitor pattern was so that we could define the traversal of the OpenAPI DOM just once and then have different visitors for different purposes. Validation is the obvious Visitor, but code generation could be another. Either client code gen, or server code scaffolding.
The Visitor related classes in this PR are specifically tied to validation via the ValidationContext. Even the abstract base class and IVisitor interface.

If you look at the Visitor pattern https://en.wikipedia.org/wiki/Visitor_pattern here the Visitor implements all the visit methods for all the elements it is interested in. This allows the Visitor to hold shared state whilst doing the traversal. It also plays the role of the context object.

With the current approach, ignoring the fact that it is tied to validation, each new "visiting" capability needs to create classes for each and every element and then pass the context between them.

I'm going to see if I can use my walker and validationVisitor to execute your rules classes and we can compare the end result. #Pending

Sure!

In reply to: 157663995 [](ancestors = 157663995)

I was able to incorporate your Rules classes into the OpenApiValidator like this,

public class OpenApiValidator : OpenApiVisitorBase { readonly ValidationRuleSet _ruleSet; readonly ValidationContext _context; /// <summary> /// Create a vistor that will validate an OpenAPIDocument /// </summary> /// <param name="ruleSet"></param> public OpenApiValidator(ValidationRuleSet ruleSet = null) { _ruleSet = ruleSet ?? ValidationRuleSet.DefaultRuleSet; _context = new ValidationContext(_ruleSet); } public override void Visit(OpenApiDocument item) => Validate(item); public override void Visit(OpenApiInfo item) => Validate(item); public override void Visit(OpenApiContact item) => Validate(item); public override void Visit(OpenApiResponse item) => Validate(item); public IEnumerable<ValidationError> Errors => _context.Errors; private void Validate<T>(T item) { if (item == null) return; // Required fields should be checked by higher level objects var rules = _ruleSet.Where(r => r.ElementType == typeof(T)); foreach (var rule in rules) { rule.Evaluate(_context, item); } } } }

Obviously this doesn't have the Visit methods for all types yet, and some types may be more complex than just the simple expression. Also, the walker class would need to build the property path.
However, using this approach, we need one class for walking the DOM (we could create other walkers if other visitors need to traverse in a different way) and one class for dispatching the Visit methods to the rules. This makes implementing other types of Visitors easier by only requiring one new class.
My understanding is that this approach would remove the need for :

the Visitor class for each element

the OpenApiVisitorSet

the ValidationContextExtensions

I'm still not quite sure how the ValidationMap and ValidateCollection extensions fit into this approach.
#Pending

It's ok. Please share your thoughts and codes. We can discuss to select one or just merge together.

In reply to: 157841020 [](ancestors = 157841020)

xuzhg · 2017-12-18T22:19:16Z

@darrelmiller @PerthCharern I added the attribute for Rule classes. Would you please take a look at da5f90a #WontFix

xuzhg · 2017-12-18T23:01:37Z

@darrelmiller @PerthCharern I also added the attribute for visitor classes. Would you please take a look at :

4a6ca43 #WontFix

xuzhg · 2017-12-18T23:02:36Z

+            {
+                if (_visitors == null)
+                {
+                    _visitors = new Lazy<IDictionary<Type, IVisitor>>(() => BuildVisitorSet(), isThreadSafe: false).Value;


Maybe Lazy is unnecessary? #Closed

I would say the underlying operation is not that time consuming/complex, so I think it's unnecessary. But I don't really mind using it if you prefer.

In reply to: 157624690 [](ancestors = 157624690)

Ok.

In reply to: 157634816 [](ancestors = 157634816,157624690)

PerthCharern · 2017-12-18T23:09:33Z

+    /// <summary>
+    /// The Validator attribute.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)]


Class [](start = 37, length = 5)

Why class attribute vs field/property attribute? #Resolved

It's easy to target "Rule" Class.

If it's target on the Rule property, we should go through all the properties in all the types, it's time consuming. #Pending

PerthCharern · 2017-12-19T00:07:43Z

You just meant the logic to traverse all the Visitors, not a C# attribute, right? I want to make sure I'm not missing something.

In reply to: 352585589 [](ancestors = 352585589)

xuzhg · 2017-12-19T00:09:18Z

@PerthCharern For the visitor, Yes. I have the interface IVisitor. That's enough. #WontFix

PerthCharern · 2017-12-19T02:12:54Z

+
+            // Assert
+            var exception = Assert.Throws<OpenApiException>(test);
+            Assert.Equal("Can not find visitor type registered for type 'Microsoft.OpenApi.Validations.Visitors.Tests.OpenApiVisitorSetTests'.",


Can not find visitor type registered for type [](start = 26, length = 45)

This should be a public const in the main project and referenced here. #Pending

@PerthCharern

Yes. I can use the
Assert.Equal(String.Format(SRResource.xxxxx, typeof(xxx).FullName), exception.Message);
However, it looks not good than the plain text in the test project. Because it can help us verify the error message thrown as our expected. What do you think?
#WontFix

@PerthCharern

Yes. I can use the

Assert.Equal(String.Format(SRResource.xxxxx, typeof(xxx).FullName), exception.Message);

However, it looks not good than the plain text in the test project. Because it can help us verify the error message thrown as our expected. What do you think?

In reply to: 157649686 [](ancestors = 157649686)

I think we should go with that instead of duplicating string literals. We are essentially checking that the exception thrown has the "right string", not that it is spelled correctly, etc.

I would say we get better verification from referring to the constant string in SRResource.

In reply to: 157832135 [](ancestors = 157832135)

Changed.

In reply to: 157882006 [](ancestors = 157882006,157832135)

PerthCharern · 2017-12-19T02:13:52Z

+        public void VisitorsPropertyReturnsTheCorrectVisitorList()
+        {
+            for (int i = 0; i < 5; i++) // 5 is just a magic number
+            {


Not sure what the loop helps with. Maybe remove? #Closed

I just want to test multiple retrieving the property "Visitors" can work as expected. #Closed

I just want to test multiple retrieving the property "Visitors" can work as expected.

In reply to: 157649800 [](ancestors = 157649800)

PerthCharern · 2017-12-19T02:14:12Z

+                // Assert
+                Assert.NotNull(visitors);
+                Assert.NotEmpty(visitors);
+                Assert.Equal(29, visitors.Count);


Assert.Equal(29, visitors.Count); [](start = 16, length = 33)

Provide comment #Closed

PerthCharern · 2017-12-19T02:14:55Z

+        [InlineData(typeof(OpenApiDocument), typeof(DocumentVisitor))]
+        [InlineData(typeof(OpenApiInfo), typeof(InfoVisitor))]
+        [InlineData(typeof(OpenApiXml), typeof(XmlVisitor))]
+        [InlineData(typeof(OpenApiComponents), typeof(ComponentsVisitor))]


[InlineData(typeof(OpenApiComponents), typeof(ComponentsVisitor))] [](start = 8, length = 66)

Can we add all types here? It's a one time thing, and it will help ensure no mistakes in the real code. #Closed

Added.

In reply to: 157649917 [](ancestors = 157649917)

PerthCharern · 2017-12-19T02:17:05Z

+                            ruleSet.Add(rule);
+                        }
+                    }
+                }


It's a little risky to me that we rely on the type and not an attribute assigned to each property. #Closed

It is okay for now though. I don't have a better way than to tag all properties, which is something we want to avoid.

In reply to: 157650194 [](ancestors = 157650194)

Yes. Basically, that's convention. It's ok if we follow up this convention.

In reply to: 157650606 [](ancestors = 157650606,157650194)

PerthCharern · 2017-12-19T02:18:50Z

+                {
+                    rule.Evaluate(context, extensbile);
+                }
+            }


Why is this not done in Visit above? #Pending

:) My design is

Visit() method is used to verify the itself properities

Next() method is used to verify the Children element.

So, I think Extension belongs to the Children scope. What do you think? #WontFix

:) My design is

Visit() method is used to verify the itself properities

Next() method is used to verify the Children element.
So, I think Extension belongs to the Children scope. What do you think?

In reply to: 157650373 [](ancestors = 157650373)

Why does Extension fall into the Children scope? Isn't it one of the properties?

In reply to: 157838009 [](ancestors = 157838009,157650373)

PerthCharern · 2017-12-19T02:19:54Z

+        /// </summary>
+        /// <param name="context">The validation context.</param>
+        /// <param name="item">The element.</param>
+        public void Visit(ValidationContext context, object item)


object [](start = 53, length = 6)

Can this be T? #WontFix

I tried, But i can't.

In reply to: 157650480 [](ancestors = 157650480)

PerthCharern · 2017-12-19T02:20:16Z

+        /// <param name="element">The element.</param>
+        protected virtual void Next(ValidationContext context, T element)
+        {
+            IOpenApiExtensible extensbile = element as IOpenApiExtensible;


extensbile [](start = 31, length = 10)

typo, extensible #Closed

Resolved. #Resolved

PerthCharern · 2017-12-19T02:22:25Z

+            Assert.NotNull(errors);
+
+            Assert.Equal(new[] { "The field 'url' in 'info' object is REQUIRED.",
+                "The field 'version' in 'info' object is REQUIRED."}, errors.Select(e => e.ErrorMessage));


These strings in all test classes should come from the main project, so that we don't need to keep track of the strings in two places. #Pending

Same as others:

Yes. I can use the

Assert.Equal(String.Format(SRResource.xxxxx, typeof(xxx).FullName), exception.Message);

However, it looks not good than the plain text in the test project. Because it can help us verify the error message thrown as expected. What do you think?

In reply to: 157650687 [](ancestors = 157650687)

PerthCharern · 2017-12-19T02:22:54Z

+        /// <returns>The error string.</returns>
+        public override string ToString()
+        {
+            return "ErroCode: " + ErrorCode + ", " + ErrorPath + " | " + ErrorMessage;


ErroCode [](start = 20, length = 8)

typo, ErrorCode #Closed

PerthCharern · 2017-12-19T02:24:55Z

+            Assert.False(result);
+            Assert.NotNull(errors);
+            ValidationError error = Assert.Single(errors);
+            Assert.Equal("The field 'description' in 'response' object is REQUIRED.", error.ErrorMessage);


Should we check that the path and the reason are also as expected? #Closed

Good catch. I added such here.

Such test cases is helping us to understand the design. I'd like to make sure we can finalize the design first.

In reply to: 157651079 [](ancestors = 157651079)

PerthCharern · 2017-12-19T02:25:53Z

+        protected override void Next(ValidationContext context, OpenApiPathItem pathItem)
+        {
+            Debug.Assert(context != null);
+            Debug.Assert(pathItem != null);


We haven't been using this anywhere else. Can we be consistent and use the normal null checks? #Pending

@Perth,

Yes. However, it's a protected method. It's only be called from "Visit(...)" method. So, I think it's only to add such Debug assert.

And, yes. Maybe I should add the null check at the "Visit(...)" method.

In reply to: 157651170 [](ancestors = 157651170)

protected class is practically a public class (since our customer can technically just create a class that inherits the PathItemVisitor and calls the method).

I think we should do the null check here.

In reply to: 157838274 [](ancestors = 157838274,157651170)

PerthCharern · 2017-12-19T02:26:43Z

+            new ValidationRule<OpenApiExternalDocs>(
+                (context, item) =>
+                {
+                    // title


title [](start = 23, length = 5)

url #Closed

Thx.

In reply to: 157651258 [](ancestors = 157651258)

PerthCharern · 2017-12-19T02:26:55Z

+            new ValidationRule<OpenApiResponse>(
+                (context, response) =>
+                {
+                    // title


title [](start = 23, length = 5)

description #Resolved

PerthCharern · 2017-12-19T02:27:16Z

+    public static class OpenApiTagRules
+    {
+        /// <summary>
+        /// REQUIRED.


REQUIRED [](start = 12, length = 8)

change comment #Resolved

PerthCharern · 2017-12-19T02:27:52Z

+        /// Input string must be in the format of an email address
+        /// </summary>
+        /// <param name="input">The input string.</param>
+        /// <returns></returns>


remove or fill in #Resolved

PerthCharern · 2017-12-19T02:35:49Z

+        /// </summary>
+        /// <param name="input">The input string.</param>
+        /// <returns></returns>
+        public static bool IsEmailAddress(this string input)


IsEmailAddress [](start = 27, length = 14)

The accepted answer here looks reasonable to me:

https://stackoverflow.com/questions/1365407/c-sharp-code-to-validate-email-address

It is though a little more forgiving than it has to be, but I believe it is strict enough for our purpose. #WontFix

PerthCharern

🕐

xuzhg · 2017-12-19T19:07:37Z

For the visitor, Yes. I have the interface IVisitor. That's enough.

In reply to: 352597979 [](ancestors = 352597979,352585589)

PerthCharern · 2017-12-19T21:44:49Z

+        /// <param name="item">The element.</param>
+        public void Visit(ValidationContext context, object item)
+        {
+            Debug.Assert(item is T, "item should be " + typeof(T));


Debug.Assert(item is T, "item should be " + typeof(T)); [](start = 12, length = 55)

Remove Debug.Assert and use a normal check #Resolved

…the product codes

xuzhg · 2017-12-20T00:57:39Z

@PerthCharern @darrelmiller

xuzhg mentioned this pull request Dec 15, 2017

Init implementation for the Open API validation #125

Closed

xuzhg changed the title ~~Validation~~ Implementation for the Open API validation Dec 15, 2017

xuzhg mentioned this pull request Dec 15, 2017

Add the validator attribute, validation rule and validator #133

Closed

xuzhg requested review from PerthCharern and darrelmiller December 15, 2017 22:26

PerthCharern reviewed Dec 15, 2017

View reviewed changes

darrelmiller reviewed Dec 16, 2017

View reviewed changes

xuzhg added 7 commits December 18, 2017 10:26

Init implementation for the Open API validation

7316184

add the whole visitor classes for validation

0988b1b

add the rules and modify the header copyright

620b0f2

Add test cases for external docs and response

31a3cd9

add a validation rule for the OAuthFlow object

401072c

modify to give more meaningful comments

bd39460

add the OpenApiRuleAttribute for each rule classes

da5f90a

xuzhg force-pushed the Validation branch from adb625d to da5f90a Compare December 18, 2017 22:17

change to lazy loading for the validation visitors

4a6ca43

xuzhg commented Dec 18, 2017

View reviewed changes

Change the namespace for the Validation test files

2e7e2dd

PerthCharern reviewed Dec 18, 2017

View reviewed changes

Add the test cases for ValidationRuleSet

aebee6f

PerthCharern reviewed Dec 19, 2017

View reviewed changes

PerthCharern suggested changes Dec 19, 2017

View reviewed changes

resolve the comments, typos and some test codes

df86936

PerthCharern reviewed Dec 19, 2017

View reviewed changes

Change debug.assert to throw arugment null and use the resource from …

b29d532

…the product codes

PerthCharern mentioned this pull request Jan 2, 2018

Alternative Validation implementation #163

Merged

darrelmiller merged commit b29d532 into microsoft:master Jan 11, 2018

Uh oh!

Conversation

xuzhg commented Dec 15, 2017 • edited by PerthCharern Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Dec 16, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Dec 16, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller commented Dec 15, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darrelmiller Dec 16, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Dec 16, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Dec 16, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Dec 19, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Dec 19, 2017 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuzhg commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuzhg commented Dec 15, 2017 •

edited by PerthCharern

Loading

PerthCharern Dec 15, 2017 •

edited

Loading

xuzhg Dec 15, 2017 •

edited

Loading

darrelmiller Dec 16, 2017 •

edited by xuzhg

Loading

PerthCharern Dec 16, 2017 •

edited by xuzhg

Loading

darrelmiller commented Dec 15, 2017 •

edited by xuzhg

Loading

darrelmiller Dec 16, 2017 •

edited by xuzhg

Loading

darrelmiller Dec 16, 2017 •

edited by xuzhg

Loading

darrelmiller Dec 16, 2017 •

edited by xuzhg

Loading

xuzhg Dec 16, 2017 •

edited

Loading

darrelmiller Dec 19, 2017 •

edited by xuzhg

Loading

darrelmiller Dec 19, 2017 •

edited by xuzhg

Loading

xuzhg commented Dec 18, 2017 •

edited

Loading

xuzhg commented Dec 18, 2017 •

edited

Loading

xuzhg Dec 18, 2017 •

edited by PerthCharern

Loading

PerthCharern Dec 18, 2017 •

edited by xuzhg

Loading

xuzhg Dec 18, 2017 •

edited

Loading

xuzhg commented Dec 19, 2017 •

edited

Loading

PerthCharern Dec 19, 2017 •

edited by xuzhg

Loading

xuzhg Dec 19, 2017 •

edited

Loading

PerthCharern Dec 19, 2017 •

edited

Loading

xuzhg Dec 19, 2017 •

edited

Loading