Skip to content

Commit

Permalink
Merge 5f63edf into f3b8999
Browse files Browse the repository at this point in the history
  • Loading branch information
philipphofmann authored Jan 16, 2025
2 parents f3b8999 + 5f63edf commit 04e8394
Show file tree
Hide file tree
Showing 7 changed files with 299 additions and 2 deletions.
28 changes: 28 additions & 0 deletions Sentry.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@
33EB2A922C341300004FED3D /* Sentry.h in Headers */ = {isa = PBXBuildFile; fileRef = 63AA76931EB9C1C200D153DE /* Sentry.h */; settings = {ATTRIBUTES = (Public, ); }; };
51B15F7E2BE88A7C0026A2F2 /* URLSessionTaskHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 51B15F7D2BE88A7C0026A2F2 /* URLSessionTaskHelper.swift */; };
51B15F802BE88D510026A2F2 /* URLSessionTaskHelperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 51B15F7F2BE88D510026A2F2 /* URLSessionTaskHelperTests.swift */; };
620078722D38F00D0022CB67 /* SentryGeoCodable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 620078712D38F00D0022CB67 /* SentryGeoCodable.swift */; };
620078742D38F0DF0022CB67 /* SentryCodable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 620078732D38F0DF0022CB67 /* SentryCodable.swift */; };
620078782D3906BF0022CB67 /* SentryCodableTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 620078772D3906BF0022CB67 /* SentryCodableTests.swift */; };
620203B22C59025E0008317C /* SentryFileContents.swift in Sources */ = {isa = PBXBuildFile; fileRef = 620203B12C59025E0008317C /* SentryFileContents.swift */; };
620379DB2AFE1415005AC0C1 /* SentryBuildAppStartSpans.h in Headers */ = {isa = PBXBuildFile; fileRef = 620379DA2AFE1415005AC0C1 /* SentryBuildAppStartSpans.h */; };
620379DD2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m in Sources */ = {isa = PBXBuildFile; fileRef = 620379DC2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m */; };
Expand Down Expand Up @@ -1089,6 +1092,9 @@
33EB2A8F2C3411AE004FED3D /* SentryWithoutUIKit.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryWithoutUIKit.h; path = Public/SentryWithoutUIKit.h; sourceTree = "<group>"; };
51B15F7D2BE88A7C0026A2F2 /* URLSessionTaskHelper.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = URLSessionTaskHelper.swift; sourceTree = "<group>"; };
51B15F7F2BE88D510026A2F2 /* URLSessionTaskHelperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = URLSessionTaskHelperTests.swift; sourceTree = "<group>"; };
620078712D38F00D0022CB67 /* SentryGeoCodable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryGeoCodable.swift; sourceTree = "<group>"; };
620078732D38F0DF0022CB67 /* SentryCodable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCodable.swift; sourceTree = "<group>"; };
620078772D3906BF0022CB67 /* SentryCodableTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryCodableTests.swift; sourceTree = "<group>"; };
620203B12C59025E0008317C /* SentryFileContents.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryFileContents.swift; sourceTree = "<group>"; };
620379DA2AFE1415005AC0C1 /* SentryBuildAppStartSpans.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryBuildAppStartSpans.h; path = include/SentryBuildAppStartSpans.h; sourceTree = "<group>"; };
620379DC2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryBuildAppStartSpans.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2131,6 +2137,23 @@
path = ViewHierarchy;
sourceTree = "<group>";
};
620078752D38F1110022CB67 /* Codable */ = {
isa = PBXGroup;
children = (
620078712D38F00D0022CB67 /* SentryGeoCodable.swift */,
620078732D38F0DF0022CB67 /* SentryCodable.swift */,
);
path = Codable;
sourceTree = "<group>";
};
620078762D3906AD0022CB67 /* Codable */ = {
isa = PBXGroup;
children = (
620078772D3906BF0022CB67 /* SentryCodableTests.swift */,
);
path = Codable;
sourceTree = "<group>";
};
621D9F2D2B9B030E003D94DE /* Helper */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -2902,6 +2925,7 @@
7B3D0474249A3D5800E106B6 /* Protocol */ = {
isa = PBXGroup;
children = (
620078762D3906AD0022CB67 /* Codable */,
7BC6EBF7255C05060059822A /* TestData.swift */,
7B869EBB249B91D8004F4FDB /* SentryDebugMetaEquality.swift */,
7B869EBD249B964D004F4FDB /* SentryThreadEquality.swift */,
Expand Down Expand Up @@ -3996,6 +4020,7 @@
D8F016B12B9622B7007B9AFB /* Protocol */ = {
isa = PBXGroup;
children = (
620078752D38F1110022CB67 /* Codable */,
64F9571C2D12DA1800324652 /* SentryViewControllerBreadcrumbTracking.swift */,
D8F016B22B9622D6007B9AFB /* SentryId.swift */,
D8CAC0402BA0984500E38F34 /* SentryIntegrationProtocol.swift */,
Expand Down Expand Up @@ -4648,6 +4673,7 @@
7B7A30C824B48389005A4C6E /* SentryCrashWrapper.m in Sources */,
D8CAC0732BA4473000E38F34 /* SentryViewPhotographer.swift in Sources */,
D8ACE3C92762187200F5A213 /* SentryFileIOTrackingIntegration.m in Sources */,
620078742D38F0DF0022CB67 /* SentryCodable.swift in Sources */,
63FE713B20DA4C1100CDBAE8 /* SentryCrashFileUtils.c in Sources */,
63FE716920DA4C1100CDBAE8 /* SentryCrashStackCursor.c in Sources */,
6221BBCA2CAA932100C627CA /* SentryANRType.swift in Sources */,
Expand All @@ -4670,6 +4696,7 @@
8459FCC02BD73EB20038E9C9 /* SentryProfilerSerialization.mm in Sources */,
621C884A2CAD23E9000EABCB /* SentryCaptureTransactionWithProfile.mm in Sources */,
63EED6C02237923600E02400 /* SentryOptions.m in Sources */,
620078722D38F00D0022CB67 /* SentryGeoCodable.swift in Sources */,
D8CB741B2947286500A5F964 /* SentryEnvelopeItemHeader.m in Sources */,
D8CB7417294724CC00A5F964 /* SentryEnvelopeAttachmentHeader.m in Sources */,
D84793262788737D00BE8E99 /* SentryByteCountFormatter.m in Sources */,
Expand Down Expand Up @@ -5168,6 +5195,7 @@
D8CB742B294B1DD000A5F964 /* SentryUIApplicationTests.swift in Sources */,
63FE720920DA66EC00CDBAE8 /* XCTestCase+SentryCrash.m in Sources */,
D8918B222849FA6D00701F9A /* SentrySDKIntegrationTestsBase.swift in Sources */,
620078782D3906BF0022CB67 /* SentryCodableTests.swift in Sources */,
7B85BD8E24C5C3A6000A4225 /* SentryFileManagerTestExtension.swift in Sources */,
7B0002342477F52D0035FEF1 /* SentrySessionTests.swift in Sources */,
);
Expand Down
18 changes: 16 additions & 2 deletions Sources/Sentry/SentryGeo.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#import "SentryGeo.h"

#import "SentrySwift.h"
#import <Foundation/Foundation.h>

NS_ASSUME_NONNULL_BEGIN
Expand All @@ -21,7 +21,21 @@ - (id)copyWithZone:(nullable NSZone *)zone

- (NSDictionary<NSString *, id> *)serialize
{
return @{ @"city" : self.city, @"country_code" : self.countryCode, @"region" : self.region };
NSMutableDictionary *serializedData = [[NSMutableDictionary alloc] init];

if (self.city) {
[serializedData setValue:self.city forKey:@"city"];
}

if (self.countryCode) {
[serializedData setValue:self.countryCode forKey:@"country_code"];
}

if (self.region) {
[serializedData setValue:self.region forKey:@"region"];
}

return serializedData;
}

- (BOOL)isEqual:(id _Nullable)other
Expand Down
15 changes: 15 additions & 0 deletions Sources/Swift/Protocol/Codable/SentryCodable.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import Foundation

func decodeFromJSONData<T: Decodable>(jsonData: Data) -> T? {
if jsonData.isEmpty {
return nil
}

do {
let decoder = JSONDecoder()
return try decoder.decode(T.self, from: jsonData)
} catch {
SentryLog.error("Could not decode object of type \(T.self) from JSON data due to error: \(error)")
return nil
}
}
19 changes: 19 additions & 0 deletions Sources/Swift/Protocol/Codable/SentryGeoCodable.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
@_implementationOnly import _SentryPrivate
import Foundation

extension Geo: Decodable {

private enum CodingKeys: String, CodingKey {
case city
case countryCode = "country_code"
case region
}

required convenience public init(from decoder: any Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
self.init()
self.city = try container.decodeIfPresent(String.self, forKey: .city)
self.countryCode = try container.decodeIfPresent(String.self, forKey: .countryCode)
self.region = try container.decodeIfPresent(String.self, forKey: .region)
}
}
28 changes: 28 additions & 0 deletions Tests/SentryTests/Protocol/Codable/SentryCodableTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
@testable import Sentry
import XCTest

class SentryCodableTests: XCTestCase {

func testDecodeWithEmptyData_ReturnsNil() {
XCTAssertNil(decodeFromJSONData(jsonData: Data()) as Geo?)
}

func testDecodeWithGarbageData_ReturnsNil() {
let data = "garbage".data(using: .utf8)!
XCTAssertNil(decodeFromJSONData(jsonData: data) as Geo?)
}

func testDecodeWithWrongJSON_ReturnsEmptyObject() {
let wrongJSON = "{\"wrong\": \"json\"}".data(using: .utf8)!
let actual = decodeFromJSONData(jsonData: wrongJSON) as Geo?
let expected = Geo()

XCTAssertEqual(expected, actual)
}

func testDecodeWithBrokenJSON_ReturnsNil() {
let brokenJSON = "{\"broken\": \"json\"".data(using: .utf8)!
XCTAssertNil(decodeFromJSONData(jsonData: brokenJSON) as Geo?)
}

}
43 changes: 43 additions & 0 deletions Tests/SentryTests/Protocol/SentryGeoTests.swift
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
@testable import Sentry
import XCTest

class SentryGeoTests: XCTestCase {

func testSerializationWithAllProperties() throws {
let geo = try XCTUnwrap(TestData.geo.copy() as? Geo)
let actual = geo.serialize()
Expand All @@ -15,6 +17,47 @@ class SentryGeoTests: XCTestCase {
XCTAssertEqual(TestData.geo.region, actual["region"] as? String)
}

func testSerialization_WithAllPropertiesNil() throws {
let geo = Geo()

let actual = geo.serialize()

XCTAssertNil(actual["city"])
XCTAssertNil(actual["country_code"])
XCTAssertNil(actual["region"])
}

func testSerialization_WithEmptyString() throws {
let geo = Geo()
geo.city = ""

let actual = geo.serialize()

XCTAssertEqual("", actual["city"] as? String)
XCTAssertNil(actual["country_code"])
XCTAssertNil(actual["region"])
}

func testDecodeWithAllProperties() throws {
let geo = TestData.geo
let actual = geo.serialize()
let data = try XCTUnwrap(SentrySerialization.data(withJSONObject: actual))

let decoded = decodeFromJSONData(jsonData: data) as Geo?

XCTAssertEqual(geo, decoded)
}

func testDecode_WithAllPropertiesNil() throws {
let geo = Geo()
let actual = geo.serialize()
let data = try XCTUnwrap(SentrySerialization.data(withJSONObject: actual))

let decoded = decodeFromJSONData(jsonData: data) as Geo?

XCTAssertEqual(geo, decoded)
}

func testHash() {
XCTAssertEqual(TestData.geo.hash(), TestData.geo.hash())

Expand Down
150 changes: 150 additions & 0 deletions develop-docs/DECISIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -236,3 +236,153 @@ To enable visionOS support with the Sentry static framework, you need to set the

However, C functions can still be accessed from code that is conditionally compiled using directives, such as `#if os(iOS)`.

## Deserializing Events

Date: January 16, 2025
Contributors: @brustolin, @philipphofmann, @armcknight, @philprime

Decision: Mutual Agreement on Option B

Comments:

1. @philprime: I would prefer to manually (because automatically is not possible without external tools) write extensions of existing Objective-C classes to conform to Decodable, then use the JSONDecoder. If the variables of the classes/structs do not match the JSON spec (i.e. ipAddress in Swift, but ip_address serialized), we might have to implement custom CodingKeys anyways.
2. @brustolin: I agree with @philprime , manually writing the Decodable extensions for ObjC classes seems to be the best approach right now.
3. @armcknight: I think the best bet to get the actual work done that is needed is to go with option B, vs all the refactors that would be needed to use Codable to go with A. Then, protocol APIs could be migrated from ObjC to Swift as-needed and converted to Codable.
4. @philipphofmann: I think Option B/ manually deserializing is the way to go for now. I actually tried it and it seemed a bit wrong. I evaluated the other options and with all your input, I agree with doing it manually. We do it once and then all good. Thanks everyone.

### Background
To report fatal app hangs and measure how long an app hangs lasts ([GH Epic](https://github.com/getsentry/sentry-cocoa/issues/4261)), we need to serialize events to disk, deserialize, modify, and send them to Sentry. As of January 14, 2025, the Cocoa SDK doesn’t support deserializing events. As the fatal app hangs must go through beforeSend, we can’t simply modify the serialized JSON stored on disk. Instead, we must deserialize the event JSON and initialize a SentryEvent so that it can go through beforeSend.

As of January 14, 2025, all the serialization is custom-made with the [SentrySerializable](https://github.com/getsentry/sentry-cocoa/blob/main/Sources/Sentry/Public/SentrySerializable.h) protocol:

```objectivec
@protocol SentrySerializable <NSObject>

- (NSDictionary<NSString *, id> *)serialize;

@end
```
The SDK manually creates a JSON-like dict:
```objectivec
- (NSDictionary<NSString *, id> *)serialize
{
return @{ @"city" : self.city, @"country_code" : self.countryCode, @"region" : self.region };
}
```

And then the [SentryEnvelope](https://github.com/getsentry/sentry-cocoa/blob/72e34fae44b817d8c12490bbc5c1ce7540f86762/Sources/Sentry/SentryEnvelope.m#L70-L90) calls serialize on the event and then converts it to JSON data.

```objectivec
NSData *json = [SentrySerialization dataWithJSONObject:[event serialize]];
```
To implement a deserialized method, we would need to manually implement the counterparts, which is plenty of code. As ObjC is slowly dying out and the future is Swift, we would like to avoid writing plenty of ObjC code that we will convert to Swift in the future.
### Option A: Use Swifts Built In Codable and convert Serializable Classes to Swift
As Swift has a built-in [Decoding and Encoding](https://developer.apple.com/documentation/foundation/archives_and_serialization/encoding_and_decoding_custom_types) mechanisms it makes sense to explore this option.
Serializing a struct in Swift to JSON is not much code:
```objectivec
struct Language: Codable {
var name: String
var version: Int
}
let swift = Language(name: "Swift", version: 6)
let encoder = JSONEncoder()
if let encoded = try? encoder.encode(swift) {
// save `encoded` somewhere
}
```

The advantage is that we don’t have to manually create the dictionaries in serialize and a potential deserialize method. The problem is that this only works with Swift structs and classes. We can’t use Swift structs, as they’re not working in ObjC. So, we need to convert the classes to serialize and deserialize to Swift.

The major challenge is that doing this without breaking changes for both Swift and ObjC is extremely hard to achieve. One major problem is that some existing classes such as SentryUser overwrite the `- (NSUInteger)hash` method, which is `var hash: [Int](https://developer.apple.com/documentation/swift/int) { get }` in Swift. When converting SentryUser to Swift, calling `user.hash()` converts to `user.hash`. While most of our users don’t call this method, it still is a breaking change. And that’s only one issue we found when converting classes to Swift.

To do this conversion safely, we should do it in a major release. We need to convert all public protocol classes to Swift. Maybe it even makes sense to convert all public classes to Swift to avoid issues with our package managers that get confused when there is a mix of public classes of Swift and ObjC. SPM, for example, doesn’t allow this, and we need to precompile our SDK to be compatible.

The [SentryEnvelope](https://github.com/getsentry/sentry-cocoa/blob/72e34fae44b817d8c12490bbc5c1ce7540f86762/Sources/Sentry/SentryEnvelope.m#L70-L90) first creates a JSON dict and then converts it to JSON data. Instead, we could directly use the Swift JSONEncoder to save one step in between. This would convert the classes to JSON data directly.

```objectivec
NSData *json = [SentrySerialization dataWithJSONObject:[event serialize]];
```
All that said, I suggest converting all public protocol classes to Swift and switching to Swift Codable for serialization, cause it will be less code and more future-proof. Of course, we will run into problems and challenges on the way, but it’s worth doing it.
#### Pros
1. Less code.
2. More Swift code is more future-proof.
#### Cons
- Major release
- Risk of adding bugs
### Option B: Add Deserialize in Swift
We could implement all deserializing code in Swift without requiring a major version. The implementation would be the counterpart of ObjC serialize implementations, but written in Swift.
#### Pros
1. No major
2. Low risk of introducing bugs
3. Full control of serializing and deserializing
#### Cons
1. Potentially slightly higher maintenance effort, which is negligible as we hardly change the protocol classes.
*Sample for implementation of Codable:*
```swift
@_implementationOnly import _SentryPrivate
import Foundation
// User is the Swift name of SentryUser
extension User: Codable {
private enum CodingKeys: String, CodingKey {
case id
case email
case username
case ipAddress
}
public func encode(to encoder: any Encoder) throws {
var container = encoder.container(keyedBy: CodingKeys.self)
try container.encode(userId, forKey: .id)
try container.encode(email, forKey: .email)
try container.encode(username, forKey: .username)
try container.encode(ipAddress, forKey: .ipAddress)
}
public required convenience init(from decoder: any Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
self.init(userId: try container.decode(String.self, forKey: .id))
email = try container.decode(String.self, forKey: .email)
username = try container.decode(String.self, forKey: .username)
ipAddress = try container.decode(String.self, forKey: .ipAddress)
}
}
```

### Option C: Duplicate protocol classes and use Swift Codable

We do option A, but we keep the public ObjC classes, duplicate them in Swift, and use the internal Swift classes only for serializing and deserializing. Once we have a major release, we replace the ObjC classes with the internal Swift classes.

We can also start with this option to evaluate Swift Codable and switch to option A once we’re confident it’s working correctly.

#### Pros

1. No major.
2. We can refactor it step by step.
3. The risk of introducing bugs can be distributed across multiple releases.

#### Cons

1. Duplicate code.

0 comments on commit 04e8394

Please sign in to comment.