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.
@codecov
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

❌ Patch coverage is 95.83333% with 5 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@17a26bf). Learn more about missing BASE report.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../Swift/Tools/MsgPack/SentryMsgPackSerializer.swift 95.161% 3 Missing ⚠️
...ces/Swift/Tools/MsgPack/URL+SentryStreamable.swift 84.615% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #6143   +/-   ##
========================================
  Coverage        ?   85.198%           
========================================
  Files           ?       455           
  Lines           ?     27734           
  Branches        ?     12142           
========================================
  Hits            ?     23629           
  Misses          ?      4059           
  Partials        ?        46           
Files with missing lines Coverage Δ
SentryTestUtils/TestStreamableObject.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryClient.m 99.070% <ø> (ø)
...es/Swift/Tools/MsgPack/Data+SentryStreamable.swift 100.000% <100.000%> (ø)
Sources/Swift/Tools/SentryEnvelopeItem.swift 88.073% <100.000%> (ø)
...ces/Swift/Tools/MsgPack/URL+SentryStreamable.swift 84.615% <84.615%> (ø)
.../Swift/Tools/MsgPack/SentryMsgPackSerializer.swift 95.161% <95.161%> (ø)

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 17a26bf...014d2cf. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1210.81 ms 1244.58 ms 33.77 ms
Size 23.75 KiB 1.00 MiB 1004.87 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
995b436 1217.54 ms 1254.26 ms 36.72 ms
f37e3fe 1230.24 ms 1254.51 ms 24.27 ms
0ac4c65 1236.85 ms 1267.84 ms 30.98 ms
339539a 1219.58 ms 1254.63 ms 35.05 ms
ebc3a34 1236.24 ms 1261.02 ms 24.78 ms
781f560 1232.83 ms 1263.56 ms 30.73 ms
e98d6f5 1228.51 ms 1258.85 ms 30.34 ms
b709887 1193.52 ms 1216.74 ms 23.22 ms
37183fe 1212.33 ms 1238.92 ms 26.59 ms
3279d4e 1215.76 ms 1256.45 ms 40.69 ms

App size

Revision Plain With Sentry Diff
995b436 23.75 KiB 1.02 MiB 1016.76 KiB
f37e3fe 23.75 KiB 919.70 KiB 895.95 KiB
0ac4c65 23.75 KiB 968.24 KiB 944.50 KiB
339539a 23.75 KiB 968.24 KiB 944.50 KiB
ebc3a34 23.75 KiB 919.91 KiB 896.16 KiB
781f560 23.75 KiB 1.02 MiB 1016.46 KiB
e98d6f5 23.75 KiB 933.03 KiB 909.28 KiB
b709887 23.75 KiB 1.01 MiB 1016.14 KiB
37183fe 23.75 KiB 913.63 KiB 889.87 KiB
3279d4e 23.75 KiB 938.32 KiB 914.57 KiB

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

Startup times

Revision Plain With Sentry Diff
96bec69 1231.04 ms 1258.70 ms 27.65 ms
13910c9 1222.09 ms 1248.02 ms 25.93 ms
2c09b1c 1196.13 ms 1225.79 ms 29.66 ms
8215a0d 1206.23 ms 1237.04 ms 30.81 ms
c0e1523 1229.65 ms 1263.41 ms 33.76 ms
152d1b8 1217.40 ms 1247.78 ms 30.38 ms
43dc3b5 1236.69 ms 1255.65 ms 18.95 ms

App size

Revision Plain With Sentry Diff
96bec69 23.75 KiB 1.02 MiB 1020.10 KiB
13910c9 23.74 KiB 1.00 MiB 1002.97 KiB
2c09b1c 23.75 KiB 992.26 KiB 968.52 KiB
8215a0d 23.75 KiB 969.21 KiB 945.46 KiB
c0e1523 23.75 KiB 988.63 KiB 964.88 KiB
152d1b8 23.75 KiB 1.01 MiB 1011.93 KiB
43dc3b5 23.75 KiB 988.55 KiB 964.80 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
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@philprime philprime requested a review from noahsmartin October 9, 2025 11:58
@philprime
Copy link
Member Author

@sentry review

cursor[bot]

This comment was marked as outdated.

…ror handling

- Updated `serializeDictionary` method to directly serialize to a file.
- Introduced `serializeToFile` method for better separation of concerns.
- Enhanced error handling to clean up partial files on serialization failure.
- Updated tests to validate new serialization approach and error scenarios.
cursor[bot]

This comment was marked as outdated.

@cursor
Copy link

cursor bot commented Oct 23, 2025

Bug: Logging Level Change Causes Unnecessary Alerts

The logging level for file attribute read errors changed from DEBUG to ERROR. This goes against the PR's goal of maintaining original logging levels and may cause unwanted log spam or trigger monitoring alerts for expected failure cases.

Fix in Cursor Fix in Web

@philprime philprime marked this pull request as draft October 24, 2025 15:31
- Remove @objc attribute (only used from Swift)
- Mark class as final
- Change parameter type from [String: Any] to [String: SentryStreamable]
- Use Data(key.utf8) instead of optional conversion
- Use NSSwapHostIntToBig for byte order conversion to match ObjC
- Change error logs to debug in URL+SentryStreamable to match ObjC behavior
- Fix SwiftLint force unwrapping warning
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.

Fix nullability in SentryMsgPackSerializer.m

3 participants