Fix: SentryOptions initWithDict return nil for error #2679

brustolin · 2023-02-02T12:08:43Z

📜 Description

Changed [SentryOptions initWithDict:error:] to return nil if an error occurs even if no error argument is provided.
Before this, it only returned nil if and error argument was provided.

💡 Motivation and Context

According to Apple's documentation on how to deal with error we see this:

When dealing with errors passed by reference, it’s important to test the return value of the method to see whether an error occurred, as shown above. Don’t just test to see whether the error pointer was set to point to an error.

When using SentryOptions initWithDict:error: with Unity, we found out that it did not fail when the DSN was invalid. Thats because Unity code cant use an error reference and needs to rely solely on the initialization result.

Also, relying only on the error reference value can be misleading because it may be used in multiple calls and have 'dirty' value.

💚 How did you test it?

Unit tests

📝 Checklist

I reviewed the submitted code.
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.
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.

#skip-changelog

codecov · 2023-02-02T13:01:04Z

Codecov Report

Merging #2679 (81f63e6) into main (adcc7d8) will increase coverage by 1.79%.
The diff coverage is 100.00%.

❗ Current head 81f63e6 differs from pull request most recent head 962e158. Consider uploading reports for the commit 962e158 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2679      +/-   ##
==========================================
+ Coverage   78.90%   80.69%   +1.79%     
==========================================
  Files         249      247       -2     
  Lines       23091    22869     -222     
  Branches    10037    10129      +92     
==========================================
+ Hits        18219    18455     +236     
+ Misses       4390     3960     -430     
+ Partials      482      454      -28

Impacted Files	Coverage Δ
Sources/Sentry/SentryDsn.m	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryOptions.m	`96.80% <100.00%> (+1.60%)`	⬆️
Sources/Sentry/SentryMetricProfiler.mm	`93.47% <0.00%> (-6.53%)`	⬇️
Sources/Sentry/SentryProfiler.mm	`79.48% <0.00%> (-0.82%)`	⬇️
Sources/Sentry/SentryBreadcrumbTracker.m	`76.82% <0.00%> (-0.76%)`	⬇️
Sources/Sentry/SentryUIViewControllerSwizzling.m	`56.10% <0.00%> (-0.26%)`	⬇️
Sources/Sentry/SentryFramesTracker.m	`98.00% <0.00%> (-0.06%)`	⬇️
Sources/Sentry/SentryThreadInspector.m	`98.00% <0.00%> (-0.06%)`	⬇️
.../Sentry/SentryUIViewControllerPerformanceTracker.m	`98.55% <0.00%> (-0.05%)`	⬇️
Sources/Sentry/SentryLog.m	`100.00% <0.00%> (ø)`
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adcc7d8...962e158. Read the comment docs.

philipphofmann · 2023-02-03T10:35:06Z

What is the motivation for this change, @brustolin? Could you please fill out the PR template?

armcknight

Just some comments!

Sources/Sentry/SentryOptions.m

armcknight · 2023-02-07T20:11:49Z

Sources/Sentry/SentryOptions.m

-    NSString *dsn = @"";
-    if (nil != options[@"dsn"] && [options[@"dsn"] isKindOfClass:[NSString class]]) {
-        dsn = options[@"dsn"];
-    }
+    if (options[@"dsn"] != [NSNull null]) {
+        NSString *dsn = @"";
+        if (nil != options[@"dsn"] && [options[@"dsn"] isKindOfClass:[NSString class]]) {
+            dsn = options[@"dsn"];
+        }

-    self.parsedDsn = [[SentryDsn alloc] initWithString:dsn didFailWithError:error];
+        self.parsedDsn = [[SentryDsn alloc] initWithString:dsn didFailWithError:error];
+        if (self.parsedDsn == nil) {
+            return NO;
+        }
+    }


I'm trying to understand why the test case flips from asserting the DSN is not nil to asserting that it is nil in the default case. Do we allow initializing the SDK without a valid DSN? If not, I think it would be clearer what the eventual outcome would be here if instead, we made this an early return like so:

if (options[@"dsn"] == [NSNull null]) { return NO; }

and then do the rest of the work at one level higher of scope below that. It would also avoid all the work below being done which wouldn't matter if the SDK winds up in an error state anyways.

The test case flipped because the prior assumption was wrong.

Why we allow options to be initialized without a DSN I dont know , maybe @philipphofmann can give us some context here.

You can initialize the SDK without a DSN. The SDK would run and do its job, but the client will drop everything see

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 636 to 639 in 72c650a

- (BOOL)isDisabled

{

return !_isEnabled || !self.options.enabled || nil == self.options.parsedDsn;

}

It's similar to using options.enabled = false.

I opened an issue where we can discuss this further, since it's out of scope for this PR: #2712

philipphofmann

If you add one test as pointed out, LGTM.

I think we could also remove didFailWithError:(NSError *_Nullable *_Nullable)error from SentryOptions.initWithDict. Could you check the hybrid SDKs if they use it, and if not remove it in another PR, @brustolin?

Sources/Sentry/SentryDsn.m

philipphofmann · 2023-02-20T14:05:39Z

Sources/Sentry/SentryOptions.m

-    NSString *dsn = @"";
-    if (nil != options[@"dsn"] && [options[@"dsn"] isKindOfClass:[NSString class]]) {
-        dsn = options[@"dsn"];
-    }
+    if (options[@"dsn"] != [NSNull null]) {
+        NSString *dsn = @"";
+        if (nil != options[@"dsn"] && [options[@"dsn"] isKindOfClass:[NSString class]]) {
+            dsn = options[@"dsn"];
+        }

-    self.parsedDsn = [[SentryDsn alloc] initWithString:dsn didFailWithError:error];
+        self.parsedDsn = [[SentryDsn alloc] initWithString:dsn didFailWithError:error];
+        if (self.parsedDsn == nil) {
+            return NO;
+        }
+    }


You can initialize the SDK without a DSN. The SDK would run and do its job, but the client will drop everything see

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 636 to 639 in 72c650a

- (BOOL)isDisabled

{

return !_isEnabled || !self.options.enabled || nil == self.options.parsedDsn;

}

It's similar to using options.enabled = false.

brustolin · 2023-02-28T14:44:09Z

Could you check the hybrid SDKs if they use it

RN and Unity use it.

github-actions · 2023-02-28T15:12:49Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1237.51 ms	1266.69 ms	29.18 ms
Size	20.76 KiB	426.13 KiB	405.37 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`4977fbc`	1231.55 ms	1239.80 ms	8.25 ms
`7fb7afb`	1230.12 ms	1251.04 ms	20.92 ms
`8cb9355`	1225.23 ms	1231.22 ms	5.99 ms
`f8fc36d`	1226.31 ms	1247.80 ms	21.49 ms
`443723a`	1205.24 ms	1220.52 ms	15.28 ms
`b6ba04e`	1217.45 ms	1248.92 ms	31.47 ms
`005bb4c`	1237.38 ms	1255.54 ms	18.16 ms
`4259afd`	1234.04 ms	1256.76 ms	22.72 ms
`ecd9ecd`	1215.77 ms	1238.70 ms	22.93 ms
`d413317`	1203.27 ms	1215.02 ms	11.75 ms

App size

Revision	Plain	With Sentry	Diff
`4977fbc`	20.76 KiB	419.86 KiB	399.10 KiB
`7fb7afb`	20.76 KiB	419.70 KiB	398.94 KiB
`8cb9355`	20.76 KiB	419.70 KiB	398.95 KiB
`f8fc36d`	20.76 KiB	419.70 KiB	398.94 KiB
`443723a`	20.76 KiB	414.44 KiB	393.68 KiB
`b6ba04e`	20.76 KiB	414.45 KiB	393.69 KiB
`005bb4c`	20.76 KiB	419.70 KiB	398.94 KiB
`4259afd`	20.76 KiB	419.70 KiB	398.94 KiB
`ecd9ecd`	20.76 KiB	420.23 KiB	399.47 KiB
`d413317`	20.76 KiB	420.71 KiB	399.95 KiB

SentryOptions should fail even with no error argument

4c2470d

brustolin requested review from philipphofmann and armcknight as code owners February 2, 2023 12:08

brustolin marked this pull request as draft February 2, 2023 13:43

armcknight reviewed Feb 7, 2023

View reviewed changes

brustolin added 2 commits February 9, 2023 10:21

Merge branch 'main' into fix/optionInitWithDict

a799c30

Update SentryOptions.m

81f63e6

brustolin marked this pull request as ready for review February 15, 2023 09:26

philipphofmann approved these changes Feb 20, 2023

View reviewed changes

brustolin and others added 3 commits February 28, 2023 15:44

Merge branch 'main' into fix/optionInitWithDict

727ce18

Update SentryDsnTests.m

bea3d9e

Format code

962e158

brustolin merged commit d40512b into main Mar 7, 2023

brustolin deleted the fix/optionInitWithDict branch March 7, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: SentryOptions initWithDict return nil for error #2679

Fix: SentryOptions initWithDict return nil for error #2679

brustolin commented Feb 2, 2023 •

edited

Loading

codecov bot commented Feb 2, 2023 •

edited

Loading

philipphofmann commented Feb 3, 2023

armcknight left a comment

armcknight Feb 7, 2023 •

edited

Loading

brustolin Feb 9, 2023

philipphofmann Feb 20, 2023

armcknight Feb 21, 2023

philipphofmann left a comment

philipphofmann Feb 20, 2023

brustolin commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

Baseline results on branch: main

Startup times

App size

	- (BOOL)isDisabled
	{
	return !_isEnabled \|\| !self.options.enabled \|\| nil == self.options.parsedDsn;
	}

Fix: SentryOptions initWithDict return nil for error #2679

Fix: SentryOptions initWithDict return nil for error #2679

Conversation

brustolin commented Feb 2, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

codecov bot commented Feb 2, 2023 • edited Loading

Codecov Report

philipphofmann commented Feb 3, 2023

armcknight left a comment

Choose a reason for hiding this comment

armcknight Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

brustolin Feb 9, 2023

Choose a reason for hiding this comment

philipphofmann Feb 20, 2023

Choose a reason for hiding this comment

armcknight Feb 21, 2023

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Feb 20, 2023

Choose a reason for hiding this comment

brustolin commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

brustolin commented Feb 2, 2023 •

edited

Loading

codecov bot commented Feb 2, 2023 •

edited

Loading

armcknight Feb 7, 2023 •

edited

Loading