Added more comparers by Shwetap05 · Pull Request #328 · microsoft/OpenAPI.NET

Shwetap05 · 2018-10-05T23:37:02Z

Added comparison logic for

OpenApiInfo
OpenApiExternalDocs
OpenApiSecurityRequirements

- OpenApiInfo - OpenApiContact - OpenApiLicense - OpenApiExternalDocs - OpenApiSecurityRequirements - OpenApiTag

PerthCharern · 2018-10-08T22:09:50Z

test/Microsoft.OpenApi.Tests/Services/OpenApiInfoComparerTests.cs

+                    Style = ParameterStyle.Form,
+                    Explode = false,
+                    AllowReserved = false
+                },


This doesn't look right. Copy-paste error? #Closed

sorry what doesn't seem right?

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

oh got it yes copy paste error for sure

In reply to: 223851085 [](ancestors = 223851085,223510742)

PerthCharern · 2018-10-08T22:11:20Z

src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs

+            comparisonContext
+                .GetComparer<TReference>()
+                .Compare(source, target, comparisonContext);
+        }


This seems like something that should be in its own class OpenApiReferenceComparer rather than the base class #Closed

I tried that but didn't implement it b/c even if i add as separate comparer class like below

public class OpenApiReferenceComparer : OpenApiComparerBase where T : IOpenApiReferenceable

It can't be registered under OpenApiComparerFactory, b/c the type of reference is an enum and can't register various reference comparer based on the value of enum,

That said i will still add the comparer class and reference it directly in the comparers where we diff reference.

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

PerthCharern · 2018-10-08T22:15:28Z

src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs

+                            newKeyInTarget,
+                            target[newKeyInTarget]),
+                        OpenApiComparedElementType = typeof(KeyValuePair<string, string>)
+                    });


Based on this http://jsonpatch.com/, this should be

WalkAndAddOpenApiDifference( comparisonContext, newKeyInTarget, new OpenApiDifference { OpenApiDifferenceOperation = OpenApiDifferenceOperation.Add, TargetValue = target[newKeyInTarget], OpenApiComparedElementType = typeof(string) }); #Closed

I do not see a dictionary example for json patch , so i am not sure if this is correct, can you point me to a dictionary example that you might have found?

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

C# Dictionary should be treated as a JSON object.

The first example on http://jsonpatch.com/
See the second line:

{ "op": "add", "path": "/hello", "value": ["world"] }

and the third line:

{ "op": "remove", "path": "/foo" }

In reply to: 223857178 [](ancestors = 223857178,223511856)

PerthCharern · 2018-10-08T22:16:20Z

src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs

+                            removedKeyFromSource,
+                            source[removedKeyFromSource]),
+                        OpenApiComparedElementType = typeof(KeyValuePair<string, string>)
+                    });


WalkAndAddOpenApiDifference( comparisonContext, removedKeyFromSource, new OpenApiDifference { OpenApiDifferenceOperation = OpenApiDifferenceOperation.Remove, SourceValue = source[removedKeyFromSource], OpenApiComparedElementType = typeof(string) }); #Closed

PerthCharern · 2018-10-08T22:35:14Z

src/Microsoft.OpenApi/Services/OpenApiComparerBase.cs

+
+            comparisonContext
+                .GetComparer<TReference>()
+                .Compare(source, target, comparisonContext);


What's the example case of this? If the references are indeed pointing to the same object, wouldn't these two objects always be the same? #Closed

if Id are same we can't assume that the referenced object will be same too.

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

That could be true in the C# sense, but from the way we use the Reference object, how would that be possible? If the ID is the same, doesn't that mean this refers to exactly the same object. In fact, the reference object will only be serialized as $ref (or plain string in case of tag) with id only.

In reply to: 223858079 [](ancestors = 223858079,223515635)

Chatted with Shweta. We will do a deep comparison here, so that it's useful for API consumer to see what exactly has changed to an API. Same principle applies to the Security Scheme in Security Requirement.

In reply to: 223899238 [](ancestors = 223899238,223858079,223515635)

PerthCharern · 2018-10-08T22:38:47Z

src/Microsoft.OpenApi/Services/OpenApiSecurityRequirementComparer.cs

+                        OpenApiDifferenceOperation = OpenApiDifferenceOperation.Add,
+                        TargetValue = new KeyValuePair<OpenApiSecurityScheme, IList<string>>(
+                            newSecuritySchemeInTarget,
+                            targetSecurityRequirement[newSecuritySchemeInTarget]),


This should not be a key-value pair. We are already pointing the the value itself - just use the IList. #Closed

PerthCharern · 2018-10-08T22:39:17Z

src/Microsoft.OpenApi/Services/OpenApiSecurityRequirementComparer.cs

