Add operation transformer for query parameter validation (Issue #200)

[avgalex]

Summary

• Fixes Query parameters miss validation attributes with MicroElements.AspNetCore.OpenApi.FluentValidation #200: Query parameters with [AsParameters] were not getting validation constraints in MicroElements.AspNetCore.OpenApi.FluentValidation
• Root cause: only IOpenApiSchemaTransformer was implemented (processes component schemas), but query parameters are expanded inline and need IOpenApiOperationTransformer
• New FluentValidationOperationTransformer applies FluentValidation rules to operation parameters, matching the behavior of Swashbuckle's FluentValidationOperationFilter
• Supports [AsParameters] container type resolution with fallback via reflection
• Copies validation constraints (min/max, minLength, pattern, etc.) from schema to parameter schema

Note: Nested DTO validation in request bodies is a separate known limitation of the schema transformer, tracked separately.

Test plan

• Added QueryParameters_WithAsParameters_ShouldHaveValidationConstraints test
• Test passes on both net9.0 and net10.0
• All existing tests pass (82 Swashbuckle net8.0/net9.0, 68 net10.0, 6+1 AspNetCore)

🤖 Generated with Claude Code

[avgalex]

@claude review this PR

[claude]

Claude finished @avgalex's task in 2m 38s —— View job

---

PR Review: Add operation transformer for query parameter validation (Issue #200)

• Gather context and understand the PR
• Read changed files
• Analyze code quality, correctness, and design
• Post review feedback

---

Overall this is a well-structured solution to a real problem. The approach is correct and the test coverage is good. Below are observations ranging from bugs to minor improvements.

---

Unused imports

FluentValidationOperationTransformer.cs (lines 8–9) imports two namespaces that are not used anywhere in the file:

using System.Text.Json;                          // unused
using System.Text.Json.Serialization.Metadata;  // unused

Fix this →

---

Performance: repeated work per parameter

In ApplyRulesToParameters (line 82), for each operationParameter the code:

1. Resolves the container type (including optional reflection scan of method parameters)
2. Calls _validatorRegistry.GetValidator(parameterType)
3. Calls BuildSchemaForType(parameterType) (reflection over all properties)

For a container with N properties, all N parameters resolve to the same parameterType, causing N redundant validator lookups and schema builds. Grouping by container type before the loop would be a meaningful optimization for larger query models:

// Group parameters by their resolved container type, build schema once per type
var grouped = operation.Parameters
    .Select(p => (Parameter: p, ContainerType: ResolveType(p, context)))
    .Where(x => x.ContainerType != null)
    .GroupBy(x => x.ContainerType!);

foreach (var group in grouped)
{
    var schema = BuildSchemaForType(group.Key!);
    var validator = _validatorRegistry.GetValidator(group.Key!);
    // apply to each parameter in the group
}

---

BuildSchemaForType incomplete type mapping

SetSchemaType (line 213) defaults to "string" for anything not int/long/short/byte/float/double/decimal/string/bool. Types like DateTime, DateTimeOffset, Guid, Enum, and collections all fall through to "string". While this is unlikely to break validation rule application for the most common query param types, it could cause incorrect schema types if someone places a GreaterThan rule on a DateTime property, for example.

More critically, enum properties would get type "string" instead of "integer", which could produce wrong OpenAPI output.

---

ResolveContainerType will fail for nested parameter paths

ResolveContainerType at line 271 searches for a property on the container type using the full parameterName (e.g. "address.city"):

var property = paramType.GetProperties()
    .FirstOrDefault(p => p.Name.Equals(parameterName, StringComparison.OrdinalIgnoreCase));

For nested [AsParameters] models where [FromQuery] produces dot-separated names (e.g. "address.city"), this will never match any property because no property is literally named "address.city". The dot-path stripping that happens later (lines 117–119) should ideally also apply here, or at least use the leaf name for property lookup.

In practice this only matters for nested [AsParameters] models with sub-objects, which is a rarer case, but it's worth noting.

---

Double property lookup with ToLowerCamelCase is likely redundant

Lines 171–172:

if (OpenApiSchemaCompatibility.TryGetProperty(schema, schemaPropertyName.ToLowerCamelCase(), out var property)
    || OpenApiSchemaCompatibility.TryGetProperty(schema, schemaPropertyName, out property))

Since BuildSchemaForType already applies _schemaGenerationOptions.NameResolver when building property keys (lines 198–199), the property in the temp schema should already be in the correct resolved name. The camelCase lookup first is a second-guess of the resolver's output. If NameResolver produces "skip", looking for "skip" (via ToLowerCamelCase()) will succeed; but if it produces "Skip", neither lookup may match correctly. Relying on the resolver output alone (using schemaPropertyName which was set from apiProperty.Key at line 126) would be cleaner.

---

Minor: validatorRegistry nullable parameter but throws immediately

IValidatorRegistry? validatorRegistry = null,
...
_validatorRegistry = validatorRegistry ?? throw new ArgumentNullException(nameof(validatorRegistry));

This pattern is copied from FluentValidationSchemaTransformer for consistency, which is fine since DI always provides it. But callers using the constructor directly will get a confusing ArgumentNullException despite the parameter being declared nullable. A non-nullable parameter would be more honest, though that's a broader API design question.

---

CopyValidationProperties is missing some constraints

The method copies 8 properties but omits MultipleOf, MinItems/MaxItems, and UniqueItems. These are unlikely to be used on query parameters, but if someone adds a custom FluentValidation rule that maps to MultipleOf, those constraints would silently not be copied. Not a blocker, just a completeness note.

---

Positive observations

• The try/catch wrapper in TransformAsync (lines 68–77) is good — a transformer failure should not crash the OpenAPI document generation.
• The fallback from modelMetadata.ContainerType → reflection on [AsParameters] (lines 96–100) is a solid two-stage approach.
• Using AspNetCoreSchemaProvider(null, _logger) correctly mirrors the schema transformer's approach and the null-safety on .NET 9 is documented in AspNetCoreSchemaProvider.
• Tests cover the new scenario, regression scenarios, and a non-obvious edge case (QueryParameters_ShouldNotAppearInComponentSchemas).
• The TransformerCanResolveWithoutScope test is valuable for validating build-time document generation compatibility.

---

The two most actionable items before merging are: removing the unused imports and considering the ResolveContainerType leaf-name issue for nested query models. The performance concern is worth tracking for a follow-up if larger query containers become common.

[avgalex]

@claude review this PR

[claude]

Claude finished @avgalex's task in 1m 54s —— View job

---

PR Review: Add operation transformer for query parameter validation (Issue #200)

• Gather context and understand the PR
• Read changed files
• Analyze code quality, correctness, and design
• Post review feedback

---

This PR is well-structured and addresses a real gap in the ASP.NET Core OpenAPI integration. The previous review comments have largely been addressed. Below is a focused review of what remains and what looks good.

---

Addressed since the previous review

These concerns from the prior Claude review have been resolved:

• Unused imports — System.Text.Json and System.Text.Json.Serialization.Metadata are no longer present in the transformer.
• Performance grouping — parameterGroups dictionary (line 83) correctly caches (Validator, Schema) per container type, eliminating redundant lookups.
• ResolveContainerType dot-path — Lines 287–291 now extract the leaf name before searching properties, fixing the nested path lookup.
• Double ToLowerCamelCase lookup — Removed; property lookup at line 192 uses the resolved name directly.
• MinItems/MaxItems in CopyValidationProperties — Added at lines 268–269, and Format at line 275.

---

Remaining issues

1. SetSchemaType still falls back to "string" for many types

SetSchemaType (lines 232–258) handles int, long, short, byte, float, double, decimal, string, and bool, but everything else (including DateTime, DateTimeOffset, Guid, enums, and collection types) maps to "string". This means:

• An enum property gets type: "string" in the temp schema used for validation rule application. If a GreaterThan or InclusiveBetween rule is placed on an enum property, the built schema type would be wrong.
• More practically, if someone adds a GreaterThanOrEqualTo rule on a DateTime query param, it would be applied against a schema with type: "string", potentially causing rule mismatches depending on how the rule implementation checks the schema type.

This is pre-existing in this PR and not a blocker for merging, but worth a follow-up issue or a TODO comment in the code.

Fix this →

---

2. NestedQueryParameters_WithDotPath_ShouldHaveValidationConstraints does not actually test dot-paths

The test at line 282 claims to cover the dot-path scenario ("filter.minAge" style), but the endpoint being tested is:

app.MapGet("/api/filter", ([AsParameters] TestFilterParams filter) => Results.Ok(filter));

TestFilterParams is the leaf type (has MinAge, MaxAge directly). Its parameters are emitted as MinAge and MaxAge — no dots. The test correctly validates that direct [AsParameters] works, but it is misleadingly named.

The actual dot-path scenario would require an endpoint like:

app.MapGet("/api/nested-filter", ([AsParameters] TestNestedQueryParameters query) => Results.Ok(query));

Where TestNestedQueryParameters.Filter is of type TestFilterParams — producing parameters named filter.MinAge and filter.MaxAge. TestNestedQueryParameters was added to TestModels.cs (line 104) but is never used in an endpoint.

This means the dot-path code path in ApplyRulesToParameter (lines 138–140) and ResolveContainerType (lines 287–291) is not covered by any test. Consider renaming the test to DirectAsParameters_ShouldHaveValidationConstraints and adding a separate test with a true nested container.

---

3. MultipleOf and UniqueItems missing from CopyValidationProperties

Minor completeness note: CopyValidationProperties (lines 264–276) still omits MultipleOf and UniqueItems. These are uncommon for query parameters, but the omission means custom rules mapping to those properties would silently not copy. Not a blocker.

---

4. IValidatorRegistry? validatorRegistry = null but throws immediately

Line 50:

_validatorRegistry = validatorRegistry ?? throw new ArgumentNullException(nameof(validatorRegistry));

This is consistent with FluentValidationSchemaTransformer so it's acceptable for API symmetry, but it's misleading — the nullable annotation signals "optional" while the implementation makes it mandatory. Worth aligning both transformers in a future cleanup.

---

Positive observations

• The two-stage fallback (modelMetadata.ContainerType → reflection via [AsParameters]) is solid and mirrors what Swashbuckle's filter does.
• The try/catch in TransformAsync correctly swallows transformer errors without crashing doc generation.
• TransformerCanResolveWithoutScope is a valuable test — build-time doc generation in .NET 10 runs without an HTTP scope and this catches that regression.
• QueryParameters_ShouldNotAppearInComponentSchemas is a good non-obvious regression test.
• The parameterGroups caching is a clean, well-commented optimization.

---

Summary

The two items most worth addressing before merging:

1. Rename or fix the dot-path test — it currently tests a different scenario than its name implies and the actual dot-path code path is untested.
2. SetSchemaType DateTime/enum coverage — at minimum add a TODO comment so this isn't lost.

Everything else is minor or inherited from the existing codebase.

[avgalex]

@claude review this commit 42b8a3f

[claude]

Claude finished @avgalex's task in 1m 35s —— View job

---

Review: Commit 42b8a3f — "Address PR #201 review: fix dot-path resolution, add TODO for SetSchemaType"

• Gather context
• Read commit diff
• Analyze changes
• Post review feedback

---

This commit addresses the two highest-priority items from the previous review. Here's a focused assessment of each change.

---

SetSchemaType TODO comment — ✅ Good

The TODO at lines 230–231 correctly documents the DateTime/enum limitation. No concerns.

---

Test rename — ✅ Good

NestedQueryParameters_WithDotPath_ShouldHaveValidationConstraints → DirectAsParameters_ShouldHaveValidationConstraints is accurate; the test never exercised a dot-path scenario.

---

ResolveContainerType rewrite — ✅ Logic is correct, but dead code remains

The new implementation (walking path segments) is logically sound:

// segments.Length == 1: return the [AsParameters] container type directly
// segments.Length > 1: walk intermediate properties to find the leaf container

The algorithm correctly returns the container of the leaf property, so the validator and schema lookups at the call site remain unchanged.

However, the segments.Length > 1 branch is still dead code. The commit message says:

Remove unsupported nested [AsParameters] endpoint (minimal API limitation)

ASP.NET Core minimal APIs do not expand nested [AsParameters] into dot-path query parameters. So the branch that walks multiple segments can never be reached via the minimal API path. The code is correct if that scenario ever becomes supported, but right now it's an untested, unreachable branch.

The previous review asked to either add a test or rename the method. This commit renamed the test but kept the implementation — which is a reasonable middle ground, as long as the limitation is documented. Consider adding a brief inline comment:

// Note: ASP.NET Core minimal APIs do not currently emit dot-path parameters
// for nested [AsParameters] types, so the segments.Length > 1 branch is
// defensive code for potential future support.

---

TestNestedQueryParameters — ⚠️ [FromQuery] attribute is confusing noise

TestModels.cs now has:

public class TestNestedQueryParameters
{
    [Microsoft.AspNetCore.Mvc.FromQuery]   // ← added in this commit
    public TestFilterParams Filter { get; set; } = new();
}

But TestNestedQueryParameters is not used in any endpoint in Program.cs, and no test references it. Adding [FromQuery] to a dead model is misleading — it implies this scenario is expected to work but just isn't wired up. Two options:

1. Remove the class — if nested [AsParameters] is unsupported, the model has no purpose.
2. Add a comment explaining it's reserved for future support and is currently unsupported by minimal APIs.

Fix this →

---

Summary

Change	Assessment
SetSchemaType TODO	✅ Addresses the review feedback cleanly
Test rename	✅ Name now accurately reflects the scenario
ResolveContainerType path-walking	✅ Correct algorithm; segments.Length > 1 is unreachable but harmless — worth a comment
[FromQuery] on TestNestedQueryParameters	⚠️ Unused model with misleading new attribute; should be removed or commented

Nothing here is a blocker. The commit is a net improvement. Addressing the TestNestedQueryParameters dead code is the only thing worth a follow-up before merging.

[avgalex]

@claude review this commit 0942041

[claude]

Claude finished @avgalex's task in 38s —— View job

---

Review: Commit 0942041 — "Remove unused TestNestedQueryParameters, add defensive code comment"

• Gather context
• Read commit diff
• Analyze changes
• Post review feedback

---

This commit directly addresses both outstanding items from the previous review. Focused and clean.

---

TestNestedQueryParameters removal — ✅

The unused class with the misleading [FromQuery] attribute is gone. Exactly the right call — there was no endpoint using it, no test referencing it, and it implied a scenario that ASP.NET Core minimal APIs don't actually support. No concerns.

---

Defensive code comment — ✅

The comment added to the segments.Length > 1 branch is accurate and useful:

// Note: ASP.NET Core minimal APIs do not currently emit dot-path parameters
// for nested [AsParameters] types. This branch is defensive code for potential
// future framework support or MVC controller scenarios.

This directly addresses the concern from the prior review — the unreachable branch is now documented with a clear reason for its existence. The "or MVC controller scenarios" caveat is a nice addition; it acknowledges a legitimate path through which the branch could actually be reached, giving the code a concrete justification rather than just "might be useful someday."

---

Summary

All issues raised across the three review rounds have been addressed:

Issue	Status
Unused imports	✅ Fixed in earlier commit
Performance grouping	✅ Fixed in earlier commit
ResolveContainerType dot-path leaf name	✅ Fixed in earlier commit
Double ToLowerCamelCase lookup	✅ Fixed in earlier commit
MinItems/MaxItems/Format in CopyValidationProperties	✅ Fixed in earlier commit
SetSchemaType DateTime/enum coverage	✅ TODO comment added
Misleading test name	✅ Renamed to DirectAsParameters_ShouldHaveValidationConstraints
TestNestedQueryParameters dead code with [FromQuery]	✅ Removed in this commit
Unreachable branch undocumented	✅ Comment added in this commit

The PR is clean and ready to merge. The two remaining minor items (MultipleOf/UniqueItems omission and the nullable-but-required constructor parameter) are inherited from existing code and are reasonable to track as separate follow-ups.