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

Jump to bottom

Throw ArgumentNullException if DSN is null #2655

Merged
merged 9 commits into from
Sep 28, 2023
Merged

Throw ArgumentNullException if DSN is null #2655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40c7e80
Throw when no DSN is configured
jamescrosswell Sep 26, 2023
1d7e99b
Update CHANGELOG.md
jamescrosswell Sep 26, 2023
1c37375
Updated unit tests
jamescrosswell Sep 26, 2023
54301e1
Update src/Sentry/Internal/SettingLocator.cs
jamescrosswell Sep 27, 2023
afaf831
Update CHANGELOG.md
jamescrosswell Sep 27, 2023
69d0475
Update SentrySdk.cs
jamescrosswell Sep 27, 2023
69ebfa1
Merge branch 'feat/4.0.0-throw-if-no-dsn-2494' of https://github.com/…
jamescrosswell Sep 27, 2023
5a65707
Merge branch 'feat/4.0.0' into feat/4.0.0-throw-if-no-dsn-2494
jamescrosswell Sep 27, 2023
4b39688
Updated tests for new warnings
jamescrosswell Sep 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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ without native/platform specific bindings and SDKs. See [this ticket for more de
- Drop .NET 6 Mobile in favor of .NET 7 ([#2624](https://github.com/getsentry/sentry-dotnet/pull/2604))

API Changes:

- If `null` has been supplied as DSN when initializing Sentry, and ArgumentNullException is now thrown ([#2655](https://github.com/getsentry/sentry-dotnet/pull/2655))
- IHasBreadcrumbs was removed. Use IEventLike instead. ([#2670](https://github.com/getsentry/sentry-dotnet/pull/2670))
- ISpanContext was removed. Use ITraceContext instead. ([#2668](https://github.com/getsentry/sentry-dotnet/pull/2668))
- Removed IHasTransactionNameSource. Use ITransactionContext instead. ([#2654](https://github.com/getsentry/sentry-dotnet/pull/2654))
Expand Down
18 changes: 7 additions & 11 deletions src/Sentry/Internal/SettingLocator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,15 @@ public string GetDsn()
// For DSN only
// An empty string set on the option should NOT be converted to null because it is used
// to indicate the the SDK is disabled.

var dsn = _options.Dsn;
if (dsn != null)
_options.Dsn ??= GetEnvironmentVariable(Constants.DsnEnvironmentVariable)
?? AssemblyForAttributes?.GetCustomAttribute<DsnAttribute>()?.Dsn;
if (_options.Dsn is null)
Copy link
Member

Choose a reason for hiding this comment

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

Do we always call the locator after any framework bootstrapping phase? Like ASP.NET Core Configuration system? I imagine integration tests would break otherwise but worth testing a sample that loads the DSN from appsettings.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think it's all covered by the integration tests.

Some manual smoke tests on my part:

ASP.NET Core

Configure Disabled DSN as Assembly Attribute and removing from Options Callback

Setting this in the Directory.Build.props in the Samples directory and removing any other DSN configuration in C# resulted in a DisabledHub (as expected):

  <ItemGroup>
    <SentryAttributes Include="Sentry.DsnAttribute">
      <_Parameter1></_Parameter1>
    </SentryAttributes>
  </ItemGroup>

Removing DSN as Assembly Attribute and from Options Callback

Removing the Assembly Attribute from the Directory.Build.props in the Samples directory and removing any other DSN configuration in C# code resulted in an ArgumentNullException, as expected:

/usr/local/share/dotnet/dotnet /Users/jamescrosswell/code/sentry-dotnet/samples/Sentry.Samples.AspNetCore.Basic/bin/Debug/net6.0/Sentry.Samples.AspNetCore.Basic.dll
  Debug: Logging enabled with ConsoleDiagnosticLogger and min level: Debug
Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'You must supply a DSN to use Sentry.To disable Sentry, pass an empty string: "".See https://docs.sentry.io/platforms/dotnet/configuration/options/#dsn')
   at Sentry.Internal.SettingLocator.GetDsn() in /Users/jamescrosswell/code/sentry-dotnet/src/Sentry/Internal/SettingLocator.cs:line 42

{
return dsn;
throw new ArgumentNullException("You must supply a DSN to use Sentry." +
"To disable Sentry, pass an empty string: \"\"." +
"See https://docs.sentry.io/platforms/dotnet/configuration/options/#dsn");
}

dsn = GetEnvironmentVariable(Constants.DsnEnvironmentVariable)
?? AssemblyForAttributes?.GetCustomAttribute<DsnAttribute>()?.Dsn
?? Sentry.Constants.DisableSdkDsnValue;

_options.Dsn = dsn;
return dsn;
return _options.Dsn;
}

public string GetEnvironment() => GetEnvironment(true)!;
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/SentrySdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ internal static IHub InitHub(SentryOptions options)
// from anywhere else, return a disabled hub.
if (Dsn.IsDisabled(dsnString))
{
options.LogWarning("Init was called but no DSN was provided nor located. Sentry SDK will be disabled.");
options.LogWarning("Init called with an empty string as the DSN. Sentry SDK will be disabled.");
return DisabledHub.Instance;
}

Expand Down
6 changes: 2 additions & 4 deletions test/Sentry.AspNetCore.Tests/AspNetCoreSentryWebHostBuilder.IntegrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,9 @@ public void UseSentry_ValidDsnString_EnablesSdk()
}

[Fact]
public void UseSentry_NoDsnProvided_DisabledSdk()
public void UseSentry_NoDsnProvided_ThrowsException()
{
_ = _webHostBuilder.UseSentry().Build();

Assert.False(SentrySdk.IsEnabled);
Assert.Throws<ArgumentNullException>(() => _webHostBuilder.UseSentry().Build());
}

[Fact]
Expand Down
13 changes: 10 additions & 3 deletions test/Sentry.Extensions.Logging.Tests/SentryLoggerFactoryExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public void AddSentry_NoDiagnosticSet_MelSet()
var sut = Substitute.For<ILoggerFactory>();
_ = sut.AddSentry(o =>
{
o.Dsn = Sentry.Constants.DisableSdkDsnValue;
o.Debug = true;
options = o;
});
Expand All @@ -79,6 +80,7 @@ public void AddSentry_DiagnosticSet_NoOverriden()
var diagnosticLogger = Substitute.For<IDiagnosticLogger>();
_ = sut.AddSentry(o =>
{
o.Dsn = Sentry.Constants.DisableSdkDsnValue;
o.Debug = true;
Assert.Null(o.DiagnosticLogger);
o.DiagnosticLogger = diagnosticLogger;
Expand All @@ -93,7 +95,11 @@ public void AddSentry_WithOptionsCallback_CallbackInvoked()
{
var callbackInvoked = false;
var expected = Substitute.For<ILoggerFactory>();
_ = expected.AddSentry(_ => callbackInvoked = true);
_ = expected.AddSentry(o =>
{
o.Dsn = Sentry.Constants.DisableSdkDsnValue;
callbackInvoked = true;
});

Assert.True(callbackInvoked);
}
Expand All @@ -112,7 +118,7 @@ public void AddSentry_NoOptionsDelegate_ProviderAdded()
public void AddSentry_ReturnsSameFactory()
{
var expected = Substitute.For<ILoggerFactory>();
var actual = expected.AddSentry();
var actual = expected.AddSentry(o => o.Dsn = Sentry.Constants.DisableSdkDsnValue);

Assert.Same(expected, actual);
}
Expand All @@ -121,7 +127,7 @@ public void AddSentry_ReturnsSameFactory()
public void AddSentry_ConfigureOptionsOverload_ReturnsSameFactory()
{
var expected = Substitute.For<ILoggerFactory>();
var actual = expected.AddSentry(_ => { });
var actual = expected.AddSentry(o => o.Dsn = Sentry.Constants.DisableSdkDsnValue);

Assert.Same(expected, actual);
}
Expand All @@ -134,6 +140,7 @@ public void AddSentry_ConfigureOptionsOverload_InvokesCallback()
var invoked = false;
_ = expected.AddSentry(o =>
{
o.Dsn = Sentry.Constants.DisableSdkDsnValue;
Assert.NotNull(o);
invoked = true;
});
Expand Down
4 changes: 2 additions & 2 deletions test/Sentry.NLog.Tests/SentryTargetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ void Continuation(Exception _)
[Fact]
public void InitializeTarget_InitializesSdk()
{
_fixture.Options.Dsn = null;
_fixture.Options.Dsn = Sentry.Constants.DisableSdkDsnValue;
_fixture.SdkDisposeHandle = null;
_fixture.Options.InitializeSdk = true;

Expand All @@ -468,7 +468,7 @@ public void InitializeTarget_InitializesSdk()
_fixture.GetLoggerFactory();

var logOutput = logWriter.ToString();
Assert.Contains("Init was called but no DSN was provided nor located. Sentry SDK will be disabled.", logOutput);
Assert.Contains("Init called with an empty string as the DSN. Sentry SDK will be disabled.", logOutput);
}
finally
{
Expand Down
2 changes: 1 addition & 1 deletion test/Sentry.Serilog.Tests/SerilogAspNetSentrySdkTestFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ protected override void ConfigureBuilder(WebHostBuilder builder)
builder.ConfigureLogging(loggingBuilder =>
{
var logger = new LoggerConfiguration()
.WriteTo.Sentry()
.WriteTo.Sentry(ValidDsn)
.CreateLogger();
loggingBuilder.AddSerilog(logger);
});
Expand Down
7 changes: 2 additions & 5 deletions test/Sentry.Tests/Internals/SettingLocatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,11 @@ public void GetDsn_WithOptionAlreadySet_IgnoresAttribute()
}

[Fact]
public void GetDsn_WithNoValueAnywhere_ReturnsAndSetsDisabledDsn()
public void GetDsn_WithNoValueAnywhere_ThrowsException()
{
var options = new SentryOptions();

var dsn = options.SettingLocator.GetDsn();

Assert.Equal(DisableSdkDsnValue, dsn);
Assert.Equal(DisableSdkDsnValue, options.Dsn);
Assert.Throws<ArgumentNullException>(() => options.SettingLocator.GetDsn());
}

[Fact]
Expand Down
8 changes: 5 additions & 3 deletions test/Sentry.Tests/SentrySdkTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,15 @@ public void Init_EmptyDsn_LogsWarning()
{
var options = new SentryOptions
{
Dsn = Constants.DisableSdkDsnValue,
DiagnosticLogger = _logger,
Debug = true,
InitNativeSdks = false
};

using (SentrySdk.Init(options))
{
_logger.Received(1).Log(SentryLevel.Warning, "Init was called but no DSN was provided nor located. Sentry SDK will be disabled.");
_logger.Received(1).Log(SentryLevel.Warning, "Init called with an empty string as the DSN. Sentry SDK will be disabled.");
}
}

Expand Down Expand Up @@ -176,6 +177,7 @@ public void Init_EmptyDsnDisabledDiagnostics_DoesNotLogWarning()
{
var options = new SentryOptions
{
Dsn = Constants.DisableSdkDsnValue,
DiagnosticLogger = _logger,
Debug = false,
InitNativeSdks = false,
Expand Down Expand Up @@ -821,14 +823,14 @@ public void Implements_ScopeManagement()
[Fact]
public void InitHub_NoDsn_DisposeDoesNotThrow()
{
var sut = SentrySdk.InitHub(new SentryOptions()) as IDisposable;
var sut = SentrySdk.InitHub(new SentryOptions(){Dsn = Constants.DisableSdkDsnValue}) as IDisposable;
sut?.Dispose();
}

[Fact]
public async Task InitHub_NoDsn_FlushAsyncDoesNotThrow()
{
var sut = SentrySdk.InitHub(new SentryOptions());
var sut = SentrySdk.InitHub(new SentryOptions(){Dsn = Constants.DisableSdkDsnValue});
await sut.FlushAsync();
}

Expand Down
Loading