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

Replace user other with data #2258

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### 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

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
3 changes: 3 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -2970,6 +2970,7 @@ 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;
Expand All @@ -2978,6 +2979,7 @@ public final class io/sentry/protocol/User : io/sentry/JsonSerializable, io/sent
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
Expand All @@ -2994,6 +2996,7 @@ 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;
Expand Down
6 changes: 3 additions & 3 deletions sentry/src/main/java/io/sentry/Baggage.java
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,9 @@ public void setValuesFromTransaction(
return user.getSegment();
}

final Map<String, String> others = user.getOthers();
if (others != null) {
return others.get("segment");
final Map<String, String> userData = user.getData();
if (userData != null) {
return userData.get("segment");
} else {
return null;
}
Expand Down
49 changes: 41 additions & 8 deletions sentry/src/main/java/io/sentry/protocol/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public final class User implements JsonUnknown, JsonSerializable {
* 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 @@ -53,7 +53,7 @@ public User(final @NotNull User user) {
this.id = user.id;
this.ipAddress = user.ipAddress;
this.segment = user.segment;
this.other = CollectionUtils.newConcurrentHashMap(user.other);
this.data = CollectionUtils.newConcurrentHashMap(user.data);
this.unknown = CollectionUtils.newConcurrentHashMap(user.unknown);
}

Expand Down Expand Up @@ -150,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 @@ -185,6 +209,7 @@ public static final class JsonKeys {
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 @@ -206,8 +231,8 @@ public void serialize(@NotNull JsonObjectWriter writer, @NotNull ILogger logger)
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 @@ -245,11 +270,19 @@ public static final class Deserializer implements JsonDeserializer<User> {
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:
Copy link
Member

Choose a reason for hiding this comment

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

l: Maybe a short comment on why we still have this?

Copy link
Member Author

Choose a reason for hiding this comment

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

added

// 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
30 changes: 15 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 @@ -32,7 +32,7 @@ class UserTest {
assertEquals("123.x", clone.ipAddress)
assertEquals("userName", clone.username)
assertEquals("userSegment", clone.segment)
assertEquals("others", clone.others!!["others"])
assertEquals("data", clone.data!!["data"])
assertEquals("unknown", clone.unknown!!["unknown"])
}

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

Expand All @@ -56,27 +56,27 @@ class UserTest {
assertEquals("123.x", clone.ipAddress)
assertEquals("userName", clone.username)
assertEquals("userSegment", clone.segment)
assertEquals("others", clone.others!!["others"])
assertEquals(1, clone.others!!.size)
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 @@ -88,8 +88,8 @@ class UserTest {
ipAddress = "123.x"
username = "userName"
segment = "userSegment"
val others = mutableMapOf(Pair("others", "others"))
setOthers(others)
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
4 changes: 2 additions & 2 deletions sentry/src/test/resources/json/user.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
"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"
}
}
}
10 changes: 10 additions & 0 deletions sentry/src/test/resources/json/user_legacy.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"email": "c4d61c1b-c144-431e-868f-37a46be5e5f2",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this 👏

"id": "efb2084b-1871-4b59-8897-b4bd9f196a01",
"username": "60c05dff-7140-4d94-9a61-c9cdd9ca9b96",
"ip_address": "51d22b77-f663-4dbe-8103-8b749d1d9a48",
"other":
{
"dc2813d0-0f66-4a3f-a995-71268f61a8fa": "991659ad-7c59-4dd3-bb89-0bd5c74014bd"
}
}