Update Reference implementation to handle all tag, security scheme, and $ref-style pointers correctly. #90

PerthCharern · 2017-11-15T03:10:37Z

Update Reference implementation to handle all tag, security scheme, and $ref-style pointers correctly.
Unit tests will follow tomorrow. Given that this set of changes is quite large, I have uploaded this PR in case you guys have some free time to look at it tomorrow while I'm still working on the unit tests. I'll follow up as soon as I'm done.

Fixes #81

…nceable

…cheme, and other ref-style referenceable objects.

…f LoadTag

msftclas · 2017-11-15T03:10:42Z

@PerthCharern,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

PerthCharern · 2017-11-15T03:13:16Z

src/Microsoft.OpenApi/Models/OpenApiSecurityRequirement.cs

@@ -35,7 +35,7 @@ internal override void WriteAsV3(IOpenApiWriter writer)

            foreach (var scheme in Schemes)
            {
-                writer.WritePropertyName(scheme.Key.Pointer.Name);
+                writer.WritePropertyName(scheme.Key.Pointer.Id);


writer.WritePropertyName(scheme.Key.Pointer.Id); [](start = 16, length = 48)

will modify this to leverage WriteAsV3 in SecurityScheme #Resolved

PerthCharern · 2017-11-15T03:13:36Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

@@ -73,7 +74,8 @@ public OpenApiDocument Read(Stream input, out OpenApiDiagnostic diagnostic)
                    return OpenApiV2Deserializer.LoadOpenApi(rootNode);

                default:
-                    context.SetReferenceService(new OpenApiV3ReferenceService(rootNode));
+                    var referenceService = new OpenApiV3ReferenceService(rootNode);
+                    context.SetReferenceService(referenceService);


will revert #Resolved

+1

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

PerthCharern · 2017-11-15T03:14:22Z

src/Microsoft.OpenApi.Readers/ParseNodes/JsonPointerExtensions.cs

+            }
+        }
+    }
+}


This is existing code. I took this out of RootNode to its own class. #Resolved

…Microsoft/OpenAPI.NET into PerthCharern/FixBugForReference

PerthCharern · 2017-11-15T03:17:17Z

src/Microsoft.OpenApi.Readers/V2/OpenAPiV2ReferenceService.cs

-}
-
-
-


This is moved to ConvertToOpenApiReferenceV2Tests #Resolved

PerthCharern · 2017-11-15T03:17:23Z

This is moved to ConvertToOpenApiReferenceV3Tests #Resolved

Refers to: src/Microsoft.OpenApi.Readers/V3/OpenApiV3ReferenceService.cs:170 in e342c66. [](commit_id = e342c66, deletion_comment = True)

xuzhg · 2017-11-15T18:18:43Z

        writer.WriteOptionalCollection(OpenApiConstants.Tags, Tags, (w, t) => t.WriteAsV3(w));

@darrelmiller
@PerthCharern

This line code has problem, right?

It should write as a list of string, but now it writes as a list of Tag object if it's not a reference. That's bug what i mentioned. #Resolved

Refers to: src/Microsoft.OpenApi/Models/OpenApiOperation.cs:134 in 7644970. [](commit_id = 7644970, deletion_comment = False)

xuzhg · 2017-11-15T18:19:50Z

    OpenApiReference Pointer { get; set; }

Can we rename the "Pointer" as "Reference"? #Resolved

Refers to: src/Microsoft.OpenApi/Interfaces/IOpenApiReference.cs:18 in 7644970. [](commit_id = 7644970, deletion_comment = False)

xuzhg · 2017-11-15T18:22:10Z

src/Microsoft.OpenApi/Models/OpenApiReference.cs

@@ -109,14 +61,22 @@ public string ReferenceV3
                    return GetExternalReference();
                }

-                if (ReferenceType == ReferenceType.Tag)
+                if (!Type.HasValue)


if (!Type.HasValue) [](start = 16, length = 19)

Need to test "Type != null" otherwise

Type.HasValue will throw "NullReference..." exception. #WontFix

Both are the same. The compiler translates null comparison for a nullable to a call to HasValue anyway.

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

xuzhg · 2017-11-15T18:23:23Z

src/Microsoft.OpenApi/Models/OpenApiReference.cs

+                if (Type == ReferenceType.SecurityScheme)
+                {
+                    return Id;
+                }


I don't know the "tag" & "SecurityScheme" reference.

@darrelmiller would you please help to verify?

Reviewing now....

xuzhg · 2017-11-15T18:23:34Z

src/Microsoft.OpenApi/Models/OpenApiReference.cs

-                return "#/" + GetReferenceTypeNameAsV2() + "/" + Name;
-            }
-        }
+                if (!Type.HasValue)


if (!Type.HasValue) [](start = 16, length = 19)

Same here #WontFix

Both != null and .HasValue are the same in the compiler's eyes.

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

PerthCharern · 2017-11-15T18:25:54Z

        writer.WriteOptionalCollection(OpenApiConstants.Tags, Tags, (w, t) => t.WriteAsV3(w));

This should work now. See OpenApiTag.WriteAsV3.

I am writing unit tests, so we will know for sure soon.

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

Refers to: src/Microsoft.OpenApi/Models/OpenApiOperation.cs:134 in 7644970. [](commit_id = 7644970, deletion_comment = False)

PerthCharern · 2017-11-15T18:51:28Z

        writer.WriteOptionalCollection(OpenApiConstants.Tags, Tags, (w, t) => t.WriteAsV3(w));

Actually I see what you meant now. I'll fix this.

In reply to: 344683545 [](ancestors = 344683545,344681626)

Refers to: src/Microsoft.OpenApi/Models/OpenApiOperation.cs:134 in 7644970. [](commit_id = 7644970, deletion_comment = False)

PerthCharern · 2017-11-15T19:02:30Z

    OpenApiReference Pointer { get; set; }

Will do. I prefer that as well.

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

Refers to: src/Microsoft.OpenApi/Interfaces/IOpenApiReference.cs:18 in 7644970. [](commit_id = 7644970, deletion_comment = False)

xuzhg · 2017-11-15T19:23:18Z

others look good to me

…rences, so we got the unit test working one way now. - Always write tag as strings regardless of whether Pointer exists in the Operation.

PerthCharern · 2017-11-15T19:47:28Z

Unit tests for Operation (OpenApiOperationTests) have been added. Given that Operation contains Tag and SecurityScheme, this verifies that writing references for those two special objects works.

I'm working on the Reader unit tests, and will push a new commit as soon as I can. #Resolved

darrelmiller · 2017-11-15T19:49:04Z

src/Microsoft.OpenApi.Readers/ReferenceServices/OpenApiReferenceServiceBase.cs

+    /// <summary>
+    /// The base class for Open API Reference service.
+    /// </summary>
+    internal abstract class OpenApiReferenceServiceBase : IOpenApiReferenceService


Do we really need a base class and an interface? There really isn't any behavior in this class. Would it not be simpler with just the interface? #Resolved

I agree. There used to be some logic in this class but I have since removed them. I'll remove this. #Resolved

Yes. Originally i removed the interface. Don't know why @PerthCharern re-added it. 😄

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

We need some kind of standard interface, it's the base class that is really redundant and potentially restrictive. In my experience interfaces should almost always be favoured over base classes. #Resolved

darrelmiller · 2017-11-15T19:51:53Z

src/Microsoft.OpenApi.Readers/ReferenceServices/OpenApiV2ReferenceService.cs

+                var tagListPointer = new JsonPointer("#/tags/");
+                var tagListNode = (ListNode)RootNode.Find(tagListPointer);
+
+                var tags = tagListNode.CreateList(OpenApiV2Deserializer.LoadTag);


Are we deserializing the entire list of tags every time we de-reference a single tag? That seems less than optimal #Resolved

You are right. Maybe we can cache this? #Resolved

This is fixed now.

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

- Add Unit Test for full document reading including schema/tag/securityScheme. One test is not working properly since we can't do dictionary comparison with securityScheme being the key right now, but I've verified that the output is what we expect.

…sure that each tag can be referenced without re-deserializing again and again.

PerthCharern · 2017-11-15T23:02:27Z

@darrelmiller @xuzhg

Apologies for the large PR :(

I have addressed all you guys' comments above and added two unit tests in FullDocumentReaderTests. The document files contain references using 1) $ref style (Schema), 2) Tag style, and 3) SecurityScheme style, so this should get us some confidence that deserialization with references is now working.

@andystumpp

Adding you to this PR per our discussion yesterday.

andystumpp · 2017-11-15T23:56:06Z

src/Microsoft.OpenApi.Readers/ParsingContext.cs

@@ -110,7 +101,7 @@ public IOpenApiReference GetReferencedObject(OpenApiDiagnostic diagnostic, OpenA
        /// <param name="referenceService"></param>
        public void SetReferenceService(IOpenApiReferenceService referenceService)
        {
-            this._referenceService = referenceService;
+            _referenceService = referenceService;
        }


Is there a reason why we don't use a property with a setter here? #Resolved

I don't think so. I'll fix this.

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

andystumpp · 2017-11-16T07:14:21Z

        var obj = new OpenApiSecurityRequirement();

Just seeing this, should we rename it to "securityRequirement"? #Resolved

Refers to: src/Microsoft.OpenApi.Readers/V3/OpenApiSecurityRequirementDeserializer.cs:22 in 72d612e. [](commit_id = 72d612e, deletion_comment = False)

andystumpp · 2017-11-16T07:24:31Z

src/Microsoft.OpenApi.Readers/ParsingContext.cs

        }

        /// <summary>
-        /// Get the referenced object.
+        /// Gets the referenced object


Nit: Period at the end #Resolved

andystumpp · 2017-11-16T07:28:34Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

@@ -69,13 +70,13 @@ public OpenApiDocument Read(Stream input, out OpenApiDiagnostic diagnostic)
            switch (inputVersion)
            {
                case "2.0":
-                    context.SetReferenceService(new OpenApiV2ReferenceService(rootNode));
+                    context.ReferenceService = new OpenApiV2ReferenceService(rootNode);


context.ReferenceService [](start = 20, length = 24)

The flow here is interesting:
In order to set the reference service in the context, you need the rootnode, for that though you need the context to have (partially?) initialized... Is there some potential circular reference? #Resolved

Right, this flow is strange. Fixing this won't be trivial, so I will open an issue. Given that this is specific to the reader (not the models), this should not block us from releasing the model as planned.

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

andystumpp · 2017-11-16T07:29:37Z

src/Microsoft.OpenApi/Models/OpenApiTag.cs

-            else
-            {
-                writer.WriteStartObject();
+            writer.WriteStartObject();


remove empty line #Resolved

PerthCharern · 2017-11-16T18:38:22Z

@darrelmiller @xuzhg

Friendly ping on this. Given the breadth of the code this PR covers, I would like to check this in as soon as possible to prevent merge conflicts with future changes.

PerthCharern added 5 commits November 14, 2017 11:15

Update name from IExtension and IReference to IExtensible and IRefere…

ed6a8b8

…nceable

- Update Reference implementation to correctly handle tag, security s…

79894df

…cheme, and other ref-style referenceable objects.

Add GetReferencedObject to all applicable deserializers

68fe07d

Small issue cleanup

d54e435

Move files for ReferenceService and remove duplicate implementation o…

e7a91d7

…f LoadTag

PerthCharern requested review from darrelmiller and xuzhg November 15, 2017 03:10

msftclas added the cla-not-required label Nov 15, 2017

Merge branch 'master' into PerthCharern/FixBugForReference

e342c66

PerthCharern commented Nov 15, 2017

View reviewed changes

PerthCharern added 2 commits November 14, 2017 19:16

Remove duplicate implementation of LoadTag

299b138

Merge branch 'PerthCharern/FixBugForReference' of https://github.com/…

7644970

…Microsoft/OpenAPI.NET into PerthCharern/FixBugForReference

PerthCharern commented Nov 15, 2017

View reviewed changes

xuzhg reviewed Nov 15, 2017

View reviewed changes

- Add OperationTests. This writes both Tag and SecurityScheme as refe…

f50b409

…rences, so we got the unit test working one way now. - Always write tag as strings regardless of whether Pointer exists in the Operation.

darrelmiller reviewed Nov 15, 2017

View reviewed changes

PerthCharern added 3 commits November 15, 2017 13:54

- Remove ReferenceService Base

3b472e9

- Add Unit Test for full document reading including schema/tag/securityScheme. One test is not working properly since we can't do dictionary comparison with securityScheme being the key right now, but I've verified that the output is what we expect.

Use JsonConvert as workaround for the equality issue.

6ffb16c

Precompute the tag arrays and store inside the ReferenceService to en…

4a28f05

…sure that each tag can be referenced without re-deserializing again and again.

PerthCharern requested a review from andystumpp November 15, 2017 22:56

andystumpp reviewed Nov 15, 2017

View reviewed changes

PerthCharern requested a review from scott-lin November 15, 2017 23:58

Minor cleanup for RootNode and ParsingContext

72d612e

andystumpp reviewed Nov 16, 2017

View reviewed changes

Some style changes based on andystumpp's comments

70b3503

xuzhg approved these changes Nov 16, 2017

View reviewed changes

PerthCharern merged commit 3e1077a into master Nov 16, 2017

PerthCharern mentioned this pull request Nov 16, 2017

Bugs in Tags and Schemes (IReference) #81

Closed

PerthCharern deleted the PerthCharern/FixBugForReference branch November 18, 2017 00:49

Update Reference implementation to handle all tag, security scheme, and $ref-style pointers correctly. #90

Update Reference implementation to handle all tag, security scheme, and $ref-style pointers correctly. #90

Uh oh!

Conversation

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

Uh oh!

Uh oh!

msftclas commented Nov 15, 2017

Uh oh!

PerthCharern Nov 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!

PerthCharern Nov 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!

PerthCharern Nov 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!

PerthCharern Nov 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

xuzhg Nov 15, 2017 • edited by PerthCharern 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!

xuzhg Nov 15, 2017 • edited by PerthCharern 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 commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern commented Nov 15, 2017

Uh oh!

PerthCharern commented Nov 15, 2017

Uh oh!

xuzhg commented Nov 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

darrelmiller Nov 15, 2017 • edited by PerthCharern 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 Nov 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 Nov 15, 2017 • edited by PerthCharern 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 Nov 15, 2017 • edited by PerthCharern Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PerthCharern commented Nov 15, 2017 •

edited

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

PerthCharern commented Nov 15, 2017 •

edited

Loading

xuzhg commented Nov 15, 2017 •

edited by PerthCharern

Loading

xuzhg commented Nov 15, 2017 •

edited by PerthCharern

Loading

xuzhg Nov 15, 2017 •

edited by PerthCharern

Loading

xuzhg Nov 15, 2017 •

edited by PerthCharern

Loading

PerthCharern commented Nov 15, 2017 •

edited

Loading

PerthCharern commented Nov 15, 2017 •

edited

Loading

darrelmiller Nov 15, 2017 •

edited by PerthCharern

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

darrelmiller Nov 15, 2017 •

edited by PerthCharern

Loading

darrelmiller Nov 15, 2017 •

edited by PerthCharern

Loading

PerthCharern Nov 15, 2017 •

edited

Loading

andystumpp Nov 15, 2017 •

edited by PerthCharern

Loading

andystumpp commented Nov 16, 2017 •

edited by PerthCharern

Loading

andystumpp Nov 16, 2017 •

edited by PerthCharern

Loading

andystumpp Nov 16, 2017 •

edited by PerthCharern

Loading

andystumpp Nov 16, 2017 •

edited by PerthCharern

Loading