Skip to content

refactor: write backend user attributes in Swift - #1060

Draft
thomson-t wants to merge 1 commit into
mainfrom
refactor/swift-backend-user-attributes
Draft

thomson-t wants to merge 1 commit into
mainfrom
refactor/swift-backend-user-attributes

Conversation

@thomson-t

Copy link
Copy Markdown
Contributor

Why

Reading and writing user attributes was the largest remaining block of logic in the backend controller — nine Objective-C methods, about 200 lines, covering everything an app does when it tags a user, sets or removes an attribute, or increments a counter. It is a customer-data path, and it was only reachable through the controller, so it could not be unit-tested on its own. After this change that logic is Swift with its own tests, and the Objective-C side only converts values at the boundary.

It also fixes a crash. The four key-taking methods called -mutableCopy on the key before checking it, and neither NSNull nor NSNumber conforms to NSMutableCopying, so a caller that put a non-string object behind the declared NSString * brought the process down. Those keys are now rejected with MPExecStatusMissingParam.

Programme

Continues the Objective-C to Swift migration described in docs/swift-migration/CONVERSION-RECIPE.md, whose compatibility inventory lists MPBackendController_PRIVATE as an internal wrapper-removal candidate (classification 2). Second slice of a planned sequence; the first was #1038.

What changes

Before, nine methods on MPBackendController read the stored attribute dictionary, validated keys and values, decided whether to store, delete or reject, wrote the result back through MPUserDefaults, and emitted the attribute-change message — all in Objective-C. After, each is a forwarder into a new Swift MPBackendUserAttributeWriter, with selectors, signatures and nullability unchanged: userAttributesForUserId:, logUserAttributeChange:, setUserAttributeChange:completionHandler:, incrementUserAttribute:byValue:, setUserTag:timestamp:completionHandler:, both setUserAttribute:… variants, removeUserAttribute:timestamp:completionHandler: and clearUserAttributes. The deletedUserAttributes set moves onto the writer, and its two upload-coordinator call sites move with it.

The new type follows the closure-bag shape the other backend components already use. It also adds MPExecStatusSwift, mirroring the Objective-C MPExecStatus so a Swift workflow can return a status the .m casts back. Those two enums are hand-maintained in different modules, because MPExecStatus is declared in a header the Swift module cannot import, so thirteen _Static_asserts pin every pair — drift is a build failure, not a silently wrong status on every attribute call.

Three things reviewers should look at, because they are where the judgement is:

  • One intentional behaviour change. A non-string key used to crash rather than return an error, as described under Why. The key parameters are therefore typed Any? rather than String?: a String? import has no safe representation for the NSNull a caller can still pass. The surviving half of the original guard, isKindOfClass:NSString.class, was unreachable, because -mutableCopy on a string returns an NSMutableString.
  • Three reads deliberately stay in Objective-C. The opt-out flag and the validate-and-log step are resolved by closures evaluated in the .m. MPStateMachine_PRIVATE is a Swift class, so Swift reads its properties directly instead of through objc_msgSend; moving the opt-out read to the Swift side made the partial mock in an existing Objective-C test invisible, and that test caught it. The validate-and-log step builds its message behind MPILogError's level gate, which must not become eager. mpId comes from MPPersistenceUtilities, which the Swift module cannot import.
  • Two unreachable branches were kept rather than deleted, both commented: a nil-key guard in incrementUserAttribute that cannot fire while the case-insensitive lookup falls back to the requested key, and a change-initialiser failure that the callers above already exclude. Both are noted as unreachable; the second reports the failure with the real key and value, where the original would have reported success and stored nothing.

Start at MPBackendUserAttributeWriter.swift and read it against the deleted bodies in the MPBackendController.m diff. The things to check are the order of operations in applyChange(_:) (validate, mutate, compute the stored form, log, write, report), the case-insensitive key fallback, and incrementUserAttribute, where the original re-read storage three times and called setUserAttributeChange: — which writes — before overwriting storage with a dictionary computed earlier.

No public API is added, so documentation and the changelog do not move with this; the changelog is generated from pull request titles at release time.

Linked work

Depends on: nothing; this is based on current main.
Unblocks: the remaining conversions in the same file — identities, attribute validation, attribution, startup and the dependency graph — each of which edits MPBackendController.m and so has to follow rather than run alongside.
Related: #1038 (the first slice). Two defects found while verifying this one are written up for separate work rather than fixed here — see the collapsed note below.

Rollout

