Skip to content

refactor: move attribute projection parsing to Swift - #927

Merged
nickolas-dimitrakas merged 1 commit into
workstation/swift-migrationfrom
refactor/attribute-projection-parsing-to-swift
Sep 3, 2026
Merged

nickolas-dimitrakas merged 1 commit into
workstation/swift-migrationfrom
refactor/attribute-projection-parsing-to-swift

Conversation

@nickolas-dimitrakas

@nickolas-dimitrakas nickolas-dimitrakas commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Background

MPAttributeProjection was the last unmigrated file in the projection/filter series, duplicating navigation logic MPProjectionFieldParser already implements.

What Has Changed

  • Added attributeProjectionFields(from:attributeIndex:) to MPProjectionFieldParser.swift, reusing the existing private navigation so the two readers can't drift; coerces string-encoded data_type/is_required via MPJSONCoercion.
  • Fixed a crash: a null entry in attribute_maps hit -[NSNull objectForKeyedSubscript:]; unreadable entries now fall back to the existing absent-key defaults.
  • Added a bounds guard in configureProjections: for the unvalidated remote message_type index — preserves the existing (working) off-by-one that lets Media projections register, pinned by a new test.
  • -setDataType:'s range clamp stays in ObjC (asserted by an existing test).

Validation

  • abi-guard PASSED (no public ABI diff)
  • trunk check clean
  • ObjC 1004 / Swift 629 tests green on both schemes
  • tvOS: CI only (no local simulator runtime)

Checklist

  • Self-review completed
  • Tests added or updated
  • Tested locally

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

🐦 Swift Migration Progress

Production implementation code at 013280fb5774 compared with bbb0af88e5ed.

Area Goal Progress Base This PR Swift SLOC Objective-C remaining Change
Core SDK Short term — in scope █████▏░░░░ 51.43% 51.43% 7,893 7,454 ➖ 0.00 pp
Core SDK Long term — all Objective-C ███▊░░░░░░ 37.32% 37.32% 7,893 13,256 ➖ 0.00 pp
SDK kit infrastructure Short term — in scope ██████▊░░░ 66.70% 66.96% 2,021 997 🚀 +0.26 pp
SDK kit infrastructure Long term — all Objective-C ████░░░░░░ 39.57% 39.95% 2,021 3,038 🚀 +0.38 pp
Standalone kits Short term — in scope ▏░░░░░░░░░ 1.06% 1.06% 152 14,218 ➖ 0.00 pp
Standalone kits Long term — all Objective-C ▏░░░░░░░░░ 1.06% 1.06% 152 14,218 ➖ 0.00 pp

Objective-C retained by design: Core SDK 5,802 · SDK kit infrastructure 2,041 · Standalone kits 0.

This PR's code movement

Area Swift lines added Objective-C lines removed
Core SDK 0 0
SDK kit infrastructure 55 10
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% is the end of this project: every in-scope implementation gone.
  • 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

github-actions Bot commented Sep 2, 2026

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.46 MB 2.47 MB +16 KB
Executable Impact 848 bytes 848 bytes +N/A
XCFramework Size 6.80 MB 6.82 MB +16 KB

➡️ SDK size impact change is minimal.

Raw measurements

Target branch (workstation/swift-migration):

{"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":2600,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2516,"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":2616,"with_sdk_executable_size_bytes":76312,"sdk_impact_kb":2532,"sdk_executable_impact_bytes":848,"xcframework_size_kb":6984}

@nickolas-dimitrakas nickolas-dimitrakas self-assigned this Sep 2, 2026
@nickolas-dimitrakas
nickolas-dimitrakas force-pushed the refactor/attribute-projection-parsing-to-swift branch from 95134f2 to ad8a9e1 Compare September 2, 2026 21:03
@nickolas-dimitrakas
nickolas-dimitrakas marked this pull request as ready for review September 3, 2026 18:41
@nickolas-dimitrakas
nickolas-dimitrakas requested a review from a team as a code owner September 3, 2026 18:41
@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches kit projection registration and remote-config parsing paths that drive event routing; behavior is mostly defensive but preserves a deliberate Media message-type quirk.

Overview
Moves data_type / is_required parsing for attribute projections out of MPAttributeProjection into MPProjectionFieldParser.attributeProjectionFields, shared with the existing attribute-map navigation so the two code paths stay aligned. MPAttributeProjection now applies those fields via the existing setDataType: clamp.

Fixes crashes on malformed remote config: null or non-dictionary attribute_maps entries no longer trigger -[NSNull objectForKeyedSubscript:]; they fall back to string type and not required, matching the old absent-key behavior.

configureProjections: now skips projections whose message_type is above messageTypeSize (logs and continues) so bogus indices cannot raise NSRangeException, while still allowing index messageTypeSize so Media projections keep the historical off-by-one append behavior. New ObjC/Swift tests cover null maps, invalid attribute_maps, and in/out-of-range message types.

Reviewed by Cursor Bugbot for commit 013280f. Bugbot is set up for automated code reviews on this repo. Configure here.

MPAttributeProjection was the one file the kit projection series never opened —
no Swift import, unchanged since 2018 — and it still walked
configuration["action"]["attribute_maps"][index] itself, duplicating navigation
MPProjectionFieldParser already does with type guards.

data_type and is_required now come from
+attributeProjectionFieldsFromConfiguration:attributeIndex:, which reuses the
same private navigation as attributeFieldsFromAction:attributeIndex: so the two
readers cannot drift, and goes through MPJSONCoercion so a string-encoded value
behaves as -integerValue and -boolValue did.

-setDataType: keeps its range clamp and stays the single source of it; Swift
returns the raw value.

Fixes a crash reachable from remote configuration: attribute_maps: [null]
crashed with -[NSNull objectForKeyedSubscript:], because the element was
subscripted with no guard on the container, the index, or the element type.
Unreadable entries now fall back to the same defaults the absent-key path
always used.

Also bounds-guards configureProjections:, where message_type is unvalidated
remote configuration used as an array index. Note that
-setObject:atIndexedSubscript: allows index == count and appends, so
MPMessageTypeMedia (20) has always been registered into a 21st slot even though
+[MPEnum messageTypeSize] is 20 — that case is deliberately preserved, and a
test pins it so the guard cannot regress Media projections. Only a larger
value, or a negative one that wrapped, raises NSRangeException; those are now
skipped and logged. The messageTypeSize/MPMessageTypeMedia off-by-one itself is
left alone: making Media an explicit slot is a behaviour change that needs
product input.

Not done: the dead-looking early return in -setFilters:. `filters` is a
readwrite property, so that short-circuit is reachable by any holder of the
header, not dead code.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@nickolas-dimitrakas
nickolas-dimitrakas force-pushed the refactor/attribute-projection-parsing-to-swift branch from ad8a9e1 to 013280f Compare September 3, 2026 19:28
@nickolas-dimitrakas
nickolas-dimitrakas merged commit 7982b6e into workstation/swift-migration Sep 3, 2026
78 of 79 checks passed
@nickolas-dimitrakas
nickolas-dimitrakas deleted the refactor/attribute-projection-parsing-to-swift branch September 3, 2026 19:30
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.

2 participants