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

Replace user other with data #2258

Merged
Show file tree
Hide file tree
Changes from 2 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
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))
- Make user segment a top level property ([#2258](https://github.com/getsentry/sentry-java/pull/2258))
Copy link
Contributor

Choose a reason for hiding this comment

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

PR title and changelog seem to be off, this is not about the user.segment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow good catch thanks! Not sure how that happened. Guess I just rely on the auto generated title too much and in this case it used the title from the PR I want to merge into.


## 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
48 changes: 40 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 getData instead
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the @link javadocs
Eg {{@link SentryOptions#getShutdownTimeoutMillis()} }

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

* @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 setData 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,18 @@ 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

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 deserializeLegacy() {
Copy link
Member

Choose a reason for hiding this comment

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

馃憤 for adding a test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use backticks and call it deserialize legacy

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

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"
}
}