Prevent overflow for self-referencing schemas #185

darrelmiller · 2018-01-19T19:48:36Z

Fix for #182
UPDATE Major update to this PR. See below

This fix does address the problem in the parser. However, it produces a bit of an unusual DOM and I'm not sure this is what we want.
We detect the cycle whilst constructing the dom object, so when we discover there is a cycle we don't have direct access to the original object we are creating. What this currently does is create a second instance of the self-referenced object and the second instance contains just an OpenApiReference back to the original object. This means the object graph itself doesn't have a cycle in it. This is handy so that the Walker doesn't get stuck in loops. The reader doesn't have a problem because it only fully explodes objects that live under $/components/schemas so it doesn't even try to render the second instance of the object.

It does mean that someone could construct a graph manually with a cycle in it, that would break the walker. Again I think the reader would survive.

I will open an issue regarding the walker failing for graphs with cycles and we should consider if the reader should actually try and create a cycle in the DOM.

PerthCharern · 2018-01-19T21:17:57Z

I'll take a look at the code. Here's my comment based on the PR description:

Would it be better if we handle this with the reader/walker/etc. instead of at the DOM level?

The DOM having cycles should remain valid -- an object should be able to refer to itself, just like normal objects in C#. Whoever is operating on the DOM (reader/walker/etc.) should be able to stop the infinite loop once it is detected.

In this case, the walker can memorize the node it has visited and make sure it does not visit the duplicate node. The reader can do something similar.

I might be missing nuances here, so I'll take a look at the code and I might see the limitations with the method above. #Closed

PerthCharern · 2018-01-19T22:24:52Z

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

+                var openApiDoc = new OpenApiStreamReader().Read(stream, out var diagnostic);
+
+                // Assert
+                var components = openApiDoc.Components;


var components = openApiDoc.Components; [](start = 16, length = 39)

Let's also check that components is parsed correctly. #Closed

PerthCharern · 2018-01-19T22:27:47Z

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

+
+
+            }
+        }


Location-wise I think this should go into OpenApiComponentTests (which doesn't exist yet) - also with the yaml not containing sections other than components. #Closed

PerthCharern · 2018-01-19T23:19:42Z

Ah. My mistake. I read your code and understand now what you are trying to do. I still think we should allow the graph to have a cycle, so let me think about how to do that for a bit.

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

PerthCharern · 2018-01-22T06:56:31Z

One way I can think of:

Post-process the schema section in the deserialization of Components. After all the schemas are loaded, recursively delve into the schemas (into the fields in Schema object that are also a Schema object) and ensure that they reference the schemas in the Components.Schemas map.

Essentially, we change a schema we see (possible just a dummy schema with $ref you created) to the real schema. This is possible since all schemas have already been loaded at this point. #Closed

darrelmiller · 2018-01-22T22:05:17Z

@PerthCharern Yeah, doing a post process to hook up the cycles will be my fallback approach. I'll see if I can find a cleaner way first. #Closed

darrelmiller · 2018-01-23T21:55:28Z

@PerthCharern After failing to find a solution to resolve these $ref on first pass, I'm thinking that using a second pass to resolve the $refs might actually address another concern. In another branch I have been working on resolving $refs to external documents and have the problem that external $refs need to be loaded asynchronously.

Perhaps reading can be a three step process:

Read graph and don't resolve $refs.
Iterate over $refs looking for external $refs and load the documents
Walk the graph and convert $refs into resolved objects.

I'm not sure how to expose the API for this, but at least knowing that it could help with resolving external references gives me more confidence in breaking this down into multiple steps

PerthCharern · 2018-01-24T21:25:36Z

@darrelmiller That sounds good to me. I don't have any concerns with doing multiple passes to complete the reading process.

Regarding exposing these as APIs, I am afraid I don't have much input for you now. One thing I can think of is we should expose the API to resolve external references independently since that sounds pretty useful as a standalone feature.

Given that the logic would be similar regardless of what we expose, I think we can also discuss once you send out a PR. #Closed

darrelmiller · 2018-02-08T01:46:02Z

** NEW BEHAVIOUR **

Ok, this PR is finally ready for review.

Component objects are new loaded with Reference object populated
Initial parsing phase does not resolve references. All references are marked as "unresolved".
After parsing, the DOM is traversed and references are resolved and replaced with complete objects
ReaderSettings now have a property to control the reference resolution. If desired, resolution process can be skipped.
Cycles due to self $refs are now supported
The "referenceStore" has been removed from ParsingContext as it is no longer needed

Remote references are not yet supported. A new branch has been started to support that. There will need to be a new ReadAsync method to support resolving remote references.

/cc @PerthCharern @xuzhg

PerthCharern · 2018-02-12T22:32:40Z

src/Microsoft.OpenApi/Models/OpenApiSchema.cs

+        /// <summary>
+        /// Indicates object is a placeholder reference to an actual object and does not contain valid data.
+        /// </summary>
+        public bool UnresolvedReference { get; set; } = false;


= false; [](start = 54, length = 8)

nit, remove #Closed

PerthCharern · 2018-02-12T22:33:36Z

src/Microsoft.OpenApi/Models/OpenApiDocument.cs

+
+            if (!reference.Type.HasValue)
+            {
+                throw new ArgumentException("Local reference must have type specified.");


"Local reference must have type specified." [](start = 44, length = 43)

nit, This should be a resource for consistency #Closed

PerthCharern · 2018-02-12T22:34:52Z

src/Microsoft.OpenApi/Services/OpenApiVisitorBase.cs

+
        /// <summary>
-        /// Visits responses.
+        /// Visits headers.


headers [](start = 19, length = 7)

callbacks

PerthCharern · 2018-02-12T22:39:28Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

+            {
+                foreach (var header in headers)
+                {
+                    Walk(header.Key.Replace("/", "~1"), () => Walk(header.Value));


.Replace("/", "~1") [](start = 35, length = 19)

nit, I thought we already handled this somewhere else. If not, can we do this in the VisitorBase.Enter, given that this is a JSON pointer requirement.

PerthCharern · 2018-02-12T22:40:15Z

src/Microsoft.OpenApi.Readers/OpenApiReaderSettings.cs

+        /// <summary>
+        /// Convert all references to references of valid domain objects.
+        /// </summary>
+        ResolveRemoteReferences


ResolveRemoteReferences [](start = 8, length = 23)

either ResolveAllReferences or ResolveLocalAndRemoteReferences #Closed

PerthCharern · 2018-02-12T22:42:03Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

+            switch (_settings.ReferenceResolution) 
+            {
+                case ReferenceResolutionSetting.ResolveRemoteReferences:
+                    throw new ArgumentException(Properties.SRResource.CannotResolveRemoteReferencesSynchronously);


throw new ArgumentException(Properties.SRResource.CannotResolveRemoteReferencesSynchronously); [](start = 20, length = 94)

This will be supported, right? Can we just go with NotImplementedException()?

I don't think this will be handled. If you want to resolve remote references then you should call the ReadAsync method instead. The ReadAsync will also load related documents into a workspace. #Closed

Makes sense.

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

PerthCharern · 2018-02-12T22:42:53Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

+                    walker.Walk(document);
+                    break;
+                case
+                    ReferenceResolutionSetting.DoNotResolveReferences:


nit, remove this awkward linebreak :)

PerthCharern · 2018-02-12T22:44:17Z

src/Microsoft.OpenApi.Readers/ParsingContext.cs

+        /// Reset loop tracking stack
+        /// </summary>
+        /// <param name="loopid"></param>
+        internal void ClearLoop(string loopid)


loopid [](start = 39, length = 6)

nit, loopId #Closed

PerthCharern · 2018-02-12T22:44:25Z

src/Microsoft.OpenApi.Readers/ParsingContext.cs

+        /// Exit from the context in cycle detection
+        /// </summary>
+        /// <param name="loopid"></param>
+        public void PopLoop(string loopid)


loopid [](start = 35, length = 6)

nit, loopId

PerthCharern · 2018-02-12T22:44:55Z

src/Microsoft.OpenApi.Readers/ParsingContext.cs

+        /// </summary>
+        /// <param name="loopId">Any unique identifier for a stack</param>
+        /// <param name="key">Identifier used to </param>
+        /// <returns></returns>


/// [](start = 7, length = 24)

nit, you can just remove these non-filled tags #Closed

Actually, for this one, you might want to fill since it's not that obvious what the returned bool represents.

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

PerthCharern · 2018-02-12T22:50:40Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

            Walk(OpenApiConstants.RequestBody, () => Walk(operation.RequestBody));
            Walk(OpenApiConstants.Responses, () => Walk(operation.Responses));
+            Walk(OpenApiConstants.Callbacks, () => Walk(operation.Callbacks));
+


Walk the Tags array too? #Closed

PerthCharern · 2018-02-12T22:57:49Z

src/Microsoft.OpenApi.Readers/Services/OpenApiReferenceResolver.cs

+            ResolveMap(components.Callbacks);
+            ResolveMap(components.Examples);
+            ResolveMap(components.Schemas);
+            ResolveMap(components.SecuritySchemes);


Shouldn't it be the job of the walker to do this "walking"?

Shouldn't this file just do reference-resolving for the object that's actually a reference and then leave the logic of getting into that object to the Walker?

I wish I could do that. However, we replace the "unresolved" object that is empty, except for the Reference, with the completely filled object. Therefore, I need to detect the reference from the parent object that holds the pointer to the unresolved object so that we can change it.

That is true. It's kinda unfortunate since that means we can't fully leverage the Walking capability.

If we revise the Walk method design to take in another variable (in this case the parent object), and then it can make changes to that variable, then that would work. But that's a big change, and it doesn't seem clean.

@PerthCharern Yes, the walker is not ideal in this scenario. However, it is expected to use the parent object to do certain types of work. e.g. Checking if a value is not null, needs to be done from above. Checking if there are no duplicate parameter names. Checking if two peer values are not contradicting each other.

Passing in the parent object is hard because you don't know its type. It would need to be passed in as an object and then cast to each of the possible parent types. In the end I think it is just as easy to do certain contextually sensitive work from the parent visit method.

PerthCharern · 2018-02-12T22:59:37Z

src/Microsoft.OpenApi.Readers/V3/OpenApiSchemaDeserializer.cs

            {s => s.StartsWith("x-"), (o, p, n) => o.AddExtension(p, n.CreateAny())}
        };

+        private const string schemaLoopId = "schema";


private const string schemaLoopId = "schema"; [](start = 6, length = 47)

remove

PerthCharern · 2018-02-12T23:04:11Z

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

+                //    }
+                //};
+
+                //schemaExtension.AllOf[0].Properties["child"] = schemaExtension;


Does this fail the unit test due to the loop? If so, you can set xUnit to only delve down until specified level just for this test #Closed

I couldn't figure out why it was failing. I kept getting an error that made no sense. Anyway, I just tried running it again and it is working now. #Resolved

PerthCharern

🕐

PerthCharern

darrelmiller added 2 commits January 19, 2018 14:38

Prevent overflow for self-referencing schemas

77586ac

Merge from master

728d061

PerthCharern reviewed Jan 19, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into dm/ref-overflow

16b058d

darrelmiller added 8 commits January 25, 2018 16:31

Only parse reference in first pass

f5a56c7

Merge remote-tracking branch 'origin/master' into dm/ref-overflow

6e833ff

Merge from master

7d3c68d

Merge remote-tracking branch 'origin' into dm/ref-overflow

636cf08

Changed reader to create unresolved references

9fc3ed3

Resolver resolving

202fbe1

Local references working

1697d8a

Merge remote-tracking branch 'origin/master' into dm/ref-overflow

f91e710

darrelmiller added 2 commits February 7, 2018 17:49

Removed extra blank line

d88e18c

Clean up

e5f64cb

darrelmiller mentioned this pull request Feb 8, 2018

NullReferenceException when definitions value is null #210

Closed

darrelmiller added 3 commits February 7, 2018 22:06

Fixed issue resolving response $refs

029280b

Fixed issue with parameter reference

2a4eeb3

Remove uses of old load reference

f12d221

PerthCharern reviewed Feb 12, 2018

View reviewed changes

PerthCharern suggested changes Feb 12, 2018

View reviewed changes

darrelmiller added 2 commits February 12, 2018 23:07

Merge remote-tracking branch 'origin/master' into dm/ref-overflow

42fce80

Fixes based on CR

fb31544

PerthCharern approved these changes Feb 13, 2018

View reviewed changes

darrelmiller merged commit b68b226 into master Feb 13, 2018

darrelmiller deleted the dm/ref-overflow branch February 13, 2018 19:13

Prevent overflow for self-referencing schemas #185

Prevent overflow for self-referencing schemas #185

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

PerthCharern Jan 19, 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 19, 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 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

darrelmiller commented Jan 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

darrelmiller commented Feb 8, 2018

Uh oh!

PerthCharern Feb 12, 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 Feb 12, 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 Feb 12, 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!

darrelmiller Feb 13, 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 Feb 12, 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 Feb 12, 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 Feb 12, 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 Feb 12, 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

darrelmiller commented Jan 19, 2018 •

edited

Loading

PerthCharern commented Jan 19, 2018 •

edited

Loading

PerthCharern Jan 19, 2018 •

edited

Loading

PerthCharern Jan 19, 2018 •

edited

Loading

PerthCharern commented Jan 22, 2018 •

edited

Loading

darrelmiller commented Jan 22, 2018 •

edited by PerthCharern

Loading

PerthCharern commented Jan 24, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

darrelmiller Feb 13, 2018 •

edited by PerthCharern

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

PerthCharern Feb 12, 2018 •

edited

Loading

darrelmiller Feb 13, 2018 •

edited by PerthCharern

Loading