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

SetTag no longer allows null value #3946

Closed
Cheesebaron opened this issue Feb 7, 2025 · 3 comments · Fixed by #3948
Closed

SetTag no longer allows null value #3946

Cheesebaron opened this issue Feb 7, 2025 · 3 comments · Fixed by #3948

Comments

@Cheesebaron
Copy link
Contributor

Package

Sentry

.NET Flavor

.NET

.NET Version

9.0.100

OS

iOS

SDK Version

5.1.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

Some classes in our app don't have Nullable Attributes enabled and in previous versions of Sentry doing something like this was allowed:

string? value = null!;

SentrySdk.ConfigureScope(scope =>
{
    scope.SetTag("some-key", value);
});

However, with 5.1.0 it seems like this is not allowed any more and a ArgumentNullException is thrown:

Received unhandled Objective-C exception that was marshalled from a managed exception: ArgumentNull_Generic Arg_ParamName_Name, value (System.ArgumentNullException)
   at ObjCRuntime.ThrowHelper.ThrowArgumentNullException(String )
   at Sentry.CocoaSdk.SentryScope.SetTagValue(String value, String key)
   at Sentry.Cocoa.CocoaScopeObserver.<>c__DisplayClass5_0.<SetTag>b__0(SentryScope scope)
   at Sentry.CocoaSdk.SentrySDK.ConfigureScope(Action`1 callback)
   at Sentry.Cocoa.CocoaScopeObserver.SetTag(String , String )
   at Sentry.Scope.SetTag(String , String )
   at TrackMan.DrivingRangeApp.Crashes.SentryCrashReporter.<>c__DisplayClass2_0.<ConfigureGlobalScopeTags>b__0(Scope scope)
   at Sentry.Internal.SentryScopeManager.ConfigureScope(Action`1 )
   at Sentry.Internal.Hub.ConfigureScope(Action`1 )

Expected Result

Not sure what to expect, but sure the nullable attributes explicitly say value cannot be null. But previously it was allowed and I guess ignored internally.

Actual Result

See crash above.

@bruno-garcia
Copy link
Member

Thanks for reporting this @Cheesebaron!

Any chance you'd be willing to contribute a PR to improve this?

@Cheesebaron
Copy link
Contributor Author

Cheesebaron commented Feb 7, 2025

Sure. I can do that. What should the behavior be? Simply ignoring the null?

I assume unsetting the tag when the value is null is not a desired side effect.

@bruno-garcia
Copy link
Member

Sure. I can do that. What should the behavior be? Simply ignoring the null?

I assume unsetting the tag when the value is null is not a desired side effect.

hm that's a fair question. The only risk I see is that code that calls this API with some variable. Didn't intend to be passing null and removing the tag. And that could cause confusion. "I never called UnsetTag how is the tag gone?".

But the argument of a single API is al reasonable if we didn't have already UnsetTag in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants