Validation for custom extensions by darrelmiller · Pull Request #176 · microsoft/OpenAPI.NET

darrelmiller · 2018-01-13T00:14:41Z

This PR incorporates the other strongly typed extension work and allows defining validation rules for those extension types.

xuzhg · 2018-01-13T00:16:56Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

+        private OpenApiReaderSettings _settings;

+        /// <summary>
+        /// 


fill? #Resolved

xuzhg · 2018-01-13T00:17:45Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

+        /// 
+        /// </summary>
+        /// <param name="settings"></param>
+        public OpenApiStreamReader(OpenApiReaderSettings settings = null)


it's better to separate two constructors? #WontFix

I don't think so. The optional parameter conveys that settings are optional and having one constructor prevents someone from adding code to one ctor that needs to be called from both.

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

xuzhg · 2018-01-13T00:19:01Z

src/Microsoft.OpenApi/Any/OpenApiObject.cs

        public AnyType AnyType { get; } = AnyType.Object;
+
+        /// <summary>
+        /// 


fill? #Resolved

xuzhg · 2018-01-13T00:19:26Z

src/Microsoft.OpenApi/Any/OpenApiArray.cs

+        /// Write out contents of OpenApiArray to passed writer
+        /// </summary>
+        /// <param name="writer">Instance of JSON or YAML writer.</param>
+        public void Write(IOpenApiWriter writer)


public? not internal? #WontFix

Yes it needs to be public because it is part of an interface that will be implemented by any custom extension implementation.

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

xuzhg · 2018-01-13T00:22:22Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

    {
-        readonly ValidationRuleSet _ruleSet;
-        readonly ValidationContext _context;
+


remove #Resolved

PerthCharern · 2018-01-18T21:04:50Z

using Microsoft.OpenApi.Writers;

nit, add copyright #Closed

Refers to: src/Microsoft.OpenApi/Interfaces/IOpenApiExtension.cs:1 in 7e1f151. [](commit_id = 7e1f151, deletion_comment = False)

PerthCharern · 2018-01-18T21:07:47Z

src/Microsoft.OpenApi/Services/OpenApiVisitorBase.cs

+        private readonly Stack<string> _path = new Stack<string>();
+
+        /// <summary>
+        /// Allow Rule to indicate validation error occured a deeper context level.  


[](start = 59, length = 1)

nit, preposition missing

still missing :)

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

Now it's fixed :-)

PerthCharern · 2018-01-18T21:26:18Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+            foreach (var pathItem in paths)
            {
-                Walk(pathItem);
+                _visitor.Enter(pathItem.Key.Replace("/","~1"));


~1" [](start = 57, length = 3)

nit, could you put comment in the code to explain that "~1" is an escape character for "/" in JSON pointer? #Closed

PerthCharern · 2018-01-18T21:31:53Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

@@ -101,6 +115,7 @@ internal void Walk(IList<OpenApiServer> servers)
                    Walk(server);
                }


We should add the array index to get the correct json pointer here.

for ( int i =0; i<servers.Count(); i++ ) { _visitor.Enter(i); Walk(servers[i]); _visitor.Exit(); } #Closed

PerthCharern · 2018-01-18T21:39:29Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

            foreach (var tag in tags)
            {
                Walk(tag);
            }


We should add the array index to get the correct json pointer here.

for ( int i =0; i<tags.Count(); i++ ) { _visitor.Enter(i); Walk(tags[i]); _visitor.Exit(); }

Also, this raises another point that the Enter call should be the responsibility of the caller. In this case, the Walk(tags[i]) should NOT need to call any enter for the Tag object itself (since the array index is used instead). #Closed

