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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make user segment a top level property #2257

Merged
merged 5 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Changelog

## Unreleased

### Features

- Make user segment a top level property ([#2257](https://github.com/getsentry/sentry-java/pull/2257))
- Replace user `other` with `data` ([#2258](https://github.com/getsentry/sentry-java/pull/2258))

## 6.5.0-beta.1

### Features
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ private void apply(final @NotNull User existingUser, final @Nullable User userFr
Optional.ofNullable(userFromProvider.getId()).ifPresent(existingUser::setId);
Optional.ofNullable(userFromProvider.getIpAddress()).ifPresent(existingUser::setIpAddress);
Optional.ofNullable(userFromProvider.getUsername()).ifPresent(existingUser::setUsername);
if (userFromProvider.getOthers() != null && !userFromProvider.getOthers().isEmpty()) {
Map<String, String> existingUserOthers = existingUser.getOthers();
if (existingUserOthers == null) {
existingUserOthers = new ConcurrentHashMap<>();
if (userFromProvider.getData() != null && !userFromProvider.getData().isEmpty()) {
Map<String, String> existingUserData = existingUser.getData();
if (existingUserData == null) {
existingUserData = new ConcurrentHashMap<>();
}
for (final Map.Entry<String, String> entry : userFromProvider.getOthers().entrySet()) {
existingUserOthers.put(entry.getKey(), entry.getValue());
for (final Map.Entry<String, String> entry : userFromProvider.getData().entrySet()) {
existingUserData.put(entry.getKey(), entry.getValue());
}
existingUser.setOthers(existingUserOthers);
existingUser.setData(existingUserData);
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -3017,17 +3017,21 @@ public final class io/sentry/protocol/TransactionNameSource : java/lang/Enum {
public final class io/sentry/protocol/User : io/sentry/JsonSerializable, io/sentry/JsonUnknown {
public fun <init> ()V
public fun <init> (Lio/sentry/protocol/User;)V
public fun getData ()Ljava/util/Map;
public fun getEmail ()Ljava/lang/String;
public fun getId ()Ljava/lang/String;
public fun getIpAddress ()Ljava/lang/String;
public fun getOthers ()Ljava/util/Map;
public fun getSegment ()Ljava/lang/String;
public fun getUnknown ()Ljava/util/Map;
public fun getUsername ()Ljava/lang/String;
public fun serialize (Lio/sentry/JsonObjectWriter;Lio/sentry/ILogger;)V
public fun setData (Ljava/util/Map;)V
public fun setEmail (Ljava/lang/String;)V
public fun setId (Ljava/lang/String;)V
public fun setIpAddress (Ljava/lang/String;)V
public fun setOthers (Ljava/util/Map;)V
public fun setSegment (Ljava/lang/String;)V
public fun setUnknown (Ljava/util/Map;)V
public fun setUsername (Ljava/lang/String;)V
}
Expand All @@ -3039,10 +3043,12 @@ public final class io/sentry/protocol/User$Deserializer : io/sentry/JsonDeserial
}

public final class io/sentry/protocol/User$JsonKeys {
public static final field DATA Ljava/lang/String;
public static final field EMAIL Ljava/lang/String;
public static final field ID Ljava/lang/String;
public static final field IP_ADDRESS Ljava/lang/String;
public static final field OTHER Ljava/lang/String;
public static final field SEGMENT Ljava/lang/String;
public static final field USERNAME Ljava/lang/String;
public fun <init> ()V
}
Expand Down
10 changes: 7 additions & 3 deletions sentry/src/main/java/io/sentry/Baggage.java
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,13 @@ public void setValuesFromTransaction(
}

private static @Nullable String getSegment(final @NotNull User user) {
final Map<String, String> others = user.getOthers();
if (others != null) {
return others.get("segment");
if (user.getSegment() != null) {
return user.getSegment();
}

final Map<String, String> userData = user.getData();
if (userData != null) {
return userData.get("segment");
} else {
return null;
}
Expand Down
77 changes: 69 additions & 8 deletions sentry/src/main/java/io/sentry/protocol/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@ public final class User implements JsonUnknown, JsonSerializable {
/** Username of the user. */
private @Nullable String username;

private @Nullable String segment;

/** Remote IP address of the user. */
private @Nullable String ipAddress;

/**
* Additional arbitrary fields, as stored in the database (and sometimes as sent by clients). All
* data from `self.other` should end up here after store normalization.
*/
private @Nullable Map<String, @NotNull String> other;
private @Nullable Map<String, @NotNull String> data;

/** unknown fields, only internal usage. */
private @Nullable Map<String, @NotNull Object> unknown;
Expand All @@ -50,7 +52,8 @@ public User(final @NotNull User user) {
this.username = user.username;
this.id = user.id;
this.ipAddress = user.ipAddress;
this.other = CollectionUtils.newConcurrentHashMap(user.other);
this.segment = user.segment;
this.data = CollectionUtils.newConcurrentHashMap(user.data);
this.unknown = CollectionUtils.newConcurrentHashMap(user.unknown);
}

Expand Down Expand Up @@ -108,6 +111,24 @@ public void setUsername(final @Nullable String username) {
this.username = username;
}

/**
* Gets the segment of the user.
*
* @return the user segment.
*/
public @Nullable String getSegment() {
return segment;
}

/**
* Sets the segment of the user.
*
* @param segment the segment.
*/
public void setSegment(final @Nullable String segment) {
this.segment = segment;
}

/**
* Gets the IP address of the user.
*
Expand All @@ -129,19 +150,43 @@ public void setIpAddress(final @Nullable String ipAddress) {
/**
* Gets other user related data.
*
* @deprecated use {{@link User#getData()}} instead
* @return the other user data.
*/
@Deprecated
@SuppressWarnings("InlineMeSuggester")
public @Nullable Map<String, @NotNull String> getOthers() {
return other;
return getData();
}

/**
* Sets other user related data.
*
* @deprecated use {{@link User#setData(Map)}} instead
* @param other the other user related data..
*/
@Deprecated
@SuppressWarnings("InlineMeSuggester")
public void setOthers(final @Nullable Map<String, @NotNull String> other) {
this.other = CollectionUtils.newConcurrentHashMap(other);
this.setData(other);
}

/**
* Gets additional arbitrary fields of the user.
*
* @return the other user data.
*/
public @Nullable Map<String, @NotNull String> getData() {
return data;
}

/**
* Sets additional arbitrary fields of the user.
*
* @param data the other user related data..
*/
public void setData(final @Nullable Map<String, @NotNull String> data) {
this.data = CollectionUtils.newConcurrentHashMap(data);
}

// region json
Expand All @@ -161,8 +206,10 @@ public static final class JsonKeys {
public static final String EMAIL = "email";
public static final String ID = "id";
public static final String USERNAME = "username";
public static final String SEGMENT = "segment";
public static final String IP_ADDRESS = "ip_address";
public static final String OTHER = "other";
public static final String DATA = "data";
}

@Override
Expand All @@ -178,11 +225,14 @@ public void serialize(@NotNull JsonObjectWriter writer, @NotNull ILogger logger)
if (username != null) {
writer.name(JsonKeys.USERNAME).value(username);
}
if (segment != null) {
writer.name(JsonKeys.SEGMENT).value(segment);
}
if (ipAddress != null) {
writer.name(JsonKeys.IP_ADDRESS).value(ipAddress);
}
if (other != null) {
writer.name(JsonKeys.OTHER).value(logger, other);
if (data != null) {
writer.name(JsonKeys.DATA).value(logger, data);
}
if (unknown != null) {
for (String key : unknown.keySet()) {
Expand Down Expand Up @@ -214,14 +264,25 @@ public static final class Deserializer implements JsonDeserializer<User> {
case JsonKeys.USERNAME:
user.username = reader.nextStringOrNull();
break;
case JsonKeys.SEGMENT:
user.segment = reader.nextStringOrNull();
break;
case JsonKeys.IP_ADDRESS:
user.ipAddress = reader.nextStringOrNull();
break;
case JsonKeys.OTHER:
user.other =
case JsonKeys.DATA:
user.data =
CollectionUtils.newConcurrentHashMap(
(Map<String, String>) reader.nextObjectOrNull());
break;
case JsonKeys.OTHER:
// restore `other` from legacy JSON
if (user.data == null || user.data.isEmpty()) {
user.data =
CollectionUtils.newConcurrentHashMap(
(Map<String, String>) reader.nextObjectOrNull());
}
break;
default:
if (unknown == null) {
unknown = new ConcurrentHashMap<>();
Expand Down
4 changes: 2 additions & 2 deletions sentry/src/test/java/io/sentry/ScopeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ class ScopeTest {
user.id = "123"
user.ipAddress = "123.x"
user.username = "userName"
val others = mutableMapOf(Pair("others", "others"))
user.others = others
val data = mutableMapOf(Pair("data", "data"))
user.data = data

scope.user = user

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ class UserSerializationTest {
assertEquals(expectedJson, actualJson)
}

@Test
fun `deserialize legacy`() {
val inputJson = sanitizedFile("json/user_legacy.json")
val expectedJson = sanitizedFile("json/user.json")
val actual = deserialize(inputJson)
val actualJson = serialize(actual)
assertEquals(expectedJson, actualJson)
}

// Helper

private fun sanitizedFile(path: String): String {
Expand Down
34 changes: 19 additions & 15 deletions sentry/src/test/java/io/sentry/protocol/UserTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class UserTest {
assertNotNull(clone)
assertNotSame(user, clone)

assertNotSame(user.others, clone.others)
assertNotSame(user.data, clone.data)

assertNotSame(user.unknown, clone.unknown)
}
Expand All @@ -31,7 +31,8 @@ class UserTest {
assertEquals("123", clone.id)
assertEquals("123.x", clone.ipAddress)
assertEquals("userName", clone.username)
assertEquals("others", clone.others!!["others"])
assertEquals("userSegment", clone.segment)
assertEquals("data", clone.data!!["data"])
assertEquals("unknown", clone.unknown!!["unknown"])
}

Expand All @@ -44,36 +45,38 @@ class UserTest {
user.id = "456"
user.ipAddress = "456.x"
user.username = "newUserName"
user.others!!["others"] = "newOthers"
user.others!!["anotherOne"] = "anotherOne"
user.segment = "newUserSegment"
user.data!!["data"] = "newOthers"
user.data!!["anotherOne"] = "anotherOne"
val newUnknown = mapOf(Pair("unknown", "newUnknown"), Pair("otherUnknown", "otherUnknown"))
user.setUnknown(newUnknown)

assertEquals("a@a.com", clone.email)
assertEquals("123", clone.id)
assertEquals("123.x", clone.ipAddress)
assertEquals("userName", clone.username)
assertEquals("others", clone.others!!["others"])
assertEquals(1, clone.others!!.size)
assertEquals("userSegment", clone.segment)
assertEquals("data", clone.data!!["data"])
assertEquals(1, clone.data!!.size)
assertEquals("unknown", clone.unknown!!["unknown"])
assertEquals(1, clone.unknown!!.size)
}

@Test
fun `setting null others do not crash`() {
fun `setting null data do not crash`() {
val user = createUser()
user.others = null
user.data = null

assertNull(user.others)
assertNull(user.data)
}

@Test
fun `when setOther receives immutable map as an argument, its still possible to add more others to the user`() {
fun `when setOther receives immutable map as an argument, its still possible to add more data to the user`() {
val user = User().apply {
others = Collections.unmodifiableMap(mapOf("key1" to "value1"))
others!!["key2"] = "value2"
data = Collections.unmodifiableMap(mapOf("key1" to "value1"))
data!!["key2"] = "value2"
}
assertNotNull(user.others) {
assertNotNull(user.data) {
assertEquals(mapOf("key1" to "value1", "key2" to "value2"), it)
}
}
Expand All @@ -84,8 +87,9 @@ class UserTest {
id = "123"
ipAddress = "123.x"
username = "userName"
val others = mutableMapOf(Pair("others", "others"))
setOthers(others)
segment = "userSegment"
val data = mutableMapOf(Pair("data", "data"))
setData(data)
val unknown = mapOf(Pair("unknown", "unknown"))
setUnknown(unknown)
}
Expand Down
2 changes: 1 addition & 1 deletion sentry/src/test/resources/json/sentry_base_event.json
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@
"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
"data":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
Expand Down
2 changes: 1 addition & 1 deletion sentry/src/test/resources/json/sentry_event.json
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@
"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
"data":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
Expand Down
2 changes: 1 addition & 1 deletion sentry/src/test/resources/json/sentry_transaction.json
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@
"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
"data":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@
"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
"data":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@
"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
"data":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
Expand Down