domaindrivendev / Swashbuckle.AspNetCore

Swagger tools for documenting API's built on ASP.NET Core
MIT License
5.17k stars 1.29k forks source link

Swagger Generator Does Not Use All API Explorer Metadata #412

Open commonsensesoftware opened 7 years ago

commonsensesoftware commented 7 years ago

Overview

The implementation of SwaggerGenerator.cs should consider all relevant metadata provided by API explorers. The following information is not currently mapped in all NonBodyParameter scenarios:

Scenarios

There are undoubtedly numerous scenarios where this woudl be useful. In the case of API Versioning, this will enable discovered API version parameters to be injected into Swagger documents and the UI via its API explorer extensions without requiring additional configuration from service authors. Service authors must currently use custom IOperationFilter implementations to bridge this gap.

steveoh commented 5 years ago

It seems the microsoft docs think the [Required] and [DefaultValue(true)] work with this. I'm noticing that they do not.

Maybe I am doing something wrong but I do not see default: anywhere in the json.

commonsensesoftware commented 5 years ago

This thread has gone quiet, but it's not dead. @domaindrivendev and I have talked it over. Ultimately, this is being held up by https://github.com/aspnet/Mvc/issues/7563, which demonstrates a gap in the API Explorer library. It has been updated, but not published. Once that happens, this thread can breathe new life.

CumpsD commented 5 years ago

It seems it happened: https://github.com/Microsoft/aspnet-api-versioning/issues/414

commonsensesoftware commented 5 years ago

Indeed this has happened in both the ASP.NET Core and API Versioning libraries. It does, however, require ASP.NET Core 2.2+.

@domaindrivendev, I tried reaching out by email before the holidays. Perhaps you've been busy, away, or my message fell into the Inbox Abyss. Regardless, we should rehash and discuss how this information should manifest in a Swagger document now that it's possible.

Once the details are hashed out about how this information should be used, I think a new PR can be created to close this out whether that's by me or some other eager community member. :)

bartlannoeye commented 5 years ago

What's the state of this issue?

lonix1 commented 5 years ago

Do we need to use the SwaggerDefaultValues class until this issue is closed?

commonsensesoftware commented 5 years ago

You don't have to. This class is providing bridge to fill in default values from the API Explorer. If you don't need that, things will work just fine.

In addition to Required, Description, and DefaultValue, it was recently brought to my attention that there is also a gap with respect to Deprecated. This is not specific to the API Explorer though as it does not provide this information. However, I do think it's worth discussing, even if it means creating a new issue, that a new hook (ex: callback) that can bridge the deprecated information from API Versioning to Swashbuckle would be useful. The most current version of SwaggerDefautlValues shows how to do this, but would not currently be addressed by this issue.

This issue may be moving slow due to the priority of other issues, but it's not forgotten. Keep pushing for it. It's going to happen - eventually. ;)

jstawski commented 4 years ago

Any news on this? I'm trying to write a DocumentFilter that defaults the version parameter automatically using DefaultValue and it doesn't seem to be working:

public class VersionDefaultingDocumentFilter : IDocumentFilter
{
    public void Apply(SwaggerDocument swaggerDoc, DocumentFilterContext context)
    {
        var version = swaggerDoc.Info.Version.Replace("v", "");
        foreach(var ad in context.ApiDescriptions)
        {
            var versionParam = ad.ParameterDescriptions.Where(pd => pd.Name == "version").FirstOrDefault();
            if (versionParam != null)
                versionParam.DefaultValue = version;
        }
    }
}
RehanSaeed commented 4 years ago

@commonsensesoftware The Versioning sample has a SwaggerDefaultValues IOperationFilter that references this issue.

Now that ASP.NET Core 3.0 is out, can this be resolved?

commonsensesoftware commented 4 years ago

@RehanSaeed does Swashbuckle support or otherwise use all of the relevant, provided metadata now? I was under the impression the answer is still no. If it does, then this issue could be resolved. The gap with respect to deprecation is interesting, but it's not based on metadata so that can be a new issue.

ericsampson commented 3 years ago

@commonsensesoftware is this fixable now? I have trouble tracking all the moving pieces across the various parts of the stack here, sorry :D I'd be happy to help if someone can point out what needs to be done now.

commonsensesoftware commented 3 years ago

@ericsampson it is fixable. There was a time when IsOptional was the wrong thing (it meant something different). There is now a IsRequired property on API descriptors. There is also a DefaultValue. These just aren't being consumed. There are two other issues that have cropped up as well. There is no good way to hook API Versioning to Swashbuckle for deprecated APIs. Swashbuckle would need to provide a hook to connect the two. API Versioning has a way to report this, but there's no way to marry the two without a filter. Additionally, a few people have mentioned that when versioning by media type, Swashbuckle does not honor the reported values. I was able to confirm that. The values appear to be hardcoded or somehow otherwise assumed. For example, when the reported media type should be application/json; v=2.0, Swashbuckle still reports application/json. Updating the values in a filter will force Swashbuckle to do the right thing.

martincostello commented 2 months ago

@commonsensesoftware This issue is soon going to celebrate it's 7th birthday 🎂 - is it still relevant to keep open?

commonsensesoftware commented 2 months ago

@martincostello Wow! Time flies. Sadly, as far as I can tell, it is still relevant - if not at least in part. In the ideal state, combining Swashbuckle and API Versioning would not require:

https://github.com/dotnet/aspnet-api-versioning/blob/main/examples/AspNetCore/WebApi/OpenApiExample/SwaggerDefaultValues.cs

or, at least, significantly less of it.

As we burn down the list and catch back up in 6.6.0 and beyond, I can re-create a PR that would address these and push it forward. There is no available bridge for deprecated between Swashbuckle and the API Explorer so we'll need to think about whether that's something worth addressing. I believe all of the other barriers are resolvable now.

elibroftw commented 2 months ago

Alternative solution: include API versioning from the get go in the Visual Studio template with the swagger integration "hack." There are many things I had to add on top of the basic template such as integration testing with xunit, swagger dark theme, hyphenated URLs, and good JSON defaults (such as camel case). Another thing that the asp.net web API temlalte should include is a password authentication endpoint with time based on time password. Like if we want a more secure web, it should be as easy to implement the preferred design versus the naive design. The perspective should be imagining if Mark Zuckerberg chose asp.net in 2008. With the current template, Facebook still would've stored passwords in plain text...

I'm sorry if this is the wrong place to comment, but I consider this issue resolved based on the example that showed us how to integrate API versioning into the swagger. The only thing wrong with that example is that it requires hard coding the API name, but that's an easy mitigation if it really matters to someone.

martincostello commented 2 months ago

The contents of the Visual Studio templates are controlled by the ASP.NET Core team, not us.

Further, Swashbuckle is being completely removed from the templates in .NET 9: https://github.com/dotnet/aspnetcore/issues/54599

If you have feedback on the ASP.NET Core templates that Visual Studio exposes, you should create an issue in the dotnet/aspnetcore repository.

commonsensesoftware commented 2 months ago

We could create templates for the CLI and possibly VS, but that would be a separate issue and feature request. That may be the happy bridge with Swashbuckle being removed from the OOB, default experience.