+            {
+                WalkAndAddOpenApiDifference(
+                    comparisonContext,
+                    newSecuritySchemeInTarget.Reference.Id,


newSecuritySchemeInTarget.Reference.Id, [](start = 19, length = 40)

Does this yield the right string (i.e. just the security scheme name, not the entire json pointer)? #Closed

Yes i have test for it in OpenApiComparerTests

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

PerthCharern · 2018-10-08T22:40:05Z

src/Microsoft.OpenApi/Services/OpenApiSecurityRequirementComparer.cs

+                            OpenApiDifferenceOperation = OpenApiDifferenceOperation.Remove,
+                            SourceValue = new KeyValuePair<OpenApiSecurityScheme, IList<string>>(
+                                sourceSecurityScheme,
+                                sourceSecurityRequirement[sourceSecurityScheme]),


similar to above. This should not be a Key Value Pair #Closed

PerthCharern · 2018-10-08T22:42:11Z

src/Microsoft.OpenApi/Services/OpenApiSecurityRequirementComparer.cs

+                        sourceSecurityScheme.Reference.Id,
+                        () => comparisonContext
+                            .GetComparer<OpenApiSecurityScheme>()
+                            .Compare(sourceSecurityScheme, targetSecurityScheme, comparisonContext));


I don't think we should be comparing the OpenApiSecurityScheme. Shouldn't we be comparing the list of strings in the value side? #Closed

why not compare Security scheme? if source and target reference two different types of security schemes there should be a diff for it.

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

That said i was thinking whether to compare scope list or not, and if i do compare how the pointer should be ?

e.g. if source is

new OpenApiSecurityRequirement
{
[
new OpenApiSecurityScheme
{
Reference = new OpenApiReference {Type = ReferenceType.SecurityScheme, Id = "scheme1"}
}
] = new List
{
"scope1",
"scope2",
"scope3"
}
}

and Target is

new OpenApiSecurityRequirement
{
[
new OpenApiSecurityScheme
{
Reference = new OpenApiReference {Type = ReferenceType.SecurityScheme, Id = "scheme1"}
}
] = new List
{
"scope2",
"scope3"
}
}

Then should diff be #/0 --> remove?

In reply to: 223852204 [](ancestors = 223852204,223516846)

We should not compare SecurityScheme here because we only use its name as a key. From the OpenAPI document standpoint, only the key (id of the ref) will show up. Even if we manually craft the DOM to actually be different (say, add some random other things into the Security Scheme), the resulting serialized document will be exactly the same for this SecurityRequirement object.

Ideally, diff in the above case should be Remove /scheme1/0. However, from the way you handle the Ordered List (in OpenApiOrderedListComparer), this can be

Update /scheme1/0 from scope1 to scope2

Update /scheme1/1 from scope2 to scope3

Remove /scheme1/2

which is also okay (and correct).

In reply to: 223852979 [](ancestors = 223852979,223852204,223516846)

If we want to be really fancy, we can use an algo for "minimum edit distance" to find the best list of update/add/remove (think about one scope being one letter in the original problem). I'd strongly suggest we don't do it in this PR though :)

In reply to: 223900360 [](ancestors = 223900360,223852979,223852204,223516846)

I will have a separate PR to compare scopes

In reply to: 224182344 [](ancestors = 224182344,223900360,223852979,223852204,223516846)

PerthCharern · 2018-10-08T22:43:44Z

test/Microsoft.OpenApi.Tests/Services/OpenApiSecurityRequirementComparerTests.cs

+                    ] = new List<string>
+                    {
+                        "scope4",
+                        "scope5"


Try add test cases with two different scope lists #Closed

I am currently not comparing scope lists, have replied on other thread on question i have on diff of it

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

Will cover in another PR

In reply to: 223853512 [](ancestors = 223853512,223517115)

PerthCharern

🕐

PerthCharern · 2018-10-09T20:29:39Z

This file should be comparing Info objects, not Encoding objects. #Closed

PerthCharern

Added comparison logic for

a674f47

- OpenApiInfo - OpenApiContact - OpenApiLicense - OpenApiExternalDocs - OpenApiSecurityRequirements - OpenApiTag

Shwetap05 requested a review from PerthCharern October 5, 2018 23:37

PerthCharern reviewed Oct 8, 2018

View reviewed changes

PerthCharern suggested changes Oct 8, 2018

View reviewed changes

PerthCharern added the type:feature New experience request label Oct 10, 2018

FIx review comments

f9bdc15

PerthCharern approved these changes Oct 11, 2018

View reviewed changes

Shwetap05 merged commit 59260d3 into vnext Oct 11, 2018

darrelmiller deleted the Comparer-2 branch February 28, 2021 18:55

Conversation

Shwetap05 commented Oct 5, 2018

Uh oh!

PerthCharern Oct 8, 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 Oct 8, 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!

Shwetap05 Oct 10, 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 Oct 8, 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 Oct 8, 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 Oct 8, 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!

Choose a reason for hiding this comment

Uh oh!

PerthCharern Oct 8, 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 Oct 8, 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 Oct 8, 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 Oct 8, 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!

Shwetap05 Oct 9, 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 Oct 10, 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 Oct 8, 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

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

Shwetap05 Oct 10, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

Shwetap05 Oct 9, 2018 •

edited

Loading

PerthCharern Oct 10, 2018 •

edited

Loading

PerthCharern Oct 8, 2018 •

edited

Loading

PerthCharern commented Oct 9, 2018 via email •

edited

Loading