Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Fix: Apply user from the scope to transaction. #1424

Merged
merged 5 commits into from
Apr 21, 2021
Merged

Fix: Apply user from the scope to transaction. #1424

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de41eb8
Apply user from the scope to transaction.
maciejwalkowiak Apr 19, 2021
a79f6a3
Changelog.
maciejwalkowiak Apr 19, 2021
ddd27b1
Fix Android test.
maciejwalkowiak Apr 19, 2021
f003a37
Add nullability annotation.
maciejwalkowiak Apr 19, 2021
86c0a40
Merge branch 'main' into gh-1335-2
marandaneto Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Fix: set correct transaction status for unhandled exceptions in SentryTracingFilter (#1406)
* Fix: handle network errors in SentrySpanClientHttpRequestInterceptor (#1407)
* Fix: set scope on transaction (#1409)
* Fix: Apply user from the scope to transaction (#1424)

# 4.4.0-alpha.2

Expand Down
5 changes: 3 additions & 2 deletions sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,9 @@ class DefaultAndroidEventProcessorTest {
setUser(user)
}
event = sut.process(event, null)
assertNotNull(event.user)
assertNotNull(event.user.id)
assertNotNull(event.user) {
assertNotNull(it.id)
}
}

@Test
Expand Down
7 changes: 4 additions & 3 deletions ...ring-boot-starter/src/test/kotlin/io/sentry/spring/boot/it/SentrySpringIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ class SentrySpringIntegrationTest {
assertThat(event.request).isNotNull()
assertThat(event.request!!.url).isEqualTo("http://localhost:$port/hello")
assertThat(event.user).isNotNull()
assertThat(event.user.username).isEqualTo("user")
assertThat(event.user.ipAddress).isEqualTo("169.128.0.1")
assertThat(event.user!!.username).isEqualTo("user")
assertThat(event.user!!.ipAddress).isEqualTo("169.128.0.1")
Comment on lines 86 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use assertNotNull with the lambda so we avoid !! for all user asserts on this PR?

}, anyOrNull())
}
}
Expand All @@ -101,7 +101,8 @@ class SentrySpringIntegrationTest {

await.untilAsserted {
verify(transport).send(checkEvent { event ->
assertThat(event.user.ipAddress).isEqualTo("169.128.0.1")
assertThat(event.user).isNotNull()
assertThat(event.user!!.ipAddress).isEqualTo("169.128.0.1")
}, anyOrNull())
}
}
Expand Down
7 changes: 4 additions & 3 deletions sentry-spring/src/test/kotlin/io/sentry/spring/SentrySpringIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ class SentrySpringIntegrationTest {
assertThat(event.request).isNotNull()
assertThat(event.request!!.url).isEqualTo("http://localhost:$port/hello")
assertThat(event.user).isNotNull()
assertThat(event.user.username).isEqualTo("user")
assertThat(event.user.ipAddress).isEqualTo("169.128.0.1")
assertThat(event.user!!.username).isEqualTo("user")
assertThat(event.user!!.ipAddress).isEqualTo("169.128.0.1")
}, anyOrNull())
}
}
Expand All @@ -92,7 +92,8 @@ class SentrySpringIntegrationTest {

await.untilAsserted {
verify(transport).send(checkEvent { event ->
assertThat(event.user.ipAddress).isEqualTo("169.128.0.1")
assertThat(event.user).isNotNull()
assertThat(event.user!!.ipAddress).isEqualTo("169.128.0.1")
}, anyOrNull())
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...pring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class SentryUserProviderEventProcessorIntegrationTest {
await.untilAsserted {
verify(transport).send(checkEvent { event: SentryEvent ->
assertThat(event.user).isNotNull
assertThat(event.user.username).isEqualTo("john.smith")
assertThat(event.user!!.username).isEqualTo("john.smith")
}, anyOrNull())
}
}
Expand Down
96 changes: 54 additions & 42 deletions sentry-spring/src/test/kotlin/io/sentry/spring/SentryUserProviderEventProcessorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@ class SentryUserProviderEventProcessorTest {
val result = processor.process(event, null)

assertNotNull(result)
assertNotNull(result.user)
assertEquals("john.doe", result.user.username)
assertEquals("user-id", result.user.id)
assertEquals("192.168.0.1", result.user.ipAddress)
assertEquals("john.doe@example.com", result.user.email)
assertEquals(mapOf("key" to "value"), result.user.others)
assertNotNull(result.user) {
assertEquals("john.doe", it.username)
assertEquals("user-id", it.id)
assertEquals("192.168.0.1", it.ipAddress)
assertEquals("john.doe@example.com", it.email)
assertEquals(mapOf("key" to "value"), it.others)
}
Comment on lines +40 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

this block is duplicated across tests, maybe we extract it to a method called asserUser or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in case of this test class duplicated code is a good trade off. Having fixed string values in assertUser method although resolves duplication, doesn't feel right to couple this method with test methods. Feel free to refactor it if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

tbh I'm not a big fan of that, it's the same strings and same asserts across every test, every time you'd need to change either a value or adding/removing a field, you've to change it everywhere.
I know @philipphofmann has strong opinions about that too, it's not scalable when the number of tests increases.

Copy link
Member

Choose a reason for hiding this comment

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

I agree 100% with @marandaneto. Treat your test code as production code. A few duplications are fine, but in this case, I would extract them to a method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will merge as it is because I am blocked by this PR, but lets please discuss and refactor this as soon as we touch this class again

Copy link
Member

Choose a reason for hiding this comment

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

I'm not willing to die on this hill so bare with me:

I believe a test should be as self-sufficient as possible. When a test calls out to a private method getUser then assertUser it isn't clear what user is, and how assert works, unless I peek into those methods. It also means that I can be setting the user now thinking of a particular test, but be negatively affecting another, without knowing, since multiple tests "change" by means of modifying these common functions.

It has the advantage, obviously, of having a single place to "fix" things, or even to add a new property to be set and asserted. It But that comes with the cost of potentially changing a test that wasn't supposed to be changed.

Perhaps what we're missing here is a library that validates two objects are "equivalent". It solves the problem of adding a new property to the field and also expecting that to be mapped through which is a common mistake when mapping objects by hand.

Both these principles/approaches are used in the .NET SDK. The assertion library (introduced by @Tyrrrz, who from what I noticed also follows the idea of self-sufficient tests) is called FluentAssertions , is there something similar for Java?

All of that said, this class in particular does have a lot of duplication for basic object mapping, to the point that this diff was mainly changes to asserting user. I'd be very tempted to at least create a assertUserEquals(User a, User b) to get rid of this duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kotlin generates equals and hashCode for data classes, but Java does not, we'd need to implement and bloat the SDK with that (which is gonna be used only in tests), so we could do an equality check.

another approach is having a test per field, instead of the whole user every time.

I do agree that's a trade-off, but I often spend more time scaling the tests other than "understanding" them.

}

@Test
Expand All @@ -62,12 +63,13 @@ class SentryUserProviderEventProcessorTest {
val result = processor.process(event, null)

assertNotNull(result)
assertNotNull(result.user)
assertEquals("john.doe", result.user.username)
assertEquals("user-id", result.user.id)
assertEquals("192.168.0.1", result.user.ipAddress)
assertEquals("john.doe@example.com", result.user.email)
assertEquals(mapOf("key" to "value"), result.user.others)
assertNotNull(result.user) {
assertEquals("john.doe", it.username)
assertEquals("user-id", it.id)
assertEquals("192.168.0.1", it.ipAddress)
assertEquals("john.doe@example.com", it.email)
assertEquals(mapOf("key" to "value"), it.others)
}
}

@Test
Expand All @@ -78,22 +80,24 @@ class SentryUserProviderEventProcessorTest {
}

val event = SentryEvent()
event.user = User()
event.user.username = "jane.smith"
event.user.id = "jane-smith"
event.user.ipAddress = "192.168.0.3"
event.user.email = "jane.smith@example.com"
event.user.others = mapOf("key" to "value")
event.user = User().apply {
username = "jane.smith"
id = "jane-smith"
ipAddress = "192.168.0.3"
email = "jane.smith@example.com"
others = mapOf("key" to "value")
}

val result = processor.process(event, null)

assertNotNull(result)
assertNotNull(result.user)
assertEquals("jane.smith", result.user.username)
assertEquals("jane-smith", result.user.id)
assertEquals("192.168.0.3", result.user.ipAddress)
assertEquals("jane.smith@example.com", result.user.email)
assertEquals(mapOf("key" to "value"), result.user.others)
assertNotNull(result.user) {
assertEquals("jane.smith", it.username)
assertEquals("jane-smith", it.id)
assertEquals("192.168.0.3", it.ipAddress)
assertEquals("jane.smith@example.com", it.email)
assertEquals(mapOf("key" to "value"), it.others)
}
}

@Test
Expand All @@ -103,22 +107,24 @@ class SentryUserProviderEventProcessorTest {
}

val event = SentryEvent()
event.user = User()
event.user.username = "jane.smith"
event.user.id = "jane-smith"
event.user.ipAddress = "192.168.0.3"
event.user.email = "jane.smith@example.com"
event.user.others = mapOf("key" to "value")
event.user = User().apply {
username = "jane.smith"
id = "jane-smith"
ipAddress = "192.168.0.3"
email = "jane.smith@example.com"
others = mapOf("key" to "value")
}

val result = processor.process(event, null)

assertNotNull(result)
assertNotNull(result.user)
assertEquals("jane.smith", result.user.username)
assertEquals("jane-smith", result.user.id)
assertEquals("192.168.0.3", result.user.ipAddress)
assertEquals("jane.smith@example.com", result.user.email)
assertEquals(mapOf("key" to "value"), result.user.others)
assertNotNull(result.user) {
assertEquals("jane.smith", it.username)
assertEquals("jane-smith", it.id)
assertEquals("192.168.0.3", it.ipAddress)
assertEquals("jane.smith@example.com", it.email)
assertEquals(mapOf("key" to "value"), it.others)
}
}

