Skip to content
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

Use OpenAPI.NET? #347

Closed
tlmii opened this issue May 25, 2020 · 10 comments · Fixed by #376
Closed

Use OpenAPI.NET? #347

tlmii opened this issue May 25, 2020 · 10 comments · Fixed by #376
Labels
open-api-dotnet Label for everything we deal with during the Open API change
Milestone

Comments

@tlmii
Copy link
Member

tlmii commented May 25, 2020

Change Details

We currently have three separate custom parsers for Swagger/OpenAPI - SwaggerV1ApiDefinitionReader, SwaggerV2ApiDefinitionReader and OpenApiV3ApiDefinitionReader. We can remove those three parsers and make use of the Reader functionality in Microsoft.OpenApi.Readers.

Note: This means we lose support for Swagger V1, but it was determined through discussions below hat was acceptable.

One added benefit, beyond the removal of a lot of code, is that we get support for YAML descriptions "for free", something we'd have needed to add additional parsers for anyway.

Original Issue:

There's an existing parser library - OpenAPI.NET - that we could make use of instead of our custom parser. I'm not positive why it wasn't used in the first place, but best guess is just timing - I believe that when the original parser code was written for HttpRepl, OpenAPI.NET was still under initial development.

From an initial glance at the project, the only downside I see is that it doesn't currently support the v1 spec, and HttpRepl does. But that could be fixed by either a) contributing the v1 implementation to that project, thus helping everyone, or b) dropping v1 support from HttpRepl, since usage is likely low (v2 is almost 6 years old at this point).

On the plus side, it would mean removing the majority of the code in /src/Microsoft.HttpRepl/OpenApi/, would likely save us from having to fix some bugs in the parser (I'm expecting that #345 is one of those, for example) and anything that we do run into, we could push the fix upstream and help everyone.

@timheuer The v1 question probably goes through you first. Also cc @darrelmiller in case he has any data or info that could help us on that decision.

@timheuer
Copy link
Member

@bradygaster do we have prior art here using this in any of our products to date? I think we also need to support v1 for some Azure needs as well.

@darrelmiller
Copy link

@timheuer Azure API Management use it. We use it in Microsoft Graph team. The .NET team use it in their arcade tooling. There are also a number of internal tools that use it.

We can definitely look into adding support for V1 if it is a blocker.

@bradygaster
Copy link
Member

Could this help with generating Swagger from an object graph, as well? Like a 3rd in the series of Swashbuckle and NSwag?

@tlmii
Copy link
Member Author

tlmii commented May 27, 2020

Could this help with generating Swagger from an object graph, as well? Like a 3rd in the series of Swashbuckle and NSwag?

I believe both Swashbuckle and NSwag (via Swashbuckle) are already using OpenAPI.NET for generation of the swagger from an object graph. Making this change would mean we could consume using the same implementation as many of our customers were using to generate it. So that's probably another plus for making the switch.

@bradygaster
Copy link
Member

Rico was working on something like this in the NSwag area.

@bradygaster
Copy link
Member

@darrelmiller do you have any friends who could help us understand how many OAS 1.0 documents are still being created or used? Http-Repl has support for it now, but your lib doesn't. I'm personally okay dropping it. I can't speak for NSwag but I know Swashbuckle has support for v2 and v3, and is v3 by default. My 2c would be if we know there's a low usage count, to not invest in it. Curious your thoughts, d.

@darrelmiller
Copy link

I spoke with Mike Ralphson who manages the APIs.guru repository of OpenAPI descriptions. He believes that the few people who are still using V1.x are only using it for Swagger-UI and have had no reason to upgrade. And Ron Ratovsky the lead for Swagger UI says that the current version of Swagger UI no longer supports v1.x.
It is highly unlikely that anyone actively working on APIs today would be still using V1.x.

@bradygaster
Copy link
Member

then I move we close this for now.

@tlmii
Copy link
Member Author

tlmii commented Jun 18, 2020

@bradygaster The discussion veered off into just what to do about V1 support, but the issue was initially about whether or not to make the move to OpenAPI.NET at all. Should we open another issue to track that work if we're going that direction (which, if we can get rid of V1 support, seems like a no-brainer now)?

@bradygaster
Copy link
Member

bradygaster commented Jun 22, 2020

Reopening as we'll use this to track the move to OpenAPI.NET

@bradygaster bradygaster reopened this Jun 22, 2020
@bradygaster bradygaster added the open-api-dotnet Label for everything we deal with during the Open API change label Jun 22, 2020
@tlmii tlmii added this to the 5.0.0-preview milestone Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-api-dotnet Label for everything we deal with during the Open API change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants