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

Add swagger model APIs #520

Closed
wants to merge 2 commits into from
Closed

Add swagger model APIs #520

wants to merge 2 commits into from

Conversation

xuzhg
Copy link
Member

@xuzhg xuzhg commented Sep 29, 2015

Add the Utility method for Swagger converter

/// <summary>
/// Represents an <see cref="ODataSwaggerConverter"/> used to converter an Edm model to Swagger model.
/// </summary>
public class ODataSwaggerConverter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E2E test case to show how customers can create a controller or action to return swagger spec, when a request uri is:
~/service.root/swagger.json, or ~/service.root/$swagger.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my new updated. Thanks

@xuzhg xuzhg force-pushed the SwaggerMapping branch 3 times, most recently from b31d111 to 7994f5c Compare October 21, 2015 06:44
@xuzhg
Copy link
Member Author

xuzhg commented Oct 21, 2015

@OData/odata-team I update the PR. Would you please take a look? Thanks

}

/// <summary>
/// Convert the Edm model to Swagger model.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converts? Same for verbs below. Since Verbs above are all have suffixes. Just keep consistent, with or without suffix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.

@congysu congysu added this to the v5.8 milestone Oct 29, 2015
xuzhg added a commit that referenced this pull request Oct 30, 2015
@xuzhg
Copy link
Member Author

xuzhg commented Oct 30, 2015

Merged. Thanks.

@joshbooker
Copy link

Many thanks to OData team for adding swagger generation.

However, there is still a problem with the resulting swagger output in interoperability with Azure. For example, AutoRest is a tool which generates client code from swagger.json and it requires OperationIDs on all operations.

https://github.com/Azure/autorest

The swagger being output from WebAPIOData is missing the required OperationIDs.

Not sure if this can be fixed before 5.8 release but it would be really swell if the resulting swagger was Azure compliant.

#302

@bradygaster-zz
Copy link

Wait, you guys added Swagger generation to OData and didn't tell me about it? Shame on you @markdstafford for losing touch with me. :) BUT SERIOUSLY. Where's a sample, I need to party on this with Justice.

@joshbooker
Copy link

@bradygaster the swagger gen was merged but not yet released. You can clone and build to try it out. The sample code is in the test here:
https://github.com/OData/WebApi/tree/master/OData/test/E2ETest/WebStack.QA.Test.OData/Swagger

I'm afraid, cuz they didn't use swashbuckle, we cannot override and affect the swagger output. Hope I'm wrong on that.

@TomDu
Copy link
Contributor

TomDu commented Nov 11, 2015

@joshbooker, we added operationId for operations in PR #552, would you please try it out and let us know any issues, thanks.

@joshbooker
Copy link

@TomDu,

Wow...Thanks! ...Ask and you a shall receive! I haven't tried this yet, but have one more suggestion:

OperationIds are used in AutoRest to generate Classes using a naming convention. For example, consider the following swagger:

"paths": {
"/Customers": {
"get": {
"operationId": "Customers_Get",
..........
"/Customers({Id})": {
"get": {
"operationId": "Customers_GetById",

If there is an underscore in the OperationId, AutoRest will use the convention [ClassName_MethodName] to generate a Customers class having .Get and .GetById as methods.

More info:
https://github.com/Azure/autorest/blob/master/Documentation/defining-clients-swagger.md#generating-operation-classes

It would be really swell if OData swagger used this convention for OperationIds.

Also see this early example of how WebAPI team handles multi parameter operations:

https://azure.microsoft.com/en-us/documentation/articles/app-service-api-dotnet-swashbuckle-customize/

I believe WebAPI team recently settled on a swashbuckle IOperationFilter that handles this, but cannot find a blog about that. Maybe @bradygaster has more info.

Thanks for listening!
#302
#552

@joshbooker
Copy link

@TomDu,

PS...Is any of the OData swagger gen override-able? It would be great if we could have more control over swagger output. Did you consider allowing APIExplorer and using swashbuckle as the rest of WebAPI team is doing?

@TomDu
Copy link
Contributor

TomDu commented Nov 12, 2015

@joshbooker , we updated the naming convention for operationId in #552, and the swagger format could be customized by overriding methods in ODataSwaggerConverter.

@markdstafford
Copy link

@bradygaster - ;) You of all people know how high on my priority list this is. To be clear, there's a big loss of fidelity still and a lot of work to do even just within our bits.
@joshbooker - Always appreciate your feedback. Keep it coming. I don't think Swashbuckle would work very well for us - or any hypermedia API. Web API works off of the presumption that URLs are a very finite space. OData really doesn't work that way. That said, we should probably take a closer look at what they're doing - I may have missed the potential for us to build on their foundation.

@joshbooker
Copy link

@markdstafford thanks for the comments and your teams fast turn around is impressive. Perhaps look at swashbuckle ISwaggerProvider for wrapping your custom swagger gen. Curious whether that would enable post-gen customization of swag via swagger.config making this more on par with webAPI method for affecting swagger output. Thanks for listening.

@bradygaster-zz
Copy link

yes indeed i know how high a pri this is for you @markdstafford and like @joshbooker i sincerely appreciate the efforts you're putting into this.

@joshbooker
Copy link

@TomDu & @markdstafford,

Does this work for RESTier?

I tried this in vanilla WebAPI\OData project and was indeed able to get swagger output by building system.web.odata and then using code from the PR#552 test project (SwaggerController, SwaggerPathHandler, SwaggerPathSegment, SwaggerRoutingCovention, etc.)
Is this the correct way to output swagger?
Curious whether the plan includes building this into OData Routing Conventions to make swagger output simpler to implement.(?)
Also, in the test code, adding the swagger routing convention is done by passing conventions collection into MapODataServiceRoute(). I tried this on a RESTier project and found the new MapRestierRoute doesn't have a signature which accepts conventions. Is there another way?

Let me know if I'm off track here - basically I'm hoping:

  1. swagger metadata will be seemless and built-in just like $metadata
  2. It will work with RESTier as well.

Thanks for listening.

@xuzhg
Copy link
Member Author

xuzhg commented Nov 19, 2015

@joshbooker Our original design to support $swagger is same as $metadata. However, it's rejected by some reason. If you want to get the original implementation, please let me know. I can share it.

@congysu
Copy link
Contributor

congysu commented Nov 19, 2015

The goal of swagger support in OData v4 Web API is with flexibility and can be customized. This requires:

  1. A flexible, and tolerable Uri parser in OData v4 Lib Uri parser to support Swagger odata.net#129
  2. A set of customizable and replaceable utility classes in OData v4 Web API. Ideally, with a POCO controller. Before item1 is done, there is a need to use SwaggerPathHandler, which might not meet a production bar.
  3. A turn key solution in RESTier. RESTier provider for Swagger RESTier#69

@joshbooker
Copy link

Thanks for your replies.
@xuzhg , please share if what you have is more turnkey than this implementation.
@congysu, regarding #3: a swagger 'provider' in RESTier is a connector like that to Entity Framework but instead connects to a REST API that exposes swagger metadata. This is great, but different and unrelated to the ability to expose metadata of a RESTier OData endpoint as swagger.json, correct?
We need RESTier to output swagger.json in order to be used in logic apps for example.

@joshbooker
Copy link

@markdstafford Here is an example of extending Swashbuckle for OData by wrapping your converter in ISwaggerProvider. https://github.com/rbeauchamp/Swashbuckle.OData
Nice Work @rbeauchamp !
cc: @bradygaster @congysu @xuzhg @TomDu

@joshbooker
Copy link

Not sure if anyone's still listening to this closed PR but....
the following code is all that's missing for swagger output via swashbuckle custom provider:
https://github.com/rbeauchamp/Swashbuckle.OData/blob/master/Swashbuckle.OData/ODataSwaggerProvider.cs
Please build this into your release.

@xuzhg
Copy link
Member Author

xuzhg commented Dec 2, 2015

@joshbooker Sorry, I don't listen to this closed PR recently. I re-open it to track on.

@xuzhg
Copy link
Member Author

xuzhg commented Dec 2, 2015

@joshbooker you can find our original implementation here. Thanks.

@congysu
Copy link
Contributor

congysu commented Dec 2, 2015

@joshbooker The 3. A turn key solution in RESTier. OData/RESTier#69 is for RESTier to output a swagger.json. Created a new issue to make it clear: OData/RESTier#265
Your and @rbeauchamp's contributions are very welcome.

@joshbooker
Copy link

@xuzhg @congysu Thanks all for reopening. Just to clarify...
the code here:
https://github.com/rbeauchamp/Swashbuckle.OData/blob/master/Swashbuckle.OData/ODataSwaggerProvider.cs
does the following:

  1. wraps your ODataSwaggerConverter in Swashbuckle ISwaggerProvider
  2. enables swagger output via Swashbuckle at the default location /swagger/docs/v1
    3) enables users to post-modify swagger output via standard swashbuckle methods in(SwaggerConfig.cs) - just like we do for vanilla WebAPI projects. not yet but with SwaggerGeneratorOptions implemented like so:
    https://github.com/domaindrivendev/Swashbuckle/blob/master/Swashbuckle.Core/Swagger/SwaggerGenerator.cs
  3. it works with RESTier too

If you simply add ODataSwaggerProvider.cs to System.Web.OData then you're done.

Thanks again for listening.
cc: @rbeauchamp , @markdstafford

@congysu
Copy link
Contributor

congysu commented Dec 3, 2015

Welcome @joshbooker, @rbeauchamp's contribution to the OData/WebApi repo. Re-opened #302.

@ghost
Copy link

ghost commented Dec 7, 2015

FYI, item 3) from @joshbooker's comment above is now supported in Swashbuckle.OData. See rbeauchamp/Swashbuckle.OData#8

@xuzhg
Copy link
Member Author

xuzhg commented May 10, 2016

Closed it. Please file new issues if you find any problem related to the swagger support. Thanks.

@xuzhg xuzhg closed this May 10, 2016
@xuzhg xuzhg removed the in-progress label May 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants