-
Notifications
You must be signed in to change notification settings - Fork 280
Open api schema diff #316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open api schema diff #316
Conversation
- Added various OpenApiComparer's to compare fragments of OpenApiDocument
|
|
||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Microsoft.OpenApi.Models; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ft.OpenAp [](start = 13, length = 9)
Still adding more tests for comparers #Closed
| } | ||
|
|
||
| var comparisionContext = new ComparisonContext(new OpenApiComparerFactory()); | ||
| var comparisionContext = new ComparisonContext(new OpenApiComparerFactory(), source, target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comparisionContext [](start = 16, length = 18)
nit, typo #Closed
| /// <param name="source">The source.</param> | ||
| /// <param name="target">The target.</param> | ||
| /// <param name="comparisonContext">The context under which to compare the objects.</param> | ||
| internal void Compare<TE>(Enum source, Enum target, ComparisonContext comparisonContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TE [](start = 30, length = 2)
TEnum #Closed
|
|
||
| WalkAndCompare( | ||
| comparisonContext, | ||
| OpenApiConstants.Components, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components [](start = 33, length = 10)
Servers #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| comparisonContext.AddOpenApiDifference(new OpenApiDifference | ||
| { | ||
| OpenApiDifferenceOperation = OpenApiDifferenceOperation.Update, | ||
| OpenApiComparedElementType = typeof(T), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T [](start = 56, length = 1)
TEnum? #Closed
need to escape ~ to ~0 too. comparisonContext.Enter(segment.Replace("~", "~0").Replace("/", "~1")); Same thing with the one below. #Closed Refers to: src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs:152 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
| WalkAndCompare(comparisonContext, OpenApiConstants.AllowReserved, | ||
| () => Compare(sourceEncoding.AllowReserved, targetEncoding.AllowReserved, comparisonContext)); | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, remove this extra line #Closed
| { | ||
| WalkAndAddOpenApiDifference( | ||
| comparisonContext, | ||
| "$ref", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$ref" [](start = 20, length = 6)
Use DollarRef constant #Closed
| if (sourceHeader.Reference != null) | ||
| { | ||
| sourceHeader = (OpenApiHeader) comparisonContext.SourceDocument.ResolveReference( | ||
| targetHeader.Reference); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targetHeader [](start = 20, length = 12)
sourceHeader #Closed
|
|
||
| WalkAndCompare( | ||
| comparisonContext, | ||
| OpenApiConstants.Content, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content [](start = 33, length = 7)
Encoding #Closed
Is it not possible to model this based on the VisitorBase, i.e. in a similar way to https://github.com/Microsoft/OpenAPI.NET/blob/vnext/src/Microsoft.OpenApi/Validations/OpenApiValidator.cs? #Closed Refers to: src/Microsoft.OpenApi/Services/OpenApiComparer.cs:12 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
| { | ||
| WalkAndAddOpenApiDifference( | ||
| comparisonContext, | ||
| "$ref", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$ref" [](start = 20, length = 6)
DollarRef constant #Closed
| { | ||
| WalkAndAddOpenApiDifference( | ||
| comparisonContext, | ||
| "$ref", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$ref" [](start = 20, length = 6)
DollarRef constant #Closed
|
|
||
| WalkAndCompare( | ||
| comparisonContext, | ||
| OpenApiConstants.Components, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components [](start = 33, length = 10)
Servers #Closed
This number is misleading. It's a bit difficult to show the differences of a list. Suppose source = [1,2,3] How do we want to report this? The easiest way seems to be the most straightforward way: report as updates 1->3, 2->1, 3->4 (indexing is easy), and then add 5. But if It seems like the easiest way to understand this is to report that 2 has been removed and 4 has been added. However, this is not easy to determined. First, we need to get the sublist of the two (in this case [1,3]). If we see that everything in the sublist is still in the same order, then we can report the rest as simply additions or deletions. I think it's fine if we want to add in this "smarter" method later and simply report it with a straightforward way in this PR (go element by element). In this case, we should report it as updates (2->3 and 3->4). You can decide. #Closed Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
|
|
||
| WalkAndCompare( | ||
| comparisonContext, | ||
| OpenApiConstants.Parameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameters [](start = 33, length = 10)
Servers #Closed
That way, we don't need to handle the walking separately. We can use it as: var comparingWalker = new OpenApiWalker(openApiComparer) directly. Something like how we use the validators in this test: In reply to: 424881544 [](ancestors = 424881544) Refers to: src/Microsoft.OpenApi/Services/OpenApiComparer.cs:12 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
| && expectedDiff.OpenApiDifferenceOperation == | ||
| actualDiff.OpenApiDifferenceOperation)).ToList(); | ||
|
|
||
| notExpectedDifferences.Count.ShouldBeEquivalentTo(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just do
differences.ShouldBeEquivalentTo(expectedDifferences) ? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i can't do that, as in the difference result we have SourceValue and TargetValue of type object and equivalent fails when i use the actual type e.g. Server in expected result.
That is why only comparing fields except Target And Source
In reply to: 220743996 [](ancestors = 220743996,220743939)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ShouldBeEquivalentTo should be able to to do a deep comparison into any object type. In your expectedDifferences, you can assign the correct sourceValue and targetValue and FluentAssertion should be able to compare it with the differences you got.
In the case there's a bug in FluentAssertion library and the above doesn't work, you can simply exclude certain path from comparison in fluent assertion. https://stackoverflow.com/questions/22142576/how-to-use-exclude-in-fluentassertions-for-property-in-collection
This will ensure this test has good coverage even when we update OpenApiDifference in the future.
In reply to: 221082998 [](ancestors = 221082998,220743996,220743939)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting it worked, i remember it not working somewhere will fix all test.
In reply to: 221094506 [](ancestors = 221094506,221082998,220743996,220743939)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually does compare, right? Did you try testing it by intentionally making it fail? Just want to make sure it's not just giving out an automatic pass without comparing.
In reply to: 221702898 [](ancestors = 221702898,221094506,221082998,220743996,220743939)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes tested it, it does compare it and not false positive.
In reply to: 221749561 [](ancestors = 221749561,221702898,221094506,221082998,220743996,220743939)
PerthCharern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
The diff that is being performed above is not index to index so if source = [1,2,3] diff is : New parameters 4,5 but the index of them in path would be 0.1 which is not correct, it should be 2,3 respectively. Removed parameter 2, but the index of it in path would be 0, again not correct and should be 1. so i will fix code to represent the location from the source/target list. so targetParameters.IndexOf( newParametersInTarget[i] ).ToString() for new parameters In reply to: 424889731 [](ancestors = 424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
While designing this feature i did go through these files implementing visitor pattern, and couldn't use them b/c VisitorBase is the class that manages path stack and Validator derives from it and leverages visiting logic from it. But Comparer cannot derive from VisitorBase as it just visits one document. Make sense? In reply to: 424890444 [](ancestors = 424890444,424881544) Refers to: src/Microsoft.OpenApi/Services/OpenApiComparer.cs:12 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
That's fair. There might be a way to unify these two, but that can be done as a separate work. In reply to: 425254973 [](ancestors = 425254973,424890444,424881544) Refers to: src/Microsoft.OpenApi/Services/OpenApiComparer.cs:12 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
But order should matter. If source [3,1] This should report 2 updates. If we are not going with the index-to-index comparison, we'll need to first find a baseline on how to determine which objects remain the same in the list. In reply to: 425251278 [](ancestors = 425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
It must be in the order I described above. Otherwise the ~ in ~1 will get replaced again. i.e. / will be come ~01 (which is not what we want) In reply to: 424875930 [](ancestors = 424875930) Refers to: src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs:152 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
But if i am consumer of diff and i moved around my parameters in terms of index so per me that should generate a diff In reply to: 425256973 [](ancestors = 425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
Not sure if I understand. Based on the current code, if you put in something like source [3,1] it will not generate any diff. This doesn't sound right to me. In reply to: 425257623 [](ancestors = 425257623,425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
so if you perform suppose a string list compare options in C# it also does index variant compare and it will not result in diff. And if in a OpenApiDocument i have parameters shuffled around e.g. like below it should not result in diff SOURCE TARGET: "parameters": [ In reply to: 425258563 [](ancestors = 425258563,425257623,425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
Is that the behavior we want though?
There's a bit of hint suggesting that order matters in the tags section of the OpenApi Object. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#openapi-object
I don't think they should be considered the same list. In reply to: 425260367 [](ancestors = 425260367,425258563,425257623,425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
May be we can provide both options as setting so consumer of library can set it based on their needs, i will chat with you offline tomorrow about this in more details. In reply to: 425262254 [](ancestors = 425262254,425260367,425258563,425257623,425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
Discussed with @Shwetap05. We will use sourceIndex for removal, and targetIndex for addition. We will treat parameter list as unordered given its semantic meaning. In reply to: 425273672 [](ancestors = 425273672,425262254,425260367,425258563,425257623,425256973,425251278,424889731) Refers to: src/Microsoft.OpenApi/Services/OpenApiParametersComparer.cs:46 in 4a66112. [](commit_id = 4a66112, deletion_comment = False) |
| WalkAndAddOpenApiDifference( | ||
| comparisonContext, | ||
| i.ToString(), | ||
| targetParameters.IndexOf(newParametersInTarget[i]).ToString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.IndexOf(newParametersInTarget[i]) [](start = 36, length = 34)
Interesting way to use IndexOf. :) #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| WalkAndCompare( | ||
| comparisonContext, | ||
| i.ToString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.ToString(), [](start = 1, length = 32)
This one is kinda tricky but I think it's already alright. It makes sense to use the source index here. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: With the changes as discussed, we will need to use the target index here.
In reply to: 221344148 [](ancestors = 221344148)
PerthCharern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
Added more comparers.