Update to return HttpResponseMessage for file downloads#877
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds detection of file/binary responses in the code generator. Operations with binary content types or file-like schemas now produce Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Refitter.Core/RefitInterfaceGenerator.cs`:
- Around line 166-181: The file-detection logic uses response.Schema which can
be the schema of a different content type when multiple content types exist;
instead iterate the response.Content entries and for each matching file
content-type (use the existing StartsWith checks but exclude
"application/x-www-form-urlencoded" rather than matching all "application/x-"),
capture that specific content entry's schema (e.g., the media type's Schema) and
use it to determine binary/file type (check schema.Format == "binary" or
schema.Type == JsonObjectType.File) and return true; update the check around the
"application/x-" prefix to exclude "application/x-www-form-urlencoded" so form
data is not treated as file content.
🧹 Nitpick comments (2)
src/Refitter.Tests/Examples/FileStreamResponseTests.cs (2)
115-120: Consider adding a clarifying comment or renaming the test.The test verifies that
HttpResponseMessageis returned even whenReturnIApiResponseis enabled, which is the expected behavior. However, the test name could be clearer about this intentional bypass.✨ Suggested improvement
[Test] - public async Task Generates_HttpResponseMessage_With_IApiResponse_Setting() + public async Task Generates_HttpResponseMessage_Even_When_ReturnIApiResponse_Is_Enabled() { string generatedCode = await GenerateCode(returnIApiResponse: true); + // File downloads should return HttpResponseMessage directly, bypassing IApiResponse wrapping generatedCode.Should().Contain("Task<HttpResponseMessage> DownloadFile(string fileId"); }
122-134: Consider adding test coverage forIObservablemode.The generator supports
ReturnIObservablewhich would produceIObservable<HttpResponseMessage>instead ofTask<HttpResponseMessage>. Adding a test for this mode would improve coverage.✨ Example additional test
[Test] public async Task Generates_IObservable_HttpResponseMessage_When_ReturnIObservable_Is_Enabled() { var swaggerFile = await SwaggerFileHelper.CreateSwaggerFile(OpenApiSpec); var settings = new RefitGeneratorSettings { OpenApiPath = swaggerFile, ReturnIObservable = true }; var sut = await RefitGenerator.CreateAsync(settings); var generatedCode = sut.Generate(); generatedCode.Should().Contain("IObservable<HttpResponseMessage> DownloadFile(string fileId"); }
…be form output from a website I'm not sure...) also catch all application/vnd* files. Pulled the IsFileContentType into it's own method as well.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Refitter.Core/RefitInterfaceGenerator.cs`:
- Around line 152-173: The IsFileStreamResponse method misses media types that
have no Schema and thus won't be detected as file responses; update
IsFileStreamResponse to treat content entries whose Schema is null as file
streams for appropriate content types by adding a fallback check after
retrieving var schema = contentEntry.Value?.Schema — if schema is null and
IsFileContentType(contentEntry.Key) (optionally also ensure the content type is
not a vendor JSON like application/vnd* if you want to exclude those), return
true so responses with schema-less file media types map to HttpResponseMessage;
keep the existing binary/Type checks (schema.Format == "binary" || schema.Type
== JsonObjectType.File) and only use the schema-null fallback when those are
absent.
In `@src/Refitter.Tests/Examples/FileStreamResponseTests.cs`:
- Around line 125-131: The presence check uses the wrong casing: update the
assertion in the test Does_Not_Generate_HttpResponseMessage_For_FormUrlEncoded
to look for "SubmitForm" (PascalCase) or perform a case-insensitive containment
check on generatedCode so it matches the generator's PascalCase method names;
ensure the assertion that checks for the absence of "Task<HttpResponseMessage>
SubmitForm(" still references SubmitForm with correct casing.
| private static bool IsFileStreamResponse(OpenApiOperation operation) | ||
| { | ||
| var successCodes = new[] { "200", "201", "203", "206", "2XX" }; | ||
|
|
||
| foreach (var code in successCodes) | ||
| { | ||
| if (!operation.Responses.ContainsKey(code)) | ||
| continue; | ||
|
|
||
| var response = operation.Responses[code].ActualResponse; | ||
|
|
||
| if (response.Content?.Any() != true) | ||
| continue; | ||
|
|
||
| foreach (var contentEntry in response.Content) | ||
| { | ||
| if (IsFileContentType(contentEntry.Key)) | ||
| { | ||
| var schema = contentEntry.Value?.Schema; | ||
| if (schema?.Format == "binary" || schema?.Type == NJsonSchema.JsonObjectType.File) | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Handle file content types with missing schemas.
OpenAPI allows a media type without a schema; those would be treated as non-file here. If you want schema-less file responses to map to HttpResponseMessage, add a fallback (optionally excluding application/vnd* if you want to keep vendor JSON strict).
💡 Possible fallback
if (IsFileContentType(contentEntry.Key))
{
var schema = contentEntry.Value?.Schema;
- if (schema?.Format == "binary" || schema?.Type == NJsonSchema.JsonObjectType.File)
+ if (schema == null)
+ return true;
+ if (schema.Format == "binary" || schema.Type == NJsonSchema.JsonObjectType.File)
return true;
}🤖 Prompt for AI Agents
In `@src/Refitter.Core/RefitInterfaceGenerator.cs` around lines 152 - 173, The
IsFileStreamResponse method misses media types that have no Schema and thus
won't be detected as file responses; update IsFileStreamResponse to treat
content entries whose Schema is null as file streams for appropriate content
types by adding a fallback check after retrieving var schema =
contentEntry.Value?.Schema — if schema is null and
IsFileContentType(contentEntry.Key) (optionally also ensure the content type is
not a vendor JSON like application/vnd* if you want to exclude those), return
true so responses with schema-less file media types map to HttpResponseMessage;
keep the existing binary/Type checks (schema.Format == "binary" || schema.Type
== JsonObjectType.File) and only use the schema-null fallback when those are
absent.
There was a problem hiding this comment.
Pull request overview
This pull request enhances Refitter to automatically detect file/binary stream responses in OpenAPI specifications and generate HttpResponseMessage return types instead of empty IApiResponse results. This addresses a common use case for file downloads and provides better type safety for binary content.
Changes:
- Added automatic detection logic for file stream responses based on content type and schema format
- Enhanced
IsApiResponseTypeto recognizeHttpResponseMessagefor proper XML documentation generation - Comprehensive test coverage for common file download scenarios including octet-stream, images, PDFs, and negative cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Refitter.Tests/Examples/FileStreamResponseTests.cs | New test file with comprehensive coverage of file stream detection including positive cases (octet-stream, JPEG, PNG, PDF) and negative cases (form-urlencoded), plus validation that generated code builds successfully |
| src/Refitter.Core/RefitInterfaceGenerator.cs | Core implementation adding IsFileStreamResponse and IsFileContentType methods to detect binary responses, integrated into GetTypeName with proper precedence, and updated IsApiResponseType to handle HttpResponseMessage |
| foreach (var code in successCodes) | ||
| { | ||
| if (!operation.Responses.ContainsKey(code)) | ||
| continue; | ||
|
|
||
| var response = operation.Responses[code].ActualResponse; | ||
|
|
||
| if (response.Content?.Any() != true) | ||
| continue; | ||
|
|
||
| foreach (var contentEntry in response.Content) | ||
| { | ||
| if (IsFileContentType(contentEntry.Key)) | ||
| { | ||
| var schema = contentEntry.Value?.Schema; | ||
| if (schema?.Format == "binary" || schema?.Type == NJsonSchema.JsonObjectType.File) | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var contentEntry in response.Content) | ||
| { | ||
| if (IsFileContentType(contentEntry.Key)) | ||
| { | ||
| var schema = contentEntry.Value?.Schema; | ||
| if (schema?.Format == "binary" || schema?.Type == NJsonSchema.JsonObjectType.File) | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@all-contributors please add @frogcrush for code |
|
I've put up a pull request to add @frogcrush! 🎉 |
|
|
@christianhelle failing check issue doesn't seem like a code issue? |
It's some concurrency issue that got introduced when I switched to TUnit (from XUnit) some time ago. I can't reproduce it locally but it happens every now and then when built from Github Actions. I actually just removed this test on the latest master |
|
The changes look good to me and the smoke tests all pass so let's get this in |
|
@frogcrush thank you for this contribution |
Updated [refitter](https://github.com/christianhelle/refitter) from 1.7.1 to 2.0.0. <details> <summary>Release notes</summary> _Sourced from [refitter's releases](https://github.com/christianhelle/refitter/releases)._ ## 2.0.0 ## What's Changed * Fix numeric format with pattern quirk - infer type from format for all numeric types by @Copilot in christianhelle/refitter#869 * Read group documentation from document tags. by @DJ4ddi in christianhelle/refitter#887 * Fix null reference and XML escaping in XmlDocumentationGenerator by @Copilot in christianhelle/refitter#890 * Fix build workflow: add dotnet restore before dotnet msbuild in Prepare step by @Copilot in christianhelle/refitter#892 christianhelle/refitter#895 * Fix MSBuild workflow by @christianhelle in christianhelle/refitter#898 * Add support for generating a single client from multiple OpenAPI specifications by @Copilot in christianhelle/refitter#904 * Migrate from Microsoft.OpenApi.Readers 1.x to Microsoft.OpenApi 3.x by @vgmello in christianhelle/refitter#907 * Improve Smoke Tests execution time by @christianhelle in christianhelle/refitter#915 * Fix #580: Nullable strings marked correctly by @christianhelle in christianhelle/refitter#921 * Fix #672: MultipleInterfaces ByTag method naming scoped per-interface by @christianhelle in christianhelle/refitter#922 * Fix #635: Refactor source generator to use context.AddSource() by @christianhelle in christianhelle/refitter#923 * Add custom format mappings configuration by @christianhelle in christianhelle/refitter#927 * Fix multipart form-data parameter extraction by @christianhelle in christianhelle/refitter#928 christianhelle/refitter#911 christianhelle/refitter#902 * Fix smoke tests: --interface-only variant missing using directive for contract types by @Copilot in christianhelle/refitter#933 * Fix SonarCloud Code Quality Issues by @Copilot in christianhelle/refitter#932 * Add debug logging for source generator when searching for .refitter files by @codymullins in christianhelle/refitter#743 * Fix: Base type not generated for types using oneOf with discriminator by @Copilot in christianhelle/refitter#906 * Add option for Method Level Authorization header attribute by @Roflincopter in christianhelle/refitter#897 * Fix PR #897 review feedback and add comprehensive bearer auth tests by @christianhelle in christianhelle/refitter#936 * Fix numeric suffix added to interface method names in ByTag mode by @Copilot in christianhelle/refitter#914 * Improve OpenAPI parse + codegen throughput by removing avoidable allocations and repeated regex work by @Copilot in christianhelle/refitter#937 * Move [JsonConverter] from enum properties to enum types by @christianhelle in christianhelle/refitter#938 * Fix broken CLI tool help text by @christianhelle in christianhelle/refitter#940 * Improve code coverage to >90% by @christianhelle in christianhelle/refitter#941 * Microsoft.OpenApi v3.4 by @christianhelle in christianhelle/refitter#945 * Add Unicode support for XML doc comment generation by @christianhelle in christianhelle/refitter#948 * Add PropertyNamingPolicy support for JSON property naming by @christianhelle in christianhelle/refitter#969 christianhelle/refitter#966 * Fix recursive schema stack overflows by @christianhelle in christianhelle/refitter#971 * Made Header Parameters for Security Schemes safe to use as C# variable name by @smoerijf in christianhelle/refitter#977 * Enhance schema alias handling and property name generation by @christianhelle in christianhelle/refitter#996 * Verify alias handling and sanitize PascalCase properties by @christianhelle in christianhelle/refitter#997 * Harden .refitter settings deserialization and output path handling by @christianhelle in christianhelle/refitter#1000 * Special-case the default .refitter filename before deriving OutputFilename by @coderabbitai[bot] in christianhelle/refitter#1002 * [v2.0 audit] Fix pre-release regressions from #1057 by @christianhelle in christianhelle/refitter#1064 * Resolve high-severity audit findings from #1057 by @christianhelle in christianhelle/refitter#1067 * [v2.0 audit] Close remaining verified #1057 regressions by @christianhelle in christianhelle/refitter#1070 * Harden generation flows by @christianhelle in christianhelle/refitter#1071 * Breaking changes and Migration Guide for v2.0.0 by @christianhelle in christianhelle/refitter#1009 * Handle equivalent duplicate schemas in multi-spec merge by @christianhelle in christianhelle/refitter#1076 * Tighten warning handling across Refitter builds by @christianhelle in christianhelle/refitter#1079 * Fix default solution path and update .NET version in VS Code config by @christianhelle in christianhelle/refitter#1082 * Fix docker smoke tests by @christianhelle in christianhelle/refitter#1081 * Sanitize malformed schema type names without renaming clean schemas by @christianhelle in christianhelle/refitter#1085 ... (truncated) ## 1.7.3 ## What's Changed * Add support for systems running only .NET 10.0 (without .NET 8.0 or 9.0) in Refitter.MSBuild by @christianhelle in christianhelle/refitter#882 * Update to return HttpResponseMessage for file downloads by @frogcrush in christianhelle/refitter#877 ## New Contributors * @frogcrush made their first contribution in christianhelle/refitter#877 **Full Changelog**: christianhelle/refitter@1.7.2...1.7.3 ## 1.7.2 **Implemented enhancements:** - Improve Immutable Records ergonomics [\#844](christianhelle/refitter#844) - Omit certain operation headers and include all others [\#840](christianhelle/refitter#840) - Create .refitter settings file as part of output [\#859](christianhelle/refitter#859) by @[christianhelle - Fix integerType enum deserialization issue [\#855](christianhelle/refitter#855) ([christianhelle](https://github.com/christianhelle)) - support custom nswag template directory \#844 [\#854](christianhelle/refitter#854) by @kmc059000 - Fix missing method parameter XML code-documentation [\#850](christianhelle/refitter#850) ([christianhelle](https://github.com/christianhelle)) **Fixed bugs:** - integerType parsing in settings file fails [\#851](christianhelle/refitter#851) - CS1573 : Method parameter has no matching XML comment [\#846](christianhelle/refitter#846) **Merged pull requests:** - docs: add frogcrush as a contributor for code [\#878](christianhelle/refitter#878) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - Migrate solution files from .sln to .slnx format [\#876](christianhelle/refitter#876) ([Copilot](https://github.com/apps/copilot-swe-agent)) - Fix issue with randomly failing tests to due parallel execution [\#872](christianhelle/refitter#872) ([christianhelle](https://github.com/christianhelle)) - chore\(deps\): update dependency tunit to 1.9.2 [\#863](christianhelle/refitter#863) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.7.7 [\#862](christianhelle/refitter#862) ([renovate[bot]](https://github.com/apps/renovate)) - Add unit tests for WriteRefitterSettingsFile functionality [\#860](christianhelle/refitter#860) ([Copilot](https://github.com/apps/copilot-swe-agent)) - chore\(deps\): update dependency ruby to v4 [\#858](christianhelle/refitter#858) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.28 [\#857](christianhelle/refitter#857) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add 0x2badc0de as a contributor for bug [\#856](christianhelle/refitter#856) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency swashbuckle.aspnetcore to 10.1.0 [\#853](christianhelle/refitter#853) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.0 [\#852](christianhelle/refitter#852) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add lilinus as a contributor for code [\#849](christianhelle/refitter#849) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency ruby to v3.4.8 [\#845](christianhelle/refitter#845) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.5.70 [\#837](christianhelle/refitter#837) ([renovate[bot]](https://github.com/apps/renovate)) **Full Changelog**: christianhelle/refitter@1.7.1...1.7.2 Commits viewable in [compare view](christianhelle/refitter@1.7.1...2.0.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>



As discussed in #507, determine common file responses and generate HttpResponseMessage instead of an empty IApiResult.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.