Conversation
|
/gemini review |
|
Would you do a thorough check to make sure that the behavior will always match the behavior of the previous Objective-C implementation I've just done a deep line-by-line comparison of the new Swift components against the original 1. Concurrency and Deduplication (Perfect Match)
2. Provider Initialization (Perfect Match)
3. Edge Cases & Type Handling (Matched & Improved)
ConclusionOutside of the
|
… internal AppCheckCore types, and using @_spi for Recaptcha API dependency
… in MockAppCheckCoreAPIService
… type and correcting the 'Core' naming inconsistencies
…y to DebugProvider, DeviceCheckProvider, AppAttestProvider and RecaptchaProvider
…) within AppCheckCore
Review: #111 — "Rewrite AppCheckCore in Swift"PR: google/app-check#111 (draft) ·
Note Every status below was re-checked against the extracted source tree at the current head, not inferred from commit messages or diffs. HeadlineImportant All 5 blockers and all 9 major findings are resolved. 22 of 31 tracked items are closed; 2 have narrow residuals; 11 nits remain. Nothing outstanding would ship broken behavior.
Fixed since the last review pass
B5 deserves specific creditI recommended bumping macOS to 12.0. The branch instead kept genuine macOS 11 support with an availability fallback ( if #available(macOS 12.0, iOS 15.0, tvOS 15.0, watchOS 8.0, *) {
(data, response) = try await urlSession.data(for: request)
} else {
(data, response) = try await withCheckedThrowingContinuation { continuation in
let task = urlSession.dataTask(with: request) { ... }
task.resume()
}
}That preserves the advertised support matrix rather than narrowing it, and re-enabling the macOS CI leg in the same change means it's actually exercised. Better outcome than the fix I proposed.
|
| Symbol | v11 location |
|---|---|
AppCheckCoreStorage / AppCheckCoreStorageProtocol |
Sources/Core/Storage/ (non-public) |
AppCheckCoreBackoffWrapper, AppCheckCoreBackoffType, …ErrorHandler, …DateProvider, …WrapperProtocol |
Sources/Core/Backoff/ (non-public) |
AppCheckCoreTokenRefreshResult, AppCheckCoreTokenRefreshStatus |
Sources/Core/TokenRefresh/ (non-public) |
AppCheckCoreToken+APIResponse extension |
GACAppCheckToken+APIResponse.h (non-public) |
The 167 → 106 reduction covered the important cases. These four are the remainder; same argument applies (public = a v12-lifetime commitment, and tests already use @testable import).
🟡 Remaining nits
| # | Item | Location |
|---|---|---|
| 4 | requestHooks: [Any]? + compactMap { $0 as? … } silently drops non-matching elements; v11 used a typed array |
AppCheckCoreAPIService.swift:50, 64, 68 |
| 6 | URL(string: urlString)! — a malformed resourceName becomes a crash |
AppCheckCoreAppAttestAPIService.swift:191 |
| 7 | Still two lock idioms: NSLock.execute { } (defined at the bottom of the App Attest provider, used from AppCheckCore.swift) vs. stdlib withLock in the backoff wrapper |
AppCheckCoreAppAttestProvider.swift:545 |
| 8 | token(forcingRefresh:) lock/Task ordering is correct but subtle — deserves a comment; also blocks a cooperative-pool thread on NSLock |
AppCheckCore.swift |
| 9 | AppCheckCoreTokenResult still isn't Sendable, which is why deliverOnMainQueue needs nonisolated(unsafe) |
AppCheckCoreTokenResult.swift |
| 10 | AppCheckCoreHTTPError.init(coder:) = fatalError — crashes if archived inside NSUnderlyingErrorKey |
AppCheckCoreHTTPError.swift:48 |
| 11 | DeviceCheck @objc selector inconsistency — no @objc on getToken(completion:), bare @objc on getLimitedUseToken(completion:), explicit selectors elsewhere |
AppCheckCoreDeviceCheckProvider.swift:66, 77 |
| 13 | #if canImport(DeviceCheck) && !os(watchOS) guarding an #available(… watchOS 9.0 …) |
AppCheckCoreErrorUtil.swift:190 |
| 14 | reCAPTCHA client fetch is still a fire-and-forget Task in init — a transient fetchClient failure is cached and replayed forever, and the task is never cancelled on deinit |
RecaptchaTokenGenerator.swift:42-53 |
| 17 | CI runs only Xcode_26.2 everywhere, so nothing validates the minimum toolchain implied by swift-tools-version:6.0 |
spm.yml, app_check_core.yml |
Two new observations
_ObjectiveCBridgeableErroris an underscored stdlib protocol. Conforming to it manually is the right call — it's exactly what the compiler synthesizes forNS_ERROR_ENUM, and it's what makescatch AppCheckCoreErrorCode.keychainwork on a bridgedNSError. But it's SPI, so it isn't source-stable across Swift releases. Worth a comment saying why it's there and thatAppCheckCoreErrorsTests.testErrorCodePatternMatchingis the canary if a future toolchain changes it.AppCheckCoreAppAttestProviderTests.swift:887, 947still usetry await Task.sleep(nanoseconds: 50_000_000)to let a request "reach the chaining branch". Unlike the oldAppCheckCoreTestspattern this can't hang — an actor gate opens regardless — so it's a potential flaky assert rather than a flaky hang. Still, the same file already polls withwhile … { await Task.yield() }a few lines earlier; extending that to the chaining state would remove the timing assumption.
✅ What's good
- Storage wire-format compatibility is genuinely verified, not assumed.
tools/generate_storage_fixtures.shchecks out the real11.0.0ObjC sources, compiles them standalone withclang, and archives fixtures that the Swift tests decode. The "IMMUTABLE TEST: do not edit" markers and theNSStringFromClass(...) == "GACAppCheckStoredToken"assertion are exactly right. - Keychain service name,
GULUserDefaultssuite names, and debug-token keys all carry "Do not rename — v11 compatibility" comments and match byte for byte. - The parity comments are a model for this kind of migration. Each cites the specific v11 construct (
FBLPromise.defaultDispatchQueue,.thenOnvs.recoverOn,FBLPromiseAwaitreturningnil, thevolatilestatic) and explains why the Swift shape reproduces it. That reasoning is the expensive part and it's now captured in the source rather than in a PR thread. - Fixes consistently went in at the right layer. The App Attest blocker was fixed by correcting the test fake's contract, not just the production code.
deliverOnMainQueueandSafeContinuationwere extracted as reusable primitives rather than patched per-site. B5 preserved the support matrix instead of narrowing it.#125independently found a bug in the same code path I'd flagged. - Dropping
PromisesObjC/PromisesSwiftremains a real win for downstream dependency graphs.
Remaining worklist
Before merge
- M4 residual — make
SafeContinuationreachable fromAppCheckRecaptchaProvider, or duplicate it there. The reCAPTCHA SDK is the one genuinely third-party callback boundary in the codebase. - Changelog — two entries still missing: (a) Objective-C classes can no longer be subclassed (
objc_subclassing_restricted), namingFIRAppCheckSettings/GACAppCheckSettingsspecifically; (b) forced refreshes are no longer coalesced with in-flight unforced ones — this resolvesTODO(#42)fromGACAppCheck.m:96, so close that issue too. - Nit 6 — replace the
URL(string:)!force-unwrap with a thrown error. Cheapest crash removal left.
Worth doing before 12.0 ships (much cheaper now than after the API freezes)
- M7 residual — demote
AppCheckCoreStorage, the backoff types,AppCheckCoreTokenRefreshResult, and theToken+APIResponseextension tointernal. - Nit 4 — type
requestHooksproperly. - Nit 11 — explicit
@objc(...)selectors on the DeviceCheck provider. - Nit 17 — pin one CI leg at the lowest supported Xcode.
Opportunistic — nits 7, 8, 9, 10, 13, 14, plus a comment explaining the _ObjectiveCBridgeableError conformance and the Task.sleep timing assumption in the App Attest tests.
Still open on the PR's own checklist: "Refactor of reCAPTCHA provider" (nit 14 is the substantive item there) and "Retest with Firebase 13 branch".
…ke SafeContinuation final/package
…nd Token+APIResponse extension to package visibility
…ray signatures and fix AppAttestProvider flaky deduplication test
…Sendable constraints in deliverOnMainQueue
Review: #111 — "Rewrite AppCheckCore in Swift"PR: google/app-check#111 (draft) ·
Note Every status below was re-checked against the extracted source tree at the current head, not inferred from commit messages or diffs. HeadlineImportant All 5 blockers, all 9 majors, and both residuals are now resolved. 26 of 31 tracked items are closed with no residuals; 5 nits and 2 changelog entries remain, none of which affect shipped behavior.
Fixed since
|
| Finding | How — verified at 968afecb |
|
|---|---|---|
| M4 residual | reCAPTCHA's third-party continuations were unguarded | SafeContinuation is now package final class SafeContinuation<T: Sendable, E: Error> and withSafeCheckedThrowingContinuation is package, so RecaptchaTokenGenerator.swift:43, 60 now use it. The doc comment I asked for ("does not enforce that a resume is ever called") was added. @preconcurrency import RecaptchaInterop handles the un-annotated third-party callbacks. Only two raw continuations remain in the tree, both wrapping Apple APIs with guaranteed single completion (AppCheckCoreAPIService.swift:142, AppCheckCore.swift:292). |
| M7 residual | Four v11-private types still public |
All demoted to package: AppCheckCoreStorage/…Protocol, the four backoff types, AppCheckCoreTokenRefreshResult/…Status, and the AppCheckCoreToken+APIResponse extension. Public declarations in Sources are now 119 → 92 (167 at the first review). package rather than internal is the right call — it keeps cross-target use working under SwiftPM, and SWIFT_PACKAGE_NAME = 'AppCheck' in the podspec matches Package.swift's name: "AppCheck" so CocoaPods resolves it identically. |
| Nit 4 | requestHooks: [Any]? |
All four public inits plus both AppCheckCoreAPIService inits now take [AppCheckCoreAPIRequestHook]?, and the lossy compactMap { $0 as? … } is replaced by requestHooks ?? []. AppCheckAPITests.swift:120-128 now passes a real typed hook instead of nil, so the bridging is actually exercised. ObjC parity with v11's NSArray<GACAppCheckAPIRequestHook> * restored. |
| Nit 6 | URL(string:)! force-unwrap |
urlForEndpoint is now throws and all three call sites propagate. |
| Nit 9 | AppCheckCoreTokenResult not Sendable |
Now public final class … @unchecked Sendable; both deliverOnMainQueue overloads constrain T: Sendable / E: Sendable and drop the nonisolated(unsafe) on the payload (only the caller-supplied handler keeps it, correctly). The @unchecked is sound — AppCheckCoreToken and AppCheckCoreTokenResult are all-let. |
| Nit 11 | Stray bare @objc on DeviceCheck |
Removed. |
| Bonus | Artifact storage continuation was typed NSSecureCoding? |
Narrowed to AppCheckCoreAppAttestStoredArtifact? with the cast moved into the resume, and the artifact marked @unchecked Sendable — a real Sendable hole closed, not just a signature change. |
Fixed in the previous pass (9d5eaf11 → 1b6142ec)
| Finding | How | |
|---|---|---|
| B2b | Staging endpoint compiled into release builds | #if !NDEBUG → #if DEBUG in AppCheckCoreAPIService.swift:21, 72. Zero NDEBUG directives remain in the tree — the only hits are the explanatory comment in the logger. |
| B4b | Swift catch AppCheckCoreErrorCode.x never matched |
CustomNSError + _ObjectiveCBridgeableError conformances (AppCheckCoreErrors.swift:60-73), plus AppCheckCoreErrorsTests.swift whose testErrorCodePatternMatching throws a raw NSError and catches it as AppCheckCoreErrorCode.keychain. That test would have failed before. |
| B5 | macOS 11 target vs. macOS 12 API | Solved better than I suggested — see below. |
| M3b | Provider completions off the main queue | All four providers now route through deliverOnMainQueue; AppCheckCore.deliverOnMainQueue was generalized to <T> / <T, E> and made internal so Debug, DeviceCheck, and App Attest share it, with a parallel private copy in the reCAPTCHA module (necessarily, cross-module). |
| M4 | CheckedContinuation double-resume crash |
New SafeContinuation.swift with lock-protected resume-once semantics + withSafeCheckedThrowingContinuation, applied at 11 call sites across the provider, storage, and token-generator boundaries. |
| M5 | Ad-hoc NSError domains |
All 5 occurrences gone; zero domain: "App…" literals remain. |
| M6 | Any-typed backoff wrapper |
Now applyBackoffToOperation<T>; the as! AppCheckCoreToken and both bespoke guard let … as? blocks are gone. Protocol also renamed AppCheckBackoffWrapperProtocol → AppCheckCoreBackoffWrapperProtocol. |
| M7 | Public API surface expansion | public/open declarations in Sources dropped 167 → 106. AppCheckCore's six leaked properties are now internal; the App Attest internals (API service, both storages, provider state, rejection error) are all internal. |
| M8 | CocoaPods not building ObjC/integration tests | Tests/Unit/ObjC/**/*.m added to unit_tests, s.test_spec 'integration' restored pointing at the Swift E2E test, swift_version 5.5 → 5.9, SWIFT_PACKAGE_NAME added. |
| M9 | Task.sleep hang-race in tests |
Replaced with await fulfillment(of: [continuationSetExpectation], timeout: 5.0) in both affected tests. |
| Nit 3 | // Assuming … LLM scaffolding comments |
Removed. |
| Nit 5 | appCheckToken(withAPIResponse:) async but never suspends |
Now plain throws, in both the protocol and the implementation. |
| Nit 12 | Duplicate GoogleUtilities imports |
Removed. |
| Nit 16 | macOS tests skipped in CI | target: [ios, tvos, macos, watchos] — --skip-tests and the stale OCMock TODO are gone. |
| Item 11 (partial) | Changelog | Error-domain breaking change documented with a before/after snippet. |
B5 deserves specific credit
I recommended bumping macOS to 12.0. The branch instead kept genuine macOS 11 support with an availability fallback (AppCheckCoreAPIService.swift:139-154):
if #available(macOS 12.0, iOS 15.0, tvOS 15.0, watchOS 8.0, *) {
(data, response) = try await urlSession.data(for: request)
} else {
(data, response) = try await withCheckedThrowingContinuation { continuation in
let task = urlSession.dataTask(with: request) { ... }
task.resume()
}
}That preserves the advertised support matrix rather than narrowing it, and re-enabling the macOS CI leg in the same change means it's actually exercised. Better outcome than the fix I proposed.
✅ Both residuals closed — one thing to verify
SafeContinuation reached the reCAPTCHA module and the four v11-private types became package. Neither change has a residual, but the package demotion moves a risk from "API surface" to "build configuration", so confirm these three before merging:
- CocoaPods test targets.
SWIFT_PACKAGE_NAMEis set ins.pod_target_xcconfigat the root ofAppCheckCore.podspec:52-56. Test specs are separate native targets; if they don't inherit it,@testable importwon't seepackagesymbols and the unit tests won't compile. CI will catch it, but it's the first thing to look at if the pod lint leg goes red. - Downstream pods.
FirebaseAppCheckis built with a different package name, so it sees onlypublic. Verified that the ObjC API tests don't touch any demoted symbol; firebase-ios-sdk#16544 is the real check. - The Swift API-surface test now sees
package.AppCheckAPITests.swiftuses a plainimport AppCheckCore, but because it's in the same package it can now reachpackagedeclarations too. It no longer proves "this is the public surface." Not worth restructuring, just don't rely on it for that.
🟡 Remaining nits
| # | Item | Location |
|---|---|---|
| 7 | Still two lock idioms: NSLock.execute { } (defined at the bottom of the App Attest provider, used from AppCheckCore.swift) vs. stdlib withLock in the backoff wrapper |
AppCheckCoreAppAttestProvider.swift:545 |
| 8 | token(forcingRefresh:) lock/Task ordering is correct but subtle — deserves a comment; also blocks a cooperative-pool thread on NSLock |
AppCheckCore.swift |
| 10 | AppCheckCoreHTTPError.init(coder:) = fatalError — crashes if archived inside NSUnderlyingErrorKey |
AppCheckCoreHTTPError.swift:48 |
| 13 | #if canImport(DeviceCheck) && !os(watchOS) guarding an #available(… watchOS 9.0 …) |
AppCheckCoreErrorUtil.swift:190 |
| 14 | reCAPTCHA client fetch is still a fire-and-forget Task in init. The continuation is now safe (M4), but the two behavioral halves are untouched: a transient fetchClient failure is cached in recaptchaClientTask and replayed forever, and there is still no deinit cancelling the task |
RecaptchaTokenGenerator.swift:42-53 |
| 17 | CI runs only Xcode_26.2 everywhere, so nothing validates the minimum toolchain implied by swift-tools-version:6.0 (and now by package access / SWIFT_PACKAGE_NAME, which need Xcode 15+) |
spm.yml, app_check_core.yml |
Three new observations
-
init NS_UNAVAILABLEparity — checked, and it's fine. v11 declared- (instancetype)init NS_UNAVAILABLE;onGACAppCheck,GACDeviceCheckProvider,GACAppCheckDebugProvider, andGACAppAttestProvider. In the Swift port onlyAppCheckCoreAppAttestProvider.swift:35-37has an explicit@available(*, unavailable) override public init(). The other three don't need one: each declares its own designated initializers and therefore does not inheritNSObject.init, so the generated ObjC header emits- (nonnull instancetype)init SWIFT_UNAVAILABLE;and+ new SWIFT_UNAVAILABLE_MSGautomatically. The explicit App Attest version buys only a cleanfatalErrorfor dynamic (NSClassFromString/performSelector) callers. Either standardize on it for all four or drop it — but nothing is broken. -
_ObjectiveCBridgeableErroris an underscored stdlib protocol. Conforming to it manually is the right call — it's exactly what the compiler synthesizes forNS_ERROR_ENUM, and it's what makescatch AppCheckCoreErrorCode.keychainwork on a bridgedNSError. But it's SPI, so it isn't source-stable across Swift releases. Worth a comment saying why it's there and thatAppCheckCoreErrorsTests.testErrorCodePatternMatchingis the canary if a future toolchain changes it. -
Test timing, partially addressed.
AppCheckCoreAppAttestProviderTests.swift:888, 948still usetry await Task.sleep(nanoseconds: 50_000_000)to let the second request reach the chaining branch; the new commit instead addedtry? await Task.sleep(nanoseconds: 20_000_000)inside the fake'sgetAppCheckToken(line 125) to widen the in-flight window for the tests that have no gate. That does make the flake much less likely, but it makes every test on that path 20 ms slower and keeps the assumption implicit. The deterministic version is already in the file:AsyncGateongetRandomChallenge. A second gate ongetAppCheckTokenwould remove both sleeps.
✅ What's good
- Storage wire-format compatibility is genuinely verified, not assumed.
tools/generate_storage_fixtures.shchecks out the real11.0.0ObjC sources, compiles them standalone withclang, and archives fixtures that the Swift tests decode. The "IMMUTABLE TEST: do not edit" markers and theNSStringFromClass(...) == "GACAppCheckStoredToken"assertion are exactly right. - Keychain service name,
GULUserDefaultssuite names, and debug-token keys all carry "Do not rename — v11 compatibility" comments and match byte for byte. - The parity comments are a model for this kind of migration. Each cites the specific v11 construct (
FBLPromise.defaultDispatchQueue,.thenOnvs.recoverOn,FBLPromiseAwaitreturningnil, thevolatilestatic) and explains why the Swift shape reproduces it. That reasoning is the expensive part and it's now captured in the source rather than in a PR thread. - Fixes consistently went in at the right layer. The App Attest blocker was fixed by correcting the test fake's contract, not just the production code.
deliverOnMainQueueandSafeContinuationwere extracted as reusable primitives rather than patched per-site. B5 preserved the support matrix instead of narrowing it.#125independently found a bug in the same code path I'd flagged. - Dropping
PromisesObjC/PromisesSwiftremains a real win for downstream dependency graphs.
Remaining worklist
Before merge
- Changelog — the only pre-merge item left, and still untouched at
968afecb. Two entries missing: (a) Objective-C classes can no longer be subclassed (objc_subclassing_restricted), namingFIRAppCheckSettings/GACAppCheckSettingsspecifically; (b) forced refreshes are no longer coalesced with in-flight unforced ones — this resolvesTODO(#42)fromGACAppCheck.m:96, so close that issue too. Arguably a third now:GACAppCheckTokenResultisfinal, so ObjC/Swift subclasses of it break. - Confirm CI is green on the CocoaPods legs — the
packagedemotion is the first change in this PR whose failure mode is a build failure in a configuration SwiftPM doesn't exercise.
Worth doing before 12.0 ships
- Nit 17 — pin one CI leg at the lowest supported Xcode. Now slightly more valuable:
package+SWIFT_PACKAGE_NAMErequire Xcode 15+, and nothing in CI proves what the real floor is.
Opportunistic — nits 7, 8, 10, 13, 14, plus a comment explaining the _ObjectiveCBridgeableError conformance.
Still open on the PR's own checklist: "Refactor of reCAPTCHA provider" (nit 14 is the substantive item there) and "Retest with Firebase 13 branch" — the latter is now also the check for item 2.
Swift's bridging mechanism silently fails to cast Objective-C blocks inside [Any]? arrays to @convention(block) closures. By catching blocks via type name checking and using unsafeBitCast, we successfully unwrap them and prevent silently dropping request hooks in ObjC clients.
1. Updated dynamic Objective-C block fallback in AppCheckCoreAPIService.swift to strictly verify block ancestry using NSClassFromString("NSBlock"), preventing UB on other callers simulating strings with 'Block' inside their classname.
2. Formatted required block signatures strictly into /// doc comments in all public initializers.
3. Repurposed the bridging crash verification ObjC test to mock GACAppCheckAPIService directly with a stub session intercept, making it deterministic and hermetic. Additionally injected a negative non-block string argument to pin down the filter.
…ed initializers 1. Added leading underscore prefixes (_GACAppCheckAPIService, _GACURLSessionDataResponse, _GACAppCheckErrorUtil) and dropped Core from GACAppCheckBackoffType to restore explicit v11 Objective-C parity. 2. Pinned ObjC selectors for AppCheckCoreAPIService using @objc(initWithURLSession:baseURL:APIKey:requestHooks:) and removed the residual M7 comment. 3. Cleaned up AppCheckCoreObjCAPITests.m by moving GACAppCheckMockURLProtocol out of the standalone category block and above the main test suite implementation.
Signed-off-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
2 participants
This PR migrates AppCheckCore from Objective-C to Swift, replacing FBLPromises with async/await while maintaining Objective-C backwards compatibility.
CocoaPods port is not intended for release - only to increase test coverage, since several tests are CocoaPods only.
See google/GoogleSignIn-iOS#626 and firebase/firebase-ios-sdk#16544 for integration testing with the AppCheckCore dependencies.
Fixes #42
Before merge:
- [ ] Remove podspecArchitectural Changes & Code Review Summary
1. Code Conversion and Modernization
FBLPromiseand manualdispatch_queuecallback chains were successfully migrated to Swiftasync/await.@objcand@objcMembersannotations were attached to Swift classes. LegacyGAC...prefixes (e.g.,@objc(GACAppCheckSettings)) were properly maintained so that down-stream clients wouldn't encounter missing symbols.objc_subclassing_restrictedattribute, breaking existing Objective-C classes that historically subclassed them (e.g.FIRAppCheckSettings). This was resolved by removing direct subclassing and satisfying protocols (GACAppCheckSettingsProtocol) instead.2. File and Structural Integrity
.swiftfiles correctly apply the Apache 2.0 license headers.AppCheckCore.h) and public module headers have been safely retired or modified to point to theAppCheckCore-Swift.hbridge.3. API Signature Consistency
initWithToken:expirationDate:receivedAtDate:explicitly uses thereceivedAt:argument label in Swift to prevent breaking compilation across SDKs that depended on Swift's automatic Clang truncation.GACAppCheckErrorDomain) do not export easily, so usages have been cleaned up or replaced by their literal string representations.4. Build and Compilation Validation
AppCheckCoreframework compiles with 0 warnings and 0 errors locally.Downstream Compatibility Matrix
Downstream Effect: A GSI 9 x Firebase 12 project that updates to GSI to 10 will have no issue. If the project updates to Firebase 13, it will need to update GSI.