Alternative Validation implementation #163

darrelmiller · 2017-12-20T19:10:02Z

This uses the Rules from @xuzhg 's PR but my Validator and Walker. The tests that @xuzhg created have been updated to use the Validator instead.

There are still some parts of the DOM graph that are not yet being visited. Walker tests will need to be implemented.

What I believe are the advantages of this approach:

Walker can be re-used for purposes other than validation
A new Visitor can be created as easily as just creating one class and overloading the methods you want to visit.
Visitor classes are replaced by a set of methods on a Single Visitor class
The Validation visitor class plays the role of VisitorSet

It is almost 1000 LOC less :-)

…the product codes

xuzhg · 2017-12-20T19:29:29Z

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

+            var validator = new OpenApiValidator();
+            validator.Visit(components);
            // Act
-            bool result = components.Validate(out errors);


Now, the developer should create an object of OpenApiValidator and check the validator.Errors?

Why do you think the three line codes is better than the one line code call? components.Validate(out errors)?

@PerthCharern what do you think?

xuzhg · 2017-12-20T19:30:33Z

src/Microsoft.OpenApi/Validations/Visitors/IVisitor.cs

-    /// <summary>
-    /// The interface for visitor.
-    /// </summary>
-    internal interface IVisitor


Maybe I rename IVistor as IValidator can help to understand?

xuzhg · 2017-12-20T19:32:33Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

        /// </summary>
-        /// <param name="doc"></param>
+        /// <param name="doc">OpenApiDocument to be walked</param>
        public void Walk(OpenApiDocument doc)


That's my concern. Does the customer only walk starting from OpenApiDocument?

If you look at the tests, you can Visit an element directly. If we wanted to expose Walk entry points for sub-graphs we could do that too. But then in your next comment you suggest that OpenApiValidator should be internal.

Calling Visit is not the same as calling Walk though. Visit doesn't recursively walk into the children, right? I think it's still nice to provide a Walk-like feature at every level of the subgraph.

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

Sure it would be nice, but exposing functionality publicly that people can take a dependency on before we have any real experience with how well the validation works seems risky to me. Why don't we get the validation working as an internal feature and once we are comfortable with it, then consider exposing more. Unless we have a concrete usecase, I would rather not increase our support surface area.

In reply to: 158124882 [](ancestors = 158124882,158121892)

Do you know of use cases for doing anything else?

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

One possible use case I can envision:

Define a large reusable component (say, schema) in
a separate file and refer to it using Reference. User wants to make sure this component is defined correctly by running a validation on this component (schema), which is technically not in a document.

In reply to: 158125996 [](ancestors = 158125996,158115415)

The only minor problem we have with that at the moment is we don't support reading anything other than a complete document via our public interface. We will need to do that internally when pulling in external references.

In reply to: 158127399 [](ancestors = 158127399,158125996,158115415)

You are right. This subgraph-level discussion seems to be a recurring theme (writer, reader, and now walker).

I agree with you on this:

"It is worth noting that is much easier to add more granularity in a future release than to take it away."

So I think this level of exposure is fine as is as a beginning point.

On that note, there might be value in adding the Walk methods for all leaf nodes. In case we would like to flip the internal/public switch in the future, it will be a simple string change.

In reply to: 158128738 [](ancestors = 158128738,158127399,158125996,158115415)

xuzhg · 2017-12-20T19:35:43Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

+    /// <summary>
+    /// Class containing dispatchers to execute validation rules on for Open API document.
+    /// </summary>
+    public class OpenApiValidator : OpenApiVisitorBase


Normally, the OpenApiValidator should be internal.

I think it needs to be public in this implementation though. The user needs to instantiate OpenApiWalker with new OpenApiValidator() as an argument to perform validation.

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

@PerthCharern Sorry, Yes. In Darrel PR, it's public. But I mean it should be internal if we have a OpenApiValidator.

I'm not sure how we want to incorporate the Validator. We could make it an internal implementation detail, or we could allow users to initiate it directly via the Validator visitor.

If we can decide what are the use cases for validating fragments of the OpenApiDocument then we can decide what portions of the traversal algorithm we should expose as public. It is worth noting that is much easier to add more granularity in a future release than to take it away.

PerthCharern · 2017-12-20T19:48:29Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+
+            _visitor.Visit(doc as IOpenApiExtensible);
+
+        }


It took me a bit to understand how to distinguish the case when we simply call .Visit and the case where we need to have WalkXXX private method.

Would it be easier to read if we always create WalkXXX method for every OpenApiXXX and its collection properties? If it's something simple without further inner object like OpenApiInfo, then sure, WalkInfo will simply be a wrapper around _visitor.Visit(info).