@Test
Expand All @@ -130,14 +136,16 @@ class SentryUserProviderEventProcessorTest {
}

val event = SentryEvent()
event.user = User()
event.user.others = mapOf("new-key" to "new-value")
event.user = User().apply {
others = mapOf("new-key" to "new-value")
}

val result = processor.process(event, null)

assertNotNull(result)
assertNotNull(result.user)
assertEquals(mapOf("key" to "value", "new-key" to "new-value"), result.user.others)
assertNotNull(result.user) {
assertEquals(mapOf("key" to "value", "new-key" to "new-value"), it.others)
}
}

@Test
Expand Down Expand Up @@ -169,7 +177,9 @@ class SentryUserProviderEventProcessorTest {
val result = processor.process(event, null)

assertNotNull(result)
assertEquals(user.ipAddress, result.user.ipAddress)
assertNotNull(result.user) {
assertEquals(user.ipAddress, it.ipAddress)
}
}

@Test
Expand All @@ -186,6 +196,8 @@ class SentryUserProviderEventProcessorTest {
val result = processor.process(event, null)

assertNotNull(result)
assertNull(result.user.ipAddress)
assertNotNull(result.user) {
assertNull(it.ipAddress)
}
}
}
4 changes: 2 additions & 2 deletions sentry/api/sentry.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ public abstract class io/sentry/SentryBaseEvent {
public fun getTag (Ljava/lang/String;)Ljava/lang/String;
public fun getTags ()Ljava/util/Map;
public fun getThrowable ()Ljava/lang/Throwable;
public fun getUser ()Lio/sentry/protocol/User;
public fun removeTag (Ljava/lang/String;)V
public fun setEnvironment (Ljava/lang/String;)V
public fun setEventId (Lio/sentry/protocol/SentryId;)V
Expand All @@ -622,6 +623,7 @@ public abstract class io/sentry/SentryBaseEvent {
public fun setTag (Ljava/lang/String;Ljava/lang/String;)V
public fun setTags (Ljava/util/Map;)V
public fun setThrowable (Ljava/lang/Throwable;)V
public fun setUser (Lio/sentry/protocol/User;)V
}

public final class io/sentry/SentryClient : io/sentry/ISentryClient {
Expand Down Expand Up @@ -709,7 +711,6 @@ public final class io/sentry/SentryEvent : io/sentry/SentryBaseEvent, io/sentry/
public fun getTimestamp ()Ljava/util/Date;
public fun getTransaction ()Ljava/lang/String;
public fun getUnknown ()Ljava/util/Map;
public fun getUser ()Lio/sentry/protocol/User;
public fun isCrashed ()Z
public fun isErrored ()Z
public fun removeExtra (Ljava/lang/String;)V
Expand All @@ -729,7 +730,6 @@ public final class io/sentry/SentryEvent : io/sentry/SentryBaseEvent, io/sentry/
public fun setServerName (Ljava/lang/String;)V
public fun setThreads (Ljava/util/List;)V
public fun setTransaction (Ljava/lang/String;)V
public fun setUser (Lio/sentry/protocol/User;)V
}

public final class io/sentry/SentryItemType : java/lang/Enum {
Expand Down
12 changes: 12 additions & 0 deletions sentry/src/main/java/io/sentry/SentryBaseEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io.sentry.protocol.Request;
import io.sentry.protocol.SdkVersion;
import io.sentry.protocol.SentryId;
import io.sentry.protocol.User;
import java.util.HashMap;
import java.util.Map;
import org.jetbrains.annotations.ApiStatus;
Expand Down Expand Up @@ -74,6 +75,9 @@ public abstract class SentryBaseEvent {
*/
private @Nullable String platform;

/** Information about the user who triggered this event. */
private @Nullable User user;

/** The captured Throwable */
protected transient @Nullable Throwable throwable;

Expand Down Expand Up @@ -198,4 +202,12 @@ public void setEnvironment(String environment) {
public void setPlatform(final @Nullable String platform) {
this.platform = platform;
}

public @Nullable User getUser() {
return user;
}

public void setUser(final @Nullable User user) {
this.user = user;
}
}
3 changes: 3 additions & 0 deletions sentry/src/main/java/io/sentry/SentryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,9 @@ private <T extends SentryBaseEvent> T applyScope(
if (sentryBaseEvent.getRequest() == null) {
sentryBaseEvent.setRequest(scope.getRequest());
}
if (sentryBaseEvent.getUser() == null) {
sentryBaseEvent.setUser(scope.getUser());
}
Comment on lines +499 to +501
Copy link
Contributor

Choose a reason for hiding this comment

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

what's about the combination with this check and MainEventProcessor? do they conflict with each other when isSendDefaultPii is enabled? I believe one of them will never be executed if it sets to a non-null value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If user is not set on the event, SentryClient sets it from the scope. Then in MainEventProcessor if user is null it sets the user with a default ip address, if its not null but the ip address is null it sets the default ip address. What conflict do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, apply scope happens before event processors, that's why, the other way around would be problematic I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

a problem that I see is, MainEventProcessor takes into consideration the isSendDefaultPii and sets IP address with DEFAULT_IP_ADDRESS, but that won't happen for transactions, this can be fixed later IMO when we run event processors for transactions too.

Copy link
Member

Choose a reason for hiding this comment

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

We could start setting the default value even if a user is set, but the IpAddress is null.
I don't believe the "setting user semantics" dictates it's a all or nothing thing, or does it?

Can't we merge values instead?

If I do captureEvent(Event{ user = User(name)) while having sendDefaultPii=true I'd still like the the {{auto}} IP Address to be added to the user. I just set the name because that's all I have.

if (sentryBaseEvent.getTags() == null) {
sentryBaseEvent.setTags(new HashMap<>(scope.getTags()));
} else {
Expand Down
10 changes: 0 additions & 10 deletions sentry/src/main/java/io/sentry/SentryEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,6 @@ public final class SentryEvent extends SentryBaseEvent implements IUnknownProper
*/
private String transaction;

/** Information about the user who triggered this event. */
private User user;
/**
* Manual fingerprint override.
*
Expand Down Expand Up @@ -206,14 +204,6 @@ public void setTransaction(String transaction) {
this.transaction = transaction;
}

public User getUser() {
return user;
}

public void setUser(User user) {
this.user = user;
}

public List<String> getFingerprints() {
return fingerprint;
}
Expand Down
20 changes: 12 additions & 8 deletions sentry/src/test/java/io/sentry/MainEventProcessorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,19 +215,22 @@ class MainEventProcessorTest {
val sut = fixture.getSut(sendDefaultPii = true)
val event = SentryEvent()
sut.process(event, null)
assertNotNull(event.user)
assertEquals("{{auto}}", event.user.ipAddress)
assertNotNull(event.user) {
assertEquals("{{auto}}", it.ipAddress)
}
}

@Test
fun `when event has ip address set and sendDefaultPii is set to true, keeps original ip address`() {
val sut = fixture.getSut(sendDefaultPii = true)
val event = SentryEvent()
event.user = User()
event.user.ipAddress = "192.168.0.1"
event.user = User().apply {
ipAddress = "192.168.0.1"
}
sut.process(event, null)
assertNotNull(event.user)
assertEquals("192.168.0.1", event.user.ipAddress)
assertNotNull(event.user) {
assertEquals("192.168.0.1", it.ipAddress)
}
}

@Test
Expand All @@ -236,8 +239,9 @@ class MainEventProcessorTest {
val event = SentryEvent()
event.user = User()
sut.process(event, null)
assertNotNull(event.user)
assertNull(event.user.ipAddress)
assertNotNull(event.user) {
assertNull(it.ipAddress)
}
}

@Test
Expand Down
Loading