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: add NSNull handling to sentry_sanitize #4760

Merged
merged 5 commits into from
Jan 29, 2025
Merged

Conversation

philprime
Copy link
Contributor

📜 Description

  • Adds handling of [NSNull null] as parameter for sentry_sanitize
  • Adds unit tests for sentry_sanitize

💡 Motivation and Context

Closes #4748

💚 How did you test it?

  • Additional unit tests

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

none

@philprime philprime marked this pull request as ready for review January 28, 2025 14:51
@philprime philprime self-assigned this Jan 28, 2025
Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.220%. Comparing base (1d128a6) to head (6eb4fe0).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4760       +/-   ##
=============================================
+ Coverage   91.077%   91.220%   +0.142%     
=============================================
  Files          622       625        +3     
  Lines        72360     72654      +294     
  Branches     25643     26459      +816     
=============================================
+ Hits         65904     66275      +371     
+ Misses        6362      6281       -81     
- Partials        94        98        +4     
Files with missing lines Coverage Δ
Sources/Sentry/SentryNSDictionarySanitize.m 100.000% <100.000%> (ø)
...ests/Categories/SentryNSDictionarySanitize+Tests.m 100.000% <100.000%> (ø)
...s/Categories/SentryNSDictionarySanitizeTests.swift 100.000% <100.000%> (ø)

... and 48 files with indirect coverage changes


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 1d128a6...6eb4fe0. Read the comment docs.

Copy link

github-actions bot commented Jan 28, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1232.80 ms 1248.02 ms 15.22 ms
Size 22.31 KiB 775.27 KiB 752.96 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
077e940 1218.33 ms 1243.24 ms 24.92 ms
f1ed6f8 1210.94 ms 1230.78 ms 19.84 ms
1f9387b 1229.25 ms 1242.62 ms 13.37 ms
35647ef 1250.42 ms 1251.87 ms 1.46 ms
b9a9ffd 1221.18 ms 1235.37 ms 14.19 ms
0001a09 1223.90 ms 1249.56 ms 25.66 ms
add9550 1221.20 ms 1250.04 ms 28.84 ms
c0b4b71 1246.98 ms 1256.77 ms 9.79 ms
061c982 1226.33 ms 1243.65 ms 17.33 ms
86e9bf2 1225.70 ms 1252.19 ms 26.49 ms

App size

Revision Plain With Sentry Diff
077e940 21.58 KiB 706.97 KiB 685.39 KiB
f1ed6f8 21.58 KiB 683.51 KiB 661.93 KiB
1f9387b 21.58 KiB 654.26 KiB 632.68 KiB
35647ef 21.58 KiB 706.97 KiB 685.39 KiB
b9a9ffd 21.58 KiB 418.43 KiB 396.85 KiB
0001a09 20.76 KiB 433.19 KiB 412.43 KiB
add9550 21.58 KiB 418.37 KiB 396.79 KiB
c0b4b71 20.76 KiB 430.98 KiB 410.22 KiB
061c982 21.58 KiB 683.51 KiB 661.93 KiB
86e9bf2 22.31 KiB 758.78 KiB 736.47 KiB

Previous results on branch: issue-4748-null-check

Startup times

Revision Plain With Sentry Diff
e06c5b8 1226.11 ms 1247.77 ms 21.66 ms
f29efb5 1238.69 ms 1255.90 ms 17.20 ms

App size

Revision Plain With Sentry Diff
e06c5b8 22.31 KiB 775.27 KiB 752.95 KiB
f29efb5 22.31 KiB 775.37 KiB 753.06 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the tests for this function!

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, when the tests are included in Xcode.

@philprime philprime merged commit c75412c into main Jan 29, 2025
71 checks passed
@philprime philprime deleted the issue-4748-null-check branch January 29, 2025 17:19
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.

NSInvalidArgumentException -[NSNull allKeys]: unrecognized selector sent to instance 0x1f005b678
3 participants