-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add UploadSourceMaterializer with policy-driven transforms #25620
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
Open
crazytonyli
wants to merge
6
commits into
trunk
Choose a base branch
from
task/media-v2-upload-materializer
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fbd1fe2
Rename MediaUploadPolicy.stripImageLocation to stripGPSLocation
crazytonyli f548d7a
Add UploadSourceMaterializer with policy-driven transforms
crazytonyli 1dd6715
Sweep orphaned staging files using this run's sweep reference date
crazytonyli 82bc166
Release the remote download temp file on failure paths
crazytonyli a25f413
Reuse an ephemeral URLSession for remote downloads
crazytonyli 18466f5
Bound and sanitize upload basenames in the allocator
crazytonyli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
Modules/Sources/WordPressMediaLibrary/Upload/MaterializerError.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import Foundation | ||
|
|
||
| enum MaterializerError: LocalizedError { | ||
| case securityScopedAccessDenied | ||
| case fileNotFound | ||
| case durationCapExceeded | ||
| case disallowedContentType | ||
| case invalidImageData | ||
| case imageEncodeFailed | ||
| case locationStripFailed | ||
| case videoExportFailed(underlyingError: Error) | ||
| case videoExportSessionUnavailable | ||
| case unknownContentType | ||
| case remoteDownloadFailed(underlyingError: Error) | ||
|
|
||
| var errorDescription: String? { | ||
| switch self { | ||
| case .securityScopedAccessDenied: return Strings.uploadErrorSecurityScopedAccess | ||
| case .fileNotFound: return Strings.uploadErrorFileNotFound | ||
| case .durationCapExceeded: return Strings.uploadErrorDurationCap | ||
| case .disallowedContentType: return Strings.uploadErrorDisallowedType | ||
| case .invalidImageData: return Strings.uploadErrorInvalidImage | ||
| case .imageEncodeFailed: return Strings.uploadErrorImageEncode | ||
| case .locationStripFailed: return Strings.uploadErrorLocationStripFailed | ||
| case .videoExportFailed(let underlyingError): | ||
| return String.localizedStringWithFormat( | ||
| Strings.uploadErrorVideoExport, | ||
| underlyingError.localizedDescription | ||
| ) | ||
| case .videoExportSessionUnavailable: return Strings.uploadErrorVideoExportNoExporter | ||
| case .unknownContentType: return Strings.uploadErrorUnknownContentType | ||
| case .remoteDownloadFailed(let underlyingError): | ||
| return String.localizedStringWithFormat( | ||
| Strings.materializerErrorRemoteDownloadFailed, | ||
| underlyingError.localizedDescription | ||
| ) | ||
| } | ||
| } | ||
| } |
101 changes: 101 additions & 0 deletions
101
Modules/Sources/WordPressMediaLibrary/Upload/RemoteDownloader.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import Foundation | ||
|
|
||
| /// Downloader for `.remoteURL` materialization. Owns a private ephemeral | ||
| /// `URLSession` and is reused across downloads (the materializer holds one | ||
| /// instance), so downloaded media bytes, cookies, and credentials never | ||
| /// touch the app-wide `URLSession.shared` cache and jar. | ||
| /// | ||
| /// Built on the async `URLSession.download(from:delegate:)`, so the runtime | ||
| /// owns the continuation, cancellation, temp-file delivery, and error | ||
| /// propagation. The only thing it can't give us is byte-level progress | ||
| /// (the async convenience methods suppress `didWriteData`), so we observe | ||
| /// the task's own `Progress` via KVO from `didCreateTask`, the workaround | ||
| /// Apple's URL Loading System team recommends for this case: | ||
| /// https://developer.apple.com/forums/thread/723015 | ||
| final class RemoteDownloader: Sendable { | ||
|
|
||
| private let session: URLSession | ||
|
|
||
| init() { | ||
| let configuration = URLSessionConfiguration.ephemeral | ||
| // Media downloads are large and single-use, so keep them out of the | ||
| // session's in-memory URL cache. | ||
| configuration.urlCache = nil | ||
| self.session = URLSession(configuration: configuration) | ||
| } | ||
|
|
||
| /// Downloads `url` into `parentDir` and returns the local file URL. Reports | ||
| /// byte-level progress on `progress` (scaled to its existing | ||
| /// `totalUnitCount`). Cooperative task cancellation cancels the request and | ||
| /// surfaces as `CancellationError`. | ||
| func download(from url: URL, into parentDir: URL, progress: Progress) async throws -> URL { | ||
| let delegate = ProgressForwardingDelegate(progress: progress) | ||
| let location: URL | ||
| let response: URLResponse | ||
| do { | ||
| (location, response) = try await session.download(from: url, delegate: delegate) | ||
| } catch { | ||
| if error is CancellationError || (error as? URLError)?.code == .cancelled { | ||
| throw CancellationError() | ||
| } | ||
| throw MaterializerError.remoteDownloadFailed(underlyingError: error) | ||
| } | ||
|
|
||
| // The async `download` hands back a temp file it does not reap for us, | ||
| // so we own it now. Delete it on every path that doesn't move it into | ||
| // parentDir (the status check below throws before the move), otherwise a | ||
| // failed download strands bytes in the system temp directory. On success | ||
| // the move renames it away first, leaving this a no-op. | ||
| defer { try? FileManager.default.removeItem(at: location) } | ||
|
|
||
| // HTTP status check: a 404/500 response body looks like a successful | ||
| // download to URLSession (it still hands back a valid temp file). For | ||
| // Stock Photos the image-byte validator catches HTML error bodies | ||
| // later, but remote GIFs are a raw passthrough, so without this we'd | ||
| // happily upload an HTML 404 response as 'image/gif'. | ||
| if let httpResponse = response as? HTTPURLResponse, | ||
| !(200..<300).contains(httpResponse.statusCode) | ||
| { | ||
| let statusError = NSError( | ||
| domain: "RemoteDownloader", | ||
| code: httpResponse.statusCode, | ||
| userInfo: [NSLocalizedDescriptionKey: "HTTP \(httpResponse.statusCode) response"] | ||
| ) | ||
| throw MaterializerError.remoteDownloadFailed(underlyingError: statusError) | ||
| } | ||
|
|
||
| // Move out of the system temp location (reaped after this call) into our | ||
| // owned parentDir, reusing the system-generated unique name. | ||
| let dest = parentDir.appendingPathComponent(location.lastPathComponent) | ||
| do { | ||
| try FileManager.default.moveItem(at: location, to: dest) | ||
| } catch { | ||
| throw MaterializerError.remoteDownloadFailed(underlyingError: error) | ||
| } | ||
| return dest | ||
| } | ||
| } | ||
|
|
||
| /// Forwards the download task's `Progress` to the caller-supplied `progress`. | ||
| /// `didCreateTask` fires for the async download API (unlike `didWriteData`), | ||
| /// so it's the hook for installing the KVO observation. | ||
| private final class ProgressForwardingDelegate: NSObject, URLSessionTaskDelegate { | ||
| private let progress: Progress | ||
| private var observation: NSKeyValueObservation? | ||
|
|
||
| init(progress: Progress) { | ||
| self.progress = progress | ||
| super.init() | ||
| } | ||
|
|
||
| func urlSession(_ session: URLSession, didCreateTask task: URLSessionTask) { | ||
| observation = task.progress.observe(\.fractionCompleted) { [progress] taskProgress, _ in | ||
| // `fractionCompleted` stays 0 when the server sends no Content-Length | ||
| // (totalUnitCount is -1), leaving the row indeterminate until the | ||
| // upload phase drives it to completion. | ||
| progress.completedUnitCount = Int64( | ||
| (taskProgress.fractionCompleted * Double(progress.totalUnitCount)).rounded() | ||
| ) | ||
| } | ||
| } | ||
| } | ||
105 changes: 105 additions & 0 deletions
105
Modules/Sources/WordPressMediaLibrary/Upload/UploadFilenameAllocator.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| import Foundation | ||
| import os | ||
|
|
||
| /// Generates the temp-file basenames used for media uploads, and with them the | ||
| /// filenames WordPress ends up storing. | ||
| /// | ||
| /// The uploaded media's filename is the basename of the file sent to the | ||
| /// server: `MediaCreateParams` carries no explicit filename, so the upload | ||
| /// layer (wordpress-rs) falls back to the last path component of the file it | ||
| /// uploads. Naming each temp file `<stem>.<ext>` from the source's own name is | ||
| /// therefore the whole of filename preservation (and, since the title is left | ||
| /// unset, drives the server-derived attachment title too). | ||
| /// | ||
| /// Holds the basenames already handed out so repeated names within one uploader | ||
| /// session get ` (2)`, ` (3)` suffixes instead of overwriting each other. | ||
| final class UploadFilenameAllocator: Sendable { | ||
| private let usedBasenames = OSAllocatedUnfairLock<Set<String>>(initialState: []) | ||
|
|
||
| /// Upper bound on the name stem, in UTF-8 bytes. Kept well under APFS | ||
| /// `NAME_MAX` (255 bytes) so the `.<ext>` and any ` (n)` dedup suffix still | ||
| /// fit without per-candidate length arithmetic. A source name longer than | ||
| /// this would otherwise make the temp-file write fail with `ENAMETOOLONG`. | ||
| private static let maxStemBytes = 200 | ||
|
|
||
| /// The sanitized `preferred` name, or `<fallbackPrefix>-<timestamp>` when | ||
| /// the source has no usable name. Carries no extension and is not yet | ||
| /// deduplicated; pass the result to `basename`. | ||
| func stem(preferred: String?, fallbackPrefix: String, date: Date) -> String { | ||
| if let cleaned = preferred.map(sanitize), !cleaned.isEmpty { | ||
| return cleaned | ||
| } | ||
| return "\(fallbackPrefix)-\(timestampedName(date: date))" | ||
| } | ||
|
|
||
| /// A unique `<stem>.<ext>` basename, safe to hand to | ||
| /// `URL.appendingPathComponent`. The stem is sanitized and length-capped | ||
| /// here (not only in `stem`), so a caller that passes a raw source name | ||
| /// (`.remoteURL`, `.imagePlayground`) can't smuggle a path separator or an | ||
| /// over-long name onto disk. Names already handed out for the lifetime of | ||
| /// this allocator are tracked, so a batch with two same-named files becomes | ||
| /// `name.jpg`, `name (2).jpg`, and so on. | ||
| func basename(stem: String, ext: String) -> String { | ||
| let fitted = truncated(sanitize(stem), toUTF8ByteCount: Self.maxStemBytes) | ||
| let safeStem = fitted.isEmpty ? "file" : fitted | ||
| return usedBasenames.withLock { used in | ||
| let first = "\(safeStem).\(ext)" | ||
| if used.insert(first).inserted { | ||
| return first | ||
| } | ||
| var n = 2 | ||
| while true { | ||
| let candidate = "\(safeStem) (\(n)).\(ext)" | ||
| if used.insert(candidate).inserted { | ||
| return candidate | ||
| } | ||
| n += 1 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Formatter for `timestampedName`. `DateFormatter` construction is | ||
| /// expensive, and the class is documented thread-safe, so one shared | ||
| /// instance serves every allocation. | ||
| private static let timestampFormatter: DateFormatter = { | ||
| let formatter = DateFormatter() | ||
| formatter.locale = Locale(identifier: "en_US_POSIX") | ||
| formatter.dateFormat = "yyyy-MM-dd HH-mm-ss" | ||
| return formatter | ||
| }() | ||
|
|
||
| /// Filesystem-safe `yyyy-MM-dd HH-mm-ss` timestamp (fixed POSIX locale) | ||
| /// used to build fallback stems when a source has no name of its own. | ||
| private func timestampedName(date: Date) -> String { | ||
| Self.timestampFormatter.string(from: date) | ||
| } | ||
|
|
||
| /// Replaces path separators with `_` and strips NUL so the result is a | ||
| /// single path component that `appendingPathComponent` can't use to escape | ||
| /// its parent directory (e.g. `../escaped` becomes `.._escaped`). May be | ||
| /// empty; `stem` treats that as "no usable name" and `basename` substitutes | ||
| /// a stub. | ||
| private func sanitize(_ name: String) -> String { | ||
| name | ||
| .replacingOccurrences(of: "/", with: "_") | ||
| .replacingOccurrences(of: "\u{0}", with: "") | ||
| } | ||
|
|
||
| /// Truncates `name` to at most `maxBytes` UTF-8 bytes without splitting a | ||
| /// grapheme, so the temp-file write can't fail with `ENAMETOOLONG` and the | ||
| /// result is never cut mid-character. | ||
| private func truncated(_ name: String, toUTF8ByteCount maxBytes: Int) -> String { | ||
| guard name.utf8.count > maxBytes else { return name } | ||
| var result = "" | ||
| var count = 0 | ||
| for character in name { | ||
| let size = String(character).utf8.count | ||
| if count + size > maxBytes { | ||
| break | ||
| } | ||
| result.append(character) | ||
| count += size | ||
| } | ||
| return result | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Potential fun bug – some servers will return an HTML
200response for an image URL (for instance, to avoid hot linking). It may be worth reading the magic bytes of the top of the file to validate that it's actually an image?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.
In practice, this downloading code only runs on
UploadSource.RemoteURL, which is produced by "Stock Photos" from https://www.pexels.com/ (endpoint: https://public-api.wordpress.com/rest/v1/meta/external-media/pexels?search=cat). It's probably not likely that they'd return a 200 html page when the url is xxx.jpeg?If they do, the image processing code is likely to catch the issue because the content can be decoded as an image. However, we do have code that skip image processing (like for gif). Still, I think we can trust the "Stock Photos" provider to return a sensible html response if an image url does not return the requested image?