Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling immutable collections on SentryEvent and protocol objects #1468

Merged
merged 10 commits into from
May 12, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ private void setSideLoadedInfo(final @NotNull SentryBaseEvent event) {
if (sideLoadedInfo instanceof Map) {
for (final Map.Entry<String, String> entry :
((Map<String, String>) sideLoadedInfo).entrySet()) {
event.setTag(entry.getKey(), entry.getValue());
event.addTag(entry.getKey(), entry.getValue());
}
}
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class SentryTimberTree(
}

tag?.let {
setTag("TimberTag", it)
addTag("TimberTag", it)
}

logger = "Timber"
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/MainEventProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ private void setSdk(final @NotNull SentryBaseEvent event) {

private void setTags(final @NotNull SentryBaseEvent event) {
if (event.getTags() == null) {
event.setTags(new HashMap<>(options.getTags()));
event.addTags(new HashMap<>(options.getTags()));
} else {
for (Map.Entry<String, String> item : options.getTags().entrySet()) {
if (!event.getTags().containsKey(item.getKey())) {
event.setTag(item.getKey(), item.getValue());
event.addTag(item.getKey(), item.getValue());
}
}
}
Expand Down
22 changes: 17 additions & 5 deletions sentry/src/main/java/io/sentry/SentryBaseEvent.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,15 @@ public Map<String, String> getTags() {
return tags;
}

public void setTags(Map<String, String> tags) {
this.tags = tags;
public void addTags(Map<String, String> tags) {
maciejwalkowiak marked this conversation as resolved.
Show resolved Hide resolved
if (tags != null) {
if (this.tags == null) {
this.tags = new HashMap<>();
}
for (final Map.Entry<String, String> entry : tags.entrySet()) {
this.tags.put(entry.getKey(), entry.getValue());
}
}
}

public void removeTag(@NotNull String key) {
Expand All @@ -202,7 +209,7 @@ public void removeTag(@NotNull String key) {
return null;
}

public void setTag(String key, String value) {
public void addTag(String key, String value) {
if (tags == null) {
tags = new HashMap<>();
}
Expand Down Expand Up @@ -261,8 +268,13 @@ public List<Breadcrumb> getBreadcrumbs() {
return breadcrumbs;
}

public void setBreadcrumbs(List<Breadcrumb> breadcrumbs) {
this.breadcrumbs = breadcrumbs;
public void addBreadcrumbs(List<Breadcrumb> breadcrumbs) {
if (breadcrumbs != null) {
if (this.breadcrumbs == null) {
this.breadcrumbs = new ArrayList<>();
}
this.breadcrumbs.addAll(breadcrumbs);
}
}

public void addBreadcrumb(Breadcrumb breadcrumb) {
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/main/java/io/sentry/SentryClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ private <T extends SentryBaseEvent> T applyScope(
sentryBaseEvent.setUser(scope.getUser());
}
if (sentryBaseEvent.getTags() == null) {
sentryBaseEvent.setTags(new HashMap<>(scope.getTags()));
sentryBaseEvent.addTags(new HashMap<>(scope.getTags()));
} else {
for (Map.Entry<String, String> item : scope.getTags().entrySet()) {
if (!sentryBaseEvent.getTags().containsKey(item.getKey())) {
Expand All @@ -524,7 +524,7 @@ private <T extends SentryBaseEvent> T applyScope(
}
}
if (sentryBaseEvent.getBreadcrumbs() == null) {
sentryBaseEvent.setBreadcrumbs(new ArrayList<>(scope.getBreadcrumbs()));
sentryBaseEvent.addBreadcrumbs(new ArrayList<>(scope.getBreadcrumbs()));
} else {
sortBreadcrumbsByDate(sentryBaseEvent, scope.getBreadcrumbs());
}
Expand Down
2 changes: 1 addition & 1 deletion sentry/src/test/java/io/sentry/GsonSerializerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ class GsonSerializerTest {
@Test
fun `empty maps are serialized to null`() {
val event = SentryEvent()
event.tags = emptyMap()
event.addTags(emptyMap())
val element = JsonParser().parse(serializeToString(event)).asJsonObject
assertNull(element.asJsonObject["tags"])
}
Expand Down
2 changes: 1 addition & 1 deletion sentry/src/test/java/io/sentry/MainEventProcessorTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class MainEventProcessorTest {
fun `when event has a tag set with the same name as SentryOptions tags, the tag value from the event is retained`() {
val sut = fixture.getSut(tags = mapOf("tag1" to "value1", "tag2" to "value2"))
val event = SentryEvent()
event.setTag("tag2", "event-tag-value")
event.addTag("tag2", "event-tag-value")
sut.process(event, null)
assertEquals("value1", event.tags["tag1"])
assertEquals("event-tag-value", event.tags["tag2"])
Expand Down
10 changes: 5 additions & 5 deletions sentry/src/test/java/io/sentry/SentryClientTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class SentryClientTest {
@Test
fun `when beforeSend is returns new instance, new instance is sent`() {
val expected = SentryEvent().apply {
setTag("test", "test")
addTag("test", "test")
}
fixture.sentryOptions.setBeforeSend { _, _ -> expected }
val sut = fixture.getSut()
Expand Down Expand Up @@ -286,7 +286,7 @@ class SentryClientTest {

val b3 = Breadcrumb(DateUtils.getDateTime("2020-03-27T08:52:58.003Z"))
val event = SentryEvent().apply {
breadcrumbs = mutableListOf(b3)
addBreadcrumbs(mutableListOf(b3))
}

sut.captureEvent(event, scope)
Expand Down Expand Up @@ -952,8 +952,8 @@ class SentryClientTest {
fixture.sentryOptions.setTag("tag2", "value2")
val sut = fixture.getSut()
val transaction = SentryTransaction(SentryTracer(TransactionContext("name", "op"), mock()))
transaction.setTag("tag3", "value3")
transaction.setTag("tag2", "transaction-tag")
transaction.addTag("tag3", "value3")
transaction.addTag("tag2", "transaction-tag")
sut.captureTransaction(transaction)
assertEquals(mapOf("tag1" to "value1", "tag2" to "transaction-tag", "tag3" to "value3"), transaction.tags)
}
Expand Down Expand Up @@ -1017,7 +1017,7 @@ class SentryClientTest {
message = "eventMessage"
})
setExtra("eventExtra", "eventExtra")
setTag("eventTag", "eventTag")
addTag("eventTag", "eventTag")
fingerprints = listOf("eventFp")
transaction = "eventTransaction"
level = SentryLevel.DEBUG
Expand Down
29 changes: 29 additions & 0 deletions sentry/src/test/java/io/sentry/SentryEventTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,33 @@ class SentryEventTest {
event.throwable = ex
assertEquals(ex, event.originThrowable)
}

@Test
fun `addBreadcrumbs copies elements to SentryEvent and does not change the event breadcrumbs reference`() {
val event = SentryEvent()
event.addBreadcrumb("breadcrumb")
val eventBreadcrumbs = event.breadcrumbs

val breadcrumbsList = listOf(Breadcrumb(), Breadcrumb())
event.addBreadcrumbs(breadcrumbsList)

assertEquals(3, event.breadcrumbs.size)
assertEquals(eventBreadcrumbs, event.breadcrumbs)
assertNotEquals(breadcrumbsList, event.breadcrumbs)
}

@Test
fun `addTags copies elements to SentryEvent and does not change the event tags reference`() {
val event = SentryEvent()
event.addTag("key1", "value1")
val eventTags = event.tags

val tagsMap = mapOf("key2" to "value2", "key3" to "value3")
event.addTags(tagsMap)

assertEquals(3, event.tags.size)
assertEquals(eventTags, event.tags)
assertNotEquals(tagsMap, event.tags)

}
}