Path: merging publishes nothing on its own; the change ships in the next release cut by the Release - Draft workflow, live rather than dark, with no ordering or timing constraint. The stored attribute format is unchanged — the same null sentinel, the same key — so no migration and nothing to reverse on disk.

Feature flags: none.

Turning it off: revert the pull request, which takes effect at the next release. A published version cannot be recalled.

What we watch: the size-report comment on this pull request for an unexpected binary-size move, and Tools/swift-migration-progress.sh, whose short-term in-scope percentage should rise by roughly 200 lines' worth. Both are read once, at review time. Nothing about production behaviour is intended to change except the crash described under Why.

Risks

  • A wire key, a write order or a nil check was translated wrongly, so attributes are stored or reported differently; the fifteen new Swift tests assert every status, both the store and the delete path, the null-sentinel round trip and all four increment cases, and the existing Objective-C tests for this surface were kept and pass; we would see it as wrong or missing user attributes on batches in the events pipeline.
  • The graceful rejection of a non-string key hides a caller error that used to be loud; it returns MPExecStatusMissingParam through the same completion handler the other validation failures use, so a caller that checks its status still sees it; we would see it as a partner reporting an attribute silently not set rather than a crash report.
  • A value an Objective-C test partial-mocks is read through Swift and the mock is bypassed, because Swift reads a Swift class's properties directly; this happened during the first slice and an existing test caught it, so every mocked value here is resolved by a closure evaluated in the .m; we would see it as an Objective-C contract test failing.
  • MPExecStatusSwift drifts from MPExecStatus and every status crossing the boundary becomes wrong; prevented outright by thirteen _Static_asserts, so drift cannot compile; we would see it at build time.
  • The timestamp threaded into the change message for an increment is not asserted by any test, because the fixture does not override the clock for that path; not addressed, as the increment's stored value and return value are covered and the timestamp is passed straight through; we would see it as an attribute-change message carrying the wrong event time.

Risk class: low. Stated for a change that does replace the sole write path for every app's user attributes, so the reasoning is worth being explicit about: the stored format, storage key and null sentinel are untouched, so nothing already on disk can be misread and there is nothing to migrate or roll back; the translation is covered both by fifteen new tests that assert each status and each storage outcome and by the pre-existing Objective-C tests for this surface, which were kept and pass; and the change is revertible by reverting the pull request, with no persisted state left behind. The one intentional behaviour change replaces a process-terminating crash with an error return, which cannot lose data that the previous behaviour preserved.

Who

Written by: an automated coding agent, working from a conversion plan for this file — derived from the compatibility inventory in docs/swift-migration/CONVERSION-RECIPE.md — that was reviewed and approved before any code was written.

Code reviewed before opening: an independent review agent reviewed the staged change twice. The first pass blocked it: a comment claimed the original guarded against NSNull keys, and it did not — it crashed. That was verified with a throwaway Objective-C program, the comment corrected, and the behaviour change called out here and in the commit message. The second pass passed with three advisories, all recorded in the collapsed note below.

Design reviewed before opening: the approach follows docs/swift-migration/CONVERSION-RECIPE.md and the shape of the backend conversions already merged; no separate design review.

Decision this implements: the classification of MPBackendController_PRIVATE as an internal wrapper-removal candidate, recorded in the compatibility inventory of docs/swift-migration/CONVERSION-RECIPE.md as of 2026-09-22.

Checked: all five items of docs/swift-migration/PR-GATE.md were run locally on the rebased tree, and the analyzer leg from the build workflow as well. Clean: iOS and tvOS builds with no new warnings, the analyzer with no warning lines at all, lint, the fifteen new Swift mirror tests, and the Objective-C behaviour-contract tests for this surface. Three items did not come back clean, all three for reasons that predate this branch and reproduce on unmodified main — the ABI guard, pod lib lint, and one flaky session test. Each is described in the collapsed note below with the evidence.

Not checked: the tvOS leg of the unit test suites, and the sample app against a proxy. The stored attribute format is unchanged, so neither is expected to be affected.

Size

Hand-written: 589 lines added and 168 removed, across 6 files. Of the additions, 303 are the new Swift type, 186 its tests, and 20 the status mirror; the rest is the fixture extension and the Objective-C forwarders.

Generated: none.

Why one pull request: the nine methods share a single mutation core, a single storage key and one case-insensitive lookup, and 186 of the added lines are the mirror test the dual-test convention requires for extracted logic. Splitting them would mean writing a temporary seam between the Swift and Objective-C halves of one attribute path and deleting it again in the next pull request, and would leave one commit carrying untested logic.

