From 046a8e918a8e71b1211ab1d71699153202851e8c Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:33:03 -0600 Subject: [PATCH 1/2] feat: add MediaUploader, for a host that owns the whole upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Additive for hosts: `uploadFile` still works and is marked deprecated, pointing them at the replacement, and an uploader takes precedence when both are set. Internally the upload server's startup gate widens to admit an uploader as well as a delegate. 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. --- android/Gutenberg/detekt-baseline.xml | 1 + .../org/wordpress/gutenberg/GutenbergView.kt | 37 +++- .../wordpress/gutenberg/MediaUploadServer.kt | 121 ++++++++++++- .../gutenberg/MediaUploadServerTest.kt | 120 ++++++++++++ .../Sources/EditorViewController.swift | 33 +++- .../Sources/Media/MediaUploadDelegate.swift | 100 +++++++++- .../Sources/Media/MediaUploadServer.swift | 65 ++++++- .../Media/MediaUploadServerTests.swift | 171 ++++++++++++++++++ 8 files changed, 620 insertions(+), 28 deletions(-) diff --git a/android/Gutenberg/detekt-baseline.xml b/android/Gutenberg/detekt-baseline.xml index 4f6c96915..ce3b4481a 100644 --- a/android/Gutenberg/detekt-baseline.xml +++ b/android/Gutenberg/detekt-baseline.xml @@ -11,6 +11,7 @@ ExplicitItLambdaParameter:EditorAssetsLibrary.kt$EditorAssetsLibrary${ str, it -> str + "%02x".format(it) } FunctionNaming:EditorURLCache.kt$EditorURLCache$private fun __store( response: EditorURLResponse, url: String, httpMethod: EditorHttpMethod, currentDate: Date ) LargeClass:GutenbergView.kt$GutenbergView : FrameLayout + LargeClass:MediaUploadServerTest.kt$MediaUploadServerTest LongMethod:FixtureTests.kt$FixtureTests$@Test fun `request parsing - all basic cases pass`() LongMethod:FixtureTests.kt$FixtureTests$@Test fun `request parsing - all incremental cases pass`() LongMethod:HTTPRequestParser.kt$HTTPRequestParser$fun append(data: ByteArray): Unit diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt index accb34f03..fee46d45b 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -123,14 +123,32 @@ class GutenbergView : FrameLayout { */ var mediaUploadDelegate: MediaUploadDelegate? = null set(value) { - check(!hasStartedLoading) { - "mediaUploadDelegate must be set before the editor loads (e.g. right " + - "after construction). It is captured when the page begins loading; " + - "setting it afterward has no effect." - } + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploadDelegate") } + field = value + } + + /** + * Takes over media upload on the host's own stack (background service, offline + * queue, resumable transport). Setting it makes the host own every upload and its + * whole lifecycle; GutenbergKit stays out of the network entirely for media. + * + * Same lifecycle rules as [mediaUploadDelegate]: set it before the editor loads, + * and this view owns it for its lifetime — so you needn't retain it yourself, just + * don't strongly retain this [GutenbergView] from your uploader. + * + * Takes precedence over the deprecated [MediaUploadDelegate.uploadFile]: with an + * uploader set, that hook is never called. + */ + var mediaUploader: MediaUploader? = null + set(value) { + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } field = value } + private fun lateMediaAssignmentMessage(name: String) = + "$name must be set before the editor loads (e.g. right after construction). " + + "It is captured when the page begins loading; setting it afterward has no effect." + @Volatile private var uploadServer: MediaUploadServer? = null /** @@ -676,10 +694,10 @@ class GutenbergView : FrameLayout { } private fun startUploadServer() { - // No delegate means nothing wants to customize uploads, so there's no reason - // to route them through the native server — leave it down and let uploads - // fall to the default WebView path. (Matches iOS.) - if (mediaUploadDelegate == null) return + // Nothing to route through the native server unless the host provided a + // delegate or an uploader — leave it down and let uploads fall to the default + // WebView path. (Matches iOS.) + if (mediaUploadDelegate == null && mediaUploader == null) return // The native upload server relays through InternalMediaClient, which needs a // site root and an auth header (every host provides one — the editor injects @@ -715,6 +733,7 @@ class GutenbergView : FrameLayout { uploadServer = MediaUploadServer( uploadDelegate = mediaUploadDelegate, internalClient = internalClient, + uploader = mediaUploader, cacheDir = context.cacheDir, scope = coroutineScope ) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index defbe3b38..71d8201ed 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -106,13 +106,97 @@ interface MediaUploadDelegate { * Upload a processed file to the remote WordPress site. * * Return the raw WordPress response (status code + body), which GutenbergKit - * relays to the editor unchanged, or null to use the default uploader. A host - * that uploads to WordPress should return the exact response it received so + * relays to the editor unchanged, or null to use the internal media client. A + * host that uploads to WordPress should return the exact response it received so * the editor sees a complete attachment object. + * + * Returning a raw response splits one upload's HTTP across two owners: you + * perform the POST, but the editor drives the `post-process` retries and orphan + * cleanup behind it, through the WebView rather than your stack. It also receives + * no form fields, so an attachment uploaded this way lands unattached to its post. + * Implement [MediaUploader] instead — it owns the upload end-to-end and receives a + * [MediaUpload] carrying the fields. */ + @Deprecated( + "Implement MediaUploader instead — it owns the upload's retries and receives the editor's form fields.", + ReplaceWith("MediaUploader") + ) suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null } +/** + * One of the editor's non-file form fields, as sent with a media upload. + * + * A named type rather than a pair so the field's meaning is legible at every call + * site, and so the type can gain members without a source break for every host. + * + * @property name The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + * @property value The field's value, decoded as UTF-8. + */ +data class MediaUploadField(val name: String, val value: String) + +/** + * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, + * its metadata, the editor's non-file form fields, and the request's query. + * + * @property file The file to upload — already processed, if a [MediaUploadDelegate] ran. + * @property mimeType The file's MIME type. + * @property filename The file's name. + * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — + * most importantly `post`, the parent post's ID, without which the attachment is + * created unattached. A list, not a map, so repeated field names (e.g. a `field[]` + * array) survive verbatim. Send each as a form part on your `POST /wp/v2/media`, in + * the given order. + * @property query The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), + * or empty. Carry it on your request so the editor gets the response it expects. + */ +data class MediaUpload( + val file: File, + val mimeType: String, + val filename: String, + val fields: List, + val query: String +) + +/** + * Takes over *performing* a media upload — on the host's own stack: its own + * networking (say, to log every request), a background service, an offline queue, a + * resumable transport, its own retry policy. + * + * This is a choice of *who executes the requests*, not where they go: an uploader and + * GutenbergKit's internal media client both target the same configured site. Setting + * [GutenbergView.mediaUploader] makes the host own that upload end-to-end — the + * request, its own retries, and its recovery and cleanup — with GutenbergKit out of + * the network entirely. Because the host does the retries itself, there's no raw + * response left for the editor to retry behind it. + */ +interface MediaUploader { + /** + * Upload a (possibly processed) file and return the finished WordPress attachment + * JSON the editor inserts — the same object a direct `POST /wp/v2/media` returns. + * Return only once the upload is genuinely done, or throw on terminal failure: a + * returned value is taken as a completed attachment, and there is no GutenbergKit + * recovery behind you. + * + * The [MediaUpload] carries the file plus the editor's form fields (e.g. `post`) + * and query — send them all so the created attachment matches a native upload + * rather than landing as an unattached orphan. + * + * That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side + * post-processing it returns a 5xx carrying the attachment's ID in + * `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't + * re-upload; drive `POST /wp/v2/media//post-process` to completion, the way + * core recovers its own uploads (up to 5 attempts), then return the finished + * attachment. + * + * Owning the upload means owning cleanup on the server too: if post-process can't + * be recovered, force-delete the orphan (`DELETE /wp/v2/media/?force=true`) + * before you throw, or it stays on the site — neither GutenbergKit nor the editor + * cleans up behind you. + */ + suspend fun upload(upload: MediaUpload): ByteArray +} + /** * A local HTTP server that receives file uploads from the WebView and routes * them through the native media processing pipeline. @@ -128,6 +212,7 @@ interface MediaUploadDelegate { internal class MediaUploadServer( private val uploadDelegate: MediaUploadDelegate?, private val internalClient: InternalMediaClient?, + private val uploader: MediaUploader? = null, cacheDir: File? = null, scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO @@ -247,6 +332,15 @@ internal class MediaUploadServer( * Deliberately narrow: this server relays media operations, not arbitrary * REST requests, so only a numeric attachment ID under `/media/` matches. */ + /** + * The editor's non-file form parts as ordered, UTF-8-decoded fields. + * + * A list rather than a map so repeated names (e.g. a `field[]` array) survive + * verbatim, in the order the editor sent them. + */ + private fun formFields(parts: List): List = + parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } + private fun attachmentIdFromPath(path: String): String? { val components = path.split("/").filter { it.isNotEmpty() } if (components.size != 2 || components[0] != "media") return null @@ -290,7 +384,10 @@ internal class MediaUploadServer( // like this. If not, forward the original upload to WordPress directly, // skipping a full temp-file copy of a file the delegate won't process or // upload (e.g. a video handed to an image-only delegate). - if (uploadDelegate?.handlesFile(mimeType, filename) != true) { + // An uploader takes over delivery for *every* file, so with one set there is no + // passthrough to fall to: only the delegate's metadata gate can decline a file, + // and only when no uploader is configured. + if (uploader == null && uploadDelegate?.handlesFile(mimeType, filename) != true) { return passthroughResponse(request, query) } @@ -462,7 +559,23 @@ internal class MediaUploadServer( } try { - // If the delegate provided its own upload, use that. + // An uploader owns delivery on the host's own stack and returns the finished + // attachment JSON (or throws); GutenbergKit relays that as a success and + // never runs its own recovery behind it. + uploader?.let { hostUploader -> + val upload = MediaUpload( + file = targetFile, + mimeType = targetMimeType, + filename = targetFilename, + fields = formFields(extraParts), + query = query + ) + return UploadResult.Uploaded(MediaUploadResponse(201, hostUploader.upload(upload))) + } + + // The deprecated delegate path: the host performs the POST but returns the + // raw response, leaving the editor to drive post-process recovery behind it. + @Suppress("DEPRECATION") uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { return UploadResult.Uploaded(it) } diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index ba8690037..f2797cb2f 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -168,6 +168,111 @@ class MediaUploadServerTest { assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) } + @Test + fun `an uploader performs the upload and its result is relayed`() { + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + uploadDelegate = null, internalClient = client, uploader = uploader, cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-uploader" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + assertTrue(response.body.contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + assertFalse(client.uploadCalled) + assertFalse(client.passthroughUploadCalled) + assertEquals("photo.jpg", uploader.received?.filename) + assertEquals("image/jpeg", uploader.received?.mimeType) + } + + @Test + fun `an uploader receives the editor's form fields in order, and the query`() { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a map. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + uploadDelegate = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-fields" + val body = java.io.ByteArrayOutputStream().apply { + for ((name, value) in listOf("post" to "42", "tags[]" to "a", "tags[]" to "b")) { + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"$name\"\r\n\r\n".toByteArray()) + write("$value\r\n".toByteArray()) + } + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload?_embed=wp:featuredmedia", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals( + listOf( + MediaUploadField("post", "42"), + MediaUploadField("tags[]", "a"), + MediaUploadField("tags[]", "b") + ), + uploader.received?.fields + ) + assertEquals("?_embed=wp:featuredmedia", uploader.received?.query) + } + + @Test + fun `an uploader sees a file the delegate's metadata gate would have declined`() { + // The gate exists to skip a temp copy for a file the delegate won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + uploadDelegate = DecliningDelegate(), internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-declined" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals("clip.mov", uploader.received?.filename) + assertFalse(client.passthroughUploadCalled) + } + @Test fun `routes upload with a query string and relays the query`() { val delegate = ProcessOnlyDelegate() @@ -830,6 +935,21 @@ class MediaUploadServerTest { ) } + /** Records the [MediaUpload] it is handed, and returns a finished attachment. */ + private class RecordingUploader : MediaUploader { + @Volatile var received: MediaUpload? = null + + override suspend fun upload(upload: MediaUpload): ByteArray { + received = upload + return """{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray() + } + } + + /** A delegate that declines every file by metadata. */ + private class DecliningDelegate : MediaUploadDelegate { + override fun handlesFile(mimeType: String, filename: String) = false + } + private class MockInternalMediaClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 269d21b72..a1690d737 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -143,6 +143,22 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // swiftlint:disable:next weak_delegate public private(set) var mediaUploadDelegate: (any MediaUploadDelegate)? + /// Takes over media upload on the host's own stack (background session, offline + /// queue, resumable transport). Passing one makes the host own every upload and its + /// whole lifecycle; GutenbergKit stays out of the network entirely for media. + /// + /// Same ownership rules as ``mediaUploadDelegate``: supplied at `init`, held for the + /// editor's lifetime, and not conformed by the object that owns the editor. + /// + /// Reuse is the expected shape here, more so than for a delegate: the transports this + /// exists for outlive any one editor by definition — a background `URLSession` has a + /// fixed identifier and must survive app relaunch, an offline queue spans sessions. + /// Build the uploader once, hold it, and pass the same instance to each editor. + /// + /// Takes precedence over the deprecated ``MediaUploadDelegate/uploadFile(at:mimeType:filename:)``: + /// with an uploader set, that hook is never called. + public private(set) var mediaUploader: (any MediaUploader)? + // MARK: - Private Properties (Services) private let editorService: EditorService private let httpClient: any EditorHTTPClientProtocol @@ -206,6 +222,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// object carrying the settings it needs. If you must write the retaining shape, /// call ``stopMediaHandling()`` when you are done. To reuse one delegate across /// editors, keep your own reference — the editor drops only its own when it goes. + /// - mediaUploader: Takes over media upload on the host's own stack. Same ownership + /// rules as `mediaUploadDelegate`. /// - httpClient: Replaces the client used for editor and media requests. /// - isWarmupMode: Loads the editor shell without dependencies, to warm WebKit. public init( @@ -213,6 +231,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro dependencies: EditorDependencies? = nil, mediaPicker: MediaPickerController? = nil, mediaUploadDelegate: (any MediaUploadDelegate)? = nil, + mediaUploader: (any MediaUploader)? = nil, httpClient: EditorHTTPClient? = nil, isWarmupMode: Bool = false ) { @@ -231,6 +250,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker self.mediaUploadDelegate = mediaUploadDelegate + self.mediaUploader = mediaUploader self.lockdownModeMonitor = LockdownModeMonitor() self.controller = GutenbergEditorController(configuration: configuration, lockdownModeMonitor: self.lockdownModeMonitor) @@ -344,7 +364,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro } /// Releases the editor's media handling: stops the local upload server, drops the - /// host's ``mediaUploadDelegate``, and withdraws the upload endpoint from the page. + /// host's ``mediaUploadDelegate`` and ``mediaUploader``, and withdraws the upload + /// endpoint from the page. /// /// Most hosts never need this. Releasing the editor runs `deinit`, which does the /// same work. It is only required when the delegate holds the editor back — which @@ -388,6 +409,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro uploadServer?.stop() uploadServer = nil mediaUploadDelegate = nil + mediaUploader = nil revokeNativeUploadEndpoint() } @@ -557,10 +579,10 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { // Nothing to route through the native server unless the host provided a - // delegate. The editor owns it — `mediaUploadDelegate` is strong — so there's - // no released-before-load case to guard against; it lives as long as the - // editor does. - guard mediaUploadDelegate != nil else { + // delegate or an uploader. The editor owns whichever it was given — both + // properties are strong — so there's no released-before-load case to guard + // against; they live as long as it does. + guard mediaUploadDelegate != nil || mediaUploader != nil else { return } @@ -588,6 +610,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro do { let server = try await MediaUploadServer.start( uploadDelegate: mediaUploadDelegate, + uploader: mediaUploader, internalClient: internalClient ) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 73752166b..8c7f791c6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -78,9 +78,18 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// Upload a processed file to the remote WordPress site. /// /// Return the raw WordPress response (status code + body), which GutenbergKit - /// relays to the editor unchanged, or `nil` to use the default uploader. A + /// relays to the editor unchanged, or `nil` to use the internal media client. A /// host that uploads to WordPress should return the exact response it /// received so the editor sees a complete attachment object. + /// + /// - Warning: Returning a raw response splits one upload's HTTP across two + /// owners — you perform the `POST`, but GutenbergKit's editor drives the + /// `post-process` retries and orphan cleanup behind it, through the WebView + /// rather than your stack. It also receives no form fields, so an attachment + /// uploaded this way lands unattached to its post. Conform to ``MediaUploader`` + /// instead: it owns the upload end-to-end and receives a ``MediaUpload`` + /// carrying the fields. + @available(*, deprecated, message: "Conform to MediaUploader instead — it owns the upload's retries and receives the editor's form fields.") func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? } @@ -94,7 +103,96 @@ extension MediaUploadDelegate { .original } + @available(*, deprecated, message: "Conform to MediaUploader instead — it owns the upload's retries and receives the editor's form fields.") public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { nil } } + +/// One of the editor's non-file form fields, as sent with a media upload. +/// +/// A named type rather than a `(name, value)` tuple: tuples are not nominal, so a +/// tuple-typed property would permanently block `Equatable`/`Hashable`/`Codable` +/// synthesis on ``MediaUpload`` — including inside GutenbergKit, and not fixable +/// later without a source break for every host. +public struct MediaUploadField: Sendable, Hashable, Codable { + /// The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + public let name: String + + /// The field's value, decoded as UTF-8. + public let value: String + + public init(name: String, value: String) { + self.name = name + self.value = value + } +} + +/// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to +/// send, its metadata, the editor's non-file form fields, and the request's query. +public struct MediaUpload: Sendable { + /// The file to upload — already processed, if a ``MediaUploadDelegate`` ran. + public let fileURL: URL + + /// The file's MIME type. + public let mimeType: String + + /// The file's name. + public let filename: String + + /// The editor's non-file form fields, in order, each decoded as UTF-8 — most + /// importantly `post`, the parent post's ID, without which the attachment is + /// created unattached. A list, not a dictionary, so repeated field names (e.g. a + /// `field[]` array) survive verbatim. Send each as a form part on your + /// `POST /wp/v2/media`, in the given order. + public let fields: [MediaUploadField] + + /// The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), or + /// empty. Carry it on your request so the editor gets the response it expects. + public let query: String + + public init(fileURL: URL, mimeType: String, filename: String, fields: [MediaUploadField], query: String) { + self.fileURL = fileURL + self.mimeType = mimeType + self.filename = filename + self.fields = fields + self.query = query + } +} + +/// Takes over *performing* a media upload — on the host's own stack: its own +/// networking (say, to log every request), a background session, an offline queue, +/// a resumable transport, its own retry policy. +/// +/// This is a choice of *who executes the requests*, not where they go: an uploader +/// and GutenbergKit's internal media client both target the same configured site. +/// Setting ``EditorViewController/mediaUploader`` makes the host own that upload +/// end-to-end — the request, its own retries, and its recovery and cleanup — with +/// GutenbergKit out of the network entirely. Because the host does the retries +/// itself, there's no raw response left for the editor to retry behind it. The +/// attachment you return lives on that same configured site, where the editor reads +/// and updates it by ID. +public protocol MediaUploader: AnyObject, Sendable { + /// Upload a (possibly processed) file and return the finished WordPress + /// attachment JSON the editor inserts — the same object a direct + /// `POST /wp/v2/media` returns. Return only once the upload is genuinely done, + /// or `throw` on terminal failure: a returned value is taken as a completed + /// attachment, and there is no GutenbergKit recovery behind you. + /// + /// The ``MediaUpload`` carries the file plus the editor's form fields (e.g. + /// `post`) and query — send them all so the created attachment matches a native + /// upload rather than landing as an unattached orphan. + /// + /// That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side + /// post-processing it returns a 5xx carrying the attachment's ID in + /// `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't + /// re-upload; drive `POST /wp/v2/media//post-process` to completion, the way + /// core recovers its own uploads (up to 5 attempts), then return the finished + /// attachment. + /// + /// Owning the upload means owning cleanup on the server too: if post-process + /// can't be recovered, force-delete the orphan + /// (`DELETE /wp/v2/media/?force=true`) before you `throw`, or it stays on the + /// site — neither GutenbergKit nor the editor cleans up behind you. + func upload(_ upload: MediaUpload) async throws -> Data +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index ed58ad69e..63b573cbb 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -30,12 +30,14 @@ final class MediaUploadServer: Sendable { /// /// - Parameters: /// - uploadDelegate: Optional delegate for customizing file processing and upload. + /// - uploader: Optional host uploader that performs the upload on its own stack. /// - internalClient: GutenbergKit's own client for the configured site. Delivers - /// uploads when no delegate provides `uploadFile`, and every media delete. + /// uploads when no host uploader or delegate does, and every media delete. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( uploadDelegate: (any MediaUploadDelegate)? = nil, + uploader: (any MediaUploader)? = nil, internalClient: InternalMediaClient? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { @@ -46,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, internalClient: internalClient) + let context = UploadContext(uploadDelegate: uploadDelegate, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -182,11 +184,16 @@ final class MediaUploadServer: Sendable { let filename = filePart.filename ?? "upload" let mimeType = filePart.contentType - // Ask the delegate — from metadata alone — whether it will touch a file - // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process or - // upload (e.g. a video handed to an image-only delegate). - guard context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false else { + // Ask the delegate — from metadata alone — whether it will touch a file like + // this. If not, forward the original upload to WordPress directly, skipping a + // full temp-file copy of a file the delegate won't process (e.g. a video handed + // to an image-only delegate). + // + // An uploader takes over delivery for *every* file, so with one set there is + // no passthrough to fall to: only the delegate's metadata gate can decline a + // file, and only when no uploader is configured. + let delegateWantsFile = context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false + guard context.uploader != nil || delegateWantsFile else { do { return try await passthroughResponse(request, query: query, internalClient: context.internalClient) } catch { @@ -260,6 +267,29 @@ final class MediaUploadServer: Sendable { /// /// Deliberately narrow: this server relays media operations, not arbitrary /// REST requests, so only a numeric attachment ID under `/media/` matches. + /// The editor's non-file form parts as ordered, UTF-8-decoded fields. + /// + /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) + /// survive verbatim, in the order the editor sent them. + private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { + var fields: [MediaUploadField] = [] + for part in parts { + fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) + } + return fields + } + + /// Calls the deprecated `uploadFile` hook from one place. + /// + /// This deliberately leaves one deprecation warning in GutenbergKit's own build: + /// the marker exists to tell *hosts* to migrate, and supporting the hook until it + /// is removed means calling it. The warning marks the code that goes with it. + private static func deprecatedUploadFile( + _ delegate: any MediaUploadDelegate, _ url: URL, _ mimeType: String, _ filename: String + ) async throws -> MediaUploadResponse? { + try await delegate.uploadFile(at: url, mimeType: mimeType, filename: filename) + } + private static func attachmentId(fromPath path: String) -> String? { let components = path.split(separator: "/", omittingEmptySubsequences: true) guard components.count == 2, components[0] == "media" else { return nil } @@ -380,9 +410,25 @@ final class MediaUploadServer: Sendable { // keeps this true for a host-injected `URLSessionProtocol` that doesn't. try Task.checkCancellation() - // Step 2: Upload to remote WordPress + // Step 2: deliver. An uploader owns delivery on the host's own stack and + // returns the finished attachment JSON (or throws); GutenbergKit relays that + // as a success and never runs its own recovery behind it. + if let uploader = context.uploader { + let upload = MediaUpload( + fileURL: uploadURL, + mimeType: uploadMimeType, + filename: uploadFilename, + fields: try await formFields(from: extraParts), + query: query + ) + let attachment = try await uploader.upload(upload) + return .uploaded(MediaUploadResponse(statusCode: 201, body: attachment)) + } + + // The deprecated delegate path: the host performs the POST but returns the raw + // response, leaving the editor to drive post-process recovery behind it. if let delegate = context.uploadDelegate, - let result = try await delegate.uploadFile(at: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) { + let result = try await deprecatedUploadFile(delegate, uploadURL, uploadMimeType, uploadFilename) { return .uploaded(result) } else if let internalClient = context.internalClient { // Unmodified — forward the original request body directly, skipping @@ -543,6 +589,7 @@ enum UploadError: Error, LocalizedError { /// protocol and `InternalMediaClient` is `@unchecked Sendable`. private struct UploadContext: Sendable { let uploadDelegate: (any MediaUploadDelegate)? + let uploader: (any MediaUploader)? let internalClient: InternalMediaClient? } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 0a3294326..a1cddd3c0 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -385,6 +385,147 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } + @Test("an uploader performs the upload and its result is relayed") + func uploaderPerformsUpload() async throws { + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (data, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 201) + #expect(String(decoding: data, as: UTF8.self).contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + #expect(uploader.received?.filename == "photo.jpg") + #expect(uploader.received?.mimeType == "image/jpeg") + } + + @Test("an uploader receives the editor's form fields in order, and the query") + func uploaderReceivesFieldsAndQuery() async throws { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a dictionary. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + for (name, value) in [("post", "42"), ("tags[]", "a"), ("tags[]", "b")] { + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"\(name)\"\r\n\r\n") + body.append("\(value)\r\n") + } + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n") + body.append("Content-Type: image/jpeg\r\n\r\n") + body.append(Data("fake image data".utf8)) + body.append("\r\n--\(boundary)--\r\n") + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + let received = try #require(uploader.received) + #expect(received.fields == [ + MediaUploadField(name: "post", value: "42"), + MediaUploadField(name: "tags[]", value: "a"), + MediaUploadField(name: "tags[]", value: "b"), + ]) + #expect(received.query == "?_embed=wp:featuredmedia") + } + + @Test("an uploader takes precedence over the deprecated uploadFile hook") + func uploaderWinsOverDeprecatedHook() async throws { + let delegate = MockUploadDelegate() + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The delegate still processes; only delivery moves to the uploader. + #expect(delegate.processFileCalled) + #expect(!delegate.uploadFileCalled) + #expect(uploader.received != nil) + } + + @Test("an uploader sees a file the delegate's metadata gate would have declined") + func uploaderSeesDeclinedFile() async throws { + // The gate exists to skip a temp copy for a file the delegate won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + let delegate = DecliningDelegate() + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + #expect(uploader.received?.filename == "clip.mov") + #expect(!internalClient.passthroughUploadCalled) + } + + @Test("an uploader that throws surfaces as a failure, with no GutenbergKit retry") + func uploaderThrowSurfaces() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: ThrowingUploader(), internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 500) + // Recovery is the uploader's, not GutenbergKit's — it must not re-deliver. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + } + @Test("retains the delegate for the server's lifetime, and releases it after") func retainsDelegateForServerLifetime() async throws { weak var weakDelegate: MockUploadDelegate? @@ -873,6 +1014,36 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks +/// Records the ``MediaUpload`` it is handed, and returns a finished attachment. +private final class RecordingUploader: MediaUploader, @unchecked Sendable { + private let lock = NSLock() + private var _received: MediaUpload? + + var received: MediaUpload? { lock.withLock { _received } } + + func upload(_ upload: MediaUpload) async throws -> Data { + lock.withLock { _received = upload } + return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8) + } +} + +/// An uploader whose delivery fails terminally, as one would after exhausting its own +/// post-process recovery and force-deleting the orphan. +private final class ThrowingUploader: MediaUploader, @unchecked Sendable { + struct Failure: Error {} + + func upload(_ upload: MediaUpload) async throws -> Data { + throw Failure() + } +} + +/// A delegate that declines every file by metadata. +private final class DecliningDelegate: MediaUploadDelegate, @unchecked Sendable { + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + false + } +} + /// A delegate that transcodes, used to check the server holds it across the whole /// request rather than re-reading a reference the host may have dropped. private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable { From 204040ecc726dc49a7314c7e0bf20fa7e8d8f3a4 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:18:04 -0600 Subject: [PATCH 2/2] fix: don't hand a declined file to processFile, and close the gaps around it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to the MediaUploader commit: a behavior bug in the metadata gate, a cross-platform divergence, a missing cancellation check on Android, four documentation defects, two test gaps, and a shadowed local. - `processFile` ran on a file the delegate's metadata gate had declined. Widening the gate to `uploader != nil || delegateWantsFile` left `processFile` called unconditionally, so an image-only delegate paired with an uploader was handed the `.mov` it had just said it won't touch — breaking the contract `handlesFile` documents. `delegateWantsFile` is now carried into `processAndUpload` and gates `processFile`. With an uploader set the file is still delivered; it just skips processing on the way. - iOS evaluated `handlesFile` eagerly while Android's `&&` short-circuited past it, so the same host saw one callback per upload on iOS and zero on Android. Android now binds it eagerly too: asked exactly once per upload on both. - Android had no pre-flight cancellation check before handing work to the host uploader, where iOS has `Task.checkCancellation()`. Added `currentCoroutineContext().ensureActive()`, so a torn-down editor no longer starts an upload whose attachment nobody would clean up. - The recovery recipe omitted `post-process`'s required `action` parameter. Core registers `action` as required, so a host following the doc verbatim would 400 five times and then run the doc's *other* instruction — `DELETE /wp/v2/media/?force=true` — destroying an attachment `wp_update_image_subsizes()` would have recovered. - `mediaUploader`'s doc had been appended to `mediaUploadDelegate`'s `///` block, merging the two: `mediaUploadDelegate` shipped with no documentation and `mediaUploader` opened by describing a delegate. Confirmed with `swiftc -emit-symbol-graph` (`mediaUploadDelegate => None`); both now bind their own 11 lines. - `formFields` and `deprecatedUploadFile` were inserted between `attachmentId(fromPath:)`'s doc and its declaration — merging into it on iOS, dropping it outright on Android — costing the "deliberately narrow, not a general REST proxy" rationale. Moved below their only caller, per AGENTS.md's call-order rule. - `ReplaceWith("MediaUploader")` takes a replacement *expression*; applying the quick-fix drops all three arguments and leaves a type name where a `MediaUploadResponse?` was expected. Removed, with a note so it doesn't return. - Nothing pinned the Android gate or the uploader/deprecated-hook precedence: deleting `&& mediaUploader == null` or reordering the two delivery paths left the suite green. Three tests added; both mutations now fail. - `DecliningDelegate` duplicated the pre-existing `DeclineByMetadataDelegate` minus its `processFileCalled` recorder — the one probe that catches the `processFile` bug above. Merged, and the declined-file test now asserts it. - Two locals named `uploader` shadowed the new `MediaUploader` property, silently (kotlinc has no diagnostic for it, detekt no rule). Renamed to `client`. - `RecordingUploader`'s fixture carried no `title`, so the repo's only worked example of an uploader result was a body that trips `transformAttachment`. --- .../wordpress/gutenberg/MediaUploadServer.kt | 82 ++++++++++++------ .../GutenbergViewUploadServerTest.kt | 33 +++++++ .../gutenberg/MediaUploadServerTest.kt | 55 ++++++++++-- .../Sources/Media/MediaUploadDelegate.swift | 4 +- .../Sources/Media/MediaUploadServer.swift | 85 ++++++++++--------- .../Media/MediaUploadServerTests.swift | 24 +++--- 6 files changed, 197 insertions(+), 86 deletions(-) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index 71d8201ed..27c58f2f4 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -6,6 +6,8 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.cancel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import kotlin.coroutines.resume @@ -117,9 +119,12 @@ interface MediaUploadDelegate { * Implement [MediaUploader] instead — it owns the upload end-to-end and receives a * [MediaUpload] carrying the fields. */ + // No ReplaceWith: it takes a replacement *expression* the IDE substitutes for the + // call, and there is none that means "implement a different interface" — the + // quick-fix would drop the arguments and leave a type name where a + // MediaUploadResponse? was expected. The message carries the guidance instead. @Deprecated( - "Implement MediaUploader instead — it owns the upload's retries and receives the editor's form fields.", - ReplaceWith("MediaUploader") + "Implement MediaUploader instead — it owns the upload's retries and receives the editor's form fields." ) suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null } @@ -187,7 +192,9 @@ interface MediaUploader { * `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't * re-upload; drive `POST /wp/v2/media//post-process` to completion, the way * core recovers its own uploads (up to 5 attempts), then return the finished - * attachment. + * attachment. That request needs a body of `{"action": "create-image-subsizes"}` — + * core registers `action` as **required**, so a post-process request without it + * fails with a 400 every time rather than recovering. * * Owning the upload means owning cleanup on the server too: if post-process can't * be recovered, force-delete the orphan (`DELETE /wp/v2/media/?force=true`) @@ -332,15 +339,6 @@ internal class MediaUploadServer( * Deliberately narrow: this server relays media operations, not arbitrary * REST requests, so only a numeric attachment ID under `/media/` matches. */ - /** - * The editor's non-file form parts as ordered, UTF-8-decoded fields. - * - * A list rather than a map so repeated names (e.g. a `field[]` array) survive - * verbatim, in the order the editor sent them. - */ - private fun formFields(parts: List): List = - parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } - private fun attachmentIdFromPath(path: String): String? { val components = path.split("/").filter { it.isNotEmpty() } if (components.size != 2 || components[0] != "media") return null @@ -358,9 +356,9 @@ internal class MediaUploadServer( * browser blocks it at preflight. Relaying it here lets the cleanup run. */ private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = internalClient ?: return errorResponse(500, "No internal media client configured") + val client = internalClient ?: return errorResponse(500, "No internal media client configured") return try { - relayResponse(uploader.deleteMedia(attachmentId, query)) + relayResponse(client.deleteMedia(attachmentId, query)) } catch (e: IOException) { Log.e(TAG, "Media deletion failed", e) errorResponse(500, e.message ?: "Deletion failed") @@ -385,16 +383,20 @@ internal class MediaUploadServer( // skipping a full temp-file copy of a file the delegate won't process or // upload (e.g. a video handed to an image-only delegate). // An uploader takes over delivery for *every* file, so with one set there is no - // passthrough to fall to: only the delegate's metadata gate can decline a file, - // and only when no uploader is configured. - if (uploader == null && uploadDelegate?.handlesFile(mimeType, filename) != true) { + // passthrough to fall to and the gate can't decline the upload outright. It + // still decides whether processFile runs, though — a declined file is handed to + // the uploader unprocessed rather than to a delegate that said it won't touch it + // — so the answer is carried into processAndUpload rather than short-circuited + // away here. Asked exactly once per upload, matching iOS. + val delegateWantsFile = uploadDelegate?.handlesFile(mimeType, filename) == true + if (uploader == null && !delegateWantsFile) { return passthroughResponse(request, query) } val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") - return processAndRespond(request, tempFile, filePart, extraParts, query) + return processAndRespond(request, tempFile, filePart, extraParts, query, delegateWantsFile) } @Suppress("TooGenericExceptionCaught") @@ -478,11 +480,12 @@ internal class MediaUploadServer( @Suppress("TooGenericExceptionCaught") private suspend fun processAndRespond( request: HttpRequest, tempFile: File, filePart: MultipartPart, - extraParts: List, query: String + extraParts: List, query: String, delegateWantsFile: Boolean ): HttpResponse { try { val uploadResult = processAndUpload( - tempFile, filePart.contentType, filePart.filename ?: "upload", extraParts, query + tempFile, filePart.contentType, filePart.filename ?: "upload", + extraParts, query, delegateWantsFile ) val response = when (uploadResult) { is UploadResult.Uploaded -> { @@ -527,18 +530,29 @@ internal class MediaUploadServer( private suspend fun performPassthroughUpload(request: HttpRequest, query: String): MediaUploadResponse { val body = request.body val contentType = request.header("Content-Type") - val uploader = internalClient - if (body == null || contentType == null || uploader == null) { - throw MediaUploadException("Passthrough upload requires a request body, Content-Type, and internal media client") + val client = internalClient + if (body == null || contentType == null || client == null) { + throw MediaUploadException( + "Passthrough upload requires a request body, Content-Type, and internal media client" + ) } - return uploader.passthroughUpload(body, contentType, query) + return client.passthroughUpload(body, contentType, query) } private suspend fun processAndUpload( file: File, mimeType: String, filename: String, - extraParts: List, query: String + extraParts: List, query: String, delegateWantsFile: Boolean ): UploadResult { - val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + // Process (resize, transcode, etc.) — but only for a file the delegate's + // metadata gate accepted. handlesFile returning false is the delegate saying it + // won't touch a file like this, so handing it one anyway would break the + // contract the gate documents. With an uploader set the file still gets + // delivered; it just skips processing on its way there. + val processed = if (delegateWantsFile) { + uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + } else { + ProcessedProxyFile.Original + } // Resolve the file to upload and its metadata. Processed uses the // delegate's values verbatim, so a format change is reported to WordPress. @@ -559,6 +573,13 @@ internal class MediaUploadServer( } try { + // The editor was torn down (or the client disconnected) while we processed. + // Don't put an upload on the wire whose response nobody will read — it would + // create an attachment neither GutenbergKit nor the host knows to clean up. + // Checking here rather than relying on the delivery path to notice keeps this + // true for a host uploader that isn't cancellation-cooperative. (Matches iOS.) + currentCoroutineContext().ensureActive() + // An uploader owns delivery on the host's own stack and returns the finished // attachment JSON (or throws); GutenbergKit relays that as a success and // never runs its own recovery behind it. @@ -598,6 +619,15 @@ internal class MediaUploadServer( } } + /** + * The editor's non-file form parts as ordered, UTF-8-decoded fields. + * + * A list rather than a map so repeated names (e.g. a `field[]` array) survive + * verbatim, in the order the editor sent them. + */ + private fun formFields(parts: List): List = + parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } + // MARK: - Response Building private fun errorResponse(status: Int, message: String): HttpResponse { diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt index a83cfe5f5..5db7e718a 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -94,6 +94,39 @@ class GutenbergViewUploadServerTest { } } + @Test + fun `the upload server starts for an uploader with no delegate`() { + val view = makeView() + try { + // An uploader alone must bring the server up: it is the only route the + // editor has to the host's upload stack. Without this, `startUploadServer` + // could drop the `mediaUploader` clause from its gate and stay green. + view.mediaUploader = mock(MediaUploader::class.java) + startLoading(view) + idle() + assertNotNull( + "an uploader provided before load should bring up the upload server", + uploadServerOf(view) + ) + } finally { + detach(view) // stops the server, releasing the bound socket + } + } + + @Test + fun `setting the uploader after the page has started loading throws`() { + val view = makeView() + try { + startLoading(view) + idle() + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + @Test fun `setting the delegate after the page has started loading throws`() { val view = makeView() diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index f2797cb2f..3e43d0902 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -244,6 +244,37 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", uploader.received?.query) } + @Test + fun `an uploader takes precedence over the deprecated uploadFile hook`() { + // Both set: the uploader owns delivery and the deprecated hook must not run. + // The delegate still processes — only delivery moves to the uploader. + val uploader = RecordingUploader() + val delegate = MockUploadDelegate() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + uploadDelegate = delegate, internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-precedence" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "data".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertNotNull(uploader.received) + assertFalse(delegate.uploadFileCalled) + assertTrue(delegate.processFileCalled) + assertFalse(client.uploadCalled) + } + @Test fun `an uploader sees a file the delegate's metadata gate would have declined`() { // The gate exists to skip a temp copy for a file the delegate won't touch. An @@ -251,9 +282,10 @@ class MediaUploadServerTest { // silently bypass it. val uploader = RecordingUploader() val client = MockInternalMediaClient() + val delegate = DeclineByMetadataDelegate() server.stop() server = MediaUploadServer( - uploadDelegate = DecliningDelegate(), internalClient = client, uploader = uploader, + uploadDelegate = delegate, internalClient = client, uploader = uploader, cacheDir = tempFolder.root ) @@ -271,6 +303,9 @@ class MediaUploadServerTest { assertEquals("clip.mov", uploader.received?.filename) assertFalse(client.passthroughUploadCalled) + // ...but a declined file must still not reach processFile: handlesFile + // returning false is the delegate saying it won't touch a file like this. + assertFalse(delegate.processFileCalled) } @Test @@ -873,6 +908,7 @@ class MediaUploadServerTest { return ProcessedProxyFile.Original } + @Suppress("OVERRIDE_DEPRECATION") override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? { uploadFileCalled = true lastFilename = filename @@ -891,8 +927,9 @@ class MediaUploadServerTest { } /** - * Declines every file by metadata via [handlesFile], so the server must pass - * through without materializing the file or calling [processFile]. + * Declines every file by metadata via [handlesFile]. With no uploader the server + * must pass through without materializing the file; with one, delivery still + * happens but [processFile] must not be called. [processFileCalled] pins both. */ private class DeclineByMetadataDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false @@ -941,15 +978,15 @@ class MediaUploadServerTest { override suspend fun upload(upload: MediaUpload): ByteArray { received = upload - return """{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray() + // Shaped like a real attachment: the editor's `transformAttachment` + // reads `title.raw`, so an example without it would model a body that + // fails in the editor. + val attachment = """{"id":7,"source_url":"https://example.com/photo.jpg",""" + + """"media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}""" + return attachment.toByteArray() } } - /** A delegate that declines every file by metadata. */ - private class DecliningDelegate : MediaUploadDelegate { - override fun handlesFile(mimeType: String, filename: String) = false - } - private class MockInternalMediaClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 8c7f791c6..a5bfb7d5b 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -188,7 +188,9 @@ public protocol MediaUploader: AnyObject, Sendable { /// `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't /// re-upload; drive `POST /wp/v2/media//post-process` to completion, the way /// core recovers its own uploads (up to 5 attempts), then return the finished - /// attachment. + /// attachment. That request needs a body of `{"action": "create-image-subsizes"}` + /// — core registers `action` as **required**, so a post-process request without + /// it fails with a 400 every time rather than recovering. /// /// Owning the upload means owning cleanup on the server too: if post-process /// can't be recovered, force-delete the orphan diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 63b573cbb..634492f3f 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -190,8 +190,11 @@ final class MediaUploadServer: Sendable { // to an image-only delegate). // // An uploader takes over delivery for *every* file, so with one set there is - // no passthrough to fall to: only the delegate's metadata gate can decline a - // file, and only when no uploader is configured. + // no passthrough to fall to and the gate can't decline the upload outright. + // It still decides whether `processFile` runs, though — a declined file is + // handed to the uploader unprocessed rather than to a delegate that said it + // won't touch it — so the answer is carried into `processAndUpload` rather + // than discarded here. Asked exactly once per upload, matching Android. let delegateWantsFile = context.uploadDelegate?.handlesFile(ofType: mimeType, named: filename) ?? false guard context.uploader != nil || delegateWantsFile else { do { @@ -201,10 +204,10 @@ final class MediaUploadServer: Sendable { } } - // The delegate wants the file. Stream the part body to a dedicated temp - // file for it — the library's RequestBody may be a byte-range slice of a - // larger temp file whose lifecycle is tied to ARC, so the delegate needs a - // standalone file that outlives the handler return. + // Someone wants the file — the delegate, the uploader, or both. Stream the + // part body to a dedicated temp file for them: the library's RequestBody may + // be a byte-range slice of a larger temp file whose lifecycle is tied to ARC, + // so they need a standalone file that outlives the handler return. let tempDir = uploadsTempDirectory try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) @@ -226,7 +229,8 @@ final class MediaUploadServer: Sendable { do { let uploadResult = try await processAndUpload( fileURL: fileURL, mimeType: mimeType, filename: filename, - extraParts: extraParts, query: query, context: context + extraParts: extraParts, query: query, + delegateWantsFile: delegateWantsFile, context: context ) switch uploadResult { case .uploaded(let uploaded): @@ -267,29 +271,6 @@ final class MediaUploadServer: Sendable { /// /// Deliberately narrow: this server relays media operations, not arbitrary /// REST requests, so only a numeric attachment ID under `/media/` matches. - /// The editor's non-file form parts as ordered, UTF-8-decoded fields. - /// - /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) - /// survive verbatim, in the order the editor sent them. - private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { - var fields: [MediaUploadField] = [] - for part in parts { - fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) - } - return fields - } - - /// Calls the deprecated `uploadFile` hook from one place. - /// - /// This deliberately leaves one deprecation warning in GutenbergKit's own build: - /// the marker exists to tell *hosts* to migrate, and supporting the hook until it - /// is removed means calling it. The warning marks the code that goes with it. - private static func deprecatedUploadFile( - _ delegate: any MediaUploadDelegate, _ url: URL, _ mimeType: String, _ filename: String - ) async throws -> MediaUploadResponse? { - try await delegate.uploadFile(at: url, mimeType: mimeType, filename: filename) - } - private static func attachmentId(fromPath path: String) -> String? { let components = path.split(separator: "/", omittingEmptySubsequences: true) guard components.count == 2, components[0] == "media" else { return nil } @@ -358,8 +339,8 @@ final class MediaUploadServer: Sendable { /// Result of the delegate processing + upload pipeline. private enum UploadResult { - /// The delegate (or the internal media client) completed the upload; carries the - /// raw WordPress response to relay. + /// The uploader, delegate, or internal media client completed the upload; + /// carries the raw WordPress response to relay. case uploaded(MediaUploadResponse) /// The delegate didn't modify the file and `uploadFile` returned nil. /// The caller should forward the original request body to WordPress. @@ -368,11 +349,16 @@ final class MediaUploadServer: Sendable { private static func processAndUpload( fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, context: UploadContext + extraParts: [MultipartPart], query: String, + delegateWantsFile: Bool, context: UploadContext ) async throws -> UploadResult { - // Step 1: Process (resize, transcode, etc.) + // Step 1: Process (resize, transcode, etc.) — but only for a file the + // delegate's metadata gate accepted. `handlesFile` returning false is the + // delegate saying it won't touch a file like this, so handing it one anyway + // would break the contract the gate documents. With an uploader set the file + // still gets delivered; it just skips processing on its way there. let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { + if let delegate = context.uploadDelegate, delegateWantsFile { processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { processed = .original @@ -443,6 +429,29 @@ final class MediaUploadServer: Sendable { } } + /// The editor's non-file form parts as ordered, UTF-8-decoded fields. + /// + /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) + /// survive verbatim, in the order the editor sent them. + private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { + var fields: [MediaUploadField] = [] + for part in parts { + fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) + } + return fields + } + + /// Calls the deprecated `uploadFile` hook from one place. + /// + /// This deliberately leaves one deprecation warning in GutenbergKit's own build: + /// the marker exists to tell *hosts* to migrate, and supporting the hook until it + /// is removed means calling it. The warning marks the code that goes with it. + private static func deprecatedUploadFile( + _ delegate: any MediaUploadDelegate, _ url: URL, _ mimeType: String, _ filename: String + ) async throws -> MediaUploadResponse? { + try await delegate.uploadFile(at: url, mimeType: mimeType, filename: filename) + } + private static func errorResponse(status: Int, message: String) -> HTTPResponse { // Emit a WordPress-REST-style error object so the JS middleware normalizes // it (and surfaces `message`) the same way it does a relayed WordPress @@ -565,10 +574,10 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context -/// Container for the upload delegate and the internal media client, captured by the -/// HTTPServer handler closure and read on each request. +/// Container for the upload delegate, host uploader, and internal media client, +/// captured by the HTTPServer handler closure and read on each request. /// -/// Both are held **strongly**, so a delegate that admitted a file for processing +/// All are held **strongly**, so a delegate that admitted a file for processing /// will process it — the three reads within a request can't disagree, and an /// in-flight upload keeps the host's delegate alive until it unwinds. That lifetime /// comes from the handler closure, which the listener retains for the server's diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index a1cddd3c0..d8ef1b848 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -481,7 +481,7 @@ struct MediaUploadServerTests { // The gate exists to skip a temp copy for a file the delegate won't touch. An // uploader takes over delivery for every file, so passing through here would // silently bypass it. - let delegate = DecliningDelegate() + let delegate = DeclineByMetadataDelegate() let uploader = RecordingUploader() let internalClient = MockInternalMediaClient() let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: internalClient) @@ -500,6 +500,9 @@ struct MediaUploadServerTests { #expect(uploader.received?.filename == "clip.mov") #expect(!internalClient.passthroughUploadCalled) + // ...but a declined file must still not reach `processFile`: `handlesFile` + // returning false is the delegate saying it won't touch a file like this. + #expect(!delegate.processFileCalled) } @Test("an uploader that throws surfaces as a failure, with no GutenbergKit retry") @@ -1023,7 +1026,10 @@ private final class RecordingUploader: MediaUploader, @unchecked Sendable { func upload(_ upload: MediaUpload) async throws -> Data { lock.withLock { _received = upload } - return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8) + // Shaped like a real attachment: the editor's `transformAttachment` reads + // `title.raw`, so an example without it would model a body that fails in the + // editor. + return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}"#.utf8) } } @@ -1037,13 +1043,6 @@ private final class ThrowingUploader: MediaUploader, @unchecked Sendable { } } -/// A delegate that declines every file by metadata. -private final class DecliningDelegate: MediaUploadDelegate, @unchecked Sendable { - func handlesFile(ofType mimeType: String, named filename: String) -> Bool { - false - } -} - /// A delegate that transcodes, used to check the server holds it across the whole /// request rather than re-reading a reference the host may have dropped. private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendable { @@ -1100,9 +1099,10 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`, so the -/// server must pass through without ever materializing the file or calling -/// `processFile`. +/// A delegate that declines every file by metadata via `handlesFile`. With no +/// uploader the server must pass through without ever materializing the file; with +/// one, delivery still happens but `processFile` must not be called. +/// `processFileCalled` pins both. private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false