-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add MediaUploader, for a host that owns the whole upload #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,14 +123,32 @@ class GutenbergView : FrameLayout { | |
| */ | ||
| var mediaUploadDelegate: MediaUploadDelegate? = null | ||
| set(value) { | ||
| check(!hasStartedLoading) { | ||
| "mediaUploadDelegate must be set before the editor loads (e.g. right " + | ||
| "after construction). It is captured when the page begins loading; " + | ||
| "setting it afterward has no effect." | ||
| } | ||
| check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploadDelegate") } | ||
| field = value | ||
| } | ||
|
|
||
| /** | ||
| * Takes over media upload on the host's own stack (background service, offline | ||
| * queue, resumable transport). Setting it makes the host own every upload and its | ||
| * whole lifecycle; GutenbergKit stays out of the network entirely for media. | ||
| * | ||
| * Same lifecycle rules as [mediaUploadDelegate]: set it before the editor loads, | ||
| * and this view owns it for its lifetime — so you needn't retain it yourself, just | ||
| * don't strongly retain this [GutenbergView] from your uploader. | ||
| * | ||
| * Takes precedence over the deprecated [MediaUploadDelegate.uploadFile]: with an | ||
| * uploader set, that hook is never called. | ||
| */ | ||
| var mediaUploader: MediaUploader? = null | ||
| set(value) { | ||
| check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } | ||
| field = value | ||
| } | ||
|
|
||
| private fun lateMediaAssignmentMessage(name: String) = | ||
| "$name must be set before the editor loads (e.g. right after construction). " + | ||
| "It is captured when the page begins loading; setting it afterward has no effect." | ||
|
|
||
| @Volatile private var uploadServer: MediaUploadServer? = null | ||
|
|
||
| /** | ||
|
|
@@ -676,10 +694,10 @@ class GutenbergView : FrameLayout { | |
| } | ||
|
|
||
| private fun startUploadServer() { | ||
| // No delegate means nothing wants to customize uploads, so there's no reason | ||
| // to route them through the native server — leave it down and let uploads | ||
| // fall to the default WebView path. (Matches iOS.) | ||
| if (mediaUploadDelegate == null) return | ||
| // Nothing to route through the native server unless the host provided a | ||
| // delegate or an uploader — leave it down and let uploads fall to the default | ||
| // WebView path. (Matches iOS.) | ||
| if (mediaUploadDelegate == null && mediaUploader == null) return | ||
|
|
||
| // The native upload server relays through InternalMediaClient, which needs a | ||
| // site root and an auth header (every host provides one — the editor injects | ||
|
|
@@ -715,6 +733,7 @@ class GutenbergView : FrameLayout { | |
| uploadServer = MediaUploadServer( | ||
| uploadDelegate = mediaUploadDelegate, | ||
| internalClient = internalClient, | ||
| uploader = mediaUploader, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding from Claude: With #632 traps the missing-credentials case because falling back "would silently drop" the uploader. Should these paths follow suit for an uploader host? A |
||
| cacheDir = context.cacheDir, | ||
| scope = coroutineScope | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -106,13 +108,102 @@ interface MediaUploadDelegate { | |
| * Upload a processed file to the remote WordPress site. | ||
| * | ||
| * Return the raw WordPress response (status code + body), which GutenbergKit | ||
| * relays to the editor unchanged, or null to use the default uploader. A host | ||
| * that uploads to WordPress should return the exact response it received so | ||
| * relays to the editor unchanged, or null to use the internal media client. A | ||
| * host that uploads to WordPress should return the exact response it received so | ||
| * the editor sees a complete attachment object. | ||
| * | ||
| * Returning a raw response splits one upload's HTTP across two owners: you | ||
| * perform the POST, but the editor drives the `post-process` retries and orphan | ||
| * cleanup behind it, through the WebView rather than your stack. It also receives | ||
| * no form fields, so an attachment uploaded this way lands unattached to its post. | ||
| * Implement [MediaUploader] instead — it owns the upload end-to-end and receives a | ||
| * [MediaUpload] carrying the fields. | ||
| */ | ||
| // No ReplaceWith: it takes a replacement *expression* the IDE substitutes for the | ||
| // call, and there is none that means "implement a different interface" — the | ||
| // quick-fix would drop the arguments and leave a type name where a | ||
| // MediaUploadResponse? was expected. The message carries the guidance instead. | ||
| @Deprecated( | ||
| "Implement MediaUploader instead — it owns the upload's retries and receives the editor's form fields." | ||
| ) | ||
| suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null | ||
| } | ||
|
|
||
| /** | ||
| * One of the editor's non-file form fields, as sent with a media upload. | ||
| * | ||
| * A named type rather than a pair so the field's meaning is legible at every call | ||
| * site, and so the type can gain members without a source break for every host. | ||
| * | ||
| * @property name The field name, e.g. `post`. Not unique — a `field[]` array repeats it. | ||
| * @property value The field's value, decoded as UTF-8. | ||
| */ | ||
| data class MediaUploadField(val name: String, val value: String) | ||
|
|
||
| /** | ||
| * Everything a [MediaUploader] needs to reproduce a native upload: the file to send, | ||
| * its metadata, the editor's non-file form fields, and the request's query. | ||
| * | ||
| * @property file The file to upload — already processed, if a [MediaUploadDelegate] ran. | ||
| * @property mimeType The file's MIME type. | ||
| * @property filename The file's name. | ||
| * @property fields The editor's non-file form fields, in order, each decoded as UTF-8 — | ||
| * most importantly `post`, the parent post's ID, without which the attachment is | ||
| * created unattached. A list, not a map, so repeated field names (e.g. a `field[]` | ||
| * array) survive verbatim. Send each as a form part on your `POST /wp/v2/media`, in | ||
| * the given order. | ||
| * @property query The request's query string (leading `?`, e.g. `?_embed=wp:featuredmedia`), | ||
| * or empty. Carry it on your request so the editor gets the response it expects. | ||
| */ | ||
| data class MediaUpload( | ||
| val file: File, | ||
| val mimeType: String, | ||
| val filename: String, | ||
| val fields: List<MediaUploadField>, | ||
| val query: String | ||
| ) | ||
|
|
||
| /** | ||
| * Takes over *performing* a media upload — on the host's own stack: its own | ||
| * networking (say, to log every request), a background service, an offline queue, a | ||
| * resumable transport, its own retry policy. | ||
| * | ||
| * This is a choice of *who executes the requests*, not where they go: an uploader and | ||
| * GutenbergKit's internal media client both target the same configured site. Setting | ||
| * [GutenbergView.mediaUploader] makes the host own that upload end-to-end — the | ||
| * request, its own retries, and its recovery and cleanup — with GutenbergKit out of | ||
| * the network entirely. Because the host does the retries itself, there's no raw | ||
| * response left for the editor to retry behind it. | ||
| */ | ||
| interface MediaUploader { | ||
| /** | ||
| * Upload a (possibly processed) file and return the finished WordPress attachment | ||
| * JSON the editor inserts — the same object a direct `POST /wp/v2/media` returns. | ||
| * Return only once the upload is genuinely done, or throw on terminal failure: a | ||
| * returned value is taken as a completed attachment, and there is no GutenbergKit | ||
| * recovery behind you. | ||
| * | ||
| * The [MediaUpload] carries the file plus the editor's form fields (e.g. `post`) | ||
| * and query — send them all so the created attachment matches a native upload | ||
| * rather than landing as an unattached orphan. | ||
| * | ||
| * That recovery is yours to run. When `POST /wp/v2/media` fatals in server-side | ||
| * post-processing it returns a 5xx carrying the attachment's ID in | ||
| * `x-wp-upload-attachment-id` — the attachment exists but is unfinished. Don't | ||
| * re-upload; drive `POST /wp/v2/media/<id>/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/<id>?force=true`) | ||
| * before you throw, or it stays on the site — neither GutenbergKit nor the editor | ||
| * cleans up behind you. | ||
| */ | ||
| suspend fun upload(upload: MediaUpload): ByteArray | ||
| } | ||
|
|
||
| /** | ||
| * A local HTTP server that receives file uploads from the WebView and routes | ||
| * them through the native media processing pipeline. | ||
|
|
@@ -128,6 +219,7 @@ interface MediaUploadDelegate { | |
| internal class MediaUploadServer( | ||
| private val uploadDelegate: MediaUploadDelegate?, | ||
| private val internalClient: InternalMediaClient?, | ||
| private val uploader: MediaUploader? = null, | ||
| cacheDir: File? = null, | ||
| scope: CoroutineScope? = null, | ||
| ioDispatcher: CoroutineDispatcher = Dispatchers.IO | ||
|
|
@@ -264,9 +356,9 @@ internal class MediaUploadServer( | |
| * browser blocks it at preflight. Relaying it here lets the cleanup run. | ||
| */ | ||
| private suspend fun handleDelete(attachmentId: String, query: String): HttpResponse { | ||
| val uploader = internalClient ?: return errorResponse(500, "No internal media client configured") | ||
| val client = internalClient ?: return errorResponse(500, "No internal media client configured") | ||
| return try { | ||
| relayResponse(uploader.deleteMedia(attachmentId, query)) | ||
| relayResponse(client.deleteMedia(attachmentId, query)) | ||
| } catch (e: IOException) { | ||
| Log.e(TAG, "Media deletion failed", e) | ||
| errorResponse(500, e.message ?: "Deletion failed") | ||
|
|
@@ -290,14 +382,21 @@ internal class MediaUploadServer( | |
| // like this. If not, forward the original upload to WordPress directly, | ||
| // skipping a full temp-file copy of a file the delegate won't process or | ||
| // upload (e.g. a video handed to an image-only delegate). | ||
| if (uploadDelegate?.handlesFile(mimeType, filename) != true) { | ||
| // An uploader takes over delivery for *every* file, so with one set there is no | ||
| // passthrough to fall to and the gate can't decline the upload outright. It | ||
| // still decides whether processFile runs, though — a declined file is handed to | ||
| // the uploader unprocessed rather than to a delegate that said it won't touch it | ||
| // — so the answer is carried into processAndUpload rather than short-circuited | ||
| // away here. Asked exactly once per upload, matching iOS. | ||
| val delegateWantsFile = uploadDelegate?.handlesFile(mimeType, filename) == true | ||
| if (uploader == null && !delegateWantsFile) { | ||
| return passthroughResponse(request, query) | ||
| } | ||
|
|
||
| val tempFile = writePartToTempFile(filePart) | ||
| ?: return errorResponse(500, "Failed to save file") | ||
|
|
||
| return processAndRespond(request, tempFile, filePart, extraParts, query) | ||
| return processAndRespond(request, tempFile, filePart, extraParts, query, delegateWantsFile) | ||
| } | ||
|
|
||
| @Suppress("TooGenericExceptionCaught") | ||
|
|
@@ -381,11 +480,12 @@ internal class MediaUploadServer( | |
| @Suppress("TooGenericExceptionCaught") | ||
| private suspend fun processAndRespond( | ||
| request: HttpRequest, tempFile: File, filePart: MultipartPart, | ||
| extraParts: List<MultipartPart>, query: String | ||
| extraParts: List<MultipartPart>, query: String, delegateWantsFile: Boolean | ||
| ): HttpResponse { | ||
| try { | ||
| val uploadResult = processAndUpload( | ||
| tempFile, filePart.contentType, filePart.filename ?: "upload", extraParts, query | ||
| tempFile, filePart.contentType, filePart.filename ?: "upload", | ||
| extraParts, query, delegateWantsFile | ||
| ) | ||
| val response = when (uploadResult) { | ||
| is UploadResult.Uploaded -> { | ||
|
|
@@ -430,18 +530,29 @@ internal class MediaUploadServer( | |
| private suspend fun performPassthroughUpload(request: HttpRequest, query: String): MediaUploadResponse { | ||
| val body = request.body | ||
| val contentType = request.header("Content-Type") | ||
| val uploader = internalClient | ||
| if (body == null || contentType == null || uploader == null) { | ||
| throw MediaUploadException("Passthrough upload requires a request body, Content-Type, and internal media client") | ||
| val client = internalClient | ||
| if (body == null || contentType == null || client == null) { | ||
| throw MediaUploadException( | ||
| "Passthrough upload requires a request body, Content-Type, and internal media client" | ||
| ) | ||
| } | ||
| return uploader.passthroughUpload(body, contentType, query) | ||
| return client.passthroughUpload(body, contentType, query) | ||
| } | ||
|
|
||
| private suspend fun processAndUpload( | ||
| file: File, mimeType: String, filename: String, | ||
| extraParts: List<MultipartPart>, query: String | ||
| extraParts: List<MultipartPart>, query: String, delegateWantsFile: Boolean | ||
| ): UploadResult { | ||
| val processed = uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original | ||
| // Process (resize, transcode, etc.) — but only for a file the delegate's | ||
| // metadata gate accepted. handlesFile returning false is the delegate saying it | ||
| // won't touch a file like this, so handing it one anyway would break the | ||
| // contract the gate documents. With an uploader set the file still gets | ||
| // delivered; it just skips processing on its way there. | ||
| val processed = if (delegateWantsFile) { | ||
| uploadDelegate?.processFile(file, mimeType, filename) ?: ProcessedProxyFile.Original | ||
| } else { | ||
| ProcessedProxyFile.Original | ||
| } | ||
|
|
||
| // Resolve the file to upload and its metadata. Processed uses the | ||
| // delegate's values verbatim, so a format change is reported to WordPress. | ||
|
|
@@ -462,7 +573,30 @@ internal class MediaUploadServer( | |
| } | ||
|
|
||
| try { | ||
| // If the delegate provided its own upload, use that. | ||
| // 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))) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finding from Claude: If a host's own Rethrowing only when this coroutine is actually cancelled would fix it, replacing the catch body at } catch (e: kotlin.coroutines.cancellation.CancellationException) {
currentCoroutineContext().ensureActive()
Log.e(TAG, "Upload failed", e)
return errorResponse(500, e.message ?: "Upload failed")
}Repro: local, uncommitted test on #629's head (
|
||
| } | ||
|
|
||
| // The deprecated delegate path: the host performs the POST but returns the | ||
| // raw response, leaving the editor to drive post-process recovery behind it. | ||
| @Suppress("DEPRECATION") | ||
| uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { | ||
| return UploadResult.Uploaded(it) | ||
| } | ||
|
|
@@ -485,6 +619,15 @@ internal class MediaUploadServer( | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * The editor's non-file form parts as ordered, UTF-8-decoded fields. | ||
| * | ||
| * A list rather than a map so repeated names (e.g. a `field[]` array) survive | ||
| * verbatim, in the order the editor sent them. | ||
| */ | ||
| private fun formFields(parts: List<MultipartPart>): List<MediaUploadField> = | ||
| parts.map { MediaUploadField(it.name, String(it.body.readBytes(), Charsets.UTF_8)) } | ||
|
|
||
| // MARK: - Response Building | ||
|
|
||
| private fun errorResponse(status: Int, message: String): HttpResponse { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding from Claude:
Nit: the
hasStartedLoadingdocs (:155-157here,EditorViewController.swift:107-109) still name only the delegate, though a latemediaUploaderwrite now throws (traps on iOS) too.