@darrelmiller

I'm seeing from the spec repo that

Open API Initiative is written with a space between Open and API, but OpenAPI Specification doesn't have that space.

https://github.com/OAI/OpenAPI-Specification

Is this intentional? If so, I can update our README to follow the same styling.

The current logic is just a verbatim copy of V3 deserializer, which is incorrect.

We have some build warning like:

warning CS1591: Missing XML comment for publicly visible type or member 'OpenApiConstants.OpenApi'
1>Models\OpenApiConstants.cs(15,29,15,33): warning CS1591: Missing XML comment for publicly visible type or member 'OpenApiConstants.Info'
1>Models\OpenApiConstants.cs(17,29,17,34): warning CS1591: Missing XML comment for publicly visible type or member 'OpenApiConstants.Title'
1>Models\OpenApiConstants.cs(19,29,19,33): warning CS1591: Missing XML comment for publicly visible type or member 'OpenApiConstants.Type'
1>Models\OpenApiConstants.cs(21,29,21,35): warning CS1591: Missing XML comment for publicly visible type or member 'OpenApiConstants.Format'

///

I will help to add the missing XML.

@darrelmiller I don't think we should release the fixtures folder with this repo. It has more to do with the spec itself. If we need some of these docs in the unit tests, we can include them directly there. What do you think?

continuation from PR #113

I'll add the attribute [Collection("DefaultSettings")] in a separate PR to keep #113 reasonably sized.

Need to change collection objects to be empty by default and not render out when empty and optional

@xuzhg @darrelmiller What do you guys think about changing the Deserializer from one big file with multiple regions into multiple partial classes (based on the model)?

One advantage I see is that now the reader files can be easily mapped 1-to-1 to the model, and it will be clearer to the contributor which file(s) to touch when a model is changed.

Path items can be referenced but they are a special case for a couple of reasons.
a) they don't have a home in components
b) the reference doesn't work like a normal JSON reference because peer properties in the target path item are not ignored, they are merged.

@xuzhg @darrelmiller

Is one of you working on this already? If not, I can take a stab at this.

Based on our last discussion, we will have something along the line of:

• An interface class (possibly with generic template)

• Load/Parse/Read method

@darrelmiller Darrel, you mentioned in the call that we can base the methods out of XDocument. I looked through that but it seems like most other methods (Write, ToString, ReplaceNode, Add, etc.) don't really fit in out concept of the reader. I'd say we only have one "read" method here. I think Read seems like a good name for me (instead of Load/Parse) since we name the projects Readers - do you guys agree/disagree?

Discriminator.Mapping can contain string that refers to schema reference. Currently it's just string right now. We might be able to switch this over to something that leverages our Reference implementation. (similar to tag/security scheme)

static void Main(string[] args)
{
      OpenApiDocument document = new OpenApiDocument();
      Console.WriteLine(document.SerializeAsJson(OpenApiSpecVersion.OpenApi3_0_0));
}

Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object. at Microsoft.OpenApi.Models.OpenApiDocument.SerializeAsV3(IOpenApiWriter writer) at Microsoft.OpenApi.Extensions.OpenApiSerializableExtensions.Serialize[T](T element, IOpenApiWriter writer, OpenApiSpecVersion specVersion) at Microsoft.OpenApi.Extensions.OpenApiSerializableExtensions.Serialize[T](T element, Stream stream, OpenApiSpecVersion specVersion, OpenApiFormat format) at Microsoft.OpenApi.Extensions.OpenApiSerializableExtensions.Serialize[T](T element, OpenApiSpecVersion specVersion, OpenApiFormat format) at Microsoft.OpenApi.Extensions.OpenApiSerializableExtensions.SerializeAsJson[T](T element, OpenApiSpecVersion specVersion) at OpenApiTest.Program.Main(String[] args) in E:\study\OpenApiTest\OpenApiTest\Program.cs:line 17 Press any key to continue . . .

1. Design the validation rule framework
2. Add the validator

String handling

https://stackoverflow.com/questions/19109912/do-i-need-quotes-for-strings-in-yaml

For example,

If the string "10" is the input, WriteValue("10") should yield '10' (with single quote) to signify that this is indeed a string. Otherwise, this 10 can be interpreted as a number.

Empty object inside the document

{} can be used for empty object. Current implementation only shows empty space, which is not correct.

Empty array inside the document

Add --- in the beginning of file

• check in requires build + test to pass

estimate:

Strong naming 5 min

Authenticode not sure if possible. Can check OSS guideline. A day at most.

So far, we read string and only use OpenApiString to host it.

We should convert the string into the correct IOpenApiAny object.

For example:

When we read the response, Can we make sure the response status codes is HTTP status code (3-digit-number starting with 1, 2, 3, 4, 5 or the range definition allowed: 1XX, 2XX, 3XX, 4XX, 5XX)

@darrelmiller Darrel, is the Workbench project supposed to be used for something? Can we remove it?

These are required fields, therefore assigning a default value effectively invalidates that requirement. Should we raise a validation error if either of those values are not provided?

Please share your thoughts:

Open API spec says:

So, the Any type could be:

1. Null,
2. Primitive
3. Array
4. Object

While, Open API spec says:

So, I think I want to use the PR #15: to represent the structure.

Unit tests we have are not sufficient, and the way each test is written is inconsistent with one another.

It currently seems to be unused. Do we need it?

@xuzhg I'm creating these subtasks to let you know which ones I'm working on (so that we don't collide too much).

Subtask of #44

Currently the DOM is designed so that strong types such as OpenAPISchema can be marked as a reference. This enables the model to know that these types are explicitly being re-used from the components collection.

Currently references are loaded immediately upon parsing a document so that references are always represented by the correct types and those instances contain all the referenced data. This is convenient when working with the object model as referenced objects and non-referenced can be interacted with transparently.

When it comes to loading references from external documents, this approach presents some challenges:

• Loading from an external document should be async. Transparently loading external references would push async all the way through the model to the main load even if there were no external references in a particular document.
• What if someone wanted to process a set of documents that contained external references but didn't need to load them to perform the updates? Should we not support loading documents without dereferencing external references? If so how should those references be represented in the model?
• How do you construct a DOM with an external reference? Do you need an instance of the external DOM to do it?

It feels like we cannot get away from having a model type that can have a OpenApiReference instance but be in an "unresolved" state.

   var unresolvedSchema = new OpenApiSchema {
      Reference = new OpenApiReference("common.json#/components/schema/cat")
   };

Does this mean that we need to put a guard clause in every get accessor that prevents reading the regular properties? How do we construct a reusable object to be added to Components? Do we create it without the reference and then when adding it to the components collection a reference gets added automatically?

Or do we change the approach to the way we deal with references and instead of using the same object in the components collection and the reference to it, we create a new SchemaReference type that does aggregation and delegation.

e.g.

   content.Schema = new SchemaReference("cat",doc.Components.Schemas["cat"]);

This would require the SchemaReference to re-surface the entire Schema API. This would provide a nice place to put the guard against derefencing an unresolved reference, but it would also mean either creating an ISchema interface, or deriving from Schema and making all the properties virtual. Both of which are high maintenance approaches.

SecurityScheme object is used as dictionary key in SecurityRequirement.Schemes.

We need to do one of the following:

1. Implement GetHashCode / Equals in SecurityScheme and ALL the downstream classes
2. Implement SecuritySchemeComparer : IEqualityComparer and use that to construct the dictionary. This is an ad-hoc comparer and all the dive-down equality comparisons will only be present in this class.

I personally prefer 1, but it might make the models a little larger and cluttered. I'll send out a PR to see what you guys think.

@darrelmiller @xuzhg

We have been using an OpenApiAny type (e.g. OpenApiString, OpenApiDouble) to represent strings, numbers, arrays, etc. in the property that can be of type OpenApiAny.

However, in other properties of a normal object, we simply use the C# primitive types like string, int.

This is semantically inconsistent in my opinion. An "Any" type in the spec simply means it can be anything. If we use OpenApiString to mean a string inside that "Any" type, we should exclusively use OpenApiString everywhere. If we use the C# primitive, then we should use the C# primitive everywhere.

Using OpenApiAny allows us to control the type to match what JSON schema allows and add some types recognized by the spec like Password, but will be a pain for our customers if they need to assign values with OpenApiAny everywhere since it's less intuitive to do so compared to just C# primitives.

Continuation of #109.

#109 (through PR #113) already sets the structure of how the tests can be written for both versions of the deserializers. The remaining job is actually going ahead and write the tests for each and every object.

I am a big fan of the Fluent Assertions framework http://fluentassertions.com/, which integrates well with xUnit.

It gets rid of the Assert.XXX syntax and use a more human-readable actual.Should().Be().EqualTo(expected) syntax. It also provides out-of-the-box support for deep object comparison. The error message pinpoints where exactly two objects are different.

I don't see a downside except from an extra dependency. This project is available on NuGet and widely used though, so I don't think that's too big of an issue either.

@andystumpp

"In order to set the reference service in the context, you need the rootnode, for that though you need the context to have (partially?) initialized... Is there some potential circular reference?"

This is a valid concern. Even if we do not have any circular reference, the flow seems a little strange and difficult to understand and debug if there are any issues later on.

When I sent the Parameters as "null", this line will throw exception:

https://github.com/Microsoft/OpenAPI.NET/blob/master/src/Microsoft.OpenApi/Models/OpenApiOperation.cs#L202

We should agree on a coding guideline to use:

I suggest we follow the one prescribed for aspnet

https://github.com/aspnet/Home/wiki/Engineering-guidelines#coding-guidelines

I looked through some of the major Microsoft OSS repos and they generally follow this guideline. It's very generic and does not prescribe too many rules; rules not explicitly prescribed mean that Visual Studio default should be followed.