Use Load/LoadAsync pattern to load OpenApi documents

[MaggieKimani1]

Fixes #1434

[darrelmiller]

These should be extension methods so that they appear on the OpenApiDocument class.

[baywet]

we don't have an instance of OpenAPIDocument yet, and as far as I know static extension methods are not a thing. Maybe the design needs to be reviewed here?

[MaggieKimani1]

These should be extension methods so that they appear on the OpenApiDocument class.

So the format should ideally look like this:

var doc = new OpenApiDocument().Load(stream)

[baywet]

in which case we wouldn't be returning the document from the method? Works for me but I'm not sure the internals of OpenAPIDocument are accessible in an extension method outside of the assembly.

[VisualBean]

This would create an odd pattern of

var document = new OpenApiDocument();

document.Load();

Where document would now be null.

Perhaps marking OpenApiDocument as partial could do the trick as well (adding the static load methods in the reader lib)

[MaggieKimani1]

in which case we wouldn't be returning the document from the method? Works for me but I'm not sure the internals of OpenAPIDocument are accessible in an extension method outside of the assembly.

We can return a document in order to avoid null references when we return a void as @VisualBean has indicated here.

[baywet]

This pattern is strange though. Why define those methods as extension methods if we're returning a new instance? How is the consumer supposed to know which instance to hold on to?

[MaggieKimani1]

Yeah you're right, that's a fair point. This then brings us back to how do we ensure the value of the original object is modified using the extension methods?

[VisualBean]

Perhaps simply making static methods on the readers will provide an adequate solution without resorting to new classes people will have to use.

The main issue (compared to XmlDocument) is that reading is separate from the models. And extension methods on non-instances aren't a thing (yet).

The main problem Darrel seems to reference is that the readers aren't really 'readers', they simply parse the entirety of a string/stream/whatever and make a document (or fragment), so newing them seems wrong, as they don't keep state (other than options).

[baywet]

the easiest compromise for now is probably to rename the static class to OpenAPIDocumentFactory (or something like that), and revert back to "regular" static method (not extension methods).
But I'd like to get @darrelmiller confirmation since I don't have all the context here for that work.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[darrelmiller]

Goals

• We would like to move the loading code into OpenApi.Net core library so that people who only want to load JSON don't need a dependency on the third party YAML library
• We would like to avoid developers needing to discover a secondary "factory/reader" class and simply be able to use a static member on the model to load/parse content

Constraints

• We cannot create a circular dependency between the core library and reader library
• We must support loading any OpenAPI model object directly, not just a complete document. (fragments)
• We don't want to duplicate all of the loading/parsing code in every model object's static methods.

Proposal

In the core library we create a static factory for loading/parsing any OpenAPI model object.

T OpenApiModelFactory.LoadAsync(url)
T OpenApiModelFactory.LoadAsync(stream, format)
T OpenApiModelFactory.LoadAsync(textReader, format)
...
T OpenApiModelFactory.Parse(string)

We can add simple static helper methods to the OpenAPIDocument class to avoid the "discovery problem".
OpenApiDocument OpenApiDocument.LoadAsync(stream) -> Calls factory
OpenApiDocument OpenApiDocument.Parse(string) -> Calls factory

We need to be able to add support for other formats in the factory:

OpenApiModelFactory.RegisterReader(IOpenApiReader) <- Making this up. Look at how Kiota does it.

OpenApiJsonReader : IOpenApiReader <- Lives in the core library
OpenApiYamlReader : IOpenApiReader <- Lives in the reader library

OpenApiModelFactory.RegisterReader(new OpenApiJsonReader) <= Do this by default in a static ctor.

// User chooses to enable YAML parsing by adding reference to Microsoft.OpenApiReaders
OpenApiModelFactory.RegisterReader(new OpenApiYamlReader) ;

[baywet]

@darrelmiller

OpenApiJsonReader : IOpenApiReader <- Lives in the core library

Is this representative of things now or a desired change? Why would we play favorite child with a specific format?

OpenApiDocument OpenApiDocument.LoadAsync(stream) -> Calls factory

Assuming the factory lives in readers, and the LoadAsync method in core, this is not going to be possible due to circular references

User chooses to enable YAML parsing by adding reference to Microsoft.OpenApiReaders

Didn't we say we'd enable auto-registration through static constructors? (I've never done that in the past, I'm not sure this is possible)

[MaggieKimani1]

Superseded by #1553