Fix/csharp reserved headers and file parameter not serialising correctly#23593
Fix/csharp reserved headers and file parameter not serialising correctly#23593BottlecapDave wants to merge 7 commits intoOpenAPITools:masterfrom
Conversation
|
thanks for the PR cc @devhl-labs |
| private JsonSerializerOptions _jsonSerializerOptions; | ||
|
|
||
| private readonly string[] _contentHeaders = | ||
| [ |
There was a problem hiding this comment.
Consider making this static readonly, or move it to ClientUtils.
| "content-type", | ||
| "expires", | ||
| "last-modified", | ||
| "extension-header" |
There was a problem hiding this comment.
Copilot is telling me extension-header is demonstrating the format, but isn't a real entry. Consider removing it. I'd also appreciate if you remove the spaces that appear after these values.
| httpRequestMessageLocalVar.Content = ({{paramName}}{{^required}}.Value{{/required}} as object) is System.IO.Stream stream | ||
| ? httpRequestMessageLocalVar.Content = new StreamContent(stream) | ||
| : httpRequestMessageLocalVar.Content = new StringContent(JsonSerializer.Serialize({{paramName}}{{^required}}.Value{{/required}}, _jsonSerializerOptions)); | ||
| if (({{paramName}}{{^required}}.Value{{/required}} as object) is System.IO.Stream stream) |
There was a problem hiding this comment.
In generichost I don't believe it will ever be a System.IO.Stream anymore, I think its always going to be a FileParameter. Im not positive though. Worth looking into. You can leave this alone if you want though.
edit - I had copilot check and despite the type mapping being a Stream, the templates actually use FileParameter, so it should be safe to remove.
There was a problem hiding this comment.
17 issues found across 207 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed. cubic prioritises the most important files to review.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/csharp/generichost/net10/AnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/AnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs:333">
P2: `ContentHeaders` is missing `content-disposition`, so `IsContentHeader()` will misclassify valid multipart/file headers and route them to request headers instead of content headers.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/Tags/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/Tags/src/Org.OpenAPITools/Client/ClientUtils.cs:344">
P2: `Content-Disposition` is missing from the content-header allowlist, so `IsContentHeader()` will misclassify a valid content header as a request header.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/SourceGeneration/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/SourceGeneration/src/Org.OpenAPITools/Client/ClientUtils.cs:434">
P2: `ContentHeaders` allowlist is incomplete (missing `content-disposition`), so valid content headers can be misrouted to request headers.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/ComposedEnum/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/ComposedEnum/src/Org.OpenAPITools/Client/ClientUtils.cs:324">
P1: `ContentHeaders` is missing the standard `content-disposition` content header, so `IsContentHeader()` will misclassify valid content headers and can route them to the wrong header collection.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/AnyOfNoCompare/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/AnyOfNoCompare/src/Org.OpenAPITools/Client/ClientUtils.cs:318">
P2: `Content-Disposition` is a standard content header but is missing from the allowlist, so `IsContentHeader()` misclassifies it as a non-content header.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Client/ClientUtils.cs:419">
P2: `IsContentHeader` uses an incomplete content-header allowlist and omits `content-disposition`, which can misroute that header to request headers instead of content headers.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/NullReferenceTypes/src/Org.OpenAPITools/Client/ClientUtils.cs:434">
P1: `content-disposition` is missing from the reserved content-header allowlist, so it can still be routed to request headers instead of `HttpContent.Headers`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/Petstore/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/Petstore/src/Org.OpenAPITools/Client/ClientUtils.cs:432">
P2: `ContentHeaders` is missing `content-disposition`, so `IsContentHeader()` will misclassify that reserved content header and route it to the wrong header collection.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/InlineEnumAnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/InlineEnumAnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs:320">
P2: `ContentHeaders` is incomplete for `HttpContentHeaders` because it omits `content-disposition`, causing potential misclassification of a valid content header.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/AnyOfNoCompare/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/AnyOfNoCompare/src/Org.OpenAPITools/Client/ClientUtils.cs:304">
P2: `Content-Disposition` is a valid `HttpContentHeaders` member but is missing from the content-header whitelist, so `IsContentHeader()` will misclassify it.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/Petstore/src/Org.OpenAPITools/Client/ClientUtils.cs:432">
P1: Content headers allowlist is incomplete: `content-disposition` is missing, so that header can be misclassified and routed to request headers.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/OneOf/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/OneOf/src/Org.OpenAPITools/Client/ClientUtils.cs:328">
P2: `content-disposition` is a standard content header but is missing from the allowlist, so `IsContentHeader` will misclassify it and route it to request headers instead of `HttpContent.Headers`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/AnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/AnyOf/src/Org.OpenAPITools/Client/ClientUtils.cs:328">
P2: `ContentHeaders` is missing the `content-disposition` content header, so `IsContentHeader()` will misclassify a real `HttpContent` header and can route file-upload headers to the wrong collection.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net10/FormModels/src/Org.OpenAPITools/Client/ClientUtils.cs:428">
P2: Content header allowlist is incomplete (missing `content-disposition`), causing valid content headers to be misclassified and routed incorrectly.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/AllOf/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/AllOf/src/Org.OpenAPITools/Client/ClientUtils.cs:328">
P2: Content header allowlist is incomplete: `content-disposition` is missing, so `IsContentHeader` can misclassify a valid content header as a request header.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/net4.7/FormModels/src/Org.OpenAPITools/Client/ClientUtils.cs:428">
P2: Missing `Content-Disposition` from the reserved content-header list can misroute a valid content header to `HttpRequestMessage.Headers` instead of `HttpContent.Headers`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Client/ClientUtils.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Client/ClientUtils.cs:318">
P2: `ContentHeaders` whitelist is incomplete: it omits `content-disposition`, causing valid content headers to be misclassified.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 37 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/csharp/libraries/generichost/api.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/csharp/libraries/generichost/api.mustache:593">
P1: Routing content headers to `HttpRequestMessage.Content.Headers` can dereference null when the request has no body/content initialized.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This PR fixes a couple of issues
FileParameterparameters which are incorrectly JSON serialised as the content instead of the contained streamI've tested this locally via template overrides. I'm not sure what else needs to be included for this PR.
@mandrean, @shibayan, @Blackclaws, @lucamazzanti, @iBicha
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)Summary by cubic
Fixes request building in the C#
generichostclient so reserved headers and file uploads are handled correctly. Fixes #21727.Bug Fixes
HttpContentheaders instead of the request message.FileParameteras the contained stream (not JSON) for correct file upload behavior.Refactors
ClientUtils(IsContentHeader) with a header list compatible with older .NET versions; updated templates to use it.generichostsamples to reflect template changes.Written for commit dd199ab. Summary will update on new commits.