test(ios): reuse ResizingProcessor instead of a second transcoding mock - #637
Open
jkmassel wants to merge 10 commits into
Open
test(ios): reuse ResizingProcessor instead of a second transcoding mock#637jkmassel wants to merge 10 commits into
jkmassel wants to merge 10 commits into
Conversation
Both delivery paths could put bytes on the wire after the editor was gone. `EditorViewController.deinit` calls `stop()`, which cancels the in-flight connection tasks, but Swift cancellation is cooperative: the body read is an uninterruptible loop and a host's `processFile` need not check at all, so a request can reach delivery well after teardown. Whether it then actually reached WordPress rested entirely on URLSession noticing the cancellation. That is not a guarantee the server can rely on. `URLSessionProtocol` is public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack — `withCheckedThrowingContinuation` around a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up. Check cancellation explicitly before delivery in `processAndUpload` and before the passthrough forward, so the guarantee comes from this file rather than from the HTTP client's behavior. `uploadErrorResponse` already logs CancellationError quietly, and HTTPServer drops the response for a cancelled task. Not covered by a test: reaching the window deterministically means driving teardown between the parse and the delivery of a live socket request, and a timing-based approximation would be flaky without pinning the behavior.
`DefaultMediaUploader` reads as an implementation of a host-facing protocol — the "default" one, as against a host's. It is not. It is GutenbergKit's own HTTP client for the configured site: it performs the uploads no host took over, and it relays every media delete, because every attachment lives on that site no matter who delivered it. Rename it, and the `defaultUploader` parameters and properties that carry it, on both platforms. On iOS this also narrows two signatures. `passthroughResponse` and `handleDelete` took the whole `UploadContext` and touched only the client. Pass it directly. On the delete path that is more than tidiness: a deletion always relays to the configured site, never to a delegate. That was a convention the signature let you break; now the type won't. The three functions that keep the context genuinely need every field. Android's server holds the client as a constructor property rather than threading a context, so it needs the rename only.
Performing a media upload — and retrying it — should be a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does. Both go to the same configured site; the only difference is who executes the requests. `MediaUploadDelegate.uploadFile` doesn't offer that. A host performs the `POST /wp/v2/media` and returns the raw response it received — then the editor, reading that response, drives the `post-process` retries and the orphan cleanup behind it, through the WebView rather than the host's stack. A host that took over uploads to run them through its own networking still didn't own the retries. It also receives no form fields, so an attachment it uploads lands unattached to its post. Add `MediaUploader`, which owns the upload end to end: - `upload(_:)` returns the finished attachment or throws. There is no raw response left for the editor to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. - It receives a `MediaUpload` carrying the file, its metadata, the editor's non-file form fields (`post`, additionalData) and the request query (`?_embed`) — everything needed to reproduce a native request. - Fields are a `MediaUploadField` list rather than a dictionary, so repeated names (a `field[]` array) survive verbatim and in order. Purely additive. `uploadFile` still works and is marked deprecated, pointing hosts at the replacement; an uploader takes precedence when both are set. GutenbergKit's own build keeps one deprecation warning at the call site that supports the old hook — the marker exists to tell hosts to migrate, and supporting the hook until it is removed means calling it. With an uploader set, the delegate's metadata gate can no longer decline a file: the gate exists to skip a temp copy for a file the delegate won't touch, but an uploader takes over delivery for *every* file, so passing through would silently bypass it. Covered on both platforms. `MediaUploadServerTest` crosses Detekt's LargeClass threshold; baselined rather than split, which is its own change.
`MediaUploader` replaces it. Returning a raw response split one upload's HTTP across two owners — the host performed the `POST`, the editor drove the `post-process` retries and orphan cleanup behind it — and the hook received no form fields, so an attachment it uploaded landed unattached to its post. Neither is fixable while the hook returns a raw response, which is what the replacement changes. What is left is a clean division: a delegate transforms bytes and GutenbergKit owns delivery and its retries; a `MediaUploader` owns delivery and its retries entirely. There is no longer an in-between where the host performs the upload but the editor retries it. `handlesFile` no longer gates the temp copy for two callers, only for `processFile` — and only when no uploader is set, since an uploader takes over delivery for every file. `MediaUploadResponse` drops to internal on both platforms: `uploadFile` was the only public API that named it. BREAKING CHANGE: hosts implementing `uploadFile` must conform to `MediaUploader` instead. Hosts that only implement `processFile` / `handlesFile` are unaffected.
The protocol no longer uploads anything — the previous commit removed `uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now describes the one thing it can't do, and next to `MediaUploader` the two names read as variations on the same job rather than the two halves of a deliberate split. `MediaProcessor` says what is left: it transforms bytes, GutenbergKit delivers them. Mechanical throughout — the property becomes `mediaProcessor`, the server parameter `processor`, the file `MediaHandlers.swift` (it holds both protocols now), and Android's demo `DemoMediaProcessor`. Prose follows the types. The `weak_delegate` suppression added when the property became strong goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and `MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes beyond the name.
The closure form of `start` can't capture the object that owns the server: the closure has to exist before the server does, and retrofitting `self` would form `owner -> HTTPServer -> handler -> owner`, so the owner's deinit — and its `stop()` — would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is how MediaUploadServer is written today. Add an `HTTPRequestHandler` protocol and a `start` overload that takes one. The dependencies become stored properties and the request logic becomes instance methods. The protocol is deliberately not `AnyObject`-constrained: a struct conformer cannot participate in a reference cycle at all, so the ownership question doesn't arise. A final class works too, under the same leaf discipline HTTPServerDelegate already documents. The closure overload is unchanged and forwards to the same code path, so this is purely additive — no existing caller, test, or the debug server is affected. Request handling is mandatory, so it can't be a defaulted HTTPServerDelegate method the way optional customization points are; hence an overload rather than a new delegate requirement.
`MediaUploadServer` handled requests through static functions threading an `UploadContext` parameter through every call, because the closure form of `HTTPServer.start` can't capture the object that owns the server: the closure has to exist before the server does, and capturing `self` would form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer`, so `deinit` — and its `stop()` — would never run. The previous commit added `HTTPRequestHandler` for exactly this. The dependencies become stored properties on a `Handler` struct and the request logic becomes instance methods; a value type can't participate in a reference cycle, so the ownership question doesn't arise. Mechanically: `handleRequest` becomes `handle`, the functions that use the dependencies become instance methods, and the ones that don't (`attachmentId`, `relayResponse`, `uploadErrorResponse`, `formFields`) stay static. `UploadContext` goes away — `Handler` is what it was. Helpers outside the handler (`errorResponse`, `writeStream`, `sanitizeFilename`, `uploadsTempDirectory`) are qualified rather than moved. No behavior change: only this file is touched, and no test changed.
Setting a `mediaUploader` means the host is taking over uploads. With no site credentials the server would previously just not start, silently dropping the uploader — and its media deletes still need the internal media client to reach the configured site, since every attachment lives there no matter who delivered it. Starting anyway would give a server whose every delete 500s. So the behavior forks by intent. A `mediaProcessor` with no credentials leaves the server down and uploads fall to the default WebView path — there is nothing to deliver through, so nothing to process. A `mediaUploader` with no credentials is a configuration error and fails fast: `precondition` on iOS, `check` on Android. The iOS policy lives in `MediaServerCredentials` rather than `EditorViewController`, which is `#if canImport(UIKit)` and therefore absent from the macOS host — the one platform that can run Swift Testing's exit tests. Living outside the gate, the trap itself is testable, not just the predicate: two exit tests run it in a child process, and neutering the precondition fails both. Android's `check` is covered through `GutenbergView`, and neutering it fails those two as well.
`formFields` decodes each non-file form value as UTF-8, which substitutes U+FFFD on malformed input. That is lossless today, but only because of an invariant nothing in the code states or enforces: the sole client is the editor's browser FormData. The server binds to loopback behind a per-session token; a FormData string value is a USVString, already well-formed at append time; and its only way to carry arbitrary bytes is a Blob, which always gets a filename and is filtered out of `extraParts`. Write that down on both platforms, including the part that makes it matter — if it stops holding, the two platforms are lossy *differently*, so there is no single behavior that could be documented instead. Relaxing each platform's filename filter demonstrates it: for `ED A0 80` Swift yields three replacement characters under its maximal-subpart rule where Java's decoder yields one. Cover the partition rather than the decode, since the partition is what makes the invariant true: a request carrying a second, Blob-shaped part whose bytes are not valid UTF-8 must not surface that part in `fields`. A second test pins the other half — valid UTF-8 (emoji, non-Latin scripts) round-trips exactly, so real captions are unaffected. Both fail when the filter is relaxed, so neither is vacuous. Neither asserts what becomes of that second part — it is currently dropped rather than relayed, which is a separate open question. Also reword the raw-bytes comment on the re-encode path. "So a non-UTF-8 value is forwarded verbatim" read as though malformed values were expected, which made the two delivery paths look contradictory. The actual hazard is the failable decode returning nil and an obvious `?? ""` dropping the whole value; the reason to keep bytes is that the re-encode should stay byte-identical to the passthrough it stands in for. Finally, restore `attachmentId`'s doc comment, which an earlier commit in this series orphaned onto `formFields` when it inserted the helper above it.
`TranscodingProcessor` duplicated `ResizingProcessor` — same `.processed(_, mimeType: "video/mp4", filename: "clip.mp4")` result, one call site — and was the weaker of the two. It wrote to a fixed `$TMPDIR/clip.mp4` instead of a per-call UUID path inside the managed upload directory, and swallowed the write with `try?`, so a failed write still returned `.processed(<nonexistent URL>, …)` and the test passed green against a file that never existed. `ResizingProcessor` uses `try` and a unique path. Also drops `@unchecked Sendable` from `ThrowingUploader` and `DecliningProcessor`, which are stateless. The escape hatch is only needed by the mocks holding `NSLock`-guarded state; carrying it on stateless ones normalizes it as boilerplate, which is how a real race gets hidden later. `ContentTypeDeleteClient` keeps it — it subclasses an `@unchecked Sendable` class and must restate the conformance.
github-actions
Bot
added
the
[Type] Automated Testing
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/637")Built from 8bbf776 |
jkmassel
force-pushed
the
docs/media-field-decode-invariant
branch
from
September 9, 2026 00:58
7447607 to
7d96796
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #633. Follow-up from an adversarial review of #625.
What?
Deletes
TranscodingProcessorand points its one call site at the existingResizingProcessor. Drops@unchecked Sendablefrom two stateless mocks.Why?
TranscodingProcessorwas a duplicate ofResizingProcessor— same.processed(_, mimeType: "video/mp4", filename: "clip.mp4")result, one call site — and the weaker of the two on both counts that matter:$TMPDIR/clip.mp4rather than a per-call UUID path inside the managed upload directory.processAndUpload's cleanupdeferthen deletes that path, so the mock unlinks a process-wide filename it does not own. Verified by planting a sentinel:before: exists=true→after: exists=false.try?and still returned.processed(<URL>, …). Forcing the write to fail leaves the file absent and all four assertions still pass — the test cannot detect a regression where processing silently produces nothing.ResizingProcessorusestry.@unchecked SendableonThrowingUploaderandDecliningProcessorbought nothing — both are statelessfinal classes that satisfy the conformance on their own. The annotation belongs on the mocks holdingNSLock-guarded state; carrying it on stateless ones normalizes it as boilerplate, which is how an unsynchronized property gets added later without a diagnostic.How?
TranscodingProcessordeleted;processesForHostReleasedDelegateusesResizingProcessor.@unchecked Sendableremoved fromThrowingUploaderandDecliningProcessor.ContentTypeDeleteClientkeeps it — it subclasses an@unchecked Sendableclass and must restate the conformance.Mutation sensitivity is unchanged: against a weakly-held processor the swapped test still fails in all four places with the real symptom (
passthroughUploadCalled → true).Testing Instructions
swift test— 981 tests, host suite greenswift build --build-tests— zero warnings in the library and test targetsRelated
Two other findings from the same review are already fixed upstream in this stack: the off-main delegate release (
HTTPServer.stop()now clearsnewConnectionHandler) and the#WeakMutabilitywarning, both in #625.EditorViewControllerMediaLifetimeTests— vacuous for the same class of reason, since it never loads the editor — has been deleted on #625, where the file lived. That also removedLifetimeProbeDelegate, so the mock cleanup in this PR is the remainder.