fix: Reuse Objective-C SDK installation on migration#462
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughMigration now triggers when the current installationId differs or the current installation lacks an objectId. On migration, the initializer queries for an existing Installation by the Objective-C installationId (cache-bypassed), reuses and updates it if found, or updates/replaces the current installation otherwise. Tests cover empty, failure, and existing-installation reuse cases. ChangesObjective-C SDK Installation Migration
Sequence DiagramsequenceDiagram
participant ParseInit as ParseSwift.initialize()
participant InstallationCurrent as Installation.current
participant Network as Installation.queryByInstallationId()
participant InstallationState as InstallationState
ParseInit->>InstallationCurrent: read installationId and objectId
ParseInit->>ParseInit: compute shouldMigrateInstallation
alt migration needed
ParseInit->>Network: query by ObjC installationId no-cache
Network-->>ParseInit: return fetched / objectNotFound / error
alt fetched
ParseInit->>InstallationState: set installationId
ParseInit->>InstallationState: call updateAutomaticInfo
ParseInit->>InstallationCurrent: replace with updatedInstallation
else objectNotFound
ParseInit->>InstallationCurrent: replace or keep current and set installationId
ParseInit->>InstallationCurrent: call updateAutomaticInfo
else error
ParseInit-->>ParseInit: return early
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.10][ERROR]: Error: exception Unix_error: No such file or directory stat Tests/ParseSwiftTests/InitializeSDKTests.swift 🔧 SwiftLint (0.63.2)Sources/ParseSwift/Parse.swiftError: No lintable files found at paths: 'Sources/ParseSwift/Parse.swift' Tests/ParseSwiftTests/InitializeSDKTests.swiftError: No lintable files found at paths: 'Tests/ParseSwiftTests/InitializeSDKTests.swift' Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ParseSwift/Parse.swift`:
- Around line 167-181: The fallback path currently reuses
BaseParseInstallation.current (updatedInstallation) and only updates
installationId, which preserves a saved objectId and can accidentally mutate a
server row; change the else branch handling so that if updatedInstallation is a
persisted installation (e.g., updatedInstallation?.objectId != nil or otherwise
indicates saved state) you replace it with a fresh unsaved BaseParseInstallation
instance (or clear its saved identity fields) before assigning installationId
and calling updateAutomaticInfo as needed; then continue to set
BaseParseInstallation.currentContainer.installationId/currentInstallation and
call BaseParseInstallation.saveCurrentContainerToKeychain() so the code no
longer preserves an unrelated objectId when the lookup misses.
In `@Tests/ParseSwiftTests/InitializeSDKTests.swift`:
- Around line 586-607: Wrap the MockURLProtocol cleanup in a defer immediately
after calling MockURLProtocol.mockRequests so removeAll() always runs even on
early returns; specifically, after the MockURLProtocol.mockRequests { ... }
invocation add defer { MockURLProtocol.removeAll() } to ensure the mock is
unregistered for the test that initializes ParseSwift (the block that calls
ParseSwift.initialize and accesses Installation.current and
Installation.currentContainer) and apply the same pattern to the other similar
test blocks mentioned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e34da14c-c07f-432e-a1ee-951d68a0bacc
📒 Files selected for processing (2)
Sources/ParseSwift/Parse.swiftTests/ParseSwiftTests/InitializeSDKTests.swift
|
Ready for review. I addressed the CodeRabbit feedback in
Local validation:
|
|
@coderabbitai review The two actionable comments are now marked addressed in commit 6c7f78a. Please re-review the latest head commit so the stale requested-changes state can be refreshed. |
|
✅ Actions performedFull review triggered. |
|
@coderabbitai review The rate-limit window has elapsed. Please re-review the latest head commit 6c7f78a; the two inline findings are already marked addressed on the review comments, but the PR review decision still shows stale requested changes. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ParseSwift/Parse.swift`:
- Around line 169-186: Replace the use of try? on
BaseParseInstallation.query(...).first(...) with an explicit do-try-catch so you
only treat a nil/empty query result as a lookup miss; if the query returns nil,
create a new BaseParseInstallation and set installationId/updateAutomaticInfo,
but if the query throws (network/auth/decoding errors) catch and preserve the
existing updatedInstallation (do not replace or clear it) and propagate or
rethrow the error instead of falling through to the "not found" branch; update
the logic around BaseParseInstallation.query, foundInstallation,
updatedInstallation, and installationId accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6ff6d11-2652-4aeb-a0c4-b6b92e1c9060
📒 Files selected for processing (2)
Sources/ParseSwift/Parse.swiftTests/ParseSwiftTests/InitializeSDKTests.swift
|
Addressed the latest CodeRabbit feedback in
Validation:
|
|
@coderabbitai review The rate-limit window has elapsed. Please re-review the latest head commit |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/ParseSwiftTests/InitializeSDKTests.swift (1)
679-682: ⚡ Quick winAssert the cache bypass in the reuse-path request.
This fix depends on querying
_Installationwith.reloadIgnoringLocalCacheData, but the mock currently only checks method and URL. Adding a cache-policy assertion here would keep that part of the contract from silently regressing.Suggested test assertion
MockURLProtocol.mockRequests { request in XCTAssertEqual(request.httpMethod, "GET") + XCTAssertEqual(request.cachePolicy, .reloadIgnoringLocalCacheData) XCTAssertTrue(request.url?.absoluteString.contains("_Installation") == true) XCTAssertTrue(request.url?.absoluteString.contains("installationId") == true) do {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/ParseSwiftTests/InitializeSDKTests.swift` around lines 679 - 682, The test MockURLProtocol.mockRequests closure currently asserts only httpMethod and URL but misses verifying the cache policy used for the `_Installation` reload; update the closure used in InitializeSDKTests (MockURLProtocol.mockRequests) to assert that the incoming URLRequest.cachePolicy equals .reloadIgnoringLocalCacheData so the reuse-path query to `_Installation` can't regress silently—locate the closure where XCTAssertEqual(request.httpMethod, "GET") and XCTAssertTrue(request.url?... contains("_Installation")) are checked and add an assertion checking request.cachePolicy (or request.cachePolicy.rawValue if needed) for .reloadIgnoringLocalCacheData.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/ParseSwiftTests/InitializeSDKTests.swift`:
- Around line 679-682: The test MockURLProtocol.mockRequests closure currently
asserts only httpMethod and URL but misses verifying the cache policy used for
the `_Installation` reload; update the closure used in InitializeSDKTests
(MockURLProtocol.mockRequests) to assert that the incoming
URLRequest.cachePolicy equals .reloadIgnoringLocalCacheData so the reuse-path
query to `_Installation` can't regress silently—locate the closure where
XCTAssertEqual(request.httpMethod, "GET") and XCTAssertTrue(request.url?...
contains("_Installation")) are checked and add an assertion checking
request.cachePolicy (or request.cachePolicy.rawValue if needed) for
.reloadIgnoringLocalCacheData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 511a7cd2-edb7-479c-976e-339f4a222ba0
📒 Files selected for processing (2)
Sources/ParseSwift/Parse.swiftTests/ParseSwiftTests/InitializeSDKTests.swift
1 participant
New Pull Request Checklist
Issue Description
When migrating from the Objective-C SDK,
migratingFromObjcSDKcopies the Objective-C KeychaininstallationIdinto the Swift SDK current installation, but it does not reuse the existing_Installationobject from Parse Server. The current installation can therefore remain without the existingobjectId, so a later save may create a new installation instead of using the migrated one.Closes: #426
Approach
When the Objective-C Keychain contains an
installationId, initialization now queries_Installationby thatinstallationIdusing a reload cache policy. If an existing installation is found, it becomes the current installation and is persisted back to the Swift SDK storage/keychain, preserving the existingobjectId.If the lookup misses, the fallback still migrates the Objective-C
installationId, but it avoids carrying over an unrelated savedobjectIdfrom the current Swift installation.Added regression coverage for both the empty lookup fallback and the existing-installation reuse path.
TODOs before merging
Testing
git diff --checkswift test --filter InitializeSDKTestslocally with Swift 6.3.1 on Windows after installing the Swift toolchain and loading the Visual Studio Build Tools environment. The package currently fails before the targeted tests due to an existing Swift 6 parser error inSources/ParseSwift/Coding/ParseEncoder.swift, outside this PR's changed files:'let' binding pattern cannot appear in an expression.Summary by CodeRabbit
Bug Fixes
Tests