Skip to content
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

Move uploading media to WP Media Library to MediaImportService #20322

Merged
merged 12 commits into from
Mar 24, 2023

Conversation

crazytonyli
Copy link
Contributor

There is the usual accessing managed object in a wrong thread issue (I'll highlight a few in the PR comments) in the implementation of uploading a media asset to the WP Media Library.

This PR rewrites (large chunk of the original code is still reused) the uploading logic to make the Core Data operations safer, by adopting the new writer API.

To help with the review, I'll quickly described what happens during uploading a media asset to WP media library. The entry point is the MediaCoordinator.addMedia function, which can be called during editing a post or simply uploading an photo (on device or from the free online photo library) on the "Media" screen. This function first imports the photo into the app's sandbox, using MediaImportService. Then it generates a thumbnail using MediaThumbnailService. At the same time, it starts to upload the imported media to the WP media library, using MediaService.

The refactor does not change the above logic, but moves some code around, makes the Core Data operations safer, and changed a few call sites to adopt to the new API.

Test instructions

Verify all the places that can upload a photo to WP media library. I've tested uploading a new site icon, uploading a photo to the Media Library on "Media" screen, uploading a photo to a post in the post editor.

Regression Notes

  1. Potential unintended areas of impact
    None

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    What described in the "Test instructions"

  3. What automated tests I added (or what prevented me from doing so)
    None.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@crazytonyli crazytonyli added the Core Data Issues related to Core Data label Mar 15, 2023
@crazytonyli crazytonyli added this to the 22.0 milestone Mar 15, 2023
@crazytonyli crazytonyli requested a review from mokagio March 15, 2023 02:23
@crazytonyli crazytonyli self-assigned this Mar 15, 2023
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

self.managedObjectContext.perform {
self.configureMedia(media, withExport: export)
ContextManager.sharedInstance().save(self.managedObjectContext, completion: {
onCompletion(media)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue: media is changed in the managedObjectContext queue, but there is no guarantee it's bound to the context object.

Copy link
Contributor Author

@crazytonyli crazytonyli Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue: media is send to the onCompletion closure on the main thread, but there is no guarantee that media is bound to the main context.

[self.managedObjectContext performBlock:^{
// Setup completion handlers
void(^completionWithMedia)(Media *) = ^(Media *media) {
media.remoteStatus = MediaRemoteStatusLocal;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue: media is modified in the main thread since this completionWithMedia is called from the main thread, but there is no guarantee media is bound to the main context.


#pragma mark - Creating media

- (Media *)createMediaWith:(id<ExportableAsset>)exportable
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue: this functions returns a Media instance with no guarantee what context object this instance is bound to.

*progress = createProgress;
}
return media;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is moved to MediaImportService and duplicated as two different variants: one that returns a Media instance which can only be called from the main thread, and the other that can be called from any thread.

/// This functions returns a `Media` instance. To ensure the returned `Media` instance continue to be a valid
/// instance, it can't be bound to a background context which are all temporary context. The only long living
/// context is the main context. And the safe way to create and return an object bound to the main context is
/// doing it from the main thread, which is why this function must be called from the main thread.
Copy link
Contributor Author

@crazytonyli crazytonyli Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API doc explains why there are two createMedia functions in this class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informative but not verbose 👍

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 15, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr20322-0191a21
Version22.0
Bundle IDcom.jetpack.alpha
Commit0191a21
App Center Buildjetpack-installable-builds #4337
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 15, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr20322-0191a21
Version22.0
Bundle IDorg.wordpress.alpha
Commit0191a21
App Center BuildWPiOS - One-Offs #5311
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio
Copy link
Contributor

mokagio commented Mar 16, 2023

Just a note to say that I saw this PR, but haven't gotten an open chunk of time long enough to look at it yet.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my comments are nitpicks.

I'm running short on time ATM, so I haven't tested this locally.

My only concern is that with test coverage. This is a refactor, so it should mostly rely on previous tests to ensure that the desired behavior hasn't been affected. But, how comprehensive and trustworthy are those tests?

For example, I modified the code randomly like this and got no test failures 😢

--- a/WordPress/Classes/Services/MediaImportService.swift
+++ b/WordPress/Classes/Services/MediaImportService.swift
@@ -136,29 +136,30 @@ class MediaImportService: NSObject {
             try context.obtainPermanentIDs(for: [media])
             return media.objectID
         }, completion: { (result: Result<NSManagedObjectID, Error>) in
-            let transformed = result.flatMap { mediaObjectID in
-                Result {
-                    (
-                        try self.coreDataStack.mainContext.existingObject(with: mediaObjectID) as! Media,
-                        try self.coreDataStack.mainContext.existingObject(with: blog.objectID) as! Blog
-                    )
-                }
-            }
-            switch transformed {
-            case let .success((media, blog)):
-                let progress = self.import(exportable, to: media, blog: blog, thumbnailCallback: thumbnailCallback) {
-                    switch $0 {
-                    case let .success(media):
-                        completion(media, nil)
-                    case let .failure(error):
-                        completion(nil, error)
-                    }
-                }
-                createProgress.addChild(progress, withPendingUnitCount: 1)
-                receiveUpdate?(media)
-            case let .failure(error):
-                completion(nil, error)
-            }
+//            let transformed = result.flatMap { mediaObjectID in
+//                Result {
+//                    (
+//                        try self.coreDataStack.mainContext.existingObject(with: mediaObjectID) as! Media,
+//                        try self.coreDataStack.mainContext.existingObject(with: blog.objectID) as! Blog
+//                    )
+//                }
+//            }
+//            switch transformed {
+//            case let .success((media, blog)):
+//                let progress = self.import(exportable, to: media, blog: blog, thumbnailCallback: thumbnailCallback) {
+//                    switch $0 {
+//                    case let .success(media):
+//                        completion(media, nil)
+//                    case let .failure(error):
+//                        completion(nil, error)
+//                    }
+//                }
+//                createProgress.addChild(progress, withPendingUnitCount: 1)
+//                receiveUpdate?(media)
+//            case let .failure(error):
+//                completion(nil, error)
+//            }
+            completion(nil, nil)
         }, on: .main)
         return createProgress
     }

WordPress/Classes/Services/MediaCoordinator.swift Outdated Show resolved Hide resolved
WordPress/Classes/Services/MediaCoordinator.swift Outdated Show resolved Hide resolved
WordPress/Classes/Services/MediaCoordinator.swift Outdated Show resolved Hide resolved
Comment on lines +217 to +219
private func handleMediaImportResult(coordinator: MediaProgressCoordinator, totalProgress: Progress, analyticsInfo: MediaAnalyticsInfo?, media: Media?, error: Error?) -> Void {
if let error = error as NSError? {
if let media = media {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as I saw that media, error pair in the code about my mind screamed: Use Result! But here I see that it wouldn't work properly in we can have both an error and a media 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I now noticed there are a couple places I should have passed the Media instance along with the error, which are updated in e8b02b8.

)
progress.addChild(exportProgress, withPendingUnitCount: 1)
}
return progress
}

func makeExporter(for exportable: ExportableAsset) -> MediaExporter? {
private func makeExporter(for exportable: ExportableAsset, allowableFileExtensions: Set<String>) -> MediaExporter? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using "allowed"?

Update: Oh, I see that name was already used. 🤔 No point in using a different one, then, it would end up being more confusing than useful, even if we were to agree that "allowed" is in itself clearer than "allowable".

@@ -97,6 +296,30 @@ open class MediaImportService: LocalCoreDataService {
}
}

/// Generate a thumbnail image for the Media asset so that consumers of the absoluteThumbnailLocalURL property
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Generate a thumbnail image for the Media asset so that consumers of the absoluteThumbnailLocalURL property
/// Generate a thumbnail image for the Media asset so that consumers of the `absoluteThumbnailLocalURL` property

@@ -97,6 +296,30 @@ open class MediaImportService: LocalCoreDataService {
}
}

/// Generate a thumbnail image for the Media asset so that consumers of the absoluteThumbnailLocalURL property
/// will have an image ready to load, without using the async methods provided via MediaThumbnailService.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// will have an image ready to load, without using the async methods provided via MediaThumbnailService.
/// will have an image ready to load, without using the async methods provided via `MediaThumbnailService`.

Comment on lines 304 to 307
/// Note: Ideally we wouldn't need this at all, but the synchronous usage of absoluteThumbnailLocalURL across the code-base
/// to load a thumbnail image is relied on quite heavily. In the future, transitioning to asynchronous thumbnail loading
/// via the new thumbnail service methods is much preferred, but would indeed take a good bit of refactoring away from
/// using absoluteThumbnailLocalURL.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Note: Ideally we wouldn't need this at all, but the synchronous usage of absoluteThumbnailLocalURL across the code-base
/// to load a thumbnail image is relied on quite heavily. In the future, transitioning to asynchronous thumbnail loading
/// via the new thumbnail service methods is much preferred, but would indeed take a good bit of refactoring away from
/// using absoluteThumbnailLocalURL.
/// Note: Ideally we wouldn't need this at all, but the synchronous usage of `absoluteThumbnailLocalURL` across the code-base
/// to load a thumbnail image is relied on quite heavily. In the future, transitioning to asynchronous thumbnail loading
/// via the new thumbnail service methods is much preferred, but would indeed take a good bit of refactoring away from
/// using `absoluteThumbnailLocalURL`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, 👍 looking forward to that.

Comment on lines +356 to 361
private var exporterVideoOptions: MediaVideoExporter.Options {
var options = MediaVideoExporter.Options()
options.stripsGeoLocationIfNeeded = MediaSettings().removeLocationSetting
options.exportPreset = MediaSettings().maxVideoSizeSetting.videoPreset
return options
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick. This doesn't access anything from self. Could it become a constant so we don't need to compute it on every access?

@mokagio
Copy link
Contributor

mokagio commented Mar 20, 2023

I'll move the milestone to 22.1 because I'm about to start the 22.0 code freeze and this hasn't been approved yet. Given this is a refactor/rewrite, we should be able to afford not shipping to beta in this release cycle. Feel free to revert if necessary.

@mokagio mokagio modified the milestones: 22.0, 22.1 Mar 20, 2023
@crazytonyli
Copy link
Contributor Author

My only concern is that with test coverage. This is a refactor, so it should mostly rely on previous tests to ensure that the desired behavior hasn't been affected. But, how comprehensive and trustworthy are those tests?

I wouldn't trust the unit tests to assure any quality on this refactor, unfortunately.

Incorrectly accessing the created Media instance in a wrong thread—the issue this PR tries to address—doesn't cause any obvious issues to the app. This uploading media issue might be one of them, but it's not easily reproducible. Thus, it's hard to verify that the refactor indeed achieve what it sets out to achieve.

A more valuable test coverage would be making sure this refactor doesn't break things, uploading Media to the WP Media Library in particular. IMO, for now, the most effective way to ensure the coverage is testing all these features manually. But I can spend sometime looking into adding some UI tests, which will benefit us in long run, as I can see the Media area sure needs some further refactors similar to this PR.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for considering my suggestion.

Regarding test coverage and trustworthiness. I see what you mean with the PR tackling the thread access issue 🤔 That would be indeed hard to unit test.

I remain worried about the ratio between the amount of changes that went into this and the unit tests covering them. But I don't want this to come across as a critique to your work or a blocker, just an observation and maybe a collective call to action to add some unit tests.

But I can spend sometime looking into adding some UI tests, which will benefit us in long run, as I can see the Media area sure needs some further refactors similar to this PR.

I'd love to see that happening. However I do wonder how feasible it would be, given that we rely on API mocks? I presume this would need to actually upload the images, or would we be find with stubbed responses? 🤔

Also, before investing in that, I'd recommend checking if there are plans to change the UX soon, just to avoid wasted effort.

@crazytonyli
Copy link
Contributor Author

I remain worried about the ratio between the amount of changes

I agree. I am worried too. 🥲

I wanted to break the changes down to small PRs. I think I'll have another go, not just for the sake of PR review, for myself too, so that I have more confident that the changes don't break things.

But I don't want this to come across as a critique to your work or a blocker

I didn't take it that way. 😄 Thanks for your sharp eyes, that I caught a changed behavior, which I'm not sure if will cause issues, but still, it's a change that I didn't intend to make.

@mokagio
Copy link
Contributor

mokagio commented Mar 23, 2023

I wanted to break the changes down to small PRs. I think I'll have another go, not just for the sake of PR review, for myself too, so that I have more confident that the changes don't break things.

Sounds good to me. We have this approved and we can keep it warm, ready to merge if the need arises.

@crazytonyli crazytonyli force-pushed the tonyli-refactor-media-import-service branch from 2e68e35 to f6cc6e4 Compare March 24, 2023 03:18
@crazytonyli
Copy link
Contributor Author

@mokagio I had made an attempt to split this diff, but couldn't get it right, they are all very much tangled together. I've added a release note highlighting this is a change that may have a high impact on the app. Since I've got your approval, I'll merge this PR and see if there is any issue from the beta testing phase.

RELEASE-NOTES.txt Outdated Show resolved Hide resolved
@mokagio
Copy link
Contributor

mokagio commented Mar 24, 2023

@crazytonyli no worries. Auto-merge enabled 🤖

@mokagio mokagio enabled auto-merge March 24, 2023 04:30
@mokagio mokagio merged commit 8b7eba1 into trunk Mar 24, 2023
@mokagio mokagio deleted the tonyli-refactor-media-import-service branch March 24, 2023 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Data Issues related to Core Data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants