diff --git a/CHANGELOG.md b/CHANGELOG.md index cb77ce9..7bfda58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# Changes in 7.0.5 +- Added: `RemoveUnusedQuerySchemas` option (default: `true`) to control cleanup of + container type schemas for `[FromQuery]`/`[AsParameters]` types (Issue #180) + # Changes in 7.0.4 - Fixed: `[AsParameters]` types in minimal API and `[FromQuery]` container types create unused schemas in `components/schemas` (Issue #180) - Added: Support for keyed DI services (Issue #165) diff --git a/docs/adr/ADR-002-configurable-schema-cleanup.md b/docs/adr/ADR-002-configurable-schema-cleanup.md new file mode 100644 index 0000000..03f3c72 --- /dev/null +++ b/docs/adr/ADR-002-configurable-schema-cleanup.md @@ -0,0 +1,114 @@ +# ADR-002: Configurable Schema Cleanup for Query Parameter Container Types + +**Status:** Accepted +**Date:** 2026-02-26 +**Issue:** [#180 (comment)](https://github.com/micro-elements/MicroElements.Swashbuckle.FluentValidation/issues/180#issuecomment-3968741044) +**Milestone:** v7.0.5 + +--- + +## 1. Context and Problem + +### Background + +In v7.0.4, a fix for issue #180 removed side-effect schemas from `components/schemas` created when the library processes `[FromQuery]`/`[AsParameters]` container types. `SwashbuckleSchemaProvider.GetSchemaForType()` calls `GenerateSchema()` which registers schemas in `SchemaRepository` as a side-effect. Since Swashbuckle expands these types into individual query parameters (not `$ref` schemas), the registered schemas are unreferenced and pollute the OpenAPI document. + +The fix takes a snapshot of existing schema IDs before processing and removes any newly-created schemas afterward. This exists in: + +- `FluentValidationOperationFilter.ApplyRulesToParameters()` +- `FluentValidationDocumentFilter.Apply()` + +### The Regression + +User @thunderstatic reports v7.0.4 breaks a workflow that worked since ~5.7.0: + +1. Custom FluentValidation rules (e.g. `IsOneOfString`) constrain `string[]` properties +2. MicroElements writes `enum` on the schema in `components/schemas` +3. A custom `IDocumentFilter` copies enum values from `components/schemas` onto query parameters +4. With v7.0.4, source schemas are removed before the custom DocumentFilter runs + +### Tension + +Two legitimate, conflicting needs: +- **Original #180 reporters**: clean `components/schemas` without unreferenced container type schemas +- **@thunderstatic's workflow**: schemas must remain for downstream DocumentFilters to consume + +--- + +## 2. Options Considered + +### Option A: Boolean Flag on `SchemaGenerationOptions` (CHOSEN) + +Add `RemoveUnusedQuerySchemas` property (default: `true`) to the core `ISchemaGenerationOptions`/`SchemaGenerationOptions`. + +**Pros:** Simple, consistent with existing patterns, accessible from both filters +**Cons:** Technically Swashbuckle-specific concern in core package + +### Option B: Flag on `RegistrationOptions` (Swashbuckle-specific) + +Add to `RegistrationOptions` which is only consumed at DI registration. + +**Pros:** Correct layer placement +**Cons:** `RegistrationOptions` is not injected into filters; requires significant plumbing to pass through + +### Option C: Predicate/Callback for Per-Schema Decisions + +`Func? ShouldRemoveSchema` + +**Pros:** Maximum flexibility +**Cons:** Over-engineered; no user has requested per-schema granularity + +### Option D: Reference-Counting (Remove Only Truly Unreferenced) + +Scan entire document post-processing, remove only schemas with zero `$ref` references. + +**Pros:** Semantically correct +**Cons:** Does not solve the problem (user's DocumentFilter has not run yet, so schemas appear unreferenced); expensive; fragile + +--- + +## 3. Decision: Option A + +**Rationale:** +1. A single boolean with sensible default is simplest to implement, document, and use +2. `SchemaGenerationOptions` is already the single options class both filters read; alternatives add plumbing for no benefit +3. `SchemaGenerationOptions` already contains Swashbuckle-leaning properties (e.g. `SchemaIdSelector`) +4. Default `true` preserves v7.0.4 behavior; opt-in `false` restores compatibility +5. NSwag and AspNetCore.OpenApi packages simply ignore the property + +**User-facing API:** +```csharp +services.AddFluentValidationRulesToSwagger(options => +{ + options.RemoveUnusedQuerySchemas = false; +}); +``` + +--- + +## 4. Implementation + +### Changes Made + +1. **`ISchemaGenerationOptions`** - Added `bool RemoveUnusedQuerySchemas` property +2. **`SchemaGenerationOptions`** - Added implementation with default `true` +3. **`SchemaGenerationOptionsExtensions.SetFrom()`** - Copies the new property +4. **`FluentValidationOperationFilter.ApplyRulesToParameters()`** - Snapshot and cleanup are conditional on the flag +5. **`FluentValidationDocumentFilter.Apply()`** - Snapshot and cleanup are conditional on the flag +6. **Tests** - Added `Default_RemoveUnusedQuerySchemas_Should_Be_True` and `OperationFilter_Should_Preserve_Schemas_When_RemoveUnusedQuerySchemas_Is_False` + +--- + +## 5. Consequences + +### Positive +- Users who depend on container type schemas for custom DocumentFilters can opt out of cleanup +- Default behavior remains clean (v7.0.4 fix preserved) +- Minimal implementation complexity + +### Negative +- Swashbuckle-specific concern leaks into the core `ISchemaGenerationOptions` interface +- NSwag/AspNetCore.OpenApi packages expose a property they don't use + +### Risks +- None significant; the property is additive and backward-compatible diff --git a/src/MicroElements.OpenApi.FluentValidation/FluentValidation/ISchemaGenerationOptions.cs b/src/MicroElements.OpenApi.FluentValidation/FluentValidation/ISchemaGenerationOptions.cs index 708e938..02bba54 100644 --- a/src/MicroElements.OpenApi.FluentValidation/FluentValidation/ISchemaGenerationOptions.cs +++ b/src/MicroElements.OpenApi.FluentValidation/FluentValidation/ISchemaGenerationOptions.cs @@ -53,6 +53,13 @@ public interface ISchemaGenerationOptions /// Gets filter. /// ICondition? RuleComponentFilter { get; } + + /// + /// Gets a value indicating whether schemas created as side-effects of processing + /// [FromQuery]/[AsParameters] container types should be removed from components/schemas. + /// Default: true. Set to false to preserve these schemas for use in custom DocumentFilters. + /// + bool RemoveUnusedQuerySchemas { get; } } /// @@ -93,6 +100,9 @@ public class SchemaGenerationOptions : ISchemaGenerationOptions /// public ICondition? RuleComponentFilter { get; set; } + /// + public bool RemoveUnusedQuerySchemas { get; set; } = true; + /// /// Sets values that compatible with FluentValidation. /// diff --git a/src/MicroElements.OpenApi.FluentValidation/FluentValidation/SchemaGenerationOptionsExtensions.cs b/src/MicroElements.OpenApi.FluentValidation/FluentValidation/SchemaGenerationOptionsExtensions.cs index 9590d45..6191970 100644 --- a/src/MicroElements.OpenApi.FluentValidation/FluentValidation/SchemaGenerationOptionsExtensions.cs +++ b/src/MicroElements.OpenApi.FluentValidation/FluentValidation/SchemaGenerationOptionsExtensions.cs @@ -47,6 +47,7 @@ public static SchemaGenerationOptions SetFrom(this SchemaGenerationOptions optio options.ValidatorFilter = other.ValidatorFilter; options.RuleFilter = other.RuleFilter; options.RuleComponentFilter = other.RuleComponentFilter; + options.RemoveUnusedQuerySchemas = other.RemoveUnusedQuerySchemas; return options; } } diff --git a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationDocumentFilter.cs b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationDocumentFilter.cs index 8e06ab5..94161ef 100644 --- a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationDocumentFilter.cs +++ b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationDocumentFilter.cs @@ -82,7 +82,9 @@ public void Apply(OpenApiDocument swaggerDoc, DocumentFilterContext context) // GetSchemaForType() has a side-effect of registering schemas in SchemaRepository. // For [AsParameters]/[FromQuery] container types, Swashbuckle does NOT create schemas // (it expands them into individual parameters), so any schemas we create are unused. - var existingSchemaIds = new HashSet(schemaRepositorySchemas.Keys); + HashSet? existingSchemaIds = _schemaGenerationOptions.RemoveUnusedQuerySchemas + ? new HashSet(schemaRepositorySchemas.Keys) + : null; var schemaProvider = new SwashbuckleSchemaProvider(context.SchemaRepository, context.SchemaGenerator, schemaIdSelector); var apiDescriptions = context.ApiDescriptions.ToArray(); @@ -227,13 +229,16 @@ IEnumerable GetParameters() // Issue #180: Remove schemas that we created as a side-effect of GetSchemaForType(). // These schemas were not created by Swashbuckle and are not referenced elsewhere. - var schemasToRemove = schemaRepositorySchemas.Keys - .Where(key => !existingSchemaIds.Contains(key)) - .ToList(); - - foreach (var schemaId in schemasToRemove) + if (existingSchemaIds != null) { - schemaRepositorySchemas.Remove(schemaId); + var schemasToRemove = schemaRepositorySchemas.Keys + .Where(key => !existingSchemaIds.Contains(key)) + .ToList(); + + foreach (var schemaId in schemasToRemove) + { + schemaRepositorySchemas.Remove(schemaId); + } } } diff --git a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs index d3c1cc1..7b7f8f3 100644 --- a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs +++ b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs @@ -104,7 +104,9 @@ private void ApplyRulesToParameters(OpenApiOperation operation, OperationFilterC // GetSchemaForType() has a side-effect of registering schemas in SchemaRepository. // For [AsParameters]/[FromQuery] container types, Swashbuckle does NOT create schemas // (it expands them into individual parameters), so any schemas we create are unused. - var existingSchemaIds = new HashSet(context.SchemaRepository.Schemas.Keys); + HashSet? existingSchemaIds = _schemaGenerationOptions.RemoveUnusedQuerySchemas + ? new HashSet(context.SchemaRepository.Schemas.Keys) + : null; foreach (var operationParameter in operation.Parameters!) { @@ -220,11 +222,14 @@ private void ApplyRulesToParameters(OpenApiOperation operation, OperationFilterC // Issue #180: Remove schemas that we created as a side-effect of GetSchemaForType(). // These schemas were not created by Swashbuckle and are not referenced elsewhere. - foreach (var schemaId in context.SchemaRepository.Schemas.Keys.ToArray()) + if (existingSchemaIds != null) { - if (!existingSchemaIds.Contains(schemaId)) + foreach (var schemaId in context.SchemaRepository.Schemas.Keys.ToArray()) { - context.SchemaRepository.Schemas.Remove(schemaId); + if (!existingSchemaIds.Contains(schemaId)) + { + context.SchemaRepository.Schemas.Remove(schemaId); + } } } } diff --git a/test/MicroElements.Swashbuckle.FluentValidation.Tests/AsParametersTests.cs b/test/MicroElements.Swashbuckle.FluentValidation.Tests/AsParametersTests.cs index 97ad3ae..6a87d52 100644 --- a/test/MicroElements.Swashbuckle.FluentValidation.Tests/AsParametersTests.cs +++ b/test/MicroElements.Swashbuckle.FluentValidation.Tests/AsParametersTests.cs @@ -109,6 +109,17 @@ public void Schema_Cleanup_Should_Remove_Only_Newly_Created_Schemas() schemaRepository.Schemas.Count.Should().Be(originalCount); } + /// + /// Verifies that the default value of RemoveUnusedQuerySchemas is true. + /// + [Fact] + public void Default_RemoveUnusedQuerySchemas_Should_Be_True() + { + var options = new SchemaGenerationOptions(); + options.RemoveUnusedQuerySchemas.Should().BeTrue( + because: "the default should preserve v7.0.4 behavior of cleaning up unused schemas"); + } + // OperationFilter integration tests require framework-specific OpenApi types. // Use #if to handle Swashbuckle v8/v9 vs v10 (OPENAPI_V2) differences. #if !OPENAPI_V2 @@ -316,6 +327,240 @@ public void OperationFilter_Should_Preserve_PreExisting_Schemas() schemaRepository.Schemas.Should().ContainKey("SearchRequest", because: "schemas that existed before OperationFilter processing should be preserved"); } + + /// + /// Verifies that when RemoveUnusedQuerySchemas is false, container type schemas + /// created as a side-effect of GetSchemaForType() are preserved in SchemaRepository. + /// This supports workflows where custom DocumentFilters consume these schemas. + /// + [Fact] + public void OperationFilter_Should_Preserve_Schemas_When_RemoveUnusedQuerySchemas_Is_False() + { + // Arrange + var schemaGeneratorOptions = new SchemaGeneratorOptions(); + var schemaRepository = new SchemaRepository(); + var schemaGenerator = SchemaGenerator(new SearchRequestValidator()); + + var schemaGenerationOptions = new SchemaGenerationOptions + { + NameResolver = new SystemTextJsonNameResolver(), + SchemaIdSelector = schemaGeneratorOptions.SchemaIdSelector, + RemoveUnusedQuerySchemas = false, + }; + + var validatorRegistry = new ValidatorRegistry( + new IValidator[] { new SearchRequestValidator() }, + new OptionsWrapper(schemaGenerationOptions)); + + var metadataProvider = new EmptyModelMetadataProvider(); + var queryMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Query)); + var pageMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Page)); + + var apiDescription = new ApiDescription(); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Query", + ModelMetadata = queryMetadata, + Source = BindingSource.Query, + }); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Page", + ModelMetadata = pageMetadata, + Source = BindingSource.Query, + }); + + var operation = new OpenApiOperation + { + Parameters = new List + { + new OpenApiParameter { Name = "Query", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "string" } }, + new OpenApiParameter { Name = "Page", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "integer" } }, + }, + }; + + var context = new OperationFilterContext( + apiDescription, + schemaGenerator, + schemaRepository, + typeof(AsParametersTests).GetMethod(nameof(OperationFilter_Should_Preserve_Schemas_When_RemoveUnusedQuerySchemas_Is_False))!); + + schemaRepository.Schemas.Should().BeEmpty(); + + // Act + var operationFilter = new FluentValidationOperationFilter( + validatorRegistry: validatorRegistry, + schemaGenerationOptions: new OptionsWrapper(schemaGenerationOptions)); + + operationFilter.Apply(operation, context); + + // Assert: SchemaRepository SHOULD contain SearchRequest schema (not cleaned up) + schemaRepository.Schemas.Should().ContainKey("SearchRequest", + because: "RemoveUnusedQuerySchemas is false, so container type schemas should be preserved for custom DocumentFilters"); + } + + /// + /// Verifies that when RemoveUnusedQuerySchemas is false, the DocumentFilter preserves + /// container type schemas created as a side-effect of GetSchemaForType(). + /// This is the key scenario from the regression: custom DocumentFilters that run after + /// this filter depend on these schemas being present. + /// + [Fact] + public void DocumentFilter_Should_Preserve_Schemas_When_RemoveUnusedQuerySchemas_Is_False() + { + // Arrange + var schemaGeneratorOptions = new SchemaGeneratorOptions(); + var schemaRepository = new SchemaRepository(); + var schemaGenerator = SchemaGenerator(new SearchRequestValidator()); + + var schemaGenerationOptions = new SchemaGenerationOptions + { + NameResolver = new SystemTextJsonNameResolver(), + SchemaIdSelector = schemaGeneratorOptions.SchemaIdSelector, + RemoveUnusedQuerySchemas = false, + }; + + var validatorRegistry = new ValidatorRegistry( + new IValidator[] { new SearchRequestValidator() }, + new OptionsWrapper(schemaGenerationOptions)); + + var metadataProvider = new EmptyModelMetadataProvider(); + var queryMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Query)); + var pageMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Page)); + + var apiDescription = new ApiDescription { RelativePath = "api/search" }; + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Query", + ModelMetadata = queryMetadata, + Source = BindingSource.Query, + }); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Page", + ModelMetadata = pageMetadata, + Source = BindingSource.Query, + }); + + var swaggerDoc = new OpenApiDocument + { + Paths = new OpenApiPaths + { + ["/api/search"] = new OpenApiPathItem + { + Operations = new Dictionary + { + [OperationType.Get] = new OpenApiOperation + { + Parameters = new List + { + new OpenApiParameter { Name = "Query", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "string" } }, + new OpenApiParameter { Name = "Page", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "integer" } }, + }, + }, + }, + }, + }, + }; + + var documentFilterContext = new DocumentFilterContext( + new[] { apiDescription }, + schemaGenerator, + schemaRepository); + + schemaRepository.Schemas.Should().BeEmpty(); + + // Act + var documentFilter = new FluentValidationDocumentFilter( + validatorRegistry: validatorRegistry, + schemaGenerationOptions: new OptionsWrapper(schemaGenerationOptions)); + + documentFilter.Apply(swaggerDoc, documentFilterContext); + + // Assert: SchemaRepository SHOULD contain SearchRequest schema (not cleaned up) + schemaRepository.Schemas.Should().ContainKey("SearchRequest", + because: "RemoveUnusedQuerySchemas is false, so container type schemas should be preserved for custom DocumentFilters"); + } + + /// + /// Verifies that with the default RemoveUnusedQuerySchemas=true, the DocumentFilter + /// removes container type schemas created as a side-effect. + /// + [Fact] + public void DocumentFilter_Should_Cleanup_ContainerType_Schemas_By_Default() + { + // Arrange + var schemaGeneratorOptions = new SchemaGeneratorOptions(); + var schemaRepository = new SchemaRepository(); + var schemaGenerator = SchemaGenerator(new SearchRequestValidator()); + + var schemaGenerationOptions = new SchemaGenerationOptions + { + NameResolver = new SystemTextJsonNameResolver(), + SchemaIdSelector = schemaGeneratorOptions.SchemaIdSelector, + }; + + var validatorRegistry = new ValidatorRegistry( + new IValidator[] { new SearchRequestValidator() }, + new OptionsWrapper(schemaGenerationOptions)); + + var metadataProvider = new EmptyModelMetadataProvider(); + var queryMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Query)); + var pageMetadata = metadataProvider.GetMetadataForProperty(typeof(SearchRequest), nameof(SearchRequest.Page)); + + var apiDescription = new ApiDescription { RelativePath = "api/search" }; + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Query", + ModelMetadata = queryMetadata, + Source = BindingSource.Query, + }); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Page", + ModelMetadata = pageMetadata, + Source = BindingSource.Query, + }); + + var swaggerDoc = new OpenApiDocument + { + Paths = new OpenApiPaths + { + ["/api/search"] = new OpenApiPathItem + { + Operations = new Dictionary + { + [OperationType.Get] = new OpenApiOperation + { + Parameters = new List + { + new OpenApiParameter { Name = "Query", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "string" } }, + new OpenApiParameter { Name = "Page", In = ParameterLocation.Query, Schema = new OpenApiSchema { Type = "integer" } }, + }, + }, + }, + }, + }, + }; + + var documentFilterContext = new DocumentFilterContext( + new[] { apiDescription }, + schemaGenerator, + schemaRepository); + + schemaRepository.Schemas.Should().BeEmpty(); + + // Act + var documentFilter = new FluentValidationDocumentFilter( + validatorRegistry: validatorRegistry, + schemaGenerationOptions: new OptionsWrapper(schemaGenerationOptions)); + + documentFilter.Apply(swaggerDoc, documentFilterContext); + + // Assert: SchemaRepository should NOT contain SearchRequest schema (cleaned up) + schemaRepository.Schemas.Should().NotContainKey("SearchRequest", + because: "container type schemas created as a side-effect should be cleaned up by default (Issue #180)"); + } #endif } } diff --git a/version.props b/version.props index 4b86969..00f2f48 100644 --- a/version.props +++ b/version.props @@ -1,6 +1,6 @@ - 7.0.4 + 7.0.5