Browse Source
* Improve swagger OperationIDs: Part 1 * Fix tests and fmt * Improve docs and add more tests * Fmt * Improve Swagger OperationIDs for Auth * Fix review feedback * Use generic getcustomattributes * Format * replace swaggerexclude by split+obsolete * Format * Some remaining excludespull/6210/head
Daniel García
4 months ago
committed by
GitHub
22 changed files with 583 additions and 55 deletions
@ -0,0 +1,25 @@ |
|||||||
|
using System.Text.Json; |
||||||
|
using Microsoft.OpenApi.Any; |
||||||
|
using Microsoft.OpenApi.Models; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace Bit.SharedWeb.Swagger; |
||||||
|
|
||||||
|
/// <summary> |
||||||
|
/// Adds the action name (function name) as an extension to each operation in the Swagger document. |
||||||
|
/// This can be useful for the code generation process, to generate more meaningful names for operations. |
||||||
|
/// Note that we add both the original action name and a snake_case version, as the codegen templates |
||||||
|
/// cannot do case conversions. |
||||||
|
/// </summary> |
||||||
|
public class ActionNameOperationFilter : IOperationFilter |
||||||
|
{ |
||||||
|
public void Apply(OpenApiOperation operation, OperationFilterContext context) |
||||||
|
{ |
||||||
|
if (!context.ApiDescription.ActionDescriptor.RouteValues.TryGetValue("action", out var action)) return; |
||||||
|
if (string.IsNullOrEmpty(action)) return; |
||||||
|
|
||||||
|
operation.Extensions.Add("x-action-name", new OpenApiString(action)); |
||||||
|
// We can't do case changes in the codegen templates, so we also add the snake_case version of the action name |
||||||
|
operation.Extensions.Add("x-action-name-snake-case", new OpenApiString(JsonNamingPolicy.SnakeCaseLower.ConvertName(action))); |
||||||
|
} |
||||||
|
} |
||||||
@ -0,0 +1,80 @@ |
|||||||
|
using Microsoft.OpenApi.Models; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace Bit.SharedWeb.Swagger; |
||||||
|
|
||||||
|
/// <summary> |
||||||
|
/// Checks for duplicate operation IDs in the Swagger document, and throws an error if any are found. |
||||||
|
/// Operation IDs must be unique across the entire Swagger document according to the OpenAPI specification, |
||||||
|
/// but we use controller action names to generate them, which can lead to duplicates if a Controller function |
||||||
|
/// has multiple HTTP methods or if a Controller has overloaded functions. |
||||||
|
/// </summary> |
||||||
|
public class CheckDuplicateOperationIdsDocumentFilter(bool printDuplicates = true) : IDocumentFilter |
||||||
|
{ |
||||||
|
public bool PrintDuplicates { get; } = printDuplicates; |
||||||
|
|
||||||
|
public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context) |
||||||
|
{ |
||||||
|
var operationIdMap = new Dictionary<string, List<(string Path, OpenApiPathItem PathItem, OperationType Method, OpenApiOperation Operation)>>(); |
||||||
|
|
||||||
|
foreach (var (path, pathItem) in swaggerDoc.Paths) |
||||||
|
{ |
||||||
|
foreach (var operation in pathItem.Operations) |
||||||
|
{ |
||||||
|
if (!operationIdMap.TryGetValue(operation.Value.OperationId, out var list)) |
||||||
|
{ |
||||||
|
list = []; |
||||||
|
operationIdMap[operation.Value.OperationId] = list; |
||||||
|
} |
||||||
|
|
||||||
|
list.Add((path, pathItem, operation.Key, operation.Value)); |
||||||
|
|
||||||
|
} |
||||||
|
} |
||||||
|
|
||||||
|
// Find duplicates |
||||||
|
var duplicates = operationIdMap.Where((kvp) => kvp.Value.Count > 1).ToList(); |
||||||
|
if (duplicates.Count > 0) |
||||||
|
{ |
||||||
|
if (PrintDuplicates) |
||||||
|
{ |
||||||
|
Console.WriteLine($"\n######## Duplicate operationIds found in the schema ({duplicates.Count} found) ########\n"); |
||||||
|
|
||||||
|
Console.WriteLine("## Common causes of duplicate operation IDs:"); |
||||||
|
Console.WriteLine("- Multiple HTTP methods (GET, POST, etc.) on the same controller function"); |
||||||
|
Console.WriteLine(" Solution: Split the methods into separate functions, and if appropiate, mark the deprecated ones with [Obsolete]"); |
||||||
|
Console.WriteLine(); |
||||||
|
Console.WriteLine("- Overloaded controller functions with the same name"); |
||||||
|
Console.WriteLine(" Solution: Rename the overloaded functions to have unique names, or combine them into a single function with optional parameters"); |
||||||
|
Console.WriteLine(); |
||||||
|
|
||||||
|
Console.WriteLine("## The duplicate operation IDs are:"); |
||||||
|
|
||||||
|
foreach (var (operationId, duplicate) in duplicates) |
||||||
|
{ |
||||||
|
Console.WriteLine($"- operationId: {operationId}"); |
||||||
|
foreach (var (path, pathItem, method, operation) in duplicate) |
||||||
|
{ |
||||||
|
Console.Write($" {method.ToString().ToUpper()} {path}"); |
||||||
|
|
||||||
|
|
||||||
|
if (operation.Extensions.TryGetValue("x-source-file", out var sourceFile) && operation.Extensions.TryGetValue("x-source-line", out var sourceLine)) |
||||||
|
{ |
||||||
|
var sourceFileString = ((Microsoft.OpenApi.Any.OpenApiString)sourceFile).Value; |
||||||
|
var sourceLineString = ((Microsoft.OpenApi.Any.OpenApiInteger)sourceLine).Value; |
||||||
|
|
||||||
|
Console.WriteLine($" {sourceFileString}:{sourceLineString}"); |
||||||
|
} |
||||||
|
else |
||||||
|
{ |
||||||
|
Console.WriteLine(); |
||||||
|
} |
||||||
|
} |
||||||
|
Console.WriteLine("\n"); |
||||||
|
} |
||||||
|
} |
||||||
|
|
||||||
|
throw new InvalidOperationException($"Duplicate operation IDs found in Swagger schema"); |
||||||
|
} |
||||||
|
} |
||||||
|
} |
||||||
@ -0,0 +1,33 @@ |
|||||||
|
using Microsoft.AspNetCore.Hosting; |
||||||
|
using Microsoft.Extensions.DependencyInjection; |
||||||
|
using Microsoft.Extensions.Hosting; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace Bit.SharedWeb.Swagger; |
||||||
|
|
||||||
|
public static class SwaggerGenOptionsExt |
||||||
|
{ |
||||||
|
|
||||||
|
public static void InitializeSwaggerFilters( |
||||||
|
this SwaggerGenOptions config, IWebHostEnvironment environment) |
||||||
|
{ |
||||||
|
config.SchemaFilter<EnumSchemaFilter>(); |
||||||
|
config.SchemaFilter<EncryptedStringSchemaFilter>(); |
||||||
|
|
||||||
|
config.OperationFilter<ActionNameOperationFilter>(); |
||||||
|
|
||||||
|
// Set the operation ID to the name of the controller followed by the name of the function. |
||||||
|
// Note that the "Controller" suffix for the controllers, and the "Async" suffix for the actions |
||||||
|
// are removed already, so we don't need to do that ourselves. |
||||||
|
// TODO(Dani): This is disabled until we remove all the duplicate operation IDs. |
||||||
|
// config.CustomOperationIds(e => $"{e.ActionDescriptor.RouteValues["controller"]}_{e.ActionDescriptor.RouteValues["action"]}"); |
||||||
|
// config.DocumentFilter<CheckDuplicateOperationIdsDocumentFilter>(); |
||||||
|
|
||||||
|
// These two filters require debug symbols/git, so only add them in development mode |
||||||
|
if (environment.IsDevelopment()) |
||||||
|
{ |
||||||
|
config.DocumentFilter<GitCommitDocumentFilter>(); |
||||||
|
config.OperationFilter<SourceFileLineOperationFilter>(); |
||||||
|
} |
||||||
|
} |
||||||
|
} |
||||||
@ -0,0 +1,67 @@ |
|||||||
|
using Bit.SharedWeb.Swagger; |
||||||
|
using Microsoft.AspNetCore.Mvc.Abstractions; |
||||||
|
using Microsoft.AspNetCore.Mvc.ApiExplorer; |
||||||
|
using Microsoft.OpenApi.Any; |
||||||
|
using Microsoft.OpenApi.Models; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace SharedWeb.Test; |
||||||
|
|
||||||
|
public class ActionNameOperationFilterTest |
||||||
|
{ |
||||||
|
[Fact] |
||||||
|
public void WithValidActionNameAddsActionNameExtensions() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var operation = new OpenApiOperation(); |
||||||
|
var actionDescriptor = new ActionDescriptor(); |
||||||
|
actionDescriptor.RouteValues["action"] = "GetUsers"; |
||||||
|
|
||||||
|
var apiDescription = new ApiDescription |
||||||
|
{ |
||||||
|
ActionDescriptor = actionDescriptor |
||||||
|
}; |
||||||
|
|
||||||
|
var context = new OperationFilterContext(apiDescription, null, null, null); |
||||||
|
var filter = new ActionNameOperationFilter(); |
||||||
|
|
||||||
|
// Act |
||||||
|
filter.Apply(operation, context); |
||||||
|
|
||||||
|
// Assert |
||||||
|
Assert.True(operation.Extensions.ContainsKey("x-action-name")); |
||||||
|
Assert.True(operation.Extensions.ContainsKey("x-action-name-snake-case")); |
||||||
|
|
||||||
|
var actionNameExt = operation.Extensions["x-action-name"] as OpenApiString; |
||||||
|
var actionNameSnakeCaseExt = operation.Extensions["x-action-name-snake-case"] as OpenApiString; |
||||||
|
|
||||||
|
Assert.NotNull(actionNameExt); |
||||||
|
Assert.NotNull(actionNameSnakeCaseExt); |
||||||
|
Assert.Equal("GetUsers", actionNameExt.Value); |
||||||
|
Assert.Equal("get_users", actionNameSnakeCaseExt.Value); |
||||||
|
} |
||||||
|
|
||||||
|
[Fact] |
||||||
|
public void WithMissingActionRouteValueDoesNotAddExtensions() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var operation = new OpenApiOperation(); |
||||||
|
var actionDescriptor = new ActionDescriptor(); |
||||||
|
// Not setting the "action" route value at all |
||||||
|
|
||||||
|
var apiDescription = new ApiDescription |
||||||
|
{ |
||||||
|
ActionDescriptor = actionDescriptor |
||||||
|
}; |
||||||
|
|
||||||
|
var context = new OperationFilterContext(apiDescription, null, null, null); |
||||||
|
var filter = new ActionNameOperationFilter(); |
||||||
|
|
||||||
|
// Act |
||||||
|
filter.Apply(operation, context); |
||||||
|
|
||||||
|
// Assert |
||||||
|
Assert.False(operation.Extensions.ContainsKey("x-action-name")); |
||||||
|
Assert.False(operation.Extensions.ContainsKey("x-action-name-snake-case")); |
||||||
|
} |
||||||
|
} |
||||||
@ -0,0 +1,84 @@ |
|||||||
|
using Bit.SharedWeb.Swagger; |
||||||
|
using Microsoft.AspNetCore.Mvc; |
||||||
|
using Microsoft.OpenApi.Models; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace SharedWeb.Test; |
||||||
|
|
||||||
|
public class UniqueOperationIdsController : ControllerBase |
||||||
|
{ |
||||||
|
[HttpGet("unique-get")] |
||||||
|
public void UniqueGetAction() { } |
||||||
|
|
||||||
|
[HttpPost("unique-post")] |
||||||
|
public void UniquePostAction() { } |
||||||
|
} |
||||||
|
|
||||||
|
public class OverloadedOperationIdsController : ControllerBase |
||||||
|
{ |
||||||
|
[HttpPut("another-duplicate")] |
||||||
|
public void AnotherDuplicateAction() { } |
||||||
|
|
||||||
|
[HttpPatch("another-duplicate/{id}")] |
||||||
|
public void AnotherDuplicateAction(int id) { } |
||||||
|
} |
||||||
|
|
||||||
|
public class MultipleHttpMethodsController : ControllerBase |
||||||
|
{ |
||||||
|
[HttpGet("multi-method")] |
||||||
|
[HttpPost("multi-method")] |
||||||
|
[HttpPut("multi-method")] |
||||||
|
public void MultiMethodAction() { } |
||||||
|
} |
||||||
|
|
||||||
|
public class CheckDuplicateOperationIdsDocumentFilterTest |
||||||
|
{ |
||||||
|
[Fact] |
||||||
|
public void UniqueOperationIdsDoNotThrowException() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var (swaggerDoc, context) = SwaggerDocUtil.CreateDocFromControllers(typeof(UniqueOperationIdsController)); |
||||||
|
var filter = new CheckDuplicateOperationIdsDocumentFilter(); |
||||||
|
filter.Apply(swaggerDoc, context); |
||||||
|
// Act & Assert |
||||||
|
var exception = Record.Exception(() => filter.Apply(swaggerDoc, context)); |
||||||
|
Assert.Null(exception); |
||||||
|
} |
||||||
|
|
||||||
|
[Fact] |
||||||
|
public void DuplicateOperationIdsThrowInvalidOperationException() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var (swaggerDoc, context) = SwaggerDocUtil.CreateDocFromControllers(typeof(OverloadedOperationIdsController)); |
||||||
|
var filter = new CheckDuplicateOperationIdsDocumentFilter(false); |
||||||
|
|
||||||
|
// Act & Assert |
||||||
|
var exception = Assert.Throws<InvalidOperationException>(() => filter.Apply(swaggerDoc, context)); |
||||||
|
Assert.Contains("Duplicate operation IDs found in Swagger schema", exception.Message); |
||||||
|
} |
||||||
|
|
||||||
|
[Fact] |
||||||
|
public void MultipleHttpMethodsThrowInvalidOperationException() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var (swaggerDoc, context) = SwaggerDocUtil.CreateDocFromControllers(typeof(MultipleHttpMethodsController)); |
||||||
|
var filter = new CheckDuplicateOperationIdsDocumentFilter(false); |
||||||
|
|
||||||
|
// Act & Assert |
||||||
|
var exception = Assert.Throws<InvalidOperationException>(() => filter.Apply(swaggerDoc, context)); |
||||||
|
Assert.Contains("Duplicate operation IDs found in Swagger schema", exception.Message); |
||||||
|
} |
||||||
|
|
||||||
|
[Fact] |
||||||
|
public void EmptySwaggerDocDoesNotThrowException() |
||||||
|
{ |
||||||
|
// Arrange |
||||||
|
var swaggerDoc = new OpenApiDocument { Paths = [] }; |
||||||
|
var context = new DocumentFilterContext([], null, null); |
||||||
|
var filter = new CheckDuplicateOperationIdsDocumentFilter(false); |
||||||
|
|
||||||
|
// Act & Assert |
||||||
|
var exception = Record.Exception(() => filter.Apply(swaggerDoc, context)); |
||||||
|
Assert.Null(exception); |
||||||
|
} |
||||||
|
} |
||||||
@ -0,0 +1,85 @@ |
|||||||
|
using System.Reflection; |
||||||
|
using Microsoft.AspNetCore.Hosting; |
||||||
|
using Microsoft.AspNetCore.Mvc; |
||||||
|
using Microsoft.AspNetCore.Mvc.ApiExplorer; |
||||||
|
using Microsoft.AspNetCore.Mvc.ApplicationParts; |
||||||
|
using Microsoft.AspNetCore.Mvc.Controllers; |
||||||
|
using Microsoft.Extensions.DependencyInjection; |
||||||
|
using Microsoft.OpenApi.Models; |
||||||
|
using NSubstitute; |
||||||
|
using Swashbuckle.AspNetCore.Swagger; |
||||||
|
using Swashbuckle.AspNetCore.SwaggerGen; |
||||||
|
|
||||||
|
namespace SharedWeb.Test; |
||||||
|
|
||||||
|
public class SwaggerDocUtil |
||||||
|
{ |
||||||
|
/// <summary> |
||||||
|
/// Creates an OpenApiDocument and DocumentFilterContext from the specified controller type by setting up |
||||||
|
/// a minimal service collection and using the SwaggerProvider to generate the document. |
||||||
|
/// </summary> |
||||||
|
public static (OpenApiDocument, DocumentFilterContext) CreateDocFromControllers(params Type[] controllerTypes) |
||||||
|
{ |
||||||
|
if (controllerTypes.Length == 0) |
||||||
|
{ |
||||||
|
throw new ArgumentException("At least one controller type must be provided", nameof(controllerTypes)); |
||||||
|
} |
||||||
|
|
||||||
|
var services = new ServiceCollection(); |
||||||
|
services.AddLogging(); |
||||||
|
services.AddSingleton(Substitute.For<IWebHostEnvironment>()); |
||||||
|
services.AddControllers() |
||||||
|
.ConfigureApplicationPartManager(manager => |
||||||
|
{ |
||||||
|
// Clear existing parts and feature providers |
||||||
|
manager.ApplicationParts.Clear(); |
||||||
|
manager.FeatureProviders.Clear(); |
||||||
|
|
||||||
|
// Add a custom feature provider that only includes the specific controller types |
||||||
|
manager.FeatureProviders.Add(new MultipleControllerFeatureProvider(controllerTypes)); |
||||||
|
|
||||||
|
// Add assembly parts for all unique assemblies containing the controllers |
||||||
|
foreach (var assembly in controllerTypes.Select(t => t.Assembly).Distinct()) |
||||||
|
{ |
||||||
|
manager.ApplicationParts.Add(new AssemblyPart(assembly)); |
||||||
|
} |
||||||
|
}); |
||||||
|
services.AddSwaggerGen(config => |
||||||
|
{ |
||||||
|
config.SwaggerDoc("v1", new OpenApiInfo { Title = "Test API", Version = "v1" }); |
||||||
|
config.CustomOperationIds(e => $"{e.ActionDescriptor.RouteValues["controller"]}_{e.ActionDescriptor.RouteValues["action"]}"); |
||||||
|
}); |
||||||
|
var serviceProvider = services.BuildServiceProvider(); |
||||||
|
|
||||||
|
// Get API descriptions |
||||||
|
var allApiDescriptions = serviceProvider.GetRequiredService<IApiDescriptionGroupCollectionProvider>() |
||||||
|
.ApiDescriptionGroups.Items |
||||||
|
.SelectMany(group => group.Items) |
||||||
|
.ToList(); |
||||||
|
|
||||||
|
if (allApiDescriptions.Count == 0) |
||||||
|
{ |
||||||
|
throw new InvalidOperationException("No API descriptions found for controller, ensure your controllers are defined correctly (public, not nested, inherit from ControllerBase, etc.)"); |
||||||
|
} |
||||||
|
|
||||||
|
// Generate the swagger document and context |
||||||
|
var document = serviceProvider.GetRequiredService<ISwaggerProvider>().GetSwagger("v1"); |
||||||
|
var schemaGenerator = serviceProvider.GetRequiredService<ISchemaGenerator>(); |
||||||
|
var context = new DocumentFilterContext(allApiDescriptions, schemaGenerator, new SchemaRepository()); |
||||||
|
|
||||||
|
return (document, context); |
||||||
|
} |
||||||
|
} |
||||||
|
|
||||||
|
public class MultipleControllerFeatureProvider(params Type[] controllerTypes) : ControllerFeatureProvider |
||||||
|
{ |
||||||
|
private readonly HashSet<Type> _allowedControllerTypes = [.. controllerTypes]; |
||||||
|
|
||||||
|
protected override bool IsController(TypeInfo typeInfo) |
||||||
|
{ |
||||||
|
return _allowedControllerTypes.Contains(typeInfo.AsType()) |
||||||
|
&& typeInfo.IsClass |
||||||
|
&& !typeInfo.IsAbstract |
||||||
|
&& typeof(ControllerBase).IsAssignableFrom(typeInfo); |
||||||
|
} |
||||||
|
} |
||||||
Loading…
Reference in new issue