-
-
Notifications
You must be signed in to change notification settings - Fork 376
refactor: Convert SentryCoreDataTrackingIntegration to Swift
#7255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
itaybre
wants to merge
15
commits into
main
Choose a base branch
from
itaybrenner/cocoa-988-refactor-sentrycoredatatrackingintegrationm-in-swift
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
170788d
chore: Convert `SentryNSDataSwizzling` and `SentryNSFileManagerSwizzl…
itaybre a148b57
refactor: Remove legacy swizzling imports and add new helper classes …
itaybre 86a92da
Merge branch 'main' into itaybrenner/cocoa-1000-refactor-sentryfileio…
itaybre cfc9e58
Add tests for SentryNSDataSwizzlingHelper
itaybre 5e10557
Add SentryNSFileManagerSwizzlingHelper and corresponding tests
itaybre 628fcb0
Use weak reference for tracker
itaybre 3892c7e
refactor: remove unused tracker property from SentryNSDataSwizzling a…
itaybre 54a5665
Rename tests
itaybre 265eef3
refactor: Convert `SentryCoreDataTrackingIntegration` to Swift
itaybre 13a5f3a
Merge branch 'main' of github.com:getsentry/sentry-cocoa into itaybre…
itaybre eded808
Move fetchManagedObjectContext helper method to SentryCoreDataTracker…
itaybre a0208cd
Remove singleton
itaybre 47313bc
Fix build on macOS
itaybre 71db266
Merge branch 'main' of github.com:getsentry/sentry-cocoa into itaybre…
itaybre 918e252
Merge branch 'main' of github.com:getsentry/sentry-cocoa into itaybre…
itaybre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #import "SentryDefines.h" | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface SentryCoreDataSwizzlingHelper : NSObject | ||
|
|
||
| + (void)swizzleWithTracker:(SENTRY_SWIFT_MIGRATION_ID(SentryCoreDataTracker))tracker; | ||
|
|
||
| + (void)unswizzle; | ||
|
|
||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| + (BOOL)swizzlingActive; | ||
| #endif | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| #import "SentryCoreDataSwizzlingHelper.h" | ||
| #import "SentryCoreDataTracker.h" | ||
| #import "SentrySwift.h" | ||
| #import "SentrySwizzle.h" | ||
| #import <CoreData/CoreData.h> | ||
| #import <objc/runtime.h> | ||
|
|
||
| @implementation SentryCoreDataSwizzlingHelper | ||
|
|
||
| static __weak SentryCoreDataTracker *_tracker = nil; | ||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| static BOOL swizzlingIsActive = FALSE; | ||
| #endif | ||
|
|
||
| // SentrySwizzleInstanceMethod declaration shadows a local variable. The swizzling is working | ||
| // fine and we accept this warning. | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wshadow" | ||
| + (void)swizzleWithTracker:(SentryCoreDataTracker *)tracker | ||
| { | ||
| _tracker = tracker; | ||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| swizzlingIsActive = TRUE; | ||
| #endif | ||
|
|
||
| SEL fetchSelector = NSSelectorFromString(@"executeFetchRequest:error:"); | ||
| SentrySwizzleInstanceMethod(NSManagedObjectContext.class, fetchSelector, | ||
| SentrySWReturnType(NSArray *), | ||
| SentrySWArguments(NSFetchRequest * originalRequest, NSError * *error), SentrySWReplacement({ | ||
| return _tracker != nil | ||
| ? [_tracker | ||
| managedObjectContext:self | ||
| executeFetchRequest:originalRequest | ||
| error:error | ||
| originalImp:^NSArray *(NSFetchRequest *request, NSError **outError) { | ||
| return SentrySWCallOriginal(request, outError); | ||
| }] | ||
| : SentrySWCallOriginal(originalRequest, error); | ||
| }), | ||
| SentrySwizzleModeOncePerClassAndSuperclasses, (void *)fetchSelector); | ||
|
|
||
| SEL saveSelector = NSSelectorFromString(@"save:"); | ||
| SentrySwizzleInstanceMethod(NSManagedObjectContext.class, saveSelector, | ||
| SentrySWReturnType(BOOL), SentrySWArguments(NSError * *error), SentrySWReplacement({ | ||
| return _tracker != nil ? [_tracker managedObjectContext:self | ||
| save:error | ||
| originalImp:^BOOL(NSError **outError) { | ||
| return SentrySWCallOriginal(outError); | ||
| }] | ||
| : SentrySWCallOriginal(error); | ||
| }), | ||
| SentrySwizzleModeOncePerClassAndSuperclasses, (void *)saveSelector); | ||
| } | ||
|
|
||
| + (void)unswizzle | ||
| { | ||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| _tracker = nil; | ||
| swizzlingIsActive = FALSE; | ||
|
|
||
| // Unswizzling is only supported in test targets as it is considered unsafe for production. | ||
| SEL fetchSelector = NSSelectorFromString(@"executeFetchRequest:error:"); | ||
| SentryUnswizzleInstanceMethod( | ||
| NSManagedObjectContext.class, fetchSelector, (void *)fetchSelector); | ||
|
|
||
| SEL saveSelector = NSSelectorFromString(@"save:"); | ||
| SentryUnswizzleInstanceMethod(NSManagedObjectContext.class, saveSelector, (void *)saveSelector); | ||
| #endif // SENTRY_TEST || SENTRY_TEST_CI | ||
| } | ||
| #pragma clang diagnostic pop | ||
|
|
||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| + (BOOL)swizzlingActive | ||
| { | ||
| return swizzlingIsActive; | ||
| } | ||
| #endif | ||
| @end | ||
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #import "SentryDefines.h" | ||
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface SentryCoreDataSwizzlingHelper : NSObject | ||
|
|
||
| + (void)swizzleWithTracker:(SENTRY_SWIFT_MIGRATION_ID(SentryCoreDataTracker))tracker; | ||
|
|
||
| + (void)unswizzle; | ||
|
|
||
| #if SENTRY_TEST || SENTRY_TEST_CI | ||
| + (BOOL)swizzlingActive; | ||
| #endif | ||
|
|
||
| @end | ||
|
|
||
| NS_ASSUME_NONNULL_END |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 changes: 13 additions & 0 deletions
13
Sources/Swift/Integrations/Performance/CoreData/SentryCoreDataSwizzling.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // swiftlint:disable missing_docs | ||
| @_implementationOnly import _SentryPrivate | ||
|
|
||
| final class SentryCoreDataSwizzling: NSObject { | ||
| func start(with tracker: Any) { | ||
| SentryCoreDataSwizzlingHelper.swizzle(withTracker: tracker) | ||
| } | ||
|
|
||
| func stop() { | ||
| SentryCoreDataSwizzlingHelper.unswizzle() | ||
| } | ||
| } | ||
| // swiftlint:enable missing_docs |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition when accessing weak tracker reference twice
Medium Severity
The
__weakstatic_trackervariable is accessed twice in the ternary expression: once for the nil check and once for the method call. Between these accesses, the tracker could be deallocated on another thread, causing the second access to return nil. This would result in the message being sent to nil, returning nil/NO instead of calling the original Core Data implementation. The old code avoided this by capturing the weak reference into a local strong variable first. This could cause fetch operations to silently return empty results or save operations to report false failures.Additional Locations (1)
Sources/Sentry/SentryCoreDataSwizzlingHelper.m#L44-L50