Skip to content

feat(aspnetcore-openapi): emit encoding.contentType for IFormFile parts (#216 follow-up)#219

Merged
avgalex merged 2 commits into
masterfrom
feature/issue-216-media-types
Jun 21, 2026
Merged

feat(aspnetcore-openapi): emit encoding.contentType for IFormFile parts (#216 follow-up)#219
avgalex merged 2 commits into
masterfrom
feature/issue-216-media-types

Conversation

@avgalex

@avgalex avgalex commented Jun 21, 2026

Copy link
Copy Markdown
Member

Follow-up to #218 (merged) for issue #216.

The issue author (@marcuscfarias) is on Scalar + Microsoft.AspNetCore.OpenApi and reported the allowed media types weren't showing. Diagnosis: he's using the new .FileContentType(...) API correctly, but his .NET 10 document emits the file part as a $ref to a shared IFormFile component, whose description Scalar doesn't render in the form-field row — and that backend did not yet emit the machine-readable encoding.contentType.

Change

FluentValidationOperationTransformer now writes requestBody.content["multipart/form-data"].encoding.<part>.contentType for parts restricted via .FileContentType(...):

multipart/form-data:
  schema: { $ref: "#/components/schemas/UploadProductImageRequest" }   # net10
  encoding:
    File:
      contentType: "image/jpeg, image/png"

Part-name resolution handles both shapes:

  • net9 (and some net10 endpoints): inline multipart schema → keys read directly.
  • net10: the whole body is a $ref to a component whose Target is not resolved when the operation transformer runs, so the component is looked up in context.Document.Components.Schemas by the reference id.

The backend matrix is now content-type + size on all three backends, both as description and encoding.contentType.

Tests

Issue216SpikeTests.FileContentType_Emits_Encoding_ContentType verifies the encoding appears in the live /openapi/v1.json on net9 and net10.

Full solution green: Swashbuckle 117/117/86 (net8/9/10), NSwag 3/3 (net8/9), AspNetCore.OpenApi 18/18 (net9/10).

Release

Version bumped to 7.1.8-beta.2; pre-release published. CHANGELOG + README matrix updated.

🤖 Generated with Claude Code

…ts (#216)

The FluentValidationOperationTransformer now writes
requestBody.content["multipart/form-data"].encoding.<part>.contentType for parts
restricted via .FileContentType(...), so UIs like Scalar/Swagger UI show the
accepted media types (not just the description).

Part-name resolution handles both shapes: an inline multipart schema (net9, and
some net10 endpoints) and a whole-body $ref to a component (net10). In the $ref
case the reference Target is not resolved at operation-transform time, so the
component is looked up in context.Document.Components.Schemas by ref id.

Test: new Issue216SpikeTests.FileContentType_Emits_Encoding_ContentType verifies
the encoding appears in /openapi/v1.json on net9 and net10.

Release: 7.1.8-beta.2. Backend matrix is now content-type+size description AND
encoding.contentType across all three backends. Full solution green:
Swashbuckle 117/117/86, NSwag 3/3, AspNetCore.OpenApi 18/18.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@avgalex

avgalex commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

@claude review this PR

@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

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


PR Review: feat(aspnetcore-openapi): emit encoding.contentType for IFormFile parts

  • Gather context
  • Read changed files
  • Review main implementation (FluentValidationOperationTransformer.cs)
  • Review tests (Issue216SpikeTests.cs)
  • Review docs/metadata changes
  • Post final review

Overall: The implementation is clean, well-reasoned, and solves the right problem. The net9/net10 dual-path in GetFormPartKeys is the key design decision and it's correct. A few things worth calling out below, mostly minor.


FluentValidationOperationTransformer.cs — Core Logic

ApplyRulesToRequestBody (lines 91–136)

The structure is solid: bail out early if no multipart body, resolve part keys, then match validators. A few observations:

1. Overwriting pre-existing OpenApiEncoding entries (line 133)

mediaType.Encoding[partKey] = new OpenApiEncoding { ContentType = string.Join(", ", allowed) };

This replaces the whole OpenApiEncoding object, discarding any other properties (Headers, Style, Explode, AllowReserved) that may have been set earlier in the pipeline. In practice, ASP.NET Core doesn't emit those for file parts, so this is safe today — but defensive code would merge rather than overwrite:

if (!mediaType.Encoding.TryGetValue(partKey, out var enc))
    mediaType.Encoding[partKey] = enc = new OpenApiEncoding();
enc.ContentType = string.Join(", ", allowed);

Fix this →

2. "Last parameter wins" for multiple validators (lines 111–135)

The outer loop over methodInfo.GetParameters() means if two action parameters both have a FileContentType validator for the same part name, the second one silently overwrites the first. Not a real-world concern for a typical [FromForm] endpoint, but worth a note.

GetFormPartKeys (lines 143–166)

The dual-path (inline schema for net9, $ref component lookup for net10 via #if OPENAPI_V2) is exactly right. The comment explains it clearly. No issues here.

One subtle note: the net9 fast-path (schema?.Properties is { Count: > 0 }) executes for both compile targets. On OPENAPI_V2 (net10), if the schema has inline properties, it returns them directly without entering the $ref branch. That is correct — if the schema happens to be inline on net10 it still works.


Issue216SpikeTests.cs — Test Quality

3. JsonDocument resource leak in GetMultipartMediaTypeAsync (line 31)

var doc = JsonDocument.Parse(json);
return doc.RootElement.GetProperty(...);

JsonDocument owns a pooled buffer. Returning a JsonElement (which points into that buffer) while letting doc fall out of scope is a lifetime bug — if GC runs before the caller consumes the element, it will read from a returned buffer, causing ObjectDisposedException or memory corruption.

The safe patterns are:

  • Clone: return element.Clone(); (allocates but breaks the doc dependency)
  • Return the doc along with the element as a tuple and dispose in the caller
  • Parse as JsonNode instead, which does not have this lifecycle constraint

This is the same pattern used in AspNetCoreOpenApiTests.cs (lines 413–414), so it's an existing codebase issue being replicated — but it's still a latent bug, especially in heavy GC conditions.

Fix this →

4. Weakened assertion in FileContentType_And_MaxFileSize_Are_Documented_For_Upload_Endpoint

The original test verified that the multipart/form-data key exists in the response:

// was:
.TryGetProperty("multipart/form-data", out _).Should().BeTrue();

This was removed in the refactor. The string-based .Contain() checks compensate somewhat (if the strings appear, the response must have them), but they don't verify document structure. Minor; the new FileContentType_Emits_Encoding_ContentType_For_Upload_Endpoint test is a much stronger structural check anyway.

5. FileContentType_Emits_Encoding_ContentType_For_Upload_Endpoint — hardcodes .First()

var fileEncoding = encoding.EnumerateObject().Select(property => property.Value).First();

If the encoding object ever has multiple parts (e.g., a second field alongside the file), this silently tests whichever part comes first in the serialized JSON. Better to select by part name:

var fileEncoding = encoding.GetProperty("File"); // or whatever the part key is
fileEncoding.GetProperty("contentType").GetString().Should().Be("image/jpeg, image/png");

Fix this →


Docs / Metadata

CHANGELOG.md and README.md are accurate and clearly describe both the behavior change and the net10 caveat (that the file part description is shared across endpoints via the $ref component, but encoding.contentType is per-operation and unaffected). Good.

version.propsbeta.1beta.2 is correct.


Summary

Core logic ✅ Correct — dual-path $ref resolution is well-handled
#if OPENAPI_V2 usage ✅ Consistent with codebase conventions
Error safety ✅ Covered by outer try/catch in TransformAsync
JsonDocument lifetime in test ⚠️ Latent resource-leak bug
Encoding overwrite ⚠️ Defensive only — safe in practice today
Test assertion on part name ⚠️ .First() is fragile

All three issues are minor. The core change is correct and ready to ship. If you want to address any of them, the JsonDocument lifetime issue is the one most likely to cause a flaky test in the future.

… lifetime

- FluentValidationOperationTransformer: merge into an existing OpenApiEncoding
  (set only ContentType) instead of replacing the whole object, preserving any
  Headers/Style/Explode that an earlier stage may have set (matches the NSwag
  processor's pattern).
- Issue216SpikeTests: dispose the JsonDocument (using) and Clone() the returned
  JsonElement so it no longer references the document's pooled buffer (latent
  lifetime bug); select the encoding entry by the "File" part name instead of
  .First() so the assertion is robust to ordering and self-documenting.

Green: AspNetCore.OpenApi 18/18 (net9/net10).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@avgalex

avgalex commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review! Addressed in the latest commit:

  • Encoding overwrite → merge ✅ — now sets only ContentType on the existing OpenApiEncoding (or creates one), preserving any Headers/Style/Explode. This also matches the pattern already used by the NSwag FluentValidationOperationProcessor.
  • JsonDocument lifetime in the test helper ✅ — the helper now usings the JsonDocument and returns .Clone(), so the element no longer points into the disposed pooled buffer.
  • .First() → named part lookup ✅ — the encoding assertion now selects the File part by name (case-insensitive), robust to ordering and self-documenting.

Skipped (with reasons):

  • "Last parameter wins" for multiple form validators — left as-is; not a real-world [FromForm] shape, and you flagged it as a note rather than a fix.
  • Restored multipart key assertion — the structural check is already covered: GetMultipartMediaTypeAsync navigates content → multipart/form-data and throws if absent, so the encoding test exercises that path.

AspNetCore.OpenApi suite green: 18/18 on net9 and net10.

@avgalex

avgalex commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

@claude review this PR

@claude

claude Bot commented Jun 21, 2026

Copy link
Copy Markdown

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


PR Review: feat(aspnetcore-openapi): emit encoding.contentType for IFormFile parts

  • Gather context
  • Read changed files (current state after fixes)
  • Verify previous review issues were addressed
  • Full review of core implementation
  • Full review of tests
  • Post final review

This is a re-review of the current branch state after 8584323 addressed the previous round of feedback. All three flagged issues are correctly fixed. Below is the verification and a fresh pass.


Previous issues — verified fixed

Encoding overwrite → merge ✅ (FluentValidationOperationTransformer.cs lines 132–135)

mediaType.Encoding ??= new Dictionary<string, OpenApiEncoding>();
if (!mediaType.Encoding.TryGetValue(partKey, out var encoding) || encoding == null)
    mediaType.Encoding[partKey] = encoding = new OpenApiEncoding();
encoding.ContentType = string.Join(", ", allowed);

Correctly reuses an existing OpenApiEncoding and only sets ContentType, preserving Headers/Style/Explode. The null check on the out-value (|| encoding == null) handles the edge case where the dictionary contains an explicit null entry. Solid.

JsonDocument lifetime ✅ (Issue216SpikeTests.cs lines 31–35)

using var doc = JsonDocument.Parse(json);
return doc.RootElement.GetProperty("paths").GetProperty("/api/upload")
    ...GetProperty("multipart/form-data").Clone();

using guarantees disposal; .Clone() breaks the element's dependency on the pooled buffer. Correct.

Named part lookup ✅ (Issue216SpikeTests.cs lines 59–61)

var fileEncoding = encoding.EnumerateObject()
    .First(property => property.Name.Equals("File", StringComparison.OrdinalIgnoreCase))
    .Value;

Named lookup, case-insensitive, self-documenting. Correct.


Fresh pass — no new issues

GetFormPartKeys dual-path — The net9 fast-path (schema?.Properties is { Count: > 0 }) is tried first on both compile targets, so an inline schema on net10 still works. The #if OPENAPI_V2 block then handles the $ref lookup. Both branches return only the key set — the part schemas themselves are never needed. The logic is correct and the comment explains the net10 resolved-Target caveat well.

ApplyRulesToRequestBody control flow — The early-return sequence (no request body → no multipart content → no part keys → no method info) keeps the happy path readable. The per-parameter outer loop with per-part-key inner loop is necessary because a form model can technically have multiple file fields; FirstOrDefault() on allowed means the last validator match wins per part key (benign for the common case of one file field).

Test coverageFileContentType_And_MaxFileSize_Are_Documented_For_Upload_Endpoint verifies description text (robust to inline vs. $ref) and FileContentType_Emits_Encoding_ContentType_For_Upload_Endpoint verifies the structural encoding.contentType field. Together they cover the two distinct behaviors of the feature. The 18/18 AspNetCore.OpenApi suite (net9 + net10) green is the key signal.

CHANGELOG / README — Clear and accurate. The net10 caveat (description is on the shared IFormFile component; encoding.contentType is per-operation and unaffected) is correctly explained.


One minor observation (no action required)

In FileContentType_Emits_Encoding_ContentType_For_Upload_Endpoint, if encoding.TryGetProperty returns false, FluentAssertions will throw on the .Should().BeTrue() assertion before encoding.EnumerateObject() is reached — so the test fails with a clear message. This is fine test code; just noting the ordering dependency for future readers.


Summary

Area Status
Core logic (net9 inline / net10 $ref) ✅ Correct
Encoding merge (preserves existing properties) ✅ Fixed
JsonDocument lifetime in test helper ✅ Fixed
Named part lookup in test assertion ✅ Fixed
#if OPENAPI_V2 guards ✅ Consistent with codebase
CHANGELOG / README ✅ Accurate

The implementation is correct and the test fixes address all latent issues. Ready to merge.

@avgalex avgalex merged commit 2ea8233 into master Jun 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant