Skip to content

Catch Objective-C exceptions from Core Data object query #20755

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 1, 2023
Merged

Catch Objective-C exceptions from Core Data object query #20755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ec21e1
Capture an Objective-C exception and send it to Sentry
crazytonyli May 31, 2023
5672b0d
Removed `tabBar` init from redesigned DomainsSuggestionsScreen
pachlava May 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions WordPress/Classes/ViewRelated/Gutenberg/EditorMediaUtility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,22 @@ final class AuthenticatedImageDownload: AsyncOperation {
throw DownloadError.blogNotFound
}

let blog = try context.existingObject(with: self.blogObjectID) as! Blog
return MediaHost(with: blog) { error in
var blog: Result<Blog, Error> = .failure(DownloadError.blogNotFound)
Copy link
Contributor

@mokagio mokagio May 31, 2023

Choose a reason for hiding this comment

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

Is this default value necessary? I don't see a code path where it's used, since if a try in the do-catch throws, the method rethrows.

Copy link
Contributor Author

@crazytonyli crazytonyli May 31, 2023

Choose a reason for hiding this comment

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

It's necessary in terms of making the code look nicer, but also kind of not necessary because the default value will never get used.

The alternative is, we don't give it a default value, then the blog variable needs to be defined as Optional. That means at line 49 where blog.get() is called to get the Blog instance, we need to unwrap the optional blog. Now, if the code ever reaches line 49 at runtime, the blog will never be nil, because no Objective-C exception was thrown and the blog must be a Result instance that wraps a blog object or a Swift Error.

Considering the alternative is unnecessarily unwrapping an Optional, I decided to just give it a default value, which is never going to be used.

Copy link
Contributor

@mokagio mokagio Jun 1, 2023

Choose a reason for hiding this comment

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

Gotcha. I was hoping we could do something like

                let x = true
                var foo: Int
                if x {
                    foo = 2
                } else {
                    foo = 3
                }

Alas, the construct work in the context of an if else but not in a closure

image

do {
// Catch an Objective-C `NSInvalidArgumentException` exception from `existingObject(with:)`.
// See https://github.com/wordpress-mobile/WordPress-iOS/issues/20630
try WPException.objcTry {
blog = Result {
try context.existingObject(with: self.blogObjectID) as! Blog
}
}
} catch {
// Send Objective-C exceptions to Sentry for further diagnosis.
WordPressAppDelegate.crashLogging?.logError(error)
throw error
}

return try MediaHost(with: blog.get()) { error in
// We'll log the error, so we know it's there, but we won't halt execution.
WordPressAppDelegate.crashLogging?.logError(error)
}
Expand Down
3 changes: 0 additions & 3 deletions WordPress/UITestsFoundation/Screens/DomainsSuggestionsScreen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import ScreenObject
import XCTest

public class DomainsSuggestionsScreen: ScreenObject {
public let tabBar: TabNavComponent

let siteDomainsNavbarHeaderGetter: (XCUIApplication) -> XCUIElement = {
$0.staticTexts["Search domains"]
Expand All @@ -11,8 +10,6 @@ public class DomainsSuggestionsScreen: ScreenObject {
var siteDomainsNavbarHeader: XCUIElement { siteDomainsNavbarHeaderGetter(app) }

public init(app: XCUIApplication = XCUIApplication()) throws {
tabBar = try TabNavComponent()

try super.init(
expectedElementGetters: [ siteDomainsNavbarHeaderGetter ],
app: app,
Expand Down