Adopt the new Core Data writer API part 2 by crazytonyli · Pull Request #19186 · wordpress-mobile/WordPress-iOS

crazytonyli · 2022-08-15T08:14:01Z

Part 2 of #19185.

Changes

This PR introduces a new write API which accepts a throw closure, which is the main reason that this PR is separated from the one I mentioned above. This new API is implemented in Swift, since a throw closure isn't something can be done in Objective-C—unlike its "sibling APIs".

This new API is used in a few newDerivedContext usages where the Core Data operations throw.

Review tip: turning on "Hide whitespace" may make this PR easier to review.

Test instructions

I think (according to my understanding of the codebase, not the product...), here is a list of area that are changed in this PR. To test, make sure they are working as expected:

blogging prompts settings are reflected in the app.
Add and remove an user to a site.
Create a page with selected layout.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)

Added and removed an user from my blog.
Created a few pages with a few different layouts.

What automated tests I added (or what prevented me from doing so)
N/A

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.

wpmobilebot · 2022-08-15T08:32:50Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19186-d3c4c96 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-08-15T08:35:02Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19186-d3c4c96 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

jkmassel

Given the fact that both the old and new implementations swallow the error, do we need this? I wonder if just continuing to do do/catch inside the core data block is preferable from a "smaller diffs" perspective?

As much as I'd like to improve on our error handling, IMHO the time to do it is when we're able to create a new fully async method that propagates the error instead of hoisting it up one level then discarding it.

WDYT?

crazytonyli · 2022-08-15T23:31:07Z

@jkmassel I should've mentioned this in the description, but one question we need to answer about the ContextManager.saveUsingBlock APIs is, should an exception inside the block prevents any changes happened before it being saved?

I don't think we can make the decision for the call sites. What we can do is providing two variants, one accepts a non-throwing block and the other accepts a throwing block. As a result, they can decide whether to save the changes on call site:

// Example 1: Error is ignored and the data is saved.
contextManager.save { context in
    let foo = Foo(context)
    foo.bar = try? context.fetch(Bar.self)
}

// Example 2: Error is propagated outside and the data isn't saved.
do {
    try contextManager.save { context in
        let foo = Foo(context)
        foo.bar = try context.fetch(Bar.self)
    }
} catch {
    handle(error)
}

jkmassel · 2022-08-16T22:49:03Z

Ah this is interesting! As of right now, I believe that every instance is currently an example of #1.

IMHO (assuming that's true), it'd make sense to create a proper throws variant of save that doesn't do any error internal error handling. Where possible, we can then incrementally adopt it (though IINM we'll need to create throws variants of most of our service class methods as well, so that's not a small change).

My suggestion would be to find one path we can completely convert to throwing methods (ie – a single fetch operation from top to bottom) and do so – that allows us to design an API that's immediately used (thus hopefully shaking out any design issues). From there, we can incrementally convert other parts of the codebase as we see the opportunity.

WDYT?

crazytonyli · 2022-08-17T03:01:30Z

@jkmassel Can you please expand on what you mean by "a single fetch operation from top to bottom"? Do you mean like a query-only operation?

An incremental approach makes sense to me. For this PR though, we can't use the existing saveUsingBlock:completion: function in the changed files, since the function doesn't return a success or failure result—completion always gets called as it has no way of knowing if there is any error in the block.

Do you think it's worth adding a simple performBackgroundTask and using it to replace the newDerivedContext().perform calls in the changed files here?

    func performBackgroundTask(_ block: @escaping (NSManagedObjectContext) -> Void) {
        let context = newDerivedContext()
        context.perform {
            block(context)
        }
    }

jkmassel · 2022-08-17T03:50:15Z

Could we use something like this to get the error back to the caller?

func performAndSave(_ block: @escaping (NSManagedObjectContext) throws -> Void, completion: @escaping (Result<Void, Error>) -> Void) {
    let context = newDerivedContext()
    context.perform {
        do {
            try block(context)
            self.saveContextAndWait(context)
            completion(.success(Void()))
        }
        catch {
            completion(.failure(error))
        }
    }
}

The async version then becomes:

func save(_ block: @escaping (NSManagedObjectContext) throws -> Void) async throws {
    try await withCheckedThrowingContinuation { ct in
        performAndSave(block) { ct.resume(with: $0) }
    }
}

WDYT?

crazytonyli · 2022-08-17T04:13:35Z

@jkmassel 🤦‍♂️ Of course, I somehow forgot we only need to make the performAndSave a non-throwing function. I'll take your suggestion and update this PR accordingly. Thanks!

~~BTW, as discussed previously, I think I'll leave the async version out for now.~~ Actually... I'll include the async version as using it in unit test is quite nice.

mokagio · 2022-08-21T20:28:17Z

@crazytonyli I bumped this to the next release because we'll be code freezing 20.7 today and this hasn't been approved yet.

If this cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready.

crazytonyli · 2022-08-30T23:47:51Z

Friendly PR review reminder to @jkmassel and @mokagio 😄

mokagio

Love that we are using Result here. IMHO, it's. a poorly understood type that should be used much more often.

The implementation looks good to me, I left only a couple of nitpicks.

I think this PR addresses the point @jkmassel's raised in that we have the two throws implementation, and two production use cases for the sync one. As for the async version, Tony's explanation for having it here, "I'll include the async version as using it in unit test is quite nice", is reasonable. 👍

I'm going to approve to unblock, but I took the liberty to disable auto-merge so @jkmassel can have another look. This should give @crazytonyli the flexibility to merge if necessary.

mokagio · 2022-08-31T03:33:09Z

WordPress/WordPressTest/ContextManagerTests.swift

+                account.username = "Unknown User"
+                throw NSError(domain: "save", code: 1)
+            }
+            XCTFail("The above call should throw")


Nitpick: Would you consider a more verbose version of the failure message?

Suggested change

XCTFail("The above call should throw")

XCTFail("Expected `performAndSave` to throw error, but this code was reached so no error was thrown.")

mokagio · 2022-08-31T03:37:19Z

WordPress/Classes/Utility/CoreDataHelper.swift

    }
 }
+
+extension CoreDataStack {


I was going to suggest moving this in a dedicated file, then I noticed CoreDataHelper.swift already contains various extensions. I guess it's worth sticking to the existing pattern for now.

mokagio · 2022-08-31T03:45:27Z

WordPress/Classes/Services/PageLayoutService.swift

+        ContextManager.shared.performAndSave({ context in
+            guard let blog = context.object(with: blogPersistentID) as? Blog else {
+                let userInfo = [NSLocalizedFailureReasonErrorKey: "Couldn't find blog to save the fetched results to."]
+                throw NSError(domain: "PageLayoutService.persistToCoreData", code: 0, userInfo: userInfo)
            }
-            ContextManager.shared.save(context)
-            completion(.success(()))
-        }
+            cleanUpStoredLayouts(forBlog: blog, context: context)
+            try persistCategoriesToCoreData(blog, layouts.categories, context: context)
+            try persistLayoutsToCoreData(blog, layouts.layouts, context: context)
+        }, completion: completion)


Readability nitpick: Given the second closure is named, it looks odd to me that the first isn't.

What would you think of something like

ContextManager.shared.performAndSave( attempt: { context in ... }, completion: completion )

👍 Good call.

mokagio · 2022-08-31T03:47:57Z

WordPress/Classes/Services/BloggingPromptsService.swift

-                        return
-                    }
-                    newPrompt.configure(with: remotePrompt, for: self.siteID.int32Value)
+        contextManager.performAndSave { derivedContext in


I assume derivedContext is so named because that's what it was in the previous code. I wonder if it's useful to keep the "derived" information in the name, or if context would work just as well and be shorter, more straight to the point.

Yeah, I was trying to minimize the diff 😄

This `Result` initializer takes a throwing closure as input and returns a `Result` wrapping its result. See https://developer.apple.com/documentation/swift/result/init(catching:)

mokagio · 2022-09-05T00:33:34Z

I bumped this to the next milestone, 20.8, even though I had approved it. Given this is not user facing, hence we don't have any particular rush in shipping it, I'd rather let @crazytonyli merge when satisfied.

Of course, the sooner we ship, the sooner we'll get real world feedback, but I think in this particular case it's okay to hold back. However, @crazytonyli let me know if you want this in the 20.7 build, and I'll take over merging and ship a new beta for it.

crazytonyli · 2022-09-07T00:24:57Z

I'm going to merge this PR. @jkmassel Let me know if you have any further comments on this change and I'll address in a separate PR if needed 😺

Let me know if you have any further comments on this change and I'll address in a separate PR if needed

crazytonyli added 4 commits August 15, 2022 20:13

Implement an async throw variant of the new writer API

bc77a8e

Use the new writer API in BloggingPromptsService.swift

7c78a37

Use the new writer API in PageLayoutService.swift

e8f6e8e

Use the new writer API in PeopleService.swift

b6dfbb4

crazytonyli added the Core Data Issues related to Core Data label Aug 15, 2022

crazytonyli added this to the 20.6 milestone Aug 15, 2022

crazytonyli requested a review from a team August 15, 2022 08:14

crazytonyli self-assigned this Aug 15, 2022

jkmassel previously requested changes Aug 15, 2022

View reviewed changes

crazytonyli added 2 commits August 17, 2022 16:40

Add performAndSave non-async function that accepts a throwing block

16d4658

Use the new performAndSave function that accepts a throwing block

b335d05

crazytonyli requested review from jkmassel and mokagio August 19, 2022 04:41

mokagio modified the milestones: 20.6, 20.7 Aug 21, 2022

crazytonyli enabled auto-merge August 22, 2022 02:09

mokagio disabled auto-merge August 31, 2022 03:56

mokagio approved these changes Aug 31, 2022

View reviewed changes

mokagio and others added 2 commits August 31, 2022 14:05

Replace a do-catch block with Result(catching:)

320b58f

This `Result` initializer takes a throwing closure as input and returns a `Result` wrapping its result. See https://developer.apple.com/documentation/swift/result/init(catching:)

Replace a do-catch block with Result(catching:) (#19259)

d3c4c96

mokagio removed this from the 20.7 milestone Sep 5, 2022

mokagio added this to the 20.8 milestone Sep 5, 2022

crazytonyli merged commit 0de35b1 into trunk Sep 7, 2022

crazytonyli deleted the core-data-adopt-the-new-writer-api-part-2 branch September 7, 2022 00:25

	XCTFail("The above call should throw")
	XCTFail("Expected `performAndSave` to throw error, but this code was reached so no error was thrown.")

Conversation

crazytonyli commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test instructions

Regression Notes

Uh oh!

wpmobilebot commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel left a comment

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkmassel commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli commented Aug 17, 2022

Uh oh!

jkmassel commented Aug 17, 2022

Uh oh!

crazytonyli commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio commented Aug 21, 2022

Uh oh!

crazytonyli commented Aug 30, 2022

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

crazytonyli Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

crazytonyli Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crazytonyli commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crazytonyli commented Aug 15, 2022 •

edited

Loading

wpmobilebot commented Aug 15, 2022 •

edited

Loading

wpmobilebot commented Aug 15, 2022 •

edited

Loading

crazytonyli commented Aug 15, 2022 •

edited

Loading

jkmassel commented Aug 16, 2022 •

edited

Loading

crazytonyli commented Aug 17, 2022 •

edited

Loading

mokagio commented Sep 5, 2022 •

edited

Loading