From 0daecc82815545be036e5492199da3884b801c8c Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 13 May 2022 07:53:28 +0200 Subject: [PATCH 01/15] Store attachments in hints and allow manipulation in beforeSend and eventProcessor --- sentry/api/sentry.api | 14 ++ .../src/main/java/io/sentry/SentryClient.java | 38 ++-- .../main/java/io/sentry/TypeCheckHint.java | 2 + .../io/sentry/hints/AttachmentContainer.java | 39 ++++ .../src/main/java/io/sentry/hints/Hints.java | 52 ++++- .../main/java/io/sentry/util/HintUtils.java | 2 +- .../test/java/io/sentry/SentryClientTest.kt | 201 +++++++++++++++++- .../sentry/hints/AttachmentContainerTest.kt | 60 ++++++ .../test/java/io/sentry/hints/HintsTest.kt | 101 +++++++++ 9 files changed, 482 insertions(+), 27 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/hints/AttachmentContainer.java create mode 100644 sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt create mode 100644 sentry/src/test/java/io/sentry/hints/HintsTest.kt diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 2489f2d3bb0..1d4468533b4 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1638,6 +1638,7 @@ public final class io/sentry/TypeCheckHint { public static final field OKHTTP_RESPONSE Ljava/lang/String; public static final field OPEN_FEIGN_REQUEST Ljava/lang/String; public static final field OPEN_FEIGN_RESPONSE Ljava/lang/String; + public static final field SENTRY_ATTACHMENTS Ljava/lang/String; public static final field SENTRY_SCREENSHOT Ljava/lang/String; public static final field SENTRY_SYNTHETIC_EXCEPTION Ljava/lang/String; public static final field SENTRY_TYPE_CHECK_HINT Ljava/lang/String; @@ -1835,6 +1836,15 @@ public final class io/sentry/exception/SentryEnvelopeException : java/lang/Excep public abstract interface class io/sentry/hints/ApplyScopeData { } +public final class io/sentry/hints/AttachmentContainer { + public fun ()V + public fun add (Lio/sentry/Attachment;)V + public fun addAll (Ljava/util/List;)V + public fun clear ()V + public fun getAll ()Ljava/util/List; + public fun replaceAll (Ljava/util/List;)V +} + public abstract interface class io/sentry/hints/Cached { } @@ -1849,8 +1859,12 @@ public abstract interface class io/sentry/hints/Flushable { public final class io/sentry/hints/Hints { public fun ()V public fun get (Ljava/lang/String;)Ljava/lang/Object; + public fun getAs (Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object; + public fun getAttachmentContainer ()Lio/sentry/hints/AttachmentContainer; public fun remove (Ljava/lang/String;)V public fun set (Ljava/lang/String;Ljava/lang/Object;)V + public static fun withAttachment (Lio/sentry/Attachment;)Lio/sentry/hints/Hints; + public static fun withAttachments (Ljava/util/List;)Lio/sentry/hints/Hints; } public abstract interface class io/sentry/hints/Resettable { diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 790434e6c96..a09824ed9c1 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -80,6 +80,11 @@ private boolean shouldApplyScopeData( hints = new Hints(); } + // TODO what does cached etc mean for devs manipulating hints in beforeSend and eventProcessor? + if (shouldApplyScopeData(event, hints)) { + addScopeAttachmentsToHints(scope, hints); + } + options.getLogger().log(SentryLevel.DEBUG, "Capturing event: %s", event.getEventId()); if (event != null) { @@ -173,7 +178,7 @@ private boolean shouldApplyScopeData( ? scope.getTransaction().traceState() : null; final boolean shouldSendAttachments = event != null; - List attachments = shouldSendAttachments ? getAttachments(scope, hints) : null; + List attachments = shouldSendAttachments ? getAttachments(hints) : null; final SentryEnvelope envelope = buildEnvelope(event, attachments, session, traceState, null); if (envelope != null) { @@ -189,6 +194,12 @@ private boolean shouldApplyScopeData( return sentryId; } + private void addScopeAttachmentsToHints(@Nullable Scope scope, @NotNull Hints hints) { + if (scope != null) { + hints.getAttachmentContainer().addAll(scope.getAttachments()); + } + } + private boolean shouldSendSessionUpdateForDroppedEvent( @Nullable Session sessionBeforeUpdate, @Nullable Session sessionAfterUpdate) { if (sessionAfterUpdate == null) { @@ -215,21 +226,12 @@ private boolean shouldSendSessionUpdateForDroppedEvent( return false; } - private @Nullable List getAttachments( - final @Nullable Scope scope, final @NotNull Hints hints) { - List attachments = null; - if (scope != null) { - attachments = scope.getAttachments(); - } - - final Object screenshotAttachment = hints.get(SENTRY_SCREENSHOT); - if (screenshotAttachment instanceof Attachment) { - - if (attachments == null) { - attachments = new ArrayList<>(); - } + private @Nullable List getAttachments(final @NotNull Hints hints) { + @NotNull final List attachments = hints.getAttachmentContainer().getAll(); - attachments.add((Attachment) screenshotAttachment); + @Nullable final Attachment screenshot = hints.getAs(SENTRY_SCREENSHOT, Attachment.class); + if (screenshot != null) { + attachments.add(screenshot); } return attachments; @@ -506,6 +508,10 @@ public void captureSession(final @NotNull Session session, final @Nullable Hints hints = new Hints(); } + if (shouldApplyScopeData(transaction, hints)) { + addScopeAttachmentsToHints(scope, hints); + } + options .getLogger() .log(SentryLevel.DEBUG, "Capturing transaction: %s", transaction.getEventId()); @@ -540,7 +546,7 @@ public void captureSession(final @NotNull Session session, final @Nullable Hints final SentryEnvelope envelope = buildEnvelope( transaction, - filterForTransaction(getAttachments(scope, hints)), + filterForTransaction(getAttachments(hints)), null, traceState, profilingTraceData); diff --git a/sentry/src/main/java/io/sentry/TypeCheckHint.java b/sentry/src/main/java/io/sentry/TypeCheckHint.java index 9bf5a79f14f..e79bb2f767b 100644 --- a/sentry/src/main/java/io/sentry/TypeCheckHint.java +++ b/sentry/src/main/java/io/sentry/TypeCheckHint.java @@ -27,6 +27,8 @@ public final class TypeCheckHint { public static final String ANDROID_FRAGMENT = "android:fragment"; /** Used for screenshots. */ public static final String SENTRY_SCREENSHOT = "sentry:screenshot"; + /** Used for attachments. */ + public static final String SENTRY_ATTACHMENTS = "sentry:attachments"; /** Used for OkHttp response breadcrumbs. */ public static final String OKHTTP_RESPONSE = "okHttp:response"; diff --git a/sentry/src/main/java/io/sentry/hints/AttachmentContainer.java b/sentry/src/main/java/io/sentry/hints/AttachmentContainer.java new file mode 100644 index 00000000000..133fe7057d9 --- /dev/null +++ b/sentry/src/main/java/io/sentry/hints/AttachmentContainer.java @@ -0,0 +1,39 @@ +package io.sentry.hints; + +import io.sentry.Attachment; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +public final class AttachmentContainer { + + private final @NotNull List internalStorage = new CopyOnWriteArrayList<>(); + + public void add(@Nullable Attachment attachment) { + if (attachment != null) { + internalStorage.add(attachment); + } + } + + @SuppressWarnings("unchecked") + public void addAll(@Nullable List attachments) { + if (attachments != null) { + internalStorage.addAll(attachments); + } + } + + @SuppressWarnings("unchecked") + public @NotNull List getAll() { + return new CopyOnWriteArrayList<>(internalStorage); + } + + public void replaceAll(@Nullable List attachments) { + clear(); + addAll(attachments); + } + + public void clear() { + internalStorage.clear(); + } +} diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index 51ea2b57ca6..208350eb596 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -1,6 +1,10 @@ package io.sentry.hints; +import static io.sentry.TypeCheckHint.SENTRY_ATTACHMENTS; + +import io.sentry.Attachment; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -9,21 +13,51 @@ public final class Hints { private final @NotNull Map internalStorage = new HashMap(); + public static @NotNull Hints withAttachment(@Nullable Attachment attachment) { + @NotNull final Hints hints = new Hints(); + hints.getAttachmentContainer().add(attachment); + return hints; + } + + public static @NotNull Hints withAttachments(@Nullable List attachments) { + @NotNull final Hints hints = new Hints(); + hints.getAttachmentContainer().addAll(attachments); + return hints; + } + public void set(@NotNull String hintType, @Nullable Object hint) { internalStorage.put(hintType, hint); } - public @Nullable Object get(@NotNull String hintType) { - return internalStorage.get(hintType); + public @Nullable Object get(@NotNull String hintName) { + return internalStorage.get(hintName); } - // TODO maybe not public - public void remove(@NotNull String hintType) { - internalStorage.remove(hintType); + @SuppressWarnings("unchecked") + public @Nullable T getAs(@NotNull String hintName, @NotNull Class clazz) { + Object hintValue = internalStorage.get(hintName); + + if (clazz.isInstance(hintValue)) { + return (T) hintValue; + } else { + return null; + } + } + + public void remove(@NotNull String hintName) { + internalStorage.remove(hintName); } - // TODO addAttachment(one) - // TODO getAttachments(): List - // TODO setAttachments(list) - // TODO clearAttachments() + public @NotNull AttachmentContainer getAttachmentContainer() { + if (internalStorage.containsKey(SENTRY_ATTACHMENTS)) { + AttachmentContainer container = getAs(SENTRY_ATTACHMENTS, AttachmentContainer.class); + if (container != null) { + return container; + } + } + + AttachmentContainer attachmentContainer = new AttachmentContainer(); + internalStorage.put(SENTRY_ATTACHMENTS, attachmentContainer); + return attachmentContainer; + } } diff --git a/sentry/src/main/java/io/sentry/util/HintUtils.java b/sentry/src/main/java/io/sentry/util/HintUtils.java index cf7bafb8fcb..78e802076a4 100644 --- a/sentry/src/main/java/io/sentry/util/HintUtils.java +++ b/sentry/src/main/java/io/sentry/util/HintUtils.java @@ -10,7 +10,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -/** Util class for Applying or not scope's data to an event */ +/** Util class dealing with Hints as not to pollute the Hints API with internal functionality */ @ApiStatus.Internal public final class HintUtils { diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 0429f583c7e..815a6a86d3c 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -84,6 +84,8 @@ class SentryClientTest { } var attachment = Attachment("hello".toByteArray(), "hello.txt", "text/plain", true) + var attachment2 = Attachment("hello2".toByteArray(), "hello2.txt", "text/plain", true) + var attachment3 = Attachment("hello3".toByteArray(), "hello3.txt", "text/plain", true) val profilingTraceFile = Files.createTempFile("trace", ".trace").toFile() var profilingTraceData = ProfilingTraceData(profilingTraceFile, sentryTracer) var profilingNonExistingTraceData = ProfilingTraceData(File("non_existent.trace"), sentryTracer) @@ -1592,6 +1594,201 @@ class SentryClientTest { ) } + @Test + fun `can pass an attachment via hints`() { + val sut = fixture.getSut() + + sut.captureException(IllegalStateException(), Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 0, 1) + } + + @Test + fun `an attachment passed via hint is used with scope attachments`() { + val sut = fixture.getSut() + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 1, 2) + } + + @Test + fun `can add to attachments in beforeSend`() { + val sut = fixture.getSut { options -> + options.setBeforeSend { event, hints -> + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) + hints.attachmentContainer.add(fixture.attachment3) + event + } + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 1, 3) + } + + @Test + fun `can replace attachments in beforeSend`() { + val sut = fixture.getSut { options -> + options.setBeforeSend { event, hints -> + hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + event + } + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 1, 1) + } + + @Test + fun `can add to attachments in eventProcessor`() { + val sut = fixture.getSut { options -> + options.addEventProcessor(object : EventProcessor { + override fun process(event: SentryEvent, hints: Hints): SentryEvent? { + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) + hints.attachmentContainer.add(fixture.attachment3) + return event + } + + override fun process( + transaction: SentryTransaction, + hints: Hints + ): SentryTransaction? { + return transaction + } + }) + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 1, 3) + } + + @Test + fun `can replace attachments in eventProcessor`() { + val sut = fixture.getSut { options -> + options.addEventProcessor(object : EventProcessor { + override fun process(event: SentryEvent, hints: Hints): SentryEvent? { + hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + return event + } + + override fun process( + transaction: SentryTransaction, + hints: Hints + ): SentryTransaction? { + return transaction + } + }) + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) + + thenEnvelopeIsSentWith(1, 1, 1) + } + + @Test + fun `can pass an attachment via hints for transactions`() { + val sut = fixture.getSut() + val scope = createScope() + + sut.captureTransaction( + SentryTransaction(fixture.sentryTracer), + scope, + Hints.withAttachment(fixture.attachment) + ) + + thenEnvelopeIsSentWith(0, 0, 1, 1) + } + + @Test + fun `an attachment passed via hint is used with scope attachments for transactions`() { + val sut = fixture.getSut() + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + + sut.captureTransaction( + SentryTransaction(fixture.sentryTracer), + scope, + Hints.withAttachment(fixture.attachment) + ) + + thenEnvelopeIsSentWith(0, 0, 2, 1) + } + + @Test + fun `can add to attachments in eventProcessor for transactions`() { + val sut = fixture.getSut { options -> + options.addEventProcessor(object : EventProcessor { + override fun process(event: SentryEvent, hints: Hints): SentryEvent? { + return event + } + + override fun process( + transaction: SentryTransaction, + hints: Hints + ): SentryTransaction? { + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) + hints.attachmentContainer.add(fixture.attachment3) + return transaction + } + }) + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + + sut.captureTransaction( + SentryTransaction(fixture.sentryTracer), + scope, + Hints.withAttachment(fixture.attachment) + ) + + thenEnvelopeIsSentWith(0, 0, 3, 1) + } + + @Test + fun `can replace attachments in eventProcessor for transactions`() { + val sut = fixture.getSut { options -> + options.addEventProcessor(object : EventProcessor { + override fun process(event: SentryEvent, hints: Hints): SentryEvent? { + return event + } + + override fun process( + transaction: SentryTransaction, + hints: Hints + ): SentryTransaction? { + hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + return transaction + } + }) + } + + val scope = givenScopeWithStartedSession() + scope.addAttachment(fixture.attachment2) + + sut.captureTransaction( + SentryTransaction(fixture.sentryTracer), + scope, + Hints.withAttachment(fixture.attachment) + ) + + thenEnvelopeIsSentWith(0, 0, 1, 1) + } +// TODO breadcrumb test private fun givenScopeWithStartedSession(errored: Boolean = false, crashed: Boolean = false): Scope { val scope = createScope(fixture.sentryOptions) scope.startSession() @@ -1611,7 +1808,7 @@ class SentryClientTest { verify(fixture.transport, never()).send(anyOrNull(), anyOrNull()) } - private fun thenEnvelopeIsSentWith(eventCount: Int, sessionCount: Int) { + private fun thenEnvelopeIsSentWith(eventCount: Int, sessionCount: Int, attachmentCount: Int = 0, transactionCount: Int = 0) { val argumentCaptor = argumentCaptor() verify(fixture.transport, times(1)).send(argumentCaptor.capture(), anyOrNull()) @@ -1619,6 +1816,8 @@ class SentryClientTest { val envelopeItemTypes = envelope.items.map { it.header.type } assertEquals(eventCount, envelopeItemTypes.count { it == SentryItemType.Event }) assertEquals(sessionCount, envelopeItemTypes.count { it == SentryItemType.Session }) + assertEquals(attachmentCount, envelopeItemTypes.count { it == SentryItemType.Attachment }) + assertEquals(transactionCount, envelopeItemTypes.count { it == SentryItemType.Transaction }) } private fun thenSessionIsStillOK(scope: Scope) { diff --git a/sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt b/sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt new file mode 100644 index 00000000000..46b198db086 --- /dev/null +++ b/sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt @@ -0,0 +1,60 @@ +package io.sentry.hints + +import io.sentry.Attachment +import kotlin.test.Test +import kotlin.test.assertEquals + +class AttachmentContainerTest { + + @Test + fun `can add an attachment`() { + val container = AttachmentContainer() + val attachment = newAttachment("test1") + container.add(attachment) + + assertEquals(listOf(attachment), container.all) + } + + @Test + fun `can add multiple attachments`() { + val container = AttachmentContainer() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + container.add(attachment1) + container.add(attachment2) + + assertEquals(listOf(attachment1, attachment2), container.all) + } + + @Test + fun `after reset list is empty`() { + val container = AttachmentContainer() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + container.add(attachment1) + container.add(attachment2) + + container.clear() + + assertEquals(emptyList(), container.all) + } + + @Test + fun `after replace list contains only new item`() { + val container = AttachmentContainer() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + val attachment3 = newAttachment("test2") + val attachment4 = newAttachment("test2") + container.add(attachment1) + container.add(attachment2) + + container.replaceAll(listOf(attachment3, attachment4)) + + assertEquals(listOf(attachment3, attachment4), container.all) + } + + companion object { + fun newAttachment(content: String) = Attachment(content.toByteArray(), "$content.txt") + } +} diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt new file mode 100644 index 00000000000..d5ed951b4d8 --- /dev/null +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -0,0 +1,101 @@ +package io.sentry.hints + +import io.sentry.hints.AttachmentContainerTest.Companion.newAttachment +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull + +class HintsTest { + @Test + fun `getting as wrong class returns null`() { + val hints = Hints() + hints.set("hint1", "not a number") + + assertNull(hints.getAs("hint1", Int::class.java)) + } + + @Test + fun `getting as correct class returns it`() { + val hints = Hints() + hints.set("hint1", "some string") + + assertEquals("some string", hints.getAs("hint1", String::class.java)) + } + + @Test + fun `getting casted returns null if not contained`() { + val hints = Hints() + assertNull(hints.getAs("hint-does-not-exist", Int::class.java)) + } + + @Test + fun `getting returns null if not contained`() { + val hints = Hints() + assertNull(hints.get("hint-does-not-exist")) + } + + @Test + fun `kotlin java interop for primitives does not work yet`() { + val hints = Hints() + + hints.set("hint1", 1718) + + assertNull(hints.getAs("hint1", Long::class.java)) + } + + @Test + fun `setting twice only keeps second value`() { + val hints = Hints() + + hints.set("hint1", "some string") + hints.set("hint1", "a different string") + + assertEquals("a different string", hints.getAs("hint1", String::class.java)) + } + + @Test + fun `after removing the value is gone`() { + val hints = Hints() + + hints.set("hint1", "some string") + assertEquals("some string", hints.getAs("hint1", String::class.java)) + + hints.remove("hint1") + assertNull(hints.get("hint1")) + } + + @Test + fun `removing leaves other values`() { + val hints = Hints() + + hints.set("hint1", "some string") + assertEquals("some string", hints.getAs("hint1", String::class.java)) + hints.set("hint2", "another string") + + hints.remove("hint1") + assertNull(hints.get("hint1")) + assertEquals("another string", hints.getAs("hint2", String::class.java)) + } + + @Test + fun `can retrieve AttachmentContainer`() { + val hints = Hints() + assertNotNull(hints.attachmentContainer) + } + + @Test + fun `can create hints with attachment`() { + val attachment = newAttachment("test1") + val hints = Hints.withAttachment(attachment) + assertEquals(listOf(attachment), hints.attachmentContainer.all) + } + + @Test + fun `can create hints with attachments`() { + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test1") + val hints = Hints.withAttachments(listOf(attachment1, attachment2)) + assertEquals(listOf(attachment1, attachment2), hints.attachmentContainer.all) + } +} From 26e3bbc2ca78451d8932a0447074ca623f10ef50 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 13 May 2022 08:11:15 +0200 Subject: [PATCH 02/15] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b931ceb687..58a0a6a20ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Allow setting SDK info (name & version) in manifest ([#2016](https://github.com/getsentry/sentry-java/pull/2016)) - Allow setting native Android SDK name during build ([#2035](https://github.com/getsentry/sentry-java/pull/2035)) +- Attachments can now be manipulated via hints ([#2046](https://github.com/getsentry/sentry-java/pull/2046)) ## 6.0.0-beta.3 From 8fa62bba9a820b557a08ccecc2f391c6934550d6 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 13 May 2022 09:34:59 +0200 Subject: [PATCH 03/15] Add tests for breadcrumbs and attachments via hints --- .../test/java/io/sentry/SentryClientTest.kt | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 815a6a86d3c..7dc38ad5308 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1788,7 +1788,40 @@ class SentryClientTest { thenEnvelopeIsSentWith(0, 0, 1, 1) } -// TODO breadcrumb test + + @Test + fun `passing attachments via hint into breadcrumb ignores them`() { + val sut = fixture.getSut { options -> + options.setBeforeBreadcrumb { breadcrumb, hints -> + breadcrumb + } + } + + val scope = givenScopeWithStartedSession() + scope.addBreadcrumb(Breadcrumb.info("hello from breadcrumb"), Hints.withAttachment(fixture.attachment)) + + sut.captureException(IllegalStateException(), scope) + + thenEnvelopeIsSentWith(1, 1, 0) + } + + @Test + fun `adding attachments in beforeBreadcrumb ignores them`() { + val sut = fixture.getSut { options -> + options.setBeforeBreadcrumb { breadcrumb, hints -> + hints.attachmentContainer.add(fixture.attachment) + breadcrumb + } + } + + val scope = givenScopeWithStartedSession() + scope.addBreadcrumb(Breadcrumb.info("hello from breadcrumb")) + + sut.captureException(IllegalStateException(), scope) + + thenEnvelopeIsSentWith(1, 1, 0) + } + private fun givenScopeWithStartedSession(errored: Boolean = false, crashed: Boolean = false): Scope { val scope = createScope(fixture.sentryOptions) scope.startSession() From f916def3133b0216e8386c0ff8ed8e6a299244ba Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 13 May 2022 13:39:59 +0200 Subject: [PATCH 04/15] Update CHANGELOG.md Co-authored-by: Philipp Hofmann --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58a0a6a20ec..f61b9712716 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ - Allow setting SDK info (name & version) in manifest ([#2016](https://github.com/getsentry/sentry-java/pull/2016)) - Allow setting native Android SDK name during build ([#2035](https://github.com/getsentry/sentry-java/pull/2035)) -- Attachments can now be manipulated via hints ([#2046](https://github.com/getsentry/sentry-java/pull/2046)) +- Attachments can be manipulated via hints ([#2046](https://github.com/getsentry/sentry-java/pull/2046)) ## 6.0.0-beta.3 From 04edda0b65f548b4793f346b72aa4c6ca3cd0f34 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Fri, 13 May 2022 16:30:48 +0200 Subject: [PATCH 05/15] Rename AttachmentContainer to Attachments --- sentry/api/sentry.api | 4 ++-- .../src/main/java/io/sentry/SentryClient.java | 4 ++-- ...achmentContainer.java => Attachments.java} | 2 +- .../src/main/java/io/sentry/hints/Hints.java | 14 ++++++------- .../test/java/io/sentry/SentryClientTest.kt | 20 +++++++++---------- ...entContainerTest.kt => AttachmentsTest.kt} | 10 +++++----- .../test/java/io/sentry/hints/HintsTest.kt | 10 +++++----- 7 files changed, 32 insertions(+), 32 deletions(-) rename sentry/src/main/java/io/sentry/hints/{AttachmentContainer.java => Attachments.java} (95%) rename sentry/src/test/java/io/sentry/hints/{AttachmentContainerTest.kt => AttachmentsTest.kt} (87%) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 1d4468533b4..106d720b6b2 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1836,7 +1836,7 @@ public final class io/sentry/exception/SentryEnvelopeException : java/lang/Excep public abstract interface class io/sentry/hints/ApplyScopeData { } -public final class io/sentry/hints/AttachmentContainer { +public final class io/sentry/hints/Attachments { public fun ()V public fun add (Lio/sentry/Attachment;)V public fun addAll (Ljava/util/List;)V @@ -1860,7 +1860,7 @@ public final class io/sentry/hints/Hints { public fun ()V public fun get (Ljava/lang/String;)Ljava/lang/Object; public fun getAs (Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object; - public fun getAttachmentContainer ()Lio/sentry/hints/AttachmentContainer; + public fun getAttachments ()Lio/sentry/hints/Attachments; public fun remove (Ljava/lang/String;)V public fun set (Ljava/lang/String;Ljava/lang/Object;)V public static fun withAttachment (Lio/sentry/Attachment;)Lio/sentry/hints/Hints; diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index a09824ed9c1..f28e2d2b7ef 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -196,7 +196,7 @@ private boolean shouldApplyScopeData( private void addScopeAttachmentsToHints(@Nullable Scope scope, @NotNull Hints hints) { if (scope != null) { - hints.getAttachmentContainer().addAll(scope.getAttachments()); + hints.getAttachments().addAll(scope.getAttachments()); } } @@ -227,7 +227,7 @@ private boolean shouldSendSessionUpdateForDroppedEvent( } private @Nullable List getAttachments(final @NotNull Hints hints) { - @NotNull final List attachments = hints.getAttachmentContainer().getAll(); + @NotNull final List attachments = hints.getAttachments().getAll(); @Nullable final Attachment screenshot = hints.getAs(SENTRY_SCREENSHOT, Attachment.class); if (screenshot != null) { diff --git a/sentry/src/main/java/io/sentry/hints/AttachmentContainer.java b/sentry/src/main/java/io/sentry/hints/Attachments.java similarity index 95% rename from sentry/src/main/java/io/sentry/hints/AttachmentContainer.java rename to sentry/src/main/java/io/sentry/hints/Attachments.java index 133fe7057d9..4ff6b37755d 100644 --- a/sentry/src/main/java/io/sentry/hints/AttachmentContainer.java +++ b/sentry/src/main/java/io/sentry/hints/Attachments.java @@ -6,7 +6,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public final class AttachmentContainer { +public final class Attachments { private final @NotNull List internalStorage = new CopyOnWriteArrayList<>(); diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index 208350eb596..fce6fd20f96 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -15,13 +15,13 @@ public final class Hints { public static @NotNull Hints withAttachment(@Nullable Attachment attachment) { @NotNull final Hints hints = new Hints(); - hints.getAttachmentContainer().add(attachment); + hints.getAttachments().add(attachment); return hints; } public static @NotNull Hints withAttachments(@Nullable List attachments) { @NotNull final Hints hints = new Hints(); - hints.getAttachmentContainer().addAll(attachments); + hints.getAttachments().addAll(attachments); return hints; } @@ -48,16 +48,16 @@ public void remove(@NotNull String hintName) { internalStorage.remove(hintName); } - public @NotNull AttachmentContainer getAttachmentContainer() { + public @NotNull Attachments getAttachments() { if (internalStorage.containsKey(SENTRY_ATTACHMENTS)) { - AttachmentContainer container = getAs(SENTRY_ATTACHMENTS, AttachmentContainer.class); + Attachments container = getAs(SENTRY_ATTACHMENTS, Attachments.class); if (container != null) { return container; } } - AttachmentContainer attachmentContainer = new AttachmentContainer(); - internalStorage.put(SENTRY_ATTACHMENTS, attachmentContainer); - return attachmentContainer; + Attachments attachments = new Attachments(); + internalStorage.put(SENTRY_ATTACHMENTS, attachments); + return attachments; } } diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 7dc38ad5308..bd58f9e752c 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1618,8 +1618,8 @@ class SentryClientTest { fun `can add to attachments in beforeSend`() { val sut = fixture.getSut { options -> options.setBeforeSend { event, hints -> - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) - hints.attachmentContainer.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) + hints.attachments.add(fixture.attachment3) event } } @@ -1635,7 +1635,7 @@ class SentryClientTest { fun `can replace attachments in beforeSend`() { val sut = fixture.getSut { options -> options.setBeforeSend { event, hints -> - hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + hints.attachments.replaceAll(listOf(fixture.attachment)) event } } @@ -1652,8 +1652,8 @@ class SentryClientTest { val sut = fixture.getSut { options -> options.addEventProcessor(object : EventProcessor { override fun process(event: SentryEvent, hints: Hints): SentryEvent? { - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) - hints.attachmentContainer.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) + hints.attachments.add(fixture.attachment3) return event } @@ -1678,7 +1678,7 @@ class SentryClientTest { val sut = fixture.getSut { options -> options.addEventProcessor(object : EventProcessor { override fun process(event: SentryEvent, hints: Hints): SentryEvent? { - hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + hints.attachments.replaceAll(listOf(fixture.attachment)) return event } @@ -1740,8 +1740,8 @@ class SentryClientTest { transaction: SentryTransaction, hints: Hints ): SentryTransaction? { - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachmentContainer.all) - hints.attachmentContainer.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) + hints.attachments.add(fixture.attachment3) return transaction } }) @@ -1771,7 +1771,7 @@ class SentryClientTest { transaction: SentryTransaction, hints: Hints ): SentryTransaction? { - hints.attachmentContainer.replaceAll(listOf(fixture.attachment)) + hints.attachments.replaceAll(listOf(fixture.attachment)) return transaction } }) @@ -1809,7 +1809,7 @@ class SentryClientTest { fun `adding attachments in beforeBreadcrumb ignores them`() { val sut = fixture.getSut { options -> options.setBeforeBreadcrumb { breadcrumb, hints -> - hints.attachmentContainer.add(fixture.attachment) + hints.attachments.add(fixture.attachment) breadcrumb } } diff --git a/sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt similarity index 87% rename from sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt rename to sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt index 46b198db086..02cf8307601 100644 --- a/sentry/src/test/java/io/sentry/hints/AttachmentContainerTest.kt +++ b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt @@ -4,11 +4,11 @@ import io.sentry.Attachment import kotlin.test.Test import kotlin.test.assertEquals -class AttachmentContainerTest { +class AttachmentsTest { @Test fun `can add an attachment`() { - val container = AttachmentContainer() + val container = Attachments() val attachment = newAttachment("test1") container.add(attachment) @@ -17,7 +17,7 @@ class AttachmentContainerTest { @Test fun `can add multiple attachments`() { - val container = AttachmentContainer() + val container = Attachments() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") container.add(attachment1) @@ -28,7 +28,7 @@ class AttachmentContainerTest { @Test fun `after reset list is empty`() { - val container = AttachmentContainer() + val container = Attachments() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") container.add(attachment1) @@ -41,7 +41,7 @@ class AttachmentContainerTest { @Test fun `after replace list contains only new item`() { - val container = AttachmentContainer() + val container = Attachments() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") val attachment3 = newAttachment("test2") diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt index d5ed951b4d8..5d332d089fa 100644 --- a/sentry/src/test/java/io/sentry/hints/HintsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -1,6 +1,6 @@ package io.sentry.hints -import io.sentry.hints.AttachmentContainerTest.Companion.newAttachment +import io.sentry.hints.AttachmentsTest.Companion.newAttachment import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull @@ -79,16 +79,16 @@ class HintsTest { } @Test - fun `can retrieve AttachmentContainer`() { + fun `can retrieve Attachments`() { val hints = Hints() - assertNotNull(hints.attachmentContainer) + assertNotNull(hints.attachments) } @Test fun `can create hints with attachment`() { val attachment = newAttachment("test1") val hints = Hints.withAttachment(attachment) - assertEquals(listOf(attachment), hints.attachmentContainer.all) + assertEquals(listOf(attachment), hints.attachments.all) } @Test @@ -96,6 +96,6 @@ class HintsTest { val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test1") val hints = Hints.withAttachments(listOf(attachment1, attachment2)) - assertEquals(listOf(attachment1, attachment2), hints.attachmentContainer.all) + assertEquals(listOf(attachment1, attachment2), hints.attachments.all) } } From 815bf53fb6a05ef7a4ea80f907956cb0efa2527a Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 16 May 2022 19:16:47 +0200 Subject: [PATCH 06/15] Use long for test --- sentry/src/test/java/io/sentry/hints/HintsTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt index 5d332d089fa..721d05ff3d1 100644 --- a/sentry/src/test/java/io/sentry/hints/HintsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -39,7 +39,7 @@ class HintsTest { fun `kotlin java interop for primitives does not work yet`() { val hints = Hints() - hints.set("hint1", 1718) + hints.set("hint1", 1718L) assertNull(hints.getAs("hint1", Long::class.java)) } From 1aa927530f85e9ddc199f564b62ede73557801c9 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 07:07:58 +0200 Subject: [PATCH 07/15] Move attachments into Hints --- sentry/api/sentry.api | 15 +++---- .../src/main/java/io/sentry/SentryClient.java | 4 +- .../main/java/io/sentry/TypeCheckHint.java | 2 - .../java/io/sentry/hints/Attachments.java | 39 ------------------ .../src/main/java/io/sentry/hints/Hints.java | 40 +++++++++++++------ .../test/java/io/sentry/SentryClientTest.kt | 20 +++++----- .../java/io/sentry/hints/AttachmentsTest.kt | 34 ++++++++-------- .../test/java/io/sentry/hints/HintsTest.kt | 4 +- 8 files changed, 63 insertions(+), 95 deletions(-) delete mode 100644 sentry/src/main/java/io/sentry/hints/Attachments.java diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 106d720b6b2..aee590d8f3d 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1836,15 +1836,6 @@ public final class io/sentry/exception/SentryEnvelopeException : java/lang/Excep public abstract interface class io/sentry/hints/ApplyScopeData { } -public final class io/sentry/hints/Attachments { - public fun ()V - public fun add (Lio/sentry/Attachment;)V - public fun addAll (Ljava/util/List;)V - public fun clear ()V - public fun getAll ()Ljava/util/List; - public fun replaceAll (Ljava/util/List;)V -} - public abstract interface class io/sentry/hints/Cached { } @@ -1858,10 +1849,14 @@ public abstract interface class io/sentry/hints/Flushable { public final class io/sentry/hints/Hints { public fun ()V + public fun addAttachment (Lio/sentry/Attachment;)V + public fun addAttachments (Ljava/util/List;)V + public fun clear ()V public fun get (Ljava/lang/String;)Ljava/lang/Object; public fun getAs (Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object; - public fun getAttachments ()Lio/sentry/hints/Attachments; + public fun getAttachments ()Ljava/util/List; public fun remove (Ljava/lang/String;)V + public fun replaceAttachments (Ljava/util/List;)V public fun set (Ljava/lang/String;Ljava/lang/Object;)V public static fun withAttachment (Lio/sentry/Attachment;)Lio/sentry/hints/Hints; public static fun withAttachments (Ljava/util/List;)Lio/sentry/hints/Hints; diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index f28e2d2b7ef..87331482cf1 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -196,7 +196,7 @@ private boolean shouldApplyScopeData( private void addScopeAttachmentsToHints(@Nullable Scope scope, @NotNull Hints hints) { if (scope != null) { - hints.getAttachments().addAll(scope.getAttachments()); + hints.addAttachments(scope.getAttachments()); } } @@ -227,7 +227,7 @@ private boolean shouldSendSessionUpdateForDroppedEvent( } private @Nullable List getAttachments(final @NotNull Hints hints) { - @NotNull final List attachments = hints.getAttachments().getAll(); + @NotNull final List attachments = hints.getAttachments(); @Nullable final Attachment screenshot = hints.getAs(SENTRY_SCREENSHOT, Attachment.class); if (screenshot != null) { diff --git a/sentry/src/main/java/io/sentry/TypeCheckHint.java b/sentry/src/main/java/io/sentry/TypeCheckHint.java index e79bb2f767b..9bf5a79f14f 100644 --- a/sentry/src/main/java/io/sentry/TypeCheckHint.java +++ b/sentry/src/main/java/io/sentry/TypeCheckHint.java @@ -27,8 +27,6 @@ public final class TypeCheckHint { public static final String ANDROID_FRAGMENT = "android:fragment"; /** Used for screenshots. */ public static final String SENTRY_SCREENSHOT = "sentry:screenshot"; - /** Used for attachments. */ - public static final String SENTRY_ATTACHMENTS = "sentry:attachments"; /** Used for OkHttp response breadcrumbs. */ public static final String OKHTTP_RESPONSE = "okHttp:response"; diff --git a/sentry/src/main/java/io/sentry/hints/Attachments.java b/sentry/src/main/java/io/sentry/hints/Attachments.java deleted file mode 100644 index 4ff6b37755d..00000000000 --- a/sentry/src/main/java/io/sentry/hints/Attachments.java +++ /dev/null @@ -1,39 +0,0 @@ -package io.sentry.hints; - -import io.sentry.Attachment; -import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; - -public final class Attachments { - - private final @NotNull List internalStorage = new CopyOnWriteArrayList<>(); - - public void add(@Nullable Attachment attachment) { - if (attachment != null) { - internalStorage.add(attachment); - } - } - - @SuppressWarnings("unchecked") - public void addAll(@Nullable List attachments) { - if (attachments != null) { - internalStorage.addAll(attachments); - } - } - - @SuppressWarnings("unchecked") - public @NotNull List getAll() { - return new CopyOnWriteArrayList<>(internalStorage); - } - - public void replaceAll(@Nullable List attachments) { - clear(); - addAll(attachments); - } - - public void clear() { - internalStorage.clear(); - } -} diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index fce6fd20f96..6a0d926cda8 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -1,27 +1,27 @@ package io.sentry.hints; -import static io.sentry.TypeCheckHint.SENTRY_ATTACHMENTS; - import io.sentry.Attachment; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; public final class Hints { private final @NotNull Map internalStorage = new HashMap(); + private final @NotNull List attachments = new CopyOnWriteArrayList<>(); public static @NotNull Hints withAttachment(@Nullable Attachment attachment) { @NotNull final Hints hints = new Hints(); - hints.getAttachments().add(attachment); + hints.addAttachment(attachment); return hints; } public static @NotNull Hints withAttachments(@Nullable List attachments) { @NotNull final Hints hints = new Hints(); - hints.getAttachments().addAll(attachments); + hints.addAttachments(attachments); return hints; } @@ -48,16 +48,30 @@ public void remove(@NotNull String hintName) { internalStorage.remove(hintName); } - public @NotNull Attachments getAttachments() { - if (internalStorage.containsKey(SENTRY_ATTACHMENTS)) { - Attachments container = getAs(SENTRY_ATTACHMENTS, Attachments.class); - if (container != null) { - return container; - } + public void addAttachment(@Nullable Attachment attachment) { + if (attachment != null) { + attachments.add(attachment); + } + } + + @SuppressWarnings("unchecked") + public void addAttachments(@Nullable List attachments) { + if (attachments != null) { + this.attachments.addAll(attachments); } + } + + @SuppressWarnings("unchecked") + public @NotNull List getAttachments() { + return new CopyOnWriteArrayList<>(attachments); + } + + public void replaceAttachments(@Nullable List attachments) { + clear(); + addAttachments(attachments); + } - Attachments attachments = new Attachments(); - internalStorage.put(SENTRY_ATTACHMENTS, attachments); - return attachments; + public void clear() { + attachments.clear(); } } diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index bd58f9e752c..c5662306bef 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1618,8 +1618,8 @@ class SentryClientTest { fun `can add to attachments in beforeSend`() { val sut = fixture.getSut { options -> options.setBeforeSend { event, hints -> - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) - hints.attachments.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments) + hints.addAttachment(fixture.attachment3) event } } @@ -1635,7 +1635,7 @@ class SentryClientTest { fun `can replace attachments in beforeSend`() { val sut = fixture.getSut { options -> options.setBeforeSend { event, hints -> - hints.attachments.replaceAll(listOf(fixture.attachment)) + hints.replaceAttachments(listOf(fixture.attachment)) event } } @@ -1652,8 +1652,8 @@ class SentryClientTest { val sut = fixture.getSut { options -> options.addEventProcessor(object : EventProcessor { override fun process(event: SentryEvent, hints: Hints): SentryEvent? { - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) - hints.attachments.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments) + hints.addAttachment(fixture.attachment3) return event } @@ -1678,7 +1678,7 @@ class SentryClientTest { val sut = fixture.getSut { options -> options.addEventProcessor(object : EventProcessor { override fun process(event: SentryEvent, hints: Hints): SentryEvent? { - hints.attachments.replaceAll(listOf(fixture.attachment)) + hints.replaceAttachments(listOf(fixture.attachment)) return event } @@ -1740,8 +1740,8 @@ class SentryClientTest { transaction: SentryTransaction, hints: Hints ): SentryTransaction? { - assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments.all) - hints.attachments.add(fixture.attachment3) + assertEquals(listOf(fixture.attachment, fixture.attachment2), hints.attachments) + hints.addAttachment(fixture.attachment3) return transaction } }) @@ -1771,7 +1771,7 @@ class SentryClientTest { transaction: SentryTransaction, hints: Hints ): SentryTransaction? { - hints.attachments.replaceAll(listOf(fixture.attachment)) + hints.replaceAttachments(listOf(fixture.attachment)) return transaction } }) @@ -1809,7 +1809,7 @@ class SentryClientTest { fun `adding attachments in beforeBreadcrumb ignores them`() { val sut = fixture.getSut { options -> options.setBeforeBreadcrumb { breadcrumb, hints -> - hints.attachments.add(fixture.attachment) + hints.addAttachment(fixture.attachment) breadcrumb } } diff --git a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt index 02cf8307601..2745737f774 100644 --- a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt @@ -8,50 +8,50 @@ class AttachmentsTest { @Test fun `can add an attachment`() { - val container = Attachments() + val hints = Hints() val attachment = newAttachment("test1") - container.add(attachment) + hints.addAttachment(attachment) - assertEquals(listOf(attachment), container.all) + assertEquals(listOf(attachment), hints.attachments) } @Test fun `can add multiple attachments`() { - val container = Attachments() + val hints = Hints() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") - container.add(attachment1) - container.add(attachment2) + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) - assertEquals(listOf(attachment1, attachment2), container.all) + assertEquals(listOf(attachment1, attachment2), hints.attachments) } @Test fun `after reset list is empty`() { - val container = Attachments() + val hints = Hints() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") - container.add(attachment1) - container.add(attachment2) + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) - container.clear() + hints.clear() - assertEquals(emptyList(), container.all) + assertEquals(emptyList(), hints.attachments) } @Test fun `after replace list contains only new item`() { - val container = Attachments() + val hints = Hints() val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test2") val attachment3 = newAttachment("test2") val attachment4 = newAttachment("test2") - container.add(attachment1) - container.add(attachment2) + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) - container.replaceAll(listOf(attachment3, attachment4)) + hints.replaceAttachments(listOf(attachment3, attachment4)) - assertEquals(listOf(attachment3, attachment4), container.all) + assertEquals(listOf(attachment3, attachment4), hints.attachments) } companion object { diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt index 721d05ff3d1..c4a5712dcda 100644 --- a/sentry/src/test/java/io/sentry/hints/HintsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -88,7 +88,7 @@ class HintsTest { fun `can create hints with attachment`() { val attachment = newAttachment("test1") val hints = Hints.withAttachment(attachment) - assertEquals(listOf(attachment), hints.attachments.all) + assertEquals(listOf(attachment), hints.attachments) } @Test @@ -96,6 +96,6 @@ class HintsTest { val attachment1 = newAttachment("test1") val attachment2 = newAttachment("test1") val hints = Hints.withAttachments(listOf(attachment1, attachment2)) - assertEquals(listOf(attachment1, attachment2), hints.attachments.all) + assertEquals(listOf(attachment1, attachment2), hints.attachments) } } From 60dd87a559dc09c21e66696945efe2fc4c20064f Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 08:51:45 +0200 Subject: [PATCH 08/15] Fix kotlin/java interop for Hints --- sentry/api/sentry.api | 1 - .../src/main/java/io/sentry/hints/Hints.java | 24 +++++++++ .../test/java/io/sentry/hints/HintsTest.kt | 53 ++++++++++++++++++- 3 files changed, 75 insertions(+), 3 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index aee590d8f3d..a71bff62c3b 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1638,7 +1638,6 @@ public final class io/sentry/TypeCheckHint { public static final field OKHTTP_RESPONSE Ljava/lang/String; public static final field OPEN_FEIGN_REQUEST Ljava/lang/String; public static final field OPEN_FEIGN_RESPONSE Ljava/lang/String; - public static final field SENTRY_ATTACHMENTS Ljava/lang/String; public static final field SENTRY_SCREENSHOT Ljava/lang/String; public static final field SENTRY_SYNTHETIC_EXCEPTION Ljava/lang/String; public static final field SENTRY_TYPE_CHECK_HINT Ljava/lang/String; diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index 6a0d926cda8..ce553c7eb4e 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -25,6 +25,18 @@ public final class Hints { return hints; } + public Hints() { + primitiveMappings = new HashMap<>(); + primitiveMappings.put("boolean", Boolean.class); + primitiveMappings.put("char", Character.class); + primitiveMappings.put("byte", Byte.class); + primitiveMappings.put("short", Short.class); + primitiveMappings.put("int", Integer.class); + primitiveMappings.put("long", Long.class); + primitiveMappings.put("float", Float.class); + primitiveMappings.put("double", Double.class); + } + public void set(@NotNull String hintType, @Nullable Object hint) { internalStorage.put(hintType, hint); } @@ -39,6 +51,8 @@ public void set(@NotNull String hintType, @Nullable Object hint) { if (clazz.isInstance(hintValue)) { return (T) hintValue; + } else if (isCastablePrimitive(hintValue, clazz)) { + return (T) hintValue; } else { return null; } @@ -74,4 +88,14 @@ public void replaceAttachments(@Nullable List attachments) { public void clear() { attachments.clear(); } + + private final Map> primitiveMappings; + + private boolean isCastablePrimitive(@Nullable Object hintValue, @NotNull Class clazz) { + Class nonPrimitiveClass = primitiveMappings.get(clazz.getCanonicalName()); + return hintValue != null + && clazz.isPrimitive() + && nonPrimitiveClass != null + && nonPrimitiveClass.isInstance(hintValue); + } } diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt index c4a5712dcda..984cde542fd 100644 --- a/sentry/src/test/java/io/sentry/hints/HintsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -36,12 +36,61 @@ class HintsTest { } @Test - fun `kotlin java interop for primitives does not work yet`() { + fun `kotlin java interop for primitives works for float`() { val hints = Hints() + hints.set("hint1", 1.3f) + assertEquals(1.3f, hints.getAs("hint1", Float::class.java)) + } + @Test + fun `kotlin java interop for primitives works for double`() { + val hints = Hints() + hints.set("hint1", 1.4) + assertEquals(1.4, hints.getAs("hint1", Double::class.java)) + } + + @Test + fun `kotlin java interop for primitives works for long`() { + val hints = Hints() hints.set("hint1", 1718L) + assertEquals(1718L, hints.getAs("hint1", Long::class.java)) + } + + @Test + fun `kotlin java interop for primitives works for int`() { + val hints = Hints() + hints.set("hint1", 123) + assertEquals(123, hints.getAs("hint1", Int::class.java)) + } + + @Test + fun `kotlin java interop for primitives works for short`() { + val hints = Hints() + val s: Short = 123 + hints.set("hint1", s) + assertEquals(s, hints.getAs("hint1", Short::class.java)) + } + + @Test + fun `kotlin java interop for primitives works for byte`() { + val hints = Hints() + val b: Byte = 1 + hints.set("hint1", b) + assertEquals(b, hints.getAs("hint1", Byte::class.java)) + } - assertNull(hints.getAs("hint1", Long::class.java)) + @Test + fun `kotlin java interop for primitives works for char`() { + val hints = Hints() + hints.set("hint1", 'a') + assertEquals('a', hints.getAs("hint1", Char::class.java)) + } + + @Test + fun `kotlin java interop for primitives works for boolean`() { + val hints = Hints() + hints.set("hint1", true) + assertEquals(true, hints.getAs("hint1", Boolean::class.java)) } @Test From c3d84d988e547f076a0de8dba5c09785199febd2 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 09:31:32 +0200 Subject: [PATCH 09/15] Convert screenshot from map entry to property --- .../android/core/ScreenshotEventProcessor.java | 5 +---- .../android/core/ScreenshotEventProcessorTest.kt | 15 +++++++-------- sentry/api/sentry.api | 3 ++- sentry/src/main/java/io/sentry/SentryClient.java | 4 +--- sentry/src/main/java/io/sentry/TypeCheckHint.java | 2 -- sentry/src/main/java/io/sentry/hints/Hints.java | 11 +++++++++-- .../src/test/java/io/sentry/SentryClientTest.kt | 7 +++---- 7 files changed, 23 insertions(+), 24 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java index 6fed7fcf3b8..4630e6a7bde 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java @@ -1,7 +1,6 @@ package io.sentry.android.core; import static io.sentry.TypeCheckHint.ANDROID_ACTIVITY; -import static io.sentry.TypeCheckHint.SENTRY_SCREENSHOT; import android.annotation.SuppressLint; import android.app.Activity; @@ -93,9 +92,7 @@ public ScreenshotEventProcessor( if (byteArrayOutputStream.size() > 0) { // screenshot png is around ~100-150 kb - hints.set( - SENTRY_SCREENSHOT, - Attachment.fromScreenshot(byteArrayOutputStream.toByteArray())); + hints.setScreenshot(Attachment.fromScreenshot(byteArrayOutputStream.toByteArray())); hints.set(ANDROID_ACTIVITY, activity); } else { this.options diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index 2e8ef405e79..3ae93d72c56 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -14,7 +14,6 @@ import io.sentry.Attachment import io.sentry.MainEventProcessor import io.sentry.SentryEvent import io.sentry.TypeCheckHint.ANDROID_ACTIVITY -import io.sentry.TypeCheckHint.SENTRY_SCREENSHOT import io.sentry.hints.Hints import org.junit.runner.RunWith import kotlin.test.BeforeTest @@ -103,7 +102,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.getScreenshot()) } @Test @@ -116,7 +115,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(SentryEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.screenshot) } @Test @@ -127,7 +126,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.screenshot) } @Test @@ -141,7 +140,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.screenshot) } @Test @@ -156,7 +155,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.screenshot) } @Test @@ -169,7 +168,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - val screenshot = hints[SENTRY_SCREENSHOT] + val screenshot = hints.screenshot assertTrue(screenshot is Attachment) assertEquals("screenshot.png", screenshot.filename) assertEquals("image/png", screenshot.contentType) @@ -188,7 +187,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints[SENTRY_SCREENSHOT]) + assertNull(hints.screenshot) } private fun getEvent(): SentryEvent = SentryEvent(Throwable("Throwable")) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index a71bff62c3b..1bcd3ebe82b 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1638,7 +1638,6 @@ public final class io/sentry/TypeCheckHint { public static final field OKHTTP_RESPONSE Ljava/lang/String; public static final field OPEN_FEIGN_REQUEST Ljava/lang/String; public static final field OPEN_FEIGN_RESPONSE Ljava/lang/String; - public static final field SENTRY_SCREENSHOT Ljava/lang/String; public static final field SENTRY_SYNTHETIC_EXCEPTION Ljava/lang/String; public static final field SENTRY_TYPE_CHECK_HINT Ljava/lang/String; public static final field SERVLET_REQUEST Ljava/lang/String; @@ -1854,9 +1853,11 @@ public final class io/sentry/hints/Hints { public fun get (Ljava/lang/String;)Ljava/lang/Object; public fun getAs (Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object; public fun getAttachments ()Ljava/util/List; + public fun getScreenshot ()Lio/sentry/Attachment; public fun remove (Ljava/lang/String;)V public fun replaceAttachments (Ljava/util/List;)V public fun set (Ljava/lang/String;Ljava/lang/Object;)V + public fun setScreenshot (Lio/sentry/Attachment;)V public static fun withAttachment (Lio/sentry/Attachment;)Lio/sentry/hints/Hints; public static fun withAttachments (Ljava/util/List;)Lio/sentry/hints/Hints; } diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 87331482cf1..5fd62f9b27d 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -1,7 +1,5 @@ package io.sentry; -import static io.sentry.TypeCheckHint.SENTRY_SCREENSHOT; - import io.sentry.clientreport.DiscardReason; import io.sentry.exception.SentryEnvelopeException; import io.sentry.hints.DiskFlushNotification; @@ -229,7 +227,7 @@ private boolean shouldSendSessionUpdateForDroppedEvent( private @Nullable List getAttachments(final @NotNull Hints hints) { @NotNull final List attachments = hints.getAttachments(); - @Nullable final Attachment screenshot = hints.getAs(SENTRY_SCREENSHOT, Attachment.class); + @Nullable final Attachment screenshot = hints.getScreenshot(); if (screenshot != null) { attachments.add(screenshot); } diff --git a/sentry/src/main/java/io/sentry/TypeCheckHint.java b/sentry/src/main/java/io/sentry/TypeCheckHint.java index 9bf5a79f14f..cc50b40c378 100644 --- a/sentry/src/main/java/io/sentry/TypeCheckHint.java +++ b/sentry/src/main/java/io/sentry/TypeCheckHint.java @@ -25,8 +25,6 @@ public final class TypeCheckHint { public static final String ANDROID_VIEW = "android:view"; /** Used for Fragment breadcrumbs. */ public static final String ANDROID_FRAGMENT = "android:fragment"; - /** Used for screenshots. */ - public static final String SENTRY_SCREENSHOT = "sentry:screenshot"; /** Used for OkHttp response breadcrumbs. */ public static final String OKHTTP_RESPONSE = "okHttp:response"; diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index ce553c7eb4e..91272b0bc34 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -12,6 +12,7 @@ public final class Hints { private final @NotNull Map internalStorage = new HashMap(); private final @NotNull List attachments = new CopyOnWriteArrayList<>(); + private @Nullable Attachment screenshot = null; public static @NotNull Hints withAttachment(@Nullable Attachment attachment) { @NotNull final Hints hints = new Hints(); @@ -68,14 +69,12 @@ public void addAttachment(@Nullable Attachment attachment) { } } - @SuppressWarnings("unchecked") public void addAttachments(@Nullable List attachments) { if (attachments != null) { this.attachments.addAll(attachments); } } - @SuppressWarnings("unchecked") public @NotNull List getAttachments() { return new CopyOnWriteArrayList<>(attachments); } @@ -89,6 +88,14 @@ public void clear() { attachments.clear(); } + public void setScreenshot(@Nullable Attachment screenshot) { + this.screenshot = screenshot; + } + + public @Nullable Attachment getScreenshot() { + return screenshot; + } + private final Map> primitiveMappings; private boolean isCastablePrimitive(@Nullable Object hintValue, @NotNull Class clazz) { diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index c5662306bef..76f3788b6d4 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -14,7 +14,6 @@ import com.nhaarman.mockitokotlin2.times import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.verifyNoMoreInteractions import com.nhaarman.mockitokotlin2.whenever -import io.sentry.TypeCheckHint.SENTRY_SCREENSHOT import io.sentry.clientreport.ClientReportTestHelper.Companion.assertClientReport import io.sentry.clientreport.DiscardReason import io.sentry.clientreport.DiscardedEvent @@ -1284,7 +1283,7 @@ class SentryClientTest { fun `screenshot is added to the envelope from the hint`() { val sut = fixture.getSut() val attachment = Attachment.fromScreenshot(byteArrayOf()) - val hints = Hints().also { it.set(SENTRY_SCREENSHOT, attachment) } + val hints = Hints().also { it.screenshot = attachment } sut.captureEvent(SentryEvent(), hints) @@ -1304,7 +1303,7 @@ class SentryClientTest { fixture.sentryOptions.beforeSend = CustomBeforeSendCallback() val sut = fixture.getSut() val attachment = Attachment.fromScreenshot(byteArrayOf()) - val hints = Hints().also { it.set(SENTRY_SCREENSHOT, attachment) } + val hints = Hints().also { it.screenshot = attachment } sut.captureEvent(SentryEvent(), hints) @@ -1873,7 +1872,7 @@ class SentryClientTest { class CustomBeforeSendCallback : SentryOptions.BeforeSendCallback { override fun execute(event: SentryEvent, hints: Hints): SentryEvent? { - hints.remove(SENTRY_SCREENSHOT) + hints.screenshot = null return event } From feea24cb14c982c4abc811e375ffaac2904d5d76 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 09:49:46 +0200 Subject: [PATCH 10/15] Rename hint name param --- sentry/src/main/java/io/sentry/hints/Hints.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index 91272b0bc34..23759d319a8 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -38,17 +38,17 @@ public Hints() { primitiveMappings.put("double", Double.class); } - public void set(@NotNull String hintType, @Nullable Object hint) { - internalStorage.put(hintType, hint); + public void set(@NotNull String name, @Nullable Object hint) { + internalStorage.put(name, hint); } - public @Nullable Object get(@NotNull String hintName) { - return internalStorage.get(hintName); + public @Nullable Object get(@NotNull String name) { + return internalStorage.get(name); } @SuppressWarnings("unchecked") - public @Nullable T getAs(@NotNull String hintName, @NotNull Class clazz) { - Object hintValue = internalStorage.get(hintName); + public @Nullable T getAs(@NotNull String name, @NotNull Class clazz) { + Object hintValue = internalStorage.get(name); if (clazz.isInstance(hintValue)) { return (T) hintValue; @@ -59,8 +59,8 @@ public void set(@NotNull String hintType, @Nullable Object hint) { } } - public void remove(@NotNull String hintName) { - internalStorage.remove(hintName); + public void remove(@NotNull String name) { + internalStorage.remove(name); } public void addAttachment(@Nullable Attachment attachment) { From d7d4e7278be103e83422e37c64c9d3b7e3109b4d Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 16:00:31 +0200 Subject: [PATCH 11/15] Rename clear to clearAttachments --- sentry/api/sentry.api | 2 +- sentry/src/main/java/io/sentry/hints/Hints.java | 4 ++-- sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 1bcd3ebe82b..b2195ad59b4 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1849,7 +1849,7 @@ public final class io/sentry/hints/Hints { public fun ()V public fun addAttachment (Lio/sentry/Attachment;)V public fun addAttachments (Ljava/util/List;)V - public fun clear ()V + public fun clearAttachments ()V public fun get (Ljava/lang/String;)Ljava/lang/Object; public fun getAs (Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object; public fun getAttachments ()Ljava/util/List; diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index 23759d319a8..edbbc001060 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -80,11 +80,11 @@ public void addAttachments(@Nullable List attachments) { } public void replaceAttachments(@Nullable List attachments) { - clear(); + clearAttachments(); addAttachments(attachments); } - public void clear() { + public void clearAttachments() { attachments.clear(); } diff --git a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt index 2745737f774..e9d3fdfd2f0 100644 --- a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt @@ -34,7 +34,7 @@ class AttachmentsTest { hints.addAttachment(attachment1) hints.addAttachment(attachment2) - hints.clear() + hints.clearAttachments() assertEquals(emptyList(), hints.attachments) } From f752f8fd7ded6622758cb75c83ac8e42f0dab6a9 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 17 May 2022 16:21:13 +0200 Subject: [PATCH 12/15] Use kotlin short version access for getScreenshot --- .../java/io/sentry/android/core/ScreenshotEventProcessorTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index 3ae93d72c56..0db193574a1 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -102,7 +102,7 @@ class ScreenshotEventProcessorTest { val event = fixture.mainProcessor.process(getEvent(), hints) sut.process(event, hints) - assertNull(hints.getScreenshot()) + assertNull(hints.screenshot) } @Test From 22871ce871f7b6f7498fa59952e23cc62fdee234 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Wed, 18 May 2022 07:34:46 +0200 Subject: [PATCH 13/15] Move AttachmentsTest into HintsTest; add param names --- .../test/java/io/sentry/SentryClientTest.kt | 24 ++++---- .../java/io/sentry/hints/AttachmentsTest.kt | 60 ------------------- .../test/java/io/sentry/hints/HintsTest.kt | 54 ++++++++++++++++- 3 files changed, 65 insertions(+), 73 deletions(-) delete mode 100644 sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 76f3788b6d4..b3672b04c14 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1599,7 +1599,7 @@ class SentryClientTest { sut.captureException(IllegalStateException(), Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 0, 1) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 0, attachmentCount = 1) } @Test @@ -1610,7 +1610,7 @@ class SentryClientTest { scope.addAttachment(fixture.attachment2) sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 1, 2) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 2) } @Test @@ -1627,7 +1627,7 @@ class SentryClientTest { scope.addAttachment(fixture.attachment2) sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 1, 3) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 3) } @Test @@ -1643,7 +1643,7 @@ class SentryClientTest { scope.addAttachment(fixture.attachment2) sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 1, 1) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 1) } @Test @@ -1669,7 +1669,7 @@ class SentryClientTest { scope.addAttachment(fixture.attachment2) sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 1, 3) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 3) } @Test @@ -1694,7 +1694,7 @@ class SentryClientTest { scope.addAttachment(fixture.attachment2) sut.captureException(IllegalStateException(), scope, Hints.withAttachment(fixture.attachment)) - thenEnvelopeIsSentWith(1, 1, 1) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 1) } @Test @@ -1708,7 +1708,7 @@ class SentryClientTest { Hints.withAttachment(fixture.attachment) ) - thenEnvelopeIsSentWith(0, 0, 1, 1) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 0, attachmentCount = 1, transactionCount = 1) } @Test @@ -1724,7 +1724,7 @@ class SentryClientTest { Hints.withAttachment(fixture.attachment) ) - thenEnvelopeIsSentWith(0, 0, 2, 1) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 0, attachmentCount = 2, transactionCount = 1) } @Test @@ -1755,7 +1755,7 @@ class SentryClientTest { Hints.withAttachment(fixture.attachment) ) - thenEnvelopeIsSentWith(0, 0, 3, 1) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 0, attachmentCount = 3, transactionCount = 1) } @Test @@ -1785,7 +1785,7 @@ class SentryClientTest { Hints.withAttachment(fixture.attachment) ) - thenEnvelopeIsSentWith(0, 0, 1, 1) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 0, attachmentCount = 1, transactionCount = 1) } @Test @@ -1801,7 +1801,7 @@ class SentryClientTest { sut.captureException(IllegalStateException(), scope) - thenEnvelopeIsSentWith(1, 1, 0) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 0) } @Test @@ -1818,7 +1818,7 @@ class SentryClientTest { sut.captureException(IllegalStateException(), scope) - thenEnvelopeIsSentWith(1, 1, 0) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1, attachmentCount = 0) } private fun givenScopeWithStartedSession(errored: Boolean = false, crashed: Boolean = false): Scope { diff --git a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt b/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt deleted file mode 100644 index e9d3fdfd2f0..00000000000 --- a/sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt +++ /dev/null @@ -1,60 +0,0 @@ -package io.sentry.hints - -import io.sentry.Attachment -import kotlin.test.Test -import kotlin.test.assertEquals - -class AttachmentsTest { - - @Test - fun `can add an attachment`() { - val hints = Hints() - val attachment = newAttachment("test1") - hints.addAttachment(attachment) - - assertEquals(listOf(attachment), hints.attachments) - } - - @Test - fun `can add multiple attachments`() { - val hints = Hints() - val attachment1 = newAttachment("test1") - val attachment2 = newAttachment("test2") - hints.addAttachment(attachment1) - hints.addAttachment(attachment2) - - assertEquals(listOf(attachment1, attachment2), hints.attachments) - } - - @Test - fun `after reset list is empty`() { - val hints = Hints() - val attachment1 = newAttachment("test1") - val attachment2 = newAttachment("test2") - hints.addAttachment(attachment1) - hints.addAttachment(attachment2) - - hints.clearAttachments() - - assertEquals(emptyList(), hints.attachments) - } - - @Test - fun `after replace list contains only new item`() { - val hints = Hints() - val attachment1 = newAttachment("test1") - val attachment2 = newAttachment("test2") - val attachment3 = newAttachment("test2") - val attachment4 = newAttachment("test2") - hints.addAttachment(attachment1) - hints.addAttachment(attachment2) - - hints.replaceAttachments(listOf(attachment3, attachment4)) - - assertEquals(listOf(attachment3, attachment4), hints.attachments) - } - - companion object { - fun newAttachment(content: String) = Attachment(content.toByteArray(), "$content.txt") - } -} diff --git a/sentry/src/test/java/io/sentry/hints/HintsTest.kt b/sentry/src/test/java/io/sentry/hints/HintsTest.kt index 984cde542fd..92e6d302cd9 100644 --- a/sentry/src/test/java/io/sentry/hints/HintsTest.kt +++ b/sentry/src/test/java/io/sentry/hints/HintsTest.kt @@ -1,6 +1,6 @@ package io.sentry.hints -import io.sentry.hints.AttachmentsTest.Companion.newAttachment +import io.sentry.Attachment import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull @@ -147,4 +147,56 @@ class HintsTest { val hints = Hints.withAttachments(listOf(attachment1, attachment2)) assertEquals(listOf(attachment1, attachment2), hints.attachments) } + + @Test + fun `can add an attachment`() { + val hints = Hints() + val attachment = newAttachment("test1") + hints.addAttachment(attachment) + + assertEquals(listOf(attachment), hints.attachments) + } + + @Test + fun `can add multiple attachments`() { + val hints = Hints() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) + + assertEquals(listOf(attachment1, attachment2), hints.attachments) + } + + @Test + fun `after reset list is empty`() { + val hints = Hints() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) + + hints.clearAttachments() + + assertEquals(emptyList(), hints.attachments) + } + + @Test + fun `after replace list contains only new item`() { + val hints = Hints() + val attachment1 = newAttachment("test1") + val attachment2 = newAttachment("test2") + val attachment3 = newAttachment("test2") + val attachment4 = newAttachment("test2") + hints.addAttachment(attachment1) + hints.addAttachment(attachment2) + + hints.replaceAttachments(listOf(attachment3, attachment4)) + + assertEquals(listOf(attachment3, attachment4), hints.attachments) + } + + companion object { + fun newAttachment(content: String) = Attachment(content.toByteArray(), "$content.txt") + } } From 34a695e51f32689000fee3ad1a1487450372c4fc Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 19 May 2022 17:43:42 +0200 Subject: [PATCH 14/15] Make primitiveMapping table static --- .../src/main/java/io/sentry/hints/Hints.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index edbbc001060..d3101ac78f0 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -10,6 +10,20 @@ public final class Hints { + private static final @NotNull Map> PRIMITIVE_MAPPINGS; + + static { + PRIMITIVE_MAPPINGS = new HashMap<>(); + PRIMITIVE_MAPPINGS.put("boolean", Boolean.class); + PRIMITIVE_MAPPINGS.put("char", Character.class); + PRIMITIVE_MAPPINGS.put("byte", Byte.class); + PRIMITIVE_MAPPINGS.put("short", Short.class); + PRIMITIVE_MAPPINGS.put("int", Integer.class); + PRIMITIVE_MAPPINGS.put("long", Long.class); + PRIMITIVE_MAPPINGS.put("float", Float.class); + PRIMITIVE_MAPPINGS.put("double", Double.class); + } + private final @NotNull Map internalStorage = new HashMap(); private final @NotNull List attachments = new CopyOnWriteArrayList<>(); private @Nullable Attachment screenshot = null; @@ -26,18 +40,6 @@ public final class Hints { return hints; } - public Hints() { - primitiveMappings = new HashMap<>(); - primitiveMappings.put("boolean", Boolean.class); - primitiveMappings.put("char", Character.class); - primitiveMappings.put("byte", Byte.class); - primitiveMappings.put("short", Short.class); - primitiveMappings.put("int", Integer.class); - primitiveMappings.put("long", Long.class); - primitiveMappings.put("float", Float.class); - primitiveMappings.put("double", Double.class); - } - public void set(@NotNull String name, @Nullable Object hint) { internalStorage.put(name, hint); } @@ -96,10 +98,8 @@ public void setScreenshot(@Nullable Attachment screenshot) { return screenshot; } - private final Map> primitiveMappings; - private boolean isCastablePrimitive(@Nullable Object hintValue, @NotNull Class clazz) { - Class nonPrimitiveClass = primitiveMappings.get(clazz.getCanonicalName()); + Class nonPrimitiveClass = PRIMITIVE_MAPPINGS.get(clazz.getCanonicalName()); return hintValue != null && clazz.isPrimitive() && nonPrimitiveClass != null From 84ab09fcc69ae49413685e83aae1a902bf7e1c5d Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 19 May 2022 17:57:35 +0200 Subject: [PATCH 15/15] Use ArrayList as there should not be a synchronization issue for hints --- sentry/src/main/java/io/sentry/hints/Hints.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/hints/Hints.java b/sentry/src/main/java/io/sentry/hints/Hints.java index d3101ac78f0..0475df60cf7 100644 --- a/sentry/src/main/java/io/sentry/hints/Hints.java +++ b/sentry/src/main/java/io/sentry/hints/Hints.java @@ -1,10 +1,10 @@ package io.sentry.hints; import io.sentry.Attachment; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.CopyOnWriteArrayList; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -25,7 +25,7 @@ public final class Hints { } private final @NotNull Map internalStorage = new HashMap(); - private final @NotNull List attachments = new CopyOnWriteArrayList<>(); + private final @NotNull List attachments = new ArrayList<>(); private @Nullable Attachment screenshot = null; public static @NotNull Hints withAttachment(@Nullable Attachment attachment) { @@ -78,7 +78,7 @@ public void addAttachments(@Nullable List attachments) { } public @NotNull List getAttachments() { - return new CopyOnWriteArrayList<>(attachments); + return new ArrayList<>(attachments); } public void replaceAttachments(@Nullable List attachments) {