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.

Sign up for GitHub

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: SentryOptions initWithDict return nil for error #2679

Merged
merged 6 commits into from
Mar 7, 2023
Merged

Fix: SentryOptions initWithDict return nil for error #2679

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c2470d
SentryOptions should fail even with no error argument
brustolin Feb 2, 2023
a799c30
Merge branch 'main' into fix/optionInitWithDict
brustolin Feb 9, 2023
81f63e6
Update SentryOptions.m
brustolin Feb 9, 2023
727ce18
Merge branch 'main' into fix/optionInitWithDict
brustolin Feb 28, 2023
bea3d9e
Update SentryDsnTests.m
brustolin Feb 28, 2023
962e158
Format code
getsentry-bot Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/Sentry/SentryDsn.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ - (_Nullable instancetype)initWithString:(NSString *)dsnString
self = [super init];
if (self) {
_url = [self convertDsnString:dsnString didFailWithError:error];
if (nil != error && nil != *error) {
if (_url == nil) {
brustolin marked this conversation as resolved.
Show resolved Hide resolved
return nil;
}
}
Expand Down
29 changes: 16 additions & 13 deletions Sources/Sentry/SentryOptions.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,11 @@ - (_Nullable instancetype)initWithDict:(NSDictionary<NSString *, id> *)options
{
if (self = [self init]) {
if (![self validateOptions:options didFailWithError:error]) {
[SentryLog
logWithMessage:[NSString stringWithFormat:@"Failed to initialize: %@", *error]
andLevel:kSentryLevelError];
if (error != nil) {
SENTRY_LOG_ERROR(@"Failed to initialize SentryOptions: %@", *error);
} else {
SENTRY_LOG_ERROR(@"Failed to initialize SentryOptions");
}
return nil;
}
}
Expand Down Expand Up @@ -235,12 +237,17 @@ - (BOOL)validateOptions:(NSDictionary<NSString *, id> *)options
}
}

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;
}
}
Comment on lines -243 to +255
Copy link
Member

@armcknight armcknight Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


if ([options[@"release"] isKindOfClass:[NSString class]]) {
self.releaseName = options[@"release"];
Expand Down Expand Up @@ -424,11 +431,7 @@ - (BOOL)validateOptions:(NSDictionary<NSString *, id> *)options
}
#endif

if (nil != error && nil != *error) {
return NO;
} else {
return YES;
}
return YES;
}

- (void)setBool:(id)value block:(void (^)(BOOL))block
Expand Down
9 changes: 8 additions & 1 deletion Tests/SentryTests/SentryOptionsTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ - (void)testInvalidDsn
XCTAssertNil(options);
}

- (void)testInvalidDsnWithNoErrorArgument
{
SentryOptions *options = [[SentryOptions alloc] initWithDict:@{ @"dsn" : @"https://sentry.io" }
didFailWithError:nil];
XCTAssertNil(options);
}

- (void)testRelease
{
SentryOptions *options = [self getValidOptions:@{ @"release" : @"abc" }];
Expand Down Expand Up @@ -530,7 +537,7 @@ - (void)testNSNull_SetsDefaultValue
}
didFailWithError:nil];

XCTAssertNotNil(options.parsedDsn);
XCTAssertNil(options.parsedDsn);
[self assertDefaultValues:options];
}

Expand Down