From 0b55d210ffb5a347aeb745550f4ac4112beedbca Mon Sep 17 00:00:00 2001 From: Matthijs Date: Wed, 2 Dec 2020 21:12:02 +0100 Subject: [PATCH 01/10] Do not overwrite Event properties if they are already configured. Merge tags from options and event. --- src/Client.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Client.php b/src/Client.php index 35081c4af7..84082211b4 100644 --- a/src/Client.php +++ b/src/Client.php @@ -225,12 +225,25 @@ private function prepareEvent(Event $event, ?EventHint $hint = null, ?Scope $sco $this->addMissingStacktraceToEvent($event); + // Do not overwrite event data if it's already set + if (null !== $event->getSdkIdentifier()) { $event->setSdkIdentifier($this->sdkIdentifier); + } + if (null !== $event->getSdkVersion()) { $event->setSdkVersion($this->sdkVersion); + } + if (null !== $event->getServerName()) { $event->setServerName($this->options->getServerName()); + } + if (null !== $event->getRelease()) { $event->setRelease($this->options->getRelease()); - $event->setTags($this->options->getTags()); + } + if (null !== $event->getEnvironment()) { $event->setEnvironment($this->options->getEnvironment()); + } + + // Merge tags from the event and from the main options + $event->setTags(array_merge($event->getTags(), $this->options->getTags())); if (null === $event->getLogger()) { $event->setLogger($this->options->getLogger()); From 1353c78de3b0f7e531b6976b0f240d2bd97ea22d Mon Sep 17 00:00:00 2001 From: Matthijs Date: Fri, 4 Dec 2020 23:14:20 +0100 Subject: [PATCH 02/10] Code review client fixes --- src/Client.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/Client.php b/src/Client.php index 84082211b4..78a17a5ca0 100644 --- a/src/Client.php +++ b/src/Client.php @@ -225,24 +225,26 @@ private function prepareEvent(Event $event, ?EventHint $hint = null, ?Scope $sco $this->addMissingStacktraceToEvent($event); - // Do not overwrite event data if it's already set - if (null !== $event->getSdkIdentifier()) { + if (null === $event->getSdkIdentifier()) { $event->setSdkIdentifier($this->sdkIdentifier); } - if (null !== $event->getSdkVersion()) { + + if (null === $event->getSdkVersion()) { $event->setSdkVersion($this->sdkVersion); } - if (null !== $event->getServerName()) { + + if (null === $event->getServerName()) { $event->setServerName($this->options->getServerName()); } - if (null !== $event->getRelease()) { + + if (null === $event->getRelease()) { $event->setRelease($this->options->getRelease()); } - if (null !== $event->getEnvironment()) { + + if (null === $event->getEnvironment()) { $event->setEnvironment($this->options->getEnvironment()); } - // Merge tags from the event and from the main options $event->setTags(array_merge($event->getTags(), $this->options->getTags())); if (null === $event->getLogger()) { From cc7dd580893a20bff83386bfe52cd7f2ad5f877c Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 7 Jan 2021 12:39:36 +0100 Subject: [PATCH 03/10] Edit changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90136affcc..d7b1b63203 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Fix unwanted call to the `before_send` callback with transaction events, use `traces_sampler` instead to filter transactions (#1158) - Fix the `logger` option not being applied to the event object (#1165) +- Fix Event properties being overwritten when saving Event (#1148) ## 3.1.1 (2020-12-07) From 0e59ec04d01ed3a9f26389830f750f494a4cb57e Mon Sep 17 00:00:00 2001 From: Matthijs Date: Fri, 4 Dec 2020 23:23:16 +0100 Subject: [PATCH 04/10] Fix codequality tool errors --- src/Client.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Client.php b/src/Client.php index 78a17a5ca0..c021eb2365 100644 --- a/src/Client.php +++ b/src/Client.php @@ -225,11 +225,11 @@ private function prepareEvent(Event $event, ?EventHint $hint = null, ?Scope $sco $this->addMissingStacktraceToEvent($event); - if (null === $event->getSdkIdentifier()) { + if ('' === $event->getSdkIdentifier()) { $event->setSdkIdentifier($this->sdkIdentifier); } - if (null === $event->getSdkVersion()) { + if ('' === $event->getSdkVersion()) { $event->setSdkVersion($this->sdkVersion); } From 56b729cf8185909bd13af6a5fac5a7710b4f2185 Mon Sep 17 00:00:00 2001 From: Matthijs Date: Fri, 4 Dec 2020 23:32:30 +0100 Subject: [PATCH 05/10] Add test for cacptureEvent --- tests/ClientTest.php | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 112c4cb266..8ee66dd0ff 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -492,6 +492,50 @@ public function testBuildEventWithDefaultValues(): void $client->captureEvent(Event::createEvent()); } + public function testBuildEventDontOverwriteEventPropertiesWithDefaultValues(): void + { + $options = new Options(); + $options->setServerName('testServerName'); + $options->setRelease('testRelease'); + $options->setTags(['test2' => 'tag2']); + $options->setEnvironment('testEnvironment'); + + /** @var TransportInterface&MockObject $transport */ + $transport = $this->createMock(TransportInterface::class); + $transport->expects($this->once()) + ->method('send') + ->with($this->callback(function (Event $event) use ($options): bool { + $this->assertSame('sdk.identifier', $event->getSdkIdentifier()); + $this->assertSame('4.2.0', $event->getSdkVersion()); + $this->assertSame('Debian', $event->getServerName()); + $this->assertSame('42', $event->getRelease()); + $this->assertSame(['test1' => 'tag1', 'test2' => 'tag2'], $event->getTags()); + $this->assertSame('Production', $event->getEnvironment()); + $this->assertNull($event->getStacktrace()); + + return true; + })); + + $client = new Client( + $options, + $transport, + 'sentry.sdk.identifier', + '1.2.3', + $this->createMock(SerializerInterface::class), + $this->createMock(RepresentationSerializerInterface::class) + ); + + $event = Event::createEvent(); + $event->setSdkIdentifier('sdk.identifier'); + $event->setSdkVersion('4.2.0'); + $event->setServerName('Debian'); + $event->setRelease('42'); + $event->setEnvironment('Production'); + $event->setTags(['test1' => 'tag1']); + + $client->captureEvent($event); + } + public function testBuildEventInCLIDoesntSetTransaction(): void { /** @var TransportInterface&MockObject $transport */ From f6d6f4c19a422df3283f5c9d801be2c66361715c Mon Sep 17 00:00:00 2001 From: Matthijs Date: Fri, 4 Dec 2020 23:53:07 +0100 Subject: [PATCH 06/10] Fix testBuildEventWithDefaultValues SDK identifier is no longer overwritten if the event already has a value set --- tests/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 8ee66dd0ff..781ad75946 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -468,7 +468,7 @@ public function testBuildEventWithDefaultValues(): void $transport->expects($this->once()) ->method('send') ->with($this->callback(function (Event $event) use ($options): bool { - $this->assertSame('sentry.sdk.identifier', $event->getSdkIdentifier()); + $this->assertSame('sentry.php', $event->getSdkIdentifier()); $this->assertSame('1.2.3', $event->getSdkVersion()); $this->assertSame($options->getServerName(), $event->getServerName()); $this->assertSame($options->getRelease(), $event->getRelease()); From 21da7259d1c483019ff9e15ff7e10cd3207a4ea2 Mon Sep 17 00:00:00 2001 From: Matthijs Date: Fri, 4 Dec 2020 23:59:38 +0100 Subject: [PATCH 07/10] Pass SdkIdentifier and version to captureEvent --- src/Client.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Client.php b/src/Client.php index c021eb2365..fd19aaa5e9 100644 --- a/src/Client.php +++ b/src/Client.php @@ -128,6 +128,8 @@ public function captureMessage(string $message, ?Severity $level = null, ?Scope $event = Event::createEvent(); $event->setMessage($message); $event->setLevel($level); + $event->setSdkIdentifier($this->sdkIdentifier); + $event->setSdkVersion($this->sdkVersion); return $this->captureEvent($event, null, $scope); } From 2344a67fa959b7e7e35b1e4893d919edf25b23b8 Mon Sep 17 00:00:00 2001 From: Matthijs Date: Sat, 5 Dec 2020 00:23:23 +0100 Subject: [PATCH 08/10] Update ClientTest.php --- tests/ClientTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 781ad75946..8ee66dd0ff 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -468,7 +468,7 @@ public function testBuildEventWithDefaultValues(): void $transport->expects($this->once()) ->method('send') ->with($this->callback(function (Event $event) use ($options): bool { - $this->assertSame('sentry.php', $event->getSdkIdentifier()); + $this->assertSame('sentry.sdk.identifier', $event->getSdkIdentifier()); $this->assertSame('1.2.3', $event->getSdkVersion()); $this->assertSame($options->getServerName(), $event->getServerName()); $this->assertSame($options->getRelease(), $event->getRelease()); From fc159c05fbdf3f2ecdce9c856e10f0cb08e17ef5 Mon Sep 17 00:00:00 2001 From: Matthijs Date: Sat, 5 Dec 2020 00:25:50 +0100 Subject: [PATCH 09/10] Update ClientTest.php --- tests/ClientTest.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 8ee66dd0ff..153a5182cb 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -468,8 +468,6 @@ public function testBuildEventWithDefaultValues(): void $transport->expects($this->once()) ->method('send') ->with($this->callback(function (Event $event) use ($options): bool { - $this->assertSame('sentry.sdk.identifier', $event->getSdkIdentifier()); - $this->assertSame('1.2.3', $event->getSdkVersion()); $this->assertSame($options->getServerName(), $event->getServerName()); $this->assertSame($options->getRelease(), $event->getRelease()); $this->assertSame($options->getTags(), $event->getTags()); From c57a668f904b3ae4c87aec9771eadce2a7e244c2 Mon Sep 17 00:00:00 2001 From: Stefano Arlandini Date: Thu, 7 Jan 2021 01:54:56 +0100 Subject: [PATCH 10/10] Fix CR issues --- CHANGELOG.md | 2 +- src/Client.php | 22 +++---- tests/ClientTest.php | 136 ++++++++++++++----------------------------- 3 files changed, 51 insertions(+), 109 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b1b63203..ac4e201cf2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ - Fix unwanted call to the `before_send` callback with transaction events, use `traces_sampler` instead to filter transactions (#1158) - Fix the `logger` option not being applied to the event object (#1165) -- Fix Event properties being overwritten when saving Event (#1148) +- Fix a bug that made some event attributes being overwritten by option config values when calling `captureEvent()` (#1148) ## 3.1.1 (2020-12-07) diff --git a/src/Client.php b/src/Client.php index fd19aaa5e9..8618b9a917 100644 --- a/src/Client.php +++ b/src/Client.php @@ -128,8 +128,6 @@ public function captureMessage(string $message, ?Severity $level = null, ?Scope $event = Event::createEvent(); $event->setMessage($message); $event->setLevel($level); - $event->setSdkIdentifier($this->sdkIdentifier); - $event->setSdkVersion($this->sdkVersion); return $this->captureEvent($event, null, $scope); } @@ -227,27 +225,21 @@ private function prepareEvent(Event $event, ?EventHint $hint = null, ?Scope $sco $this->addMissingStacktraceToEvent($event); - if ('' === $event->getSdkIdentifier()) { $event->setSdkIdentifier($this->sdkIdentifier); - } - - if ('' === $event->getSdkVersion()) { $event->setSdkVersion($this->sdkVersion); - } - + $event->setTags(array_merge($this->options->getTags(), $event->getTags())); + if (null === $event->getServerName()) { - $event->setServerName($this->options->getServerName()); + $event->setServerName($this->options->getServerName()); } - + if (null === $event->getRelease()) { - $event->setRelease($this->options->getRelease()); + $event->setRelease($this->options->getRelease()); } - + if (null === $event->getEnvironment()) { - $event->setEnvironment($this->options->getEnvironment()); + $event->setEnvironment($this->options->getEnvironment()); } - - $event->setTags(array_merge($event->getTags(), $this->options->getTags())); if (null === $event->getLogger()) { $event->setLogger($this->options->getLogger()); diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 153a5182cb..5c446896ff 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -14,6 +14,7 @@ use Sentry\ClientBuilder; use Sentry\Event; use Sentry\EventHint; +use Sentry\EventId; use Sentry\ExceptionMechanism; use Sentry\Frame; use Sentry\Integration\IntegrationInterface; @@ -28,7 +29,6 @@ use Sentry\State\Scope; use Sentry\Transport\TransportFactoryInterface; use Sentry\Transport\TransportInterface; -use Sentry\UserDataBag; final class ClientTest extends TestCase { @@ -132,29 +132,59 @@ public function testCaptureException(): void $this->assertNotNull($client->captureException($exception)); } - public function testCaptureEvent(): void + /** + * @dataProvider captureEventDataProvider + */ + public function testCaptureEvent(array $options, Event $event, Event $expectedEvent): void { - /** @var TransportInterface&MockObject $transport */ $transport = $this->createMock(TransportInterface::class); $transport->expects($this->once()) ->method('send') - ->willReturnCallback(static function (Event $event): FulfilledPromise { + ->willReturnCallback(function (Event $event) use ($expectedEvent): FulfilledPromise { + $this->assertEquals($expectedEvent, $event); + return new FulfilledPromise(new Response(ResponseStatus::success(), $event)); }); - $client = ClientBuilder::create() + $client = ClientBuilder::create($options) ->setTransportFactory($this->createTransportFactory($transport)) ->getClient(); - $event = Event::createEvent(); - $event->setTransaction('foo bar'); - $event->setLevel(Severity::debug()); - $event->setLogger('foo'); - $event->setTags(['foo', 'bar']); - $event->setExtra(['foo' => 'bar']); - $event->setUser(UserDataBag::createFromUserIdentifier('foo')); + $this->assertSame($event->getId(), $client->captureEvent($event)); + } - $this->assertNotNull($client->captureEvent($event)); + public function captureEventDataProvider(): \Generator + { + $eventId = EventId::generate(); + $event = Event::createEvent($eventId); + + yield 'Options set && no event properties set => use options' => [ + [ + 'server_name' => 'example.com', + 'release' => '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33', + 'environment' => 'development', + 'tags' => ['context' => 'development'], + ], + $event, + $event, + ]; + + $event = Event::createEvent($eventId); + $event->setServerName('foo.example.com'); + $event->setRelease('721e41770371db95eee98ca2707686226b993eda'); + $event->setEnvironment('production'); + $event->setTags(['context' => 'production']); + + yield 'Options set && event properties set => event properties override options' => [ + [ + 'server_name' => 'example.com', + 'release' => '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33', + 'environment' => 'development', + 'tags' => ['context' => 'development', 'ios_version' => '14.0'], + ], + $event, + $event, + ]; } /** @@ -454,86 +484,6 @@ public function testFlush(): void $this->assertTrue($promise->wait()); } - public function testBuildEventWithDefaultValues(): void - { - $options = new Options(); - $options->setServerName('testServerName'); - $options->setRelease('testRelease'); - $options->setTags(['test' => 'tag']); - $options->setEnvironment('testEnvironment'); - $options->setLogger('app.logger'); - - /** @var TransportInterface&MockObject $transport */ - $transport = $this->createMock(TransportInterface::class); - $transport->expects($this->once()) - ->method('send') - ->with($this->callback(function (Event $event) use ($options): bool { - $this->assertSame($options->getServerName(), $event->getServerName()); - $this->assertSame($options->getRelease(), $event->getRelease()); - $this->assertSame($options->getTags(), $event->getTags()); - $this->assertSame($options->getEnvironment(), $event->getEnvironment()); - $this->assertSame($options->getLogger(), $event->getLogger()); - $this->assertNull($event->getStacktrace()); - - return true; - })); - - $client = new Client( - $options, - $transport, - 'sentry.sdk.identifier', - '1.2.3', - $this->createMock(SerializerInterface::class), - $this->createMock(RepresentationSerializerInterface::class) - ); - - $client->captureEvent(Event::createEvent()); - } - - public function testBuildEventDontOverwriteEventPropertiesWithDefaultValues(): void - { - $options = new Options(); - $options->setServerName('testServerName'); - $options->setRelease('testRelease'); - $options->setTags(['test2' => 'tag2']); - $options->setEnvironment('testEnvironment'); - - /** @var TransportInterface&MockObject $transport */ - $transport = $this->createMock(TransportInterface::class); - $transport->expects($this->once()) - ->method('send') - ->with($this->callback(function (Event $event) use ($options): bool { - $this->assertSame('sdk.identifier', $event->getSdkIdentifier()); - $this->assertSame('4.2.0', $event->getSdkVersion()); - $this->assertSame('Debian', $event->getServerName()); - $this->assertSame('42', $event->getRelease()); - $this->assertSame(['test1' => 'tag1', 'test2' => 'tag2'], $event->getTags()); - $this->assertSame('Production', $event->getEnvironment()); - $this->assertNull($event->getStacktrace()); - - return true; - })); - - $client = new Client( - $options, - $transport, - 'sentry.sdk.identifier', - '1.2.3', - $this->createMock(SerializerInterface::class), - $this->createMock(RepresentationSerializerInterface::class) - ); - - $event = Event::createEvent(); - $event->setSdkIdentifier('sdk.identifier'); - $event->setSdkVersion('4.2.0'); - $event->setServerName('Debian'); - $event->setRelease('42'); - $event->setEnvironment('Production'); - $event->setTags(['test1' => 'tag1']); - - $client->captureEvent($event); - } - public function testBuildEventInCLIDoesntSetTransaction(): void { /** @var TransportInterface&MockObject $transport */