Skip to content

Conversation

philprime
Copy link
Member

@philprime philprime commented Sep 11, 2025

Summary

This PR migrates the SentryMsgPackSerializer from Objective-C to Swift while maintaining 100% behavioral compatibility.

Changes Made

Complete Migration

  • Converted SentryMsgPackSerializer.m/.h to modular Swift implementation
  • Reorganized code into separate files in Sources/Swift/Tools/MsgPack/ directory:
    • SentryMsgPackSerializer.swift - Main serialization logic
    • SentryStreamable.swift - Protocol definition
    • SentryMsgPackSerializerError.swift - Error types
    • Data+SentryStreamable.swift - Data extension
    • URL+SentryStreamable.swift - URL extension
    • NSData+SentryStreamable.swift - NSData extension
    • NSURL+SentryStreamable.swift - NSURL extension

Behavioral Compatibility

  • Maintained exact behavior matching original Objective-C implementation
  • Preserved edge cases like silent key length truncation for >255 byte keys
  • Kept error handling patterns including -1 return values for file size errors
  • Maintained logging levels (error vs debug) matching original code

Enhanced Test Coverage

  • Added comprehensive test suite with 14 test cases covering all code paths
  • Added edge case testing for large dictionaries, long keys, stream errors
  • Improved error validation for nil input streams and invalid file paths
  • AAA test pattern with proper setup/teardown and temp file cleanup

Implementation Improvements

  • Modern Swift patterns with proper error throwing instead of boolean returns
  • Type safety with explicit error types via SentryMsgPackSerializerError
  • Memory safety improvements while maintaining compatibility
  • Cleaner byte operations using modern Swift APIs

Key Technical Details

  • Protocol signature: Uses streamSize() -> Int to support -1 error values from Objective-C
  • Truncating conversion: Uses UInt8(truncatingIfNeeded:) to match Objective-C silent truncation
  • Error propagation: Swift errors are caught and converted to boolean returns for Objective-C compatibility
  • File I/O: Improved path validation through Swift's Data.write(to:) method

Testing

All existing functionality verified through comprehensive test suite:

  • 14 tests covering all code paths and edge cases
  • 100% backward compatibility with existing behavior
  • Proper error handling for all failure scenarios

Closes #6140

#skip-changelog

…nverted tests

- Updated SentryMsgPackSerializer to log errors instead of debug messages for empty data and input stream issues.
- Modified the `asInputStream` method in the SentryStreamable protocol to return nullable streams.
- Removed outdated Objective-C tests and added comprehensive Swift tests for SentryMsgPackSerializer, covering various scenarios including nil input streams and invalid file paths.
- Ensured proper cleanup of temporary files in tests.
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 94.35897% with 11 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@3b059e5). Learn more about missing BASE report.
⚠️ Report is 9 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...s/Swift/Tools/MsgPack/NSURL+SentryStreamable.swift 75.000% 6 Missing ⚠️
.../Swift/Tools/MsgPack/SentryMsgPackSerializer.swift 96.703% 3 Missing ⚠️
...ces/Swift/Tools/MsgPack/URL+SentryStreamable.swift 89.473% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #6143   +/-   ##
========================================
  Coverage        ?   86.777%           
========================================
  Files           ?       442           
  Lines           ?     37263           
  Branches        ?     17425           
========================================
  Hits            ?     32336           
  Misses          ?      4883           
  Partials        ?        44           
Files with missing lines Coverage Δ
SentryTestUtils/TestStreamableObject.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryClient.m 98.941% <ø> (ø)
...es/Swift/Tools/MsgPack/Data+SentryStreamable.swift 100.000% <100.000%> (ø)
.../Swift/Tools/MsgPack/NSData+SentryStreamable.swift 100.000% <100.000%> (ø)
...ces/Swift/Tools/MsgPack/URL+SentryStreamable.swift 89.473% <89.473%> (ø)
.../Swift/Tools/MsgPack/SentryMsgPackSerializer.swift 96.703% <96.703%> (ø)
...s/Swift/Tools/MsgPack/NSURL+SentryStreamable.swift 75.000% <75.000%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b059e5...7726ae1. Read the comment docs.

