From 27ec7fb3e009f2e63dd69f02fa09021400da30b3 Mon Sep 17 00:00:00 2001 From: Nathan White Date: Mon, 9 Jun 2025 08:23:28 -0700 Subject: [PATCH 1/3] Add more defensive checks and tests --- .../Source/Sentry/Private/SentrySubsystem.cpp | 216 ++++++++---------- .../Private/Tests/SentrySubsystem.spec.cpp | 58 +++++ 2 files changed, 157 insertions(+), 117 deletions(-) diff --git a/plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp b/plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp index 49026f3df..7b14a4145 100644 --- a/plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp +++ b/plugin-dev/Source/Sentry/Private/SentrySubsystem.cpp @@ -186,12 +186,10 @@ void USentrySubsystem::Close() OnEnsureDelegate.Reset(); } - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->Close(); } - - SubsystemNativeImpl->Close(); } bool USentrySubsystem::IsEnabled() const @@ -214,50 +212,45 @@ void USentrySubsystem::AddBreadcrumb(USentryBreadcrumb* Breadcrumb) check(SubsystemNativeImpl); check(Breadcrumb); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Breadcrumb) { - return; + SubsystemNativeImpl->AddBreadcrumb(Breadcrumb->GetNativeObject()); } - - SubsystemNativeImpl->AddBreadcrumb(Breadcrumb->GetNativeObject()); } void USentrySubsystem::AddBreadcrumbWithParams(const FString& Message, const FString& Category, const FString& Type, const TMap& Data, ESentryLevel Level) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->AddBreadcrumbWithParams(Message, Category, Type, Data, Level); } - - SubsystemNativeImpl->AddBreadcrumbWithParams(Message, Category, Type, Data, Level); } void USentrySubsystem::ClearBreadcrumbs() { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->ClearBreadcrumbs(); } - - SubsystemNativeImpl->ClearBreadcrumbs(); } FString USentrySubsystem::CaptureMessage(const FString& Message, ESentryLevel Level) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return FString(); + if (TSharedPtr SentryId = SubsystemNativeImpl->CaptureMessage(Message, Level)) + { + return SentryId->ToString(); + } } - TSharedPtr SentryId = SubsystemNativeImpl->CaptureMessage(Message, Level); - - return SentryId->ToString(); + return FString(); } FString USentrySubsystem::CaptureMessageWithScope(const FString& Message, const FConfigureScopeDelegate& OnConfigureScope, ESentryLevel Level) @@ -269,18 +262,21 @@ FString USentrySubsystem::CaptureMessageWithScope(const FString& Message, const { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return FString(); - } + const auto Lambda = FSentryScopeDelegate::CreateLambda([OnConfigureScope](TSharedPtr NativeScope) + { + USentryScope* UnrealScope = USentryScope::Create(NativeScope); + OnConfigureScope.ExecuteIfBound(UnrealScope); + }); - TSharedPtr SentryId = SubsystemNativeImpl->CaptureMessageWithScope(Message, Level, FSentryScopeDelegate::CreateLambda([OnConfigureScope](TSharedPtr NativeScope) - { - USentryScope* UnrealScope = USentryScope::Create(NativeScope); - OnConfigureScope.ExecuteIfBound(UnrealScope); - })); + if (TSharedPtr SentryId = SubsystemNativeImpl->CaptureMessageWithScope(Message, Level, Lambda)) + { + return SentryId->ToString(); + } + } - return SentryId->ToString(); + return FString(); } FString USentrySubsystem::CaptureEvent(USentryEvent* Event) @@ -288,14 +284,15 @@ FString USentrySubsystem::CaptureEvent(USentryEvent* Event) check(SubsystemNativeImpl); check(Event); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Event) { - return FString(); + if (TSharedPtr SentryId = SubsystemNativeImpl->CaptureEvent(Event->GetNativeObject())) + { + return SentryId->ToString(); + } } - TSharedPtr SentryId = SubsystemNativeImpl->CaptureEvent(Event->GetNativeObject()); - - return SentryId->ToString(); + return FString(); } FString USentrySubsystem::CaptureEventWithScope(USentryEvent* Event, const FConfigureScopeDelegate& OnConfigureScope) @@ -308,18 +305,21 @@ FString USentrySubsystem::CaptureEventWithScope(USentryEvent* Event, const FConf check(SubsystemNativeImpl); check(Event); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Event) { - return FString(); - } + const auto Lambda = FSentryScopeDelegate::CreateLambda([OnConfigureScope](TSharedPtr NativeScope) + { + USentryScope* UnrealScope = USentryScope::Create(NativeScope); + OnConfigureScope.ExecuteIfBound(UnrealScope); + }); - TSharedPtr SentryId = SubsystemNativeImpl->CaptureEventWithScope(Event->GetNativeObject(), FSentryScopeDelegate::CreateLambda([OnConfigureScope](TSharedPtr NativeScope) - { - USentryScope* UnrealScope = USentryScope::Create(NativeScope); - OnConfigureScope.ExecuteIfBound(UnrealScope); - })); + if (TSharedPtr SentryId = SubsystemNativeImpl->CaptureEventWithScope(Event->GetNativeObject(), Lambda)) + { + return SentryId->ToString(); + } + } - return SentryId->ToString(); + return FString(); } void USentrySubsystem::CaptureUserFeedback(USentryUserFeedback* UserFeedback) @@ -327,12 +327,10 @@ void USentrySubsystem::CaptureUserFeedback(USentryUserFeedback* UserFeedback) check(SubsystemNativeImpl); check(UserFeedback); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && UserFeedback) { - return; + SubsystemNativeImpl->CaptureUserFeedback(UserFeedback->GetNativeObject()); } - - SubsystemNativeImpl->CaptureUserFeedback(UserFeedback->GetNativeObject()); } void USentrySubsystem::CaptureUserFeedbackWithParams(const FString& EventId, const FString& Email, const FString& Comments, const FString& Name) @@ -340,14 +338,14 @@ void USentrySubsystem::CaptureUserFeedbackWithParams(const FString& EventId, con check(SubsystemNativeImpl); check(!EventId.IsEmpty()); - USentryUserFeedback* UserFeedback = USentryUserFeedback::Create(CreateSharedSentryUserFeedback(EventId)); - check(UserFeedback); - - UserFeedback->SetEmail(Email); - UserFeedback->SetComment(Comments); - UserFeedback->SetName(Name); + if (USentryUserFeedback* UserFeedback = USentryUserFeedback::Create(CreateSharedSentryUserFeedback(EventId))) + { + UserFeedback->SetEmail(Email); + UserFeedback->SetComment(Comments); + UserFeedback->SetName(Name); - CaptureUserFeedback(UserFeedback); + CaptureUserFeedback(UserFeedback); + } } void USentrySubsystem::SetUser(USentryUser* User) @@ -355,111 +353,95 @@ void USentrySubsystem::SetUser(USentryUser* User) check(SubsystemNativeImpl); check(User); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && User) { - return; + SubsystemNativeImpl->SetUser(User->GetNativeObject()); } - - SubsystemNativeImpl->SetUser(User->GetNativeObject()); } void USentrySubsystem::RemoveUser() { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->RemoveUser(); } - - SubsystemNativeImpl->RemoveUser(); } void USentrySubsystem::SetContext(const FString& Key, const TMap& Values) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->SetContext(Key, Values); } - - SubsystemNativeImpl->SetContext(Key, Values); } void USentrySubsystem::SetTag(const FString& Key, const FString& Value) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->SetTag(Key, Value); } - - SubsystemNativeImpl->SetTag(Key, Value); } void USentrySubsystem::RemoveTag(const FString& Key) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->RemoveTag(Key); } - - SubsystemNativeImpl->RemoveTag(Key); } void USentrySubsystem::SetLevel(ESentryLevel Level) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->SetLevel(Level); } - - SubsystemNativeImpl->SetLevel(Level); } void USentrySubsystem::StartSession() { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->StartSession(); } - - SubsystemNativeImpl->StartSession(); } void USentrySubsystem::EndSession() { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return; + SubsystemNativeImpl->EndSession(); } - - SubsystemNativeImpl->EndSession(); } USentryTransaction* USentrySubsystem::StartTransaction(const FString& Name, const FString& Operation) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return nullptr; + if (TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransaction(Name, Operation)) + { + return USentryTransaction::Create(SentryTransaction); + } } - TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransaction(Name, Operation); - check(SentryTransaction); - - return USentryTransaction::Create(SentryTransaction); + return nullptr; } USentryTransaction* USentrySubsystem::StartTransactionWithContext(USentryTransactionContext* Context) @@ -467,15 +449,15 @@ USentryTransaction* USentrySubsystem::StartTransactionWithContext(USentryTransac check(SubsystemNativeImpl); check(Context); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Context) { - return nullptr; + if (TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContext(Context->GetNativeObject())) + { + return USentryTransaction::Create(SentryTransaction); + } } - TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContext(Context->GetNativeObject()); - check(SentryTransaction); - - return USentryTransaction::Create(SentryTransaction); + return nullptr; } USentryTransaction* USentrySubsystem::StartTransactionWithContextAndTimestamp(USentryTransactionContext* Context, int64 Timestamp) @@ -483,15 +465,15 @@ USentryTransaction* USentrySubsystem::StartTransactionWithContextAndTimestamp(US check(SubsystemNativeImpl); check(Context); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Context) { - return nullptr; + if (TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContextAndTimestamp(Context->GetNativeObject(), Timestamp)) + { + return USentryTransaction::Create(SentryTransaction); + } } - TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContextAndTimestamp(Context->GetNativeObject(), Timestamp); - check(SentryTransaction); - - return USentryTransaction::Create(SentryTransaction); + return nullptr; } USentryTransaction* USentrySubsystem::StartTransactionWithContextAndOptions(USentryTransactionContext* Context, const TMap& Options) @@ -499,30 +481,30 @@ USentryTransaction* USentrySubsystem::StartTransactionWithContextAndOptions(USen check(SubsystemNativeImpl); check(Context); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled() && Context) { - return nullptr; + if (TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContextAndOptions(Context->GetNativeObject(), Options)) + { + return USentryTransaction::Create(SentryTransaction); + } } - TSharedPtr SentryTransaction = SubsystemNativeImpl->StartTransactionWithContextAndOptions(Context->GetNativeObject(), Options); - check(SentryTransaction); - - return USentryTransaction::Create(SentryTransaction); + return nullptr; } USentryTransactionContext* USentrySubsystem::ContinueTrace(const FString& SentryTrace, const TArray& BaggageHeaders) { check(SubsystemNativeImpl); - if (!SubsystemNativeImpl || !SubsystemNativeImpl->IsEnabled()) + if (SubsystemNativeImpl && SubsystemNativeImpl->IsEnabled()) { - return nullptr; + if (TSharedPtr SentryTransactionContext = SubsystemNativeImpl->ContinueTrace(SentryTrace, BaggageHeaders)) + { + return USentryTransactionContext::Create(SentryTransactionContext); + } } - TSharedPtr SentryTransactionContext = SubsystemNativeImpl->ContinueTrace(SentryTrace, BaggageHeaders); - check(SentryTransactionContext); - - return USentryTransactionContext::Create(SentryTransactionContext); + return nullptr; } bool USentrySubsystem::IsSupportedForCurrentSettings() const diff --git a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp index 44831c9a4..1ba9f3c5a 100644 --- a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp +++ b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp @@ -33,6 +33,14 @@ void SentrySubsystemSpec::Define() } }); + Describe("Add Breadcrumb", [this]() + { + It("should gracefully handle a nullptr argument", [this]() + { + SentrySubsystem->AddBreadcrumb(nullptr); + }); + }); + Describe("Capture Message", [this]() { It("should return a non-null Event ID if message captured", [this]() @@ -51,6 +59,12 @@ void SentrySubsystemSpec::Define() Describe("Capture Event", [this]() { + It("should gracefully handle a nullptr argument", [this]() + { + FString EventId = SentrySubsystem->CaptureEvent(nullptr); + TestTrue("Event ID is empty", EventId.IsEmpty()); + }); + It("should return a non-null Event ID if event captured", [this]() { USentryEvent* testEvent = USentryEvent::Create(CreateSharedSentryEvent()); @@ -60,6 +74,14 @@ void SentrySubsystemSpec::Define() TestFalse("Event ID is non-empty", eventId.IsEmpty()); }); + It("should gracefully handle a nullptr argument if scoped version used", [this]() + { + const FConfigureScopeNativeDelegate TestDelegate; + + FString EventId = SentrySubsystem->CaptureEventWithScope(nullptr, TestDelegate); + TestTrue("Event ID is empty", EventId.IsEmpty()); + }); + It("should always return non-null Event ID if scoped version used", [this]() { USentryEvent* testEvent = USentryEvent::Create(CreateSharedSentryEvent()); @@ -98,6 +120,12 @@ void SentrySubsystemSpec::Define() TestTrue("Transaction is finished", transaction->IsFinished()); }); + It("should gracefully handle a nullptr context argument", [this]() + { + USentryTransaction* Transaction = SentrySubsystem->StartTransactionWithContext(nullptr); + TestNull("Transaction is null", Transaction); + }); + It("should be started and finished with specific context", [this]() { USentryTransactionContext* transactionContext = @@ -112,6 +140,12 @@ void SentrySubsystemSpec::Define() TestTrue("Transaction is finished", transaction->IsFinished()); }); + It("should gracefully handle a nullptr context argument with timings", [this]() + { + USentryTransaction* Transaction = SentrySubsystem->StartTransactionWithContextAndTimestamp(nullptr, FDateTime::UtcNow().ToUnixTimestamp()); + TestNull("Transaction is null", Transaction); + }); + It("should be started and finished with specific context and timings", [this]() { USentryTransactionContext* transactionContext = @@ -125,6 +159,14 @@ void SentrySubsystemSpec::Define() transaction->FinishWithTimestamp(FDateTime::UtcNow().ToUnixTimestamp()); TestTrue("Transaction is finished", transaction->IsFinished()); }); + + It("should gracefully handle a nullptr context argument and options", [this]() + { + TMap Options; + + USentryTransaction* Transaction = SentrySubsystem->StartTransactionWithContextAndOptions(nullptr, Options); + TestNull("Transaction is null", Transaction); + }); }); Describe("Transaction context", [this]() @@ -152,6 +194,22 @@ void SentrySubsystemSpec::Define() }); }); + Describe("User", [this]() + { + It("should gracefully handle a nullptr argument", [this]() + { + SentrySubsystem->SetUser(nullptr); + }); + }); + + Describe("User Feedback", [this]() + { + It("should gracefully handle a nullptr argument", [this]() + { + SentrySubsystem->CaptureUserFeedback(nullptr); + }); + }); + AfterEach([this] { SentrySubsystem->Close(); From 25cadc23c83e5971a438b0dd8fb5ca0bad8969d8 Mon Sep 17 00:00:00 2001 From: Nathan White Date: Mon, 9 Jun 2025 09:24:27 -0700 Subject: [PATCH 2/3] Test expected message for check --- .../Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp index 1ba9f3c5a..174edda14 100644 --- a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp +++ b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp @@ -37,6 +37,9 @@ void SentrySubsystemSpec::Define() { It("should gracefully handle a nullptr argument", [this]() { + // We have check(...) for parameters + AddExpectedMessage(TEXT("Assertion failed: Breadcrumb"), EAutomationExpectedErrorFlags::Contains); + SentrySubsystem->AddBreadcrumb(nullptr); }); }); From b571a517c72487569c079adb007a4f422694dae8 Mon Sep 17 00:00:00 2001 From: Nathan White Date: Mon, 9 Jun 2025 09:47:11 -0700 Subject: [PATCH 3/3] Use different method --- plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp index 174edda14..5fdd5ee38 100644 --- a/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp +++ b/plugin-dev/Source/Sentry/Private/Tests/SentrySubsystem.spec.cpp @@ -38,7 +38,7 @@ void SentrySubsystemSpec::Define() It("should gracefully handle a nullptr argument", [this]() { // We have check(...) for parameters - AddExpectedMessage(TEXT("Assertion failed: Breadcrumb"), EAutomationExpectedErrorFlags::Contains); + AddExpectedError(TEXT("Assertion failed: Breadcrumb")); SentrySubsystem->AddBreadcrumb(nullptr); });