PerthCharern · 2018-01-18T21:40:18Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

        /// <param name="tags"></param>
        internal void Walk(IList<OpenApiTag> tags)
        {
+            _visitor.Enter(OpenApiConstants.Tags);


_visitor.Enter(OpenApiConstants.Tags); [](start = 8, length = 42)

Enter/Exit should be responsibility of the caller (e.g. in this case, it should go in the Walk(doc) ).

This will ensure that we don't do duplicate Enter calls for an object in an array (like the tags array). #Closed

You already kinda handled this for Tag (but not for Server), but moving the Error/Exits to the caller altogether would make it more consistent.

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

I opened another comment to discuss this.

In reply to: 162478481 [](ancestors = 162478481,162477655)

PerthCharern · 2018-01-18T21:41:31Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

                foreach (var parameter in parameters)
                {
                    Walk(parameter);
                }


need indexing - see my other comments. #Closed

PerthCharern · 2018-01-18T21:44:32Z

using System;

nit, copyright, and how is this file used? #Closed

Refers to: src/Microsoft.OpenApi/Services/VisitorContext.cs:1 in 7e1f151. [](commit_id = 7e1f151, deletion_comment = False)

PerthCharern · 2018-01-18T21:47:31Z

src/Microsoft.OpenApi/Validations/ValidationContext.cs

-                return _errors;
-            }
-        }
+        void Exit();


Why are these needed in the context? #Closed

Because often the rules validate additional depth. Rules add segments for properties in objects being validated. Also, for custom validation rules may validate several levels of object depth.

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

PerthCharern · 2018-01-18T21:48:08Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

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


, IValidationContext [](start = 54, length = 21)

It's weird that the Validator is implementing the Context interface. Do we need the context interface at all?

We could pass the entire validator object into rules, however this exposes a bunch of properties that Rules don't need access to. The way it was implemented before was the state of the validator was refactored out into an aggregated object and the state object was passed into the Rules. Rather than have the validator be made up of two objects, I just use the IValidationContext as a filter to limit what parts of the Validator the rules are allowed to access.

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

Ah that makes sense. Thanks for the explanation.

I think the design itself is fine, but the nomenclature still throws me off a bit. I would expect the interface to be called something along the line of "IValidationContextHolder".

That name is mouthful though... so I'm also fine with the current state.

In reply to: 163074706 [](ancestors = 163074706,162479509)

PerthCharern · 2018-01-18T21:55:32Z

}

Can we add tests with data that is "incorrect" in an array? (something like a tag in tags or server in servers), so that we can check that our Jsonpointer looks correct in that case #Closed

Refers to: test/Microsoft.OpenApi.Tests/Services/OpenApiValidatorTests.cs:56 in 7e1f151. [](commit_id = 7e1f151, deletion_comment = False)

PerthCharern

🕐

darrelmiller · 2018-01-22T21:38:05Z

using System;

I deleted it.

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

Refers to: src/Microsoft.OpenApi/Services/VisitorContext.cs:1 in 7e1f151. [](commit_id = 7e1f151, deletion_comment = False)

PerthCharern · 2018-01-23T18:09:46Z

src/Microsoft.OpenApi/Services/OpenApiVisitorBase.cs

 {
    /// <summary>
-    /// Open API visitor base providing base validation logic for each <see cref="IOpenApiElement"/>
+    /// Open API visitor base provides common logic for concrete vistors


vistors [](start = 65, length = 7)

visitors

PerthCharern · 2018-01-29T19:49:49Z

test/Microsoft.OpenApi.Tests/Walkers/WalkerLocationTests.cs

@@ -0,0 +1,229 @@
+using System.Collections.Generic;


nit, copyright

PerthCharern · 2018-01-29T19:55:25Z

test/Microsoft.OpenApi.Tests/Walkers/WalkerLocationTests.cs

+        }
+    }
+
+    public class LocatorVisitor : OpenApiVisitorBase


This is actually a pretty nice feature. What do you think about moving it to the source, instead of leaving it in tests?

This doesn't have to be done with this PR though. Maybe let's change this to private and we can decide?

PerthCharern · 2018-01-29T19:59:38Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

-                _visitor.Exit();
+                foreach (var pathItem in paths)
+                {
+                    InContext(pathItem.Key.Replace("/", "~1"), () => Walk(pathItem.Value));// JSON Pointer uses ~1 as an escape character for /


This "Replace" should technically apply to anything in the JSON pointer (not just pathItem names). Can we move it to the InContext method?

PerthCharern · 2018-01-29T20:04:49Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

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


Should this be InContext?

PerthCharern · 2018-01-29T20:05:54Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

        }

+        /// <summary>
+        /// Adds a segment to the context path to enable pointing to the current locationin the document


nit, space missing between location and in

PerthCharern · 2018-01-29T20:07:43Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+        /// </summary>
+        /// <param name="context">An identifier for the context.</param>
+        /// <param name="walk">An action that walks objects within the context.</param>
+        private void InContext(string context, Action walk)


nit, InContext is a little strange as a method name.

This is still a Walk method to me (just with a context name and a generic walk that is not tied to an object). Maybe it should be called Walk?

PerthCharern · 2018-01-29T20:10:47Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

    {
        private readonly OpenApiVisitorBase _visitor;
+        private readonly Stack<OpenApiSchema> _schemaLoop = new Stack<OpenApiSchema>();
+        private readonly Stack<OpenApiPathItem> _pathItemLoop = new Stack<OpenApiPathItem>();


Wow. I didn't realize before today that pathItem can also cause cycles (via Operation -> Callback)!

PerthCharern · 2018-01-29T20:13:59Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+                foreach (var item in components.Schemas)
+                {
+                    InContext(item.Key, () => Walk(item.Value));
+                }


Each of these loops should be in another InContext:

InContext( OpenApiConstants.Schemas, () => foreach (var item in components.Schemas) { InContext(item.Key, () => Walk(item.Value)); } );

PerthCharern · 2018-01-29T20:15:22Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

-                _visitor.Exit();
+                foreach (var mediaType in content)
+                {
+                    InContext(mediaType.Key.Replace("/", "~1"), () => Walk(mediaType.Value));


Let's move the Replace into InContext since it's used multiple times.

PerthCharern

Left a few more comments. Once those are addressed, I think we are good to go. :)

PerthCharern · 2018-01-29T20:26:25Z

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

            Assert.NotNull(rules);
            Assert.NotEmpty(rules);
-            Assert.Equal(13, rules.ToList().Count); // please update the number if you add new rule.
+            Assert.Equal(14, rules.ToList().Count); // please update the number if you add new rule.


I don't think this test is needed. It doesn't really add much value, and will cause maintenance issue (like the current test failure).

PerthCharern · 2018-01-29T20:27:13Z

test/Microsoft.OpenApi.Readers.Tests/TestCustomExtension.cs

@@ -0,0 +1,69 @@
+using FluentAssertions;


nit, copyright

darrelmiller · 2018-01-30T16:19:15Z

I was really hoping to merge this today. But I cannot repro this failing test locally. Even running the Xunit console runner in release mode does not repro this fail. sigh.

PerthCharern · 2018-01-30T17:34:09Z

Maybe I misunderstood something but based on the AppVeyor TESTS tab, it looks like the failing test is the one counting number of rules. I also left a comment earlier suggesting removing that test since it doesn’t really add much value and requires high maintenance. Does this solve the problem, or are you seeing other tests failing on AppVeyor as well?

darrelmiller · 2018-01-30T19:02:21Z

@PerthCharern I could remove it and that would make the problem go away, but I am concerned that for some reason when reflecting over the available Rules, we are picking up the FooExtension test that is not supposed to be in the set of default rules. I may remove the test temporarily to get these changes merged in, but I would really like to know why the problem is happening.

PerthCharern · 2018-01-30T20:18:29Z

@darrelmiller My mistake. I wasn't aware that the number increase was not intentional. For some reason, I thought you added a rule and simply did not update the number.

PerthCharern · 2018-01-31T01:22:35Z

@darrelmiller I also can't repro it locally.

One thing that might be helpful: Make the test output something (using ITestOutputHelper) and we can see the output on AppVeyor.

Given that we can't see the issue locally, I wonder if this has something to do with the lazy initialization with isThreadSafe being true.

darrelmiller · 2018-01-31T01:51:07Z

@PerthCharern Thanks for all the comments. I'll make the changes later this evening.

I do think the test problem is a threading issue (although is very consistent!). Previously isThreadSafe was false and I changed it because from the docs it appears that setting it to true adds in a lock. The parameter isn't asking a question, it is a directive.

I might try pre-reading the ruleset in the test setup to see if that makes the problem go away.

PerthCharern · 2018-01-31T18:49:23Z

@darrelmiller I think I know what's going on. I'll send out a PR today.

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.

darrelmiller added 9 commits January 7, 2018 14:43

Use rules to validate required properties instead of reader

7b42b1b

Use extension to validate

4ffe083

Added context to validators and removed required check from reader

2789c0b

Removed remnants of required

fed72e4

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

909a0d7

Added support for custom extensions

7ca70f2

Merged validation code

60a26b2

Implemented extension validation

41b049d

Extension validation

048518a

xuzhg reviewed Jan 13, 2018

View reviewed changes

Fixes based on PR reviews

a42e62a

darrelmiller mentioned this pull request Jan 16, 2018

Design what is needed for pluggable strongly typed extensions that can be validated #167

Closed

Merge branch 'master' into dm/validatedextensions

7e1f151

PerthCharern mentioned this pull request Jan 18, 2018

Added support for custom extensions #171

Closed

PerthCharern reviewed Jan 18, 2018

View reviewed changes

PerthCharern suggested changes Jan 18, 2018

View reviewed changes

darrelmiller added 2 commits January 22, 2018 16:39

Validation fixes based on PR review

e9d2569

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

3fd21d2

PerthCharern reviewed Jan 23, 2018

View reviewed changes

PerthCharern reviewed Jan 29, 2018

View reviewed changes

PerthCharern approved these changes Jan 29, 2018

View reviewed changes

PerthCharern reviewed Jan 29, 2018

View reviewed changes

darrelmiller added 2 commits January 30, 2018 09:58

Added copyright header

0d68e70

Changed configuration of Lazy to make it threadsafe

9dba5fd

Updated visitbility on rules to be consistent

2e7c33a

Streamlined rule loading code

474944d

darrelmiller added 3 commits January 30, 2018 22:32

Tried pre-loading the ruleset

576de46

Removed use of Lazy

8d44882

temporarily fixed failing test

de7e355

darrelmiller merged commit 4118028 into master Jan 31, 2018

darrelmiller deleted the dm/validatedextensions branch January 31, 2018 15:52

PerthCharern mentioned this pull request Jan 31, 2018

Re-enable the default rule count test case #200

Merged

		@@ -101,6 +115,7 @@ internal void Walk(IList<OpenApiServer> servers)
		Walk(server);
		}

Comments

Conversation

darrelmiller commented Jan 13, 2018

Uh oh!

xuzhg Jan 13, 2018 • edited by darrelmiller 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 Jan 13, 2018 • edited by darrelmiller 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 Jan 13, 2018 • edited by darrelmiller 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 Jan 13, 2018 • edited by darrelmiller 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 Jan 13, 2018 • edited by darrelmiller 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 commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

PerthCharern commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern Jan 18, 2018 • 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!

PerthCharern Jan 18, 2018 • 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!

xuzhg Jan 13, 2018 •

edited by darrelmiller

Loading

xuzhg Jan 13, 2018 •

edited by darrelmiller

Loading

xuzhg Jan 13, 2018 •

edited by darrelmiller

Loading

xuzhg Jan 13, 2018 •

edited by darrelmiller

Loading

xuzhg Jan 13, 2018 •

edited by darrelmiller

Loading

PerthCharern commented Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern commented Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern Jan 18, 2018 •

edited

Loading

PerthCharern commented Jan 18, 2018 •

edited

Loading

PerthCharern Jan 29, 2018 •

edited

Loading

PerthCharern Jan 29, 2018 •

edited

Loading