Copy link
Contributor

github-actions bot commented Sep 11, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1229.65 ms 1263.41 ms 33.76 ms
Size 23.75 KiB 988.63 KiB 964.88 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
be9107c 1223.63 ms 1242.82 ms 19.19 ms
b193842 1231.36 ms 1247.69 ms 16.33 ms
884b224 1221.11 ms 1255.88 ms 34.77 ms
3279d4e 1215.76 ms 1256.45 ms 40.69 ms
ccf1278 1226.84 ms 1248.51 ms 21.67 ms
f2bfecd 1234.92 ms 1250.34 ms 15.42 ms
52603c5 1229.35 ms 1251.82 ms 22.47 ms
8fd192f 1202.10 ms 1220.19 ms 18.09 ms
d06a4db 1233.20 ms 1243.18 ms 9.98 ms
f43a482 1219.00 ms 1245.56 ms 26.56 ms

App size

Revision Plain With Sentry Diff
be9107c 23.75 KiB 975.19 KiB 951.44 KiB
b193842 23.75 KiB 920.65 KiB 896.90 KiB
884b224 23.75 KiB 879.55 KiB 855.80 KiB
3279d4e 23.75 KiB 938.32 KiB 914.57 KiB
ccf1278 23.75 KiB 877.15 KiB 853.40 KiB
f2bfecd 23.75 KiB 919.68 KiB 895.93 KiB
52603c5 23.75 KiB 969.66 KiB 945.92 KiB
8fd192f 23.74 KiB 872.75 KiB 849.01 KiB
d06a4db 23.75 KiB 913.18 KiB 889.43 KiB
f43a482 23.75 KiB 928.16 KiB 904.41 KiB

Previous results on branch: philprime/msg-pack-serializer-null-handling

Startup times

Revision Plain With Sentry Diff
43dc3b5 1236.69 ms 1255.65 ms 18.95 ms
8215a0d 1206.23 ms 1237.04 ms 30.81 ms

App size

Revision Plain With Sentry Diff
43dc3b5 23.75 KiB 988.55 KiB 964.80 KiB
8215a0d 23.75 KiB 969.21 KiB 945.46 KiB

@philprime philprime marked this pull request as draft September 11, 2025 11:32
…nal serialization tests

- Added support for error streams in TestStreamableObject.
- Introduced new test cases for serializing empty dictionaries, single elements, large dictionaries, long keys, and handling invalid paths.
- Implemented a custom ErrorInputStream to simulate read errors during serialization.
…ntation

- Deleted the Objective-C SentryMsgPackSerializer and its associated header files.
- Introduced a new Swift implementation of SentryMsgPackSerializer with improved error handling.
- Added SentryStreamable protocol and extensions for Data, NSData, NSURL, and URL to support serialization.
- Updated tests to validate the new Swift serialization logic and error handling.
@philprime philprime changed the title refactor: Add nullability-handling to SentryMsgPackSerializer with converted tests refactor: Migrate SentryMsgPackSerializer from Objective-C to Swift Sep 15, 2025
…r error propagation

- Changed keyData.withUnsafeBytes to use try for improved error handling.
- This ensures that any errors during buffer address retrieval are properly thrown.
@philprime
Copy link
Member Author

@cursor review

cursor[bot]

This comment was marked as outdated.

- Introduced TestStreamableObject to simulate various SentryStreamable behaviors, including handling nil and error streams.
- Updated SentryMsgPackSerializerTests to utilize TestStreamableObject for improved test coverage on serialization scenarios.
- Removed redundant TestStreamableObject implementation from SentryMsgPackSerializerTests to streamline code.
@philprime
Copy link
Member Author

@cursor review
@seer review

cursor[bot]

This comment was marked as outdated.

@philprime philprime marked this pull request as ready for review September 23, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix nullability in SentryMsgPackSerializer.m
1 participant