Skip to content

refactor(s3s): move the POST Object form parsing into a handwritten module - #994

Merged
Nugine merged 4 commits into
mainfrom
refactor/post-object-handwritten-parsing
Oct 8, 2026
Merged

Nugine merged 4 commits into
mainfrom
refactor/post-object-handwritten-parsing

Conversation

@Nugine

@Nugine Nugine commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Summary

POST Object's form parsing and its success responses used to be emitted straight into the generated operation file, so changing them meant changing the generator and the generated artifact together. This moves both functions into a hand-written module, crates/s3s/src/ops/post_object.rs, and leaves the generated file a wrapper that delegates to it. The generated file drops from 157 to 91 lines while the behaviour stays put: the wire suite passes in both feature sets, and a red control on the moved status mapping fails it.

Changes

  • Add crates/s3s/src/ops/post_object.rs holding deserialize_http and serialize_http, moved verbatim from the generated file (only paths, imports and visibility changed), and register the module in ops/mod.rs.
  • Emit thin delegations from codegen/src/v2/post_object.rs, so the generated PostObject keeps its pub fn methods and the Operation::call glue is untouched.
  • Add six unit tests for the branches the wire suite cannot reach: a redirect URL with control characters, an unparsable redirect URL, and the 200 / 201 / unrecognised / default status mappings.

Behavior

None for callers: the wrappers keep their signatures and the response construction is the same code, so the recorded wire behaviour is unchanged in both feature sets.

Tests

  • cargo test -p s3s --all-features --lib → exit 0, 1048 passed, 0 failed; cargo test -p s3s --all-features --test post_object_wire and the default feature set → 12 passed each.
  • just codegen twice → exit 0 and 0, with 0 dirty files after each run, so the wrapper the generator emits is the committed one.
  • Move fidelity: the two function bodies are verbatim (20/20 and 48/48 lines after normalisation), checked by a script whose own positive and negative controls are exercised (compare_move.py --selftest). The generated file's diff is the two delegations and its imports; the Operation::call glue is unchanged.
  • just dev → exit 0; cargo +nightly clippy --workspace --all-targets --all-features -- -D warnings → exit 0; just ci-rust → exit 0, with assert_unchanged reporting a clean tree.
  • Red controls: removing the control-character check, and answering 200 with 201, each fail the suite with exit 101; restoring them returns to green. The status-mapping control was also reproduced during review by making the 200 branch answer 201.
  • Patch coverage of the moved module: 258 of 261 lines; the three remaining partials are error paths that cannot be taken — the two ? on the redirect lookups (T = String makes the parse infallible) and the header-value conversion after a parsed URL (ASCII by construction). A test pins the observable half of that: the redirect value is validated when the response is serialized, not while parsing.

References

Part of #982.

Dependencies

Based on main after the codegen v2 change merged (#993). No dependency or manifest changes.

AI

Written with DeepSeek Harness (deepseek-flash): it moved the functions, thinned the emitter, wrote the unit tests and ran the gates and red controls above; the review re-ran the acceptance checks on the rebased stack. A human reviewed the diff and owns the change.

Nugine added 2 commits October 9, 2026 01:51
…odule

PostObject::deserialize_http and serialize_http were emitted as function bodies into crates/s3s/src/ops/generated/post_object.rs. Move both verbatim into crates/s3s/src/ops/post_object.rs and emit thin delegates instead, so the POST-specific parsing and response serialization live in hand-written code.

The struct, the Operation impl and the call glue stay generated, and the wrapper methods keep their public signatures. No behavior change.

Assisted-by: DeepSeek Harness:deepseek-flash
The wire suite covers the client-visible shapes, but it cannot reach the control-character rejection or the URL parse failure of the redirect, nor pin the status mapping in isolation. Call serialize_http directly for the default, 200, 201 and unrecognized statuses, for a redirect with a control character and for an unparsable redirect.

Assisted-by: DeepSeek Harness:deepseek-flash
Copilot AI balanced review requested due to automatic review settings October 8, 2026 17:56
@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.13665% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/s3s/src/ops/post_object.rs 98.11% 0 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

It is a faithful, verifiable code move with matching idempotent codegen output, all references resolving, and added unit tests covering the extracted logic.

0 open findings

What changed in this PR

This PR refactors the PostObject operation in the s3s crate by extracting its POST-specific form-parsing (deserialize_http) and response-serialization (serialize_http) logic out of the generated operation file and into a new hand-written module crates/s3s/src/ops/post_object.rs. The generated file (ops/generated/post_object.rs) and the codegen emitter (codegen/src/v2/post_object.rs) are reduced to thin delegations. This establishes a clean generated-vs-hand-written boundary so that future changes to the POST-specific semantics (part of the broader #982 effort to stop modeling POST Object as a fork of PutObject) can be made without touching the generator. Observable wire behavior is unchanged.

Changes:

  • Move deserialize_http / serialize_http into hand-written ops/post_object.rs (as pub(crate) fns) and register the module in ops/mod.rs.
  • Thin the codegen emitter and the generated file so PostObject's methods delegate to the new module; drop the now-unused PutObject import from the generated file.
  • Add six unit tests covering the serialize branches (200 / 201 / unrecognized / default status, control-character rejection, unparsable redirect) that the wire suite cannot reach.
File Description
crates/​s3s/​src/​ops/​post_object.rs New hand-written module holding the moved parse/serialize functions plus six unit tests.
crates/​s3s/​src/​ops/​mod.rs Registers the new post_object module.
crates/​s3s/​src/​ops/​generated/​post_object.rs Generated PostObject methods now delegate to the hand-written module; unused PutObject import removed.
codegen/​src/​v2/​post_object.rs Emitter updated to produce the thin delegations and the trimmed import set.

I verified that the emitter output exactly matches the committed generated file (idempotent), all imports in both files remain used (the removed PutObject import is no longer referenced), the invalid_request!/s3_error! macros stay in scope via #[macro_use] on utils/error before ops, and all delegated paths resolve through pub use self::generated::*;. The new tests exercise the serialize branches not reachable by the wire suite. No issues were found.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Nugine added 2 commits October 9, 2026 02:05
The patch coverage of the moved module left the missing multipart form, the legacy redirect fallback, a malformed success_action_status and the XML body error path unexercised. Cover each directly; the moved functions stay byte for byte unchanged.

Assisted-by: DeepSeek Harness:deepseek-flash
parse_field_value is infallible for String, and a non-UTF-8 field is rejected by the multipart parser before the operation sees it, so the two redirect lookups have no reachable error path. Call them with a text value and show the rejection happens when the response is serialized instead.

Assisted-by: DeepSeek Harness:deepseek-flash
@Nugine
Nugine marked this pull request as ready for review October 8, 2026 18:27
@Nugine
Nugine added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit cd69f7e Oct 8, 2026
24 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.

2 participants