refactor: Migrate SentryMsgPackSerializer from Objective-C to Swift #6143
Conversation
…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.
philprime
requested review from
itaybre,
noahsmartin and
philipphofmann
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6143 +/- ##
========================================
Coverage ? 86.828%
========================================
Files ? 442
Lines ? 37345
Branches ? 17399
========================================
Hits ? 32426
Misses ? 4644
Partials ? 275
Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6e99155 | 1223.96 ms | 1249.25 ms | 25.29 ms |
| 0529194 | 1237.23 ms | 1254.67 ms | 17.44 ms |
| 22b6996 | 1234.00 ms | 1263.24 ms | 29.24 ms |
| 13bc1aa | 1244.04 ms | 1262.89 ms | 18.85 ms |
| 01faa71 | 1238.81 ms | 1263.98 ms | 25.17 ms |
| fac4ca3 | 1222.81 ms | 1235.83 ms | 13.02 ms |
| 7eafbde | 1212.16 ms | 1243.64 ms | 31.48 ms |
| f2bfecd | 1234.92 ms | 1250.34 ms | 15.42 ms |
| 4d264fa | 1223.48 ms | 1246.91 ms | 23.44 ms |
| 2a07609 | 1207.79 ms | 1233.77 ms | 25.98 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6e99155 | 23.75 KiB | 963.18 KiB | 939.43 KiB |
| 0529194 | 23.74 KiB | 891.02 KiB | 867.28 KiB |
| 22b6996 | 23.75 KiB | 908.02 KiB | 884.27 KiB |
| 13bc1aa | 23.75 KiB | 908.40 KiB | 884.65 KiB |
| 01faa71 | 23.75 KiB | 926.77 KiB | 903.02 KiB |
| fac4ca3 | 23.75 KiB | 902.01 KiB | 878.27 KiB |
| 7eafbde | 23.75 KiB | 927.53 KiB | 903.78 KiB |
| f2bfecd | 23.75 KiB | 919.68 KiB | 895.93 KiB |
| 4d264fa | 23.74 KiB | 874.07 KiB | 850.33 KiB |
| 2a07609 | 23.75 KiB | 912.78 KiB | 889.03 KiB |
Previous results on branch: philprime/msg-pack-serializer-null-handling
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8215a0d | 1206.23 ms | 1237.04 ms | 30.81 ms |
| c0e1523 | 1229.65 ms | 1263.41 ms | 33.76 ms |
| 43dc3b5 | 1236.69 ms | 1255.65 ms | 18.95 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8215a0d | 23.75 KiB | 969.21 KiB | 945.46 KiB |
| c0e1523 | 23.75 KiB | 988.63 KiB | 964.88 KiB |
| 43dc3b5 | 23.75 KiB | 988.55 KiB | 964.80 KiB |
…rializer-null-handling
…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
changed the title
…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.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- 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.
…rializer-null-handling
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rializer-null-handling
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sentry review |
| return nil | ||
| } | ||
| return fileSize.uintValue | ||
| } |
There was a problem hiding this comment.
Bug: Inconsistent Error Handling in Streamable Protocol
The SentryStreamable.streamSize() protocol method returns UInt? instead of NSInteger. This prevents signaling errors with negative values (like -1), which existing serializer logic expects. For example, the URL extension returns nil for file read errors, but the serializer expects a negative value to indicate an issue, breaking error handling compatibility.
Additional Locations (1)
| // this needs to be re-evaluated. | ||
| SentrySDKLog.error("Data for MessagePack dictionary has no content - Input: \(value)") | ||
| throw SentryMsgPackSerializerError.emptyData("Empty data for MessagePack dictionary") | ||
| } |
There was a problem hiding this comment.
Bug: Serializer Fails on Empty Streams
The SentryMsgPackSerializer incorrectly rejects valid zero-length data streams, treating them as errors. This diverges from the Objective-C implementation, breaking backward compatibility and causing SentrySDKLog.error messages for what should be non-error scenarios.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
|
|
||
| let mapHeader = UInt8(0x80 | dictionary.count) // Map up to 15 elements | ||
| _ = outputStream.write([mapHeader], maxLength: 1) | ||
|
|
There was a problem hiding this comment.
The map header implementation silently truncates dictionaries with more than 15 elements by using UInt8(0x80 | dictionary.count). This produces incorrect MessagePack format for dictionaries with 16+ elements. Consider either:
- Using fixmap16 (0xDE) or fixmap32 (0xDF) format for larger dictionaries
- Validating dictionary size and throwing an error if it exceeds 15 elements
- Adding explicit documentation/warning about this limitation
The current silent truncation can lead to data corruption that's difficult to debug.
| // Validate dictionary size for proper MessagePack format | |
| if dictionary.count > 15 { | |
| throw SentryMsgPackSerializerError.dictionaryTooLarge | |
| } | |
| let mapHeader = UInt8(0x80 | dictionary.count) // fixmap format for up to 15 elements |
Did we get this right? 👍 / 👎 to inform future reviews.
| let keyLength = UInt8(truncatingIfNeeded: keyData.count) // Truncates if > 255, matching Objective-C behavior | ||
| _ = outputStream.write([str8Header], maxLength: 1) | ||
| _ = outputStream.write([keyLength], maxLength: 1) | ||
|
|
There was a problem hiding this comment.
Key length is silently truncated to UInt8 using truncatingIfNeeded when the UTF-8 encoded key exceeds 255 bytes. This will corrupt the MessagePack output if a key is longer than 255 bytes. Consider validating the key length and throwing an error instead of silently truncating.
Note: A key with 255 UTF-8 characters might actually have more than 255 bytes if it contains multi-byte Unicode characters.
| let str8Header: UInt8 = 0xD9 // String up to 255 bytes | |
| if keyData.count > 255 { | |
| throw SentryMsgPackSerializerError.invalidInput("Key exceeds 255 bytes: \(key)") | |
| } | |
| let keyLength = UInt8(keyData.count) |
Did we get this right? 👍 / 👎 to inform future reviews.
| let mapHeader = UInt8(0x80 | dictionary.count) // Map up to 15 elements | ||
| _ = outputStream.write([mapHeader], maxLength: 1) | ||
|
|
||
| for (key, anyValue) in dictionary { |
There was a problem hiding this comment.
Stream write operations discard the return value which indicates the number of bytes actually written. If the write fails or writes fewer bytes than expected, this will silently corrupt the MessagePack data. Consider checking write return values and throwing errors on failure.
| for (key, anyValue) in dictionary { | |
| let bytesWritten = outputStream.write([mapHeader], maxLength: 1) | |
| if bytesWritten != 1 { | |
| throw SentryMsgPackSerializerError.outputError("Failed to write map header") | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| throw SentryMsgPackSerializerError.invalidInput("Could not get buffer address for key: \(key)") | ||
| } | ||
| _ = outputStream.write(bufferAddress, maxLength: keyData.count) | ||
| } | ||
|
|
||
| guard let dataLength = value.streamSize(), dataLength > 0 else { | ||
| // MsgPack is being used strictly for session replay. |
There was a problem hiding this comment.
The keyData.withUnsafeBytes closure writes to the output stream, but if the stream is in an error state or closed, this could lead to undefined behavior. Consider checking the stream status before writing.
Did we get this right? 👍 / 👎 to inform future reviews.
| let valueLength = UInt32(truncatingIfNeeded: dataLength) | ||
| // We will always use the 4 bytes data length for simplicity. | ||
| // Worst case we're losing 3 bytes. | ||
| let bin32Header: UInt8 = 0xC6 |
There was a problem hiding this comment.
Value length is truncated to UInt32 using truncatingIfNeeded when streamSize exceeds UInt32.max (4GB). This will cause a mismatch between the declared length in the MessagePack header and the actual data length, leading to corrupted data. Consider validating the size and throwing an error for excessively large values.
| let bin32Header: UInt8 = 0xC6 | |
| if dataLength > UInt32.max { | |
| throw SentryMsgPackSerializerError.invalidInput("Data size exceeds UInt32.max: \(dataLength)") | |
| } | |
| let valueLength = UInt32(dataLength) |
Did we get this right? 👍 / 👎 to inform future reviews.
| if bytesRead > 0 { | ||
| _ = outputStream.write(buffer, maxLength: bytesRead) |
There was a problem hiding this comment.
Stream write operations inside the while loop don't check the return value, which could lead to partial writes and corrupted MessagePack data. The loop should verify that all bytes are written successfully.
| if bytesRead > 0 { | |
| _ = outputStream.write(buffer, maxLength: bytesRead) | |
| let bytesWritten = outputStream.write(buffer, maxLength: bytesRead) | |
| if bytesWritten != bytesRead { | |
| throw SentryMsgPackSerializerError.outputError("Failed to write all bytes: wrote \(bytesWritten) of \(bytesRead)") | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| // Maintains Objective-C behavior: allows long keys but length will be truncated to uint8_t | ||
| XCTAssertTrue(result) | ||
| let tempFile = try Data(contentsOf: tempFileURL) | ||
| XCTAssertGreaterThan(tempFile.count, 1) | ||
| } | ||
|
|
There was a problem hiding this comment.
The test testSerializeLongKey verifies that keys longer than 255 characters succeed with truncation, but this actually produces invalid MessagePack. The test should verify that such keys either fail with an error or are properly encoded using str16 or str32 formats.
| // Maintains Objective-C behavior: allows long keys but length will be truncated to uint8_t | |
| XCTAssertTrue(result) | |
| let tempFile = try Data(contentsOf: tempFileURL) | |
| XCTAssertGreaterThan(tempFile.count, 1) | |
| } | |
| // Act | |
| let result = SentryMsgPackSerializer.serializeDictionary(toMessagePack: dictionary, intoFile: tempFileURL) | |
| // Assert | |
| // Should fail for keys longer than 255 bytes with current str8 implementation | |
| XCTAssertFalse(result) |
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
Summary
This PR migrates the SentryMsgPackSerializer from Objective-C to Swift while maintaining 100% behavioral compatibility.
Changes Made
Complete Migration
SentryMsgPackSerializer.m/.hto modular Swift implementationSources/Swift/Tools/MsgPack/directory:SentryMsgPackSerializer.swift- Main serialization logicSentryStreamable.swift- Protocol definitionSentryMsgPackSerializerError.swift- Error typesData+SentryStreamable.swift- Data extensionURL+SentryStreamable.swift- URL extensionNSData+SentryStreamable.swift- NSData extensionNSURL+SentryStreamable.swift- NSURL extensionBehavioral Compatibility
Enhanced Test Coverage
Implementation Improvements
SentryMsgPackSerializerErrorKey Technical Details
streamSize() -> Intto support -1 error values from Objective-CUInt8(truncatingIfNeeded:)to match Objective-C silent truncationData.write(to:)methodTesting
All existing functionality verified through comprehensive test suite:
Closes #6140
#skip-changelog