OpenApiComparer - Stack OverFlow

[AndrewRissing]

Steps to Reproduce

Commit: cb3c26b (vnext)
Code:

var input = await new HttpClient().GetStringAsync("https://petstore.swagger.io/v2/swagger.json");
var doc = new OpenApiStringReader().Read(input, out _);

var result = OpenApiComparer.Compare(doc, doc);  // StackOverFlowException here

Details

While testing out the OpenApiComparer against a fairly vanilla example (Swagger's Petstore), I noticed that the code kept failing with a stack overflow.

Digging into it, the calls kept bouncing between the OpenApiSecuritySchemeComparer and OpenApiReferenceComparer trying to dereference a self-referencing element.

The self-reference is due to this logic in the MapNode class that replaces any null Reference with a reference to itself.

This hasn't been caught before due to all of the unit tests creating a C# OpenApiDocument by hand, which doesn't exhibit the same self-referencing produced by the MapNode class.

Suggested Solution

I'm currently working on a PR to resolve the above, but any comments before I get there would be appreciated if the below approach isn't the desirable path.

I've recreated the problem within the OpenApiComparerTests by creating a visitor that updates all of the Component IOpenApiReferenceable collections with a self-reference that mirrors the behavior of MapNode.

I've replaced the OpenApiSecuritySchemeComparer's logic here with the following:

if (sourceSecurityScheme.Reference != null)
{
    sourceSecurityScheme = (OpenApiSecurityScheme)comparisonContext.SourceDocument.ResolveReference(
        sourceSecurityScheme.Reference);
}

if (targetSecurityScheme.Reference != null)
{
    targetSecurityScheme = (OpenApiSecurityScheme)comparisonContext.TargetDocument.ResolveReference(
        targetSecurityScheme.Reference);
}

This corrects the problem with the provided 'PetStore' example, but the remaining tests are failing. I'll apply a similar change to the other impacted comparers and create the PR once done.

This issue seems to exist in the following as well:

• OpenApiExampleComparer
• OpenApiParameterComparer
• OpenApiRequestBodyComparer

As a side note, I'm not sure how crucial that self-reference is, but if it could be removed that might benefit the codebase and potentially those trying to walk the OpenApiDocument themselves.

[AndrewRissing]

I've created two PRs recently and they're unfortunately intertwined a bit, due to the fact that I didn't want to fix the unit tests twice as they stepped over each other a bit in that area.

Here's the PRs:

• Corrected expected ordering for skipped unit tests. #504 - Fixing skipped unit tests and general unit test clean up
• Fix stack overflow #506 - Fixing the stack overflow
  • Fix stack overflow #506 contains the commits for Corrected expected ordering for skipped unit tests. #504

Let me know how you want to proceed and I'll clean up the PRs based on your feedback.

[darrelmiller]

@AndrewRissing Thank for the fixes. I knew there were a couple of unit tests that we failing for the comparer's and I just had never got around to investigating.

When I last reviewing the comparer code, I was a bit concerned with the approach that has been taken. As you noted it doesn't seem to handle cycles properly. It requires creating a new comparer class for every model class, and the end result is a difference object which I'm not sure what exactly we can do with it.

I have been considering a different approach where each model object would be responsible for comparing itself against some passed in compare target with the output being a delta that would fit into the Overlays model that are currently trying to define.

This would allow comparing doc A and A', which would then produce an Overlay document that when applied to A would produce A'. This would make easier to build integration tests that could validate the roundtripping process.

I would be interested to hear your thoughts on the current implementation. I'm concerned about the maintenance effort involved in keeping the comparing code working as the model evolves.

[AndrewRissing]

I'll try to respond in order to your comments above.

As you noted it doesn't seem to handle cycles properly.

Looking through the comparers, I couldn't find any other logic that seemed to suffer from the cyclic issue I found. Obviously, there might be other failure modes I'm not considering but this particular should be resolved.

As for preventing other future issues, perhaps moving some of the repetitive IOpenApiReferenceable logic in the comparers (for example) into the base classes might reduce chances for a bug to occur.

Unrelated, this logic could probably be pulled into the base classes as well. But none of those changes are earth shattering in their impact. They're more of a tidying up of the code base.

It requires creating a new comparer class for every model class ...

From what I can tell, the factory holds a single instance. Unless, the concern is having to create a new 'comparer' class for each type.

... and the end result is a difference object which I'm not sure what exactly we can do with it. ... I have been considering a different approach where each model ...

What do you see are the use cases for someone that the difference object doesn't solve but overlays would? I haven't dug into the link that much, so I might be missing some aspect.

For me, I'm going to use the OpenApiComparer to find all of the changes between two releases of a service and determine if the changes were breaking or not. This seems like a common need, but you might have others in mind as well.

I'm concerned about the maintenance effort involved in keeping the comparing code working as the model evolves.

If I were going to design unit tests that would be a little less brittle, I would doing the following:

• Adjust all comparer unit tests to validate changes on a single difference per test (some seem to do this already)
• Adjust the OpenApiComparerTests to order the resulting differences (e.g. by the Pointer) so that changes in the comparer's logic would be less likely to cause the unit test to fail. I don't believe the order of the differences should matter, but perhaps someone might consider that 'part of the contract'.

If I'm missing some other aspect that is a maintenance concern, please feel free to share.

[AndrewRissing]

@darrelmiller Since you merged the PR, I'm closing this. If you want to continue the discussion above, I'm more than willing.

Otherwise, take care and have a good weekend.

[darrelmiller]

@AndrewRissing I need to spend some time quality time with the comparers to fully grok the design. I explored creating the diffs in the object model and it didn't going nearly as well as I had hoped.

I'm really happy you were able to fix this implementation. I would like to be figure out how to fix the issues with creating references and things that are referenced in code. It is causing me issues in other places because references have not been setup correctly.

Here's hoping I find some time tomorrow to explore.