From 61ae594d88593cf8dcbca89b76883c9bd7312221 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Mon, 28 Mar 2022 21:21:51 -0400 Subject: [PATCH 1/2] ref: Change the internal representation of project IDs from u64s to strings --- sentry-types/src/dsn.rs | 6 +++--- sentry-types/src/project_id.rs | 28 +++++++++++++++++++++------- sentry/tests/test_client.rs | 4 ++-- 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/sentry-types/src/dsn.rs b/sentry-types/src/dsn.rs index 058d0ecc..70ca6acb 100644 --- a/sentry-types/src/dsn.rs +++ b/sentry-types/src/dsn.rs @@ -140,8 +140,8 @@ impl Dsn { } /// Returns the project_id - pub fn project_id(&self) -> ProjectId { - self.project_id + pub fn project_id(&self) -> &ProjectId { + &self.project_id } } @@ -237,7 +237,7 @@ mod test { assert_eq!(dsn.host(), "domain"); assert_eq!(dsn.port(), 8888); assert_eq!(dsn.path(), "/"); - assert_eq!(dsn.project_id(), ProjectId::new(23)); + assert_eq!(dsn.project_id(), &ProjectId::new(23)); assert_eq!(url, dsn.to_string()); } diff --git a/sentry-types/src/project_id.rs b/sentry-types/src/project_id.rs index 28aa931e..cc28314f 100644 --- a/sentry-types/src/project_id.rs +++ b/sentry-types/src/project_id.rs @@ -17,26 +17,28 @@ pub enum ParseProjectIdError { } /// Represents a project ID. -#[derive(Copy, Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash, Deserialize, Serialize)] -pub struct ProjectId(u64); +#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash, Deserialize, Serialize)] +#[serde(into = "u64", from = "u64")] +pub struct ProjectId(String); impl ProjectId { /// Creates a new project ID from its numeric value. #[inline] pub fn new(id: u64) -> Self { - Self(id) + Self(id.to_string()) } - /// Returns the numeric value of this project id. + /// Returns the numeric value of this project id. None is returned if a + /// valid could not be parsed from the project id. #[inline] - pub fn value(self) -> u64 { - self.0 + pub fn value(&self) -> Option { + self.0.parse::().ok() } } impl fmt::Display for ProjectId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.value()) + write!(f, "{}", self.0) } } @@ -93,6 +95,18 @@ impl FromStr for ProjectId { } } +// Combined with the serde into/from annotation, this allows the project ID to +// continue being serialized and deserialized as a u64 until other parts of +// sentry add in full support for project strings. +impl From for u64 { + fn from(pid: ProjectId) -> Self { + match pid.value() { + Some(val) => val, + None => u64::MAX, + } + } +} + #[cfg(test)] mod test { use super::*; diff --git a/sentry/tests/test_client.rs b/sentry/tests/test_client.rs index 984317c4..da122e39 100644 --- a/sentry/tests/test_client.rs +++ b/sentry/tests/test_client.rs @@ -11,7 +11,7 @@ fn test_into_client() { assert_eq!(dsn.public_key(), "public"); assert_eq!(dsn.host(), "example.com"); assert_eq!(dsn.scheme(), sentry::types::Scheme::Https); - assert_eq!(dsn.project_id().value(), 42); + assert_eq!(dsn.project_id().value(), Some(42)); } let c: sentry::Client = sentry::Client::from_config(( @@ -26,7 +26,7 @@ fn test_into_client() { assert_eq!(dsn.public_key(), "public"); assert_eq!(dsn.host(), "example.com"); assert_eq!(dsn.scheme(), sentry::types::Scheme::Https); - assert_eq!(dsn.project_id().value(), 42); + assert_eq!(dsn.project_id().value(), Some(42)); assert_eq!(&c.options().release.as_ref().unwrap(), &"foo@1.0"); } From 8e62a439d4ded0478cd8e3a5e538839210cafa86 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 29 Mar 2022 20:07:40 -0400 Subject: [PATCH 2/2] remove all but emptiness checks on project IDs --- sentry-types/src/dsn.rs | 17 +++++-- sentry-types/src/project_id.rs | 92 +++++++--------------------------- sentry/tests/test_client.rs | 8 +-- 3 files changed, 34 insertions(+), 83 deletions(-) diff --git a/sentry-types/src/dsn.rs b/sentry-types/src/dsn.rs index 70ca6acb..67806865 100644 --- a/sentry-types/src/dsn.rs +++ b/sentry-types/src/dsn.rs @@ -229,7 +229,7 @@ mod test { #[test] fn test_dsn_parsing() { - let url = "https://username:password@domain:8888/23"; + let url = "https://username:password@domain:8888/23%21"; let dsn = url.parse::().unwrap(); assert_eq!(dsn.scheme(), Scheme::Https); assert_eq!(dsn.public_key(), "username"); @@ -237,7 +237,7 @@ mod test { assert_eq!(dsn.host(), "domain"); assert_eq!(dsn.port(), 8888); assert_eq!(dsn.path(), "/"); - assert_eq!(dsn.project_id(), &ProjectId::new(23)); + assert_eq!(dsn.project_id(), &ProjectId::new("23%21")); assert_eq!(url, dsn.to_string()); } @@ -303,9 +303,18 @@ mod test { } #[test] - #[should_panic(expected = "InvalidProjectId")] + fn test_dsn_non_integer_project_id() { + let url = "https://username:password@domain:8888/abc123youandme%21%21"; + let dsn = url.parse::().unwrap(); + assert_eq!(dsn.project_id(), &ProjectId::new("abc123youandme%21%21")); + } + + #[test] fn test_dsn_more_than_one_non_integer_path() { - Dsn::from_str("http://username:@domain:8888/path/path2").unwrap(); + let url = "http://username:@domain:8888/pathone/pathtwo/pid"; + let dsn = url.parse::().unwrap(); + assert_eq!(dsn.project_id(), &ProjectId::new("pid")); + assert_eq!(dsn.path(), "/pathone/pathtwo/"); } #[test] diff --git a/sentry-types/src/project_id.rs b/sentry-types/src/project_id.rs index cc28314f..f55b1282 100644 --- a/sentry-types/src/project_id.rs +++ b/sentry-types/src/project_id.rs @@ -1,4 +1,3 @@ -use std::convert::TryFrom; use std::fmt; use std::str::FromStr; @@ -8,9 +7,6 @@ use thiserror::Error; /// Raised if a project ID cannot be parsed from a string. #[derive(Debug, Error, PartialEq, Eq, PartialOrd, Ord)] pub enum ParseProjectIdError { - /// Raised if the value is not an integer in the supported range. - #[error("invalid value for project id")] - InvalidValue, /// Raised if an empty value is parsed. #[error("empty or missing project id")] EmptyValue, @@ -18,21 +14,21 @@ pub enum ParseProjectIdError { /// Represents a project ID. #[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash, Deserialize, Serialize)] -#[serde(into = "u64", from = "u64")] pub struct ProjectId(String); impl ProjectId { - /// Creates a new project ID from its numeric value. + /// Creates a new project ID from its string representation. + /// This assumes that the string is already well-formed and URL + /// encoded/decoded. #[inline] - pub fn new(id: u64) -> Self { + pub fn new(id: &str) -> Self { Self(id.to_string()) } - /// Returns the numeric value of this project id. None is returned if a - /// valid could not be parsed from the project id. + /// Returns the string representation of the project ID. #[inline] - pub fn value(&self) -> Option { - self.0.parse::().ok() + pub fn value(&self) -> &str { + self.0.as_ref() } } @@ -42,44 +38,6 @@ impl fmt::Display for ProjectId { } } -macro_rules! impl_from { - ($ty:ty) => { - impl From<$ty> for ProjectId { - #[inline] - fn from(val: $ty) -> Self { - Self::new(val as u64) - } - } - }; -} - -impl_from!(u8); -impl_from!(u16); -impl_from!(u32); -impl_from!(u64); - -macro_rules! impl_try_from { - ($ty:ty) => { - impl TryFrom<$ty> for ProjectId { - type Error = ParseProjectIdError; - - #[inline] - fn try_from(val: $ty) -> Result { - match u64::try_from(val) { - Ok(id) => Ok(Self::new(id)), - Err(_) => Err(ParseProjectIdError::InvalidValue), - } - } - } - }; -} - -impl_try_from!(usize); -impl_try_from!(i8); -impl_try_from!(i16); -impl_try_from!(i32); -impl_try_from!(i64); - impl FromStr for ProjectId { type Err = ParseProjectIdError; @@ -87,23 +45,7 @@ impl FromStr for ProjectId { if s.is_empty() { return Err(ParseProjectIdError::EmptyValue); } - - match s.parse::() { - Ok(val) => Ok(ProjectId::new(val)), - Err(_) => Err(ParseProjectIdError::InvalidValue), - } - } -} - -// Combined with the serde into/from annotation, this allows the project ID to -// continue being serialized and deserialized as a u64 until other parts of -// sentry add in full support for project strings. -impl From for u64 { - fn from(pid: ProjectId) -> Self { - match pid.value() { - Some(val) => val, - None => u64::MAX, - } + Ok(ProjectId::new(s)) } } @@ -114,21 +56,21 @@ mod test { #[test] fn test_basic_api() { let id: ProjectId = "42".parse().unwrap(); - assert_eq!(id, ProjectId::new(42)); - assert_eq!( - "42xxx".parse::(), - Err(ParseProjectIdError::InvalidValue) - ); + assert_eq!(id, ProjectId::new("42")); + assert_eq!("42xxx".parse::().unwrap().value(), "42xxx"); assert_eq!( "".parse::(), Err(ParseProjectIdError::EmptyValue) ); - assert_eq!(ProjectId::new(42).to_string(), "42"); + assert_eq!(ProjectId::new("42").to_string(), "42"); - assert_eq!(serde_json::to_string(&ProjectId::new(42)).unwrap(), "42"); assert_eq!( - serde_json::from_str::("42").unwrap(), - ProjectId::new(42) + serde_json::to_string(&ProjectId::new("42")).unwrap(), + "\"42\"" + ); + assert_eq!( + serde_json::from_str::("\"42\"").unwrap(), + ProjectId::new("42") ); } } diff --git a/sentry/tests/test_client.rs b/sentry/tests/test_client.rs index da122e39..0687fb8f 100644 --- a/sentry/tests/test_client.rs +++ b/sentry/tests/test_client.rs @@ -5,17 +5,17 @@ use std::sync::Arc; #[test] fn test_into_client() { - let c: sentry::Client = sentry::Client::from_config("https://public@example.com/42"); + let c: sentry::Client = sentry::Client::from_config("https://public@example.com/42%21"); { let dsn = c.dsn().unwrap(); assert_eq!(dsn.public_key(), "public"); assert_eq!(dsn.host(), "example.com"); assert_eq!(dsn.scheme(), sentry::types::Scheme::Https); - assert_eq!(dsn.project_id().value(), Some(42)); + assert_eq!(dsn.project_id().value(), "42%21"); } let c: sentry::Client = sentry::Client::from_config(( - "https://public@example.com/42", + "https://public@example.com/42%21", sentry::ClientOptions { release: Some("foo@1.0".into()), ..Default::default() @@ -26,7 +26,7 @@ fn test_into_client() { assert_eq!(dsn.public_key(), "public"); assert_eq!(dsn.host(), "example.com"); assert_eq!(dsn.scheme(), sentry::types::Scheme::Https); - assert_eq!(dsn.project_id().value(), Some(42)); + assert_eq!(dsn.project_id().value(), "42%21"); assert_eq!(&c.options().release.as_ref().unwrap(), &"foo@1.0"); }