fix(multipart): handle typed nil readers consistently - #90
fix(multipart): handle typed nil readers consistently#90sylvesterkaczmarek wants to merge 9 commits into
Conversation
|
I followed up on the typed-nil review and pushed e6eeec6 via maintainer edits. The original shallow nil check still allowed a typed-nil pointer wrapped in an |
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed typed-nil reader handling across interface/pointer unwrapping, multipart field encoding, reader dispatch, header validation, and streamed request ownership/error propagation. No substantive correctness, security, or compatibility issues found.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Requesting changes because typed-nil reader normalization is inconsistent between multipart serialization and request transport selection. The encoder now writes a typed-nil reader as an empty scalar field, but inspectMultipartBody still classifies it as an upload and multipartRequestOptions therefore disables retries and selects a one-shot streamed body.
I reproduced this with a synthetic httptest server: a multipart request containing map[string]any{"file": (*reader)(nil)} received HTTP 429 and made only one attempt instead of retrying as a scalar-only form. Please normalize typed-nil readers consistently in the upload classifier and add an end-to-end request regression covering retry behavior. Redirect/replayability and Content-Length behavior should remain consistent with an empty scalar field as well.
The current-head CI, CodeQL, and Castiron custom-code workflows are also all stopped with action_required, so those checks still need to run successfully before merge.
| } | ||
| val = val.Elem() | ||
| } | ||
| if val.Kind() == reflect.Pointer && val.IsNil() { |
There was a problem hiding this comment.
[P2] Apply the same typed-nil semantics before choosing multipart transport. This branch correctly serializes a typed-nil reader as an empty scalar field, but pkg/cmd/multipartbody.go:inspectMultipartBody still sees the non-nil io.Reader interface and marks it as hasUpload. As a result, multipartRequestOptions unnecessarily selects one-shot streaming, applies option.WithMaxRetries(0), and rejects otherwise replayable 307/308 redirects. I verified the mismatch end to end with a synthetic HTTP 429 response: the empty-field request made one attempt and failed instead of retrying. Please update the upload classifier to recognize typed-nil readers as scalars and add a request-level retry regression.
There was a problem hiding this comment.
Fixed. The transport classifier now applies the same typed-nil reader semantics as the encoder, so typed-nil readers stay on the buffered scalar path instead of being treated as uploads. I also updated the known-length framing path so a typed-nil optional reader can coexist with a real file upload.
Added request-level regressions covering the reported 429 retry case, replay across both 307 and 308 redirects, buffered Content-Length, the empty scalar-field representation, and a known-length file upload alongside a typed-nil reader.
Fresh CI, CodeQL and Castiron runs are currently action_required pending maintainer approval.
sylvesterkaczmarek
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
4 participants
Summary
Normalize typed-nil reader semantics across multipart encoding and request transport selection so typed-nil readers are treated consistently as empty scalar fields.
Fixes #89.
Root cause
internal/apiform.encodeValuedetectedio.Readercapability before applying nil-pointer semantics. A typed-nil pointer therefore retained a concrete reader type and could reachio.Copy.The request layer had the same semantic mismatch in a different place:
inspectMultipartBodyclassified the same typed-nil reader as an upload even after the encoder treated it as an empty field. That unnecessarily selected one-shot streaming, disabled retries, and made an otherwise replayable scalar form reject 307/308 redirects.Fix
Content-Lengthcalculation;Regression coverage
The tests cover:
map[string]any;map[string]io.Reader;io.Readerinterface;Content-Length;Validation
The earlier encoder-focused revision passed focused tests, race tests, repository compile-only tests,
go mod verify, and lint locally. The transport follow-up is covered by the new request-level regressions above.Fresh upstream CI, CodeQL, and Castiron runs are currently gated with
action_requiredpending external-contributor approval.Risk
Low. The behavior change is limited to typed-nil reader values. Non-nil readers and real uploads retain the existing streamed path with retries disabled; scalar-only forms remain buffered and replayable.