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 {