Remove IDictionary implementation, remove OpenApiElement and add IOpenApiSerializable #94

PerthCharern · 2017-11-17T17:55:22Z

Remove OpenApiElement abstract class. Use IOpenApiElement interface throughout to indicate this is an OpenAPI-related object.

Using IOpenApiElement allows us to inherit the dictionary-like objects (Paths, Responses, and SecurityRequirement) directly from the Dictionary<T1, T2> class without having to use IDictionary and re-implment all the dictionary functionalities. Because of this, OpenApiDictionary is no longer needed.
The objects in the spec (which are serializable with WriteAsV2 and WriteAsV3) are marked as IOpenApiSerializable. This allows us to still indicate IOpenApiAny as IOpenApiElement but we will not have to implement WriteAsV2 and WriteAsV3 for IOpenApiAny.

…tionary - We implement this by creating a new EqualityComparer and use that in the dictionary constructor. - Equality only considers Reference.Id since that's what the final document actually cares about. If there are two SecurityScheme with same Reference.Id, they should not both be added to the Schemes dictionary since that will at the end yield the same display string and violate the spec. We also don't need to worry about equality of other downstream objects in the SecurityScheme object. - Unit tests for this equality/dictionary implementation in in SecuirtyRequirementTests. Also, note that the FullDocumentReaderTests now can compare objects directly.

…his is an OpenAPI-related object. - The objects in the spec (that are serializable with WriteAsV2 and WriteAsV3) are marked as IOpenApiSerializable. This allows us to still indicate IOpenApiAny as IOpenApiElement but we will not have to implement WriteAsV2 and WriteAsV3 for them. - Using IOpenApiElement allows us to inherit the dictionary-like objects (Paths, Responses, and SecurityRequirement) directly from the .NET Dictionary<T1, T2> class without having to resort to IDictionary and having to re-implment all the dictionary functionalities. Because of this, OpenApiDictionary is no longer needed.

msftclas · 2017-11-17T17:55:27Z

@PerthCharern,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

PerthCharern · 2017-11-17T18:57:44Z

test/Microsoft.OpenApi.Tests/OpenApiSerializerTestHelper.cs

+                {
+                    return streamReader.ReadToEnd();
+                }
+            }
        }
    }
 }


I'm starting to think these Serializer methods can be made a functionality exposed to customers instead of just for testing.

That way, if a customer simply wants to get string representation of the doc (without having to worry about creating a Stream), it would be just a one method call.

I totally agree.

scott-lin

xuzhg · 2017-11-17T20:02:28Z

src/Microsoft.OpenApi/Models/OpenApiCallback.cs

@@ -59,7 +59,7 @@ public void AddPathItem(RuntimeExpression expression, OpenApiPathItem pathItem)
        /// <summary>
        /// Serialize <see cref="OpenApiCallback"/> to Open Api v3.0
        /// </summary>
-        internal override void WriteAsV3(IOpenApiWriter writer)
+        public void WriteAsV3(IOpenApiWriter writer)


Normally, we discussed it with @darrelmiller to keep these two methods as internal?

Is there any reason we should make them as public?

These methods should be useful for our customers to serialize inner objects (not just documents). I don’t see any disadvantages of exposing them.

Talked to Darrel yesterday and he didn’t mind.

IIRC, the reason we made them internal was solely because we didn’t know how to name these properly. If that’s the only concern, we can have a discussion regarding naming later. I don’t think we should decide on their access level for this reason alone.

Regardless, I can take a look at these whole Serailize stuff again along with adding the Searialize as string extensions to the model in the next PR.

xuzhg · 2017-11-17T20:04:31Z

test/Microsoft.OpenApi.Readers.Tests/OpenApiSerializerTestHelper.cs

-        public static string SerializeAsJson<T>(this T element,
-               OpenApiSpecVersion version = OpenApiSpecVersion.OpenApi3_0)
-               where T : OpenApiElement
+        public static string SerializeAsJson<T>(


xuzhg · 2017-11-17T21:36:50Z

@PerthCharern

Let us take more consideration about making things public. Once it's public, it's hard to change in the later version, because we should avoid introducing any breaking changes in the later version.

Besides, what do we want our customers to use these "WriteV2(3)" method?

PerthCharern · 2017-11-17T21:47:26Z

Thanks for pointing this out. I was a little confused with how we allow our customers to serialize our contracts. I forgot they could already use the Serialize methods to serialize any IOpenApiSerializable.

I will try to revert this, but that means I might need to make a few more tweaks, given the limitation of interfaces.

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

…element. - WriteAsVX is changed to SerializeAsVX. The cutoff point between Write and Serialize is decided on what is being transformed and by whom: - If the whole object is translated into a stream or string, we use Serialize terminology. If the writer (TextWriter) is just adding letters, then that's a Write action. - Serialize methods (to string) in the Test projects are moved to Core.

PerthCharern · 2017-11-17T23:48:31Z

@xuzhg

@scott-lin and I just spent some time discussing this, and here's what makes sense in our point of view:

We allow those 2 methods (now renamed SerializeAsV2 and SerializeAsV3) to be public. These methods are consistent with the other methods in OpenApiElementSerializeExtensions i.e. they can be called with element.SerializeXXXX(.....) syntax. From the customers' point of view, they can be treated just like other Serialize methods, just with different signature (combination of inputs). Now we have these public methods that can be used to serialize IOpenApiSerializable:
- void element.SerializeAsV2(IOpenApiWriter)
- void element.SerializeAsV3(IOpenApiWriter)
- void element.Serialize(IOpenApiWriter, OpenApiSpecVersion)
- void element.Serialize(Stream, OpenApiSpecVersion, OpenApiFormat)
- void element.SerializeAsYaml(Stream, OpenApiSpecVersion)
- void element.SerializeAsJson(Stream, OpenApiSpecVersion)
- string element.SerializeAsYaml(OpenApiSpecVersion)
- string element.SerializeAsJson(OpenApiSpecVersion)
- string element.Serialize(OpenApiSpecVersion, OpenApiFormat)
They look consistent to me, so I don't think there would be any issues with exposing these two interface methods.
The cutoff between Write and Serialize is determined this way:

If the whole object is translated, whether into a stream or string, we use Serialize terminology. If the writer (TextWriter) is adding letters to a stream, then that's a Write action.

In reply to: 345374504 [](ancestors = 345374504,345372185)

…Microsoft/OpenAPI.NET into PerthCharern/FixDictionaryIssue

xuzhg · 2017-11-18T00:29:10Z

Aright，please go ahead and let's think it more.

PerthCharern added 8 commits November 16, 2017 14:45

Style changes in the test

d8c6b0e

This is a good release point. Upping the versions to 1.0.0-beta002

f36b505

Addressed minor style changes

2429dad

Merge from master.

f8d1036

Fix small merging issues.

f8c5fd0

Up version to beta003

bc267e8

PerthCharern requested review from darrelmiller, xuzhg and scott-lin November 17, 2017 17:55

msftclas added the cla-not-required label Nov 17, 2017

Remove default values from TestHelper

ba227c4

PerthCharern commented Nov 17, 2017

View reviewed changes

scott-lin approved these changes Nov 17, 2017

View reviewed changes

xuzhg reviewed Nov 17, 2017

View reviewed changes

PerthCharern and others added 3 commits November 17, 2017 15:51

Merge branch 'master' into PerthCharern/FixDictionaryIssue

e640ad0

Rename extensions and move to Extensions folder.

8dfe3a6

Merge branch 'PerthCharern/FixDictionaryIssue' of https://github.com/…

5305393

…Microsoft/OpenAPI.NET into PerthCharern/FixDictionaryIssue

xuzhg approved these changes Nov 18, 2017

View reviewed changes

Adjust namespace for moved files

d05555f

PerthCharern merged commit 13c32b0 into master Nov 18, 2017

PerthCharern deleted the PerthCharern/FixDictionaryIssue branch November 18, 2017 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove IDictionary implementation, remove OpenApiElement and add IOpenApiSerializable #94

Remove IDictionary implementation, remove OpenApiElement and add IOpenApiSerializable #94

Uh oh!

PerthCharern commented Nov 17, 2017

Uh oh!

msftclas commented Nov 17, 2017

Uh oh!

PerthCharern Nov 17, 2017 •

edited

Loading

Uh oh!

xuzhg Nov 17, 2017

Uh oh!

scott-lin left a comment

Uh oh!

xuzhg Nov 17, 2017

Uh oh!

PerthCharern Nov 17, 2017

Uh oh!

xuzhg Nov 17, 2017

Uh oh!

xuzhg commented Nov 17, 2017

Uh oh!

PerthCharern commented Nov 17, 2017

Uh oh!

PerthCharern commented Nov 17, 2017 •

edited

Loading

Uh oh!

xuzhg commented Nov 18, 2017

Uh oh!

Uh oh!

Remove IDictionary implementation, remove OpenApiElement and add IOpenApiSerializable #94

Remove IDictionary implementation, remove OpenApiElement and add IOpenApiSerializable #94

Uh oh!

Conversation

PerthCharern commented Nov 17, 2017

Uh oh!

msftclas commented Nov 17, 2017

Uh oh!

PerthCharern Nov 17, 2017 • 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!

xuzhg Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

scott-lin left a comment

Choose a reason for hiding this comment

Uh oh!

xuzhg Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

PerthCharern Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

xuzhg Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

xuzhg commented Nov 17, 2017

Uh oh!

PerthCharern commented Nov 17, 2017

Uh oh!

PerthCharern commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuzhg commented Nov 18, 2017

Uh oh!

Uh oh!

PerthCharern Nov 17, 2017 •

edited

Loading

PerthCharern commented Nov 17, 2017 •

edited

Loading