My concern is that with the current implementation, in case someone mistakenly uses .Visit instead of Walk in certain places, it will be very difficult to spot.

If we always create a WalkXXX, the pattern for each WalkXXX is also always consistent, with one Visit call and a bunch of following Walk calls.

public void WalkObject(OpenApiObject object) { // Visit self _visitor.Visit(object); // Walk the rest WalkProperty1(object.Property1); WalkMapProperty2(object.MapProperty2); } public void WalkProperty1(OpenApiProperty1Type property1) { // Visit self _visitor.Visit(property1); // Walk the rest WalkProperty11(property1.Property11); WalkProperty12(property1.Property12); WalkProperty13(property1.Property13); } public void WalkMapProperty2(IDictionary<string, Property2Type> property2Map) { // Visit self _visitor.Visit(property2Map); // Walk the rest foreach( var property2 in property2Map.Values ) { WalkProperty2(property2); } }

This will also address Sam's concern about having other starting point for Walking.

Walk methods are just helper methods for grouping visits. Think of Walk methods as non-leaf nodes of the traversal tree and Visit as the leaf nodes. If you want to have a Walk for every leaf I suppose we could.

I think it's a good thing to have. Say, if customer wants to only walk his OpenApiComponents, having a WalkComponents will enable that feature.

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

You mean they only want to Validate the components, but not validate anything else? Or do you mean they want to only apply a visitor to the components? If so, they can create a class that derives from the VisitorBase, create an overload of the Visit(OpenApiComponent ) method and then use the regular walk.

In reply to: 158121661 [](ancestors = 158121661,158121152)

I meant the former. Given that the implementation consistency concern is relatively minor, this boils down to the same issue as Sam's.

We can just discuss in the other thread in OpenApiWalker.cs to avoid repetition.

In reply to: 158126857 [](ancestors = 158126857,158121661,158121152)

darrelmiller · 2017-12-20T20:39:49Z

Ok, @xuzhg @PerthCharern I hear your concerns. How about we meet in the middle? I will create Walk methods for every type that Visits the object and all child objects. But for the moment, we keep those methods internal. At least until we a) have specific case that requires us to make them public and b) we have gained a bit of confidence in the effectiveness of the approach.

I will make these changes and then we can make the final call as to whether we want take my approach of having all the visiting/walking methods in a single class, or Sam's approach of having a seperate class for each visitor.

Does that seem reasonable?

PerthCharern · 2017-12-20T20:46:38Z

Yes, I didn't see this comment, so I just proposed the same thing in the thread. :) This sounds good to me.

PerthCharern · 2018-01-02T22:31:04Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

+        /// Execute validation rules against an <see cref="OpenApiSchema"/>
+        /// </summary>
+        /// <param name="item">The object to be validated</param>
+        public override void Visit(OpenApiSchema item) => Validate(item);


item [](start = 49, length = 4)

schema

PerthCharern · 2018-01-02T22:31:10Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

+        /// Execute validation rules against an <see cref="OpenApiEncoding"/>
+        /// </summary>
+        /// <param name="item">The object to be validated</param>
+        public override void Visit(OpenApiEncoding item) => Validate(item);


item [](start = 51, length = 4)

encoding

PerthCharern · 2018-01-02T22:33:36Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+        {
+            _visitor.Visit(tag);
+            _visitor.Visit(tag.ExternalDocs);
+            _visitor.Visit(tag as IOpenApiExtensible);


These should be Walk, right?

PerthCharern · 2018-01-02T22:33:42Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+            {
+                _visitor.Visit(variable);
+            }
+            _visitor.Visit(server as IOpenApiExtensible);


These should be Walk, right?

PerthCharern · 2018-01-02T22:33:52Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+            _visitor.Visit(pathItem);
+
+            Walk(pathItem.Operations);
+            _visitor.Visit(pathItem as IOpenApiExtensible);


This should be Walk, right?

PerthCharern · 2018-01-02T22:34:15Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+                }
+            }
+
+            _visitor.Visit(response as IOpenApiExtensible);


These should be Walk, right?

PerthCharern · 2018-01-02T22:34:42Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+        {
+            foreach (var item in encoding.Values)
+            {
+                _visitor.Visit(item);


_visitor.Visit(item); [](start = 14, length = 23)

Walk?

PerthCharern

PerthCharern · 2018-01-02T22:43:11Z

I prefer this pattern over the current implementation in #152 since it separates the logic of walking and validating.

One concern that Sam raised earlier about the call pattern still remains unsolved:

In #152, the call pattern is simply:

            bool result = response.Validate(out errors);

But here, it's a bit more complicated and less intuitive:

            var validator = new OpenApiValidator();
            var walker = new OpenApiWalker(validator);
            walker.Walk(response);

As a customer, I would not like to repeat the three lines over and over. It also takes away the meaning of validation from the call line itself (i.e. it's easy to understand that response.Validate is performing validation, while it's not apparent what walker.Walk(response) attempts to achieve.).

What do you think about creating extension method for IOpenApiElement that takes care of calling the three lines?

darrelmiller · 2018-01-03T18:47:55Z

@PerthCharern I have created the extension method, but needed to do a giant type switch to dispatch the walker. Should push that change soon.

darrelmiller · 2018-01-03T20:19:13Z

@PerthCharern The Validate method isn't quite the same as @xuzhg proposed. I changed it to this way because the visitor (in this case OpenApiValidator) is responsible for the state during the walking of the subgraph. I don't really like the idea of passing external state (an existing list of errors) into the visitor. A consumer can always aggregate errors themselves. If someone wants to aggregate errors across different parts of the graph, then they might as well create their own custom walker.

Calling Validate and getting a list of errors back seems like a simpler interface for the case when someone wants to validate a subgraph.

Thoughts?

PerthCharern · 2018-01-03T21:31:38Z

I'm okay with either design.

I don't think Sam's design was originally meant for aggregating errors. His design kinda resembles what we have in the reader (diagnostic passed as out param). We don't really need that here since there's only one return object, and I don't foresee us having more "outputs" for Validate.

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

xuzhg · 2018-01-03T22:00:45Z

src/Microsoft.OpenApi/Extensions/OpenApiElementExtensions.cs

+        /// <typeparam name="T"></typeparam>
+        /// <param name="element"></param>
+        /// <returns></returns>
+        public static IEnumerable<ValidationError> Validate(this IOpenApiElement element)         {


So, for the customer/developer, he will check:

The return is not null

The return has elements.

OpenApiDocument doc = new OpenApiDocument(); var errors = doc.Validate(); if (errors == null || !errors.Any()) { // validation passed. } else { // validation failed. }

Checking errors.Any() should be sufficient because returning IEnumerable as null is just evil. I will add documentation to convey the fact that the function will never return null.

PerthCharern · 2018-01-10T23:38:17Z

@darrelmiller Are you waiting for anything from me or Sam? I think the overall structure looks good. We can help fill in the validation details in future PRs once this is checked in.

xuzhg and others added 17 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

change to lazy loading for the validation visitors

4a6ca43

Change the namespace for the Validation test files

2e7e2dd

Add the test cases for ValidationRuleSet

aebee6f

resolve the comments, typos and some test codes

df86936

Validation using walker and single visitor class

bc530c3

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

b29d532

…the product codes

Merge branch 'sam/validation3' into dm/validation

5eccf02

Remove visitor classes

0188565

Enhancements to walker

43ae516

Removed unused test

fc4c334

xuzhg reviewed Dec 20, 2017

View reviewed changes

PerthCharern reviewed Dec 20, 2017

View reviewed changes

darrelmiller added 2 commits December 22, 2017 22:55

Merge remote-tracking branch 'origin/master' into dm/validation

26561e3

Updated Walker to use separate methods for each element

2fd11fa

PerthCharern reviewed Jan 2, 2018

View reviewed changes

PerthCharern approved these changes Jan 2, 2018

View reviewed changes

Updated walkers and added validate method

d6dba10

Merge branch 'master' into dm/validation

9f7c7f6

xuzhg reviewed Jan 3, 2018

View reviewed changes

darrelmiller added 2 commits January 10, 2018 19:12

Merge branch 'master' into dm/validation

6c47d49

Merge branch 'master' into dm/validation

0a4c3ee

darrelmiller merged commit ba575c4 into master Jan 11, 2018

darrelmiller deleted the dm/validation branch January 4, 2020 20:27

Alternative Validation implementation #163

Alternative Validation implementation #163

Uh oh!

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Dec 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Dec 20, 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!

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 20, 2017

Uh oh!

PerthCharern commented Dec 20, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PerthCharern left a comment

Choose a reason for hiding this comment

Uh oh!

PerthCharern commented Jan 2, 2018

Uh oh!

darrelmiller commented Jan 3, 2018

Uh oh!

darrelmiller commented Jan 3, 2018

Uh oh!

PerthCharern commented Jan 3, 2018

Uh oh!

darrelmiller commented Dec 20, 2017 •

edited by PerthCharern

Loading

PerthCharern Dec 20, 2017 •

edited

Loading

PerthCharern Dec 20, 2017 •

edited

Loading