Fix temp parameters duplicate issue + Allow relative URIs where appropriate by PerthCharern · Pull Request #197 · microsoft/OpenAPI.NET

PerthCharern · 2018-01-30T19:58:06Z

Handle URI parsing. Allows relative URI where allowed by the spec. Partially addressed Handle empty URI when reading #192.
Temp variables in Operation deserializer (formParameters, bodyParameter, operationConsumes, operationProduces) are set to null at the beginning of each operation processing. Fix "formparameters" temp variable must be cleared after each operation is processed #191

…Ensure that the reader doesn't error out when parsing absolute URI fails. - Validator should take care of validating whether the URI conforms to the spec. XML.Namespace is the only place the spec indicates the the URI must be absolute. This will be a handled in a separate PR. - formParameters temp variable is set to empty at the beginning of each operation processing.

PerthCharern · 2018-01-30T19:58:42Z

cc @sergey-tihon #Resolved

sergey-tihon · 2018-01-30T21:18:57Z

Just a question, should this PR also consider URLs in $refs?

Sample

"enum": {
  "type": "array",
  "items": {
    "$ref": "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#/definitions/apiVersion"
  },
  "minItems": 1,
  "uniqueItems": true
}

from https://github.com/Azure/azure-resource-manager-schemas/blob/master/tools/ResourceMetaSchema.json#L160 #Resolved

PerthCharern · 2018-01-30T21:42:50Z

@darrelmiller is working on the support for external references. That feature will likely come as a separate PR. #Resolved

sergey-tihon · 2018-01-31T08:53:53Z

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

                {
-                    node.Diagnostic.Errors.Add(
-                        new OpenApiError(node.Context.GetLocation(), $"Scheme {property.Name} is not found"));
+                    //node.Diagnostic.Errors.Add(


What should happen if scheme is null? #Resolved

The spec says

the name used for each property MUST correspond to a security scheme declared in the Security Schemes under the Components Object.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#security-requirement-object

So I believe we should mark this as an error. Not sure why this has been commented out /cc @PerthCharern #Resolved

I commented this out so that I could test without running into NRE and forgot to revert back.

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

sergey-tihon · 2018-01-31T08:54:44Z

test/Microsoft.OpenApi.Readers.Tests/V3Tests/OpenApiXmlTests.cs

+                    new OpenApiXml
+                    {
+                        Name = "name1",
+                        // TODO: Verify that the URL caused a parsing error.


Looks like a candidate for one more assertion for diagnostic . #Resolved

I changed this to a success case since that's what the spec allows (relative url should also work).

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

I created a new test case with empty URL. The diagnostic should contain an error for empty URL.

In reply to: 168329298 [](ancestors = 168329298,164986283)

sergey-tihon · 2018-01-31T08:56:24Z

I do not familiar with codebase yet, but there are 2 comments from me on the diff 😊 #Resolved

darrelmiller

We should have a conversation about the use of Uri.TryCreate. I'm not sure it is a good idea. And I don't think we can use the static _responseSchema

darrelmiller · 2018-01-31T15:42:24Z

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

+                        if (Uri.TryCreate(n.GetScalarValue(), UriKind.RelativeOrAbsolute, out uri))
+                        {
+                            o.Url = uri;
+                        }


I'm not sure I'm a fan of this approach. We are going to swallow the error and when reporting the problem all we can say is "you are missing a URL". This is going to be confusing to the person who believes they put a URL and it is invalid. I think it is valid to have two distinct sources of errors. One from parsing content, and one from validating the DOM. We can't avoid it anyway because sometimes the YAML/JSON is going to be badly formatted. #Resolved

We should still produce the document though. If I throw an error here, everything breaks.

We need to have a way to still allow reading to go through and add the errors along the way.

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

Without that mechanism, what we can do is making the reporting message a bit more generic:

"The string passed in cannot be parsed into a URL."

This should be a very rare case. Almost any string can be a url. Even "abcdef" is allowed to be parsed in as a URL. Cases I suspect will be caught would be something like an empty string or a white space string.

In reply to: 167382549 [](ancestors = 167382549,165092404)

darrelmiller · 2018-01-31T15:44:00Z

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

+            node.Context.SetTempStorage("bodyParameter", null);
+            node.Context.SetTempStorage("formParameters", null);
+            node.Context.SetTempStorage("operationProduces", null);
+            node.Context.SetTempStorage("operationConsumes", null);


I guess we should probably create constants for these. #Resolved

darrelmiller · 2018-01-31T15:46:15Z

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

                "schema", (o, n) =>
                {
-                    n.Context.SetTempStorage("operationschema", LoadSchema(n));
+                    _responseSchema = LoadSchema(n);


Why this change to a static? This is going to make the deserializer not thread safe. That is most likely going to be a problem. #Resolved

You are right. I wasn't thinking about that issue. I'll revert this back. There are a few instances like this in the code, so I'll fix those separately.

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

darrelmiller · 2018-01-31T15:48:44Z

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

+                        SRResource.JsonPointerCannotBeResolved,
+                        jsonPointer));
+            }
+


