feat: add connectivity error classification to RequestExecutionErrorReason - #1488
Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add to your .package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1488")Built from e5525ed |
a661e5e to
c652fa8
Compare
Temporarily resolves wordpress-rs from the `pr-build/1488` snapshot branch so the demo app can build against `WpApiError.isSiteUnreachable` and `.isDeviceOffline`, which are not in the released 0.6.0. INTERIM — must be replaced with an exact version before merging. The snapshot branch is force-pushed on every CI run of Automattic/wordpress-rs#1488 and its S3 artifact lives under `pr-builds/1488/`, so neither is maintained once that PR merges. Swap to the release that carries the new API. Refs #578 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aadfc8d to
867abf4
Compare
| /// # Platform differences | ||
| /// | ||
| /// A refused connection (the host resolves, but nothing is listening) is | ||
| /// **not** classified consistently: | ||
| /// | ||
| /// - Swift and the `reqwest` executor map it to `NonExistentSiteError`, so | ||
| /// this returns `true`. | ||
| /// - Kotlin maps it to `HttpError`, so this returns `false`. | ||
| /// | ||
| /// Only a DNS failure is treated as an unreachable site by every executor. | ||
| /// Callers that must behave identically across platforms should rely on that | ||
| /// case alone until the mappings are aligned. | ||
| /// | ||
| /// Note also that a malformed site URL never reaches this predicate: it | ||
| /// surfaces as [`WpApiError::SiteUrlParsingError`], which carries no | ||
| /// `RequestExecutionErrorReason`. | ||
| pub fn is_site_unreachable(&self) -> bool { |
There was a problem hiding this comment.
Flagging the executor divergences documented in the Known limitation section above. I uncovered these while building this out, rather than knowing them going in.
Worth noting they're preexisting: nothing here changes how any executor classifies errors. The divergences exist on trunk today. What this PR adds is a single name spanning all three executors, which is what made them visible.
It remains unclear to me whether that's acceptable for now, whether the mappings should be aligned before this ships, or whether the Swift-only option from the original feedback is the better move. I welcome guidance on the best way to move forward with this.
…eason Adds `is_site_unreachable` and `is_device_offline` to `RequestExecutionErrorReason`, exposed to the bindings as exported free functions. The request executors already map the underlying platform errors onto `NonExistentSiteError` and `DeviceIsOfflineError`, but consumers had to match those variants themselves. That is fragile — it breaks whenever the enum gains a case — and the knowledge belongs to the library rather than to each consumer. The predicates live on the reason rather than on `WpApiError` because that is where the variants are defined, and because the reason is what consumers actually hold: `WpRequestResult.RequestExecutionFailed` and `WpApiException.RequestExecutionFailed` both expose it on Kotlin, and `RequestExecutionError` carries the same reason as `WpApiError` on Swift. Export shape follows `application_passwords_url` in `login::url_discovery`: a `#[uniffi::export]` free function taking a data-carrying `uniffi::Enum` by reference, with an inherent method behind it for Rust callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wraps the exported classification functions in idiomatic properties: - Swift: `RequestExecutionErrorReason.isSiteUnreachable` / `.isDeviceOffline`, plus convenience properties on `WpApiError` and `RequestExecutionError` that delegate through the nested reason. The `WpApiError` shape matches how WordPress-iOS consumes `isCancellationError`. - Kotlin: extension properties on `RequestExecutionErrorReason`, reachable from `WpRequestResult.RequestExecutionFailed.reason` — the form WordPress-Android already hand-rolls in `ApplicationPasswordValidator` and `PostRsErrorUtils`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc comments promised more than the predicates deliver, which would lead consumers to write error-handling branches that never fire. - `is_site_unreachable` claimed "refused the connection". That is platform-divergent: Swift and the `reqwest` executor map a refused connection to `NonExistentSiteError`, while Kotlin maps it to `HttpError`. Only a DNS failure is classified as an unreachable site by every executor. - It also claimed "the URL was malformed". A malformed URL surfaces as `WpApiError::SiteUrlParsingError`, a sibling of `RequestExecutionFailed`, so it never produces a `RequestExecutionErrorReason` at all. - `is_device_offline` did not mention that `DeviceIsOfflineError` is only ever constructed by the Swift and Kotlin executors. Under the `reqwest` executor it is always `false`, and an offline failure is reported as `NonExistentSiteError` via the DNS resolution failure — inverting both predicates. Narrows each doc to what actually holds and documents the divergences, on the Rust methods and in the Swift and Kotlin wrappers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The negative-case list omitted `MisconfiguredHttpAuthenticationError`, so the suite did not assert that both predicates return `false` for it. All 12 variants are now covered: two by the positive tests, ten by the negative case list. Since the predicates use `matches!` rather than an exhaustive match, a new variant would silently return `false` from both without a compile error — this list is the only thing that would catch it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`WpApiError` and `RequestExecutionError` each carried a verbatim copy of `executionErrorReason` plus both predicates. They share the same `RequestExecutionFailed` payload, so the copies would drift if that payload ever changes. Introduces `CarriesRequestExecutionErrorReason`, which both conform to. The predicates are written once in a protocol extension; each type supplies only the reason accessor. No change to the public surface: `error.isSiteUnreachable` and `.isDeviceOffline` still resolve on both types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
867abf4 to
e5525ed
Compare
Temporarily resolves wordpress-rs from the `pr-build/1488` snapshot branch so the demo app can build against `WpApiError.isSiteUnreachable` and `.isDeviceOffline`, which are not in the released 0.6.0. INTERIM — must be replaced with an exact version before merging. The snapshot branch is force-pushed on every CI run of Automattic/wordpress-rs#1488 and its S3 artifact lives under `pr-builds/1488/`, so neither is maintained once that PR merges. Swap to the release that carries the new API. Refs #578 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `pr-build/1488` snapshot branch was deleted once that PR merged, taking its S3 artifact under `pr-builds/1488/` with it, so the previous pin no longer resolves. Repoints at `trunk-build`, the published mirror of trunk. Trunk itself cannot be consumed as a remote SPM dependency: its `Package.swift` sets `libwordpressFFIVersion = .local`, which expects `target/libwordpressFFI.xcframework` to be built locally by Cargo and is absent from a fresh checkout. `trunk-build` carries the same Swift sources but declares `.release`, fetching the prebuilt xcframework from the CDN. Pinned by revision rather than branch because `trunk-build` is force-pushed on every trunk CI run. `d70c99e1` is the build of trunk `c2f8a25f`, which carries `WpApiError.isSiteUnreachable` and `.isDeviceOffline`. INTERIM — must be replaced with an exact version before merging. No tagged release carries these helpers yet; the latest, `alpha-20260313.1`, predates Automattic/wordpress-rs#1488. Swap to the release that carries the new API. Refs #578 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A refused connection (the host resolves, but nothing is listening) was classified as NonExistentSiteError on Swift but not on Kotlin or reqwest, so the isSiteUnreachable predicate from #1488 returned a different answer per platform for the same outage. Move Swift's .cannotConnectToHost out of the non-existent-site set, leaving NonExistentSiteError to mean a DNS-resolution failure. The follow-up commit gives refused/unreachable connections a dedicated ConnectionError reason across all three executors. Refs #1495.
Description
The request executors map the underlying platform errors onto
NonExistentSiteErrorandDeviceIsOfflineError, but consumers have to match those variants themselves to tell "this site could not be reached" from "this device has no connection". That is fragile — it breaks whenever the enum gains a case — and the knowledge belongs to the library rather than to each consumer.Raised in GutenbergKit review feedback, tracked by GutenbergKit#578, where the iOS demo app hand-rolls both checks.
Changes
RequestExecutionErrorReason::is_site_unreachable()andis_device_offline()as inherent methods, for Rust callersrequest_execution_error_reason_is_site_unreachable/..._is_device_offlineas#[uniffi::export]free functions, for the bindingsRequestExecutionErrorReason, plus convenience properties onWpApiErrorandRequestExecutionError. Both conform to aCarriesRequestExecutionErrorReasonprotocol, so the predicates are written once and each type supplies only the reason accessor.RequestExecutionErrorReasonRequestExecutionErrorReasonvariants — two positives, ten negativesAdditive only; no behaviour change to existing APIs.
Why these live on the reason rather than on
WpApiErrorThe variants are defined on
RequestExecutionErrorReason, and the reason is what consumers actually hold:WordPress-iOSreaches the reason from two different outer errors (SelfHostedSiteAuthenticator.swifthas both alog(error: WpApiError)and alog(error: RequestExecutionError)overload that funnel into the same reason handler). Predicates on the reason serve both; theWpApiErrorconvenience property preserves theerror.isCancellationErrorshape thatExtensions/Error.swiftalready uses.WpApiExceptionis largely internal plumbing; consumers hold aWpRequestResult. In GutenbergKit's Android demo app, all four files that touch wordpress-rs errors useWpRequestResultand none referenceWpApiException.WpRequestResult.RequestExecutionFailedexposesreasondirectly.Export shape
Follows
application_passwords_urlinlogin::url_discovery— a#[uniffi::export]free function taking a data-carryinguniffi::Enumby reference, with an inherent method behind it. Every#[uniffi::export] implblock inwp_api/src/targets auniffi::Object, so an exportedimplon an enum would be novel here.Open questions for review
WpApiError. The evidence above favours the reason, but I did not find a precedent that settles it either way — happy to move them if you'd rather they sat on the error.request_execution_error_reason_is_site_unreachablegeneratesrequestExecutionErrorReasonIsSiteUnreachable(reason:), which is a mouthful. The type-prefixed convention is my own;application_passwords_urlis unprefixed. Shorter names welcome.boolvsOption<bool>. Both patterns exist (FindApiRootFailure::is_network_errorreturnsbool;is_application_passwords_disabledreturnsOption<bool>). I usedboolsince the question always applies to a reason, but flagging the choice.Known limitation: the executor mappings disagree
The three executors do not classify the same failure the same way. The doc comments on both predicates now spell this out, but it is worth stating plainly here.
Connection refused — Kotlin is the odd one out:
SafeRequestExecutor)ReqwestRequestExecutor)WpRequestExecutor)NonExistentSiteErrorNonExistentSiteErrorNonExistentSiteErrorNonExistentSiteError(.cannotConnectToHost)NonExistentSiteError(is_connect())HttpError(ConnectException)NonExistentSiteErrorNonExistentSiteErrorHttpError(NoRouteToHostException)So
isSiteUnreachablereturnstrueon Swift andfalseon Kotlin for the same real-world failure — e.g. a local dev server that is not running. Verified with okhttp 5.4.0: a closed local port throwsjava.net.ConnectException, whichWpRequestExecutormaps toHttpError.HttpErroris included as a negative case in the unit tests so the current behaviour is explicit.Offline detection is platform-only.
DeviceIsOfflineErroris constructed exclusively by the Swift and Kotlin executors, which consult aNetworkAvailabilityProvider.ReqwestRequestExecutorhas no such mapping, so for consumers that build it directly —wp_rs_web,wp_com_e2e—is_device_offline()is alwaysfalse. Worse, an offline failure there fails DNS resolution and is reported asNonExistentSiteError, sois_site_unreachable()returnstrueinstead. The two predicates are effectively inverted on that path.Only a DNS failure means the same thing everywhere. Callers needing identical behaviour across all three executors should rely on that case alone until the mappings are aligned.
This PR does not change the mappings, because aligning Kotlin with Swift is a behaviour change with a downstream hazard. In WordPress-Android,
MediaRSApiRestClientmapsNonExistentSiteErrortoMediaErrorType.NOT_FOUND, andMediaDeleteServiceresponds toNOT_FOUNDby deleting the local media record. Today a refused connection yieldsHttpError→GENERIC_ERROR, which is non-destructive. Remapping without fixing that first would make an unreachable site look like deleted remote media. Thewhenin question is exhaustive with noelse, so the change would compile silently, and there is no test covering that arm.Suggested sequencing, if the alignment is wanted: fix the WordPress-Android media mapping first, then align
ConnectException(and probablyNoRouteToHostException) in a follow-up PR here. GivingReqwestRequestExecutora way to report offline is a separate question, since it has noNetworkAvailabilityProviderequivalent.Test plan
Automated:
cargo test -p wp_api --lib api_error— 13 tests passcargo fmt --all -- --check— cleancargo clippy -p wp_api --lib --all-features -- -D warnings— cleanswift build --target WordPressAPI— 35/35 files, 0 errors./gradlew :api:kotlin:compileKotlin :api:kotlin:detekt— BUILD SUCCESSFUL, 0 findingsManual, via GutenbergKit's iOS demo app pointed at this PR's snapshot branch:
isSiteUnreachable, correct guidance shownisDeviceOffline, offline editor configuration applied/etc/hosts, network up →isSiteUnreachabletrue andisDeviceOfflinefalse on the same error, so the offline fallback correctly does not applyChangelog
CHANGELOG.mdunder## [Unreleased], using the Keep a Changelog categories (Added,Changed,Deprecated,Removed,Fixed,Security). Prefix breaking changes with**BREAKING:**.