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 c0a47f9df..fb8e8749c 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -137,29 +137,60 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for customizing media upload behavior (resize, transcode, - * custom upload). + * Optional processor that transforms media before upload (resize, transcode, + * strip EXIF). + * + * To perform the upload yourself, set [mediaUploader] instead. * * 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. */ - 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, + * and this view owns it for its lifetime — so you needn't retain it yourself, just + * don't strongly retain this [GutenbergView] from your uploader. + * + * A [mediaProcessor] can still transform the file first; only delivery moves + * to the uploader. + */ + var mediaUploader: MediaUploader? = null + set(value) { + check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } + // An uploader's media deletes still relay through the internal media client, + // which needs a site root and an auth header to reach the configured site. + // Check it here, where the host hands the uploader over, rather than at + // server start: the stack trace names the caller's own line, and the mistake + // can't hide until the page loads. `configuration` is assigned in the + // constructor, so it is always available by the time this runs. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot = configuration.siteApiRoot, + authHeader = configuration.authHeader, + hasUploader = value != null + ) 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 + * configuration has been captured. After this the [mediaProcessor] can no * longer take effect, so its setter throws. */ @Volatile private var hasStartedLoading = false @@ -722,13 +753,13 @@ class GutenbergView : FrameLayout { /** * Invoked when any page begins loading in the main frame. Resets readiness for * every page; for the editor document alone, starts the upload server once — - * capturing the [mediaUploadDelegate] provided before load — then advertises the + * capturing the [mediaProcessor] provided before load — then advertises the * editor globals (including the server's port and token). * * Starting the server here, on the UI thread, rather than from the - * [mediaUploadDelegate] setter keeps its whole lifecycle — start here, stop in + * [mediaProcessor] setter keeps its whole lifecycle — start here, stop in * [onDetachedFromWindow] — on the UI thread, so it can't race a - * background-thread delegate assignment. + * background-thread processor assignment. */ private fun onEditorPageStarted(url: String?) { // Readiness belongs to the page: a new page, including one a reload starts, @@ -762,17 +793,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 — leave it down and let uploads fall to the default + // WebView path. (Matches iOS.) + if (mediaProcessor == null && mediaUploader == null) return + + // An InternalMediaClient 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 (the editor injects the latter because the WebView + // has no auth cookies). Without them 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. + // + // Only a mediaProcessor can reach this return: a mediaUploader without + // credentials already failed in its setter, so by here it has them. + if (!MediaServerCredentials.areUsable(configuration.siteApiRoot, configuration.authHeader)) return // The editor reaches the loopback server over cleartext http://localhost. If // the host app's network-security config doesn't permit cleartext to @@ -780,7 +816,14 @@ class GutenbergView : FrameLayout { // before it leaves the page. Detect that here and don't start the server, so // the JS middleware routes uploads down the default path instead of a server // it can never reach. Hosts that want native media processing must permit - // cleartext to localhost (see the demo's res/xml/network_security_config.xml). + // cleartext to localhost — see "Android: permit cleartext to localhost" in + // docs/integration.md, and the demo's res/xml/network_security_config.xml. + // + // This drops a mediaUploader as silently as missing credentials would, and still + // only warns. The difference is the cause, not the symptom: the configuration is + // sound here — permit cleartext and the same setup works unchanged — so there is + // nothing for the host to fix in what it handed us. See + // MediaServerCredentials.requireCredentialsForUploader. if (!NetworkSecurityPolicy.getInstance().isCleartextTrafficPermitted(LOOPBACK_HOST)) { Log.w( TAG, @@ -792,15 +835,16 @@ class GutenbergView : FrameLayout { } try { - val defaultUploader = DefaultMediaUploader( + val internalClient = InternalMediaClient( httpClient = uploadHttpClient, siteApiRoot = configuration.siteApiRoot, authHeader = configuration.authHeader, siteApiNamespace = configuration.siteApiNamespace.toList() ) uploadServer = MediaUploadServer( - uploadDelegate = mediaUploadDelegate, - defaultUploader = defaultUploader, + processor = mediaProcessor, + internalClient = internalClient, + uploader = mediaUploader, cacheDir = context.cacheDir, scope = coroutineScope ) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt new file mode 100644 index 000000000..d173c8087 --- /dev/null +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaServerCredentials.kt @@ -0,0 +1,83 @@ +package org.wordpress.gutenberg + +import android.net.Uri + +/** + * Whether the editor configuration can reach the configured site for media, and the + * fail-fast that enforces it. + * + * The counterpart of iOS's `MediaServerCredentials`, kept deliberately close to it. + * This is a *crash* policy, and it has already diverged silently between the platforms + * once: iOS required an absolute site root while this side checked only `isEmpty()`, so + * a scheme-less root trapped on iOS and started a server whose every delete failed on + * Android. Both platforms keep the policy in a type of this name so the two can be + * diffed against each other rather than hunted for across view code. + */ +internal object MediaServerCredentials { + /** + * Whether an [InternalMediaClient] built from this configuration could actually + * reach the site. + * + * Both fields are required. The client delivers GutenbergKit's uploads to the + * configured site, so it needs somewhere to send them and credentials to be + * accepted; with either missing, every media request it makes fails. + * + * "Somewhere to send them" means an *absolute* root, not merely a non-empty one. + * OkHttp rejects a scheme-less URL from `Request.Builder.url` with + * `IllegalArgumentException`, which is not an `IOException` — so it escapes + * [MediaUploadServer]'s delete handler and degrades to a generic 500 the editor + * cannot parse into an error, and the orphan cleanup that delete exists for fails + * silently. An empty root parses to the same nulls, so this still rejects + * everything the older `isEmpty()` check did. + * + * Emptiness is tested as well as nullity because `Uri` and Swift's `URL` disagree + * on how they report a missing authority: `file:///tmp/wp-json` yields a `null` + * host on iOS but an *empty* one here. Treating both as absent is what keeps the + * two predicates answering alike. + */ + fun areUsable(siteApiRoot: String, authHeader: String): Boolean { + if (authHeader.isEmpty()) return false + val uri = Uri.parse(siteApiRoot) + return !uri.scheme.isNullOrEmpty() && !uri.host.isNullOrEmpty() + } + + /** + * Throws if the host supplied a [MediaUploader] without usable credentials. + * + * The behavior forks by intent: + * + * - A [MediaProcessor] only enhances GutenbergKit-owned uploads. With no + * credentials there is nothing to deliver through, so nothing to process — the + * server simply stays down and uploads fall to the default WebView path. That is + * [areUsable]'s job, at the point the server would start. + * + * - A [MediaUploader] means the host is *taking over* uploads, and falling back + * would drop that whole stack — its queueing, its retries — while media appeared + * to keep working. Worth failing over rather than logging. + * + * What makes it a *failure* rather than a warning is that the configuration is + * incoherent, not merely unlucky: an uploader's media deletes still relay through + * the internal media client, so there is no site root and auth header under which + * this host's uploader could have worked. Contrast the conditions the host's + * environment imposes at server start — the network policy that blocks cleartext to + * localhost, a port that won't bind — which log and degrade, because the very same + * configuration works once the environment allows it. Dropping the uploader is the + * symptom both share; only this one has a cause the host can fix in the + * configuration it just handed over. + * + * Called from [GutenbergView.mediaUploader]'s setter, not from the server start — + * the earliest point available here, since this platform takes its media handlers + * as mutable properties rather than at construction. Checking where the host hands + * the uploader over puts the caller's own line in the stack trace, instead of + * surfacing the mistake later from inside a page-load callback. (iOS checks in + * `EditorViewController.init`, for the same reason.) + */ + fun requireCredentialsForUploader(siteApiRoot: String, authHeader: String, hasUploader: Boolean) { + if (!hasUploader) return + check(areUsable(siteApiRoot, authHeader)) { + "A mediaUploader needs site credentials so GutenbergKit can relay the " + + "editor's media deletes to the configured site. Set an absolute " + + "siteApiRoot and the auth header in the editor configuration." + } + } +} 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 6ce861d4b..05cfb9f4d 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -6,6 +6,8 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.cancel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.suspendCancellableCoroutine import kotlin.coroutines.resume @@ -31,8 +33,11 @@ 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, or 201 for an upload a + * [MediaUploader] delivered. + */ val statusCode: Int, /** * The raw response body — a WordPress REST attachment on success, or a @@ -52,14 +57,14 @@ class MediaUploadResponse( ) /** - * The result of a delegate's [MediaUploadDelegate.processFile]. + * The result of a processor's [MediaProcessor.processFile]. */ sealed class ProcessedProxyFile { - /** The delegate did not modify the file; the original upload is forwarded unchanged. */ + /** The processor did not modify the file; the original upload is forwarded unchanged. */ data object Original : ProcessedProxyFile() /** - * The delegate produced a file to upload, along with its MIME type and + * The processor produced a file to upload, along with its MIME type and * filename. Both are used verbatim, so a format change (e.g. transcoding MOV * to MP4, or an in-place EXIF strip) must report the resulting type and * filename for WordPress to store the file correctly. @@ -68,23 +73,30 @@ sealed class ProcessedProxyFile { } /** - * Interface for customizing media upload behavior. + * Transforms media before GutenbergKit delivers it. + * + * 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, it cannot deliver media to the wrong place. Set + * [GutenbergView.mediaProcessor] to resize images, transcode video, strip EXIF, + * etc. * - * The native host app can provide an implementation to resize images, - * transcode video, or use its own upload service. + * This is the safe, common extension point: most hosts want only this. To perform the + * upload yourself, implement [MediaUploader] instead. */ -interface MediaUploadDelegate { +interface MediaProcessor { /** - * Whether this delegate might handle a file with the given metadata — either - * processing it ([processFile]) or uploading it itself ([uploadFile]). + * 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. + * image-only processor returning false for a video — so the server forwards + * the original upload to WordPress without first copying a file the processor + * won't touch. + * + * With a [MediaUploader] set this can't decline the upload itself — an uploader + * delivers every file, so there is no passthrough to fall to — but it still gates + * [processFile]: a declined file reaches the uploader unprocessed. * * Defaults to true: every file is materialized and the full pipeline runs. A * true here is not a commitment — [processFile] may still return @@ -101,16 +113,81 @@ interface MediaUploadDelegate { * stores it with the correct extension and type. */ suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original +} +/** + * One of the editor's non-file form fields, as sent with a media upload. + * + * A named type rather than a pair so the field's meaning is legible at every call + * site, and so the type can gain members without a source break for every host. + * + * @property name The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + * @property value The field's value, decoded as UTF-8. + */ +data class MediaUploadField(val name: String, val value: String) + +/** + * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, + * its metadata, the editor's non-file form fields, and the request's query. + * + * @property file The file to upload — already processed, if a [MediaProcessor] ran. + * @property mimeType The file's MIME type. + * @property filename The file's name. + * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — + * most importantly `post`, the parent post's ID, without which the attachment is + * created unattached. A list, not a map, so repeated field names (e.g. a `field[]` + * array) survive verbatim. Send each as a form part on your `POST /wp/v2/media`, in + * the given order. + * @property query The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), + * or empty. Carry it on your request so the editor gets the response it expects. + */ +data class MediaUpload( + val file: File, + val mimeType: String, + val filename: String, + val fields: List, + val query: String +) + +/** + * Takes over *performing* a media upload — on the host's own stack: its own + * networking (say, to log every request), a background service, an offline queue, a + * resumable transport, its own retry policy. + * + * This is a choice of *who executes the requests*, not where they go: an uploader and + * GutenbergKit's internal media client both target the same configured site. Setting + * [GutenbergView.mediaUploader] makes the host own that upload end-to-end — the + * request, its own retries, and its recovery and cleanup — with GutenbergKit out of + * the network entirely. Because the host does the retries itself, there's no raw + * response left for the editor to retry behind it. + */ +interface MediaUploader { /** - * Upload a processed file to the remote WordPress site. + * Upload a (possibly processed) file and return the finished WordPress attachment + * JSON the editor inserts — the same object a direct `POST /wp/v2/media` returns. + * Return only once the upload is genuinely done, or throw on terminal failure: a + * returned value is taken as a completed attachment, and there is no GutenbergKit + * recovery behind you. + * + * The [MediaUpload] carries the file plus the editor's form fields (e.g. `post`) + * and query — send them all so the created attachment matches a native upload + * rather than landing as an unattached orphan. * - * 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. + * 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. That request needs a body of `{"action": "create-image-subsizes"}` — + * core registers `action` as **required**, so a post-process request without it + * fails with a 400 every time rather than recovering. + * + * Owning the upload means owning cleanup on the server too: if post-process can't + * be recovered, force-delete the orphan (`DELETE /wp/v2/media/?force=true`) + * before you throw, or it stays on the site — neither GutenbergKit nor the editor + * cleans up behind you. */ - suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null + suspend fun upload(upload: MediaUpload): ByteArray } /** @@ -126,8 +203,9 @@ interface MediaUploadDelegate { * stop on detach. */ internal class MediaUploadServer( - private val uploadDelegate: MediaUploadDelegate?, - private val defaultUploader: DefaultMediaUploader?, + private val processor: MediaProcessor?, + private val internalClient: InternalMediaClient?, + private val uploader: MediaUploader? = null, cacheDir: File? = null, scope: CoroutineScope? = null, ioDispatcher: CoroutineDispatcher = Dispatchers.IO @@ -264,9 +342,9 @@ internal class MediaUploadServer( * browser blocks it at preflight. Relaying it here lets the cleanup run. */ private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { - val uploader = defaultUploader ?: return errorResponse(500, "No uploader configured") + val client = internalClient ?: return errorResponse(500, "No internal media client configured") return try { - relayResponse(uploader.deleteMedia(attachmentId, query)) + relayResponse(client.deleteMedia(attachmentId, query)) } catch (e: IOException) { Log.e(TAG, "Media deletion failed", e) errorResponse(500, e.message ?: "Deletion failed") @@ -286,18 +364,25 @@ internal class MediaUploadServer( val mimeType = filePart.contentType val filename = filePart.filename ?: "upload" - // Ask the delegate — from metadata alone — whether it will touch a file + // Ask the processor — 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) { + // skipping a full temp-file copy of a file the processor won't process + // (e.g. a video handed to an image-only processor). + // An uploader takes over delivery for *every* file, so with one set there is no + // passthrough to fall to and the gate can't decline the upload outright. It + // still decides whether processFile runs, though — a declined file is handed to + // the uploader unprocessed rather than to a processor that said it won't touch it + // — so the answer is carried into processAndUpload rather than short-circuited + // away here. Asked exactly once per upload, matching iOS. + val processorWantsFile = processor?.handlesFile(mimeType, filename) == true + if (uploader == null && !processorWantsFile) { return passthroughResponse(request, query) } val tempFile = writePartToTempFile(filePart) ?: return errorResponse(500, "Failed to save file") - return processAndRespond(request, tempFile, filePart, extraParts, query) + return processAndRespond(request, tempFile, filePart, extraParts, query, processorWantsFile) } @Suppress("TooGenericExceptionCaught") @@ -325,7 +410,7 @@ internal class MediaUploadServer( * The response's own `Content-Type` wins over the JSON default, matched * case-insensitively — HTTP header names are case-insensitive, and * [HttpResponse] serializes every entry it is given, so a plain map merge - * would emit the name twice for a delegate that spells it `content-type`. + * would emit the name twice for a processor that spells it `content-type`. */ private fun relayResponse(response: MediaUploadResponse): HttpResponse { val hasContentType = response.headers.keys.any { it.lowercase() == "content-type" } @@ -381,11 +466,12 @@ internal class MediaUploadServer( @Suppress("TooGenericExceptionCaught") private suspend fun processAndRespond( request: HttpRequest, tempFile: File, filePart: MultipartPart, - extraParts: List, query: String + extraParts: List, query: String, processorWantsFile: Boolean ): HttpResponse { try { val uploadResult = processAndUpload( - tempFile, filePart.contentType, filePart.filename ?: "upload", extraParts, query + tempFile, filePart.contentType, filePart.filename ?: "upload", + extraParts, query, processorWantsFile ) val response = when (uploadResult) { is UploadResult.Uploaded -> { @@ -393,7 +479,7 @@ internal class MediaUploadServer( uploadResult.response } is UploadResult.Passthrough -> { - // Delegate didn't modify the file — forward the original + // The processor didn't modify the file — forward the original // request body to WordPress without re-encoding. Log.d(TAG, "Passthrough: forwarding original request body to WordPress") performPassthroughUpload(request, query) @@ -407,11 +493,12 @@ internal class MediaUploadServer( throw e // Never swallow coroutine cancellation. } catch (e: Exception) { // Any other failure — IOException from the upload call, JSON parse - // errors, a throwing host delegate, or "no uploader configured" — - // must still be answered WITH CORS headers. Otherwise it escapes to - // HttpServer's header-less 500 fallback and the browser rejects the - // preflighted cross-origin fetch with an opaque "Failed to fetch", - // hiding the real error from the editor (mirrors the iOS catch-all). + // errors, a throwing host processor, or "no internal media client + // configured" — must still be answered WITH CORS headers. Otherwise + // it escapes to HttpServer's header-less 500 fallback and the browser + // rejects the preflighted cross-origin fetch with an opaque "Failed to + // fetch", hiding the real error from the editor (mirrors the iOS + // catch-all). Log.e(TAG, "Upload failed", e) return errorResponse(500, e.message ?: "Upload failed") } finally { @@ -419,7 +506,7 @@ internal class MediaUploadServer( } } - // MARK: - Delegate Pipeline + // MARK: - Processor Pipeline private sealed class UploadResult { data class Uploaded(val response: MediaUploadResponse) : UploadResult() @@ -429,21 +516,32 @@ internal class MediaUploadServer( private suspend fun performPassthroughUpload(request: HttpRequest, query: String): MediaUploadResponse { val body = request.body val contentType = request.header("Content-Type") - val uploader = defaultUploader - if (body == null || contentType == null || uploader == null) { - throw MediaUploadException("Passthrough upload requires a request body, Content-Type, and default uploader") + val client = internalClient + if (body == null || contentType == null || client == null) { + throw MediaUploadException( + "Passthrough upload requires a request body, Content-Type, and internal media client" + ) } - return uploader.passthroughUpload(body, contentType, query) + return client.passthroughUpload(body, contentType, query) } private suspend fun processAndUpload( file: File, mimeType: String, filename: String, - extraParts: List, query: String + extraParts: List, query: String, processorWantsFile: Boolean ): UploadResult { - val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + // Process (resize, transcode, etc.) — but only for a file the processor's + // metadata gate accepted. handlesFile returning false is the processor saying it + // won't touch a file like this, so handing it one anyway would break the + // contract the gate documents. With an uploader set the file still gets + // delivered; it just skips processing on its way there. + val processed = if (processorWantsFile) { + processor?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original + } else { + ProcessedProxyFile.Original + } // Resolve the file to upload and its metadata. Processed uses the - // delegate's values verbatim, so a format change is reported to WordPress. + // processor's values verbatim, so a format change is reported to WordPress. val targetFile: File val targetMimeType: String val targetFilename: String @@ -461,9 +559,25 @@ internal class MediaUploadServer( } try { - // If the delegate provided its own upload, use that. - uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { - return UploadResult.Uploaded(it) + // The editor was torn down (or the client disconnected) while we processed. + // Don't put an upload on the wire whose response nobody will read — it would + // create an attachment neither GutenbergKit nor the host knows to clean up. + // Checking here rather than relying on the delivery path to notice keeps this + // true for a host uploader that isn't cancellation-cooperative. (Matches iOS.) + currentCoroutineContext().ensureActive() + + // An uploader owns delivery on the host's own stack and returns the finished + // attachment JSON (or throws); GutenbergKit relays that as a success and + // never runs its own recovery behind it. + uploader?.let { hostUploader -> + val upload = MediaUpload( + file = targetFile, + mimeType = targetMimeType, + filename = targetFilename, + fields = formFields(extraParts), + query = query + ) + return UploadResult.Uploaded(MediaUploadResponse(201, hostUploader.upload(upload))) } // Unmodified — forward the original request body directly, skipping @@ -472,11 +586,11 @@ internal class MediaUploadServer( return UploadResult.Passthrough } - val result = defaultUploader?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No upload delegate or default uploader configured") + val result = internalClient?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) + ?: error("No media uploader or internal media client configured") return UploadResult.Uploaded(result) } finally { - // 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 — covers the success and throw paths alike. if (targetFile != file) { targetFile.delete() @@ -484,6 +598,31 @@ internal class MediaUploadServer( } } + /** + * The editor's non-file form parts as ordered, UTF-8-decoded fields. + * + * A list rather than a map so repeated names (e.g. a `field[]` array) survive + * verbatim, in the order the editor sent them. + * + * This decode can't mangle anything, but only because of who is on the other end — + * nothing in the code enforces it. Three things have to stay true: + * + * 1. Only the editor's own web page can reach this server. It listens on loopback, + * and every request has to carry a per-session token. + * 2. Text the editor puts in a form field is already valid Unicode. The browser + * guarantees that when the value is set, so it cannot hand us bad bytes. + * 3. The only way a browser can put *raw* bytes in a form is a file or a Blob, and + * those always arrive with a filename. Anything with a filename is handled as the + * file, never as a field — so raw bytes never reach this decode. + * + * If one of those stops being true, bad bytes quietly turn into replacement + * characters, and the platforms don't even agree on how many: ED A0 80 becomes one + * of them here and three on iOS. There is no single behavior worth documenting, so + * the tests pin rule 3 instead. + */ + private fun formFields(parts: List): List = + parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } + // MARK: - Response Building private fun errorResponse(status: Int, message: String): HttpResponse { @@ -528,9 +667,15 @@ internal class MediaUploadServer( internal class MediaUploadException(message: String, cause: Throwable? = null) : Exception(message, cause) /** - * Uploads files to the WordPress REST API using OkHttp. + * GutenbergKit's own client for the configured site, built from the site credentials + * in the editor configuration. + * + * Not an implementation of any host-facing interface — it is the thing that actually + * performs GutenbergKit's media requests. It delivers uploads the host did not take + * over, and relays the editor's media deletes: the editor only ever asks to delete + * `/wp/v2/media/` on the configured site, so that is where the relay sends it. */ -internal open class DefaultMediaUploader( +internal open class InternalMediaClient( private val httpClient: okhttp3.OkHttpClient, private val siteApiRoot: String, private val authHeader: String, @@ -568,10 +713,10 @@ internal open class DefaultMediaUploader( ): MediaUploadResponse { val mediaType = mimeType.toMediaType() val builder = okhttp3.MultipartBody.Builder().setType(okhttp3.MultipartBody.FORM) - // Preserve the non-file parts (post, additionalData) through the re-encode. - // Append each field's raw bytes (not via String) so a non-UTF-8 value is - // forwarded verbatim rather than coerced. filename=null makes it a plain - // field, matching okhttp's String overload byte-for-byte. + // Non-file parts (post, additionalData) have to survive the re-encode unchanged: + // appending raw bytes keeps them byte-for-byte identical to the plain passthrough, + // and filename=null makes each a plain field, exactly what okhttp's String overload + // would emit. (Bad bytes can't get here; see formFields.) for (part in extraParts) { builder.addFormDataPart(part.name, null, part.body.readBytes().toRequestBody()) } @@ -589,7 +734,7 @@ internal open class DefaultMediaUploader( /** * Forwards the original request body to WordPress without re-encoding. * - * Used when the delegate's `processFile` returned the file unchanged — + * Used when the processor's `processFile` returned the file unchanged — * the incoming multipart body is already valid for WordPress. */ open suspend fun passthroughUpload( 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 85dd1930b..9935fa102 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewUploadServerTest.kt @@ -3,6 +3,7 @@ package org.wordpress.gutenberg import android.os.Looper import android.view.View import kotlinx.coroutines.test.TestScope +import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertThrows @@ -33,10 +34,10 @@ class GutenbergViewUploadServerTest { private val testScope = TestScope() - private fun makeView(): GutenbergView { + private fun makeView(authHeader: String = "Bearer test", siteApiRoot: String = "https://example.com/wp-json/"): GutenbergView { val config = EditorConfiguration - .builder("https://example.com", "https://example.com/wp-json/") - .setAuthHeader("Bearer test") + .builder("https://example.com", siteApiRoot) + .setAuthHeader(authHeader) .build() return GutenbergView( config, @@ -55,7 +56,7 @@ class GutenbergViewUploadServerTest { /** * Invokes the private `onEditorPageStarted` hook (fired from the WebViewClient's * `onPageStarted`) to simulate the editor page beginning to load — the point at - * which the delegate is captured and the upload server starts. + * which the processor is captured and the upload server starts. */ private fun startLoading(view: GutenbergView, url: String = EDITOR_URL) { val method = GutenbergView::class.java.getDeclaredMethod( @@ -76,15 +77,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 { @@ -93,14 +94,14 @@ class GutenbergViewUploadServerTest { } @Test - fun `no delegate means no upload server`() { + fun `no processor means no upload server`() { val view = makeView() try { - // No delegate provided — uploads should use the default WebView path. + // No processor provided — uploads should use the default WebView path. startLoading(view) idle() assertNull( - "with no delegate, no upload server should be started", + "with no processor, no upload server should be started", uploadServerOf(view) ) } finally { @@ -108,11 +109,30 @@ class GutenbergViewUploadServerTest { } } + @Test + fun `the upload server starts for an uploader with no processor`() { + val view = makeView() + try { + // An uploader alone must bring the server up: it is the only route the + // editor has to the host's upload stack. Without this, `startUploadServer` + // could drop the `mediaUploader` clause from its gate and stay green. + view.mediaUploader = mock(MediaUploader::class.java) + startLoading(view) + idle() + assertNotNull( + "an uploader provided before load should bring up the upload server", + uploadServerOf(view) + ) + } finally { + detach(view) // stops the server, releasing the bound socket + } + } + @Test fun `a non-editor page does not start the upload server`() { val view = makeView() try { - view.mediaUploadDelegate = mock(MediaUploadDelegate::class.java) + view.mediaProcessor = mock(MediaProcessor::class.java) startLoading(view, "https://example.com/assets/support.html") idle() assertNull( @@ -125,15 +145,139 @@ class GutenbergViewUploadServerTest { } @Test - fun `setting the delegate after the page has started loading throws`() { + fun `setting the uploader after the page has started loading throws`() { + val view = makeView() + try { + startLoading(view) + idle() + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `an uploader without credentials fails at assignment`() { + // Falling back would silently drop the uploader, and its media deletes still + // need the internal media client to reach the configured site. It fails in the + // setter rather than at page load so the stack trace names the host's own line. + val view = makeView(authHeader = "") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + // The failed assignment left nothing behind, so loading still works — + // it just falls to the default WebView path. + startLoading(view) + idle() + assertNull("no server should be left behind by the rejected uploader", uploadServerOf(view)) + } finally { + detach(view) + } + } + + @Test + fun `an uploader without a site root fails at assignment too`() { + val view = makeView(siteApiRoot = "") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `an uploader with a scheme-less site root fails at assignment`() { + // What a user types when asked for their site address. This used to pass the + // isEmpty() check and start a server whose every relayed delete threw + // IllegalArgumentException out of OkHttp — while the same config trapped on + // iOS, whose check has always required an absolute root. + val view = makeView(siteApiRoot = "example.com/wp-json/") + try { + assertThrows(IllegalStateException::class.java) { + view.mediaUploader = mock(MediaUploader::class.java) + } + } finally { + detach(view) + } + } + + @Test + fun `a processor with a scheme-less site root leaves the server down`() { + // Same root, no uploader: not an error, but the server must still stay down + // rather than come up and fail every request. (Matches iOS.) + val view = makeView(siteApiRoot = "example.com/wp-json/") + try { + view.mediaProcessor = mock(MediaProcessor::class.java) + startLoading(view) + idle() + assertNull( + "a scheme-less site root should not bring up the server", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + + @Test + fun `an uploader with credentials assigns cleanly`() { + val view = makeView() + try { + val uploader = mock(MediaUploader::class.java) + view.mediaUploader = uploader + assertEquals(uploader, view.mediaUploader) + } finally { + detach(view) + } + } + + @Test + fun `a processor without credentials assigns cleanly`() { + // Only an uploader requires credentials — a processor has nothing to deliver + // through, so assigning one with no credentials is not an error. + val view = makeView(authHeader = "") + try { + val processor = mock(MediaProcessor::class.java) + view.mediaProcessor = processor + assertEquals(processor, view.mediaProcessor) + } finally { + detach(view) + } + } + + @Test + fun `a processor without credentials just leaves the server down`() { + // Nothing to deliver through, so nothing to process — uploads fall to the + // default WebView path rather than trapping. + val view = makeView(authHeader = "") + try { + view.mediaProcessor = mock(MediaProcessor::class.java) + startLoading(view) + idle() + assertNull( + "a processor with no credentials should not bring up the server", + uploadServerOf(view) + ) + } finally { + detach(view) + } + } + + @Test + fun `setting the processor 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) @@ -143,7 +287,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/MediaServerCredentialsTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaServerCredentialsTest.kt new file mode 100644 index 000000000..6dacbac56 --- /dev/null +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaServerCredentialsTest.kt @@ -0,0 +1,95 @@ +package org.wordpress.gutenberg + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * The counterpart of iOS's `MediaServerCredentialsTests`. The two suites assert the + * same cases on purpose — this policy already diverged silently between the platforms + * once, and matching cases are what makes a future divergence show up as a failing + * test rather than as a crash on one platform and a broken server on the other. + * + * Robolectric is required only because [MediaServerCredentials] parses with + * `android.net.Uri`, which is a framework class. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [28], manifest = Config.NONE) +class MediaServerCredentialsTest { + + private val siteRoot = "https://example.com/wp-json/" + + @Test + fun `accepts an absolute site root with an auth header`() { + assertTrue(MediaServerCredentials.areUsable(siteRoot, "Bearer t")) + } + + @Test + fun `rejects an empty auth header`() { + assertFalse(MediaServerCredentials.areUsable(siteRoot, "")) + } + + // The two arms below are the ones an `isEmpty()` check used to let through. + + @Test + fun `rejects a site root with no scheme`() { + // What a user types when asked for their site address. OkHttp rejects the + // resulting URL with IllegalArgumentException, which is not an IOException. + assertFalse(MediaServerCredentials.areUsable("example.com/wp-json/", "Bearer t")) + } + + @Test + fun `rejects a site root with no host`() { + assertFalse(MediaServerCredentials.areUsable("file:///tmp/wp-json", "Bearer t")) + } + + @Test + fun `rejects an empty site root, the default when a host configures none`() { + assertFalse(MediaServerCredentials.areUsable("", "Bearer t")) + } + + // MARK: - requireCredentialsForUploader + + @Test + fun `accepts usable credentials, uploader or not`() { + for (hasUploader in listOf(true, false)) { + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "Bearer t", hasUploader) + } + } + + @Test + fun `ignores missing credentials when there is no uploader`() { + // Nothing to deliver through, so nothing to process. This is not an error — the + // server just stays down (areUsable decides that) and uploads fall to the + // default WebView path, so a processor-only host must not fail here. + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "", hasUploader = false) + } + + @Test + fun `throws for an uploader with no auth header`() { + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader(siteRoot, "", hasUploader = true) + } + } + + @Test + fun `throws for an uploader with no site root`() { + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader("", "Bearer t", hasUploader = true) + } + } + + @Test + fun `throws for an uploader with a scheme-less site root`() { + // The case that previously trapped on iOS and started a doomed server here. + assertThrows(IllegalStateException::class.java) { + MediaServerCredentials.requireCredentialsForUploader( + "example.com/wp-json/", "Bearer t", hasUploader = true + ) + } + } +} 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 e7d10e942..ff22f8c1f 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, internalClient = 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, internalClient = null, cacheDir = tempFolder.root) val ownedScope = ownedScopeOf(owningServer) assertNotNull("server should own a scope when none is supplied", ownedScope) assertTrue(ownedScope!!.isActive) @@ -63,8 +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, - defaultUploader = null, + processor = null, + internalClient = null, cacheDir = tempFolder.root, scope = callerScope ) @@ -150,9 +150,9 @@ class MediaUploadServerTest { // Exercised through the delete relay because every response relayResponse // handles — WordPress's own included — carries a `Content-Type`, so this is // the ordinary path rather than an edge case. - val uploader = ContentTypeDeleteUploader() + val uploader = ContentTypeDeleteClient() server.stop() - server = MediaUploadServer(uploadDelegate = null, defaultUploader = uploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = null, internalClient = uploader, cacheDir = tempFolder.root) val response = sendRawRequest( method = "DELETE", @@ -168,12 +168,230 @@ class MediaUploadServerTest { assertEquals(listOf("text/plain"), response.rawHeaderValues("content-type")) } + @Test + fun `an uploader performs the upload and its result is relayed`() { + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = client, uploader = uploader, cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-uploader" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) + val response = sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) + assertTrue(response.body.contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + assertFalse(client.uploadCalled) + assertFalse(client.passthroughUploadCalled) + assertEquals("photo.jpg", uploader.received?.filename) + assertEquals("image/jpeg", uploader.received?.mimeType) + } + + @Test + fun `an uploader receives the editor's form fields in order, and the query`() { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a map. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-fields" + val body = java.io.ByteArrayOutputStream().apply { + for ((name, value) in listOf("post" to "42", "tags[]" to "a", "tags[]" to "b")) { + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"$name\"\r\n\r\n".toByteArray()) + write("$value\r\n".toByteArray()) + } + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload?_embed=wp:featuredmedia", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals( + listOf( + MediaUploadField("post", "42"), + MediaUploadField("tags[]", "a"), + MediaUploadField("tags[]", "b") + ), + uploader.received?.fields + ) + assertEquals("?_embed=wp:featuredmedia", uploader.received?.query) + } + + @Test + fun `a processor still processes the file an uploader delivers`() { + // With both set, the processor still processes — only delivery moves to + // the uploader. + val uploader = RecordingUploader() + val processor = ProcessOnlyProcessor() + val client = MockInternalMediaClient() + server.stop() + server = MediaUploadServer( + processor = processor, internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-precedence" + val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "data".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertNotNull(uploader.received) + assertTrue(processor.processFileCalled) + assertFalse(client.uploadCalled) + } + + @Test + fun `keeps a binary Blob part out of an uploader's fields`() { + // Pin rule 3: a Blob always has a filename, so it's dropped before the decode. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-blob" + val body = java.io.ByteArrayOutputStream().apply { + // Ordered as uploadToServer emits it: the file first, then additionalData. + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"post\"\r\n\r\n".toByteArray()) + write("42\r\n".toByteArray()) + // A Blob-shaped part: it has a filename, and its bytes are not valid UTF-8. + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"blob\"; filename=\"blob\"\r\n".toByteArray()) + write("Content-Type: application/octet-stream\r\n\r\n".toByteArray()) + write(byteArrayOf(0xED.toByte(), 0xA0.toByte(), 0x80.toByte())) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + // The Blob is dropped rather than decoded, and file is still the file. + assertEquals("photo.jpg", uploader.received?.filename) + assertEquals(listOf(MediaUploadField("post", "42")), uploader.received?.fields) + } + + @Test + fun `round-trips a non-Latin field value exactly`() { + // The other half: valid UTF-8 round-trips, so real captions and titles survive. + val uploader = RecordingUploader() + server.stop() + server = MediaUploadServer( + processor = null, internalClient = MockInternalMediaClient(), uploader = uploader, + cacheDir = tempFolder.root + ) + + val caption = "Grüße 🎉 日本語" + val boundary = "test-boundary-utf8" + val body = java.io.ByteArrayOutputStream().apply { + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"caption\"\r\n\r\n".toByteArray()) + write("$caption\r\n".toByteArray()) + write("--$boundary\r\n".toByteArray()) + write("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n".toByteArray()) + write("Content-Type: image/jpeg\r\n\r\n".toByteArray()) + write("fake image data".toByteArray()) + write("\r\n--$boundary--\r\n".toByteArray()) + }.toByteArray() + + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals(listOf(MediaUploadField("caption", caption)), uploader.received?.fields) + } + + @Test + fun `an uploader sees a file the processor's metadata gate would have declined`() { + // The gate exists to skip a temp copy for a file the processor won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + val uploader = RecordingUploader() + val client = MockInternalMediaClient() + val processor = DeclineByMetadataProcessor() + server.stop() + server = MediaUploadServer( + processor = processor, internalClient = client, uploader = uploader, + cacheDir = tempFolder.root + ) + + val boundary = "test-boundary-declined" + val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) + sendRawRequest( + method = "POST", + path = "/upload", + headers = mapOf( + "Relay-Authorization" to "Bearer ${server.token}", + "Content-Type" to "multipart/form-data; boundary=$boundary" + ), + body = body + ) + + assertEquals("clip.mov", uploader.received?.filename) + assertFalse(client.passthroughUploadCalled) + // ...but a declined file must still not reach processFile: handlesFile + // returning false is the processor saying it won't touch a file like this. + assertFalse(processor.processFileCalled) + } + @Test fun `routes upload with a query string and relays the query`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + val processor = ProcessOnlyProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = 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 @@ -192,7 +410,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) @@ -200,13 +418,14 @@ class MediaUploadServerTest { assertEquals("?_embed=wp:featuredmedia", mockUploader.lastQuery) } - // MARK: - Upload with delegate + // MARK: - Upload with processor @Test - fun `calls delegate processFile and uploadFile`() { - val delegate = MockUploadDelegate() + fun `processes with the processor, then delivers through the internal client`() { + val processor = TranscodingProcessor() + val client = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = null, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = client, cacheDir = tempFolder.root) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -222,24 +441,22 @@ 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 processor only transforms; GutenbergKit performs the upload. + assertTrue(client.uploadCalled) // The server relays WordPress's raw response body verbatim. val json = JsonParser.parseString(response.body).asJsonObject - assertEquals(42, json.get("id").asInt) - assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) - assertEquals("image", json.get("media_type").asString) + assertEquals(99, json.get("id").asInt) + assertEquals("https://example.com/doc.pdf", json.get("source_url").asString) + assertEquals("file", json.get("media_type").asString) } @Test - fun `forwards the delegate's processed metadata to the uploader`() { - val delegate = TranscodingDelegate() - val mockUploader = MockDefaultUploader() + fun `forwards the processor's processed metadata to the uploader`() { + val processor = TranscodingProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-meta" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -254,7 +471,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) @@ -262,11 +479,11 @@ class MediaUploadServerTest { } @Test - fun `deletes the delegate's processed file after upload`() { - val delegate = TranscodingDelegate() - val mockUploader = MockDefaultUploader() + fun `deletes the processor's processed file after upload`() { + val processor = TranscodingProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-cleanup" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "movie".toByteArray()) @@ -281,10 +498,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()) } @@ -304,8 +521,8 @@ class MediaUploadServerTest { // one — a flipped comparison would do the opposite and wipe an in-flight upload. server.stop() server = MediaUploadServer( - uploadDelegate = null, - defaultUploader = null, + processor = null, + internalClient = null, cacheDir = tempFolder.root, ioDispatcher = Dispatchers.Unconfined ) @@ -314,15 +531,15 @@ class MediaUploadServerTest { assertTrue("Fresh temp should be preserved", fresh.exists()) } - // MARK: - Fallback to default uploader + // MARK: - Fallback to the internal media client @Test - fun `uses passthrough when delegate does not modify file`() { - val delegate = ProcessOnlyDelegate() - val mockUploader = MockDefaultUploader() + fun `uses passthrough when processor does not modify file`() { + val processor = ProcessOnlyProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-456" val body = buildMultipartBody(boundary, "doc.pdf", "application/pdf", "fake pdf data".toByteArray()) @@ -338,7 +555,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) @@ -348,12 +565,12 @@ class MediaUploadServerTest { } @Test - fun `skips processing and the temp copy when the delegate declines by metadata`() { - val delegate = DeclineByMetadataDelegate() - val mockUploader = MockDefaultUploader() + fun `skips processing and the temp copy when the processor declines by metadata`() { + val processor = DeclineByMetadataProcessor() + val mockUploader = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, defaultUploader = mockUploader, cacheDir = tempFolder.root) + server = MediaUploadServer(processor = processor, internalClient = mockUploader, cacheDir = tempFolder.root) val boundary = "test-boundary-decline" val body = buildMultipartBody(boundary, "clip.mov", "video/quicktime", "fake movie".toByteArray()) @@ -369,17 +586,17 @@ 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) } - // MARK: - DefaultMediaUploader + // MARK: - InternalMediaClient @Test - fun `DefaultMediaUploader relays the WordPress response`() { + fun `InternalMediaClient relays the WordPress response`() { val mockWpServer = MockWebServer() val wpBody = """{"id":1,"source_url":"https://example.com/u.jpg","media_type":"image"}""" @@ -392,7 +609,7 @@ class MediaUploadServerTest { mockWpServer.start() val wpBaseUrl = mockWpServer.url("/wp-json/").toString() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = wpBaseUrl, authHeader = "Bearer test-token" @@ -417,13 +634,13 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader relays a WordPress error response instead of throwing`() { + fun `InternalMediaClient relays a WordPress error response instead of throwing`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(500).setBody("Internal error")) mockWpServer.start() val wpBaseUrl = mockWpServer.url("/wp-json/").toString() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = wpBaseUrl, authHeader = "Bearer test-token" @@ -442,7 +659,7 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader relays the upload attachment ID header`() { + fun `InternalMediaClient relays the upload attachment ID header`() { // WordPress sets this header on an upload whose attachment row was // created before metadata generation fataled. The editor reads it to // retry post-process and clean up the orphan, so it must survive the @@ -457,7 +674,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -475,12 +692,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader deletes an attachment carrying namespace and force query`() { + fun `InternalMediaClient deletes an attachment carrying namespace and force query`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(200).setBody("""{"deleted":true}""")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token", @@ -498,12 +715,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader normalizes an unslashed root and namespace`() { + fun `InternalMediaClient normalizes an unslashed root and namespace`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json").toString(), // no trailing slash authHeader = "Bearer test-token", @@ -535,7 +752,7 @@ class MediaUploadServerTest { ) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -563,12 +780,12 @@ class MediaUploadServerTest { } @Test - fun `DefaultMediaUploader re-encode preserves extra parts and query`() { + fun `InternalMediaClient re-encode preserves extra parts and query`() { val mockWpServer = MockWebServer() mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -603,7 +820,7 @@ class MediaUploadServerTest { mockWpServer.enqueue(MockResponse().setResponseCode(201).setBody("{}")) mockWpServer.start() - val uploader = DefaultMediaUploader( + val uploader = InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = mockWpServer.url("/wp-json/").toString(), authHeader = "Bearer test-token" @@ -756,27 +973,7 @@ class MediaUploadServerTest { // MARK: - Mocks - private class MockUploadDelegate : MediaUploadDelegate { - @Volatile var processFileCalled = false - @Volatile var uploadFileCalled = false - @Volatile var lastMimeType: String? = null - @Volatile var lastFilename: String? = null - - override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { - processFileCalled = 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()) - } - } - - private class ProcessOnlyDelegate : MediaUploadDelegate { + private class ProcessOnlyProcessor : MediaProcessor { @Volatile var processFileCalled = false override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { @@ -786,10 +983,11 @@ class MediaUploadServerTest { } /** - * Declines every file by metadata via [handlesFile], so the server must pass - * through without materializing the file or calling [processFile]. + * Declines every file by metadata via [handlesFile]. With no uploader the server + * must pass through without materializing the file; with one, delivery still + * happens but [processFile] must not be called. [processFileCalled] pins both. */ - private class DeclineByMetadataDelegate : MediaUploadDelegate { + private class DeclineByMetadataProcessor : MediaProcessor { @Volatile var processFileCalled = false override fun handlesFile(mimeType: String, filename: String): Boolean = false @@ -800,9 +998,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 { @@ -814,11 +1012,11 @@ class MediaUploadServerTest { } /** - * A default uploader whose delete response carries its own `Content-Type`, + * An internal media client whose delete response carries its own `Content-Type`, * lowercased, so the relay must override the JSON default rather than emit * the header twice. */ - private class ContentTypeDeleteUploader : DefaultMediaUploader( + private class ContentTypeDeleteClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" @@ -830,7 +1028,22 @@ class MediaUploadServerTest { ) } - private class MockDefaultUploader : DefaultMediaUploader( + /** Records the [MediaUpload] it is handed, and returns a finished attachment. */ + private class RecordingUploader : MediaUploader { + @Volatile var received: MediaUpload? = null + + override suspend fun upload(upload: MediaUpload): ByteArray { + received = upload + // Shaped like a real attachment: the editor's `transformAttachment` + // reads `title.raw`, so an example without it would model a body that + // fails in the editor. + val attachment = """{"id":7,"source_url":"https://example.com/photo.jpg",""" + + """"media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}""" + return attachment.toByteArray() + } + } + + private class MockInternalMediaClient : InternalMediaClient( httpClient = okhttp3.OkHttpClient(), siteApiRoot = "https://example.com/wp-json/", authHeader = "Bearer mock" 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 92% 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..b50477dd2 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaProcessor.kt @@ -5,24 +5,24 @@ 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. + * Demo media processor that resizes images to a maximum dimension of 2000px. * - * Only overrides [processFile] — [uploadFile] returns null so the default uploader is used. + * Only transforms the file; GutenbergKit performs the upload. */ -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 // everything else by metadata — the server then skips copying a file this - // delegate would only pass through. + // processor would only pass through. override fun handlesFile(mimeType: String, filename: String): Boolean { return mimeType.startsWith("image/") && mimeType != "image/gif" } 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 a5218a376..1af91a25b 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/EditorActivity.kt @@ -404,7 +404,7 @@ fun EditorScreen( } }) if (enableNativeMediaUpload) { - mediaUploadDelegate = DemoMediaUploadDelegate() + mediaProcessor = DemoMediaProcessor() } onGutenbergViewCreated(this) } diff --git a/docs/code/physical-device-setup.md b/docs/code/physical-device-setup.md index 98430d3bf..b8ccc3cf1 100644 --- a/docs/code/physical-device-setup.md +++ b/docs/code/physical-device-setup.md @@ -64,7 +64,7 @@ Look for your local network IP address (typically in the format `192.168.x.x` or ### 2. Modify Network Security Configuration -Android requires explicit network security configuration to allow cleartext (http) traffic to non-localhost addresses. +Android requires explicit network security configuration to allow cleartext (http) traffic. The demo app's config already covers `localhost` and the emulator's `10.0.2.2` alias; a development machine reached over the LAN needs its own entry. **Temporarily** modify `android/app/src/main/res/xml/network_security_config.xml` to include your development machine's IP address: @@ -72,6 +72,9 @@ Android requires explicit network security configuration to allow cleartext (htt + + localhost + 127.0.0.1 10.0.2.2 192.168.1.100 diff --git a/docs/code/preloading.md b/docs/code/preloading.md index e00795a56..7912f3365 100644 --- a/docs/code/preloading.md +++ b/docs/code/preloading.md @@ -313,6 +313,28 @@ let dependencies = try await service.prepare { progress in } ``` +#### Sharing Work Between Services + +Every `EditorService` for a site reads and writes the same on-disk caches, so there's no need to hand a service from a +prefetch to the editor — create one for each caller. Don't call `prepare()` on a service while an earlier call on it is +still running: progress is tracked per service, so the later call takes over the progress callback, and whichever +finishes first stops progress for both. + +Services for the same site also share work while it's in flight. A request identical to one already in flight joins it +rather than going out again, and a build of an asset bundle joins the one already running. So an editor opened before a +prefetch finishes fetches only its own post and the `editor-assets` manifest, even when the two are for different posts. +Requests are shared only between clients with the same `URLSession` instance, credentials, and timeout, and never from a +client with a delegate, which expects to see every request it makes. A bundle build is shared by every service for the +site whatever its client, just as the bundle it produces is once it's on disk. + +The request for the post is never shared, even between two editors on the same post: one already in flight can predate +an edit made since. It opts out through its cache policy — a request that asks to skip the cache +(`.reloadIgnoringLocalCacheData` and its siblings) always goes out on its own — and a host's own requests through +`EditorHTTPClient` can do the same. + +Cancelling a caller ends only that caller's wait; shared work stops once no caller is left waiting on it. `purge()` +doesn't stop it, so work that began before a purge can still land after it. + ### EditorViewController Loading Flows `EditorViewController` supports two loading flows based on whether dependencies are provided: @@ -337,7 +359,9 @@ let editor = EditorViewController( ) ``` -The editor displays a progress bar while fetching, then loads once complete. +The editor displays a progress bar while fetching, then loads once complete. The fetch does not hold the +editor: releasing it mid-fetch frees it immediately, and the fetch finishes in the background, warming the +cache for the next editor. ### Best Practice: Prepare Early diff --git a/docs/integration.md b/docs/integration.md index 676c3642a..872360e7f 100644 --- a/docs/integration.md +++ b/docs/integration.md @@ -252,6 +252,131 @@ val configuration = EditorConfiguration.builder() .build() ``` +## Media Handling + +The host can transform media before upload by supplying a `MediaProcessor` at init. +To take over the upload itself, supply a `MediaUploader` instead — a processor only +changes bytes; GutenbergKit still delivers them. + +```swift +let editor = EditorViewController( + configuration: configuration, + mediaProcessor: ResizingProcessor(maxDimension: 2000) +) +``` + +### Don't conform the object that owns the editor + +GutenbergKit never hands your processor the editor: every value crossing that boundary is a +value type — a file URL, a MIME type, a filename. So a processor can only reach the editor +if you put it there. + +That happens when you conform the object that already holds the editor in order to drive +it. The editor holds the processor strongly in return — deliberately, so an in-flight upload +can't lose it mid-request — which closes a retain cycle ARC cannot break. The editor is +never deallocated, and each one strands a bound loopback listener. + +```swift +// Leaks: coordinator -> editor -> mediaProcessor -> coordinator +final class PostEditorCoordinator: MediaProcessor { + var editor: EditorViewController! + init(blog: Blog, configuration: EditorConfiguration) { + editor = EditorViewController(configuration: configuration, mediaProcessor: self) + } +} +``` + +Use a leaf object instead. Nothing is lost: `processFile` is called off the main actor, so +it could not have touched your coordinator's state regardless — whatever it needs is +already separable: + +```swift +final class PostEditorCoordinator { + private let editor: EditorViewController + init(blog: Blog, configuration: EditorConfiguration) { + editor = EditorViewController( + configuration: configuration, + mediaProcessor: BlogMediaProcessor(siteID: blog.dotComID, maxDimension: 2000) + ) + } +} +``` + +If your design genuinely requires the retaining shape, call `stopMediaHandling()` when you +are finished with the editor. It is terminal — the editor cannot upload or delete media +afterwards — so call it when the editor is going away, not when it is merely covered or +backgrounded. + +### Android: permit cleartext to localhost + +**Android hosts must add localhost to their network security configuration, or native +media handling will silently not run.** + +GutenbergKit serves media through a loopback HTTP server, which the editor reaches over +cleartext `http://localhost`. Apps targeting API 28 or above deny cleartext by default, so +without an entry the WebView blocks every upload request with +`ERR_CLEARTEXT_NOT_PERMITTED` before it leaves the page. `GutenbergView` detects this and +leaves the server down, so uploads fall back to the WebView's own path rather than failing +against a server they can never reach. + +The failure is quiet by design — media still uploads — so the symptom is that your +`MediaProcessor` or `MediaUploader` is simply never called. The only signal is a warning +in logcat: + +``` +Cleartext to localhost is not permitted, so the native media upload server can't be +reached from the WebView. Permit cleartext to localhost in the app's network security +config to enable native media processing. +``` + +Add a `domain-config` to the file referenced by your ``'s +`android:networkSecurityConfig`: + +```xml + + + + localhost + 127.0.0.1 + + +``` + +This narrows cleartext to loopback only. It does not permit cleartext anywhere else — the +rest of the app keeps whatever `base-config` (or the platform default) already applies. + +#### Why GutenbergKit can't ship this for you + +`android:networkSecurityConfig` is a single-valued attribute on ``: an app +has exactly one, and the XML files do not merge. A library that declares it collides with +the host's, and the manifest merger fails the build until the app adds +`tools:replace="android:networkSecurityConfig"` — which then discards the library's +version entirely. It also collides with _other_ libraries that declare one; the WordPress +Rust API client already does. And for a host that has no config of its own, a library's +file would silently become the app's entire network security policy, replacing any +certificate pinning or trust anchors it would otherwise have had. + +So the attribute has to be the app's. Only the app can arbitrate between the libraries +that want a say in it. + +#### Devices running Android 16 and above + +API 36 added an implicit cleartext-permitted configuration for localhost, applied when the +app's own config does not already name it. On those devices native media handling works +without the entry above. GutenbergKit supports API 24 and up, so the entry is still +required in practice — and it remains correct on Android 16, where naming localhost +explicitly simply takes precedence over the implicit one. + +### Reusing a processor across editor sessions + +The editor holds the processor for its lifetime and releases it when it goes, so a processor +built for a single editor needs no reference of its own. To use the same instance for +several editors, keep your own reference — the editor drops only its own. Sharing is also +the safer shape: a processor owned by something longer-lived than any editor is a leaf, so +it cannot form the cycle above and there is nothing to tear down. It may be called +concurrently if more than one editor is live, and it must not hold on to any editor it has +served. + ## Common Patterns ### Plugin Support diff --git a/ios/Demo-iOS/Sources/Views/EditorView.swift b/ios/Demo-iOS/Sources/Views/EditorView.swift index 74329f2a1..201b12048 100644 --- a/ios/Demo-iOS/Sources/Views/EditorView.swift +++ b/ios/Demo-iOS/Sources/Views/EditorView.swift @@ -152,11 +152,12 @@ private struct _EditorView: UIViewControllerRepresentable { } func makeUIViewController(context: Context) -> EditorViewController { - let viewController = EditorViewController(configuration: configuration, dependencies: dependencies) + let viewController = EditorViewController( + configuration: configuration, + dependencies: dependencies, + mediaProcessor: enableNativeMediaUpload ? context.coordinator : nil + ) viewController.delegate = context.coordinator - if enableNativeMediaUpload { - viewController.mediaUploadDelegate = context.coordinator - } viewController.webView.isInspectable = true viewModel.perform = { [weak viewController] in @@ -216,7 +217,7 @@ private struct _EditorView: UIViewControllerRepresentable { } @MainActor - class Coordinator: NSObject, EditorViewControllerDelegate, MediaUploadDelegate { + class Coordinator: NSObject, EditorViewControllerDelegate, MediaProcessor { let viewModel: EditorViewModel init(viewModel: EditorViewModel) { @@ -333,11 +334,11 @@ private struct _EditorView: UIViewControllerRepresentable { viewModel.latestContent.map { ($0.title, $0.content) } } - // 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/EditorHTTPClient.swift b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift index e27d66f8a..d5c3c6dbc 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift @@ -94,6 +94,20 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { private let delegate: EditorHTTPClientDelegate? private let requestTimeout: TimeInterval? + /// Requests in flight that an identical `perform(_:)` joins instead of sending again. Every + /// editor and service builds its own client, so this is shared across all of them. + static let inFlightRequests = InFlightTasks() + + /// A request other callers can share: the request as it goes out, and the session it goes + /// out on. `URLRequest`'s own `==` ignores the timeout and the network service type, so + /// those are compared here; it ignores the body too, but a request with one isn't shared. + struct SharedRequest: Hashable, Sendable { + let request: URLRequest + let timeout: TimeInterval + let networkServiceType: URLRequest.NetworkServiceType + let session: ObjectIdentifier + } + public init( urlSession: URLSessionProtocol, authHeader: String, @@ -106,9 +120,63 @@ public actor EditorHTTPClient: EditorHTTPClientProtocol { self.requestTimeout = requestTimeout } + /// Sends `urlRequest`, throwing for a non-2xx status. + /// + /// A request identical to one already in flight joins it rather than going out again, so + /// callers after the same site data — an editor and a prefetch, say — pay for one round + /// trip. Only a safe request without a body is shared, and only between clients no delegate + /// is watching. A request whose cache policy asks to skip the cache goes out alone: its + /// caller wants an answer no older than the call, and a request already in flight may + /// predate a write made since. Cancelling a caller ends its own wait; the request is + /// cancelled once no caller is left waiting on it. public func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { - let configuredRequest = self.configureRequest(urlRequest) + guard let sharedRequest = sharedRequest(forConfigured: configuredRequest) else { + return try await send(configuredRequest) + } + return try await Self.inFlightRequests.value(for: sharedRequest) { _ in + try await self.send(configuredRequest) + } + } + + /// For tests: what `perform(_:)` shares `urlRequest` under, or `nil` if it goes out alone. + func sharedRequest(for urlRequest: URLRequest) -> SharedRequest? { + sharedRequest(forConfigured: configureRequest(urlRequest)) + } + + /// `nil` for a request that must go out alone: an unsafe method or a body, a cache policy + /// that asks for a fresh answer, a delegate that expects to see each request it asked for, + /// or a session that isn't an object — a shared request is keyed by the session's identity, + /// which only an object keeps. + private func sharedRequest(forConfigured request: URLRequest) -> SharedRequest? { + guard delegate == nil, + Self.sharableMethods.contains(request.httpMethod ?? "GET"), + request.httpBody == nil, + request.httpBodyStream == nil, + !Self.freshAnswerPolicies.contains(request.cachePolicy), + type(of: urlSession) is AnyClass + else { + return nil + } + return SharedRequest( + request: request, + timeout: request.timeoutInterval, + networkServiceType: request.networkServiceType, + session: ObjectIdentifier(urlSession as AnyObject) + ) + } + + private static let sharableMethods: Set = ["GET", "HEAD", "OPTIONS"] + + /// The cache policies that ask the server afresh rather than trust a stored response, and + /// so won't take one already on its way. + private static let freshAnswerPolicies: Set = [ + .reloadIgnoringLocalCacheData, + .reloadIgnoringLocalAndRemoteCacheData, + .reloadRevalidatingCacheData, + ] + + private func send(_ configuredRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { let (data, response) = try await self.urlSession.data(for: configuredRequest) self.delegate?.didPerformRequest(configuredRequest, response: response, data: .bytes(data)) diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index d23f02e2c..548c2d853 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -28,14 +28,14 @@ import UIKit // │ WARMUP MODE │ │ DEPENDENCIES │ │ NO DEPENDENCIES │ // │ (isWarmupMode) │ │ PROVIDED │ │ (Async Flow) │ // │ │ │ (Fast Path) │ │ │ -// │ Load HTML without │ │ │ │ Spawn Task to fetch │ +// │ Load HTML without │ │ │ │ Start a loader to fetch │ // │ any dependencies │ │ loadEditor() │ │ dependencies │ // │ for prewarming │ │ immediately │ │ │ // └────────────────────┘ └────────────────────┘ └───────────────────────────────┘ // │ ▼ // │ ┌───────────────────────────────┐ -// │ │ prepareEditor() │ -// │ │ • Load editor dependencies │ +// │ │ EditorDependencyLoader │ +// │ │ • Fetch editor dependencies │ // │ └───────────────────────────────┘ // │ ▼ // │ ┌───────────────────────────────┐ @@ -66,12 +66,16 @@ import UIKit // // ## Flow 2: No Dependencies (Async Flow) // -// When no dependencies are provided, the controller fetches them asynchronously. +// When no dependencies are provided, an `EditorDependencyLoader` fetches them +// asynchronously and hands them to the fast path. The loader holds the controller +// only weakly, so a controller released mid-fetch is freed at once, not when the +// fetch ends. +// // This is a fallback behaviour – the host app should provide the dependencies if it can, // because it'll be a much better user experience. // @MainActor -public final class EditorViewController: UIViewController, GutenbergEditorControllerDelegate, UIAdaptivePresentationControllerDelegate, UIPopoverPresentationControllerDelegate, UISheetPresentationControllerDelegate { +public final class EditorViewController: UIViewController, GutenbergEditorControllerDelegate, EditorDependencyLoaderDelegate, UIAdaptivePresentationControllerDelegate, UIPopoverPresentationControllerDelegate, UISheetPresentationControllerDelegate { public let webView: WKWebView public var configuration: EditorConfiguration @@ -84,7 +88,9 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The fetched or provided editor dependencies (settings, assets, preload data). private var dependencies: EditorDependencies? - private var dependencyTaskHandle: Task? + + /// Fetches `dependencies` when none were provided at init. + private var dependencyLoader: EditorDependencyLoader? /// Error encountered while loading dependencies. private var error: Error? { @@ -112,52 +118,66 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. 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. - 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 - - /// Delegate for customizing media file processing and upload behavior. + /// Transforms media before upload — resize, transcode, strip EXIF. /// - /// 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. + /// To perform the upload yourself, pass a ``mediaUploader`` instead. /// - /// - 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)? { - 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." - ) - } - } + /// Supplied at `init`, with the rest of the editor's configuration, because that is + /// when it takes effect: the processor is captured into the page's initial + /// configuration as the editor begins loading. Taking it there rather than through a + /// settable property leaves no window in which a host can hand one over too late for + /// it to ever run. (Android keeps a settable property and a fail-fast for exactly + /// that case — a `View` is inflated, not constructed by the host, so there is no + /// initializer to put this in.) + /// + /// The rest of this describes a **reference-type** conformer, which is what a host + /// that needs to observe or reuse its processor will write. ``MediaProcessor`` is not + /// class-bound, and a value-type conformer is copied at `init` — see the protocol's + /// documentation for what that changes. + /// + /// The editor holds this strongly for its lifetime, so a processor built for a single + /// editor needs no reference of its own. **To reuse one across editor sessions, keep + /// your own reference to it.** The editor's release — on `deinit`, or on + /// ``stopMediaHandling()`` — drops only *its* reference: a processor the host still + /// holds survives to be passed to the next editor, and one nobody else holds does not. + /// + /// That release is not always prompt, and not always on the main thread. A request in + /// flight holds its own reference until it unwinds, so if this editor is the processor's + /// last owner, the processor is freed when the host's `processFile` returns — on the + /// task's executor, not the caller's thread. Keep a reference of your own if that + /// matters to the conformer. + /// + /// Sharing an instance is the safer shape rather than a compromise. A processor owned + /// by something longer-lived than any editor is a leaf, so the cycle below cannot form + /// and there is nothing to call. Two caveats when you do: it may be called + /// concurrently if more than one editor is live, and it must not hold on to any editor + /// it has served. + /// + /// The one rule: **don't conform the object that owns this editor.** Nothing here + /// hands a processor the editor — every value crossing this boundary is a value type — + /// so the only way one reaches the editor is if you store it there, which is what + /// happens when the coordinator that drives the editor also conforms. Holding this + /// strongly is deliberate — losing the processor mid-request was the failure actually + /// being hit — but it means that shape closes a cycle ARC cannot break, and the editor + /// cannot detect its own teardown to break it for you. If you must write it, call + /// ``stopMediaHandling()`` when you are done with the editor. + public private(set) var mediaProcessor: (any MediaProcessor)? + + /// Takes over media upload on the host's own stack (background session, offline + /// queue, resumable transport). Passing one makes the host own every upload and its + /// whole lifecycle; GutenbergKit stays out of the network entirely for media. + /// + /// Same ownership rules as ``mediaProcessor``: supplied at `init`, held for the + /// editor's lifetime, and not conformed by the object that owns the editor. + /// + /// Reuse is the expected shape here, more so than for a processor: the transports this + /// exists for outlive any one editor by definition — a background `URLSession` has a + /// fixed identifier and must survive app relaunch, an offline queue spans sessions. + /// Build the uploader once, hold it, and pass the same instance to each editor. + /// + /// A ``mediaProcessor`` can still transform the file first; only delivery + /// moves to the uploader. + public private(set) var mediaUploader: (any MediaUploader)? // MARK: - Private Properties (Services) private let editorService: EditorService @@ -166,7 +186,18 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro private let controller: GutenbergEditorController private let bundleProvider: EditorAssetBundleProvider private let lockdownModeMonitor: LockdownModeMonitor - private var uploadServer: MediaUploadServer? + /// Whether the host supplied anything for the native upload server to route. + /// + /// Read twice by `startUploadServer()` — once before starting, once after the bind + /// returns — and the two reads have to agree. They did not: the first gained + /// `mediaUploader` and the second was left checking the processor alone, so an + /// uploader-only host bound a listener, immediately stopped it, and fell back to the + /// WebView path with nothing logged. One property, so they cannot disagree again. + private var hasMediaHandling: Bool { + mediaProcessor != nil || mediaUploader != nil + } + + private(set) var uploadServer: MediaUploadServer? // MARK: - Private Properties (UI) @@ -217,13 +248,42 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro return HTMLPreviewManager(themeStyles: dependencies.editorSettings.themeStyles) }() + /// Creates an editor. + /// + /// - Parameters: + /// - configuration: Site, post, and editor settings to load with. + /// - dependencies: Pre-fetched editor dependencies. Pass them when you have them — + /// the editor fetches its own otherwise, behind a progress bar. + /// - mediaPicker: Supplies media from the host's own picker. + /// - mediaProcessor: Transforms media before upload. **Don't conform the object + /// that owns this editor.** Nothing here hands the processor the editor, so the + /// only way one reaches it is if you store it there — and the editor holds the + /// processor strongly in return, closing a cycle ARC cannot break. Use a leaf + /// object carrying the settings it needs. If you must write the retaining shape, + /// call ``stopMediaHandling()`` when you are done. See ``mediaProcessor`` for the + /// lifetime rules, including what a value-type conformer does differently. + /// - mediaUploader: Takes over media upload on the host's own stack. Same ownership + /// rules as `mediaProcessor`. + /// - httpClient: Replaces the client used for editor and media requests. + /// - isWarmupMode: Loads the editor shell without dependencies, to warm WebKit. public init( configuration: EditorConfiguration, dependencies: EditorDependencies? = nil, mediaPicker: MediaPickerController? = nil, + mediaProcessor: (any MediaProcessor)? = nil, + mediaUploader: (any MediaUploader)? = nil, httpClient: EditorHTTPClient? = nil, isWarmupMode: Bool = false ) { + // A `mediaUploader` needs site credentials for its media deletes. Check it here, + // where the host hands it over, rather than at server start: the stack trace + // names the caller's own line, and the mistake can't hide until the page loads. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: configuration.siteApiRoot, + authHeader: configuration.authHeader, + hasUploader: mediaUploader != nil + ) + let httpClient = httpClient ?? EditorHTTPClient( urlSession: URLSession.shared, authHeader: configuration.authHeader @@ -238,6 +298,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro ) self.bundleProvider = EditorAssetBundleProvider(httpClient: httpClient) self.mediaPicker = mediaPicker + self.mediaProcessor = mediaProcessor + self.mediaUploader = mediaUploader self.lockdownModeMonitor = LockdownModeMonitor() self.controller = GutenbergEditorController(configuration: configuration, lockdownModeMonitor: self.lockdownModeMonitor) @@ -312,26 +374,11 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro if let dependencies { // FAST PATH: Dependencies were provided at init() - load immediately. - // - // Deliberately NOT tracked in `dependencyTaskHandle`: `viewDidDisappear` - // cancels that handle to abort the async dependency *fetch*, but the - // fast path is cheap local work that must run to completion — a - // transient disappearance (e.g. a modal presented over the editor) - // cancelling it mid `startUploadServer()` silently disabled native - // uploads for the session. `[weak self]` still makes it a no-op once - // the controller is torn down. - Task(priority: .userInitiated) { [weak self] in - do { - try await self?.loadEditor(dependencies: dependencies) - } catch { - self?.failToLoad(error) - } - } + startLoadingEditor(dependencies: dependencies) } else { - // ASYNC FLOW: No dependencies - fetch them asynchronously - self.dependencyTaskHandle = Task(priority: .userInitiated) { [weak self] in - await self?.prepareEditor() - } + // ASYNC FLOW: No dependencies - fetch them, then take the fast path. + displayProgressView() + dependencyLoader = EditorDependencyLoader(service: editorService, delegate: self) } } @@ -345,46 +392,133 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro removeNavigationOverlay() } - public override func viewDidDisappear(_ animated: Bool) { - super.viewDidDisappear(animated) - self.dependencyTaskHandle?.cancel() - } - - deinit { - // Stop the upload server when the editor is permanently torn down. + /// Releases the editor's media handling: stops the local upload server, drops the + /// host's ``mediaProcessor`` and ``mediaUploader``, and withdraws the upload + /// endpoint from the page. + /// + /// Most hosts never need this. Releasing the editor runs `deinit`, which does the + /// same work. It is only required when a handler holds the editor back — which + /// happens if you conformed the object that owns it, the one shape ``mediaProcessor`` + /// asks you to avoid — because that cycle keeps `deinit` from ever + /// running, stranding a bound loopback `NWListener` for every editor opened. + /// + /// Terminal, not a pause: this editor cannot upload or delete media afterwards, and + /// any upload in flight is cancelled — though cancellation is cooperative, so a + /// `processFile` that ignores it runs to completion and holds the processor until it + /// returns. Call it when the editor is going away — not + /// when it is covered, backgrounded, or otherwise coming back. Calling it more than + /// once is safe. + /// + /// Scoped to this editor. It drops this editor's reference, so a handler you share + /// across editors keeps working for the others. + public func stopMediaHandling() { + // Host-driven, and the reason is narrower than "UIKit can't tell us". It can. + // + // The editor's own `isBeingDismissed`/`isMovingFromParent` read false — they are + // true on an ancestor, because the editor is a child view controller in every + // real host — but walking to that ancestor works, and WordPress-iOS already ships + // `isBeingDismissedDirectlyOrByAncestor()` for it. Pair it with an orphan check + // (`parent`, `presentingViewController`, `presentedViewController` and + // `viewIfLoaded?.window` all nil) at `viewDidDisappear`, and a probe across + // fourteen hosting shapes fires correctly on every dismissal and pop — including + // this editor's shape in WordPress-iOS — without a single false positive on being + // covered, tab-switched, re-parented by a `UIPageViewController`, or left behind + // by a cancelled interactive pop. Detaching and being covered are distinguishable. // - // This deliberately does NOT happen in `viewDidDisappear`, which also - // fires when another view controller is merely pushed or presented over - // the editor. `HTTPServer.stop()` cancels the `NWListener`, which is - // terminal and has no restart path — stopping on disappear left uploads - // permanently broken once the user returned to the editor. + // What is *not* observable is whether a detachment is permanent. A host may + // re-present or re-attach the same editor instance later, and at the moment of + // the callback that is indistinguishable from the last one. Because this call is + // terminal — the listener cannot restart and the page is told to stop using it — + // guessing wrong permanently disables media in an editor that survived, which is + // strictly worse than the leak it would have prevented. + // + // So this stays the host's call while the action is terminal. Make the endpoint + // recoverable (have the page request the port over the bridge instead of baking + // it in at document start) and the trade reverses. uploadServer?.stop() + uploadServer = nil + mediaProcessor = nil + mediaUploader = nil + revokeNativeUploadEndpoint() } - /// Fetches all required dependencies and then loads the editor. + /// Withdraws the loopback endpoint from the page so media requests fall back to the + /// WebView's default path instead of failing against a port nothing is listening on. /// - /// This method is the entry point for the **Async Flow** (when no dependencies were provided at init). - @MainActor - private func prepareEditor() async { - self.displayProgressView() - defer { self.hideProgressView() } - - do { - // EditorService.prepare() fetches dependencies concurrently with progress reporting - let dependencies = try await self.editorService.prepare { @MainActor [weak self] progress in - self?.progressView.setProgress(progress, animated: true) + /// `nativeMediaUploadMiddleware` re-reads `nativeUploadPort`/`nativeUploadToken` on + /// every request and skips the native path when no port is advertised — but it + /// deliberately does *not* retry a failed native upload directly, on the stated + /// assumption that an advertised port is a reachable one ("cleared on stop"). Until + /// this existed nothing cleared it, so stopping the server left every image insert + /// failing with a connection error on a working connection. + /// + /// Two copies hold the endpoint and both have to go: the live page, and the injected + /// user script, which would otherwise restore the dead port verbatim at the next + /// document start — including the reload that recovers a terminated WebContent + /// process. + private func revokeNativeUploadEndpoint() { + webView.evaluateJavaScript( + """ + if (window.GBKit) { + window.GBKit.nativeUploadPort = null; + window.GBKit.nativeUploadToken = null; } + """ + ) { _, error in + // Logged rather than surfaced: this runs while the editor is going away, so + // there is no one to tell. Silence would be worse than noise — a failure here + // leaves the live page pointed at a port nothing is listening on, which is the + // exact failure this method exists to prevent. + if let error { + Logger.uploadServer.error("Failed to withdraw the native upload endpoint from the page: \(error)") + } + } - // Store dependencies for later use (e.g., HTMLPreviewManager) - self.dependencies = dependencies - - // Continue to the shared loading path - try await self.loadEditor(dependencies: dependencies) + // Rebuilt with `uploadServer` already nil, so the replacement advertises no + // endpoint. The load path is the only other `addUserScript` call site, so removing + // all of them drops exactly the script being replaced. + webView.configuration.userContentController.removeAllUserScripts() + guard let dependencies else { return } + do { + webView.configuration.userContentController.addUserScript( + try buildEditorConfiguration(dependencies: dependencies) + ) } catch { - self.failToLoad(error) + // The load path lets this throw and aborts; here the page is already up, so + // the cost is narrower and lands later: the next document start gets no + // `window.GBKit` at all rather than one with a stale port. + Logger.uploadServer.error("Failed to rebuild the editor configuration after stopping media handling: \(error)") } } + deinit { + // The ordinary path: with no cycle, ARC releases the handlers when the editor + // goes and this stops the server. A host that retains the editor from its own + // handler never reaches here — `stopMediaHandling()` is its way out. + uploadServer?.stop() + } + + // MARK: - Async Flow (EditorDependencyLoaderDelegate) + + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) { + progressView.setProgress(progress, animated: true) + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) { + hideProgressView() + + // Store dependencies for later use (e.g., HTMLPreviewManager) + self.dependencies = dependencies + + // Continue to the shared loading path + startLoadingEditor(dependencies: dependencies) + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) { + hideProgressView() + failToLoad(error) + } + private func failToLoad(_ error: Error) { self.error = error self.delegate?.editor(self, didFailToLoad: error) @@ -392,6 +526,22 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro // MARK: - Shared Loading Path: Load Editor into WebView + /// Runs `loadEditor(dependencies:)` — the step both flows end on. + /// + /// Not cancellable, and it holds the editor until the load returns: cancelling it + /// mid-`startUploadServer()` silently disables native uploads for the session + /// (#357). The hold is short — the server bind is capped by + /// `HTTPServer.defaultStartTimeout`. + private func startLoadingEditor(dependencies: EditorDependencies) { + Task(priority: .userInitiated) { [weak self] in + do { + try await self?.loadEditor(dependencies: dependencies) + } catch { + self?.failToLoad(error) + } + } + } + /// Loads the editor HTML into the WebView with the given dependencies. /// /// This is the **shared loading path** used by both flows after dependencies are available. @@ -401,10 +551,6 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// @MainActor private func loadEditor(dependencies: EditorDependencies) async throws { - // From here on the editor configuration — including `mediaUploadDelegate` — - // is captured, so the delegate setter traps if written after this point. - self.hasStartedLoading = true - self.displayActivityView() // Set asset bundle for the URL scheme handler to serve cached plugin/theme assets @@ -479,27 +625,26 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// The server binds to localhost on a random port. If it fails to start, the editor /// 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. - precondition( - !(mediaUploadDelegateWasAssigned && mediaUploadDelegate == nil), - "mediaUploadDelegate was released before the editor loaded — hold a strong reference to it." - ) - - guard mediaUploadDelegate != nil else { + func startUploadServer() async { + // Nothing to route through the native server unless the host provided a + // processor or an uploader. The editor owns whichever it was given — both + // properties are strong — so there's no released-before-load case to guard + // against; they live as long as it does. + guard hasMediaHandling else { return } - // The native upload server relays through DefaultMediaUploader, which needs a + // The native upload server relays through InternalMediaClient, 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. // - // `MediaServerCredentials` owns the check so it is reachable from the host - // test suite — this file is not. + // Only a `mediaProcessor` can reach this return: a `mediaUploader` without + // usable credentials already trapped in `init`, so by here it has credentials. + // + // `MediaServerCredentials` owns both the predicate and that trap so they are + // reachable from the host test suite — this file is not. guard MediaServerCredentials.areUsable( siteApiRoot: configuration.siteApiRoot, authHeader: configuration.authHeader @@ -507,17 +652,30 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro return } - let defaultUploader = DefaultMediaUploader( + let internalClient = InternalMediaClient( httpClient: httpClient.uploadClient(), siteApiRoot: configuration.siteApiRoot, siteApiNamespace: configuration.siteApiNamespace ) do { - self.uploadServer = try await MediaUploadServer.start( - uploadDelegate: mediaUploadDelegate, - defaultUploader: defaultUploader + let server = try await MediaUploadServer.start( + processor: mediaProcessor, + uploader: mediaUploader, + internalClient: internalClient ) + + // `stopMediaHandling()` can land while the bind is in flight: it is a + // main-actor call and this is suspended. It clears both handlers, so the + // entry guard's condition failing here means media handling was stopped after + // this started, and storing the server would undo a terminal call — the page + // would be handed a port that was just withdrawn, and in the cycle the call + // exists for, `deinit` never runs to stop it. + guard hasMediaHandling else { + server.stop() + return + } + self.uploadServer = server } catch { Logger.uploadServer.error("Failed to start upload server: \(error). Falling back to default upload behavior.") } @@ -525,9 +683,7 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Deletes all cached editor data for all sites public static func deleteAllData() throws { - if FileManager.default.directoryExists(at: Paths.defaultCacheRoot) { - try FileManager.default.removeItem(at: Paths.defaultCacheRoot) - } + try EditorURLCache.deleteAll() if FileManager.default.directoryExists(at: Paths.defaultStorageRoot) { try FileManager.default.removeItem(at: Paths.defaultStorageRoot) diff --git a/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift b/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift new file mode 100644 index 000000000..4d4e13697 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Helpers/InFlightTasks.swift @@ -0,0 +1,166 @@ +import Foundation + +/// Work in flight, one task per key, each shared by every caller asking for that key. +/// +/// A second caller for a key joins the task already running for it rather than starting the +/// same work again. The task belongs to no caller: it runs on its own, so one caller's +/// cancellation can't end it for the others. Cancelling a caller ends that caller's wait at once, +/// and the task is cancelled only when no caller is left waiting on it. A caller that has left +/// hears no further progress, though a progress call already under way when it leaves runs on. +/// A caller that joins at a higher priority than the task's raises the task to match. +final class InFlightTasks: @unchecked Sendable { + + /// Guards `joinable`, and every ``Entry`` and ``Waiter``. + private let lock = NSLock() + + /// The tasks a new caller joins. A task leaves when it finishes, or when its last waiter leaves. + private var joinable: [Key: Entry] = [:] + + /// Returns the value for `key` from the task in flight for it, or from a new one that `run` + /// performs. `progress` hears the task's progress from when this caller joins until it leaves. + func value( + for key: Key, + progress: EditorProgressCallback? = nil, + run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) async throws -> Value { + let waiter = Waiter(progress: progress) + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + join(key, waiter, continuation, run) + } + } onCancel: { + leave(waiter) + } + } + + /// For tests: how many callers are waiting on the task in flight for `key`. + func waiterCount(for key: Key) -> Int { + lock.withLock { joinable[key]?.waiters.count ?? 0 } + } + + /// For tests: the task in flight for `key`. It outlives the wait of a caller that leaves, + /// so a test of what an abandoned task does has to wait for the task itself. + func task(for key: Key) -> Task? { + lock.withLock { joinable[key]?.task } + } + + private func join( + _ key: Key, + _ waiter: Waiter, + _ continuation: CheckedContinuation, + _ run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) { + let (isCancelled, joined) = lock.withLock { () -> (Bool, Task?) in + // Cancelled before getting here, its `onCancel` has run and found nothing to leave. + guard !Task.isCancelled else { return (true, nil) } + let running = joinable[key] + let entry = running ?? start(key, run) + waiter.continuation = continuation + waiter.entry = entry + entry.waiters.append(waiter) + return (false, running?.task) + } + if isCancelled { + continuation.resume(throwing: CancellationError()) + } else if let joined { + raise(joined) + } + } + + /// Raises `task` to the calling task's priority. A task runs at the priority of the caller + /// that started it, and a caller waiting on a continuation doesn't escalate it the way one + /// awaiting `task.value` would — so an editor joining a background prefetch would otherwise + /// wait at the prefetch's priority. Escalation needs iOS 26 or macOS 26; before that, the + /// task keeps the priority it started with. + private func raise(_ task: Task) { + if #available(iOS 26, macOS 26, *) { + task.escalatePriority(to: Task.currentPriority) + } + } + + /// Starts the task for `key`. Called with `lock` held. + private func start( + _ key: Key, + _ run: @escaping @Sendable (_ report: @escaping EditorProgressCallback) async throws -> Value + ) -> Entry { + let entry = Entry(key: key) + joinable[key] = entry + entry.task = Task { + let result: Result + do { + result = .success(try await run { progress in await self.report(progress, from: entry) }) + } catch { + result = .failure(error) + } + self.finish(entry, with: result) + } + return entry + } + + /// Tells every waiter still waiting, one at a time. Each is checked again just before its + /// turn: an earlier waiter's callback can suspend for as long as it likes, and a later waiter + /// can leave meanwhile — after which its own caller has moved on. + private func report(_ progress: EditorProgress, from entry: Entry) async { + let waiters = lock.withLock { entry.waiters } + for waiter in waiters { + let callback = lock.withLock { entry.waiters.contains { $0 === waiter } ? waiter.progress : nil } + await callback?(progress) + } + } + + private func finish(_ entry: Entry, with result: Result) { + let continuations = lock.withLock { + if joinable[entry.key] === entry { + joinable[entry.key] = nil + } + entry.task = nil + defer { entry.waiters = [] } + return entry.waiters.compactMap(\.continuation) + } + for continuation in continuations { + continuation.resume(with: result) + } + } + + private func leave(_ waiter: Waiter) { + let (continuation, abandoned) = lock.withLock { () -> (CheckedContinuation?, Task?) in + guard let entry = waiter.entry, let index = entry.waiters.firstIndex(where: { $0 === waiter }) else { + return (nil, nil) + } + entry.waiters.remove(at: index) + guard entry.waiters.isEmpty else { + return (waiter.continuation, nil) + } + // No one is left waiting: stop the task, and let the next caller start afresh rather + // than join one on its way out. + if joinable[entry.key] === entry { + joinable[entry.key] = nil + } + return (waiter.continuation, entry.task) + } + continuation?.resume(throwing: CancellationError()) + abandoned?.cancel() + } + + /// One task and the callers waiting on it. Guarded by `lock`. + private final class Entry: @unchecked Sendable { + let key: Key + var task: Task? + var waiters: [Waiter] = [] + + init(key: Key) { + self.key = key + } + } + + /// One ``value(for:progress:run:)`` call. Guarded by `lock`. + private final class Waiter: @unchecked Sendable { + let progress: EditorProgressCallback? + var continuation: CheckedContinuation? + var entry: Entry? + + init(progress: EditorProgressCallback?) { + self.progress = progress + } + } +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift new file mode 100644 index 000000000..9539f1aa9 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaHandlers.swift @@ -0,0 +1,218 @@ +import Foundation + +/// A raw response from the WordPress REST API media endpoint. +/// +/// GutenbergKit relays this to the editor verbatim — it does not interpret the +/// body. The editor therefore receives the exact attachment object (on success) +/// 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. +struct MediaUploadResponse: Sendable { + /// The HTTP status code WordPress returned, or 201 for an upload a + /// ``MediaUploader`` delivered. + let statusCode: Int + + /// The raw response body — a WordPress REST attachment on success, or a + /// WordPress REST error object (`{ "code", "message", "data" }`) on failure. + let body: Data + + /// The response headers to relay to the editor. + /// + /// `x-wp-upload-attachment-id` is the one that carries behavior: WordPress + /// sets it on a failed upload whose attachment row was created before + /// 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. + let headers: [String: String] + + init(statusCode: Int, body: Data, headers: [String: String] = [:]) { + self.statusCode = statusCode + self.body = body + self.headers = headers + } +} + +/// The result of a processor's ``MediaProcessor/processFile(at:mimeType:filename:)``. +public enum ProcessedProxyFile: Sendable { + /// The processor did not modify the file; the original upload is forwarded + /// to WordPress unchanged. + case original + + /// The processor produced a file to upload, along with its MIME type and + /// filename. Both are used verbatim, so a format change (e.g. transcoding + /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and + /// filename for WordPress to store the file correctly. + case processed(URL, mimeType: String, filename: String) +} + +/// Transforms media before GutenbergKit delivers it. +/// +/// 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, it cannot deliver media to the wrong place. Pass one +/// as ``EditorViewController/mediaProcessor`` to resize images, transcode video, +/// strip EXIF, etc. +/// +/// This is the safe, common extension point: most hosts want only this. To perform +/// the upload yourself, conform to ``MediaUploader`` instead. +/// +/// Deliberately **not** class-bound. ``EditorViewController`` holds its processor +/// strongly for its own lifetime, so a conformer that holds the view controller back +/// closes a retain cycle ARC cannot break — neither object is freed, and the editor +/// stops tearing down its upload server. Dropping the class requirement lets you +/// conform with a `struct` capturing only what the transform needs, which is the +/// shape that avoids this; a class bound invited the opposite. Note a +/// value type is not automatic protection — a `struct` that stores the view +/// controller cycles just the same. The rule is simply: do not hold it back. +/// +/// A value-type conformer is **copied** when you hand it to the editor's initializer, +/// and the editor holds that copy for its lifetime. Mutating your own instance +/// afterwards changes nothing the editor will run, and there is no way to swap in a +/// new value — the property is `private(set)`, so a different processor means a +/// different editor. If you need settings the host can change while an editor is open, +/// read them inside `processFile` through a reference the conformer captures. That +/// reference must itself be `Sendable` — an actor, or a class made safe with a lock — +/// because this protocol is `Sendable` and a `struct` conformer's stored properties +/// inherit that requirement. +/// +/// Two requirements a `struct` makes easy to miss, both of which compile silently: +/// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a +/// non-mutating requirement), and its argument labels must match exactly. Either +/// mistake resolves to the no-op default below instead of failing to build, leaving a +/// processor that is never called. +public protocol MediaProcessor: 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 processor returning `false` for a video — so the server forwards + /// the original upload to WordPress without first copying a file the processor + /// won't touch. + /// + /// With a ``MediaUploader`` set this can't decline the upload itself — an + /// uploader delivers every file, so there is no passthrough to fall to — but it + /// still gates `processFile`: a declined file reaches the uploader unprocessed. + /// + /// 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. + func handlesFile(ofType mimeType: String, named filename: String) -> Bool + + /// Process 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 +} + +/// Default implementations. +extension MediaProcessor { + public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { + true + } + + public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} + +/// One of the editor's non-file form fields, as sent with a media upload. +/// +/// A named type rather than a `(name, value)` tuple: tuples are not nominal, so a +/// tuple-typed property would permanently block `Equatable`/`Hashable`/`Codable` +/// synthesis on ``MediaUpload`` — including inside GutenbergKit, and not fixable +/// later without a source break for every host. +public struct MediaUploadField: Sendable, Hashable, Codable { + /// The field name, e.g. `post`. Not unique — a `field[]` array repeats it. + public let name: String + + /// The field's value, decoded as UTF-8. + public let value: String + + public init(name: String, value: String) { + self.name = name + self.value = value + } +} + +/// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to +/// send, its metadata, the editor's non-file form fields, and the request's query. +public struct MediaUpload: Sendable { + /// The file to upload — already processed, if a ``MediaProcessor`` ran. + public let fileURL: URL + + /// The file's MIME type. + public let mimeType: String + + /// The file's name. + public let filename: String + + /// The editor's non-file form fields, in order, each decoded as UTF-8 — most + /// importantly `post`, the parent post's ID, without which the attachment is + /// created unattached. A list, not a dictionary, so repeated field names (e.g. a + /// `field[]` array) survive verbatim. Send each as a form part on your + /// `POST /wp/v2/media`, in the given order. + public let fields: [MediaUploadField] + + /// The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), or + /// empty. Carry it on your request so the editor gets the response it expects. + public let query: String + + public init(fileURL: URL, mimeType: String, filename: String, fields: [MediaUploadField], query: String) { + self.fileURL = fileURL + self.mimeType = mimeType + self.filename = filename + self.fields = fields + self.query = query + } +} + +/// Takes over *performing* a media upload — on the host's own stack: its own +/// networking (say, to log every request), a background session, an offline queue, +/// a resumable transport, its own retry policy. +/// +/// This is a choice of *who executes the requests*, not where they go: an uploader +/// and GutenbergKit's internal media client both target the same configured site. +/// Supplying ``EditorViewController/mediaUploader`` makes the host own that upload +/// end-to-end — the request, its own retries, and its recovery and cleanup — with +/// GutenbergKit out of the network entirely. Because the host does the retries +/// itself, there's no raw response left for the editor to retry behind it. The +/// attachment you return lives on that same configured site, where the editor reads +/// and updates it by ID. +/// +/// Deliberately **not** class-bound, for the same reason as ``MediaProcessor``: the +/// editor holds its uploader strongly, so a conformer that holds the view controller +/// back forms a retain cycle neither object escapes. The operative rule is that one: +/// do not store the ``EditorViewController``. A value type does not enforce it — a +/// `struct` holding the view controller cycles the same way — and it carries the same +/// copy-at-`init` caveat described on ``MediaProcessor``. An uploader that owns a +/// queue, a background session, or a retry counter wants a class; capture it behind a +/// reference either way. +public protocol MediaUploader: Sendable { + /// Upload a (possibly processed) file and return the finished WordPress + /// attachment JSON the editor inserts — the same object a direct + /// `POST /wp/v2/media` returns. Return only once the upload is genuinely done, + /// or `throw` on terminal failure: a returned value is taken as a completed + /// attachment, and there is no GutenbergKit recovery behind you. + /// + /// The ``MediaUpload`` carries the file plus the editor's form fields (e.g. + /// `post`) and query — send them all so the created attachment matches a native + /// upload rather than landing as an unattached orphan. + /// + /// That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side + /// post-processing it returns a 5xx carrying the attachment's ID in + /// `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't + /// re-upload; drive `POST /wp/v2/media//post-process` to completion, the way + /// core recovers its own uploads (up to 5 attempts), then return the finished + /// attachment. That request needs a body of `{"action": "create-image-subsizes"}` + /// — core registers `action` as **required**, so a post-process request without + /// it fails with a 400 every time rather than recovering. + /// + /// Owning the upload means owning cleanup on the server too: if post-process + /// can't be recovered, force-delete the orphan + /// (`DELETE /wp/v2/media/?force=true`) before you `throw`, or it stays on the + /// site — neither GutenbergKit nor the editor cleans up behind you. + func upload(_ upload: MediaUpload) async throws -> Data +} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift index a8ce4ee51..2b2514e0a 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaServerCredentials.swift @@ -1,24 +1,73 @@ import Foundation -/// Whether the editor configuration can reach the configured site for media. +/// Whether the editor configuration can reach the configured site for media, and the +/// fail-fast that enforces it. /// /// Deliberately outside `EditorViewController`. That type is `#if canImport(UIKit)`, /// so on the macOS host it does not exist and nothing in it can be tested — including -/// this check, which already diverged silently between iOS and Android once. Living -/// here, it is reachable from the host test suite. +/// this policy, which is a *crash* policy and already diverged silently between iOS +/// and Android once. Living here, it is reachable from the host test suite, where +/// Swift Testing's exit tests (unavailable on iOS/simulator) can assert the trap +/// itself rather than only the predicate. enum MediaServerCredentials { - /// Whether a ``DefaultMediaUploader`` built from this configuration could actually + /// Whether an ``InternalMediaClient`` built from this configuration could actually /// reach the site. /// - /// Both fields are required. The uploader delivers GutenbergKit's uploads to the + /// Both fields are required. The client delivers GutenbergKit's uploads to the /// configured site, so it needs somewhere to send them and credentials to be /// accepted; with either missing, every media request it makes fails. /// - /// `siteApiRoot` is a `URL` here where Android types it as a `String`, so the - /// equivalent of Android's `isEmpty()` check is "not absolute" — a URL with no - /// scheme or host cannot address the site, and every request built from it fails at - /// the URLSession layer. + /// "Somewhere to send them" means an *absolute* root: a URL with no scheme or host + /// cannot address the site, and every request built from it fails at the URLSession + /// layer. `siteApiRoot` is a `URL` here where Android types it as a `String`, but + /// the rule is the same on both sides — Android spells it `Uri.parse(...)` with the + /// same scheme-and-host test, having previously checked only `isEmpty()` and so + /// accepted roots this rejects. static func areUsable(siteApiRoot: URL, authHeader: String) -> Bool { siteApiRoot.scheme != nil && siteApiRoot.host() != nil && !authHeader.isEmpty } + + /// Traps if the host supplied a ``MediaUploader`` without usable credentials. + /// + /// The behavior forks by intent: + /// + /// - A ``MediaProcessor`` only enhances GutenbergKit-owned uploads. With no + /// credentials there is nothing to deliver through, so nothing to process — the + /// server simply stays down and uploads fall to the default WebView path. That + /// is ``areUsable``'s job, at the point the server would start. + /// + /// - A ``MediaUploader`` means the host is *taking over* uploads, and falling back + /// would drop that whole stack — its queueing, its retries — while media appeared + /// to keep working. Worth failing over rather than logging. + /// + /// What makes it a *trap* rather than a warning is that the configuration is + /// incoherent, not merely unlucky: an uploader's media deletes still relay through + /// the internal media client, so there is no site root and auth header under which + /// this host's uploader could have worked. Contrast the conditions the host's + /// environment imposes at server start — a network policy that blocks the loopback + /// endpoint, a port that won't bind — which log and degrade, because the very same + /// configuration works once the environment allows it. Dropping the uploader is the + /// symptom both share; only this one has a cause the host can fix in the + /// configuration it just handed over. + /// + /// Called from `EditorViewController.init`, not from the server start. The uploader + /// is `private(set)` and assigned only there, so a non-nil uploader at load time was + /// necessarily passed at `init` — checking it then puts the host's own call site in + /// the stack trace, instead of surfacing the mistake later from inside a page-load + /// callback where the trace names only GutenbergKit. This mirrors what moving the + /// handlers into `init` already did for the set-before-load contract: enforce the + /// rule where the host states its intent. + /// + /// (Android enforces this in `GutenbergView.mediaUploader`'s setter — the earliest + /// point available there, since it takes its handlers as mutable properties rather + /// than at construction.) + static func requireCredentialsForUploader(siteApiRoot: URL, authHeader: String, hasUploader: Bool) { + guard hasUploader else { return } + precondition( + areUsable(siteApiRoot: siteApiRoot, authHeader: authHeader), + "A mediaUploader needs site credentials so GutenbergKit can relay the " + + "editor's media deletes to the configured site. Set an absolute " + + "siteApiRoot and the auth header in the editor configuration." + ) + } } diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift deleted file mode 100644 index 73752166b..000000000 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ /dev/null @@ -1,100 +0,0 @@ -import Foundation - -/// A raw response from the WordPress REST API media endpoint. -/// -/// GutenbergKit relays this to the editor verbatim — it does not interpret the -/// body. The editor therefore receives the exact attachment object (on success) -/// 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 - - /// 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 - - /// The response headers to relay to the editor. - /// - /// `x-wp-upload-attachment-id` is the one that carries behavior: WordPress - /// sets it on a failed upload whose attachment row was created before - /// 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] - - public 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:)``. -public enum ProcessedProxyFile: Sendable { - /// The delegate did not modify the file; the original upload is forwarded - /// to WordPress unchanged. - case original - - /// The delegate produced a file to upload, along with its MIME type and - /// filename. Both are used verbatim, so a format change (e.g. transcoding - /// MOV to MP4, or an in-place EXIF strip) must report the resulting type and - /// filename for WordPress to store the file correctly. - case processed(URL, mimeType: String, filename: String) -} - -/// Protocol for customizing media upload behavior. -/// -/// 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 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 - /// `.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). - /// - /// 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? -} - -/// Default implementations. -extension MediaUploadDelegate { - public func handlesFile(ofType mimeType: String, named filename: String) -> Bool { - true - } - - 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 - } -} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 3318aeac8..ef17e707b 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,13 +29,16 @@ 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 that transforms the file before delivery. + /// - uploader: Optional host uploader that performs the upload on its own stack. + /// - internalClient: GutenbergKit's own client for the configured site. Delivers + /// uploads when no host uploader does, and every media delete. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( - uploadDelegate: (any MediaUploadDelegate)? = nil, - defaultUploader: DefaultMediaUploader? = nil, + processor: (any MediaProcessor)? = nil, + uploader: (any MediaUploader)? = nil, + internalClient: InternalMediaClient? = nil, maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize ) async throws -> MediaUploadServer { // Sweep temp files orphaned by a prior crash, off the editor-startup @@ -45,7 +48,7 @@ final class MediaUploadServer: Sendable { cleanOrphanedUploads() } - let context = UploadContext(uploadDelegate: uploadDelegate, defaultUploader: defaultUploader) + let handler = Handler(processor: processor, uploader: uploader, internalClient: internalClient) // A generous ceiling for receiving the upload body. The body read is // primarily bounded by the per-read idle timeout (which reaps a stalled @@ -62,13 +65,74 @@ final class MediaUploadServer: Sendable { bodyReadTimeout: bodyReadTimeout, cors: .permissive, delegate: ServerDelegate(), - handler: { request in - await Self.handleRequest(request, context: context) - } + handler: handler + ) + + let uploadServer = MediaUploadServer(server: server, cleanupTask: cleanupTask) + #if DEBUG + countServerStarted(processor: processor, uploader: uploader) + #endif + return uploadServer + } + +#if DEBUG + // MARK: - Leak Census (DEBUG) + + /// Counts live servers so a host that leaks editors finds out in its own debug build. + /// + /// Every live server is a bound loopback `NWListener`. There is one per editor and the + /// editor stops it on `deinit`, so returning to zero is the normal outcome — monotone + /// growth is the ownership cycle described on + /// ``EditorViewController/stopMediaHandling()``. Nothing else produces it: + /// `EditorViewController.warmup()` passes neither handler, so it never starts a server. + /// + /// This population is the only detectable symptom of that cycle. A `deinit` assertion + /// on the editor cannot work — a cycle is precisely what stops `deinit` from running — + /// and no UIKit callback distinguishes teardown from being covered or re-parented. + /// + /// Logged, never fatal. The threshold is a heuristic, and crashing a host's debug + /// build over a heuristic is a worse trade than the leak it reports. + private static let censusLock = NSLock() + // Guarded by `censusLock` on every access. + nonisolated(unsafe) private static var liveServerCount = 0 + + /// Live servers tolerated before the count reads as a leak. Two editors can briefly + /// overlap across a push or a modal transition; four is not a shape hosts produce. + private static let liveServerLeakThreshold = 4 + + private static func countServerStarted( + processor: (any MediaProcessor)?, + uploader: (any MediaUploader)? + ) { + let count = censusLock.withLock { + liveServerCount += 1 + return liveServerCount + } + + guard count >= liveServerLeakThreshold else { return } + + // Name every handler that was supplied, not just the first. With both set the + // retainer is as likely to be the uploader, and naming only the processor sends + // the reader to audit an object that may be a value type holding nothing at all. + let names = [processor.map { String(describing: type(of: $0)) }, + uploader.map { String(describing: type(of: $0)) }].compactMap { $0 } + let name = names.isEmpty ? "the host's media handler" : names.joined(separator: ", ") + Logger.uploadServer.fault( + """ + \(count, privacy: .public) media upload servers are live, one bound loopback \ + listener each. Editors are leaking: a host that both owns EditorViewController \ + and is one of its own media handlers (\(name, privacy: .public)) forms a retain \ + cycle ARC cannot break, so the editor's deinit never runs. Call \ + EditorViewController.stopMediaHandling() when you are done with the editor, or \ + keep the handler a leaf object that doesn't reference the editor. + """ ) + } - return MediaUploadServer(server: server, cleanupTask: cleanupTask) + deinit { + Self.censusLock.withLock { Self.liveServerCount -= 1 } } +#endif private init(server: HTTPServer, cleanupTask: Task) { self.server = server @@ -84,249 +148,342 @@ final class MediaUploadServer: Sendable { // MARK: - Request Handling - private static func handleRequest(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { - let parsed = request.parsed + /// Serves the upload server's requests. + /// + /// A `struct` rather than a closure over a context object: the dependencies become + /// stored properties and the request logic becomes instance methods, instead of + /// statics threading a context parameter through every call. It stores no reference + /// back to the `MediaUploadServer`, so it can't close the + /// `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer` loop that + /// would keep `deinit` — and therefore `stop()` — from ever running. Being a value + /// type is not what buys that: a `struct` storing the server would close the loop + /// just the same, which is why the statics above stay static. + /// + /// Everything here is held **strongly**, so a processor that admitted a file for + /// processing will process it, and an upload gated on a host uploader will be + /// delivered by it — the reads within a request can't disagree, and an in-flight + /// upload keeps the host's handlers alive until it unwinds. This matches Android, + /// which holds its `processor`/`uploader` as plain `val`s for the same reason. + /// + /// Strong is safe because `EditorViewController` owns `mediaProcessor` and + /// `mediaUploader` strongly too. A host object that retains the view controller + /// back already forms `EditorViewController -> mediaUploader -> + /// EditorViewController`, a cycle this handler can neither create nor prevent. + /// + /// Implicitly `Sendable`: `MediaProcessor` and `MediaUploader` are `Sendable` + /// protocols and `InternalMediaClient` is `@unchecked Sendable`. + private struct Handler: HTTPRequestHandler { + let processor: (any MediaProcessor)? + let uploader: (any MediaUploader)? + let internalClient: InternalMediaClient? + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + let parsed = request.parsed + + // Routes: POST /upload, and DELETE /media/ for the editor's orphan + // cleanup. (OPTIONS preflight is answered by the HTTP library under its + // permissive CORS policy.) Match on the path alone — the target carries + // a query string (e.g. `?_embed`, `?force=true`) relayed to WordPress. + let method = parsed.method.uppercased() + + if method == "POST", parsed.path == "/upload" { + return await handleUpload(request) + } - // Routes: POST /upload, and DELETE /media/ for the editor's orphan - // cleanup. (OPTIONS preflight is answered by the HTTP library under its - // permissive CORS policy.) Match on the path alone — the target carries - // a query string (e.g. `?_embed`, `?force=true`) relayed to WordPress. - let method = parsed.method.uppercased() + if method == "DELETE", let attachmentId = Self.attachmentId(fromPath: parsed.path) { + return await handleDelete(attachmentId, query: parsed.query) + } - if method == "POST", parsed.path == "/upload" { - return await handleUpload(request, context: context) + return MediaUploadServer.errorResponse(status: 404, message: "Not found") } - if method == "DELETE", let attachmentId = attachmentId(fromPath: parsed.path) { - return await handleDelete(attachmentId, query: parsed.query, context: context) - } + private func handleUpload(_ request: HTTPServer.Request) async -> HTTPResponse { + let parts: [MultipartPart] + do { + parts = try request.parsed.multipartParts() + } catch { + Logger.uploadServer.error("Multipart parse failed: \(error)") + return MediaUploadServer.errorResponse(status: 400, message: "Expected multipart/form-data") + } - return errorResponse(status: 404, message: "Not found") - } + // Find the file part (the first part with a filename). + guard let filePart = parts.first(where: { $0.filename != nil }) else { + return MediaUploadServer.errorResponse(status: 400, message: "No file found in request") + } - private static func handleUpload(_ request: HTTPServer.Request, context: UploadContext) async -> HTTPResponse { - let parts: [MultipartPart] - do { - parts = try request.parsed.multipartParts() - } catch { - Logger.uploadServer.error("Multipart parse failed: \(error)") - return errorResponse(status: 400, message: "Expected multipart/form-data") - } + // The non-file parts (post, additionalData) and the original query + // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. + let extraParts = parts.filter { $0.filename == nil } + let query = request.parsed.query + + let filename = filePart.filename ?? "upload" + let mimeType = filePart.contentType + + // Ask the processor — 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 processor won't process (e.g. a video handed + // to an image-only processor). + // + // An uploader takes over delivery for *every* file, so with one set there is + // no passthrough to fall to and the gate can't decline the upload outright. + // It still decides whether `processFile` runs, though — a declined file is + // handed to the uploader unprocessed rather than to a processor that said it + // won't touch it — so the answer is carried into `processAndUpload` rather + // than discarded here. Asked exactly once per upload, matching Android. + let processorWantsFile = processor?.handlesFile(ofType: mimeType, named: filename) ?? false + guard uploader != nil || processorWantsFile else { + do { + return try await passthroughResponse(request, query: query) + } catch { + return Self.uploadErrorResponse(error) + } + } - // Find the file part (the first part with a filename). - guard let filePart = parts.first(where: { $0.filename != nil }) else { - return errorResponse(status: 400, message: "No file found in request") - } + // Someone wants the file — the processor, the uploader, or both. Stream the + // part body to a dedicated temp file for them: the library's RequestBody may + // be a byte-range slice of a larger temp file whose lifecycle is tied to ARC, + // so they need a standalone file that outlives the handler return. + let tempDir = MediaUploadServer.uploadsTempDirectory + try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - // The non-file parts (post, additionalData) and the original query - // (e.g. ?_embed) must reach WordPress too — relay them alongside the file. - let extraParts = parts.filter { $0.filename == nil } - let query = request.parsed.query + let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(MediaUploadServer.sanitizeFilename(filename))") + do { + let inputStream = try filePart.body.makeInputStream() + try MediaUploadServer.writeStream(inputStream, to: fileURL) + } catch { + try? FileManager.default.removeItem(at: fileURL) + Logger.uploadServer.error("Failed to write upload to disk: \(error)") + return MediaUploadServer.errorResponse(status: 500, message: "Failed to save file") + } - let filename = filePart.filename ?? "upload" - let mimeType = filePart.contentType + // From here on always clean up the original temp file. The processed + // file (if the processor produced a new one) is cleaned up inside + // processAndUpload so its throw paths are covered too. + defer { try? FileManager.default.removeItem(at: fileURL) } - // 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 { do { - return try await passthroughResponse(request, query: query, context: context) + let uploadResult = try await processAndUpload( + fileURL: fileURL, mimeType: mimeType, filename: filename, + extraParts: extraParts, query: query, + processorWantsFile: processorWantsFile + ) + switch uploadResult { + case .uploaded(let uploaded): + Logger.uploadServer.debug("Uploaded file to WordPress") + return Self.relayResponse(uploaded) + case .passthrough: + // The processor didn't modify the file — forward the original request + // body to WordPress without re-encoding. + return try await passthroughResponse(request, query: query) + } } catch { - return uploadErrorResponse(error) + return Self.uploadErrorResponse(error) } } - // The delegate wants the file. Stream the part body to a dedicated temp - // file for it — the library's RequestBody may be a byte-range slice of a - // larger temp file whose lifecycle is tied to ARC, so the delegate needs a - // standalone file that outlives the handler return. - let tempDir = uploadsTempDirectory - try? FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true) - - let fileURL = tempDir.appending(component: "\(UUID().uuidString)-\(sanitizeFilename(filename))") - do { - let inputStream = try filePart.body.makeInputStream() - try writeStream(inputStream, to: fileURL) - } catch { - try? FileManager.default.removeItem(at: fileURL) - Logger.uploadServer.error("Failed to write upload to disk: \(error)") - return errorResponse(status: 500, message: "Failed to save file") + /// Forwards the original request body to WordPress unchanged (no multipart + /// re-encoding) and relays the response. Used when the processor won't touch + /// the file — it declined by metadata (`handlesFile` returned false) or + /// `processFile` returned `.original`. + private func passthroughResponse( + _ request: HTTPServer.Request, query: String + ) async throws -> HTTPResponse { + // As in `processAndUpload`: don't put bytes on the wire for a torn-down + // editor, regardless of whether the HTTP client honors cancellation. + try Task.checkCancellation() + + Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") + guard let body = request.parsed.body, + let contentType = request.parsed.header("Content-Type"), + let internalClient else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } + let response = try await internalClient.passthroughUpload(body: body, contentType: contentType, query: query) + return Self.relayResponse(response) } - // From here on always clean up the original temp file. The processed - // file (if the delegate produced a new one) is cleaned up inside - // processAndUpload so its throw paths are covered too. - defer { try? FileManager.default.removeItem(at: fileURL) } + /// The attachment ID in a `/media/` path, or `nil` if the path is not one. + /// + /// Deliberately narrow: this server relays media operations, not arbitrary + /// REST requests, so only a numeric attachment ID under `/media/` matches. + private static func attachmentId(fromPath path: String) -> String? { + let components = path.split(separator: "/", omittingEmptySubsequences: true) + guard components.count == 2, components[0] == "media" else { return nil } + let id = String(components[1]) + guard !id.isEmpty, id.allSatisfy(\.isNumber) else { return nil } + return id + } - do { - let uploadResult = try await processAndUpload( - fileURL: fileURL, mimeType: mimeType, filename: filename, - extraParts: extraParts, query: query, context: context - ) - switch uploadResult { - case .uploaded(let uploaded): - Logger.uploadServer.debug("Uploaded file to WordPress") - return relayResponse(uploaded) - case .passthrough: - // Delegate didn't modify the file — forward the original request - // body to WordPress without re-encoding. - return try await passthroughResponse(request, query: query, context: context) + /// Relays the editor's orphan cleanup to WordPress. + /// + /// 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. + private func handleDelete( + _ attachmentId: String, query: String + ) async -> HTTPResponse { + guard let internalClient else { + return MediaUploadServer.errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + } + do { + let response = try await internalClient.deleteMedia(attachmentId: attachmentId, query: query) + return Self.relayResponse(response) + } catch { + return Self.uploadErrorResponse(error) } - } catch { - return uploadErrorResponse(error) } - } - /// 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`. - private static func passthroughResponse( - _ request: HTTPServer.Request, query: String, context: UploadContext - ) async throws -> HTTPResponse { - Logger.uploadServer.debug("Passthrough: forwarding original request body to WordPress") - guard let body = request.parsed.body, - let contentType = request.parsed.header("Content-Type"), - let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) + /// Relays WordPress's exact status, body, and relayable headers to the editor + /// so it sees the same attachment object (or error) as a direct upload. + /// + /// The headers matter for recovery: `x-wp-upload-attachment-id` is what lets + /// the editor retry `post-process` for an upload whose metadata generation + /// fataled server-side, rather than surfacing a permanent failure and + /// leaving an orphaned attachment behind. + /// + /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` + /// serializes every header it is given, so appending the default unconditionally + /// would emit the name twice for a processor that sets it. + private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { + let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } + return HTTPResponse( + status: response.statusCode, + headers: (hasContentType ? [] : [("Content-Type", "application/json")]) + + response.headers.map { ($0.key, $0.value) }, + body: response.body + ) } - let response = try await defaultUploader.passthroughUpload(body: body, contentType: contentType, query: query) - return relayResponse(response) - } - - /// The attachment ID in a `/media/` path, or `nil` if the path is not one. - /// - /// Deliberately narrow: this server relays media operations, not arbitrary - /// REST requests, so only a numeric attachment ID under `/media/` matches. - private static func attachmentId(fromPath path: String) -> String? { - let components = path.split(separator: "/", omittingEmptySubsequences: true) - guard components.count == 2, components[0] == "media" else { return nil } - let id = String(components[1]) - guard !id.isEmpty, id.allSatisfy(\.isNumber) else { return nil } - return id - } - /// Relays the editor's orphan cleanup to WordPress. - /// - /// 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. - private static func handleDelete( - _ attachmentId: String, query: String, context: UploadContext - ) async -> HTTPResponse { - guard let defaultUploader = context.defaultUploader else { - return errorResponse(status: 500, message: UploadError.noUploader.localizedDescription) - } - do { - let response = try await defaultUploader.deleteMedia(attachmentId: attachmentId, query: query) - return relayResponse(response) - } catch { - return uploadErrorResponse(error) + /// Builds the 500 response for a failed upload. A cancelled connection task + /// (editor abort / server stop) surfaces here too — as CancellationError or + /// URLError.cancelled — but isn't a failure and the server closes the + /// connection without sending this response (see HTTPServer's cancellation + /// check), so log that quietly. + private static func uploadErrorResponse(_ error: any Error) -> HTTPResponse { + if Task.isCancelled { + Logger.uploadServer.debug("Upload cancelled") + } else { + Logger.uploadServer.error("Upload processing failed: \(error)") + } + return MediaUploadServer.errorResponse(status: 500, message: error.localizedDescription) } - } - /// Relays WordPress's exact status, body, and relayable headers to the editor - /// so it sees the same attachment object (or error) as a direct upload. - /// - /// The headers matter for recovery: `x-wp-upload-attachment-id` is what lets - /// the editor retry `post-process` for an upload whose metadata generation - /// fataled server-side, rather than surfacing a permanent failure and - /// leaving an orphaned attachment behind. - /// - /// The response's own `Content-Type` wins over the JSON default. `HTTPResponse` - /// serializes every header it is given, so appending the default unconditionally - /// would emit the name twice for a delegate that sets it. - private static func relayResponse(_ response: MediaUploadResponse) -> HTTPResponse { - let hasContentType = response.headers.keys.contains { $0.lowercased() == "content-type" } - return HTTPResponse( - status: response.statusCode, - headers: (hasContentType ? [] : [("Content-Type", "application/json")]) - + response.headers.map { ($0.key, $0.value) }, - body: response.body - ) - } + // MARK: - Processor Pipeline - /// Builds the 500 response for a failed upload. A cancelled connection task - /// (editor abort / server stop) surfaces here too — as CancellationError or - /// URLError.cancelled — but isn't a failure and the server closes the - /// connection without sending this response (see HTTPServer's cancellation - /// check), so log that quietly. - private static func uploadErrorResponse(_ error: any Error) -> HTTPResponse { - if Task.isCancelled { - Logger.uploadServer.debug("Upload cancelled") - } else { - Logger.uploadServer.error("Upload processing failed: \(error)") + /// Result of the processing + upload pipeline. + private enum UploadResult { + /// The uploader or internal media client completed the upload; + /// carries the raw WordPress response to relay. + case uploaded(MediaUploadResponse) + /// The processor didn't modify the file, so the original body is forwarded. + /// The caller should forward the original request body to WordPress. + case passthrough } - return errorResponse(status: 500, message: error.localizedDescription) - } - // MARK: - Delegate Pipeline + private func processAndUpload( + fileURL: URL, mimeType: String, filename: String, + extraParts: [MultipartPart], query: String, processorWantsFile: Bool + ) async throws -> UploadResult { + // Step 1: Process (resize, transcode, etc.) — but only for a file the + // processor's metadata gate accepted. `handlesFile` returning false is the + // processor saying it won't touch a file like this, so handing it one anyway + // would break the contract the gate documents. With an uploader set the file + // still gets delivered; it just skips processing on its way there. + let processed: ProcessedProxyFile + if let processor, processorWantsFile { + processed = try await processor.processFile(at: fileURL, mimeType: mimeType, filename: filename) + } else { + processed = .original + } - /// Result of the delegate processing + upload pipeline. - private enum UploadResult { - /// The delegate (or default uploader) completed the upload; carries the - /// raw WordPress response to relay. - case uploaded(MediaUploadResponse) - /// The delegate didn't modify the file and `uploadFile` returned nil. - /// The caller should forward the original request body to WordPress. - case passthrough - } + // Resolve the file to upload and its metadata. `.processed` uses the + // processor's values verbatim, so a format change is reported to WordPress. + let uploadURL: URL + let uploadMimeType: String + let uploadFilename: String + switch processed { + case .original: + uploadURL = fileURL + uploadMimeType = mimeType + uploadFilename = filename + case let .processed(url, processedMimeType, processedFilename): + uploadURL = url + uploadMimeType = processedMimeType + uploadFilename = processedFilename + } - private static func processAndUpload( - fileURL: URL, mimeType: String, filename: String, - extraParts: [MultipartPart], query: String, context: UploadContext - ) async throws -> UploadResult { - // Step 1: Process (resize, transcode, etc.) - let processed: ProcessedProxyFile - if let delegate = context.uploadDelegate { - processed = try await delegate.processFile(at: fileURL, mimeType: mimeType, filename: filename) - } else { - processed = .original - } + // 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 { + if uploadURL != fileURL { + try? FileManager.default.removeItem(at: uploadURL) + } + } - // Resolve the file to upload and its metadata. `.processed` uses the - // delegate's values verbatim, so a format change is reported to WordPress. - let uploadURL: URL - let uploadMimeType: String - let uploadFilename: String - switch processed { - case .original: - uploadURL = fileURL - uploadMimeType = mimeType - uploadFilename = filename - case let .processed(url, processedMimeType, processedFilename): - uploadURL = url - uploadMimeType = processedMimeType - uploadFilename = processedFilename - } + // The editor was torn down (or the client disconnected) while we processed. + // Don't start an outbound upload whose response nobody will read — it would + // create an attachment neither GutenbergKit nor the host knows to clean up. + // Checking here rather than relying on the HTTP client to notice cancellation + // keeps this true for a host-injected `URLSessionProtocol` that doesn't. + try Task.checkCancellation() + + // Step 2: deliver. An uploader owns delivery on the host's own stack and + // returns the finished attachment JSON (or throws); GutenbergKit relays that + // as a success and never runs its own recovery behind it. + if let uploader { + let upload = MediaUpload( + fileURL: uploadURL, + mimeType: uploadMimeType, + filename: uploadFilename, + fields: try await Self.formFields(from: extraParts), + query: query + ) + let attachment = try await uploader.upload(upload) + return .uploaded(MediaUploadResponse(statusCode: 201, body: attachment)) + } - // The processed file (if the delegate 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 { - if uploadURL != fileURL { - try? FileManager.default.removeItem(at: uploadURL) + if let internalClient { + // Unmodified — forward the original request body directly, skipping + // multipart re-encoding. + if case .original = processed { + return .passthrough + } + let result = try await internalClient.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) + return .uploaded(result) + } else { + throw UploadError.noUploader } } - // 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) - } else if let defaultUploader = context.defaultUploader { - // Unmodified — forward the original request body directly, skipping - // multipart re-encoding. - if case .original = processed { - return .passthrough + /// The editor's non-file form parts as ordered, UTF-8-decoded fields. + /// + /// A list rather than a dictionary so repeated names (e.g. a `field[]` array) + /// survive verbatim, in the order the editor sent them. + /// + /// This decode can't mangle anything, but only because of who is on the other end — + /// nothing in the code enforces it. Three things have to stay true: + /// + /// 1. Only the editor's own web page can reach this server. It listens on loopback, + /// and every request has to carry a per-session token. + /// 2. Text the editor puts in a form field is already valid Unicode. The browser + /// guarantees that when the value is set, so it cannot hand us bad bytes. + /// 3. The only way a browser can put *raw* bytes in a form is a file or a Blob, and + /// those always arrive with a filename. Anything with a filename is handled as + /// the file, never as a field — so raw bytes never reach this decode. + /// + /// If one of those stops being true, bad bytes quietly turn into replacement + /// characters, and the platforms don't even agree on how many: `ED A0 80` becomes + /// three of them here and one on Android. There is no single behavior worth + /// documenting, so the tests pin rule 3 instead. + private static func formFields(from parts: [MultipartPart]) async throws -> [MediaUploadField] { + var fields: [MediaUploadField] = [] + for part in parts { + fields.append(MediaUploadField(name: part.name, value: String(decoding: try await part.body.data, as: UTF8.self))) } - let result = try await defaultUploader.upload(fileURL: uploadURL, mimeType: uploadMimeType, filename: uploadFilename, extraParts: extraParts, query: query) - return .uploaded(result) - } else { - throw UploadError.noUploader + return fields } } @@ -443,41 +600,23 @@ enum UploadError: Error, LocalizedError { var errorDescription: String? { switch self { - case .noUploader: "No upload delegate or default uploader configured" + case .noUploader: "No media uploader or internal media client configured" case .streamReadFailed: "Failed to read upload stream" case .streamWriteFailed: "Failed to write upload to disk" } } } -// MARK: - Upload Context +// MARK: - Internal Media Client -/// Container for the upload delegate 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. +/// GutenbergKit's own client for the configured site, built from the site credentials +/// in `EditorConfiguration`. /// -/// `@unchecked Sendable`: `uploadDelegate` is 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)? - let defaultUploader: DefaultMediaUploader? - - init(uploadDelegate: (any MediaUploadDelegate)?, defaultUploader: DefaultMediaUploader?) { - self.uploadDelegate = uploadDelegate - self.defaultUploader = defaultUploader - } -} - -// MARK: - Default Media Uploader - -/// Uploads files to the WordPress REST API using site credentials from EditorConfiguration. -class DefaultMediaUploader: @unchecked Sendable { +/// Not an implementation of any host-facing protocol — it is the thing that actually +/// performs GutenbergKit's media requests. It delivers uploads the host did not take +/// over, and relays the editor's media deletes: every attachment lives on the +/// configured site, so that is where its deletion goes. +class InternalMediaClient: @unchecked Sendable { private let httpClient: EditorHTTPClientProtocol private let siteApiRoot: URL private let siteApiNamespace: String? @@ -528,7 +667,7 @@ class DefaultMediaUploader: @unchecked Sendable { /// Forwards the original request body to WordPress without re-encoding. /// - /// Used when the delegate's `processFile` returned the file unchanged — + /// Used when the processor's `processFile` returned the file unchanged — /// the incoming multipart body is already valid for WordPress. func passthroughUpload(body: RequestBody, contentType: String, query: String) async throws -> MediaUploadResponse { var request = URLRequest(url: mediaEndpointURL(query: query)) @@ -627,11 +766,12 @@ class DefaultMediaUploader: @unchecked Sendable { mimeType: String, extraFields: [(name: String, value: Data)] ) throws -> (InputStream, Int) { - // Serialize the non-file parts (post, additionalData) into the preamble - // ahead of the streamed file. They are small, so keeping them in memory is - // fine; `contentLength` counts them via `preamble.count`. Field values are - // appended as raw bytes (not through String) so a non-UTF-8 value is - // forwarded verbatim rather than coerced to empty. + // The non-file parts (post, additionalData) go into the preamble ahead of the streamed + // file; they're small, and `contentLength` counts them via `preamble.count`. Their + // values are appended as raw bytes rather than through `String(data:encoding:)`, which + // returns nil on bad UTF-8 — and the `?? ""` you'd reach for behind it would quietly + // drop a whole field. Raw bytes also keep this re-encode byte-for-byte identical to + // the plain passthrough it replaces. (Bad bytes can't get here; see `formFields`.) var preamble = Data() for field in extraFields { preamble.append(Data("--\(boundary)\r\n".utf8)) diff --git a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift index db4927c61..43ae68b5d 100644 --- a/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift +++ b/ios/Sources/GutenbergKit/Sources/RESTAPIRepository.swift @@ -66,7 +66,10 @@ public struct RESTAPIRepository: Sendable { // MARK: Post @discardableResult public func fetchPost(id: Int) async throws -> EditorURLResponse { - let request = URLRequest(method: .GET, url: self.buildPostUrl(id: id)) + var request = URLRequest(method: .GET, url: self.buildPostUrl(id: id)) + // The post is never cached, and for the same reason never joins a request in flight: + // one started by an editor since closed can predate an edit made in between. + request.cachePolicy = .reloadIgnoringLocalCacheData let response = try await self.httpClient.perform(request) return EditorURLResponse(response) } diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift new file mode 100644 index 000000000..b73e4483e --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorDependencyLoader.swift @@ -0,0 +1,50 @@ +import Foundation + +/// Fetches an editor's dependencies without holding the editor. +/// +/// The editor owns its loader, never the reverse: the loader reaches back only through +/// ``delegate``, which is weak and whose requirements are all synchronous. So a released +/// editor is freed at once rather than when the fetch ends — it never awaits the fetch, +/// and nothing the loader calls on it can suspend. Keep it that way by reading +/// `delegate` where it is used: a copy held across the `await` would retain the editor +/// for the whole fetch. +/// +/// The fetch starts on init and is never cancelled, so it keeps the loader alive until it +/// finishes. That is harmless — a loader owns no view, web view, or listener — and a +/// fetch that outlives its editor still warms the cache for the next one. +@MainActor +final class EditorDependencyLoader { + weak let delegate: (any EditorDependencyLoaderDelegate)? + + init(service: EditorService, delegate: any EditorDependencyLoaderDelegate) { + self.delegate = delegate + fetch(from: service) + } + + /// Kept out of `init`, where the strong `delegate` parameter would shadow the weak + /// property — so here the task can reach the delegate only through ``delegate``. + private func fetch(from service: EditorService) { + Task(priority: .userInitiated) { + do { + let dependencies = try await service.prepare { @MainActor progress in + self.delegate?.dependencyLoader(self, didUpdate: progress) + } + delegate?.dependencyLoader(self, didLoad: dependencies) + } catch { + delegate?.dependencyLoader(self, didFailWith: error) + } + } + } +} + +/// Receives an ``EditorDependencyLoader``'s results on the main actor. +/// +/// Every requirement is synchronous, so no call can suspend while holding the delegate. +/// An `async` requirement could, and would keep the editor alive until the call resumed — +/// for the whole fetch, if the call awaited it. +@MainActor +protocol EditorDependencyLoaderDelegate: AnyObject { + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) +} diff --git a/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift b/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift index d870c197a..0aee8e0fa 100644 --- a/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift +++ b/ios/Sources/GutenbergKit/Sources/Services/EditorService.swift @@ -180,13 +180,14 @@ public actor EditorService { } private func incrementProgress(for weight: DependencyWeights, fraction: Double = 1.0) async { - precondition( - self.progress != nil, - "Progress has not been initialized. This is a bug in the EditorService. Please file an issue." - ) + // Progress can arrive after the `prepare()` it belongs to has returned and cleared it. A + // bundle build shared with another service may already be calling in when this service + // gives up on it, and an overlapping `prepare()` on this service is cleared by whichever + // finishes first. There is nothing left to report to, so drop it. + guard let current = self.progress else { return } let progress = EditorProgress( - completed: self.progress!.completed + Int(weight.rawValue * fraction), - total: self.progress!.total) + completed: current.completed + Int(weight.rawValue * fraction), + total: current.total) self.progress = progress await self.progressCallback?(progress) } diff --git a/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift b/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift index 0c720891c..dae3364dd 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/EditorAssetLibrary.swift @@ -9,6 +9,10 @@ public actor EditorAssetLibrary { private let storageRoot: URL private let cachePolicy: EditorCachePolicy + /// Bundle builds in flight, keyed by the directory each writes. Every service builds its own + /// library, so this is shared across all of them. + static let inFlightBuilds = InFlightTasks() + /// Creates a new `EditorAssetLibrary` instance. /// /// - Parameters: @@ -118,6 +122,18 @@ public actor EditorAssetLibrary { return .empty } + // Every build of one manifest writes the same directory, whichever library runs it: + // join a build in flight rather than race a second one into it. + let destination = self.bundleRoot(for: manifest.checksum).standardizedFileURL + return try await Self.inFlightBuilds.value(for: destination, progress: progress) { report in + try await self.build(manifest, reportingTo: report) + } + } + + private func build( + _ manifest: LocalEditorAssetManifest, + reportingTo progress: EditorProgressCallback + ) async throws -> EditorAssetBundle { var complete = 0 let tempDirectory = URL.temporaryDirectory.appending(path: UUID().uuidString) @@ -147,10 +163,16 @@ public actor EditorAssetLibrary { for await _ in group { complete += 1 - await progress?(EditorProgress(completed: complete, total: links.count)) + await progress(EditorProgress(completed: complete, total: links.count)) } } + // The group swallows every per-asset failure, cancellation included, so a + // cancelled build still arrives here with assets missing. Nothing downstream + // checks for them — `readAssetBundles()` reads only the manifest — so publishing + // it would serve the gap on every later launch. + try Task.checkCancellation() + return try bundle.copy(to: self.bundleRoot(for: bundle)) } diff --git a/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift b/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift index 40cfad115..fb2545758 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/EditorURLCache.swift @@ -8,11 +8,12 @@ import OSLog /// /// Backed by `SQLiteKVCache`. The cache directory is built as /// `//`, so two caches with different `siteId`s are -/// guaranteed-distinct backing files. The "one instance per backing file" -/// contract from `SQLiteKVCache` still applies for the same `(siteId, -/// parentDirectory)` pair, but the typical call pattern (one cache per -/// `EditorService`, one service per editor view) keeps that contract by -/// construction. +/// guaranteed-distinct backing files. Caches for the same `(siteId, +/// parentDirectory)` pair share one store through +/// `SQLiteKVCache.shared(handle:directory:diskCapacity:)`, which is what keeps +/// that store's "one instance per backing file" contract: every +/// `EditorService` builds its own cache, and a prefetch and an editor for the +/// same site routinely run at once. public struct EditorURLCache: Sendable { /// About enough for 10 sites of cached responses. private static let diskCapacity = Measurement(value: 100, unit: .mebibytes) @@ -38,7 +39,9 @@ public struct EditorURLCache: Sendable { parentDirectory: URL = Paths.defaultCacheRoot, cachePolicy: EditorCachePolicy = .always ) { - self.store = SQLiteKVCache( + // Shared: every service for a site builds its own cache, and two stores on one + // file break each other. + self.store = SQLiteKVCache.shared( handle: "editorurlcache", directory: parentDirectory.appending(path: siteId), diskCapacity: Self.diskCapacity @@ -159,6 +162,17 @@ public struct EditorURLCache: Sendable { try self.store.clear() } + /// Deletes every site's cache under `parentDirectory`, including any still in use. + /// + /// A cache still open when this is called fails every read and write from then on, so its + /// store is no longer shared: a cache created afterwards opens a new file. + static func deleteAll(in parentDirectory: URL = Paths.defaultCacheRoot) throws { + guard FileManager.default.directoryExists(at: parentDirectory) else { return } + // Whether or not the removal finishes: one that fails partway has still deleted files. + defer { SQLiteKVCache.forgetInstances(under: parentDirectory) } + try FileManager.default.removeItem(at: parentDirectory) + } + /// Combines the HTTP method and URL into a single string key. `SQLiteKVCache` /// hashes the key with SHA-256 before binding to SQLite, so length, escaping, /// and encoding aren't concerns here. diff --git a/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift b/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift index 706fd9060..9b723a8d3 100644 --- a/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift +++ b/ios/Sources/GutenbergKit/Sources/Stores/SQLiteKVCache.swift @@ -42,8 +42,10 @@ import SQLite3 /// instances with different caps would clobber each other's triggers; in the /// best case you get the wrong cap, in the worst case `SQLITE_BUSY` while the /// recreations race. Each backing file must have exactly one owning -/// `SQLiteKVCache` for the lifetime of the process. Not currently enforced at -/// runtime — this is a usage contract. +/// `SQLiteKVCache` at a time. Within a process, ``shared(handle:directory:diskCapacity:)`` +/// enforces that by handing every caller the live instance for its file; `init` +/// doesn't, so use it directly only where nothing else can open the file. Across +/// processes it remains a usage contract. /// /// **Schema migrations.** A `schemaVersion` constant baked into the build is /// compared against `PRAGMA user_version` on open; mismatches drop and recreate @@ -131,6 +133,11 @@ final class SQLiteKVCache: @unchecked Sendable { /// silently wipe users' caches a second time on the next upgrade). private static let schemaVersion: Int32 = 1 + /// How long a statement waits on another connection's lock before failing with + /// `SQLITE_BUSY`. The usual holder is the previous instance on the same file checkpointing + /// its WAL as it closes, which takes milliseconds; this only bounds a pathological one. + private static let busyTimeoutMilliseconds: Int32 = 5_000 + private static let logger = Logger(subsystem: "GutenbergKit", category: "sqlite-kv-cache") /// SQLite C API: signals that bound data should be copied. Reinvented here @@ -203,6 +210,9 @@ final class SQLiteKVCache: @unchecked Sendable { try Self.openAndConfigure(directory: self.directory, filename: self.filename, diskCapacity: self.diskCapacity) } dbResult = result + if case .failure = result { + Self.forget(self) + } return try result.get() } @@ -248,6 +258,14 @@ final class SQLiteKVCache: @unchecked Sendable { throw Error.databaseUnavailable } + // Wait out another connection's lock rather than fail on it: `connection()` caches a + // failure for the life of the cache, so a lock held for milliseconds would break it for + // good. `shared(handle:directory:diskCapacity:)` makes that routine — it hands out a + // fresh instance as soon as the last one is released, while that one's `deinit` may + // still be checkpointing the WAL. Reopening in that window failed 200 times in 200 + // without a timeout, and never with one. + sqlite3_busy_timeout(connection, Self.busyTimeoutMilliseconds) + // Pragmas + schema setup. Pragmas first because `journal_mode` // changes must run with no active transaction. `journal_mode = WAL` // switches from the default rollback-journal to a write-ahead log: @@ -675,6 +693,77 @@ extension SQLiteKVCache.Error: CustomStringConvertible, LocalizedError { extension SQLiteKVCache { + /// The live cache for `handle` in `directory`, created if there is none. + /// + /// Two instances on one file race their opens, and the loser caches its failure and + /// throws for the rest of its life. Measured with two `EditorURLCache`s making their + /// first read at the same moment: at least one ended up broken in 50 runs out of 50. + /// The busy timeout doesn't save it — the switch to WAL fails with `SQLITE_BUSY` + /// without waiting on it. Every caller that can share a file must come through here. + /// + /// Callers sharing a file must agree on `diskCapacity`: one that finds the file open + /// gets the live instance as it is, cap included. + static func shared( + handle: StaticString, + directory: URL = URL.cachesDirectory, + diskCapacity: Measurement + ) -> SQLiteKVCache { + let file = registryKey(directory: directory, filename: "\(handle)".lowercased()) + let bytes = Int(diskCapacity.converted(to: .bytes).value) + return liveInstancesLock.withLock { + if let live = liveInstances[file]?.instance { + assert( + live.diskCapacity == bytes, + "'\(handle)' is already open with a different disk capacity; callers sharing a file must agree on it" + ) + return live + } + let instance = SQLiteKVCache(handle: handle, directory: directory, diskCapacity: bytes) + liveInstances[file] = WeakInstance(instance: instance) + return instance + } + } + + /// Stops `shared` handing out the instances for files under `directory`, so the next caller + /// for each opens it afresh. For a caller that has just deleted `directory`: an instance + /// still open on a deleted file fails every read and write with `SQLITE_IOERR`, and would + /// go on being shared for as long as anything held it. + static func forgetInstances(under directory: URL) { + let root = directory.standardizedFileURL.path(percentEncoded: false) + let prefix = root.hasSuffix("/") ? root : root + "/" + liveInstancesLock.withLock { + liveInstances = liveInstances.filter { !$0.key.hasPrefix(prefix) } + } + } + + /// Stops `shared` handing out `instance`, whose open has failed. It keeps that failure for + /// life, so sharing it would fail every later caller for as long as anything held it. It + /// has closed its handle, so the fresh instance the next caller gets has the file to itself. + /// + /// Called with the instance's `openLock` held, which is why `shared` asks nothing of an + /// instance: an open can take as long as the busy timeout, and `shared` runs on the main + /// thread whenever an editor builds its service. + private static func forget(_ instance: SQLiteKVCache) { + let file = registryKey(directory: instance.directory, filename: instance.filename) + liveInstancesLock.withLock { + if liveInstances[file]?.instance === instance { + liveInstances[file] = nil + } + } + } + + private static func registryKey(directory: URL, filename: String) -> String { + directory.standardizedFileURL.appending(component: filename).path(percentEncoded: false) + } + + /// Weak, so a file no one is using is closed, and opened afresh by the next caller. + nonisolated(unsafe) private static var liveInstances: [String: WeakInstance] = [:] + private static let liveInstancesLock = NSLock() + + private struct WeakInstance { + weak var instance: SQLiteKVCache? + } + /// Convenience initializer that accepts the cap as a /// `Measurement` so callers can write /// `Measurement(value: 100, unit: .mebibytes)` instead of an opaque diff --git a/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift new file mode 100644 index 000000000..e4ec8c020 --- /dev/null +++ b/ios/Sources/GutenbergKitHTTP/HTTPRequestHandler.swift @@ -0,0 +1,42 @@ +#if canImport(Network) + +import Foundation + +/// Serves requests for an ``HTTPServer``. +/// +/// The closure form of +/// ``HTTPServer/start(name:port:listenOnAllInterfaces:requiresAuthentication:maxRequestBodySize:maxConnections:readTimeout:bodyReadTimeout:idleTimeout:startTimeout:cors:delegate:handler:)-(_,_,_,_,_,_,_,_,_,_,_,_,@escaping@Sendable(HTTPServer.Request)async->HTTPResponse)`` +/// is the right tool for a handler that needs no state. Conform to this instead when +/// the handler has dependencies: they become stored properties, and the request +/// methods become ordinary instance methods rather than statics threading a context +/// parameter through every call. +/// +/// ## Lifetimes +/// +/// The server retains its handler for its lifetime, so a handler must not strongly +/// hold the object that owns the server, directly or transitively: +/// `owner → HTTPServer → handler → owner` is a cycle, the owner's `deinit` never runs, +/// and `stop()` is never called — a silently stranded listener, not a crash. +/// +/// A value type is **not** protection. A `struct` handler storing the owner closes the +/// same ring: the server captures the struct into a heap node, and its stored properties +/// are strong edges out of it. This protocol is deliberately **not** `AnyObject`-constrained +/// so a handler *can* be a `struct` holding only what it needs — not because a `struct` is +/// safe by construction. Either shape works; both must stay leaves, the same discipline +/// ``HTTPServerDelegate`` documents. +/// +/// The usual trap is the object that starts the server also serving it — a view controller +/// starting it in `viewDidLoad` and stopping it in `deinit` is the shape that bites, because +/// the cycle disables the very teardown meant to break it. Conform a separate leaf type, or +/// call `stop()` from a hook that does run. +public protocol HTTPRequestHandler: Sendable { + /// The response for a request the server has parsed and authenticated. + /// + /// Called once per request, concurrently across connections — hence `Sendable`. + /// Cancellation is cooperative: the server cancels this task when the client + /// disconnects or the server stops, and discards whatever a cancelled task + /// returns, so check `Task.isCancelled` before any side effect you can't undo. + func handle(_ request: HTTPServer.Request) async -> HTTPResponse +} + +#endif // canImport(Network) diff --git a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift index ac05fbb01..be7001e18 100644 --- a/ios/Sources/GutenbergKitHTTP/HTTPServer.swift +++ b/ios/Sources/GutenbergKitHTTP/HTTPServer.swift @@ -277,6 +277,46 @@ public final class HTTPServer: Sendable { } } + /// Starts a server that serves requests from an ``HTTPRequestHandler`` object + /// rather than a closure. + /// + /// Everything else behaves identically — this forwards to the closure form. Reach + /// for it when the handler has dependencies to hold: a `struct` conformer stores + /// them and serves from instance methods, instead of statics threading a context + /// parameter through every call. See ``HTTPRequestHandler`` for the (short) + /// lifetime rules. + public static func start( + name: String, + port: UInt16? = nil, + listenOnAllInterfaces: Bool = false, + requiresAuthentication: Bool = true, + maxRequestBodySize: Int64 = HTTPRequestParser.defaultMaxBodySize, + maxConnections: Int = HTTPServer.defaultMaxConnections, + readTimeout: Duration = HTTPServer.defaultReadTimeout, + bodyReadTimeout: Duration? = nil, + idleTimeout: Duration = HTTPServer.defaultIdleTimeout, + startTimeout: Duration = HTTPServer.defaultStartTimeout, + cors: CORSPolicy = .none, + delegate: HTTPServerDelegate? = nil, + handler: some HTTPRequestHandler + ) async throws -> HTTPServer { + try await start( + name: name, + port: port, + listenOnAllInterfaces: listenOnAllInterfaces, + requiresAuthentication: requiresAuthentication, + maxRequestBodySize: maxRequestBodySize, + maxConnections: maxConnections, + readTimeout: readTimeout, + bodyReadTimeout: bodyReadTimeout, + idleTimeout: idleTimeout, + startTimeout: startTimeout, + cors: cors, + delegate: delegate, + handler: { await handler.handle($0) } + ) + } + /// Races `operation` against `timeout`, throwing ``HTTPServerError/startTimeout`` /// if the timeout wins. Used to bound the wait for the listener to become ready /// so a caller — such as the editor load awaiting the upload server's bind — @@ -305,15 +345,37 @@ public final class HTTPServer: Sendable { /// are currently executing will receive a `CancellationError`. public func stop() { listener.cancel() + releaseConnectionHandler() connectionTasks.cancelAll() Logger.httpServer.info("HTTP server stopped") } deinit { listener.cancel() + releaseConnectionHandler() connectionTasks.cancelAll() } + /// Drops the connection handler so teardown releases what it captured *here*, + /// on the caller's thread. + /// + /// `newConnectionHandler` retains the request handler, and through it whatever + /// the caller's closure captured. `cancel()` alone does not drop the block: + /// Network.framework holds the listener until cancellation completes on its own + /// queue, so the final release — and therefore the captured object's `deinit` — + /// lands there rather than wherever `stop()` was called. + /// + /// That covers an idle server. A request still in flight holds its own copy of what + /// the handler captured until that task unwinds, so a server stopped mid-request + /// releases last on the task's executor no matter what this does. + /// + /// Clearing it after `cancel()` rather than before is deliberate: the listener is + /// already torn down, so there is no window in which it is live but has no handler + /// to hand a connection to. + private func releaseConnectionHandler() { + listener.newConnectionHandler = nil + } + /// The library's default response for a parse error: the mapped status code /// with a plain-text body echoing the RFC reason phrase (e.g. 413 "Content Too /// Large"). This is what fatal errors always use, what a recoverable error uses diff --git a/ios/Sources/GutenbergKitHTTP/README.md b/ios/Sources/GutenbergKitHTTP/README.md index 43721b305..86c14e380 100644 --- a/ios/Sources/GutenbergKitHTTP/README.md +++ b/ios/Sources/GutenbergKitHTTP/README.md @@ -46,6 +46,24 @@ server.stop() Pass `nil` (or omit `port`) to let the system assign an available port — useful for tests or when running multiple servers. +#### Handlers with state + +A closure is right for a handler that needs no state. When the handler has dependencies, conform a type to `HTTPRequestHandler` and pass it as `handler:` instead — the dependencies become stored properties and the request logic becomes instance methods, rather than statics threading a context parameter through every call. + +```swift +struct MediaHandler: HTTPRequestHandler { + let uploader: Uploader + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + await uploader.upload(request.parsed.body) + } +} + +let server = try await HTTPServer.start(name: "media", handler: MediaHandler(uploader: uploader)) +``` + +The server retains its handler, so a handler must not strongly hold the object that owns the server, directly or transitively — `owner → HTTPServer → handler → owner` is a cycle, and the owner's `deinit` would never run. A value type is **not** protection here: a `struct` handler storing the owner closes the same ring, because the server captures the struct into a heap node and its stored properties are strong edges out of it. `HTTPRequestHandler` is deliberately not `AnyObject`-constrained so a handler *can* be a `struct` holding only what it needs — not because a `struct` is safe by construction. Either shape works, as long as it stays a leaf. + When `requiresAuthentication` is enabled (the default), each request must include a `Proxy-Authorization: Bearer ` header carrying the server's randomly-generated token. The server uses `Proxy-Authorization` per RFC 9110 §11.7.1 rather than `Authorization`, so the client's `Authorization` header remains available for upstream credentials (e.g. HTTP Basic auth to the remote server). Unauthenticated requests receive a `407 Proxy Authentication Required` response with a `Proxy-Authenticate: Bearer` challenge header. ### Proxying via URLSession diff --git a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift index 17be7d603..fc86f22a9 100644 --- a/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift +++ b/ios/Tests/GutenbergKitHTTPTests/HTTPServerStartTests.swift @@ -38,6 +38,36 @@ struct HTTPServerStartTests { // rather than suspending its caller indefinitely. #expect(elapsed < .seconds(5)) } + + @Test("serves requests from an HTTPRequestHandler object, carrying its state") + func servesFromRequestHandlerObject() async throws { + // The point of the object overload: the handler holds its dependencies as + // stored properties and serves from an instance method, so a consumer with + // state doesn't need statics threading a context through every call. + let server = try await HTTPServer.start( + name: "handler-object-test", + requiresAuthentication: false, + handler: EchoHandler(greeting: "hello from a struct") + ) + defer { server.stop() } + + let url = URL(string: "http://127.0.0.1:\(server.port)/anything")! + let (data, response) = try await URLSession.shared.data(from: url) + + #expect((response as? HTTPURLResponse)?.statusCode == 200) + #expect(String(decoding: data, as: UTF8.self) == "hello from a struct") + } +} + +/// A value-type handler, holding only a `String` — so it has no strong edge back to +/// whatever owns the server. ``HTTPRequestHandler`` isn't `AnyObject`-constrained so a +/// handler *can* take this shape; a `struct` storing the owner would still cycle. +private struct EchoHandler: HTTPRequestHandler { + let greeting: String + + func handle(_ request: HTTPServer.Request) async -> HTTPResponse { + HTTPResponse(status: 200, body: Data(greeting.utf8)) + } } #endif diff --git a/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift index 0d49d9bfe..2983c13bd 100644 --- a/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift +++ b/ios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift @@ -460,6 +460,84 @@ struct EditorHTTPClientTests { #expect(userAgent.contains("macOS/")) #endif } + + // MARK: - Sharing Tests + + @Test("identical requests from clients on one session share a key") + func identicalRequestsShareAKey() async throws { + let session = SpyURLSession() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let first = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + let second = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + #expect(first != nil) + #expect(first == second) + } + + @Test("requests with other credentials, sessions, or timeouts don't share") + func requestsWithOtherCredentialsSessionsOrTimeoutsDontShare() async throws { + let session = SpyURLSession() + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let key = await EditorHTTPClient(urlSession: session, authHeader: "Bearer a").sharedRequest(for: request) + + #expect(await EditorHTTPClient(urlSession: session, authHeader: "Bearer b").sharedRequest(for: request) != key) + #expect(await EditorHTTPClient(urlSession: SpyURLSession(), authHeader: "Bearer a").sharedRequest(for: request) != key) + #expect(await EditorHTTPClient(urlSession: session, authHeader: "Bearer a", requestTimeout: 5).sharedRequest(for: request) != key) + } + + @Test("only safe requests without a body, from a client no delegate watches, are shared") + func onlySafeUnwatchedRequestsAreShared() async throws { + let session = SpyURLSession() + let client = EditorHTTPClient(urlSession: session, authHeader: "Bearer a") + let url = URL(string: "https://example.com/wp-json/wp/v2/settings")! + + #expect(await client.sharedRequest(for: URLRequest(method: .OPTIONS, url: url)) != nil) + #expect(await client.sharedRequest(for: URLRequest(method: .POST, url: url)) == nil) + + var withBody = URLRequest(url: url) + withBody.httpBody = Data("{}".utf8) + #expect(await client.sharedRequest(for: withBody) == nil) + + let watched = EditorHTTPClient(urlSession: session, authHeader: "Bearer a", delegate: SpyHTTPClientDelegate()) + #expect(await watched.sharedRequest(for: URLRequest(url: url)) == nil) + } + + @Test("a request that asks to skip the cache goes out alone") + func aRequestThatSkipsTheCacheGoesOutAlone() async throws { + let client = EditorHTTPClient(urlSession: SpyURLSession(), authHeader: "Bearer a") + var request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/posts/5")!) + + let freshAnswerPolicies: [URLRequest.CachePolicy] = [ + .reloadIgnoringLocalCacheData, .reloadIgnoringLocalAndRemoteCacheData, .reloadRevalidatingCacheData, + ] + for policy in freshAnswerPolicies { + request.cachePolicy = policy + #expect(await client.sharedRequest(for: request) == nil) + } + + request.cachePolicy = .returnCacheDataElseLoad + #expect(await client.sharedRequest(for: request) != nil) + } + + @Test("identical requests in flight go out once") + func identicalRequestsInFlightGoOutOnce() async throws { + let session = ParkedURLSession() + defer { session.release() } + let request = URLRequest(url: URL(string: "https://example.com/wp-json/wp/v2/types")!) + let clients = [ + EditorHTTPClient(urlSession: session, authHeader: "Bearer a"), + EditorHTTPClient(urlSession: session, authHeader: "Bearer a"), + ] + let key = try #require(await clients[0].sharedRequest(for: request)) + + let callers = clients.map { client in Task { try await client.perform(request) } } + try await waitUntil { EditorHTTPClient.inFlightRequests.waiterCount(for: key) == 2 } + + session.release() // fails the parked request, for every caller waiting on it + for caller in callers { + await #expect(throws: URLError.self) { try await caller.value } + } + #expect(session.requestCount == 1) + } } fileprivate extension EditorResponseData { diff --git a/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift new file mode 100644 index 000000000..4807715a6 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/EditorViewControllerLifecycleTests.swift @@ -0,0 +1,95 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +#if canImport(UIKit) +import UIKit + +/// What happens to an in-flight dependency fetch when its editor is covered or +/// released. Nothing restarts the fetch, so the editor never recovers from a cancel. +@Suite("EditorViewController dependency fetch lifecycle") +struct EditorViewControllerLifecycleTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://test.example.com")! + static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! + + @MainActor + @Test("covering the editor leaves the dependency fetch running") + func coveringTheEditorDoesNotCancelTheDependencyFetch() async throws { + let session = ParkedURLSession() + let configuration = makeIsolatedConfiguration() + defer { removeStorage(for: configuration) } + defer { session.release() } + let editor = makeEditor(configuration: configuration, session: session) + + _ = editor.view // triggers `viewDidLoad`, which starts the fetch + try await session.waitUntilStarted() + + // The editor appears, then a full-screen modal or a push covers it. + editor.beginAppearanceTransition(true, animated: false) + editor.endAppearanceTransition() + editor.beginAppearanceTransition(false, animated: false) + editor.endAppearanceTransition() + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(500)) + #expect(!cancelled) + } + + /// The loader owns the fetch and reaches the editor only weakly, so a released + /// editor is freed while its fetch is still parked — and the fetch keeps running. + @MainActor + @Test("releasing the editor mid-fetch frees it, and leaves the fetch running") + func releasingTheEditorMidFetchFreesIt() async throws { + let session = ParkedURLSession() + let configuration = makeIsolatedConfiguration() + defer { removeStorage(for: configuration) } + defer { session.release() } + var editor: EditorViewController? = makeEditor(configuration: configuration, session: session) + weak let releasedEditor = editor + + _ = editor?.view // triggers `viewDidLoad`, which starts the fetch + try await session.waitUntilStarted() + + // Polled rather than checked once, so it doesn't depend on exactly when UIKit + // lets go. The fetch stays parked throughout, so it can't be what lets go. + editor = nil + let clock = ContinuousClock() + let deadline = clock.now + .seconds(2) + while releasedEditor != nil && clock.now < deadline { + try await Task.sleep(for: .milliseconds(20)) + } + #expect(releasedEditor == nil, "the fetch should not hold the editor") + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(250)) + #expect(!cancelled, "freeing the editor should not cancel the fetch") + } + + /// A unique `siteId` per call, so no earlier run's cache can serve the fetch. + /// Pair every call with `removeStorage(for:)`: nothing else deletes the site's files. + private func makeIsolatedConfiguration() -> EditorConfiguration { + makeConfiguration( + siteURL: URL(string: "https://\(UUID().uuidString).example.invalid")! + ) + } + + /// An editor whose every network call lands in `session`. + @MainActor + private func makeEditor( + configuration: EditorConfiguration, + session: ParkedURLSession + ) -> EditorViewController { + EditorViewController( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader) + ) + } + + /// Deletes what the editor wrote for this site. `EditorViewController` can't be + /// pointed at a temporary directory the way `MakesTestFixtures.makeService` can. + private func removeStorage(for configuration: EditorConfiguration) { + try? FileManager.default.removeItem(at: Paths.storageRoot(for: configuration)) + try? FileManager.default.removeItem(at: Paths.cacheRoot(for: configuration)) + } +} + +#endif diff --git a/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift b/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift new file mode 100644 index 000000000..be31af8be --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Helpers/ParkedURLSession.swift @@ -0,0 +1,77 @@ +import Foundation +@testable import GutenbergKit + +/// A `URLSessionProtocol` whose requests never finish until the test lets them, so +/// work started against it stays in flight for as long as the test needs, and which +/// records whether the task waiting on a request was cancelled. +final class ParkedURLSession: URLSessionProtocol, @unchecked Sendable { + private let lock = NSLock() + private var started = false + private var cancelled = false + private var released = false + private var requests = 0 + + private var isStarted: Bool { lock.withLock { started } } + private var isCancelled: Bool { lock.withLock { cancelled } } + private var isReleased: Bool { lock.withLock { released } } + + /// How many requests have been made against this session. + var requestCount: Int { lock.withLock { requests } } + + func data(for request: URLRequest) async throws -> (Data, URLResponse) { + try await park() + } + + func download(for request: URLRequest, delegate: (any URLSessionTaskDelegate)?) async throws -> (URL, URLResponse) { + try await park() + } + + /// Lets every parked request fail, so the work waiting on them — and the task + /// running it — finishes. Always call this: a request left parked stays in + /// flight for the rest of the run. + func release() { + lock.withLock { released = true } + } + + /// Suspends until `release()` or until the calling task is cancelled. + /// `Never` because every exit throws — it satisfies both return types. + private func park() async throws -> Never { + lock.withLock { + started = true + requests += 1 + } + while !isReleased { + do { + try await Task.sleep(for: .milliseconds(20)) + } catch { + lock.withLock { cancelled = true } + throw URLError(.cancelled) + } + } + throw URLError(.networkConnectionLost) + } + + func waitUntilStarted(timeout: Duration = .seconds(10)) async throws { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isStarted { return } + try await Task.sleep(for: .milliseconds(20)) + } + throw ParkedURLSessionTimeout.requestNeverStarted + } + + func waitUntilCancelled(timeout: Duration) async -> Bool { + let clock = ContinuousClock() + let deadline = clock.now + timeout + while clock.now < deadline { + if isCancelled { return true } + try? await Task.sleep(for: .milliseconds(20)) + } + return isCancelled + } +} + +enum ParkedURLSessionTimeout: Error { + case requestNeverStarted +} diff --git a/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift b/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift new file mode 100644 index 000000000..72376d2b2 --- /dev/null +++ b/ios/Tests/GutenbergKitTests/InFlightTasksTests.swift @@ -0,0 +1,228 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +@Suite("InFlightTasks", .timeLimit(.minutes(1))) +struct InFlightTasksTests { + + /// Each test's own table, so tests running in parallel can't meet in it. + private let tasks = InFlightTasks() + + // MARK: - Sharing + + @Test("callers for the same key share one task, and all hear its progress") + func callersForTheSameKeyShareOneTask() async throws { + let work = ParkedWork() + let (first, second) = (ProgressTracker(), ProgressTracker()) + let a = startCaller(for: "key", running: work, progress: first) + let b = startCaller(for: "key", running: work, progress: second) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + try await waitUntil { !first.updates.isEmpty && !second.updates.isEmpty } + + work.release() + #expect(try await a.value == ParkedWork.value) + #expect(try await b.value == ParkedWork.value) + #expect(work.runs == 1) + } + + @Test("callers for different keys run separately") + func callersForDifferentKeysRunSeparately() async throws { + let work = ParkedWork() + let one = startCaller(for: "one", running: work) + let other = startCaller(for: "other", running: work) + try await waitUntil { work.runs == 2 } + + work.release() + #expect(try await one.value == ParkedWork.value) + #expect(try await other.value == ParkedWork.value) + } + + @Test("a failed task fails every caller waiting on it") + func aFailedTaskFailsEveryCaller() async throws { + let work = ParkedWork() + let a = startCaller(for: "key", running: work) + let b = startCaller(for: "key", running: work) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + + work.release(throwing: URLError(.timedOut)) + await #expect(throws: URLError.self) { try await a.value } + await #expect(throws: URLError.self) { try await b.value } + } + + // MARK: - Cancellation + + @Test("cancelling a caller ends its wait at once, and leaves the task running for the rest") + func cancellingACallerLeavesTheTaskRunning() async throws { + let work = ParkedWork() + let leaving = startCaller(for: "key", running: work) + let staying = startCaller(for: "key", running: work) + try await waitUntil { tasks.waiterCount(for: "key") == 2 } + + leaving.cancel() + // Returns while the task is still parked — it doesn't wait for it. + await #expect(throws: CancellationError.self) { try await leaving.value } + #expect(!work.wasCancelled) + + work.release() + #expect(try await staying.value == ParkedWork.value) + } + + @Test("the last caller leaving cancels the task, and the next caller starts afresh") + func theLastCallerLeavingCancelsTheTask() async throws { + let work = ParkedWork() + let first = startCaller(for: "key", running: work) + try await waitUntil { work.runs == 1 } + + first.cancel() + await #expect(throws: CancellationError.self) { try await first.value } + try await waitUntil { work.wasCancelled } + + let next = startCaller(for: "key", running: work) + try await waitUntil { work.runs == 2 } + work.release() + #expect(try await next.value == ParkedWork.value) + } + + @Test("a caller that has left hears no more progress, even from a report already under way") + func aCallerThatHasLeftHearsNoMoreProgress() async throws { + let work = ParkedWork() + let gate = ProgressGate() + let leaver = ProgressTracker() + let staying = startCaller(for: "key", running: work, onProgress: { await gate.pass($0) }) + try await waitUntil { tasks.waiterCount(for: "key") == 1 } + let leaving = startCaller(for: "key", running: work, progress: leaver) + try await waitUntil { !leaver.updates.isEmpty } + + // Hold a report in the staying caller's callback. The leaving caller had already heard + // one, so it is on this report's list too, waiting its turn. + gate.close() + try await waitUntil { gate.isHolding } + leaving.cancel() + await #expect(throws: CancellationError.self) { try await leaving.value } + let heard = leaver.count + + // Let the held report finish and the next one start, so the leaving caller's turn is past. + let passes = gate.passes + gate.open() + try await waitUntil { gate.passes >= passes + 2 } + #expect(leaver.count == heard, "the leaving caller should hear nothing once it has left") + + work.release() + #expect(try await staying.value == ParkedWork.value) + } + + // MARK: - Priority + + @Test("a caller joining at a higher priority raises the task to match") + func aHigherPriorityCallerRaisesTheTask() async throws { + guard #available(iOS 26, macOS 26, *) else { return } + let work = ParkedWork() + let low = startCaller(for: "key", running: work, priority: .utility) + try await waitUntil { work.runs == 1 } + #expect(work.priority.rawValue < TaskPriority.userInitiated.rawValue) + + let high = startCaller(for: "key", running: work, priority: .userInitiated) + try await waitUntil { work.priority.rawValue >= TaskPriority.userInitiated.rawValue } + + work.release() + #expect(try await low.value == ParkedWork.value) + #expect(try await high.value == ParkedWork.value) + } + + // MARK: - Helpers + + /// A caller waiting on `key`, which runs `work` if nothing is in flight for it yet. It hears + /// progress through `onProgress`, or else records it in `progress`. + private func startCaller( + for key: String, + running work: ParkedWork, + priority: TaskPriority? = nil, + progress: ProgressTracker? = nil, + onProgress: EditorProgressCallback? = nil + ) -> Task { + let callback: EditorProgressCallback? = onProgress ?? progress.map { tracker in + { @Sendable (update: EditorProgress) async in tracker.append(update) } + } + return Task(priority: priority) { [tasks] in + try await tasks.value(for: key, progress: callback) { report in + try await work.run(reporting: report) + } + } + } +} + +/// A progress callback the test can close, holding whichever report reaches it until it reopens. +private final class ProgressGate: @unchecked Sendable { + private let lock = NSLock() + private var closed = false + private var holding = false + private var passCount = 0 + + /// Whether a report is held here right now. + var isHolding: Bool { lock.withLock { holding } } + + /// How many reports have been let through. + var passes: Int { lock.withLock { passCount } } + + func close() { lock.withLock { closed = true } } + func open() { lock.withLock { closed = false } } + + func pass(_ progress: EditorProgress) async { + while lock.withLock({ holding = closed; return closed }) { + try? await Task.sleep(for: .milliseconds(5)) + } + lock.withLock { + holding = false + passCount += 1 + } + } +} + +/// Work that runs until released: it counts its runs, reports progress while it waits, and +/// records whether it was cancelled. +private final class ParkedWork: @unchecked Sendable { + static let value = 42 + + private let lock = NSLock() + private var runCount = 0 + private var cancelled = false + private var released = false + private var failure: (any Error)? + private var latestPriority = TaskPriority.medium + + var runs: Int { lock.withLock { runCount } } + var wasCancelled: Bool { lock.withLock { cancelled } } + + /// The priority the latest run was going at when it last checked. + var priority: TaskPriority { lock.withLock { latestPriority } } + + /// Lets every run finish: with `failure` if there is one, or with ``value``. + func release(throwing failure: (any Error)? = nil) { + lock.withLock { + self.failure = failure + released = true + } + } + + func run(reporting report: EditorProgressCallback) async throws -> Int { + lock.withLock { + latestPriority = Task.currentPriority + runCount += 1 + } + while !lock.withLock({ released }) { + lock.withLock { latestPriority = Task.currentPriority } + await report(EditorProgress(completed: 1, total: 100)) + do { + try await Task.sleep(for: .milliseconds(10)) + } catch { + lock.withLock { cancelled = true } + throw error + } + } + if let failure = lock.withLock({ failure }) { + throw failure + } + return Self.value + } +} diff --git a/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift new file mode 100644 index 000000000..98d66e49b --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Media/EditorViewControllerMediaTeardownTests.swift @@ -0,0 +1,173 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +#if canImport(UIKit) + +/// Pins that ``EditorViewController/stopMediaHandling()`` opens the ownership cycle a host +/// can form, and that a host which doesn't form one needs nothing. +/// +/// The editor holds `mediaProcessor` strongly so an in-flight upload can't lose it +/// mid-request. The cost is that a host which holds the editor back closes a cycle ARC +/// cannot break — and `deinit`, which does this work on every other path, is exactly what +/// a cycle prevents. `stopMediaHandling()` is the way out, and it has to be the host's +/// call: not because UIKit can't report a teardown, but because it can't report whether +/// one is permanent. A host may re-present or re-attach the same editor, and the call is +/// terminal, so guessing wrong disables media in an editor that survived. +@Suite("EditorViewController media teardown") +struct EditorViewControllerMediaTeardownTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://test.example.com")! + static let testApiRoot = URL(string: "https://test.example.com/wp-json/wp/v2")! + + @MainActor + @Test("stopMediaHandling frees the editor and the host processor that owns it") + func stopMediaHandlingBreaksTheOwnershipCycle() async { + weak var weakEditor: EditorViewController? + weak var weakHost: EditorOwningProcessor? + + do { + let host = EditorOwningProcessor(configuration: makeConfiguration()) + weakEditor = host.editor + weakHost = host + host.editor.stopMediaHandling() + } + + await waitForRelease { weakHost == nil && weakEditor == nil } + + #expect(weakHost == nil, "host processor leaked — stopMediaHandling did not release it") + #expect(weakEditor == nil, "EditorViewController leaked — cycle through mediaProcessor") + } + + @MainActor + @Test("a host that does not retain the editor is freed without stopMediaHandling") + func standaloneProcessorIsFreed() async { + weak var weakEditor: EditorViewController? + + do { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: StandaloneProcessor() + ) + weakEditor = editor + } + + await waitForRelease { weakEditor == nil } + + #expect(weakEditor == nil, "EditorViewController leaked — nothing here retains it") + } + + // MARK: - Which handlers bring the server up + + /// The regression this pins: `startUploadServer()` reads "did the host supply a + /// handler" twice — once before starting, once after the bind returns — and the two + /// reads drifted. The first gained `mediaUploader`, the second kept checking the + /// processor alone, so an uploader-only host bound a listener and then immediately + /// stopped it. `uploadServer` stayed nil, the page was advertised `nativeUploadPort: + /// nil`, and `api-fetch.js` fell through to the plain WebView path — so the host's + /// `upload(_:)` was never called for any file, with nothing logged. + /// + /// Android pins the same gate (`GutenbergViewUploadServerTest`, "the upload server + /// starts for an uploader with no processor"); iOS had no equivalent, which is why the + /// drift survived three commits with a green suite. + @MainActor + @Test( + "the upload server starts for whichever handler the host supplied", + .enabled(if: canBindUploadServer), + arguments: [ + ("uploader only", false, true), + ("processor only", true, false), + ("both", true, true) + ] + ) + func uploadServerStartsForAnyHandler(_ label: String, processor: Bool, uploader: Bool) async { + let editor = EditorViewController( + configuration: makeConfiguration(), + mediaProcessor: processor ? StandaloneProcessor() : nil, + mediaUploader: uploader ? InertUploader() : nil + ) + defer { editor.stopMediaHandling() } + + await editor.startUploadServer() + + #expect(editor.uploadServer != nil, "\(label): no upload server, so the host's media handling never runs") + } + + @MainActor + @Test("no handler leaves the upload server down", .enabled(if: canBindUploadServer)) + func noHandlerLeavesServerDown() async { + let editor = EditorViewController(configuration: makeConfiguration()) + + await editor.startUploadServer() + + #expect(editor.uploadServer == nil, "started a server with nothing to route through it") + } + + /// Polls instead of asserting outright, because a `UIViewController` can sit in an + /// autorelease pool past the end of the scope that held it. Asserting synchronously + /// passes in isolation and fails in a full suite, where other tests keep the main + /// actor busy and the pool drains later. A real leak still fails this, a second later. + @MainActor + private func waitForRelease(_ isReleased: () -> Bool) async { + for _ in 0..<100 where !isReleased() { + try? await Task.sleep(for: .milliseconds(10)) + } + } +} + +/// The shape that cycles: owns the editor *and* is its processor. Hosts reach for this +/// because the coordinator driving the editor already has the site context. +@MainActor +private final class EditorOwningProcessor: MediaProcessor { + /// Implicitly unwrapped so `self` can be passed as the editor's processor: every stored + /// property then has a value (nil) on entry to `init`, which is what makes `self` + /// available there. Taking the processor at `init` doesn't prevent this shape — it just + /// moves where the host writes it. + private(set) var editor: EditorViewController! + + init(configuration: EditorConfiguration) { + editor = EditorViewController(configuration: configuration, mediaProcessor: self) + } + + nonisolated func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + nonisolated func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} + +private final class StandaloneProcessor: MediaProcessor { + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} + +#endif + +/// Supplied only to bring the upload server up; never invoked by these tests. +private struct InertUploader: MediaUploader { + func upload(_ upload: MediaUpload) async throws -> Data { Data() } +} + +/// Whether `HTTPServer` can bind here — it cannot in some sandboxes, and these tests +/// assert on a real listener. +private let canBindUploadServer: Bool = { + let result = UnsafeSendableBox(false) + let semaphore = DispatchSemaphore(value: 0) + Task { + if let server = try? await MediaUploadServer.start() { + server.stop() + result.value = true + } + semaphore.signal() + } + semaphore.wait() + return result.value +}() + +private final class UnsafeSendableBox: @unchecked Sendable { + var value: T + init(_ value: T) { self.value = value } +} diff --git a/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift index b5b39e349..d3f6b2966 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaServerCredentialsTests.swift @@ -17,10 +17,10 @@ struct MediaServerCredentialsTests { #expect(!MediaServerCredentials.areUsable(siteApiRoot: Self.siteRoot, authHeader: "")) } - // The two arms below are what a `URL` makes different from Android's `String`: - // `isEmpty()` has no direct equivalent, so "addressable" is spelled as scheme and - // host both being present. A URL missing either cannot reach the site, and every - // request built from it fails at the URLSession layer. + // "Addressable" is spelled as scheme and host both being present. A URL missing + // either cannot reach the site, and every request built from it fails at the + // URLSession layer. Android asserts the same two arms in its own + // `MediaServerCredentialsTest` — keep the cases in step. @Test("rejects a site root with no scheme") func rejectsSchemelessSiteRoot() { @@ -42,4 +42,51 @@ struct MediaServerCredentialsTests { func rejectsRelativeSiteRoot() { #expect(!MediaServerCredentials.areUsable(siteApiRoot: URL(string: "/")!, authHeader: "Bearer t")) } + + // MARK: - requireCredentialsForUploader + + @Test("accepts usable credentials, uploader or not", arguments: [true, false]) + func acceptsUsableCredentials(hasUploader: Bool) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: Self.siteRoot, authHeader: "Bearer t", hasUploader: hasUploader + ) + } + + @Test("ignores missing credentials when there is no uploader") + func ignoresMissingCredentialsWithoutUploader() { + // Nothing to deliver through, so nothing to process. This is not an error — the + // server just stays down (`areUsable` decides that) and uploads fall to the + // default WebView path, so a processor-only host must not trap here. + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: Self.siteRoot, authHeader: "", hasUploader: false + ) + } + + // Exit tests run the body in a child process, so they can assert the trap itself. + // They are unavailable on iOS — including the simulator — and referencing them + // there is a *compile* error rather than a skip, so the whole block is gated to + // the host platform. This is exactly why the policy lives outside + // `EditorViewController`: that type is `#if canImport(UIKit)`, so on the one + // platform that can run these tests it does not exist. +#if os(macOS) + + @Test("traps for an uploader with no auth header") + func trapsForUploaderWithoutAuthHeader() async { + await #expect(processExitsWith: .failure) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: URL(string: "https://example.com/wp-json/")!, authHeader: "", hasUploader: true + ) + } + } + + @Test("traps for an uploader with no site root") + func trapsForUploaderWithoutSiteRoot() async { + await #expect(processExitsWith: .failure) { + MediaServerCredentials.requireCredentialsForUploader( + siteApiRoot: URL(string: "/")!, authHeader: "Bearer t", hasUploader: true + ) + } + } + +#endif } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 60040e670..e698fe890 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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = ProcessOnlyProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: 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) @@ -141,8 +141,8 @@ struct MediaUploadServerTests { // Exercised through the delete relay because every response `relayResponse` // handles — WordPress's own included — carries a `Content-Type`, so this is // the ordinary path rather than an edge case. - let uploader = ContentTypeDeleteUploader() - let server = try await MediaUploadServer.start(defaultUploader: uploader) + let uploader = ContentTypeDeleteClient() + let server = try await MediaUploadServer.start(internalClient: uploader) defer { server.stop() } let url = URL(string: "http://127.0.0.1:\(server.port)/media/42?force=true")! @@ -157,10 +157,11 @@ struct MediaUploadServerTests { #expect(httpResponse.value(forHTTPHeaderField: "Content-Type") == "text/plain") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("processes with the processor, then delivers and relays verbatim") + func processesThenDelivers() async throws { + let processor = ResizingProcessor() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString @@ -178,24 +179,62 @@ 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") + // The processor only transforms; GutenbergKit performs the upload. + #expect(internalClient.uploadCalled) // The server relays WordPress's raw response body verbatim. let object = try JSONSerialization.jsonObject(with: data) let json = try #require(object as? [String: Any]) - #expect(json["id"] as? Int == 42) - #expect(json["source_url"] as? String == "https://example.com/photo.jpg") - #expect(json["media_type"] as? String == "image") + #expect(json["id"] as? Int == 99) + #expect(json["source_url"] as? String == "https://example.com/doc.pdf") + #expect(json["media_type"] as? String == "file") + } + + /// Pins the one capability dropping `: AnyObject` exists to deliver: a value type can + /// conform, and the server actually calls it. + /// + /// Every other conformer in the tree is a class, so without this nothing exercises the + /// boxed-existential path — copied into `Handler`, captured by the `@Sendable` + /// handler closure, read again at `processFile`. Re-imposing a class requirement, or + /// breaking that path, would otherwise compile and pass green and surface only in a + /// host's build. + /// + /// Asserts through the client's recorded metadata rather than state on the processor, + /// because a `struct` witnessing a non-mutating requirement cannot record anything — + /// which is the point. + @Test("a value-type processor is admitted, called, and its result delivered") + func valueTypeProcessorRuns() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start( + processor: ValueTypeProcessor(), internalClient: internalClient + ) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody( + boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", + data: Data("movie".utf8) + ) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The transcoded metadata could only come from `processFile` having run. + #expect(internalClient.uploadCalled) + #expect(internalClient.lastUploadMimeType == "video/mp4") + #expect(internalClient.lastUploadFilename == "clip.mp4") } - @Test("uses passthrough when delegate does not modify file") - func delegatePassthrough() async throws { - let delegate = ProcessOnlyDelegate() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + @Test("uses passthrough when processor does not modify file") + func processorPassthrough() async throws { + let processor = ProcessOnlyProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -213,7 +252,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) @@ -224,11 +263,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() - let mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + @Test("skips processing and the temp copy when the processor declines by metadata") + func processorDeclinesByMetadata() async throws { + let processor = DeclineByMetadataProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -245,18 +284,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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = ResizingProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -271,18 +310,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 mockUploader = MockDefaultUploader() - let server = try await MediaUploadServer.start(uploadDelegate: delegate, defaultUploader: mockUploader) + let processor = ResizingProcessor() + let mockUploader = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } let boundary = UUID().uuidString @@ -297,10 +336,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))) } @@ -385,22 +424,317 @@ 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? - let server: MediaUploadServer + @Test("an uploader performs the upload and its result is relayed") + func uploaderPerformsUpload() async throws { + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (data, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 201) + #expect(String(decoding: data, as: UTF8.self).contains("\"id\":7")) + // GutenbergKit stays out of the network when a host uploader is set. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + #expect(uploader.received?.filename == "photo.jpg") + #expect(uploader.received?.mimeType == "image/jpeg") + } + + @Test("an uploader receives the editor's form fields in order, and the query") + func uploaderReceivesFieldsAndQuery() async throws { + // Without `post` the attachment is created unattached, and repeated names (a + // `field[]` array) must survive as repeats rather than collapse into a dictionary. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + for (name, value) in [("post", "42"), ("tags[]", "a"), ("tags[]", "b")] { + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"\(name)\"\r\n\r\n") + body.append("\(value)\r\n") + } + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n") + body.append("Content-Type: image/jpeg\r\n\r\n") + body.append(Data("fake image data".utf8)) + body.append("\r\n--\(boundary)--\r\n") + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload?_embed=wp:featuredmedia")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + let received = try #require(uploader.received) + #expect(received.fields == [ + MediaUploadField(name: "post", value: "42"), + MediaUploadField(name: "tags[]", value: "a"), + MediaUploadField(name: "tags[]", value: "b"), + ]) + #expect(received.query == "?_embed=wp:featuredmedia") + } + + @Test("keeps a binary Blob part out of an uploader's fields") + func binaryPartExcludedFromFields() async throws { + // Pin rule 3: a Blob always has a filename, so it's dropped before the decode. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + var body = Data() + // Ordered as `uploadToServer` emits it: the file first, then additionalData. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n") + body.append("Content-Type: image/jpeg\r\n\r\n") + body.append(Data("fake image data".utf8)) + body.append("\r\n--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"post\"\r\n\r\n") + body.append("42\r\n") + // A Blob-shaped part: it has a filename, and its bytes are not valid UTF-8. + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"blob\"; filename=\"blob\"\r\n") + body.append("Content-Type: application/octet-stream\r\n\r\n") + body.append(Data([0xED, 0xA0, 0x80])) + body.append("\r\n--\(boundary)--\r\n") + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The Blob is dropped rather than decoded, and `file` is still the file. + let received = try #require(uploader.received) + #expect(received.filename == "photo.jpg") + #expect(received.fields == [MediaUploadField(name: "post", value: "42")]) + } + + @Test("round-trips a non-Latin field value exactly") + func nonLatinFieldRoundTrips() async throws { + // The other half: valid UTF-8 round-trips, so real captions and titles survive. + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let caption = "Grüße 🎉 日本語" + let boundary = UUID().uuidString + var body = Data() + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"caption\"\r\n\r\n") + body.append("\(caption)\r\n") + body.append("--\(boundary)\r\n") + body.append("Content-Disposition: form-data; name=\"file\"; filename=\"photo.jpg\"\r\n") + body.append("Content-Type: image/jpeg\r\n\r\n") + body.append(Data("fake image data".utf8)) + body.append("\r\n--\(boundary)--\r\n") + + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + #expect(uploader.received?.fields == [MediaUploadField(name: "caption", value: caption)]) + } + + @Test("a processor still processes the file an uploader delivers") + func processorRunsForUploader() async throws { + let processor = ProcessOnlyProcessor() + let uploader = RecordingUploader() + let server = try await MediaUploadServer.start(processor: processor, uploader: uploader, internalClient: MockInternalMediaClient()) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The processor still processes; only delivery moves to the uploader. + #expect(processor.processFileCalled) + #expect(uploader.received != nil) + } + + @Test("an uploader sees a file the processor's metadata gate would have declined") + func uploaderSeesDeclinedFile() async throws { + // The gate exists to skip a temp copy for a file the processor won't touch. An + // uploader takes over delivery for every file, so passing through here would + // silently bypass it. + let processor = DeclineByMetadataProcessor() + let uploader = RecordingUploader() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(processor: processor, uploader: uploader, internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + #expect(uploader.received?.filename == "clip.mov") + #expect(!internalClient.passthroughUploadCalled) + // ...but a declined file must still not reach `processFile`: `handlesFile` + // returning false is the processor saying it won't touch a file like this. + #expect(!processor.processFileCalled) + } + + @Test("an uploader that throws surfaces as a failure, with no GutenbergKit retry") + func uploaderThrowSurfaces() async throws { + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploader: ThrowingUploader(), internalClient: internalClient) + defer { server.stop() } + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "photo.jpg", mimeType: "image/jpeg", data: Data("fake image data".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + let (_, response) = try await URLSession.shared.data(for: request) + let httpResponse = try #require(response as? HTTPURLResponse) + + #expect(httpResponse.statusCode == 500) + // Recovery is the uploader's, not GutenbergKit's — it must not re-deliver. + #expect(!internalClient.uploadCalled) + #expect(!internalClient.passthroughUploadCalled) + } + + @Test("retains the processor for the server's lifetime, and releases it after") + func retainsProcessorForServerLifetime() async throws { + weak var weakProcessor: ProcessOnlyProcessor? do { - let delegate = MockUploadDelegate() - weakDelegate = delegate - server = try await MediaUploadServer.start(uploadDelegate: delegate) + var processor: ProcessOnlyProcessor? = ProcessOnlyProcessor() + weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor) + defer { server.stop() } + + // The server owns the processor while it runs: the host can assign one and drop + // its own reference, and every request still sees it. The host reference has to + // go *before* the assert, or the local satisfies it and the server's ownership + // is never what is under test — held weakly, this is already nil here. + processor = nil + #expect(weakProcessor != nil) } + + // …and lets go when it stops, so the processor isn't leaked for the process's + // lifetime. Asserted outright rather than polled: `HTTPServer.stop()` clears the + // listener's `newConnectionHandler`, which is what holds the handler closure and + // through it this processor, so the release lands synchronously on this thread + // instead of trailing an asynchronous `NWListener` cancellation onto its queue. + #expect(weakProcessor == nil) + } + + @Test("stopping frees a processor that holds the server back") + func stopReleasesProcessorThatRetainsTheServer() async throws { + // The server-side half of the ownership story, and the one nothing else covers. + // `EditorViewController.stopMediaHandling()` clears its own properties *and* stops + // the server, because releasing only one leaves the loop routed through the other: + // `listener -> newConnectionHandler -> Handler -> processor -> server`. + // + // Polled rather than asserted outright, unlike `retainsProcessorForServerLifetime`: + // `releaseConnectionHandler()` opens the loop on the caller's thread, but it is not + // the only thing that does. Cancelling an `NWListener` also releases the blocks it + // captured, for a deployment target of iOS 16 or later (this package requires 17) — + // rdar://89677097, documented in the macOS 13 release notes — and that release lands + // on the listener's own queue. Confirmed by no-op'ing `releaseConnectionHandler()`: + // the processor is still freed, a poll tick later. Before that OS change the blocks + // were held for the listener's lifetime, so a lowered deployment target hangs here + // instead of quietly stranding listeners. + weak var weakProcessor: ServerRetainingProcessor? + var server: MediaUploadServer? + + do { + let processor = ServerRetainingProcessor() + weakProcessor = processor + let started = try await MediaUploadServer.start(processor: processor) + processor.server = started // closes the loop: server -> handler -> processor -> server + server = started + } + + #expect(weakProcessor != nil, "the server should own the processor while it runs") + + server?.stop() + server = nil + + for _ in 0..<100 where weakProcessor != nil { + try await Task.sleep(for: .milliseconds(10)) + } + #expect(weakProcessor == nil, "processor leaked — stopping did not release the handler's references") + } + + @Test("still processes for a processor the host has dropped its reference to") + func processesForHostReleasedProcessor() async throws { + // The processor is read at the admission gate and again at processFile, separated + // by a synchronous disk copy and an unbounded processFile. + // Held weakly, a host that dropped its reference changed the answer between + // those reads: a file admitted for processing was forwarded unprocessed. The + // host dropping it before the request is the same condition, deterministically. + let mockUploader = MockInternalMediaClient() + var processor: ResizingProcessor? = ResizingProcessor() + weak let weakProcessor = processor + let server = try await MediaUploadServer.start(processor: processor, internalClient: mockUploader) defer { server.stop() } - // UploadContext holds the delegate weakly, so releasing the host's strong - // reference deallocates it. A strong reference here would reintroduce the - // EditorViewController → uploadServer → … → delegate → EditorViewController - // cycle, so deinit would never fire and the server would never stop. - #expect(weakDelegate == nil) + // Drop the host's only strong reference. Under the documented contract the + // server owns the processor from here, so the upload must still be processed. + processor = nil + + let boundary = UUID().uuidString + let body = buildMultipartBody(boundary: boundary, filename: "clip.mov", mimeType: "video/quicktime", data: Data("movie".utf8)) + let url = URL(string: "http://127.0.0.1:\(server.port)/upload")! + var request = URLRequest(url: url) + request.httpMethod = "POST" + request.setValue("Bearer \(server.token)", forHTTPHeaderField: "Relay-Authorization") + request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") + request.httpBody = body + + _ = try await URLSession.shared.data(for: request) + + // The server kept it alive, so the processed metadata reached the uploader. + // Against a weak container this fails with the real symptom: the passthrough + // branch runs and the original video/quicktime is forwarded unprocessed. + #expect(weakProcessor != nil) + #expect(mockUploader.uploadCalled) + #expect(mockUploader.lastUploadMimeType == "video/mp4") + #expect(!mockUploader.passthroughUploadCalled) } private func buildMultipartBody(boundary: String, filename: String, mimeType: String, data: Data) -> Data { @@ -416,7 +750,7 @@ struct MediaUploadServerTests { // MARK: - Streaming Multipart Body Tests -@Suite("DefaultMediaUploader streaming multipart body") +@Suite("InternalMediaClient streaming multipart body") struct MultipartBodyStreamTests { @Test("streaming output matches in-memory multipart format") @@ -439,7 +773,7 @@ struct MultipartBodyStreamTests { expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) // Build streaming output. - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [] ) #expect(contentLength == expected.count) @@ -456,7 +790,7 @@ struct MultipartBodyStreamTests { // Craft a filename, field name, and MIME type that each try to smuggle a CRLF // and a fake header into the body relayed to WordPress. - let (stream, _) = try DefaultMediaUploader.multipartBodyStream( + let (stream, _) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: "boundary", filename: "evil\"\r\nX-Injected-File: 1.jpg", @@ -491,7 +825,7 @@ struct MultipartBodyStreamTests { expected.append(fileContent) expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [("post", Data("123".utf8))] ) @@ -523,7 +857,7 @@ struct MultipartBodyStreamTests { expected.append(fileContent) expected.append(Data("\r\n--\(boundary)--\r\n".utf8)) - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: boundary, filename: filename, mimeType: mimeType, extraFields: [("blob", binaryValue)] ) @@ -539,7 +873,7 @@ struct MultipartBodyStreamTests { try fileContent.write(to: tempFile) defer { try? FileManager.default.removeItem(at: tempFile) } - let (stream, contentLength) = try DefaultMediaUploader.multipartBodyStream( + let (stream, contentLength) = try InternalMediaClient.multipartBodyStream( fileURL: tempFile, boundary: "boundary", filename: "big.bin", mimeType: "application/octet-stream", extraFields: [] ) @@ -563,7 +897,7 @@ struct MultipartBodyStreamTests { let preamble = Data("PREAMBLE".utf8) let epilogue = Data("EPILOGUE".utf8) - let ok = DefaultMediaUploader.writeMultipartBody( + let ok = InternalMediaClient.writeMultipartBody( fileHandle: fileHandle, fileSize: fileContent.count, preamble: preamble, epilogue: epilogue, to: output ) @@ -590,7 +924,7 @@ struct MultipartBodyStreamTests { let preamble = Data("PREAMBLE".utf8) let epilogue = Data("EPILOGUE".utf8) // Claim the file is larger than it is, as if it shrank after being measured. - let ok = DefaultMediaUploader.writeMultipartBody( + let ok = InternalMediaClient.writeMultipartBody( fileHandle: fileHandle, fileSize: fileContent.count + 100, preamble: preamble, epilogue: epilogue, to: output ) @@ -603,17 +937,17 @@ struct MultipartBodyStreamTests { } } -// MARK: - DefaultMediaUploader Relay Tests +// MARK: - InternalMediaClient Relay Tests -@Suite("DefaultMediaUploader relay") -struct DefaultMediaUploaderRelayTests { +@Suite("InternalMediaClient relay") +struct InternalMediaClientRelayTests { @Test("relays a non-2xx WordPress response instead of throwing") func relaysErrorResponseVerbatim() async throws { // A WordPress REST error body, returned with a non-2xx status. let errorBody = Data(#"{"code":"rest_cannot_create","message":"Sorry, you are not allowed to upload this file type."}"#.utf8) let client = RelayStubHTTPClient(statusCode: 403, body: errorBody) - let uploader = DefaultMediaUploader(httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) + let uploader = InternalMediaClient(httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent("relay-\(UUID().uuidString).jpg") try Data("fake image".utf8).write(to: tempFile) @@ -640,7 +974,7 @@ struct DefaultMediaUploaderRelayTests { body: Data(#"{"code":"rest_upload_error"}"#.utf8), headerFields: ["x-wp-upload-attachment-id": "4242"] ) - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent( @@ -663,7 +997,7 @@ struct DefaultMediaUploaderRelayTests { body: Data("{}".utf8), headerFields: ["X-Powered-By": "PHP/8.2", "Set-Cookie": "session=secret"] ) - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json/")!) let tempFile = FileManager.default.temporaryDirectory.appendingPathComponent( @@ -681,7 +1015,7 @@ struct DefaultMediaUploaderRelayTests { @Test("deletes an attachment, carrying the namespace and force query") func deletesAttachment() async throws { let client = URLCapturingHTTPClient() - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json")!, siteApiNamespace: ["sites/123"] @@ -697,7 +1031,7 @@ struct DefaultMediaUploaderRelayTests { @Test("carries the namespace and request query through to the media endpoint") func forwardsNamespaceAndQuery() async throws { let client = URLCapturingHTTPClient() - let uploader = DefaultMediaUploader( + let uploader = InternalMediaClient( httpClient: client, siteApiRoot: URL(string: "https://example.com/wp-json")!, siteApiNamespace: ["sites/123"] @@ -720,7 +1054,7 @@ struct DefaultMediaUploaderRelayTests { /// An HTTP client whose `performRaw` relays a canned response without validating /// status, while `perform` throws on a non-2xx — mirroring the real -/// `EditorHTTPClient`. Lets a test prove `DefaultMediaUploader` routes uploads +/// `EditorHTTPClient`. Lets a test prove `InternalMediaClient` routes uploads /// through `performRaw` (relay) rather than `perform` (throw). private struct RelayStubHTTPClient: EditorHTTPClientProtocol { let statusCode: Int @@ -791,37 +1125,33 @@ private func readAllFromStream(_ stream: InputStream) -> Data { // MARK: - Mocks -private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { +/// Records the ``MediaUpload`` it is handed, and returns a finished attachment. +private final class RecordingUploader: MediaUploader, @unchecked Sendable { private let lock = NSLock() - private var _processFileCalled = false - private var _uploadFileCalled = false - private var _lastMimeType: String? - private var _lastFilename: String? + private var _received: MediaUpload? - var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var uploadFileCalled: Bool { lock.withLock { _uploadFileCalled } } - var lastMimeType: String? { lock.withLock { _lastMimeType } } - var lastFilename: String? { lock.withLock { _lastFilename } } + var received: MediaUpload? { lock.withLock { _received } } - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - lock.withLock { - _processFileCalled = true - _lastMimeType = mimeType - } - return .original + func upload(_ upload: MediaUpload) async throws -> Data { + lock.withLock { _received = upload } + // Shaped like a real attachment: the editor's `transformAttachment` reads + // `title.raw`, so an example without it would model a body that fails in the + // editor. + return Data(#"{"id":7,"source_url":"https://example.com/photo.jpg","media_type":"image","title":{"raw":"photo"},"caption":{"raw":""}}"#.utf8) } +} - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - lock.withLock { - _uploadFileCalled = true - _lastFilename = filename - } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) +/// An uploader whose delivery fails terminally, as one would after exhausting its own +/// post-process recovery and force-deleting the orphan. +private final class ThrowingUploader: MediaUploader { + struct Failure: Error {} + + func upload(_ upload: MediaUpload) async throws -> Data { + throw Failure() } } -private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { +private final class ProcessOnlyProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -833,10 +1163,11 @@ private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendabl } } -/// A delegate that declines every file by metadata via `handlesFile`, so the -/// server must pass through without ever materializing the file or calling -/// `processFile`. -private final class DeclineByMetadataDelegate: MediaUploadDelegate, @unchecked Sendable { +/// A processor that declines every file by metadata via `handlesFile`. With no +/// uploader the server must pass through without ever materializing the file; with +/// one, delivery still happens but `processFile` must not be called. +/// `processFileCalled` pins both. +private final class DeclineByMetadataProcessor: MediaProcessor, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false @@ -850,12 +1181,23 @@ 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). +/// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the +/// point: this is the shape ``MediaProcessor``'s documentation now recommends. +private struct ValueTypeProcessor: MediaProcessor { + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + let processed = url.deletingLastPathComponent() + .appending(component: "value-\(UUID().uuidString).mp4") + try Data("transcoded".utf8).write(to: processed) + return .processed(processed, mimeType: "video/mp4", filename: "clip.mp4") + } +} + +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 { @@ -866,7 +1208,7 @@ private final class ResizingDelegate: MediaUploadDelegate, @unchecked Sendable { } } -private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendable { +private final class MockInternalMediaClient: InternalMediaClient, @unchecked Sendable { private let lock = NSLock() private var _uploadCalled = false private var _passthroughUploadCalled = false @@ -908,9 +1250,9 @@ private final class MockDefaultUploader: DefaultMediaUploader, @unchecked Sendab } } -/// A default uploader whose delete response carries its own `Content-Type`, so the +/// An internal media client whose delete response carries its own `Content-Type`, so the /// relay must override the JSON default rather than emit the header twice. -private final class ContentTypeDeleteUploader: DefaultMediaUploader, @unchecked Sendable { +private final class ContentTypeDeleteClient: InternalMediaClient, @unchecked Sendable { init() { super.init(httpClient: MockHTTPClient(), siteApiRoot: URL(string: "https://example.com/wp-json/")!) } @@ -941,3 +1283,15 @@ private extension Data { append(string.data(using: .utf8)!) } } + +/// Holds the server that owns it, closing `server -> handler -> processor -> server`. +/// Only `stop()` — which drops the listener's captured blocks — opens it. +private final class ServerRetainingProcessor: MediaProcessor, @unchecked Sendable { + var server: MediaUploadServer? + + func handlesFile(ofType mimeType: String, named filename: String) -> Bool { false } + + func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { + .original + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift b/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift new file mode 100644 index 000000000..8e699dc0b --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Services/EditorDependencyLoaderTests.swift @@ -0,0 +1,96 @@ +import Foundation +import Testing + +@testable import GutenbergKit + +/// The loader's contract with its owner: it reports back, and it never holds the owner — +/// the property that lets a released editor go while its fetch is still in flight. +@Suite("EditorDependencyLoader") +struct EditorDependencyLoaderTests: MakesTestFixtures { + static let testSiteURL = URL(string: "https://example.com")! + static let testApiRoot = URL(string: "https://example.com/wp-json")! + + @MainActor + @Test("delivers the dependencies it fetched") + func deliversTheDependencies() async throws { + // Offline mode resolves without a request, so the fetch succeeds. + let configuration = makeConfigurationBuilder().setIsOfflineModeEnabled(true).build() + let owner = LoaderOwner(service: makeService(for: configuration)) + + try await owner.waitUntilFinished() + #expect(owner.dependencies != nil) + #expect(owner.error == nil) + } + + @MainActor + @Test("delivers the error when the fetch fails") + func deliversTheError() async throws { + let session = ParkedURLSession() + defer { session.release() } + let owner = LoaderOwner(service: makeService(session: session)) + try await session.waitUntilStarted() + + session.release() // fails every parked request + try await owner.waitUntilFinished() + #expect(owner.error != nil) + #expect(owner.dependencies == nil) + } + + @MainActor + @Test("releasing its owner mid-fetch frees the owner, and leaves the fetch running") + func releasingTheOwnerMidFetchFreesIt() async throws { + let session = ParkedURLSession() + defer { session.release() } + var owner: LoaderOwner? = LoaderOwner(service: makeService(session: session)) + weak let releasedOwner = owner + try await session.waitUntilStarted() + + owner = nil + #expect(releasedOwner == nil, "the fetch should not hold its owner") + + let cancelled = await session.waitUntilCancelled(timeout: .milliseconds(250)) + #expect(!cancelled, "freeing the owner should not cancel the fetch") + } + + /// A service whose every request lands in `session`, with storage no other test shares. + private func makeService(session: ParkedURLSession) -> EditorService { + let configuration = makeConfiguration() + return EditorService( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader), + storageRoot: .randomTemporaryDirectory, + cacheRoot: .randomTemporaryDirectory + ) + } +} + +/// Stands in for `EditorViewController`: owns its loader, and records what it is told. +@MainActor +private final class LoaderOwner: EditorDependencyLoaderDelegate { + private var loader: EditorDependencyLoader? + private(set) var dependencies: EditorDependencies? + private(set) var error: (any Error)? + + init(service: EditorService) { + loader = EditorDependencyLoader(service: service, delegate: self) + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didUpdate progress: EditorProgress) {} + + func dependencyLoader(_ loader: EditorDependencyLoader, didLoad dependencies: EditorDependencies) { + self.dependencies = dependencies + } + + func dependencyLoader(_ loader: EditorDependencyLoader, didFailWith error: any Error) { + self.error = error + } + + func waitUntilFinished() async throws { + let clock = ContinuousClock() + let deadline = clock.now + .seconds(10) + while dependencies == nil && error == nil && clock.now < deadline { + try await Task.sleep(for: .milliseconds(20)) + } + try #require(dependencies != nil || error != nil, "the loader never reported back") + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift b/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift index 9635ba5ca..0355f4b15 100644 --- a/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift +++ b/ios/Tests/GutenbergKitTests/Services/EditorServiceTests.swift @@ -112,6 +112,32 @@ struct EditorServiceTests: MakesTestFixtures { #expect(!postRequests.isEmpty, "Should request /posts/123 for positive post IDs") } + // MARK: - Progress + + /// The first `prepare()` to finish clears the service's progress while the other still has + /// progress to report, so `incrementProgress` has to drop late progress rather than trap on it. + /// A shared bundle build delivers the same late progress to a service whose `prepare()` has + /// given up on it. + @Test("overlapping prepare() calls on one service don't trap on each other's progress") + func overlappingPrepareCallsDontTrap() async throws { + let client = GatedHTTPClient(respond: Self.editorServiceResponseHandler) + let service = EditorService( + configuration: makeConfiguration(), + httpClient: client, + storageRoot: .randomTemporaryDirectory, + cacheRoot: .randomTemporaryDirectory + ) + + let first = Task { try await GatedHTTPClient.$caller.withValue("first") { try await service.prepare() } } + let second = Task { try await GatedHTTPClient.$caller.withValue("second") { try await service.prepare() } } + try await waitUntil { client.isHolding("first") && client.isHolding("second") } + + client.release("first") + _ = try await first.value + client.release("second") + _ = try await second.value + } + // MARK: - Test Helpers /// URL-based response handler for EditorService.prepare() tests. @@ -138,3 +164,43 @@ struct EditorServiceTests: MakesTestFixtures { } } } + +/// Answers every request from `respond`, but holds each one until the test releases the caller +/// that made it — named by ``caller``, which a test sets around the work it starts. +private final class GatedHTTPClient: EditorHTTPClientProtocol, @unchecked Sendable { + @TaskLocal static var caller = "" + + private let respond: @Sendable (URL) -> Data + private let lock = NSLock() + private var holding: [String: Int] = [:] + private var released: Set = [] + + init(respond: @escaping @Sendable (URL) -> Data) { + self.respond = respond + } + + /// Whether a request from `caller` is being held. + func isHolding(_ caller: String) -> Bool { + lock.withLock { holding[caller, default: 0] > 0 } + } + + /// Lets every request from `caller`, held or still to come, through. + func release(_ caller: String) { + lock.withLock { _ = released.insert(caller) } + } + + func perform(_ urlRequest: URLRequest) async throws -> (Data, HTTPURLResponse) { + let caller = Self.caller + lock.withLock { holding[caller, default: 0] += 1 } + defer { lock.withLock { holding[caller, default: 0] -= 1 } } + while !lock.withLock({ released.contains(caller) }) { + try await Task.sleep(for: .milliseconds(5)) + } + let url = try #require(urlRequest.url) + return (respond(url), try #require(HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil))) + } + + func download(_ urlRequest: URLRequest) async throws -> (URL, HTTPURLResponse) { + throw URLError(.unsupportedURL) + } +} diff --git a/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift b/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift index 09efb4ca8..f9c195129 100644 --- a/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift +++ b/ios/Tests/GutenbergKitTests/Services/RESTAPIRepositoryTests.swift @@ -23,6 +23,29 @@ struct RESTAPIRepositoryTests: MakesTestFixtures { #expect(mockClient.getCallCount == 1) } + /// An editor reopened on a post must not join the request an editor since closed still has + /// in flight for it: that one can predate an edit made in between. + @Test("fetchPost goes out for every caller, even with an identical request in flight") + func fetchPostIsNeverShared() async throws { + let session = ParkedURLSession() + defer { session.release() } + let configuration = makeConfiguration(postID: 5) + let repositories = (0..<2).map { _ in + makeRepository( + configuration: configuration, + httpClient: EditorHTTPClient(urlSession: session, authHeader: configuration.authHeader) + ) + } + + let fetches = repositories.map { repository in Task { try await repository.fetchPost(id: 5) } } + try await waitUntil { session.requestCount == 2 } + + session.release() // fails both parked requests + for fetch in fetches { + await #expect(throws: URLError.self) { try await fetch.value } + } + } + // MARK: - fetchEditorSettings Tests @Test("fetchEditorSettings returns undefined when theme styles disabled") diff --git a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift index 320f953bc..2dfa7640d 100644 --- a/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/EditorAssetLibraryTests.swift @@ -873,6 +873,68 @@ struct EditorAssetLibraryTests { let failedScriptPath = bundleRoot.appending(path: "/stats.js") #expect(!FileManager.default.fileExists(at: failedScriptPath)) } + + @Test("buildBundle publishes nothing when it is cancelled mid-download") + func buildBundlePublishesNothingWhenCancelled() async throws { + let manifestJSON = """ + { + "scripts": "", + "styles": "", + "allowed_block_types": ["core/paragraph"] + } + """ + let manifest = try LocalEditorAssetManifest( + remoteManifest: RemoteEditorAssetManifest(data: Data(manifestJSON.utf8)) + ) + + let session = ParkedURLSession() + defer { session.release() } + let library = makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token")) + let destination = await library.bundleRoot(for: manifest.checksum).standardizedFileURL + + let build = Task { try await library.buildBundle(for: manifest) } + try await session.waitUntilStarted() + let abandoned = try #require(EditorAssetLibrary.inFlightBuilds.task(for: destination)) + build.cancel() + + // The cancelled download is swallowed like any failed asset; the build must + // still refuse to publish, or every later launch serves the gap. + await #expect(throws: CancellationError.self) { try await build.value } + // The caller's wait ends before the build it abandoned is cancelled, so wait for the + // build itself: checked any sooner, a build about to publish hasn't yet. + await abandoned.value + #expect(try await library.readAssetBundles().isEmpty) + } + + @Test("builds of one bundle share one build, whichever library runs them") + func buildsOfOneBundleShareOneBuild() async throws { + let manifestJSON = """ + { + "scripts": "", + "styles": "", + "allowed_block_types": ["core/paragraph"] + } + """ + let manifest = try LocalEditorAssetManifest( + remoteManifest: RemoteEditorAssetManifest(data: Data(manifestJSON.utf8)) + ) + + let session = ParkedURLSession() + defer { session.release() } + let storageRoot = URL.randomTemporaryDirectory + let libraries = [ + makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token"), storageRoot: storageRoot), + makeLibrary(httpClient: EditorHTTPClient(urlSession: session, authHeader: "Bearer test-token"), storageRoot: storageRoot), + ] + let destination = await libraries[0].bundleRoot(for: manifest.checksum).standardizedFileURL + + let builds = libraries.map { library in Task { try await library.buildBundle(for: manifest) } } + try await waitUntil { EditorAssetLibrary.inFlightBuilds.waiterCount(for: destination) == 2 } + + session.release() // fails the parked download, which a build tolerates + #expect(try await builds[0].value == builds[1].value) + #expect(session.requestCount == 1) + } } // MARK: - Progress Tracker for Tests diff --git a/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift b/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift index dd278483c..c1266c67f 100644 --- a/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/EditorURLCacheTests.swift @@ -444,4 +444,66 @@ struct EditorURLCacheAlwaysPolicyTests { let tenYearsLater = self.referenceDate.addingTimeInterval(10 * 365 * 24 * 60 * 60) #expect(try cache.hasData(for: testURL, httpMethod: .GET, currentDate: tenYearsLater) == true) } + + // MARK: - One store per site + + /// Every service builds its own cache for a site, so caches share a file — and two stores on + /// one file race their opens, the loser failing for the rest of its life. With a store per + /// cache, at least one broke in every run. + @Test("caches for one site can be opened and used at the same time") + func cachesForOneSiteCanBeUsedAtTheSameTime() async throws { + for _ in 0..<20 { + let parent = URL.randomTemporaryDirectory + let caches = [ + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + ] + // Each makes its first read at the same moment, as two services starting a fetch do. + await withTaskGroup { group in + for cache in caches { + group.addTask { _ = try? cache.response(for: testURL, httpMethod: .GET) } + } + } + for cache in caches { + try cache.store(makeResponse(), for: testURL, httpMethod: .GET) + } + } + } + + /// A cache still open on a deleted file fails every read and write. While its store was + /// still shared, so did every cache created after the delete, for as long as it was held. + @Test("a cache created after deleteAll opens a new file") + func aCacheCreatedAfterDeleteAllOpensANewFile() throws { + let parent = URL.randomTemporaryDirectory + let stale = EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always) + try stale.store(makeResponse(), for: testURL, httpMethod: .GET) + + try EditorURLCache.deleteAll(in: parent) + + // Held throughout, as an editor still open or a fetch still running holds its cache. + try withExtendedLifetime(stale) { + let cache = EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always) + #expect(try cache.response(for: testURL, httpMethod: .GET) == nil) + let response = makeResponse() + try cache.store(response, for: testURL, httpMethod: .GET) + #expect(try cache.response(for: testURL, httpMethod: .GET) == response) + } + } + + @Test("caches for one site can write at the same time") + func cachesForOneSiteCanWriteAtTheSameTime() async throws { + let parent = URL.randomTemporaryDirectory + let caches = [ + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + EditorURLCache(siteId: "site", parentDirectory: parent, cachePolicy: .always), + ] + try await withThrowingTaskGroup { group in + for index in 0..<200 { + let cache = caches[index % caches.count] + let url = testURL.appending(path: "\(index % 20)") + group.addTask { try cache.store(makeResponse(), for: url, httpMethod: .GET) } + } + try await group.waitForAll() + } + } } diff --git a/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift b/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift index ad6757900..95d741438 100644 --- a/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift +++ b/ios/Tests/GutenbergKitTests/Stores/SQLiteKVCacheTests.swift @@ -651,4 +651,114 @@ struct SQLiteKVCacheTests { let expected = try encoder.encode(meta) #expect(entry.metadata == expected) } + + // MARK: - One instance per file + + @Test("shared hands every caller the live instance for a file") + func sharedHandsOutOneInstancePerFile() { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + let store = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + + #expect(SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) === store) + #expect(SQLiteKVCache.shared(handle: "TEST", directory: directory, diskCapacity: capacity) === store) + #expect(SQLiteKVCache.shared(handle: "other", directory: directory, diskCapacity: capacity) !== store) + #expect(SQLiteKVCache.shared(handle: "test", directory: .randomTemporaryDirectory, diskCapacity: capacity) !== store) + } + + @Test("shared opens a file afresh once no one is using it") + func sharedReopensAFileNoOneIsUsing() throws { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + var store: SQLiteKVCache? = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + try store?.put(key: "durable", storageDate: referenceDate, metadata: Data(), value: Data("v")) + weak let released = store + + store = nil + #expect(released == nil, "sharing should not keep a file open") + + let reopened = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(try reopened.get(key: "durable")?.value == Data("v")) + } + + /// An instance keeps a failed open for life. While `shared` went on handing it out, every + /// later caller failed with it for as long as anything held it, though the file could by + /// then be opened. + @Test("shared opens a file afresh after an open that failed") + func sharedReopensAFileAfterAFailedOpen() throws { + let directory = URL.randomTemporaryDirectory + let capacity = Measurement(value: 1, unit: .mebibytes) + // A directory where the database file belongs fails the open. + let obstacle = directory.appending(component: "test.sqlite") + try FileManager.default.createDirectory(at: obstacle, withIntermediateDirectories: true) + let failed = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(throws: SQLiteKVCache.Error.self) { try failed.get(key: "k") } + + try FileManager.default.removeItem(at: obstacle) + try withExtendedLifetime(failed) { + let reopened = SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity) + #expect(reopened !== failed) + try reopened.put(key: "k", storageDate: referenceDate, metadata: Data(), value: Data("v")) + #expect(try reopened.get(key: "k")?.value == Data("v")) + } + } + + @Test("forgetInstances stops sharing the instances under a directory, and no others") + func forgetInstancesIsScopedToADirectory() { + let root = URL.randomTemporaryDirectory + // Beside `root` rather than under it, though its path starts with `root`'s. + let beside = root.deletingLastPathComponent().appending(path: root.lastPathComponent + "-beside") + let capacity = Measurement(value: 1, unit: .mebibytes) + let under = SQLiteKVCache.shared(handle: "test", directory: root.appending(path: "site"), diskCapacity: capacity) + let other = SQLiteKVCache.shared(handle: "test", directory: beside.appending(path: "site"), diskCapacity: capacity) + + SQLiteKVCache.forgetInstances(under: root) + + #expect(SQLiteKVCache.shared(handle: "test", directory: root.appending(path: "site"), diskCapacity: capacity) !== under) + #expect(SQLiteKVCache.shared(handle: "test", directory: beside.appending(path: "site"), diskCapacity: capacity) === other) + } + + /// `shared` hands out a fresh instance the moment the last one is released, while that + /// one's `deinit` may still hold the file to checkpoint its WAL. Without a busy timeout the + /// reopen fails on that lock every time — 200 runs out of 200 — and caches the failure. + @Test("shared reopens a file while its last instance is still closing") + func sharedReopensAFileWhileItCloses() async throws { + let capacity = Measurement(value: 1, unit: .mebibytes) + for _ in 0..<20 { + let directory = URL.randomTemporaryDirectory + let closing = ReleasableStore(SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity)) + // Enough in the WAL that checkpointing it on close takes a moment. + for index in 0..<50 { + try closing.store?.put( + key: "\(index)", + storageDate: referenceDate, + metadata: Data(), + value: Data(repeating: 1, count: 4096) + ) + } + + async let released: Void = Task.detached { closing.release() }.value + async let reopened = Task.detached { + try SQLiteKVCache.shared(handle: "test", directory: directory, diskCapacity: capacity).get(key: "0") + }.value + await released + #expect(try await reopened != nil) + } + } +} + +/// Holds the only reference to a store until `release()`, so a test can drop it from another task. +private final class ReleasableStore: @unchecked Sendable { + private let lock = NSLock() + private var held: SQLiteKVCache? + + init(_ store: SQLiteKVCache) { + held = store + } + + var store: SQLiteKVCache? { lock.withLock { held } } + + func release() { + lock.withLock { held = nil } + } } diff --git a/ios/Tests/GutenbergKitTests/TestHelpers.swift b/ios/Tests/GutenbergKitTests/TestHelpers.swift index 2b73716b9..618abf431 100644 --- a/ios/Tests/GutenbergKitTests/TestHelpers.swift +++ b/ios/Tests/GutenbergKitTests/TestHelpers.swift @@ -1,7 +1,22 @@ import Foundation +import Testing @testable import GutenbergKit +/// Polls `condition` until it holds, failing the test at the caller's line if it hasn't within +/// `timeout`. +func waitUntil( + timeout: Duration = .seconds(10), + sourceLocation: SourceLocation = #_sourceLocation, + _ condition: () -> Bool +) async throws { + let deadline = ContinuousClock.now + timeout + while !condition() && ContinuousClock.now < deadline { + try await Task.sleep(for: .milliseconds(10)) + } + try #require(condition(), "timed out waiting", sourceLocation: sourceLocation) +} + func jsonResource(named name: String) throws -> Data { let url = Bundle.module.url(forResource: name, withExtension: "json")! return try Data(contentsOf: url)