fix: restore cookie fields across an archive round trip - #979
Conversation
PR SummaryMedium Risk Overview
Tests replace the defect-pinning case with Reviewed by Cursor Bugbot for commit f5407ad. Bugbot is set up for automated code reviews on this repo. Configure here. |
🐦 Swift Migration ProgressProduction implementation code at
Objective-C retained by design: Core SDK 6,200 · SDK kit infrastructure 2,337 · Standalone kits 0. This PR's code movement
How this is measured
Generated with |
📦 SDK Size Impact ReportMeasures how much the SDK adds to an app's size (with-SDK minus without-SDK).
➡️ SDK size impact change is minimal. Raw measurementsTarget branch (workstation/swift-migration): {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2808,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2724,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6968}This PR: {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2808,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2724,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6968} |
c9a1add to
bda3c27
Compare
bda3c27 to
75ec848
Compare
📝 WalkthroughWalkthroughThe cookie secure decoder now restores ChangesCookie archive restoration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The decoder now restores archived cookie fields, with coverage for complete and name-only cookies. No material merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: QUIET
Plan: Advanced
Run ID: c77cbcfb-608d-4993-9426-23d576841320
📒 Files selected for processing (2)
mParticle-Apple-SDK-Swift/Sources/DataModel/MPCookie.swiftmParticle-Apple-SDK-Swift/Test/Utils/MPDataModelTests.swift
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| /// The deleted Objective-C wrapper passed `NSDictionary` as the expected class for all three, | ||
| /// which never matches an `NSString` — so every restored cookie carried only its name. It went | ||
| /// unnoticed because nothing in the SDK archives a cookie (persistence writes raw sqlite | ||
| /// columns) and `isEqual(toCookie:)` compares names only, so the round-trip assertion in | ||
| /// `testCookie` passed over the loss. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove storage-implementation details from this documentation comment.
Keep the encoded-type mismatch and the correction. Remove the storage and prior-test details because they are not needed to document init(coder:).
As per path instructions, avoid internal architecture descriptions.
Source: Path instructions
75ec848 to
d2a6aa2
Compare
`MPCookie`'s `initWithCoder:` passed `NSDictionary` as the expected class for `content`, `domain` and `expiration`, all three of which are encoded as strings. The class check never matched, so the decode returned nil for each and every restored cookie carried only its name. It went unnoticed because nothing in the SDK archives a cookie — persistence writes raw sqlite columns — and `isEqual(toCookie:)` compares names only, so the round-trip assertion in `testCookie` passed straight over the loss. The three decodes now name `NSString`. Restoring the values is safe: they pass back through the same setters that escaped them on the way in, and `percentEscape()` only escapes `;` and space while leaving `%` untouched, so it is idempotent and the values do not double-escape. A restored cookie with a future `expiration` now also reports `expired == false` instead of `true`, which is the point of storing it. Split out of the migration PR deliberately: the migration ported this verbatim because repairing it changes what a decoded cookie contains, which is a behaviour change and belongs in its own entry. `testCookieArchiveRoundTripKeepsOnlyTheName`, which pinned the defect, becomes `testCookieArchiveRoundTripPreservesEveryField`, plus a name-only case covering the cookie that never had the optional fields. Verified: `abi-guard.sh check` exits 0, `trunk check` clean, `run-analyzer` filter yields 0 warnings, both test suites green — including `testCookie`, whose Objective-C assertions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d2a6aa2 to
f5407ad
Compare
2 participants
MPCookie'sinitWithCoder:passedNSDictionaryas the expected class forcontent,domainandexpiration— all three of which are encoded as strings:The class check never matched, so each decode returned nil and every restored cookie carried only its name.
Why it was never caught
No production path archives a cookie — cookies are written as raw sqlite columns, and the only production archives in the SDK are
MPUploadSettingsand a configuration dictionary.Cookies are archivable in principle:
MPConsumerInfoconforms toNSSecureCodingand encodes itscookiesarray, andMPConsumerInfoTests.testInstanceandtestConsumerInfoEncodingexercise that path. Neither asserts the cookie fields, though, andisEqual(toCookie:)compares names only — sotestCookie's round-trip assertion passed straight over the loss:The fix
The three decodes now name
NSString. Restoring the values is safe:percentEscape()escapes only;and space while leaving%untouched — so it is idempotent and the values do not double-escape.expirationnow reportsexpired == falseinstead oftrue, which is the point of storing it.Why this is separate from #978
#978 ported the coder verbatim precisely because repairing it changes what a decoded cookie contains — a behaviour change, not a migration. Keeping it here leaves #978 a pure
refactor:and gives the fix its own changelog entry.(Note the sibling defect in #977 was the opposite shape: there, unchecked
decodeObjectForKey:undersupportsSecureCoding == YESmade the decode path throw rather than lose data, so fixing it could not regress any working behaviour and it stayed in that PR.)Tests
testCookieArchiveRoundTripKeepsOnlyTheName, added by #978 to pin the defect, becomestestCookieArchiveRoundTripPreservesEveryField. Added a name-only case for the cookie that never had the optional fields, which must still restore and still read as expired.testCookie's Objective-C assertions are unchanged and still pass.Verification
Rebased with #978 onto
workstation/swift-migrationafter #975 and #977 landed, and re-run end to end:bash Tools/abi-guard.sh check→ exit 0trunk check→ clean (re-checked after committing)run-analyzer's exact filter chain → 0 surviving warnings🤖 Generated with Claude Code