Maybe once we consolidate the error classes we can just throw this exception from inside the Find. Is there somewhere where we need the response to be null on fail? #Resolved

Throwing the error here allows us to control what to do with json pointers that we cannot resolve. At least, we can leave them as a reference object (and reporting it) rather than always throwing an error and stopping the entire parsing process.

This probably no longer applies with the new reference resolving method.

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

PerthCharern · 2018-02-07T04:23:43Z

Apologies for the delay on this, and thanks for all your comments (@sergey-tihon, @darrelmiller)

I'll try to address all the comments by end of this week. #Resolved

PerthCharern · 2018-02-10T02:08:43Z

Update: I got booked up with other commitments this week, but I'm almost done. I'll update the PR next week. Sorry again for the delay. #Resolved

PerthCharern · 2018-02-14T22:58:22Z

I've been thinking a bit more about this. Here's what I'll do:

I'll revert URL-related changes from this PR. This is a bigger problem than I thought, so I'll tackle it separately. Here's what I propose: We change the models to use "string" instead of "Uri" type. This way, we can handle all the format validation in the validator without failing the parsing. This falls in line with our current paradigm (reader just reads, validator verifies the format).
I'll address all other comments and send out a new commit for this PR.

…y given that we might want to change the properties to strings. - Allow relative URL where allowed by the spec. - Use constants for temp storage variable name.

PerthCharern · 2018-02-15T00:16:02Z

@darrelmiller This PR has become much smaller and is ready now. I will handle #192 in a separate PR since it turned out to be a bit more involved than I expected. What do you think about my proposal in 1. below?

darrelmiller · 2018-02-15T04:06:51Z

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

+            catch (Exception)
            {
-                throw new ArgumentException("Failed to dereference pointer", ex);
+                return null;


I'm still a little nervous about this change, but let's try it and see if it causes any issues.

darrelmiller · 2018-02-15T04:08:51Z

PerthCharern requested review from darrelmiller and xuzhg January 30, 2018 19:58

Merge branch 'master' into PerthCharern/ReaderFixApiGUru

f98a5de

sergey-tihon reviewed Jan 31, 2018

View reviewed changes

darrelmiller requested changes Jan 31, 2018

View reviewed changes

PerthCharern added 3 commits February 14, 2018 15:29

- Remove Url try-parse changes. URL changes will be handled separatel…

55778da

…y given that we might want to change the properties to strings. - Allow relative URL where allowed by the spec. - Use constants for temp storage variable name.

Merge with master

13bc879

Remove outdated TODO comments

ca8af72

PerthCharern changed the title ~~Handle URI parsing + Fix form parameters duplicate issue~~ Fix temp parameters duplicate issue + Allow relative URIs where appropriate Feb 15, 2018

missed constant assignment

58c2f84

PerthCharern mentioned this pull request Feb 15, 2018

RequestBody.Content is not updated properly #212

Closed

darrelmiller reviewed Feb 15, 2018

View reviewed changes

darrelmiller approved these changes Feb 15, 2018

View reviewed changes

PerthCharern merged commit 361f703 into master Feb 15, 2018

PerthCharern deleted the PerthCharern/ReaderFixApiGUru branch September 26, 2018 22:40

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

sergey-tihon commented Jan 30, 2018 • edited by PerthCharern Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

sergey-tihon Jan 31, 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!

darrelmiller Jan 31, 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!

Choose a reason for hiding this comment

Uh oh!

sergey-tihon Jan 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-tihon commented Jan 31, 2018 • edited by PerthCharern Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darrelmiller left a comment

Choose a reason for hiding this comment

Uh oh!

darrelmiller Jan 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Jan 31, 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!

darrelmiller Jan 31, 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!

Choose a reason for hiding this comment

Uh oh!

darrelmiller Jan 31, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

PerthCharern commented Feb 14, 2018

Uh oh!

PerthCharern commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darrelmiller commented Feb 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

PerthCharern commented Jan 30, 2018 •

edited

Loading

PerthCharern commented Jan 30, 2018 •

edited

Loading

sergey-tihon commented Jan 30, 2018 •

edited by PerthCharern

Loading

PerthCharern commented Jan 30, 2018 •

edited

Loading

sergey-tihon Jan 31, 2018 •

edited by PerthCharern

Loading

darrelmiller Jan 31, 2018 •

edited by PerthCharern

Loading

sergey-tihon Jan 31, 2018 •

edited by PerthCharern

Loading

sergey-tihon commented Jan 31, 2018 •

edited by PerthCharern

Loading

darrelmiller Jan 31, 2018 •

edited by PerthCharern

Loading

darrelmiller Jan 31, 2018 •

edited by PerthCharern

Loading

darrelmiller Jan 31, 2018 •

edited by PerthCharern

Loading

darrelmiller Jan 31, 2018 •

edited by PerthCharern

Loading

PerthCharern commented Feb 7, 2018 •

edited

Loading

PerthCharern commented Feb 10, 2018 •

edited

Loading