From 62f7b8e7cd3cf812e2cd9054cc6d89c3c1992307 Mon Sep 17 00:00:00 2001 From: relaxolotl <5597345+relaxolotl@users.noreply.github.com> Date: Wed, 30 Mar 2022 21:07:10 -0400 Subject: [PATCH] ref: Change the internal representation of project IDs from u64s to strings (#452) Project IDs aren't guaranteed to be numbers forever, and shouldn't be treated as such any more, to be forward-compatible with whatever new identifier format that does get used in the future for project IDs. They're now treated as opaque strings internally, and are subjected to minimal validation due to their opacity. --- sentry-types/src/dsn.rs | 21 ++++++--- sentry-types/src/project_id.rs | 86 +++++++++------------------------- sentry/tests/test_client.rs | 8 ++-- 3 files changed, 40 insertions(+), 75 deletions(-) diff --git a/sentry-types/src/dsn.rs b/sentry-types/src/dsn.rs index 058d0ecc..67806865 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 } } @@ -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 28aa931e..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,76 +7,37 @@ 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, } /// 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)] +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 { - Self(id) + pub fn new(id: &str) -> Self { + Self(id.to_string()) } - /// Returns the numeric value of this project id. + /// Returns the string representation of the project ID. #[inline] - pub fn value(self) -> u64 { - self.0 + pub fn value(&self) -> &str { + self.0.as_ref() } } impl fmt::Display for ProjectId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.value()) + write!(f, "{}", self.0) } } -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; @@ -85,11 +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), - } + Ok(ProjectId::new(s)) } } @@ -100,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 984317c4..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(), 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(), 42); + assert_eq!(dsn.project_id().value(), "42%21"); assert_eq!(&c.options().release.as_ref().unwrap(), &"foo@1.0"); }