Notes for reviewers

Files changed:

  • mParticle-Apple-SDK-Swift/Sources/Backend/MPBackendUserAttributeWriter.swift — new, 303 lines
  • mParticle-Apple-SDK-Swift/Test/Backend/MPBackendUserAttributeWriterTests.swift — new, 186 lines, 15 tests
  • mParticle-Apple-SDK-Swift/Sources/Utils/MPExecStatusFormatter.swift — +20; the MPExecStatusSwift mirror, placed next to the existing formatter that already treats MPExecStatus as a raw Int
  • mParticle-Apple-SDK-Swift/Test/Backend/MPBackendSessionFixture.swift — +15; the shared fixture gains the attribute dependency bag, with a settable validation result and a recording list of validated keys
  • mParticle-Apple-SDK.xcodeproj/project.pbxproj — +2; the new test file added to both the iOS and tvOS synchronized-group membership exception sets, which is required or a file under Test/ can ship in the framework
  • mParticle-Apple-SDK/MPBackendController.m — +63/−168, a net −105; nine bodies replaced by forwarders, one lazy graph getter for the new component, and the thirteen _Static_asserts

Review advisories from the second pass, none blocking:

  1. The Objective-C forwarder setUserAttributeChange:completionHandler: now has no caller anywhere in the tree and is in no header. It is kept because it is one of the nine methods this slice scoped in; it should be deleted in the wrapper-removal pull request rather than carried further.
  2. No test asserts the timestamp threaded into the change message for an increment, because the fixture does not override the clock on that path. Recorded under Risks.
  3. The unreachable change-initialiser branch reports MPExecStatusMissingParam where the original would have reported success and written nothing — a second dead-branch divergence beyond the one behaviour change called out above. Harmless because unreachable, but named here for the record.

The three gate items that did not come back clean, all reproduced on unmodified main:

  • bash Tools/abi-guard.sh check fails with exactly two symbols, collectCustomModulePreferences and customModulePreferenceKeys. Both were added to Include/mParticle.h by fix: bound the preferences custom modules may read from the host app #1053 without regenerating Tools/abi-baseline.txt, so item 1 currently fails for every branch cut from main. This change adds no ABI diff of its own, and deliberately does not touch the baseline, because item 1 forbids folding an unrelated refresh into another pull request. Written up separately with the commands that prove provenance.

  • pod lib lint fails on all three podspecs with The iOS Simulator deployment target 'IPHONEOS_DEPLOYMENT_TARGET' is set to 13.0, but the range of supported deployment target versions is 15.0 to 27.0.x (in target 'RoktContracts' from project 'Pods'), and the same for tvOS. That setting comes from a transitive dependency's own podspec against the installed Xcode's floor, not from anything here. The identical failure was reproduced on a clean checkout of main.

  • Two session tests are flaky, and both assert on MPStateMachine_PRIVATE.currentSession — a weak mirror that a retired backend controller can publish to. Start-up work deferred by -startWithKey: holds its controller strongly and reads MParticle.sharedInstance when it runs, so after a reset or a workspace switch it can begin a session on a controller the SDK no longer uses. Measured:

    • MParticleTests.testInitStartsSessionSyncDisableAll — unmodified main fails the MPBackendControllerTests + MParticleTests pair in 2 of 4 runs (the 28.5s and 56.2s runs failed; 30.5s and 35.2s passed). Clearly pre-existing.
    • MPBackendControllerTests.testAutomaticSessionEnd — fails 1 of 2 runs on this branch and 0 of 2 on main. Those are the final counts; I stopped there. It is not deterministic on this branch either, and it reads the same mirror, so the same cause is the likely one — but two runs per tree does not rule out that this change raises the rate, so this one is unresolved and I am claiming neither that it is pre-existing nor that it is not. Worth more repetitions before anyone treats it as settled.

    A reproduction that fails deterministically in 0.5 seconds, plus two fix shapes that were tried and rejected, is written up separately. It is not fixed here: guarding the session-start choke point makes the reproduction pass but breaks ~38 assertions across MPBackendControllerTests, so a correct fix is a design decision in the start-up path and belongs in its own change.

Commands run on the rebased tree, with a concrete simulator id because OS=latest does not resolve locally:

