Add more missed objects in core by xuzhg · Pull Request #28 · microsoft/OpenAPI.NET

xuzhg · 2017-10-23T20:01:49Z

add:

OpenApiOAuthFlow
OpenApiOAuthFlows

Modify: OpenApiSecurityScheme.

It seems the OpenApiSecurityScheme structure doesn't follow up the Spec. @darrelmiller

msftclas · 2017-10-23T20:01:53Z

@xuzhg,
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-10-23T20:09:38Z

src/Microsoft.OpenApi/Models/OpenApiSecurityScheme.cs

@@ -27,6 +27,14 @@ public class OpenApiSecurityScheme : IOpenApiReference, IOpenApiExtension
        public Uri TokenUrl { get; set; }
        public IDictionary<string,string> Scopes { get; set; }


These 4 should be removed since they belong in the OAuthFlow object, not here. #Closed

see the new commit

PerthCharern · 2017-10-23T20:14:15Z

The builders in https://github.com/Microsoft/OpenAPI.NET/blob/master/src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV3Builder.cs and https://github.com/Microsoft/OpenAPI.NET/blob/master/src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV2Builder.cs would need to write things for these new fields too. #Closed

PerthCharern · 2017-10-23T21:47:11Z

src/Microsoft.OpenApi/Models/OpenApiSecurityScheme.cs

    public class OpenApiSecurityScheme : IOpenApiReference, IOpenApiExtension
    {
-        public string Key { get; set; }
        public string Type { get; set; }


string [](start = 15, length = 6)

SecuritySchemeTypeKind ?

Yes. I didn't figure out how to use this enum type. But, I do think we should use the enum to limit the value.

You should make this SecuritySchemeTypeKind.

Then, in the Builder, you can use Enum.Parse. See how the "in" parameter is mapped in the ParameterObject (OpenApiBuilderV3.cs about line 337).

It would be nice to do something similar for the In property in OpenApiSecurityScheme too.

Maybe we should have moved the YAML/JSON readers out into a different project so you wouldn't have felt obliged to fix my unfinished code because it was in the core project ;-) You folks don't need the V2 reader, for your purposes, right?

Personally, it's fine. It's more like a team project at this point, and I already feel it's something integral to the core part we should get finished before releasing anyway.

I don't need V2, and I don't think Sam does as well. Fixing V2 builder and writer is a bit of a pain since our model is sorta closely based on V3, and it's not always easy to do the translation. Let's see how it goes. If fixing V2 wastes incrementally way more time than needed, maybe one person can tackle it at the end?

:-) Agreed. Using the V2 builder to map onto the V3 dom is a pain, especially if you are not familiar with the differences between V2 and V3. However, it is the pain we do here that will save many people who are trying to make the transition from V2 to V3.

xuzhg · 2017-10-23T21:49:35Z

@PerthCharern

I modify the builder, but for the V2 builder, I can not make sure the fixed and pattern fields are ok. We can modify it in the coming PR.

Besides, It looks I have to duplicate a lot of codes both in V2 and V3, It's better to refactor them. Let's also figure out later. #Closed

PerthCharern · 2017-10-23T21:50:11Z

src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV2Builder.cs

+
+        public static OpenApiOAuthFlows LoadOAuthFlows(ParseNode node)
+        {
+            var mapNode = node.CheckMapNode("OAuthFlows");


OAuthFlows [](start = 45, length = 10)

flows

"OAuthFlows" -> "OAuthflows" ?

Sorry. OAuthFlows is ok.

PerthCharern · 2017-10-23T21:50:43Z

src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV3Builder.cs


+        public static OpenApiOAuthFlows LoadOAuthFlows(ParseNode node)
+        {
+            var mapNode = node.CheckMapNode("OAuthFlows");


OAuthFlows [](start = 45, length = 10)

flows

Actually, ignore my comment. I think your existing code "OAuthFlows" is okay here.

PerthCharern · 2017-10-23T21:52:02Z

That's fine. I think a lot of things will have to change for the V2 builder. I don't think it's correct at the moment.

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

PerthCharern · 2017-10-23T21:52:49Z

src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV3Builder.cs

+        {
+            var mapNode = node.CheckMapNode("OAuthFlows");
+
+            var oauthFlows = new OpenApiOAuthFlows();


oauthFlows [](start = 16, length = 10)

Correct casing here should be "oAuthFlows"

xuzhg · 2017-10-23T21:55:45Z

You mean to change:
var mapNode = node.CheckMapNode("OAuthFlows");
to:

var mapNode = node.CheckMapNode("flows");

PerthCharern · 2017-10-23T21:57:05Z

Ignore that comment. I misunderstood how that string is used. OAuthFlows is ok.

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

xuzhg · 2017-10-23T22:01:13Z

For the security scheme type, the spec says:

type	string	Any	REQUIRED. The type of the security scheme. Valid values are "apiKey", "http", "oauth2", "openIdConnect".

So, I think enum type is necessary to narrow the accept value for the type. What do you think?
But, so far, I don't know how to use it in the Builder. so, leave it in code. Let me figure out it later.

PerthCharern · 2017-10-23T22:03:00Z

@xuzhg See the "in" parameter of ParameterObject in V3Builder on how to use Enum in the builder.

PerthCharern · 2017-10-23T22:07:21Z

@xuzhg I forgot to mention this early on. In addition to the builder, you should also fix the WriteSecurityScheme in OpenApiV3Writer (and OpenApiV2Writer if you can).

darrelmiller · 2017-10-23T22:08:06Z

src/Microsoft.OpenApi.Readers/YamlReaders/OpenApiV2Builder.cs

+            { "scheme", (o,n) => o.Scheme = n.GetScalarValue() },
+            { "bearerFormat", (o,n) => o.BearerFormat = n.GetScalarValue() },
+            { "openIdConnectUrl", (o,n) => o.OpenIdConnectUrl = new Uri(n.GetScalarValue(), UriKind.RelativeOrAbsolute) },
+            { "flows", (o,n) => o.Flows = LoadOAuthFlows(n) }


There is no such thing as a "flows" property in V2. There is a flow property as only a single flow can be described.

There is also no "openIdConnectUrl" or "bearerFormat". See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#securityDefinitionsObject

Right. Those should be removed.

It's a little complicated now that we try to deserialize V2 doc into our model (which is closely based on V3), but there's a nice way to do it for this I think.

Keep the model as what Sam has now (commit 2). The authorizationUrl, tokenUrl, and scopes read from the document should be populated in the OpenApiOAuthFlow object and assigned to the right field in OpenApiOAuthFlows object based on value in flow

xuzhg · 2017-10-23T22:36:23Z

Would you share your approve here? I will continue on

PerthCharern · 2017-10-23T22:38:52Z

@xuzhg I would say you need to fix WriteSecurityScheme in OpenApiV3Writer and OpenApiV2Writer first. Otherwise, the change doesn't seem complete to me, and the state of the writer/model will be unsynced.

xuzhg · 2017-10-23T23:04:33Z

Oh, yes. I am working on it.

xuzhg · 2017-10-23T23:34:56Z

@PerthCharern see the latest commit.

@darrelmiller are you working on the writer stuff? It seems that I have to duplicate a lot of codes.

PerthCharern · 2017-10-23T23:44:31Z

src/Microsoft.OpenApi/Writers/OpenApiV2Writer.cs

+        private static void WriteValue(IOpenApiWriter writer, string value)
+        {
+            writer.WriteValue(value);
        }


V2 doesn't have this structure.

I suggest this: Read the object in Flows (if more than one object exist, arbitrarily choose one), and then populate the SecurityScheme object with the info from there.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#securitySchemeObject

see my next commit.

PerthCharern · 2017-10-23T23:46:01Z

src/Microsoft.OpenApi/Models/OpenApiSecurityScheme.cs

+
+        /// <summary>
+        /// REQUIRED. The location of the API key
+        /// </summary>


InEnum [](start = 15, length = 6)

The InEnum is used in multiple classes now. I'd take out its definition from Parameter object and put in its own file.

I think it's ok to be in the OpenApiParameter.cs. If you do think it should be in a separate file, We can do it later.

Why should it be in OpenApiParameter.cs when it's used in multiple places? #Closed

PerthCharern · 2017-10-23T23:58:07Z

src/Microsoft.OpenApi/Writers/OpenApiV3Writer.cs

+        private static void WriteValue(IOpenApiWriter writer, string value)
+        {
+            writer.WriteValue(value);
+        }


Can't we use writer.WriteValue above directly?

no, the signature doesn't match.

Use this:

(w, s) => w.WriteValue(s)

yes. thanks.

PerthCharern · 2017-10-24T00:09:12Z

src/Microsoft.OpenApi/Writers/OpenApiV2Writer.cs

            writer.WriteEndObject();
+        }

+        private static bool WriteOAuthFlow(IOpenApiWriter writer, OpenApiOAuthFlow oAuthFlow, string flow)


bool [](start = 23, length = 4)

This return type is not consistent with other WriteXXX method. I strongly discourage this to avoid confusion. #Closed

When this is fixed, I'll approve. Please note that the V2 reader is still not correct, but that's an isolated issue that can be fixed later. #Closed

@PerthCharern Ok. Got it. Please see the latest commits. #Closed

xuzhg · 2017-10-24T00:18:20Z

src/Microsoft.OpenApi/Writers/OpenApiV2Writer.cs

-                            WriteOAuthFlow(writer, securityScheme.Flows.AuthorizationCode, "accessCode");
-                        }
-                    }
+                    writer.WriteStringProperty("flow", "implicit");


