A variety of bug fixes for 1.1 preview #267

darrelmiller · 2018-06-05T20:22:02Z

Merged fixes from 1.0 release into 1.1
Added validation to
Made Paths null by default to fix error reporting
Do not include default basePath in output.
Handle V2 import when produces comes after responses
Fix MakeServers for V2
Add settings parameter for BaseUrl
Add support for null to GetJsonCompatibleString
Output multiple examples in V2
Exposed Settings in IOpenApiWriter interface to allow access to selected version when writing extensions. (Potentially a breaking change?)

Added parsing externalValue to Examples Object

Update to v1.0.1

PerthCharern · 2018-06-05T20:30:53Z

src/Microsoft.OpenApi/Models/OpenApiDocument.cs

        /// <summary>
        /// REQUIRED. The available paths and operations for the API.
        /// </summary>
-        public OpenApiPaths Paths { get; set; } = new OpenApiPaths();


= new OpenApiPaths(); [](start = 47, length = 22)

The reason we had this was because OpenApiPaths is a collection (a dictionary). Is it causing issue? #Resolved

Yes. We don't raise provide a validation error when a user provides a document that doesn't have a paths property. We used to validate this in the reader, but we since moved it into our Validator. #Closed

PerthCharern · 2018-06-05T20:40:57Z

src/Microsoft.OpenApi.Readers/OpenApiReaderSettings.cs

+        /// <summary>
+        /// URL where relative references should be resolved from if the description does not contain Server definitions
+        /// </summary>
+        public Uri BaseUrl { get; set; } = new Uri("https://example.org/");


Uri("https://example.org/"); [](start = 46, length = 29)

The spec says this:

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#fixed-fields

An array of Server Objects, which provide connectivity information to a target server. If the servers property is not provided, or is an empty array, the default value would be a Server Object with a url value of /. #Closed

Actually, that might be unrelated. Let me read through more, and I'll readdress this.

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

OK. I finished looking through the whole PR. I don't think we should use an arbitrary URL here.

I'd handle this inside the deserializer itself (see my other comment there about special cases). It seems a bit confusing to have a baseUrl in the reader/parsingContext, especially the one that's not given by the caller.

In reply to: 193215807 [](ancestors = 193215807,193214821)

I can leave this null by default and fix the MakeServer stuff, but we still need a way for the user to specify the URL where the doc came from. #Closed

Sounds good.

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

PerthCharern · 2018-06-05T20:47:07Z

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

                {
-                    var server = new OpenApiServer();
-                    server.Url = scheme + "://" + (host ?? "example.org/") + (basePath ?? "/");
+                    var ub = new UriBuilder(scheme, host)


ub [](start = 24, length = 2)

nit, uriBuilder #Closed

PerthCharern · 2018-06-05T21:02:22Z

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

+            }
+
+            // Fill in missing information based on the defaultUrl
+            host = host ?? defaultUrl.GetComponents(UriComponents.NormalizedHost, UriFormat.SafeUnescaped);


defaultUrl.GetComponents(UriComponents.NormalizedHost, UriFormat.SafeUnescaped); [](start = 27, length = 80)

V2 spec:

If the host is not included, the host serving the documentation is to be used (including the port).

This is in line with V3 spec:

If the servers property is not provided, or is an empty array, the default value would be a Server Object with a url value of /.

Server.url "supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the OpenAPI document is being served."

There are 8 cases in total:

=> Case 1 & 2: Host empty + schemes not empty (regardless of basePath) => This is weird. I don't know how to represent this in V3. Maybe we can simply ignore the schemes since we don't really have a way to handle it.

=> Case 3: Host empty + schemes empty + basePath not empty => Essentially, what this means is we use the Host serving the doc but with appended path. To represent this in V3, we simply need the basePath in Server.url

=> Case 4: Host empty + schemes empty + basePath empty => You handled this. This means no need for Server object in V3.

=> Case 5 & 6: Host not empty + schemes empty (regardless of basePath) => We can just use the host and basePath without schemes in Server.url in V3? That still seems like a valid URL to me.

=> Case 7 & 8: Host not empty + schemes not empty (regardless of basePath) => This is the normal case. Server.url will simply be schemes + host + basePath. #Resolved

Just so we are the same page, the BaseUrl in the Settings is where you put "the Host serving the doc". I think what you are saying, is you don't want an arbitrary default for "host serving the doc" when one isn't provided. Is that correct?
#Closed

ah okay. Correct - it's a little odd to default to an arbitrary example.org. Also, the conversion is possible (except case 1&2) above even without the info on "host serving the doc", so I'm not sure if we really need it.

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

Ah. I see that the baseUrl has to be there for reference resolving. In that case, I think we should leave it blank like you suggested, handle the MakeServers here, and use the property for Reference Resolving as planned.

In reply to: 193231179 [](ancestors = 193231179,193228718)

PerthCharern · 2018-06-05T21:05:45Z

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

+
+            foreach (var response in operation.Responses.Values)
+            {
+                ProcessProduces(response, node.Context);


ProcessProduces(response, node.Context); [](start = 12, length = 44)

Isn't this handled in OpenApiResponseDeserializer.cs line 123? #Closed

I had to put it in operation for the case when produces came after responses. But I needed it also in responses for when responses are defined at the root level as a reusable thing. So I made it work when you call it in both places. #Closed

Tricky! Sounds good to me.

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

PerthCharern

🕐

PerthCharern · 2018-06-06T23:17:29Z

src/Microsoft.OpenApi/Extensions/OpenApiSerializableExtensions.cs

+                    throw new OpenApiException(string.Format(SRResource.OpenApiSpecVersionNotSupported, writer.Settings.SpecVersion));
+            }
+
+            writer.Flush();


Can we leverage the method above?

return element.Serialize(writer, writer.Settings.SpecVersion); #Resolved

Fixed. Thanks!

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

PerthCharern · 2018-06-06T23:22:43Z

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

                {
-                    var server = new OpenApiServer();
-                    server.Url = scheme + "://" + (host ?? "example.org/") + (basePath ?? "/");
+                    var uriBuilder = new UriBuilder(scheme, host)


Check that scheme is not null or empty #Resolved

PerthCharern · 2018-06-06T23:59:52Z

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

+            {
+                if (!String.IsNullOrEmpty(host))
+                {
+                    host = "//" + host;


= "//" + host; [](start = 25, length = 14)

nit, it wasn't obvious to me in the beginning that we need this "//" to denote "the current protocol".

Could you add some comments to explain this? #Resolved

PerthCharern · 2018-06-07T00:00:44Z

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

+                var uriBuilder = new UriBuilder()
+                {
+                    Scheme = null,
+                    Host = host,


The way this works is kinda strange - I'd hope the UriBuilder can add that "//" for us when the Scheme is null. However, it seems like the default behavior is that it would add http instead - which is not what we want. #Resolved

Yeah, I was hoping UriBuilder would add the // but sadly it doesn't. Maybe we could consider building our own UriBuilder to clean this up. But Uris are tricky beasts, so I didn't want to open that can of worms just now.

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

PerthCharern

darrelmiller added 13 commits May 24, 2018 11:00

Added externalValue

7218f32

Merge pull request #264 from Microsoft/dm/externalValue

f9997e0

Added parsing externalValue to Examples Object

Update to v1.0.1

cb0b68c

Merge pull request #265 from Microsoft/dm/release-v1.0.1

d793955

Update to v1.0.1

Resolved merge conflicts.

40c224b

Add null to GetJsonCompatibleString

e058e01

Changed OpenApiDocument.Paths to be null by default

469a55a

Fix MakeServers for V2

163e655

Add tests for MakeServers

ecc9ea0

Do not include default basePath in output.

0d074d0

Output multiple examples in V2

c73cf9b

Handle V2 import when produces comes after responses

fb89e93

Reset version number

5f5a310

darrelmiller changed the base branch from master to vnext June 5, 2018 20:24

darrelmiller changed the title ~~A variety of bug fixes~~ A variety of bug fixes for 1.1 preview Jun 5, 2018

darrelmiller requested review from PerthCharern and xuzhg June 5, 2018 20:28

PerthCharern reviewed Jun 5, 2018

View reviewed changes

PerthCharern suggested changes Jun 5, 2018

View reviewed changes

Fixes based on PR review

58c4c36

PerthCharern reviewed Jun 6, 2018

View reviewed changes

PerthCharern reviewed Jun 7, 2018

View reviewed changes

PerthCharern approved these changes Jun 7, 2018

View reviewed changes

PR review fixes

8a76184

darrelmiller merged commit 3277e46 into vnext Jun 7, 2018

darrelmiller deleted the dm/1.1/fixes branch June 7, 2018 14:53

darrelmiller mentioned this pull request Jun 9, 2018

Missing paths property is not reported as an error in v2 and v3 docs #263

Closed

PerthCharern mentioned this pull request Dec 13, 2018

Duplicate response processing causes schema corruption #368

Merged

PerthCharern mentioned this pull request Apr 17, 2019

Consumes in global context is not propagated to the request body #400

Closed

A variety of bug fixes for 1.1 preview #267

A variety of bug fixes for 1.1 preview #267

Uh oh!

Conversation

darrelmiller commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern Jun 5, 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!

darrelmiller Jun 5, 2018 • 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 Jun 5, 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 Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

PerthCharern Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

darrelmiller Jun 5, 2018 • 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 Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

PerthCharern Jun 5, 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 Jun 5, 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!

darrelmiller Jun 5, 2018 • 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 Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

PerthCharern Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

PerthCharern Jun 5, 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!

darrelmiller Jun 5, 2018 • 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 Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

PerthCharern left a comment

Choose a reason for hiding this comment

Uh oh!

PerthCharern Jun 6, 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!

darrelmiller Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

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

darrelmiller commented Jun 5, 2018 •

edited

Loading

PerthCharern Jun 5, 2018 •

edited by darrelmiller

Loading

darrelmiller Jun 5, 2018 •

edited by PerthCharern

Loading

PerthCharern Jun 5, 2018 •

edited

Loading

darrelmiller Jun 5, 2018 •

edited by PerthCharern

Loading

PerthCharern Jun 5, 2018 •

edited

Loading

PerthCharern Jun 5, 2018 •

edited by darrelmiller

Loading

darrelmiller Jun 5, 2018 •

edited by PerthCharern

Loading

PerthCharern Jun 5, 2018 •

edited

Loading

darrelmiller Jun 5, 2018 •

edited by PerthCharern

Loading

PerthCharern Jun 6, 2018 •

edited by darrelmiller

Loading

PerthCharern Jun 6, 2018 •

edited by darrelmiller

Loading

PerthCharern Jun 6, 2018 •

edited by darrelmiller

Loading

PerthCharern Jun 7, 2018 •

edited by darrelmiller

Loading

darrelmiller Jun 7, 2018 •

edited

Loading