diff --git a/CHANGELOG.md b/CHANGELOG.md index 7922a5d..94e04a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +# Changes in 7.1.11 +- Fixed: a DTO shared between a flattened `[FromQuery]`/`[AsParameters]` binding and a request body (`[FromBody]`/`[FromForm]`) in the same document could lose its FluentValidation rules, and the emitted document could contain a `$ref` to a removed component (Issue #226, ADR-006). net10.0 target only + - Root cause: the Issue #180 cleanup left the container type "reserved-but-removed" in Swashbuckle's `SchemaRepository` (component removed, internal reserved-id kept). The 7.1.10 fix (#223) recovered the schema for *reading* constraint values, but a later `[FromBody]`/`[FromForm]` operation binding the same type made Swashbuckle emit a `$ref` to a component that no longer exists (confirmed empirically), and rules applied on the recovered throwaway instance never reached the document + - Fix (net10.0 / Swashbuckle 10.x): the Issue #180 cleanup now heals the repository state via `SchemaRepository.ReplaceSchemaId` (public API since Swashbuckle 10.1.0) before removing a side-effect schema — the reservation is cleared together with the component, so the next operation binding the same type regenerates a full component and the rules reach the real document object. `SwashbuckleSchemaProvider` tracks `Type → schemaId` for every `GetSchemaForType` call (including the Issue #209 ancestor walk) to make the healing possible + - net8.0/net9.0 keep the 7.1.10 behavior: they pin Swashbuckle 8.1.1 where `ReplaceSchemaId` does not exist, and bumping the minimum would force the Microsoft.OpenApi 2.x breaking change on consumers. The throwaway recovery from #223 remains as a safety net on all targets + - Design record: `docs/adr/ADR-006-shared-dto-state-healing-cleanup.md`. Thanks to @jgarciadelanoceda for suggesting the `ReplaceSchemaId` approach + # Changes in 7.1.10 - Fixed: the same `[FromQuery]`/`[AsParameters]` DTO shared by more than one endpoint lost its FluentValidation rules on every endpoint after the first (Issue #223) - `FluentValidationOperationFilter` runs once per operation. The Issue #180 cleanup removes the temporary container schema from `SchemaRepository.Schemas`, but Swashbuckle keeps the type in its internal reserved-ids map, which the cleanup does not touch. For the 2nd+ endpoint, `GetSchemaForType` → `GenerateSchema` then returns a bare `$ref` (no `Properties`) and the schema is no longer in `Schemas`, so the "has properties" guard was skipped and no rules were applied. Only reproduces with the default `RemoveUnusedQuerySchemas = true` diff --git a/docs/adr/ADR-006-shared-dto-state-healing-cleanup.md b/docs/adr/ADR-006-shared-dto-state-healing-cleanup.md new file mode 100644 index 0000000..f9c24dd --- /dev/null +++ b/docs/adr/ADR-006-shared-dto-state-healing-cleanup.md @@ -0,0 +1,181 @@ +# ADR-006: State-Healing Schema Cleanup for Shared [FromQuery] DTOs via SchemaRepository.ReplaceSchemaId + +**Status:** Implemented +**Date:** 2026-07-12 +**Issue:** [#226](https://github.com/micro-elements/MicroElements.Swashbuckle.FluentValidation/issues/226) (follow-up to [#223](https://github.com/micro-elements/MicroElements.Swashbuckle.FluentValidation/issues/223); approach suggested by @jgarciadelanoceda in [PR #224 discussion](https://github.com/micro-elements/MicroElements.Swashbuckle.FluentValidation/pull/224#issuecomment-4940080089)) +**Milestone:** v7.1.11 + +--- + +## 1. Context and Problem + +### Background + +Since Issue #180 (see [ADR-002](ADR-002-configurable-schema-cleanup.md)), `FluentValidationOperationFilter.ApplyRulesToParameters` removes component schemas that were created only as a side effect of our own `GetSchemaForType` calls for `[FromQuery]`/`[AsParameters]` container types (Swashbuckle expands those into individual parameters and never references the container component). The cleanup is controlled by `SchemaGenerationOptions.RemoveUnusedQuerySchemas` (default `true`) and runs **once per operation** (`FluentValidationOperationFilter.cs:253-264`). + +Issue #223 discovered that this cleanup leaves the `SchemaRepository` in a **reserved-but-removed** state: the schema is removed from `SchemaRepository.Schemas`, but Swashbuckle keeps the type in its internal `_reservedIds` map, which the cleanup cannot touch (no public API existed). In that state `ISchemaGenerator.GenerateSchema` returns a bare `$ref` (no `Properties`) without regenerating the component. PR #224 (v7.1.10) fixed the *parameters* path by recovering the concrete schema into a throwaway `SchemaRepository` (`SwashbuckleSchemaProvider.GetSchemaForType`, lines 69-86) — a read-only recovery that deliberately does not mutate the shared repository. + +### Remaining Problem (Issue #226) + +The throwaway recovery is only safe where the recovered schema is used for **reading** constraint values. Two gaps remain when the same DTO type is used **both** as a flattened `[FromQuery]`/`[AsParameters]` container **and** as a request body (`[FromBody]` or `[FromForm]`) in the same document, with `RemoveUnusedQuerySchemas = true` (the default) and the query operation processed **before** the body operation: + +1. **`[FromBody]` (JSON): dangling `$ref` in the emitted document.** The operation filter's request-body path does not process `application/json` at all — its content-type gate only admits form content types (`FluentValidationOperationFilter.cs:416-422`), so JSON bodies are covered by the *schema filter* (`FluentValidationRules`) at component-generation time. But in the reserved-but-removed state, when Swashbuckle generates the body operation it calls `GenerateSchema` for the body type, hits the reservation, and emits a `$ref` to a component that **no longer exists** — the schema filter is never re-invoked because the component is not regenerated. The result is an invalid document (dangling reference) with no FluentValidation rules for the body. + `NOTE: originally a hypothesis from reading Swashbuckle's SchemaRepository/SchemaGenerator source (the reserved-ids check precedes generation). CONFIRMED empirically on 2026-07-12 by the Phase 1 repro: SharedDtoMixedBindingTests fails pre-fix with SchemaRepository.Schemas empty while the type stays reserved — GenerateSchema returns a $ref to a component that does not exist.` + +2. **`[FromForm]`: rules applied to a disposable object.** For form content types, `ApplyRulesToRequestBody` resolves the container schema via `schemaProvider.GetSchemaForType(parameterType)` when `schemaRefId != null` (`FluentValidationOperationFilter.cs:464`) and then passes it to `FluentValidationSchemaBuilder.ApplyRulesToSchema` as the **mutation target** (lines 480-485) — there is no copy-back onto a separate document object. In the reserved-but-removed state, `GetSchemaForType` returns the detached throwaway-recovered instance (the #224 fallback), so all applied rules are written to an object the document never sees and are silently lost. + +### Why the #224 fix cannot close this + +The #224 recovery is intentionally isolated: it never mutates the shared repository, so it cannot restore the missing component or clear the stale reservation. Fixing the body scenarios requires the shared `SchemaRepository` state itself to be consistent after cleanup. + +### The new tool: `SchemaRepository.ReplaceSchemaId` + +Swashbuckle.AspNetCore **10.1.0** added a public `SchemaRepository.ReplaceSchemaId(Type schemaType, string replacementSchemaId)` ([PR #3708](https://github.com/domaindrivendev/Swashbuckle.AspNetCore/pull/3708)). It moves the schema to a new key in `Schemas` **and removes the type from the private `_reservedIds` map** — exactly the half of the state that public API could not reach before. It returns `false` when the schema is not present under the old id, so it must be called **before** the schema is removed. + +### Version matrix + +| TFM | Swashbuckle.AspNetCore.SwaggerGen | `ReplaceSchemaId` available | +|-----|-----------------------------------|------------------------------| +| net10.0 (`OPENAPI_V2`) | 10.2.1 | Yes (since 10.1.0) | +| net8.0, net9.0 | 8.1.1 | No | + +Bumping the net8.0/net9.0 minimum to 10.1.0+ is not acceptable: Swashbuckle 9+ moves to Microsoft.OpenApi 2.x, a large breaking change for consumers. + +## 2. Options Considered + +### Option A: State-healing cleanup via `ReplaceSchemaId` on net10.0 (CHOSEN) + +During `ApplyRulesToParameters`, track the container `Type` for every `GetSchemaForType` call made while `RemoveUnusedQuerySchemas` is active. In the Issue #180 cleanup, for each schema id that did not exist before the operation and whose `Type` is known, call `ReplaceSchemaId(type, tempId)` (which clears the reservation) and then remove the renamed entry. Ids without a tracked type (components for nested complex property types, created transitively by `GenerateSchema`) keep the current plain removal. + +**Pros:** +- Fixes the root cause: the reserved-but-removed state never arises for container types, so subsequent operations (query, body, or response) regenerate a full component and the schema filter re-applies rules to a real document object. +- Closes both #226 scenarios on net10.0, including the dangling-`$ref` document-validity hazard. +- Public API only — no reflection, no behavior dependent on Swashbuckle internals. +- Removes the repeated throwaway regeneration cost noted in the PR #224 review (the recovered-state branch stops firing for tracked types). + +**Cons:** +- net10.0-only (`OPENAPI_V2`); net8.0/net9.0 keep the #224 behavior (see Option B for why reflection was rejected). +- Components created transitively inside Swashbuckle's `GenerateSchema` (nested property types whose `Type` never passes through library code) still leave stale reservations; the #224 throwaway fallback must therefore stay as a safety net on all TFMs. +- Requires an operation-scoped `Type → schemaId` tracking map, best recorded inside `SwashbuckleSchemaProvider` itself so that **every** `GetSchemaForType` call site participates — including the #209 ancestor-requiredness walk (`IsPropertyRequiredInType`, `FluentValidationOperationFilter.cs:358`), which also registers ancestor container schemas that the cleanup removes. + +### Option B: Reflection-based healing of `_reservedIds` on net8.0/net9.0 + +Mirror what the author of Swashbuckle issue #3707 did before the API existed: after removing the schema, also remove the type from the private `_reservedIds` dictionary via reflection. + +**Pros:** +- Closes the gap uniformly on all TFMs. + +**Cons:** +- Depends on a private field name and shape across all Swashbuckle 8.x patch versions the floating consumer graph may resolve; silently breaks (reverting to the current behavior at best) if the field changes. +- The maintainer explicitly rejected reflection for this fix (assumption list, 2026-07-12): net8.0/net9.0 stay on the #224 throwaway fallback. + +**REJECTED** — fragility outweighs the benefit; the affected mixed-binding scenario is narrow and the #224 fallback keeps the primary #223 case fixed on those TFMs. + +### Option C: Promote `FluentValidationDocumentFilter` (Variant B from #223) + +Make the experimental document filter (`RegistrationOptions.ExperimentalUseDocumentFilter`, default `false`) production-ready and the default pipeline. It batches all types/parameters across the whole document and performs the #180 cleanup once at the end (`FluentValidationDocumentFilter.cs:253-265`), so per-operation desync cannot occur. + +**Pros:** +- Structural fix for the whole class of per-operation state bugs, uniformly across TFMs. +- Scenario 1 (JSON dangling `$ref`) does not arise under the document filter even today: its cleanup runs once at the end of the document (`FluentValidationDocumentFilter.cs:253-265`), after Swashbuckle has registered body components (protected by the `existingSchemaIds` snapshot); its type-level loop (lines 92-111) collects `[FromBody]` component types, and the subsequent rules loop (lines 164-201) applies rules to those schemas. + +**Cons:** +- The document filter never touches `operation.RequestBody`: scenario 2 (`[FromForm]`) and the #216 `encoding.contentType` parity would require substantial new code. +- Large parity checklist before promotion: required marking (#209), request bodies + `[FromForm]` + `encoding.contentType` (#216), `FindParam` inspects only the first operation of a path (`FluentValidationDocumentFilter.cs:152-162`), dead duplicate `schemasForParameters` assignment (lines 113-119 overwritten at 150). +- A default-pipeline swap is a behavior change requiring its own ADR and migration plan; it does not help the default (operation-filter) pipeline that all current consumers run. + +**REJECTED for this ADR** — remains the long-term structural direction, tracked separately. + +### Option D: Do nothing beyond #224 + +**Pros:** +- Zero risk; #223 (the reported, common case) is already fixed. + +**Cons:** +- The dangling-`$ref` hazard and the `[FromForm]` rule loss remain reachable with default settings (`RemoveUnusedQuerySchemas = true`) whenever a DTO is shared between query and body bindings. + +**REJECTED** — an emitted-document validity hazard with default settings is worth closing where the public API allows it. + +## 3. Decision: Option A + +Implement state-healing cleanup in `FluentValidationOperationFilter` on the net10.0 (`OPENAPI_V2`) target using `SchemaRepository.ReplaceSchemaId`, keeping the #224 throwaway recovery as a safety net for net8.0/net9.0 and for transitively created nested components. + +**Rationale:** + +1. Prevention beats compensation: with the reservation cleared, every downstream consumer of the type (our filter, Swashbuckle's own body/response generation, the schema filter) sees a normal repository and behaves correctly — no path-by-path patching. +2. Public API only: the fix cannot silently break on a Swashbuckle patch bump; on the one TFM where the API exists, we use it; where it does not, we do not degrade anything. +3. Small blast radius: the change is confined to the cleanup block and an operation-scoped tracking collection; the shared core, NSwag, and AspNetCore.OpenApi packages are untouched (their pipelines have no `SchemaRepository`/reserved-ids mechanic). + +**Cleanup mechanics (net10.0):** + +```csharp +// SwashbuckleSchemaProvider records Type -> schemaId for every GetSchemaForType call +// (covers the parameters loop AND the #209 ancestor walk in IsPropertyRequiredInType): +containerTypesForCleanup[type] = schemaId; + +// Issue #180 cleanup, per newly created schemaId: +#if OPENAPI_V2 +if (typeForSchemaId is not null) +{ + var tempId = "__fv_removed_" + Guid.NewGuid().ToString("N"); + if (context.SchemaRepository.ReplaceSchemaId(typeForSchemaId, tempId)) // clears the reservation + { + context.SchemaRepository.Schemas.Remove(tempId); + continue; + } +} +#endif +context.SchemaRepository.Schemas.Remove(schemaId); // fallback: current behavior (nested types, net8/net9) +``` + +On net8.0/net9.0 the cleanup body is unchanged; the #224 fallback in `SwashbuckleSchemaProvider.GetSchemaForType` continues to recover readable schemas for the parameters path. + +## 4. Implementation + +Affected package: **`MicroElements.Swashbuckle.FluentValidation` only.** + +### Phase 1 — Reproduction tests (investigation) + +New test file: `test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoMixedBindingTests.cs`. + +1. `OPENAPI_V2` (net10.0) test: shared DTO bound as `[FromQuery]` on operation 1 and `[FromBody]` (JSON) on operation 2, one shared `SchemaRepository`/`SchemaGenerator`, `RemoveUnusedQuerySchemas = true`. Assert: after both operations, the component schema for the type **exists** in `SchemaRepository.Schemas` (no dangling `$ref`) and carries FluentValidation constraints. This test confirms (or refutes) the dangling-`$ref` hypothesis from Context — if refuted, the ADR scope shrinks to the `[FromForm]` scenario and the Context section must be corrected. +2. Same-shape test with `[FromForm]` (`multipart/form-data`): assert the request-body schema the document references carries the applied rules. +3. Port the #223 regression scenario to `OPENAPI_V2` — extend `test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoAcrossEndpointsTest.cs` with an `OPENAPI_V2`-compatible variant (its current test body is `#if !OPENAPI_V2`), so the net10.0 target where the chosen state-healing cleanup (Option A) applies gets shared-DTO coverage. + +### Phase 2 — State-healing cleanup + +4. `SwashbuckleSchemaProvider`: record an operation-scoped `Type → schemaId` entry for every `GetSchemaForType` call (the provider already computes the schema id per call). This covers **all** call sites — the parameters loop and the #209 ancestor-requiredness walk in `IsPropertyRequiredInType` (`FluentValidationOperationFilter.cs:358`). The #223 throwaway fallback stays unchanged as safety net. +5. `FluentValidationOperationFilter.ApplyRulesToParameters` cleanup block: under `#if OPENAPI_V2`, apply the `ReplaceSchemaId` + remove dance from Decision for every newly created id with a tracked `Type`; keep plain removal for untracked ids and non-`OPENAPI_V2` builds. + +### Phase 3 — Verification + +6. Full matrix run: net8.0/net9.0 (behavior unchanged — all existing tests green), net10.0 (new tests green, existing 96 green). +7. Manual check with `samples/SampleWebApi` (`Controllers/Issue80.cs` already binds the same `BestShot` type as `[FromBody]` and `[FromQuery]`, and already ships `Issue80.Validator : AbstractValidator` with `MinimumLength(5)` on `Link` — use it as-is to observe the constraints). Note: `SampleWebApi` targets net9.0, so it exercises the #224 throwaway fallback, not the net10.0 healing; the healing itself is covered by the `OPENAPI_V2` tests above. + +## 5. Testing + +- **New tests** (see Phase 1): mixed `[FromQuery]`+`[FromBody]`, mixed `[FromQuery]`+`[FromForm]`, and the `OPENAPI_V2` port of the #223 shared-DTO regression. All follow the `SharedDtoAcrossEndpointsTest` pattern: one shared `SchemaRepository`/`SchemaGenerator` driven through multiple operations, matching the real Swagger pipeline. +- **Why existing tests didn't catch this:** no test in the repository binds the same DTO type through two different binding sources in one document (confirmed by search, 2026-07-12); and the only shared-DTO test (`SharedDtoAcrossEndpointsTest`) is excluded on `OPENAPI_V2` — exactly the target where `ReplaceSchemaId` lives. +- **Sample:** `samples/SampleWebApi`, `Controllers/Issue80.cs` (`PostBestShot([FromBody] BestShot)` + `PostBestShot2([FromQuery] BestShot)`). + +## 6. Consequences + +### Positive + +- On net10.0, the `SchemaRepository` is left consistent after every operation: no reserved-but-removed state, no dangling `$ref` in emitted documents, FluentValidation rules reach body-bound shared DTOs. +- The #224 review's performance note is addressed for tracked types (no repeated throwaway regeneration). +- No public API changes; no reflection; other packages untouched. + +### Negative + +- Behavior diverges by TFM: net10.0 is fully healed, net8.0/net9.0 keep the narrower #224 guarantees (parameters path only). This must be stated in the README compatibility notes. +- Components created transitively inside Swashbuckle's `GenerateSchema` still leave stale reservations on all TFMs (nested property types whose `Type` never passes through library code — everything that does pass through `GetSchemaForType`, including the #209 ancestor walk, is tracked and healed); the throwaway fallback continues to cover reads, but body-bound shared types of that transitive shape remain theoretically affected on net10.0 as well. `NOTE: no known report of this shape; revisit if one appears.` + +### Risks + +- If the Phase 1 `[FromBody]` repro refutes the dangling-`$ref` hypothesis (e.g. Swashbuckle regenerates the component in some path not covered by source reading), the Context section must be corrected and the ADR re-reviewed — the `[FromForm]` scenario and the state-healing rationale stand regardless. +- `ReplaceSchemaId` returns `false` when the target id already exists in `Schemas`; the GUID-based temp id makes a collision practically impossible, and the `false` branch safely degrades to the current plain removal. + +### Semver impact + +Patch release (**v7.1.11**): bug fix, no public API surface change, default behavior becomes strictly more correct. diff --git a/docs/reviews/ADR-006-iteration-log.md b/docs/reviews/ADR-006-iteration-log.md new file mode 100644 index 0000000..34a16e6 --- /dev/null +++ b/docs/reviews/ADR-006-iteration-log.md @@ -0,0 +1,39 @@ +# ADR-006 Iteration Log + +ADR: `docs/adr/ADR-006-shared-dto-state-healing-cleanup.md` +Driven by: `/adr-autonomous` (issue #226) + +## Iteration 0 — Recon + assumptions (2026-07-12) + +- Step −1 (Explore recon) findings that shaped the draft: + - `ApplyRulesToRequestBody` skips `application/json` entirely (content-type gate `FluentValidationOperationFilter.cs:416-422`) — JSON bodies are owned by the schema filter, so the #226 `[FromBody]` failure mode is a **dangling `$ref`** (component removed, reservation prevents regeneration), not a lost in-place mutation. + - `[FromForm]` path mutates `resolvedSchema` in place with no copy-back (`FluentValidationOperationFilter.cs:464,480-485`) — throwaway-recovered instance loses rules. + - `FluentValidationDocumentFilter` (Variant B) does not process request bodies at all today → cannot close #226 as-is. + - No mixed `[FromQuery]`+`[FromBody]` test exists; `SharedDtoAcrossEndpointsTest` is `#if !OPENAPI_V2` (no net10.0 shared-DTO coverage). + - Parity: NSwag / AspNetCore.OpenApi pipelines have no `SchemaRepository`/reserved-ids — Swashbuckle-package-local change. + - Repro seed: `samples/SampleWebApi/Controllers/Issue80.cs` (`BestShot` bound as both `[FromBody]` and `[FromQuery]`). +- Step 0 assumptions confirmed by user (2026-07-12): Swashbuckle package only; Variant C on net10.0/`OPENAPI_V2` only; **no reflection** on net8/net9; milestone **v7.1.11** (patch); out of scope: Variant B promotion, Swashbuckle minimum bump, NSwag/AspNetCore.OpenApi parity. + +## Iteration 1 — Draft v1 (2026-07-12) + +- Wrote ADR-006 v1: Context (two failure modes, dangling-`$ref` marked as hypothesis requiring Phase 1 repro), 4 options (A CHOSEN: `ReplaceSchemaId` state-healing on net10.0; B reflection REJECTED per user; C Variant B REJECTED for this ADR; D do-nothing REJECTED), Decision with cleanup mechanics sketch, 3-phase Implementation (repro tests → healing → matrix verification), Testing (why existing tests missed it), Consequences (TFM divergence, nested-type residual gap, semver patch). +- `/review-adr` iteration 1 result: **blocker 0, important 1, nit 2** (raw 4 findings from 3 validators; 1 duplicate deduped). Validator 1 (dotnet-architect) returned zero findings and independently corroborated `ReplaceSchemaId` presence by decompiling cached Swashbuckle DLLs (present in 10.1.5/10.2.1, absent in 10.0.0/8.1.1). + +## Iteration 2 — Revise after review 1 (2026-07-12) + +- Fixed **important** (Validator 2, rubric 5): Option C's first con overstated the gap — the document filter's end-of-document cleanup already prevents scenario 1 (JSON dangling `$ref`), and its type-level loop covers `[FromBody]` component schemas. Moved that fact to Option C **Pros**, narrowed the con to `operation.RequestBody`/`[FromForm]`/#216 encoding. +- Fixed **nit** (V2+V3, deduped): `Issue80.Validator : AbstractValidator` already exists in the sample — Phase 3 item 7 now says "use it as-is" instead of "add a validator". (Applied inline instead of deferring — trivial.) +- Fixed **nit** (V3): named the planned test artifacts — new `SharedDtoMixedBindingTests.cs`, #223 `OPENAPI_V2` port via extending `SharedDtoAcrossEndpointsTest.cs`. (Applied inline instead of deferring — trivial.) +- `/review-adr` iteration 2 result: **blocker 0, important 0, nit 3** → decision gate PASSED. Validator 2 re-verified the corrected Option C wording against `FluentValidationDocumentFilter.cs` (type selection 92-96/105-111, rules loop 164-201, snapshot 85-87, cleanup 253-265) — holds. + +## Iteration 2 findings (all nit, applied inline — trivial and they sharpen implementation guidance) + +- **[V1, rubric 1]** Option C Pros cited lines 92-111 for "applies rules", but that range only collects types; rule application is at 164-201. → Fixed: both ranges cited with correct verbs. +- **[V2, rubric 5]** Tracking scope was ambiguous: the #209 ancestor walk (`IsPropertyRequiredInType`, `FluentValidationOperationFilter.cs:358`) also calls `GetSchemaForType` with a fully known `Type`, so tracking only the parameters-loop call site would leave healable reservations stale. → Fixed: tracking moved into `SwashbuckleSchemaProvider` (covers every call site); Decision sketch, Phase 2 steps 4-5, Option A cons, and the Consequences-Negative bullet narrowed to "types that never pass through library code". +- **[V3, rubric 7]** "Variant C" used in Phase 1 item 3 but never defined in the ADR (its own Option C is the rejected document filter). → Fixed: replaced with "the chosen state-healing cleanup (Option A)". + +## Exit (2026-07-12) + +- Iterations used: **2 / 5**. Status: **PASS** (blocker 0, important 0 at iteration 2; the 3 iteration-2 nits were applied inline after the gate). +- Deferred: none. +- Next step: `/adr-implement` for `docs/adr/ADR-006-shared-dto-state-healing-cleanup.md` (Phase 1 repro tests first — they confirm or refute the marked dangling-`$ref` hypothesis). diff --git a/docs/reviews/ADR-006-review.md b/docs/reviews/ADR-006-review.md new file mode 100644 index 0000000..bc96c79 --- /dev/null +++ b/docs/reviews/ADR-006-review.md @@ -0,0 +1,52 @@ +# ADR-006 Review (2026-07-12) + +**Final verdict (iteration 2): Approve** — blocker 0, important 0; 3 nits found in iteration 2 were applied inline (see `ADR-006-iteration-log.md`). + +--- + +## Iteration 2 — re-validation after fixes + +- Validator 1 (dotnet-architect): 1 nit — Option C Pros cited lines 92-111 for rule application (collection only; rules applied at 164-201). Fixed. +- Validator 2 (domain & parity): 1 nit — tracking scope ambiguous vs the #209 ancestor walk (`IsPropertyRequiredInType:358` also calls `GetSchemaForType` with a known `Type`). Fixed: tracking moved into `SwashbuckleSchemaProvider`, covering every call site. Re-verified the corrected Option C wording against code — holds. +- Validator 3 (structure): 1 nit — "Variant C" undefined inside the ADR. Fixed: replaced with in-ADR terminology (Option A). + +--- + +# Iteration 1 (initial review) + +ADR: `docs/adr/ADR-006-shared-dto-state-healing-cleanup.md` +Validators: 1 — `dotnet-artisan:dotnet-architect` (architecture/versions/semver), 2 — general-purpose (domain & parity), 3 — general-purpose (structure & completeness). + +## Rubric summary + +| # | Rubric item | Verdict | +|---|-------------|---------| +| 1 | Architecture & trade-offs (≥2 options, CHOSEN, layering) | ✅ 4 options, layering respected, core untouched | +| 2 | Generator parity | ✅ Swashbuckle-only justified; NSwag/AspNetCore.OpenApi verified to have no SchemaRepository mechanic | +| 3 | TFM/version matrix | ✅ All claims verified against code; `ReplaceSchemaId` corroborated by DLL decompilation (10.1.5/10.2.1 have it, 10.0.0/8.1.1 do not); the one hypothesis (dangling `$ref`) properly marked `requires investigation` | +| 4 | Public API & semver | ✅ No API change, patch v7.1.11 consistent | +| 5 | OpenAPI semantics | ⚠️ 1 important: Option C con overstated (document filter does cover JSON `[FromBody]` via type-level loop; scenario 1 doesn't arise under it) — **fixed in iteration 2** | +| 6 | Testing | ⚠️ 2 nits: stale "add a validator" premise (Issue80.Validator exists); planned tests not named — **both fixed in iteration 2** | +| 7 | Structure & format | ✅ English, header block, ADR-002/003 section style, REJECTED markers | + +## 🔴 Blocker (0) + +— + +## 🟡 Important (1) + +1. **[Validator 2, rubric 5, Section 2 Option C]** "The document filter does not process request bodies at all — would not fix #226 without substantial new code" overstates: `FluentValidationDocumentFilter`'s type-level loop (lines 92-111) covers `[FromBody]` component schemas and its end-of-document cleanup (253-265) prevents scenario 1; only scenario 2 (`[FromForm]`/encoding) needs substantial new code. → **Fixed**: fact moved to Option C Pros, con narrowed to `operation.RequestBody`/`[FromForm]`/#216. + +## 🟢 Nit (2) + +1. **[V2+V3 dedup, Phase 3 item 7]** `Issue80.Validator : AbstractValidator` (MinimumLength(5) on Link) already exists in the sample; "add a BestShot validator" was a stale premise. → **Fixed** ("use it as-is"). +2. **[V3, Phase 1 / Testing]** Planned tests had no file/class names, unlike ADR-002/003 precedent. → **Fixed**: `SharedDtoMixedBindingTests.cs` (new), #223 `OPENAPI_V2` port via `SharedDtoAcrossEndpointsTest.cs`. + +## Recommendation + +**Approve with fixes** (fixes applied in iteration 2; re-validation results — see the iteration log). + +## Validator statistics + +- Raw findings: V1 = 0, V2 = 2 (1 important, 1 nit), V3 = 2 (2 nits) → total 4. +- After dedup (`rubric_item`+`location`): 3 (1 important, 2 nits) — the Issue80-validator nit was reported by both V2 and V3. diff --git a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs index a6615af..e2ff68c 100644 --- a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs +++ b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/FluentValidationOperationFilter.cs @@ -258,6 +258,24 @@ private void ApplyRulesToParameters(OpenApiOperation operation, OperationFilterC { if (!existingSchemaIds.Contains(schemaId)) { +#if OPENAPI_V2 + // Issue #226: clear Swashbuckle's internal reserved-id together with the removal, so the + // repository stays consistent and a later operation binding the same type (query, body or + // response) regenerates a full component instead of emitting a $ref to a removed one. + // ReplaceSchemaId (Swashbuckle 10.1.0+) is the only public API that reaches the reserved-id + // map and it must run BEFORE the removal; the temp GUID key cannot collide with a real id. + var trackedType = schemaProvider.RequestedSchemaIds + .FirstOrDefault(pair => pair.Value == schemaId).Key; + if (trackedType != null) + { + var tempSchemaId = "__fv_removed_" + Guid.NewGuid().ToString("N"); + if (context.SchemaRepository.ReplaceSchemaId(trackedType, tempSchemaId)) + { + context.SchemaRepository.Schemas.Remove(tempSchemaId); + continue; + } + } +#endif context.SchemaRepository.Schemas.Remove(schemaId); } } diff --git a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/SwashbuckleSchemaProvider.cs b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/SwashbuckleSchemaProvider.cs index 4bf3b65..0569e11 100644 --- a/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/SwashbuckleSchemaProvider.cs +++ b/src/MicroElements.Swashbuckle.FluentValidation/Swashbuckle/SwashbuckleSchemaProvider.cs @@ -2,6 +2,9 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +#if OPENAPI_V2 +using System.Collections.Generic; +#endif using MicroElements.OpenApi.FluentValidation; #if !OPENAPI_V2 using Microsoft.OpenApi.Models; @@ -19,6 +22,18 @@ public class SwashbuckleSchemaProvider : ISchemaProvider private readonly ISchemaGenerator _schemaGenerator; private readonly Func _schemaIdSelector; +#if OPENAPI_V2 + private readonly Dictionary _requestedSchemaIds = new Dictionary(); + + /// + /// Gets the types passed to mapped to their schema ids. + /// The Issue #180 cleanup uses this map to heal Swashbuckle's internal reserved-id state via + /// SchemaRepository.ReplaceSchemaId (Swashbuckle 10.1.0+) before removing a schema (Issue #226). + /// Covers every call site: the parameters loop and the Issue #209 ancestor-requiredness walk. + /// + internal IReadOnlyDictionary RequestedSchemaIds => _requestedSchemaIds; +#endif + /// /// Initializes a new instance of the class. /// @@ -41,6 +56,8 @@ public OpenApiSchema GetSchemaForType(Type type) var schemaId = _schemaIdSelector(type); #if OPENAPI_V2 + _requestedSchemaIds[type] = schemaId; + if (!_schemaRepository.Schemas.TryGetValue(schemaId, out var schemaInterface)) { schemaInterface = _schemaGenerator.GenerateSchema(type, _schemaRepository); diff --git a/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoAcrossEndpointsTest.cs b/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoAcrossEndpointsTest.cs index 938b557..17f86a8 100644 --- a/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoAcrossEndpointsTest.cs +++ b/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoAcrossEndpointsTest.cs @@ -128,5 +128,87 @@ private static OpenApiSchema ApplyToEndpoint( return nameParamSchema; } #endif + +// OPENAPI_V2 (net10.0) port of the same scenario — ADR-006 Phase 1: the net10.0 target where the +// state-healing cleanup (Issue #226) lives previously had no shared-DTO coverage at all. +#if OPENAPI_V2 + [Fact] + public void OperationFilter_Should_Apply_Rules_To_Every_Endpoint_Sharing_The_Same_FromQuery_Dto() + { + // Arrange — a SINGLE SchemaRepository + SchemaGenerator shared across both operations, + // exactly like the real Swagger pipeline where SchemaRepository persists for the whole document. + var schemaGeneratorOptions = new SchemaGeneratorOptions(); + var schemaRepository = new SchemaRepository(); + var schemaGenerator = SchemaGenerator(new HelloRequestValidator()); + + var schemaGenerationOptions = new SchemaGenerationOptions + { + NameResolver = new SystemTextJsonNameResolver(), + SchemaIdSelector = schemaGeneratorOptions.SchemaIdSelector, + RemoveUnusedQuerySchemas = true, // the default; this is the flag that triggers the bug + }; + + var validatorRegistry = new ValidatorRegistry( + new IValidator[] { new HelloRequestValidator() }, + new OptionsWrapper(schemaGenerationOptions)); + + var operationFilter = new FluentValidationOperationFilter( + validatorRegistry: validatorRegistry, + schemaGenerationOptions: new OptionsWrapper(schemaGenerationOptions)); + + // Act — two endpoints ("/api/hello" and "/api/hello2") binding the same HelloRequest via [FromQuery]. + var firstName = ApplyToEndpoint(operationFilter, schemaGenerator, schemaRepository); + var secondName = ApplyToEndpoint(operationFilter, schemaGenerator, schemaRepository); + + // Assert — every endpoint binding the shared HelloRequest must expose the rules (Issue #223). + firstName.MinLength.Should().Be(1, because: "the first endpoint has always worked"); + firstName.MaxLength.Should().Be(10, because: "the first endpoint has always worked"); + secondName.MinLength.Should().Be(1, + because: "every endpoint binding the shared HelloRequest must expose NotEmpty (Issue #223)"); + secondName.MaxLength.Should().Be(10, + because: "every endpoint binding the shared HelloRequest must expose MaximumLength(10) (Issue #223)"); + } + + /// + /// Runs the for one GET endpoint that binds + /// via [FromQuery], and returns the resulting "Name" parameter schema. + /// + private static OpenApiSchema ApplyToEndpoint( + FluentValidationOperationFilter operationFilter, + SchemaGenerator schemaGenerator, + SchemaRepository schemaRepository) + { + var metadataProvider = new EmptyModelMetadataProvider(); + var nameMetadata = metadataProvider.GetMetadataForProperty(typeof(HelloRequest), nameof(HelloRequest.Name)); + + var apiDescription = new ApiDescription(); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Name", + ModelMetadata = nameMetadata, + Source = BindingSource.Query, + }); + + var nameParamSchema = new OpenApiSchema { Type = JsonSchemaType.String }; + var operation = new OpenApiOperation + { + Parameters = new List + { + new OpenApiParameter { Name = "Name", In = ParameterLocation.Query, Schema = nameParamSchema }, + }, + }; + + var context = new OperationFilterContext( + apiDescription, + schemaGenerator, + schemaRepository, + new OpenApiDocument(), + typeof(SharedDtoAcrossEndpointsTest).GetMethod(nameof(OperationFilter_Should_Apply_Rules_To_Every_Endpoint_Sharing_The_Same_FromQuery_Dto))!); + + operationFilter.Apply(operation, context); + + return nameParamSchema; + } +#endif } } diff --git a/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoMixedBindingTests.cs b/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoMixedBindingTests.cs new file mode 100644 index 0000000..59380f3 --- /dev/null +++ b/test/MicroElements.Swashbuckle.FluentValidation.Tests/SharedDtoMixedBindingTests.cs @@ -0,0 +1,193 @@ +// Copyright (c) MicroElements. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +// Issue #226 requires SchemaRepository.ReplaceSchemaId (Swashbuckle 10.1.0+, net10.0/OPENAPI_V2 target only). +#if OPENAPI_V2 +using System.Collections.Generic; +using FluentAssertions; +using FluentValidation; +using MicroElements.OpenApi.FluentValidation; +using MicroElements.Swashbuckle.FluentValidation.Generation; +using Microsoft.AspNetCore.Mvc.ApiExplorer; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Options; +using Microsoft.OpenApi; +using Swashbuckle.AspNetCore.SwaggerGen; +using Xunit; + +namespace MicroElements.Swashbuckle.FluentValidation.Tests +{ + /// + /// Issue #226 (ADR-006): when the same DTO is bound as a flattened [FromQuery] container by one endpoint + /// and as a request body ([FromBody]/[FromForm]) by another, the Issue #180 cleanup after the query + /// operation left the type "reserved-but-removed" in Swashbuckle's . + /// Generating the body operation then produced a $ref to a component that no longer exists (a dangling + /// reference) and the FluentValidation rules never reached the emitted document. + /// + /// The fix (ADR-006, net10.0/OPENAPI_V2 only) heals the repository state during the cleanup via + /// SchemaRepository.ReplaceSchemaId (Swashbuckle 10.1.0+): the reservation is cleared together with the + /// component removal, so the body operation regenerates a full component and the FluentValidationRules + /// schema filter re-applies the rules to the real document object. + /// https://github.com/micro-elements/MicroElements.Swashbuckle.FluentValidation/issues/226 + /// + public class SharedDtoMixedBindingTests : UnitTestBase + { + public class HelloRequest + { + public string? Name { get; set; } + } + + public class HelloRequestValidator : AbstractValidator + { + public HelloRequestValidator() + { + RuleFor(x => x.Name).NotEmpty().MaximumLength(10); + } + } + + [Fact] + public void JsonBody_Component_Should_Exist_And_Carry_Rules_After_Query_Operation_Cleanup() + { + var (operationFilter, schemaGenerator, schemaRepository) = CreateSharedPipeline(); + + // Operation 1: [FromQuery] HelloRequest — the Issue #180 cleanup removes the side-effect component. + ApplyToQueryEndpoint(operationFilter, schemaGenerator, schemaRepository); + + // Operation 2: Swashbuckle generates the [FromBody] (JSON) request body for the SAME type. + var bodySchema = schemaGenerator.GenerateSchema(typeof(HelloRequest), schemaRepository); + + // The emitted $ref must resolve to an existing component (no dangling reference). + bodySchema.GetRefId().Should().Be("HelloRequest"); + schemaRepository.Schemas.Should().ContainKey( + "HelloRequest", + because: "a $ref emitted into the request body must resolve to an existing component (Issue #226)"); + + // ...and the component must carry the FluentValidation constraints. + var component = schemaRepository.GetSchema("HelloRequest"); + var nameProperty = component.GetProperty("Name", schemaRepository); + nameProperty.Should().NotBeNull(); + nameProperty!.MinLength.Should().Be(1, because: "NotEmpty must reach the body-bound shared DTO (Issue #226)"); + nameProperty.MaxLength.Should().Be(10, because: "MaximumLength(10) must reach the body-bound shared DTO (Issue #226)"); + } + + [Fact] + public void FormBody_Referenced_Component_Should_Exist_And_Carry_Rules_After_Query_Operation_Cleanup() + { + var (operationFilter, schemaGenerator, schemaRepository) = CreateSharedPipeline(); + + // Operation 1: [FromQuery] HelloRequest — the Issue #180 cleanup removes the side-effect component. + ApplyToQueryEndpoint(operationFilter, schemaGenerator, schemaRepository); + + // Operation 2: [FromForm] — Swashbuckle generates the multipart body schema for the same type. + var bodySchema = schemaGenerator.GenerateSchema(typeof(HelloRequest), schemaRepository); + + var metadataProvider = new EmptyModelMetadataProvider(); + var apiDescription = new ApiDescription(); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "form", + ModelMetadata = metadataProvider.GetMetadataForType(typeof(HelloRequest)), + Source = BindingSource.Form, + }); + + var operation = new OpenApiOperation + { + RequestBody = new OpenApiRequestBody + { + Content = new Dictionary + { + ["multipart/form-data"] = new OpenApiMediaType { Schema = bodySchema }, + }, + }, + }; + + var context = new OperationFilterContext( + apiDescription, + schemaGenerator, + schemaRepository, + new OpenApiDocument(), + typeof(SharedDtoMixedBindingTests).GetMethod(nameof(FormBody_Referenced_Component_Should_Exist_And_Carry_Rules_After_Query_Operation_Cleanup))!); + + operationFilter.Apply(operation, context); + + // The multipart body's $ref must resolve to an existing component carrying the rules. + schemaRepository.Schemas.Should().ContainKey( + "HelloRequest", + because: "the form body's $ref must resolve to an existing component (Issue #226)"); + + var component = schemaRepository.GetSchema("HelloRequest"); + var nameProperty = component.GetProperty("Name", schemaRepository); + nameProperty.Should().NotBeNull(); + nameProperty!.MinLength.Should().Be(1, because: "NotEmpty must reach the form-bound shared DTO (Issue #226)"); + nameProperty.MaxLength.Should().Be(10, because: "MaximumLength(10) must reach the form-bound shared DTO (Issue #226)"); + } + + /// + /// One SchemaRepository + SchemaGenerator (with the FluentValidationRules schema filter) shared across + /// operations, exactly like the real Swagger pipeline where SchemaRepository persists for the whole document. + /// + private (FluentValidationOperationFilter OperationFilter, SchemaGenerator SchemaGenerator, SchemaRepository SchemaRepository) CreateSharedPipeline() + { + var schemaGeneratorOptions = new SchemaGeneratorOptions(); + var schemaRepository = new SchemaRepository(); + var schemaGenerator = SchemaGenerator(new HelloRequestValidator()); + + var schemaGenerationOptions = new SchemaGenerationOptions + { + NameResolver = new SystemTextJsonNameResolver(), + SchemaIdSelector = schemaGeneratorOptions.SchemaIdSelector, + RemoveUnusedQuerySchemas = true, // the default; this is the flag that triggers the bug + }; + + var validatorRegistry = new ValidatorRegistry( + new IValidator[] { new HelloRequestValidator() }, + new OptionsWrapper(schemaGenerationOptions)); + + var operationFilter = new FluentValidationOperationFilter( + validatorRegistry: validatorRegistry, + schemaGenerationOptions: new OptionsWrapper(schemaGenerationOptions)); + + return (operationFilter, schemaGenerator, schemaRepository); + } + + /// + /// Runs the for one GET endpoint that binds + /// via [FromQuery] (flattened to the "Name" parameter). + /// + private static void ApplyToQueryEndpoint( + FluentValidationOperationFilter operationFilter, + SchemaGenerator schemaGenerator, + SchemaRepository schemaRepository) + { + var metadataProvider = new EmptyModelMetadataProvider(); + var nameMetadata = metadataProvider.GetMetadataForProperty(typeof(HelloRequest), nameof(HelloRequest.Name)); + + var apiDescription = new ApiDescription(); + apiDescription.ParameterDescriptions.Add(new ApiParameterDescription + { + Name = "Name", + ModelMetadata = nameMetadata, + Source = BindingSource.Query, + }); + + var nameParamSchema = new OpenApiSchema { Type = JsonSchemaType.String }; + var operation = new OpenApiOperation + { + Parameters = new List + { + new OpenApiParameter { Name = "Name", In = ParameterLocation.Query, Schema = nameParamSchema }, + }, + }; + + var context = new OperationFilterContext( + apiDescription, + schemaGenerator, + schemaRepository, + new OpenApiDocument(), + typeof(SharedDtoMixedBindingTests).GetMethod(nameof(JsonBody_Component_Should_Exist_And_Carry_Rules_After_Query_Operation_Cleanup))!); + + operationFilter.Apply(operation, context); + } + } +} +#endif diff --git a/version.props b/version.props index ac108b0..7cf19da 100644 --- a/version.props +++ b/version.props @@ -1,6 +1,6 @@ - 7.1.10 + 7.1.11