-
Notifications
You must be signed in to change notification settings - Fork 67
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
Comments
@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. |
@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. |
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. |
Rico was working on something like this in the NSwag area. |
@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. |
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. |
then I move we close this for now. |
@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)? |
Reopening as we'll use this to track the move to OpenAPI.NET |
Change Details
We currently have three separate custom parsers for Swagger/OpenAPI -
SwaggerV1ApiDefinitionReader
,SwaggerV2ApiDefinitionReader
andOpenApiV3ApiDefinitionReader
. We can remove those three parsers and make use of the Reader functionality inMicrosoft.OpenApi.Readers
.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.
The text was updated successfully, but these errors were encountered: