From 1247d09f3969a4f2e0424c5f95f62b6c7b8b2a9e Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:18:26 -0600 Subject: [PATCH] refactor: split the media delegate into MediaProcessor and MediaUploader Make performing a media upload -- and retrying it -- a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does (say, to run it through its own networking so it can log every request). Both go to the same configured site; the only difference is who executes the requests. There is no in-between where the host performs the upload but GutenbergKit retries it. When an upload fatals in server-side post-processing it has to be retried: core retries POST .../post-process up to 5x, and cleans up the orphan if that fails. The old `MediaUploadDelegate.uploadFile` let a host perform the upload itself by returning the raw response it received -- which split one upload's HTTP across two owners: the host performed the POST /wp/v2/media, then core, reading that raw response, drove the post-process retries (and the orphan cleanup) behind it. A host that took over uploads to run them through its own stack still didn't own the retries; those went out through the browser, not the host. Delivery and its retries were owned by different parties. Make the upload and its retries one unit with one owner: - `MediaProcessor` (handlesFile, processFile) only transforms the file. It never performs the upload, so GutenbergKit performs it and owns the retries -- the extension point almost every host wants. - `MediaUploader` (upload) performs the upload on the host's own stack (its networking, logging, retry policy, a background session) and owns the whole lifecycle. `upload` returns the finished attachment or throws: there's no raw response for core to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. All-or-nothing: the host performs the upload and its retries, or GutenbergKit does -- never a split. An uploader and GutenbergKit's built-in default both target the same configured site; the choice is only who executes the requests. Media deletes always relay to the default uploader (the configured site): every attachment lives there, even one a host uploader delivered, so there is no per-host delete path. The relay is left unscoped -- core issues its cleanup DELETE there, but the relay can't tell it from any other DELETE the WebView sends, so a client-side-compromised editor holding the loopback token could force-delete media on the site. Accepted -- such a script already has broad write access, and a server-side compromise deletes media directly without the editor. An earlier revision carried a per-session ledger to scope the relay; dropped as not worth the cost for a client-side-only threat. `EditorViewController`/`GutenbergView` expose `mediaProcessor` + `mediaUploader` in place of `mediaUploadDelegate`; the server starts if either is set and builds a default uploader whenever site credentials are present (it delivers GutenbergKit's own uploads and relays every media delete). `MediaUploadResponse` drops to internal -- it is no longer on any public API. Both demos and all tests move to the new protocols. Breaking change: hosts must migrate `mediaUploadDelegate` (WordPress-iOS/Android, Jetpack). iOS and Android suites green; SwiftLint and Detekt clean. --- android/Gutenberg/detekt-baseline.xml | 1 + .../org/wordpress/gutenberg/GutenbergView.kt | 99 +++++--- .../wordpress/gutenberg/MediaUploadServer.kt | 180 +++++++------- .../GutenbergViewUploadServerTest.kt | 22 +- .../gutenberg/MediaUploadServerTest.kt | 215 ++++++++--------- ...ploadDelegate.kt => DemoMediaProcessor.kt} | 11 +- .../example/gutenbergkit/EditorActivity.kt | 2 +- ios/Demo-iOS/Sources/Views/EditorView.swift | 8 +- .../Sources/EditorViewController.swift | 137 ++++++----- .../Sources/Media/MediaUploadDelegate.swift | 118 +++++---- .../Sources/Media/MediaUploadServer.swift | 118 +++++---- .../Media/MediaUploadServerTests.swift | 224 +++++++++--------- 12 files changed, 601 insertions(+), 534 deletions(-) rename android/app/src/main/java/com/example/gutenbergkit/{DemoMediaUploadDelegate.kt => DemoMediaProcessor.kt} (93%) 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 0989879ac..ae421c251 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -113,30 +113,44 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for customizing media upload behavior (resize, transcode, - * custom upload). + * Transforms media (resize, transcode, …) before GutenbergKit delivers it to + * the configured site. The safe, common extension point — a processor never + * performs the upload itself, so it cannot deliver media to the wrong place. * * Provide this **before the editor loads** — typically right after * construction (e.g. in the `AndroidView` factory). It is captured once, when - * the page begins loading, and advertised to the page then; setting it - * afterward has no effect, so the setter throws to surface the mistake. + * the page begins loading; setting it afterward has no effect, so the setter + * throws to surface the mistake. */ - var mediaUploadDelegate: MediaUploadDelegate? = null + var mediaProcessor: MediaProcessor? = 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("mediaProcessor") } 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 [mediaProcessor]: set it before the editor loads. + */ + 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 /** * True once the editor page has begun loading and the upload server's - * configuration has been captured. After this the [mediaUploadDelegate] can no - * longer take effect, so its setter throws. + * configuration has been captured. After this the [mediaProcessor]/[mediaUploader] + * can no longer take effect, so their setters throw. */ @Volatile private var hasStartedLoading = false @@ -638,13 +652,13 @@ class GutenbergView : FrameLayout { /** * Invoked when the editor page begins loading. Starts the upload server once — - * capturing the [mediaUploadDelegate] provided before load — then advertises - * the editor globals (including the server's port and token) to the page. + * capturing the [mediaProcessor]/[mediaUploader] provided before load — then + * advertises the editor globals (including the server's port and token) to the page. * * Starting the server here, on the UI thread, rather than from the - * [mediaUploadDelegate] setter keeps its whole lifecycle — start here, stop in - * [onDetachedFromWindow] — on the UI thread, so it can't race a - * background-thread delegate assignment. + * [mediaProcessor]/[mediaUploader] setters keeps its whole lifecycle — start + * here, stop in [onDetachedFromWindow] — on the UI thread, so it can't race a + * background-thread assignment. */ private fun onEditorPageStarted() { if (!hasStartedLoading) { @@ -671,17 +685,22 @@ 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 - - // The native upload server relays through DefaultMediaUploader, which needs a - // site root and an auth header (every host provides one — the editor injects - // it because the WebView has no auth cookies). Without both there is nothing - // to upload through, so leave the server down and let uploads fall to the - // default WebView path rather than start a server that could only fail. - if (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) return + // Nothing to route through the native server unless the host provided a + // processor or an uploader. (Matches iOS.) + if (mediaProcessor == null && mediaUploader == null) return + + // A DefaultMediaUploader delivers GutenbergKit-owned uploads (when no uploader + // is set) and relays the editor's media DELETEs to the configured site — every + // attachment lives there, even one a host uploader delivered. It needs a site + // root and an auth header (every host provides one — the editor injects it + // because the WebView has no auth cookies). If GutenbergKit would have to + // deliver uploads itself but lacks those, there's nothing to upload through, so + // leave the server down and let uploads fall to the default WebView path. + if (mediaUploader == null && + (configuration.siteApiRoot.isEmpty() || configuration.authHeader.isEmpty()) + ) { + return + } // The editor reaches the loopback server over cleartext http://localhost. If // the host app's network-security config doesn't permit cleartext to @@ -701,14 +720,24 @@ class GutenbergView : FrameLayout { } try { - val defaultUploader = DefaultMediaUploader( - httpClient = uploadHttpClient, - siteApiRoot = configuration.siteApiRoot, - authHeader = configuration.authHeader, - siteApiNamespace = configuration.siteApiNamespace.toList() - ) + // Build a DefaultMediaUploader whenever there are credentials to reach the + // site: it delivers GutenbergKit-owned uploads and relays the editor's + // DELETEs there. null only when a host owns uploads and no creds exist. + val defaultUploader = if ( + configuration.siteApiRoot.isNotEmpty() && configuration.authHeader.isNotEmpty() + ) { + DefaultMediaUploader( + httpClient = uploadHttpClient, + siteApiRoot = configuration.siteApiRoot, + authHeader = configuration.authHeader, + siteApiNamespace = configuration.siteApiNamespace.toList() + ) + } else { + null + } uploadServer = MediaUploadServer( - uploadDelegate = mediaUploadDelegate, + processor = mediaProcessor, + uploader = mediaUploader, defaultUploader = defaultUploader, 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 a65ceb301..a2b944912 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -31,8 +31,8 @@ import okio.source * so every consumer — image sub-sizes, attachment links, error notices — * behaves identically to a non-native upload. */ -class MediaUploadResponse( - /** The HTTP status code WordPress (or the host's upload service) returned. */ +internal class MediaUploadResponse( + /** The HTTP status code WordPress returned. */ val statusCode: Int, /** * The raw response body — a WordPress REST attachment on success, or a @@ -52,7 +52,7 @@ class MediaUploadResponse( ) /** - * The result of a delegate's [MediaUploadDelegate.processFile]. + * The result of a [MediaProcessor.processFile]. */ sealed class ProcessedProxyFile { /** The delegate did not modify the file; the original upload is forwarded unchanged. */ @@ -68,72 +68,70 @@ sealed class ProcessedProxyFile { } /** - * Interface for customizing media upload behavior. + * Transforms media before GutenbergKit delivers it. * - * The native host app can provide an implementation to resize images, - * transcode video, or use its own upload service. + * A processor only changes *bytes* — GutenbergKit still uploads the result to the + * configured site and owns the whole lifecycle (retries, cleanup). Because it + * never performs the upload itself, a processor cannot deliver media to the wrong + * place. Set [GutenbergView.mediaProcessor] to resize images, transcode video, + * strip EXIF, etc. This is the safe, common extension point: most hosts want only + * this. */ -interface MediaUploadDelegate { +interface MediaProcessor { /** - * Whether this delegate might handle a file with the given metadata — either - * processing it ([processFile]) or uploading it itself ([uploadFile]). - * - * A cheap, metadata-only gate the server consults *before* materializing the - * upload to a temp file. Return false to decline a file by type — e.g. an - * image-only delegate returning false for a video — so the server forwards - * the original upload to WordPress without first copying a file the delegate - * won't touch. Because it gates the temp-file copy needed by *both* - * [processFile] and [uploadFile], return true for any file the delegate will - * either process or upload itself. - * - * Defaults to true: every file is materialized and the full pipeline runs. A - * true here is not a commitment — [processFile] may still return - * [ProcessedProxyFile.Original] after inspecting the file's contents. + * Whether this processor might transform a file with the given metadata. A + * cheap, metadata-only gate consulted *before* the upload is materialized to a + * temp file; return false to pass a file straight through untouched — e.g. an + * image-only processor returning false for a video. Defaults to true; not a + * commitment, since [processFile] may still return [ProcessedProxyFile.Original] + * after inspecting the file's contents. */ fun handlesFile(mimeType: String, filename: String): Boolean = true /** - * Process a file before upload (e.g., resize image, transcode video). - * - * Return [ProcessedProxyFile.Original] to upload the file unchanged, or - * [ProcessedProxyFile.Processed] with the processed file and its metadata. - * When the format changes, report the new mimeType and filename so WordPress - * stores it with the correct extension and type. + * Transform a file before upload (e.g., resize image, transcode video). Return + * [ProcessedProxyFile.Original] to upload it unchanged, or + * [ProcessedProxyFile.Processed] with the new file and its metadata (report the + * new mimeType and filename when the format changes, so WordPress stores it + * with the correct extension and type). */ suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original +} +/** + * 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 built-in default 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 core to retry behind it. The attachment you return lives on + * that same configured site, where the editor reads and updates it by ID. + */ +interface MediaUploader { /** - * 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 - * the editor sees a complete attachment object. - */ - suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null - - /** - * Delete a previously uploaded attachment. + * 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 editor deletes the attachment when an upload's server-side - * post-processing fails past recovery, so it does not leave an orphan - * behind. A delegate that uploaded the attachment itself via [uploadFile] - * owns an ID only it can resolve, so it must delete the attachment itself - * too — the default uploader would address the wrong site. + * 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. * - * Return the raw response (status code + body), which GutenbergKit relays - * to the editor unchanged, or null to use the default uploader. - * - * Return null for any ID the delegate does not recognize. Unlike the upload - * path, there is no [handlesFile] gate here — an attachment ID carries no - * MIME type or filename — so this method is called for *every* deletion, - * including attachments the delegate declined at upload time and WordPress - * therefore created itself. Returning a response for one of those (an error - * from the host's own media service, say) leaves the real WordPress - * attachment undeleted — precisely the orphan this cleanup exists to remove. - * null hands it to the default uploader, which addresses the right site. + * 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 core + * cleans up behind you. */ - suspend fun deleteFile(attachmentId: String): MediaUploadResponse? = null + suspend fun upload(file: File, mimeType: String, filename: String): ByteArray } /** @@ -149,7 +147,8 @@ interface MediaUploadDelegate { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, + private val processor: MediaProcessor?, + private val uploader: MediaUploader?, private val defaultUploader: DefaultMediaUploader?, cacheDir: File? = null, scope: CoroutineScope? = null, @@ -278,24 +277,32 @@ internal class MediaUploadServer( } /** - * Relays the editor's orphan cleanup. + * Relays a media deletion. * - * Core's media upload middleware deletes the attachment when every - * `post-process` retry fails. A cross-origin editor cannot issue that - * request directly — api-fetch tunnels `DELETE` as a `POST` carrying - * `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the - * browser blocks it at preflight. Relaying it here lets the cleanup run. + * The editor deletes an attachment when the user removes it, and core deletes + * an upload's orphan when every `post-process` retry fails. A cross-origin + * editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` + * carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the + * browser blocks it at preflight; relaying it here lets the deletion run. * - * Offers the deletion to the delegate first, as [handleUpload] does, so a - * host that uploaded the attachment itself deletes it from the same place. + * Every attachment lives on the configured site — even one a host uploader + * delivered — so its deletion is relayed to the default uploader there. See + * the accepted-risk note in the body. */ @Suppress("TooGenericExceptionCaught") private suspend fun handleMediaDelete(attachmentId: String, query: String): HttpResponse { return try { - uploadDelegate?.deleteFile(attachmentId)?.let { return relayResponse(it) } - - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") - relayResponse(uploader.deleteMedia(attachmentId, query)) + // Relay to the default uploader (the configured site) — every attachment + // lives there, even one a host uploader delivered. Core issues this only + // as orphan cleanup after failed recovery, but the relay can't tell that + // from any other DELETE the WebView sends: a compromised editor script + // holding the loopback token could force-delete arbitrary media on the + // configured site. Accepted risk — such a script already has broad write + // access, and a server-side compromise (a malicious plugin) deletes media + // directly without the editor, so scoping this with a per-session ledger + // buys little for the cost. + val defaultUploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + relayResponse(defaultUploader.deleteMedia(attachmentId, query)) } catch (e: kotlin.coroutines.cancellation.CancellationException) { throw e // Never swallow coroutine cancellation. } catch (e: Exception) { @@ -317,11 +324,13 @@ internal class MediaUploadServer( val mimeType = filePart.contentType val filename = filePart.filename ?: "upload" - // 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). - if (uploadDelegate?.handlesFile(mimeType, filename) != true) { + // Materialize a temp file only if someone will touch it: a processor that + // claims this file, or an uploader (which always delivers the file itself). + // If GutenbergKit will deliver (no uploader) and no processor wants the + // file, forward the original request body directly, skipping a temp copy of + // a file nobody will process (e.g. a video handed to an image-only processor). + val processorWantsFile = processor?.handlesFile(mimeType, filename) == true + if (uploader == null && !processorWantsFile) { return passthroughResponse(request, query) } @@ -424,8 +433,8 @@ internal class MediaUploadServer( uploadResult.response } is UploadResult.Passthrough -> { - // Delegate didn't modify the file — forward the original - // request body to WordPress without re-encoding. + // No uploader is set and the processor left the file unmodified — + // forward the original request body without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") performPassthroughUpload(request, query) } @@ -450,7 +459,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Process + Deliver Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -471,10 +480,15 @@ internal class MediaUploadServer( file: File, mimeType: String, filename: String, extraParts: List, query: String ): UploadResult { - val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + // Transform (resize, transcode, …) if a processor claims the file. + val processed = if (processor?.handlesFile(mimeType, filename) == true) { + processor.processFile(file, mimeType, filename) + } 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. + // processor's values verbatim, so a format change is reported to WordPress. val targetFile: File val targetMimeType: String val targetFilename: String @@ -492,9 +506,11 @@ internal class MediaUploadServer( } try { - // If the delegate provided its own upload, use that. - uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { - return UploadResult.Uploaded(it) + // 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 { + return UploadResult.Uploaded(MediaUploadResponse(201, it.upload(targetFile, targetMimeType, targetFilename))) } // Unmodified — forward the original request body directly, skipping @@ -504,7 +520,7 @@ internal class MediaUploadServer( } val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No upload delegate or default uploader configured") + ?: error("No uploader or default uploader configured") return UploadResult.Uploaded(result) } finally { // The processed file (if the delegate produced a new one) is ours to 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..7ea179401 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -62,15 +62,15 @@ class GutenbergViewUploadServerTest { private fun idle() = shadowOf(Looper.getMainLooper()).idle() @Test - fun `the upload server starts when the page begins loading, capturing the delegate`() { + fun `the upload server starts when the page begins loading, capturing the processor`() { val view = makeView() try { - // A delegate provided before load is captured when the page starts. - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + // A processor provided before load is captured when the page starts. + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull( - "a delegate provided before load should bring up the upload server", + "a processor provided before load should bring up the upload server", uploadServerOf(view) ) } finally { @@ -79,14 +79,14 @@ class GutenbergViewUploadServerTest { } @Test - fun `no delegate means no upload server`() { + fun `no processor or uploader means no upload server`() { val view = makeView() try { - // No delegate provided — uploads should use the default WebView path. + // Nothing provided — uploads should use the default WebView path. startLoading(view) idle() assertNull( - "with no delegate, no upload server should be started", + "with no processor or uploader, no upload server should be started", uploadServerOf(view) ) } finally { @@ -95,15 +95,15 @@ class GutenbergViewUploadServerTest { } @Test - fun `setting the delegate after the page has started loading throws`() { + fun `setting a media handler after the page has started loading throws`() { val view = makeView() try { startLoading(view) idle() - // The delegate is captured at load; a later assignment is a programmer + // The processor is captured at load; a later assignment is a programmer // error and must surface loudly rather than silently do nothing. assertThrows(IllegalStateException::class.java) { - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) } } finally { detach(view) @@ -113,7 +113,7 @@ class GutenbergViewUploadServerTest { @Test fun `detaching the view stops and clears the upload server`() { val view = makeView() - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view) idle() assertNotNull(uploadServerOf(view)) 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 6cdc04da7..78ab70bd0 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -33,7 +33,7 @@ class MediaUploadServerTest { @Before fun setUp() { - server = MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) } @After @@ -53,7 +53,7 @@ class MediaUploadServerTest { fun `stop cancels an internally-created scope but leaves a caller-supplied one alone`() { // No scope supplied → the server owns one, which stop() must cancel. val owningServer = - MediaUploadServer(uploadDelegate = null, defaultUploader = null, cacheDir = tempFolder.root) + MediaUploadServer(processor = null, uploader = null, defaultUploader = null, cacheDir = tempFolder.root) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -63,7 +63,8 @@ class MediaUploadServerTest { // A caller-supplied scope belongs to the caller — stop() must not cancel it. val callerScope = CoroutineScope(Dispatchers.IO) val borrowingServer = MediaUploadServer( - uploadDelegate = null, + processor = null, + uploader = null, defaultUploader = null, cacheDir = tempFolder.root, scope = callerScope @@ -141,14 +142,19 @@ class MediaUploadServerTest { } @Test - fun `routes a deletion to the delegate when it handles one`() { - // A host that uploaded the attachment itself owns an ID only it can - // resolve, so the default uploader must not be asked to delete it. No - // default uploader is configured, so a 200 here can only come from the - // delegate — the fallback path would fail with "no uploader". - val delegate = DeletingDelegate() + fun `relays a deletion to the default uploader even when an uploader owns uploads`() { + // An attachment lives on the configured site even when a host uploader + // delivered it, so its deletion goes to the default uploader — the host + // uploader owns uploads, not deletes. + val uploader = MockUploader() + val defaultUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = uploader, + defaultUploader = defaultUploader, + cacheDir = tempFolder.root + ) val response = sendRawRequest( method = "DELETE", @@ -158,39 +164,43 @@ class MediaUploadServerTest { ) assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) - assertEquals("42", delegate.deletedAttachmentId) + assertTrue(defaultUploader.deleteMediaCalled) + assertEquals("42", defaultUploader.deletedAttachmentId) } + // MARK: - Media deletion + @Test - fun `relays a delegate's own Content-Type instead of emitting it twice`() { - // HTTP header names are case-insensitive, so a delegate spelling it - // `content-type` must still override the JSON default rather than merge - // alongside it — HttpResponse serializes every entry it is given, which - // would put the name on the wire twice (mirrors the iOS behavior). - val delegate = ContentTypeDeletingDelegate() + fun `relays a deletion to the default uploader (configured site)`() { + // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup + // DELETE is relayed to the default uploader (the configured site). + val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = null, + defaultUploader = mockUploader, + cacheDir = tempFolder.root + ) val response = sendRawRequest( method = "DELETE", - path = "/media/42?force=true", + path = "/media/512?force=true", headers = mapOf("Relay-Authorization" to "Bearer ${server.token}"), body = ByteArray(0) ) assertTrue("Expected 200 but got: ${response.statusLine}", response.statusLine.contains("200")) - // Assert on the raw header lines, not the parsed map: the parser - // lowercases keys into a map, so a duplicated header would silently - // collapse and this test would pass against the very bug it covers. - assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) + assertTrue(mockUploader.deleteMediaCalled) + assertEquals("512", mockUploader.deletedAttachmentId) } @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() + val processor = PassthroughProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, // so the middleware forwards that query on to the native server. Routing must @@ -209,7 +219,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // The delegate returns Original, so this is the passthrough branch. + // The processor returns Original, so this is the passthrough branch. // Pin which branch ran — `lastQuery` is recorded by both, so without this // the query assertion would pass even if routing collapsed onto one path. assertTrue(mockUploader.passthroughUploadCalled) @@ -217,13 +227,21 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) } - // MARK: - Upload with delegate + // MARK: - Upload with a processor or uploader @Test - fun `calls delegate processFile and uploadFile`() { - val delegate = MockUploadDelegate() + fun `routes an upload to the uploader and relays its attachment`() { + // With an uploader set, GutenbergKit hands it the file and relays the finished + // attachment it returns — the default uploader (configured site) is never used. + val uploader = MockUploader() + val defaultUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer( + processor = null, + uploader = uploader, + defaultUploader = defaultUploader, + cacheDir = tempFolder.root + ) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -239,12 +257,14 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - assertTrue(delegate.processFileCalled) - assertTrue(delegate.uploadFileCalled) - assertEquals("image/jpeg", delegate.lastMimeType) - assertEquals("photo.jpg", delegate.lastFilename) - - // The server relays WordPress's raw response body verbatim. + assertTrue(uploader.uploadCalled) + assertEquals("image/jpeg", uploader.lastMimeType) + assertEquals("photo.jpg", uploader.lastFilename) + // The host owns delivery — GutenbergKit must not upload to the configured site. + assertFalse(defaultUploader.uploadCalled) + assertFalse(defaultUploader.passthroughUploadCalled) + + // The server relays the exact attachment JSON the uploader returned. val json = JsonParser.parseString(response.body).asJsonObject assertEquals(42, json.get("id").asInt) assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) @@ -252,11 +272,11 @@ class MediaUploadServerTest { } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() + fun `forwards the processor's processed metadata to the uploader`() { + val processor = TranscodingProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -271,7 +291,7 @@ class MediaUploadServerTest { body = body ) - // The delegate changed the format, so the uploader must receive the new + // The processor changed the format, so the uploader must receive the new // metadata — not the original video/quicktime + clip.mov. assertTrue(mockUploader.uploadCalled) assertEquals("video/mp4", mockUploader.lastUploadMimeType) @@ -279,11 +299,11 @@ class MediaUploadServerTest { } @Test - fun `deletes the delegate's processed file after upload`() { - val delegate = TranscodingDelegate() + fun `deletes the processor's processed file after upload`() { + val processor = TranscodingProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -298,10 +318,10 @@ class MediaUploadServerTest { body = body ) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor produced and must delete it once the // upload finishes — the finally in processAndUpload covers success and throw // paths alike. A leaked processed file is a full-size temp per upload. - val processed = requireNotNull(delegate.producedFile) { "processFile was not called" } + val processed = requireNotNull(processor.producedFile) { "processFile was not called" } assertFalse("Processed temp file should be deleted after upload", processed.exists()) } @@ -321,7 +341,8 @@ class MediaUploadServerTest { // one — a flipped comparison would do the opposite and wipe an in-flight upload. server.stop() server = MediaUploadServer( - uploadDelegate = null, + processor = null, + uploader = null, defaultUploader = null, cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined @@ -334,12 +355,12 @@ class MediaUploadServerTest { // MARK: - Fallback to default uploader @Test - fun `uses passthrough when delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() + fun `uses passthrough when the processor does not modify the file`() { + val processor = PassthroughProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -355,7 +376,7 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - assertTrue(delegate.processFileCalled) + assertTrue(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) @@ -365,12 +386,12 @@ class MediaUploadServerTest { } @Test - fun `skips processing and the temp copy when the delegate declines by metadata`() { - val delegate = DeclineByMetadataDelegate() + fun `skips processing and the temp copy when the processor declines by metadata`() { + val processor = DecliningProcessor() val mockUploader = MockDefaultUploader() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, uploader = null, defaultUploader = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -386,9 +407,9 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - // Declined by metadata → the delegate is never asked to process (so the + // Declined by metadata → the processor is never asked to process (so the // file was never materialized), and the upload is passed through directly. - assertFalse(delegate.processFileCalled) + assertFalse(processor.processFileCalled) assertTrue(mockUploader.passthroughUploadCalled) assertFalse(mockUploader.uploadCalled) } @@ -773,54 +794,27 @@ class MediaUploadServerTest { // MARK: - Mocks - private class MockUploadDelegate : MediaUploadDelegate { - @Volatile var processFileCalled = false - @Volatile var uploadFileCalled = false + /** + * A host uploader: it performs the upload on its own stack. `upload` returns the + * finished attachment JSON (or throws). + */ + private class MockUploader( + private val uploadBody: ByteArray = + """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""".toByteArray() + ) : MediaUploader { + @Volatile var uploadCalled = false @Volatile var lastMimeType: String? = null @Volatile var lastFilename: String? = null - override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { - processFileCalled = true + override suspend fun upload(file: File, mimeType: String, filename: String): ByteArray { + uploadCalled = true lastMimeType = mimeType - return ProcessedProxyFile.Original - } - - override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? { - uploadFileCalled = true lastFilename = filename - val json = """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""" - return MediaUploadResponse(201, json.toByteArray()) - } - } - - /** - * A delegate that handles deletions itself, as a host uploading to its own - * media service would. - */ - private class DeletingDelegate : MediaUploadDelegate { - @Volatile var deletedAttachmentId: String? = null - - override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { - deletedAttachmentId = attachmentId - return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) - } - } - - /** - * A delegate that sets its own `Content-Type`, lowercased, so the relay must - * override the JSON default rather than emit the header twice. - */ - private class ContentTypeDeletingDelegate : MediaUploadDelegate { - override suspend fun deleteFile(attachmentId: String): MediaUploadResponse? { - return MediaUploadResponse( - 200, - "deleted".toByteArray(), - mapOf("content-type" to "text/plain") - ) + return uploadBody } } - private class ProcessOnlyDelegate : MediaUploadDelegate { + private class PassthroughProcessor : MediaProcessor { @Volatile var processFileCalled = false override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -833,7 +827,7 @@ class MediaUploadServerTest { * Declines every file by metadata via [handlesFile], so the server must pass * through without materializing the file or calling [processFile]. */ - private class DeclineByMetadataDelegate : MediaUploadDelegate { + private class DecliningProcessor : MediaProcessor { @Volatile var processFileCalled = false override fun handlesFile(mimeType: String, filename: String): Boolean = false @@ -844,9 +838,9 @@ class MediaUploadServerTest { } } - /** A delegate that produces a new file with changed metadata (e.g. a transcode). */ - private class TranscodingDelegate : MediaUploadDelegate { - /** The processed file this delegate wrote, for cleanup assertions. */ + /** A processor that produces a new file with changed metadata (e.g. a transcode). */ + private class TranscodingProcessor : MediaProcessor { + /** The processed file this processor wrote, for cleanup assertions. */ @Volatile var producedFile: File? = null override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -857,7 +851,13 @@ class MediaUploadServerTest { } } - private class MockDefaultUploader : DefaultMediaUploader( + private class MockDefaultUploader( + /** The response `upload`/`passthroughUpload` return. Defaults to a 201 success. */ + private val uploadResponse: MediaUploadResponse = MediaUploadResponse( + 201, + """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() + ) + ) : DefaultMediaUploader( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" @@ -867,6 +867,8 @@ class MediaUploadServerTest { @Volatile var lastUploadMimeType: String? = null @Volatile var lastUploadFilename: String? = null @Volatile var lastQuery: String? = null + @Volatile var deleteMediaCalled = false + @Volatile var deletedAttachmentId: String? = null override suspend fun upload( file: File, mimeType: String, filename: String, @@ -876,7 +878,7 @@ class MediaUploadServerTest { lastUploadMimeType = mimeType lastUploadFilename = filename lastQuery = query - return mockResponse() + return uploadResponse } override suspend fun passthroughUpload( @@ -886,13 +888,14 @@ class MediaUploadServerTest { ): MediaUploadResponse { passthroughUploadCalled = true lastQuery = query - return mockResponse() + return uploadResponse } - private fun mockResponse() = MediaUploadResponse( - 201, - """{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}""".toByteArray() - ) + override suspend fun deleteMedia(attachmentId: String, query: String): MediaUploadResponse { + deleteMediaCalled = true + deletedAttachmentId = attachmentId + return MediaUploadResponse(200, """{"deleted":true}""".toByteArray()) + } } } diff --git a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt similarity index 93% rename from android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt rename to android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt index 572836e4c..ea7de6ca4 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt @@ -5,19 +5,18 @@ import android.graphics.BitmapFactory import android.graphics.Matrix import android.media.ExifInterface import android.util.Log -import org.wordpress.gutenberg.MediaUploadDelegate +import org.wordpress.gutenberg.MediaProcessor import org.wordpress.gutenberg.ProcessedProxyFile import java.io.File import java.io.IOException /** - * Demo media upload delegate that resizes images to a maximum dimension of 2000px. - * - * Only overrides [processFile] — [uploadFile] returns null so the default uploader is used. + * Demo media processor that resizes images to a maximum dimension of 2000px, then + * lets GutenbergKit deliver the result to the configured site. */ -class DemoMediaUploadDelegate : MediaUploadDelegate { +class DemoMediaProcessor : MediaProcessor { companion object { - private const val TAG = "DemoMediaUploadDelegate" + private const val TAG = "DemoMediaProcessor" } // Only non-GIF images are ever resized (see processFile), so decline diff --git a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt index 20f3e84b2..c2a48ac0f 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt @@ -338,7 +338,7 @@ fun EditorScreen( } }) if (enableNativeMediaUpload) { - mediaUploadDelegate = DemoMediaUploadDelegate() + mediaProcessor = DemoMediaProcessor() } onGutenbergViewCreated(this) } diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index 0f9b56ca4..c358db526 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -136,7 +136,7 @@ private struct _EditorView: UIViewControllerRepresentable { let viewController = EditorViewController(configuration: configuration, dependencies: dependencies) viewController.delegate = context.coordinator if enableNativeMediaUpload { - viewController.mediaUploadDelegate = context.coordinator + viewController.mediaProcessor = context.coordinator } viewController.webView.isInspectable = true @@ -189,7 +189,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -295,11 +295,11 @@ private struct _EditorView: UIViewControllerRepresentable { return nil } - // MARK: - MediaUploadDelegate + // MARK: - MediaProcessor /// Only non-GIF images are ever resized (see `processFile`), so decline /// everything else by metadata — the server then skips copying a file - /// this delegate would only pass through. + /// this processor would only pass through. nonisolated func handlesFile(ofType mimeType: String, named _: String) -> Bool { mimeType.hasPrefix("image/") && mimeType != "image/gif" } diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index da4c1fefe..86de817ae 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -105,52 +105,62 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let isWarmupMode: Bool /// Set once the editor has begun loading and captured its configuration - /// (including ``mediaUploadDelegate``). After this, that delegate can no longer - /// take effect, so its setter traps if written. + /// (including ``mediaProcessor`` and ``mediaUploader``). After this, they can no + /// longer take effect, so their setters trap if written. private var hasStartedLoading = false - /// Whether a non-nil ``mediaUploadDelegate`` was ever assigned. Lets the load - /// path tell "the delegate was released before load" (a retention mistake to - /// trap) apart from "no delegate was configured" (a valid opt-out). - private var mediaUploadDelegateWasAssigned = false + /// Whether a non-nil ``mediaProcessor``/``mediaUploader`` was ever assigned. Lets + /// the load path tell "the host object was released before load" (a retention + /// mistake to trap) apart from "none was configured" (a valid opt-out). + private var mediaProcessorWasAssigned = false + private var mediaUploaderWasAssigned = false - /// Delegate for customizing media file processing and upload behavior. + /// Transforms media (resize, transcode, …) before GutenbergKit delivers it to + /// the configured site. The safe, common extension point — a processor never + /// performs the upload itself, so it cannot deliver media to the wrong place. /// - /// Provide this **before the editor loads** — typically right after `init`, the - /// same way the rest of the editor configuration is supplied. It is captured - /// once, when the editor begins loading, and injected into the page's initial - /// configuration; setting it afterward has no effect, so the setter traps. + /// Provide this **before the editor loads** — typically right after `init`. It + /// is captured once, when the editor begins loading; setting it afterward has no + /// effect, so the setter traps. /// - /// - Important: This is a `weak` reference — you must hold a strong reference to - /// your delegate until the editor has loaded, or native uploads are silently - /// disabled. To surface that mistake, the editor traps at load time if a - /// delegate that was assigned here has already been deallocated. - public weak var mediaUploadDelegate: (any MediaUploadDelegate)? { + /// - Important: This is a `weak` reference — hold a strong reference to your + /// processor until the editor has loaded, or native media handling is silently + /// disabled. The editor traps at load time if a processor assigned here has + /// already been deallocated. + public weak var mediaProcessor: (any MediaProcessor)? { didSet { - // Record whether a delegate was provided so the load path can tell a - // premature deallocation apart from a deliberate opt-out (see - // `startUploadServer`). - mediaUploadDelegateWasAssigned = mediaUploadDelegate != nil - // Deliberate fail-fast, not a defensive check. The delegate is captured - // into the page's initial configuration when the editor begins loading, - // so a delegate assigned afterward would silently never take effect; - // trapping surfaces that misuse loudly instead of failing quietly. - // - // `hasStartedLoading` flips at the start of the async load (see - // `loadEditor`), which runs at or after `viewDidLoad` — so this only - // *widens* the safe window versus a synchronous flip. A host that - // follows the documented contract (set right after `init`, before - // presenting) can never race it; the trap fires only on a genuinely - // late assignment. Do not soften this to a no-op or a log — silently - // dropping the delegate is exactly the failure this is here to catch. - precondition( - !hasStartedLoading, - "mediaUploadDelegate must be set before the editor loads (e.g. right after init). " - + "It is captured into the editor configuration at load; setting it afterward has no effect." - ) + mediaProcessorWasAssigned = mediaProcessor != nil + precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaProcessor")) } } + /// Takes over media upload on the host's own stack (background session, 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 ``mediaProcessor``: set it before the editor loads, + /// and hold a strong reference until then. + public weak var mediaUploader: (any MediaUploader)? { + didSet { + mediaUploaderWasAssigned = mediaUploader != nil + precondition(!hasStartedLoading, Self.lateMediaAssignmentMessage("mediaUploader")) + } + } + + /// Message for the fail-fast when media handling is assigned too late. + /// + /// Deliberate fail-fast, not a defensive check: the processor/uploader is + /// captured into the page's initial configuration when the editor begins + /// loading, so one assigned afterward would silently never take effect. + /// `hasStartedLoading` flips at the start of the async load, which runs at or + /// after `viewDidLoad`, so a host that follows the contract (set right after + /// `init`) can never race it. Do not soften this to a no-op or a log — silently + /// dropping the host's media handling is exactly the failure this catches. + private static func lateMediaAssignmentMessage(_ name: String) -> String { + "\(name) must be set before the editor loads (e.g. right after init). " + + "It is captured into the editor configuration at load; setting it afterward has no effect." + } + // MARK: - Private Properties (Services) private let editorService: EditorService private let httpClient: any EditorHTTPClientProtocol @@ -451,36 +461,51 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// falls back to Gutenberg's default upload behavior (the JS override won't activate /// because `nativeUploadPort` will be nil in GBKit). private func startUploadServer() async { - // A delegate that was provided but is already nil here was deallocated before - // the editor finished loading — the host didn't hold a strong reference to it. - // That silently disables native uploads, so trap loudly instead. + // A processor/uploader that was provided but is already nil here was + // deallocated before the editor finished loading — the host didn't hold a + // strong reference. That silently disables native media handling, so trap. + precondition( + !(mediaProcessorWasAssigned && mediaProcessor == nil), + "mediaProcessor was released before the editor loaded — hold a strong reference to it." + ) precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." + !(mediaUploaderWasAssigned && mediaUploader == nil), + "mediaUploader was released before the editor loaded — hold a strong reference to it." ) - guard mediaUploadDelegate != nil else { + // Nothing to route through the native server unless the host provided a + // processor or an uploader. + guard mediaProcessor != nil || mediaUploader != nil else { return } - // The native upload server relays through DefaultMediaUploader, which needs a - // site root and an auth header (every host provides one — the editor injects - // it because the WebView has no auth cookies). Without both there is nothing - // to upload through, so leave the server down and let uploads fall to the - // default WebView path rather than start a server that could only fail. - guard !configuration.authHeader.isEmpty else { + // A DefaultMediaUploader does two jobs: it delivers GutenbergKit-owned + // uploads (when no `mediaUploader` is set), and it relays the editor's media + // DELETEs to the configured site — every attachment lives there, even one a + // host uploader delivered, so that's where its deletion goes. It needs a site + // root and an auth header (every host provides one — the editor injects it + // because the WebView has no auth cookies). + // + // If GutenbergKit would have to deliver uploads itself but has no auth + // header, there's nothing to upload through: leave the server down and let + // uploads fall to the default WebView path rather than start a server that + // could only fail. + if mediaUploader == nil && configuration.authHeader.isEmpty { return } - - let defaultUploader = DefaultMediaUploader( - httpClient: httpClient.uploadClient(), - siteApiRoot: configuration.siteApiRoot, - siteApiNamespace: configuration.siteApiNamespace - ) + var defaultUploader: DefaultMediaUploader? + if !configuration.authHeader.isEmpty { + defaultUploader = DefaultMediaUploader( + httpClient: httpClient.uploadClient(), + siteApiRoot: configuration.siteApiRoot, + siteApiNamespace: configuration.siteApiNamespace + ) + } do { self.uploadServer = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, + processor: mediaProcessor, + uploader: mediaUploader, defaultUploader: defaultUploader ) } catch { diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 01b0be7d8..bfd9c1b07 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -7,13 +7,13 @@ import Foundation /// or WordPress REST error object (on failure) it would get from a direct /// upload, so every consumer — image sub-sizes, attachment links, error notices — /// behaves identically to a non-native upload. -public struct MediaUploadResponse: Sendable { - /// The HTTP status code WordPress (or the host's upload service) returned. - public let statusCode: Int +struct MediaUploadResponse: Sendable { + /// The HTTP status code WordPress returned. + let statusCode: Int /// The raw response body — a WordPress REST attachment on success, or a /// WordPress REST error object (`{ "code", "message", "data" }`) on failure. - public let body: Data + let body: Data /// The response headers to relay to the editor. /// @@ -22,16 +22,16 @@ public struct MediaUploadResponse: Sendable { /// metadata generation fataled, and the editor's api-fetch middleware reads /// it to retry `post-process` and clean up the orphan. Dropping it turns a /// recoverable upload into a permanent failure. - public let headers: [String: String] + let headers: [String: String] - public init(statusCode: Int, body: Data, headers: [String: String] = [:]) { + init(statusCode: Int, body: Data, headers: [String: String] = [:]) { self.statusCode = statusCode self.body = body self.headers = headers } } -/// The result of a delegate's ``MediaUploadDelegate/processFile(at:mimeType:filename:)``. +/// The result of a ``MediaProcessor/processFile(at:mimeType:filename:)``. public enum ProcessedProxyFile: Sendable { /// The delegate did not modify the file; the original upload is forwarded /// to WordPress unchanged. @@ -44,71 +44,71 @@ public enum ProcessedProxyFile: Sendable { case processed(URL, mimeType: String, filename: String) } -/// Protocol for customizing media upload behavior. +/// Transforms media before GutenbergKit delivers it. /// -/// The native host app can provide an implementation to resize images, -/// transcode video, or use its own upload service. Default implementations -/// pass files through unchanged and upload via the WordPress REST API. -public protocol MediaUploadDelegate: AnyObject, Sendable { - /// Whether this delegate might handle a file with the given metadata — either - /// processing it (``processFile(at:mimeType:filename:)``) or uploading it - /// itself (``uploadFile(at:mimeType:filename:)``). +/// A processor only changes *bytes* — GutenbergKit still uploads the result to +/// the configured site and owns the whole lifecycle (retries, cleanup). Because +/// it never performs the upload itself, a processor cannot deliver media to the +/// wrong place. Set ``EditorViewController/mediaProcessor`` to resize images, +/// transcode video, strip EXIF, etc. +/// +/// This is the safe, common extension point: most hosts want only this. +public protocol MediaProcessor: AnyObject, Sendable { + /// Whether this processor might transform a file with the given metadata. /// /// A cheap, metadata-only gate the server consults *before* materializing the - /// upload to a temp file. Return `false` to decline a file by type — e.g. an - /// image-only delegate returning `false` for a video — so the server forwards - /// the original upload to WordPress without first copying a file the delegate - /// won't touch. Because it gates the temp-file copy needed by *both* - /// `processFile` and `uploadFile`, return `true` for any file the delegate - /// will either process or upload itself. + /// upload to a temp file. Return `false` to pass a file straight through + /// untouched — e.g. an image-only processor returning `false` for a video — + /// so the server never copies a file the processor won't touch. /// - /// Defaults to `true`: every file is materialized and the full pipeline runs. - /// A `true` here is not a commitment — `processFile` may still return - /// `.original` after inspecting the file's contents. + /// Defaults to `true`. A `true` here is not a commitment — `processFile` may + /// still return `.original` after inspecting the file's contents. func handlesFile(ofType mimeType: String, named filename: String) -> Bool - /// Process a file before upload (e.g., resize image, transcode video). + /// Transform a file before upload (e.g., resize image, transcode video). /// /// Return ``ProcessedProxyFile/original`` to upload the file unchanged, or /// ``ProcessedProxyFile/processed(_:mimeType:filename:)`` with the processed /// file and its metadata. When the format changes, report the new mimeType /// and filename so WordPress stores it with the correct extension and type. func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile +} - /// 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 - /// host that uploads to WordPress should return the exact response it - /// received so the editor sees a complete attachment object. - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? - - /// Delete a previously uploaded attachment. - /// - /// The editor deletes the attachment when an upload's server-side - /// post-processing fails past recovery, so it does not leave an orphan - /// behind. A delegate that uploaded the attachment itself via - /// ``uploadFile(at:mimeType:filename:)`` owns an ID only it can resolve, so - /// it must delete the attachment itself too — the default uploader would - /// address the wrong site. +/// 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 built-in default 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 core 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. /// - /// Return the raw response (status code + body), which GutenbergKit relays - /// to the editor unchanged, or `nil` to use the default uploader. + /// 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. /// - /// Return `nil` for any ID the delegate does not recognize. Unlike the upload - /// path, there is no ``handlesFile(ofType:named:)`` gate here — an attachment - /// ID carries no MIME type or filename — so this method is called for *every* - /// deletion, including attachments the delegate declined at upload time and - /// WordPress therefore created itself. Returning a response for one of those - /// (an error from the host's own media service, say) leaves the real - /// WordPress attachment undeleted — precisely the orphan this cleanup exists - /// to remove. `nil` hands it to the default uploader, which addresses the - /// right site. - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? + /// 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 core cleans up behind you. + func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data } -/// Default implementations. -extension MediaUploadDelegate { +/// Default implementations for the optional ``MediaProcessor`` methods. +extension MediaProcessor { public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { true } @@ -116,12 +116,4 @@ extension MediaUploadDelegate { public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { .original } - - public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - nil - } - - public func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - nil - } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 0c03d8de2..cee6839f6 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,12 +29,14 @@ final class MediaUploadServer: Sendable { /// Creates and starts a new upload server. /// /// - Parameters: - /// - uploadDelegate: Optional delegate for customizing file processing and upload. - /// - defaultUploader: Fallback uploader used when no delegate provides `uploadFile`. + /// - processor: Optional processor for transforming files before upload. + /// - uploader: Optional uploader that takes over delivery on the host's own stack. + /// - defaultUploader: Delivers to the configured site when no uploader is set. /// - 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, + processor: (any MediaProcessor)? = nil, + uploader: (any MediaUploader)? = nil, defaultUploader: DefaultMediaUploader? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { @@ -45,7 +47,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let context = UploadContext(processor: processor, uploader: uploader, defaultUploader: defaultUploader) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -126,11 +128,13 @@ 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 { + // Materialize a temp file only if someone will touch it: a processor that + // claims this file, or an uploader (which always delivers the file itself). + // If GutenbergKit will deliver (no uploader) and no processor wants the + // file, forward the original request body directly, skipping a temp copy of + // a file nobody will process (e.g. a video handed to an image-only processor). + let processorWantsFile = context.processor?.handlesFile(ofType: mimeType, named: filename) ?? false + if context.uploader == nil, !processorWantsFile { do { return try await passthroughResponse(request, query: query, context: context) } catch { @@ -180,9 +184,9 @@ final class MediaUploadServer: Sendable { } /// Forwards the original request body to WordPress unchanged (no multipart - /// re-encoding) and relays the response. Used when the delegate won't touch - /// the file — it declined by metadata (`handlesFile` returned false) or - /// `processFile` returned `.original`. + /// re-encoding) and relays the response. Used on the no-uploader path when the + /// processor won't touch the file — it declined by metadata (`handlesFile` + /// returned false) or `processFile` returned `.original`. private static func passthroughResponse( _ request: HTTPServer.Request, query: String, context: UploadContext ) async throws -> HTTPResponse { @@ -208,25 +212,30 @@ final class MediaUploadServer: Sendable { return id } - /// Relays the editor's orphan cleanup. + /// Relays a media deletion. /// - /// Core's media upload middleware deletes the attachment when every - /// `post-process` retry fails. A cross-origin editor cannot issue that - /// request directly — api-fetch tunnels `DELETE` as a `POST` carrying - /// `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the - /// browser blocks it at preflight. Relaying it here lets the cleanup run. + /// The editor deletes an attachment when the user removes it, and core deletes + /// an upload's orphan when every `post-process` retry fails. A cross-origin + /// editor cannot issue `DELETE` directly — api-fetch tunnels it as a `POST` + /// carrying `X-HTTP-Method-Override`, which core's CORS allow-list omits, so the + /// browser blocks it at preflight; relaying it here lets the deletion run. /// - /// Offers the deletion to the delegate first, as ``handleUpload(_:context:)`` - /// does, so a host that uploaded the attachment itself deletes it from the - /// same place. + /// Every attachment lives on the configured site — even one a host uploader + /// delivered — so its deletion is relayed to the default uploader there. See + /// the accepted-risk note in the body. private static func handleMediaDelete( _ attachmentId: String, query: String, context: UploadContext ) async -> HTTPResponse { do { - if let response = try await context.uploadDelegate?.deleteFile(attachmentId: attachmentId) { - return relayResponse(response) - } - + // Relay to the default uploader (the configured site) — every attachment + // lives there, even one a host uploader delivered. Core issues this only + // as orphan cleanup after failed recovery, but the relay can't tell that + // from any other DELETE the WebView sends: a compromised editor script + // holding the loopback token could force-delete arbitrary media on the + // configured site. Accepted risk — such a script already has broad write + // access, and a server-side compromise (a malicious plugin) deletes media + // directly without the editor, so scoping this with a per-session ledger + // buys little for the cost. guard let defaultUploader = context.defaultUploader else { return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) } @@ -274,13 +283,13 @@ final class MediaUploadServer: Sendable { // MARK: - Delegate Pipeline - /// Result of the delegate processing + upload pipeline. + /// Result of the process + deliver pipeline. private enum UploadResult { - /// The delegate (or default uploader) completed the upload; carries the - /// raw WordPress response to relay. + /// The uploader or default uploader completed the upload; carries the + /// 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. + /// No uploader is set and the processor left the file unmodified, so the + /// caller should forward the original request body to the configured site. case passthrough } @@ -288,16 +297,16 @@ final class MediaUploadServer: Sendable { fileURL: URL, mimeType: String, filename: String, extraParts: [MultipartPart], query: String, context: UploadContext ) async throws -> UploadResult { - // Step 1: Process (resize, transcode, etc.) + // Step 1: transform (resize, transcode, …) if a processor claims the file. let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { - processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) + if let processor = context.processor, processor.handlesFile(ofType: mimeType, named: filename) { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) } else { processed = .original } // Resolve the file to upload and its metadata. `.processed` uses the - // delegate's values verbatim, so a format change is reported to WordPress. + // processor's values verbatim, so a format change is reported to WordPress. let uploadURL: URL let uploadMimeType: String let uploadFilename: String @@ -312,7 +321,7 @@ final class MediaUploadServer: Sendable { uploadFilename = processedFilename } - // The processed file (if the delegate produced a new one) is ours to + // The processed file (if the processor produced a new one) is ours to // clean up — on success it has been uploaded, on failure it is abandoned. // Cleaning up here rather than in the caller covers the throw paths too. defer { @@ -321,10 +330,13 @@ final class MediaUploadServer: Sendable { } } - // Step 2: Upload to remote WordPress - if let delegate = context.uploadDelegate, - let result = try await delegate.uploadFile(at: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) { - return .uploaded(result) + // 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. Otherwise the + // default uploader delivers to the configured site. + if let uploader = context.uploader { + let body = try await uploader.upload(fileAt: uploadURL, mimeType: uploadMimeType, filename: uploadFilename) + return .uploaded(MediaUploadResponse(statusCode: 201, body: body)) } else if let defaultUploader = context.defaultUploader { // Unmodified — forward the original request body directly, skipping // multipart re-encoding. @@ -460,24 +472,26 @@ enum UploadError: Error, LocalizedError { // MARK: - Upload Context -/// Container for the upload delegate and default uploader, captured by the -/// HTTPServer handler closure and re-read on each request. +/// Container for the media processor, uploader, and default uploader, captured by +/// the HTTPServer handler closure and re-read on each request. /// -/// The delegate is held **weakly**. `EditorViewController.mediaUploadDelegate` is -/// declared `weak` — the host owns the delegate's lifetime. Capturing it strongly -/// here would silently defeat that contract and, worse, risk a retain cycle -/// (`EditorViewController → uploadServer → HTTPServer → handler → UploadContext → -/// delegate → EditorViewController`) that would keep the view controller — and -/// therefore the server — alive forever, so `deinit` would never stop it. +/// The processor and uploader are held **weakly** — the host owns their lifetime +/// (`EditorViewController.mediaProcessor` / `.mediaUploader` are `weak`). Capturing +/// them strongly here would risk a retain cycle (`EditorViewController → +/// uploadServer → HTTPServer → handler → UploadContext → host object → +/// EditorViewController`) that would keep the view controller — and therefore the +/// server — alive forever, so `deinit` would never stop it. /// -/// `@unchecked Sendable`: `uploadDelegate` is assigned once at init and only read -/// afterwards; weak-reference reads are thread-safe at runtime. +/// `@unchecked Sendable`: `processor`/`uploader` are assigned once at init and only +/// read afterwards; weak-reference reads are thread-safe at runtime. private final class UploadContext: @unchecked Sendable { - weak var uploadDelegate: (any MediaUploadDelegate)? + weak var processor: (any MediaProcessor)? + weak var uploader: (any MediaUploader)? let defaultUploader: DefaultMediaUploader? - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate + init(processor: (any MediaProcessor)?, uploader: (any MediaUploader)?, defaultUploader: DefaultMediaUploader?) { + self.processor = processor + self.uploader = uploader self.defaultUploader = defaultUploader } } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 910f49db4..91c7a8da8 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -102,9 +102,9 @@ struct MediaUploadServerTests { @Test("routes /upload with a query string and relays the query") func uploadWithQueryString() async throws { - let delegate = ProcessOnlyDelegate() + let processor = PassthroughProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } // `@wordpress/media-utils` uploads to `/wp/v2/media?_embed=wp:featuredmedia`, @@ -123,7 +123,7 @@ struct MediaUploadServerTests { let (_, response) = try await URLSession.shared.data(for: request) let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // The delegate returns `.original`, so this is the passthrough branch. + // The processor returns `.original`, so this is the passthrough branch. // Pin which branch ran — `lastQuery` is recorded by both, so without this // the query assertion would pass even if routing collapsed onto one path. #expect(mockUploader.passthroughUploadCalled) @@ -131,14 +131,14 @@ struct MediaUploadServerTests { #expect(mockUploader.lastQuery == "?_embed=wp:featuredmedia") } - @Test("routes a deletion to the delegate when it handles one") - func delegateHandlesDeletion() async throws { - // A host that uploaded the attachment itself owns an ID only it can - // resolve, so the default uploader must not be asked to delete it. - // No default uploader is configured, so a 200 here can only come from the - // delegate — the fallback path would fail with "no uploader". - let delegate = DeletingDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("relays a deletion to the default uploader even when an uploader owns uploads") + func deletesGoToDefaultUploaderNotUploader() async throws { + // An attachment lives on the configured site even when a host uploader delivered + // it, so its deletion goes to the default uploader — the host uploader owns + // uploads, not deletes. Held strongly: UploadContext keeps the uploader weakly. + let uploader = MockUploader() + let defaultUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! @@ -150,40 +150,40 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 200) - #expect(delegate.deletedAttachmentId == "42") + #expect(defaultUploader.deleteMediaCalled) + #expect(defaultUploader.deletedAttachmentId == "42") } - @Test("relays the delegate's own Content-Type instead of emitting it twice") - func delegateContentTypeWins() async throws { - // `HTTPResponse` serializes every header it is given, so appending the JSON - // default unconditionally would put `Content-Type` on the wire twice. - // URLSession joins repeated headers with a comma, which is what a - // regression would look like here. - let delegate = ContentTypeDeletingDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("relays a deletion to the default uploader (configured site)") + func relaysDeleteToDefaultUploader() async throws { + // With no uploader set, GutenbergKit owns deletes: core's orphan cleanup DELETE + // is relayed to the default uploader (the configured site). + let mockUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(defaultUploader: mockUploader) defer { server.stop() } - let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! + let url = URL(string: "http://127.0.0.1:\(server.port)/media/512?force=true")! var request = URLRequest(url: url) request.httpMethod = "DELETE" request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") - let (_, response) = try await URLSession.shared.data(for: request) - let httpResponse = try #require(response as? HTTPURLResponse) - let contentType = httpResponse.value(forHTTPHeaderField: "Content-Type") - #expect(contentType == "text/plain") + #expect((response as? HTTPURLResponse)?.statusCode == 200) + #expect(mockUploader.deleteMediaCalled) + #expect(mockUploader.deletedAttachmentId == "512") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("routes an upload to the uploader and relays its attachment") + func uploaderDeliversAttachment() async throws { + // With an uploader set, GutenbergKit hands it the file and relays the finished + // attachment it returns — the default uploader (configured site) is never used. + let uploader = MockUploader() + let defaultUploader = MockDefaultUploader() + let server = try await MediaUploadServer.start(uploader: uploader, defaultUploader: defaultUploader) defer { server.stop() } let boundary = UUID().uuidString - let fileData = "fake image data".data(using: .utf8)! - let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: fileData) + 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) @@ -196,12 +196,14 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) - #expect(delegate.uploadFileCalled) - #expect(delegate.lastMimeType == "image/jpeg") - #expect(delegate.lastFilename == "photo.jpg") + #expect(uploader.uploadCalled) + #expect(uploader.lastMimeType == "image/jpeg") + #expect(uploader.lastFilename == "photo.jpg") + // The host owns delivery — GutenbergKit must not upload to the configured site. + #expect(!defaultUploader.uploadCalled) + #expect(!defaultUploader.passthroughUploadCalled) - // The server relays WordPress's raw response body verbatim. + // The server relays the exact attachment JSON the uploader returned. let object = try JSONSerialization.jsonObject(with: data) let json = try #require(object as? [String: Any]) #expect(json["id"] as? Int == 42) @@ -209,11 +211,11 @@ struct MediaUploadServerTests { #expect(json["media_type"] as? String == "image") } - @Test("uses passthrough when delegate does not modify file") - func delegatePassthrough() async throws { - let delegate = ProcessOnlyDelegate() + @Test("uses passthrough when the processor does not modify the file") + func processorPassthrough() async throws { + let processor = PassthroughProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -231,7 +233,7 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) + #expect(processor.processFileCalled) // Passthrough: original body forwarded directly, not re-encoded. #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) @@ -242,11 +244,11 @@ struct MediaUploadServerTests { #expect(json["id"] as? Int == 99) } - @Test("skips processing and the temp copy when the delegate declines by metadata") - func delegateDeclinesByMetadata() async throws { - let delegate = DeclineByMetadataDelegate() + @Test("skips processing and the temp copy when the processor declines by metadata") + func processorDeclinesByMetadata() async throws { + let processor = DecliningProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -263,18 +265,18 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - // Declined by metadata → the delegate is never asked to process (so the file + // Declined by metadata → the processor is never asked to process (so the file // was never materialized), and the upload is passed through directly. - #expect(!delegate.processFileCalled) + #expect(!processor.processFileCalled) #expect(mockUploader.passthroughUploadCalled) #expect(!mockUploader.uploadCalled) } - @Test("forwards the delegate's processed metadata to the uploader") + @Test("forwards the processor's processed metadata to the uploader") func processedMetadataForwarded() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -289,18 +291,18 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The delegate changed the format, so the uploader must receive the new + // The processor changed the format, so the uploader must receive the new // metadata — not the original video/quicktime + clip.mov. #expect(mockUploader.uploadCalled) #expect(mockUploader.lastUploadMimeType == "video/mp4") #expect(mockUploader.lastUploadFilename == "clip.mp4") } - @Test("deletes the delegate's processed file after upload") + @Test("deletes the processor's processed file after upload") func deletesProcessedFile() async throws { - let delegate = ResizingDelegate() + let processor = ResizingProcessor() let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let server = try await MediaUploadServer.start(processor: processor, defaultUploader: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -315,10 +317,10 @@ struct MediaUploadServerTests { _ = try await URLSession.shared.data(for: request) - // The server owns the file the delegate produced and must delete it once the + // The server owns the file the processor produced and must delete it once the // upload finishes — the defer in processAndUpload covers the success and throw // paths alike. A leaked processed file is a full-size temp per upload. - let processedURL = try #require(delegate.producedURL) + let processedURL = try #require(processor.producedURL) #expect(!FileManager.default.fileExists(atPath: processedURL.path(percentEncoded: false))) } @@ -403,22 +405,22 @@ struct MediaUploadServerTests { #expect(FileManager.default.fileExists(atPath: fresh.path(percentEncoded: false))) } - @Test("does not strongly retain the upload delegate (weak — preserves deinit teardown)") - func doesNotStronglyRetainDelegate() async throws { - weak var weakDelegate: MockUploadDelegate? + @Test("does not strongly retain the processor (weak — preserves deinit teardown)") + func doesNotStronglyRetainProcessor() async throws { + weak var weakProcessor: PassthroughProcessor? let server: MediaUploadServer do { - let delegate = MockUploadDelegate() - weakDelegate = delegate - server = try await MediaUploadServer.start(uploadDelegate: delegate) + let processor = PassthroughProcessor() + weakProcessor = processor + server = try await MediaUploadServer.start(processor: processor) } defer { server.stop() } - // UploadContext holds the delegate weakly, so releasing the host's strong + // UploadContext holds the processor weakly, so releasing the host's strong // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController + // EditorViewController → uploadServer → … → processor → EditorViewController // cycle, so deinit would never fire and the server would never stop. - #expect(weakDelegate == nil) + #expect(weakProcessor == nil) } private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { @@ -809,63 +811,34 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks -private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A host uploader: it performs the upload on its own stack. `upload` returns the +/// finished attachment JSON (or throws). +private final class MockUploader: MediaUploader, @unchecked Sendable { private let lock = NSLock() - private var _processFileCalled = false - private var _uploadFileCalled = false + private var _uploadCalled = false private var _lastMimeType: String? private var _lastFilename: String? + private let uploadBody: Data - var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var uploadFileCalled: Bool { lock.withLock { _uploadFileCalled } } + var uploadCalled: Bool { lock.withLock { _uploadCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } var lastFilename: String? { lock.withLock { _lastFilename } } - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - lock.withLock { - _processFileCalled = true - _lastMimeType = mimeType - } - return .original + init(uploadBody: Data = Data(#"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"#.utf8)) { + self.uploadBody = uploadBody } - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { + func upload(fileAt url: URL, mimeType: String, filename: String) async throws -> Data { lock.withLock { - _uploadFileCalled = true + _uploadCalled = true + _lastMimeType = mimeType _lastFilename = filename } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) + return uploadBody } } -/// A delegate that handles deletions itself, as a host uploading to its own -/// media service would. -private final class DeletingDelegate: MediaUploadDelegate, @unchecked Sendable { - private let lock = NSLock() - private var _deletedAttachmentId: String? - - var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } - - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - lock.withLock { _deletedAttachmentId = attachmentId } - return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) - } -} - -/// A delegate that sets its own `Content-Type`, so the relay must not also -/// append the JSON default. -private final class ContentTypeDeletingDelegate: MediaUploadDelegate, @unchecked Sendable { - func deleteFile(attachmentId: String) async throws -> MediaUploadResponse? { - MediaUploadResponse( - statusCode: 200, - body: Data("deleted".utf8), - headers: ["Content-Type": "text/plain"] - ) - } -} - -private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class PassthroughProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -877,10 +850,10 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`, so the +/// A processor that declines every file by metadata via `handlesFile`, so the /// server must pass through without ever materializing the file or calling /// `processFile`. -private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class DecliningProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -894,12 +867,12 @@ private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked S } } -/// A delegate that produces a new file with changed metadata (e.g. a transcode). -private final class ResizingDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A processor that produces a new file with changed metadata (e.g. a transcode). +private final class ResizingProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _producedURL: URL? - /// The URL of the processed file this delegate wrote, for cleanup assertions. + /// The URL of the processed file this processor wrote, for cleanup assertions. var producedURL: URL? { lock.withLock { _producedURL } } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { @@ -917,14 +890,22 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab private var _lastUploadMimeType: String? private var _lastUploadFilename: String? private var _lastQuery: String? + private var _deleteMediaCalled = false + private var _deletedAttachmentId: String? + + /// The response `upload`/`passthroughUpload` return. `nil` uses a 201 default. + private let uploadResponse: MediaUploadResponse? var uploadCalled: Bool { lock.withLock { _uploadCalled } } var passthroughUploadCalled: Bool { lock.withLock { _passthroughUploadCalled } } var lastUploadMimeType: String? { lock.withLock { _lastUploadMimeType } } var lastUploadFilename: String? { lock.withLock { _lastUploadFilename } } var lastQuery: String? { lock.withLock { _lastQuery } } + var deleteMediaCalled: Bool { lock.withLock { _deleteMediaCalled } } + var deletedAttachmentId: String? { lock.withLock { _deletedAttachmentId } } - init() { + init(uploadResponse: MediaUploadResponse? = nil) { + self.uploadResponse = uploadResponse super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) } @@ -935,7 +916,7 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab _lastUploadFilename = filename _lastQuery = query } - return mockResponse() + return uploadResponse ?? Self.defaultResponse } override func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { @@ -943,13 +924,20 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab _passthroughUploadCalled = true _lastQuery = query } - return mockResponse() + return uploadResponse ?? Self.defaultResponse } - private func mockResponse() -> MediaUploadResponse { - let json = #"{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) + override func deleteMedia(attachmentId: String, query: String) async throws -> MediaUploadResponse { + lock.withLock { + _deleteMediaCalled = true + _deletedAttachmentId = attachmentId + } + return MediaUploadResponse(statusCode: 200, body: Data(#"{"deleted":true}"#.utf8)) } + + private static let defaultResponse = MediaUploadResponse( + statusCode: 201, + body: Data(#"{"id":99,"source_url":"https://example.com/doc.pdf","media_type":"file"}"#.utf8)) } private struct MockHTTPClient: EditorHTTPClientProtocol {