Skip to content
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

fix: fall back to old behavior if newer value is nil in core data tracker #2865

Merged
merged 10 commits into from Apr 24, 2023

Conversation

armcknight
Copy link
Member

Fixes a possible crash, because NSEntityDescription.name is actually declared as nullable, so if it is nil and we try to use it as a key in a dictionary, that will throw an exception. In case it is nil, we'll fall back to the prior behavior for this operation.

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1219.51 ms 1239.44 ms 19.93 ms
Size 20.76 KiB 433.23 KiB 412.47 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ea73af6 1230.96 ms 1244.98 ms 14.02 ms
5b6694b 1221.71 ms 1259.06 ms 37.35 ms
f715499 1234.26 ms 1259.40 ms 25.14 ms
20163bb 1248.92 ms 1258.48 ms 9.56 ms
4c00f8c 1231.62 ms 1237.76 ms 6.14 ms
89d72e7 1222.60 ms 1252.78 ms 30.18 ms
3e7aa41 1214.02 ms 1238.84 ms 24.82 ms
c50d363 1215.10 ms 1231.82 ms 16.72 ms
c00eafe 1253.04 ms 1256.41 ms 3.37 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms

App size

Revision Plain With Sentry Diff
ea73af6 20.76 KiB 425.75 KiB 404.99 KiB
5b6694b 20.76 KiB 426.11 KiB 405.34 KiB
f715499 20.76 KiB 427.23 KiB 406.47 KiB
20163bb 20.76 KiB 426.82 KiB 406.06 KiB
4c00f8c 20.76 KiB 419.62 KiB 398.86 KiB
89d72e7 20.76 KiB 431.87 KiB 411.11 KiB
3e7aa41 20.76 KiB 432.31 KiB 411.55 KiB
c50d363 20.76 KiB 432.68 KiB 411.92 KiB
c00eafe 20.76 KiB 432.88 KiB 412.12 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB

Previous results on branch: armcknight/fix/possible-nil-crash

Startup times

Revision Plain With Sentry Diff
c23c255 1229.71 ms 1248.02 ms 18.31 ms
fa8b1a9 1219.48 ms 1248.94 ms 29.46 ms
172904f 1224.24 ms 1251.98 ms 27.73 ms
f393202 1247.47 ms 1263.53 ms 16.06 ms

App size

Revision Plain With Sentry Diff
c23c255 20.76 KiB 432.30 KiB 411.54 KiB
fa8b1a9 20.76 KiB 432.25 KiB 411.49 KiB
172904f 20.76 KiB 433.23 KiB 412.47 KiB
f393202 20.76 KiB 432.25 KiB 411.49 KiB

@armcknight armcknight changed the title fix: fall back to old behavior if newer value is nil fix: fall back to old behavior if newer value is nil in core data tracker Apr 6, 2023
@armcknight
Copy link
Member Author

tests should pass after merging #2867

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.
I never saw entity name being nil, but since is nullable I guess is possible.

Looks good, I think we need a test with nil entity name in order to finish this.

@armcknight
Copy link
Member Author

Good catch. I never saw entity name being nil, but since is nullable I guess is possible.

Looks good, I think we need a test with nil entity name in order to finish this.

So when I try to use the current core data tracker tests to write one like that, it throws the following exception:

/Users/andrewmcknight/Code/organization/getsentry/repos/public/sentry-cocoa/Tests/SentryTests/Integrations/Performance/CoreData/TestCoreDataStack.swift:10: error: -[SentryTests.SentryCoreDataTrackerTests testEntityWithNilName] : Entity name must not be nil. (NSInvalidArgumentException)

@brustolin
Copy link
Contributor

So when I try to use the current core data tracker tests to write one like that, it throws the following exception

Ow, now I remember seen something like this.
Even if non nullability is enforced at run time, I don't mind keeping the changing in this PR. I guess users can do some crazy thing that would make a nil name.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@armcknight armcknight merged commit d257eb9 into main Apr 24, 2023
66 of 68 checks passed
@armcknight armcknight deleted the armcknight/fix/possible-nil-crash branch April 24, 2023 18:48
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.

None yet

4 participants