From 1a5970bcd4482d171a9278c170603cbbdad4aa4d Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 15:16:12 +0100 Subject: [PATCH 1/8] fix(user-report): Make all fields but event-id optional --- relay-general/src/protocol/user_report.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index 324c3c874b..da9fa90acd 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -8,9 +8,15 @@ pub struct UserReport { /// The event ID for which this user feedback is created. pub event_id: EventId, /// The user's name. + // Empty strings are in fact stored in the database already + #[serde(default)] pub name: String, /// The user's email address. + // Empty strings are in fact stored in the database already + #[serde(default)] pub email: String, /// Comments supplied by the user. + // Empty strings are in fact stored in the database already + #[serde(default)] pub comments: String, } From b590401a3bc4a2dc57d766db588cec1816afed52 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:14:59 +0100 Subject: [PATCH 2/8] add changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 785eae4534..7a0d118388 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,9 @@ ## 20.12.1 -- No documented changes. +**Bug Fixes**: + +- Make all fields but event-id optional to fix regressions in user feedback ingestion. ([#886](https://github.com/getsentry/relay/pull/886)) ## 20.12.0 From 37db66771cfb0411db33fce85155893dff937f61 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:20:26 +0100 Subject: [PATCH 3/8] fix changelog again --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a0d118388..deb88421f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,15 @@ # Changelog -## 20.12.1 +## Unreleased **Bug Fixes**: - Make all fields but event-id optional to fix regressions in user feedback ingestion. ([#886](https://github.com/getsentry/relay/pull/886)) +## 20.12.1 + +- No documented changes. + ## 20.12.0 **Features**: From 47fa0a548a7102cbeb842eea7adb70f1951fa534 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:23:03 +0100 Subject: [PATCH 4/8] add code comment --- relay-general/src/protocol/user_report.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index da9fa90acd..30e54ccdf6 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -3,20 +3,26 @@ use crate::protocol::EventId; use serde::{Deserialize, Serialize}; /// User feedback for an event as sent by the client to the userfeedback/userreport endpoint. +/// +/// Historically the "schema" for user report has been "defined" as the set of possible +/// keyword-arguments `sentry.models.UserReport` accepts. Anything the model constructor +/// accepts goes. +/// +/// For example, `{"email": null}` is only invalid because `UserReport(email=None).save()` is. +/// +/// The database/model schema is a bunch of not-null strings that have (pgsql) defaults, so that's +/// how we end up with this struct definition. #[derive(Debug, Deserialize, Serialize)] pub struct UserReport { /// The event ID for which this user feedback is created. pub event_id: EventId, /// The user's name. - // Empty strings are in fact stored in the database already #[serde(default)] pub name: String, /// The user's email address. - // Empty strings are in fact stored in the database already #[serde(default)] pub email: String, /// Comments supplied by the user. - // Empty strings are in fact stored in the database already #[serde(default)] pub comments: String, } From 0ea86545feca1a9df53e11968258f433cf5e7199 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:23:59 +0100 Subject: [PATCH 5/8] more comments --- relay-general/src/protocol/user_report.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index 30e54ccdf6..d16093e3df 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -8,7 +8,9 @@ use serde::{Deserialize, Serialize}; /// keyword-arguments `sentry.models.UserReport` accepts. Anything the model constructor /// accepts goes. /// -/// For example, `{"email": null}` is only invalid because `UserReport(email=None).save()` is. +/// For example, `{"email": null}` is only invalid because `UserReport(email=None).save()` is. SDKs +/// may neither send this (we can relax this), but more importantly the ingest consumer may never +/// receive this. /// /// The database/model schema is a bunch of not-null strings that have (pgsql) defaults, so that's /// how we end up with this struct definition. From f3cd6cd923f53448516f3db881d8842658951958 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:24:29 +0100 Subject: [PATCH 6/8] more comments --- relay-general/src/protocol/user_report.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index d16093e3df..6c1b339b8a 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -10,7 +10,8 @@ use serde::{Deserialize, Serialize}; /// /// For example, `{"email": null}` is only invalid because `UserReport(email=None).save()` is. SDKs /// may neither send this (we can relax this), but more importantly the ingest consumer may never -/// receive this. +/// receive this... because it would end up crashing the ingest consumer (while in older versions +/// of Sentry it would simply crash the endpoint). /// /// The database/model schema is a bunch of not-null strings that have (pgsql) defaults, so that's /// how we end up with this struct definition. From 1345fd7e1c8d66c7ff3f728c82c5d55e0a7b0517 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:26:41 +0100 Subject: [PATCH 7/8] allow null --- relay-general/src/protocol/user_report.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index 6c1b339b8a..32e3877bfb 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -9,9 +9,9 @@ use serde::{Deserialize, Serialize}; /// accepts goes. /// /// For example, `{"email": null}` is only invalid because `UserReport(email=None).save()` is. SDKs -/// may neither send this (we can relax this), but more importantly the ingest consumer may never -/// receive this... because it would end up crashing the ingest consumer (while in older versions -/// of Sentry it would simply crash the endpoint). +/// may neither send this (historically, in Relay we relaxed this already), but more importantly +/// the ingest consumer may never receive this... because it would end up crashing the ingest +/// consumer (while in older versions of Sentry it would simply crash the endpoint). /// /// The database/model schema is a bunch of not-null strings that have (pgsql) defaults, so that's /// how we end up with this struct definition. @@ -20,12 +20,20 @@ pub struct UserReport { /// The event ID for which this user feedback is created. pub event_id: EventId, /// The user's name. - #[serde(default)] + #[serde(default, deserialize_with = "null_to_default")] pub name: String, /// The user's email address. - #[serde(default)] + #[serde(default, deserialize_with = "null_to_default")] pub email: String, /// Comments supplied by the user. - #[serde(default)] + #[serde(default, deserialize_with = "null_to_default")] pub comments: String, } + +fn null_to_default<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + let opt = Option::deserialize(deserializer)?; + Ok(opt.unwrap_or_else(Priority::lowest)) +} From 3ca3779a7582c4960c44c58d69f0ad3beb8acf8f Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Thu, 17 Dec 2020 16:33:36 +0100 Subject: [PATCH 8/8] slow down idiot --- relay-general/src/protocol/user_report.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/relay-general/src/protocol/user_report.rs b/relay-general/src/protocol/user_report.rs index 32e3877bfb..3234ce78d5 100644 --- a/relay-general/src/protocol/user_report.rs +++ b/relay-general/src/protocol/user_report.rs @@ -1,6 +1,6 @@ use crate::protocol::EventId; -use serde::{Deserialize, Serialize}; +use serde::{de::DeserializeOwned, Deserialize, Deserializer, Serialize}; /// User feedback for an event as sent by the client to the userfeedback/userreport endpoint. /// @@ -30,10 +30,11 @@ pub struct UserReport { pub comments: String, } -fn null_to_default<'de, D>(deserializer: D) -> Result +fn null_to_default<'de, D, V>(deserializer: D) -> Result where D: Deserializer<'de>, + V: Default + DeserializeOwned, { let opt = Option::deserialize(deserializer)?; - Ok(opt.unwrap_or_else(Priority::lowest)) + Ok(opt.unwrap_or_default()) }