I don't like the codes here. :)

xuzhg · 2017-10-24T16:57:06Z

@PerthCharern

PerthCharern · 2017-10-24T17:12:26Z

src/Microsoft.OpenApi/OpenApiException.cs

+    /// <summary>
+    /// Exception type representing exceptions in the Open API library.
+    /// </summary>
+    public class OpenApiException : InvalidOperationException


InvalidOperationException [](start = 36, length = 25)

I think it's better that this inherits just Exception. OpenApiException is not really always about Operation, and it is a little misleading this way.

Ok. I changed to "Exception"

PerthCharern

…um to individual file

…8d61-10c9af657b14 Add support for document $self property (OAI 3.2.0)

msftclas added the cla-not-required label Oct 23, 2017

PerthCharern reviewed Oct 23, 2017

View reviewed changes

darrelmiller reviewed Oct 23, 2017

View reviewed changes

PerthCharern reviewed Oct 23, 2017

View reviewed changes

PerthCharern reviewed Oct 24, 2017

View reviewed changes

xuzhg commented Oct 24, 2017

View reviewed changes

PerthCharern reviewed Oct 24, 2017

View reviewed changes

PerthCharern approved these changes Oct 24, 2017

View reviewed changes

xuzhg added 2 commits October 24, 2017 10:45

Add more missed objects in core

e933c84

remove unused properties and add logic in the builder

1c69ab5

xuzhg added 6 commits October 24, 2017 10:45

1. change to enum, 2 modify the v2/v3 writer

15e894e

change to meet the v2 spec for the security schema

70727da

use the lambda express

7b14503

Modify the code for v2 writer for security schema and seperate the en…

cb74c53

…um to individual file

Modify the OpenApiException

22b6039

Change OpenApiException base to Exception

867b2a6

xuzhg merged commit 256d86f into microsoft:master Oct 24, 2017

baywet added a commit that referenced this pull request Nov 11, 2025

Merge pull request #28 from BinkyLabs/copilot/fix-441fec3f-fb43-40b7-…

292a28b

…8d61-10c9af657b14 Add support for document $self property (OAI 3.2.0)

		@@ -27,6 +27,14 @@ public class OpenApiSecurityScheme : IOpenApiReference, IOpenApiExtension
		public Uri TokenUrl { get; set; }
		public IDictionary<string,string> Scopes { get; set; }

Comments

Conversation

xuzhg commented Oct 23, 2017

Uh oh!

msftclas commented Oct 23, 2017

Uh oh!

PerthCharern Oct 23, 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 commented Oct 23, 2017 • edited 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!

xuzhg Oct 23, 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!

xuzhg commented Oct 23, 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!

PerthCharern commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Oct 23, 2017

Uh oh!

PerthCharern commented Oct 23, 2017

Uh oh!

xuzhg commented Oct 23, 2017

Uh oh!

PerthCharern commented Oct 23, 2017

Uh oh!

PerthCharern commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Oct 23, 2017

Uh oh!

PerthCharern commented Oct 23, 2017

Uh oh!

xuzhg commented Oct 23, 2017

Uh oh!

xuzhg commented Oct 23, 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!

PerthCharern Oct 23, 2017 •

edited

Loading

PerthCharern commented Oct 23, 2017 •

edited

Loading

xuzhg Oct 23, 2017 •

edited

Loading

xuzhg commented Oct 23, 2017 •

edited by PerthCharern

Loading

PerthCharern Oct 24, 2017 •

edited

Loading

PerthCharern Oct 24, 2017 •

edited

Loading

PerthCharern Oct 24, 2017 •

edited

Loading

xuzhg Oct 24, 2017 •

edited by PerthCharern

Loading