bash Tools/abi-guard.sh check                                    # FAILED, only the two pre-existing symbols
trunk check                                                      # No issues
pod lib lint mParticle-Apple-SDK.podspec \
  --include-podspecs="{mParticle-Apple-SDK-Swift.podspec,mParticle-Apple-SDK-ObjC.podspec,mParticle-Apple-SDK.podspec}"
                                                                 # FAILED, pre-existing RoktContracts target floor
xcodebuild ... -scheme mParticle-Apple-SDK       -destination 'generic/platform=iOS'  build   # SUCCEEDED
xcodebuild ... -scheme mParticle-Apple-SDK       -destination 'generic/platform=tvOS' build   # SUCCEEDED
xcodebuild ... -scheme mParticle-Apple-SDK       -destination "id=<sim>" analyze               # 0 warning lines
xcodebuild ... -scheme mParticle-Apple-SDK-Swift -destination "id=<sim>" test \
  -only-testing:mParticle-Apple-SDK-SwiftTests/MPBackendUserAttributeWriterTests               # 15/15 passed
xcodebuild ... -scheme mParticle-Apple-SDK       -destination "id=<sim>" test \
  -only-testing:mParticle-Apple-SDKTests/MPBackendControllerTests \
  -only-testing:mParticle-Apple-SDKTests/MParticleUserTests \
  -only-testing:mParticle-Apple-SDKTests/MPUserIdentityChangeTests   # 101 tests, 1 failure: testAutomaticSessionEnd

Two notes for anyone re-running these. Two test runs pointed at the same simulator fail with Early unexpected exit, operation never finished bootstrapping, which looks like a crash but is contention — run them one at a time. And comparing a suite against main requires the same test count in both trees; adding a test to MPBackendControllerTests changes what runs before MParticleTests and invalidates the comparison, which is why the status mirror is pinned by _Static_assert rather than by a test in that suite.

Moves the user-attribute surface out of MPBackendController into
MPBackendUserAttributeWriter: the stored-form read, the five public mutations
(tag, scalar, value list, removal, increment), the shared mutation core, the
attribute-change message, and the set of keys deleted since the last upload.
All nine Objective-C methods keep their selectors and become forwarders.

Adds MPExecStatusSwift, mirroring the Objective-C MPExecStatus so a Swift
workflow can report a status the .m casts back at the boundary. MPExecStatus is
declared in Include/MPBackendController.h, which the Swift module cannot import,
so the two enums are hand-maintained in separate modules; thirteen
_Static_asserts pin every pair, making drift a build failure rather than a
silently wrong status.

Three reads stay on the Objective-C side. The opt-out flag and the
validate-and-log step are resolved by closures so they keep going through
objc_msgSend: the former reads MPStateMachine_PRIVATE, a Swift class whose
properties Swift would read directly and so bypass the partial mocks in the
Objective-C tests, and the latter builds its log message behind MPILogError's
level gate. mpId comes from MPPersistenceUtilities, which the Swift module
cannot import.

One intentional behaviour change: the key parameters are typed Any? and a
non-string key is now rejected with MPExecStatusMissingParam. The original
called -mutableCopy on the key before its own MPIsNull/isKindOfClass: guard, and
neither NSNull nor NSNumber conforms to NSMutableCopying, so a caller that put a
non-string object behind the declared NSString * raised
NSInvalidArgumentException and took the process down before the guard ran. Only
a literal nil ever reached it. A String? import has no safe representation for
the NSNull a caller can still pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

🐦 Swift Migration Progress

Production implementation code at 39f5a3eea8e2 compared with bb71b244a8ea.

Area Goal Progress Base This PR Swift SLOC Objective-C remaining Change
Core SDK Short term — in scope ████████▏░ 80.52% 81.16% 13,634 3,165 🚀 +0.64 pp
Core SDK Long term — all Objective-C █████▉░░░░ 58.16% 58.76% 13,634 9,570 🚀 +0.60 pp
SDK kit infrastructure Short term — in scope ████████▉░ 88.49% 88.49% 2,244 292 ➖ 0.00 pp
SDK kit infrastructure Long term — all Objective-C ████▌░░░░░ 45.00% 45.00% 2,244 2,743 ➖ 0.00 pp
Standalone kits Short term — in scope ▋░░░░░░░░░ 5.87% 5.87% 897 14,392 ➖ 0.00 pp
Standalone kits Long term — all Objective-C ▋░░░░░░░░░ 5.87% 5.87% 897 14,392 ➖ 0.00 pp

Objective-C retained by design: Core SDK 6,405 · SDK kit infrastructure 2,451 · Standalone kits 0.

This PR's code movement

Area Swift lines added Objective-C lines removed
Core SDK 323 168
SDK kit infrastructure 0 0
Standalone kits 0 0
How this is measured
  • Current composition uses production source lines of code (SLOC) from cloc; comments and blank lines are excluded.
  • Short term — in scope excludes the Objective-C the migration will not delete, so 100% completes the in-scope conversions: every in-scope implementation gone. Migration continues on main after the integration branch merge.
  • Long term — all Objective-C keeps the full denominator. Reaching 100% there means the public API itself becomes Swift, which is a breaking change reserved for a future major release.
  • The gap between the two rows is the retained public/kit contract, runtime-identity, and boundary-glue surface listed in Tools/swift-migration-retained-objc.txt.
  • Retained wrappers keep their Objective-C interface but still shed logic to Swift. That thinning moves the long-term row and the retained figure, not the short-term row.
  • Both revisions are measured with the manifest from the head revision, so a manifest edit does not by itself move the reported change. A retained file this pull request renamed or deleted still counts as retained at the base.
  • Pull request movement uses physical additions/deletions from git diff base...head --numstat; it counts retained files too and is intentionally separate from SLOC totals.
  • Core excludes SDK kit infrastructure and vendored libraries. Standalone kits include only files below Kits/**/Sources.
  • Tests, examples, headers, build outputs, vendored libraries, and the MParticle/Sources Swift overlay are excluded.
  • Objective-C++ (.mm) is included in the Objective-C figures and removed counts.

Generated with cloc 2.10. This report is informational and does not gate migration direction.

@github-actions

Copy link
Copy Markdown

📦 SDK Size Impact Report

Measures how much the SDK adds to an app's size (with-SDK minus without-SDK).

Metric Target Branch This PR Change
App Bundle Impact 2.69 MB 2.71 MB +24 KB
Executable Impact 848 bytes 848 bytes +N/A
Framework binary (ships) 2.96 MB 2.99 MB +24 KB

➡️ SDK size impact change is minimal.

Where the bytes are

Component Target branch This PR Change
Executable code (__text) 1188.0 KB 1196.9 KB +9.0 KB
Swift metadata (__swift5_*) 55.3 KB 56.3 KB +0.9 KB
ObjC metadata (__objc_*) 401.9 KB 404.0 KB +2.1 KB
Symbol tables (__LINKEDIT) 816.0 KB 832.0 KB +16.0 KB
Mach-O images 2 2 +0
Exported symbols 6026 6091 +65
Debug symbols (not shipped to users)

The SDK is embedded as a dynamic framework, so the app's own executable barely
moves and App Bundle Impact is the number that tracks shipped code.
xcodebuild -create-xcframework folds the dSYM in beside the framework, so the
xcframework total is mostly debug symbols and is not a shipping cost.

Target Branch This PR
dSYMs 3.73 MB 3.73 MB
XCFramework total 6.70 MB 6.72 MB
Raw measurements

Target branch (main):

{"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2836,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2752,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6860,"framework_size_kb":3036,"dsym_size_kb":3820}

This PR:

{"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2860,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2776,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6884,"framework_size_kb":3060,"dsym_size_kb":3820}

@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: QUIET

Plan: Enterprise

Run ID: 2e62bd7b-8608-4454-ba24-b58a4da2e6f0

📥 Commits

Reviewing files that changed from the base of the PR and between bb71b24 and 39f5a3e.

📒 Files selected for processing (6)
  • mParticle-Apple-SDK-Swift/Sources/Backend/MPBackendUserAttributeWriter.swift
  • mParticle-Apple-SDK-Swift/Sources/Utils/MPExecStatusFormatter.swift
  • mParticle-Apple-SDK-Swift/Test/Backend/MPBackendSessionFixture.swift
  • mParticle-Apple-SDK-Swift/Test/Backend/MPBackendUserAttributeWriterTests.swift
  • mParticle-Apple-SDK.xcodeproj/project.pbxproj
  • mParticle-Apple-SDK/MPBackendController.m

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a Swift writer for user-attribute reads, mutations, persistence, deletion tracking, and change logging. The Objective-C controller delegates these operations to the writer. A Swift execution-status enum and compile-time checks connect status values across the Swift and Objective-C boundary. Tests cover writer operations and outcomes.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to 39f5a

User-attribute logic moves from Objective-C to Swift, with the same selectors and stored format. The Objective-C suite has one failure the author has not explained, and the tvOS unit tests were not run. Confirm that failure is unrelated before merging.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant