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.

Author: relaxolotl

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,15 +229,15 @@ 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::<Dsn>().unwrap();
         assert_eq!(dsn.scheme(), Scheme::Https);
         assert_eq!(dsn.public_key(), "username");
         assert_eq!(dsn.secret_key(), Some("password"));
         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::<Dsn>().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::<Dsn>().unwrap();
+        assert_eq!(dsn.project_id(), &ProjectId::new("pid"));
+        assert_eq!(dsn.path(), "/pathone/pathtwo/");
     }
 
     #[test]

sentry-types/src/project_id.rs

@@ -1,4 +1,3 @@
-use std::convert::TryFrom;
 use std::fmt;
 use std::str::FromStr;
 
@@ -8,88 +7,45 @@ 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<Self, Self::Error> {
-                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;
 
     fn from_str(s: &str) -> Result<ProjectId, ParseProjectIdError> {
         if s.is_empty() {
             return Err(ParseProjectIdError::EmptyValue);
         }
-
-        match s.parse::<u64>() {
-            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::<ProjectId>(),
-            Err(ParseProjectIdError::InvalidValue)
-        );
+        assert_eq!(id, ProjectId::new("42"));
+        assert_eq!("42xxx".parse::<ProjectId>().unwrap().value(), "42xxx");
         assert_eq!(
             "".parse::<ProjectId>(),
             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::<ProjectId>("42").unwrap(),
-            ProjectId::new(42)
+            serde_json::to_string(&ProjectId::new("42")).unwrap(),
+            "\"42\""
+        );
+        assert_eq!(
+            serde_json::from_str::<ProjectId>("\"42\"").unwrap(),
+            ProjectId::new("42")
         );
     }
 }

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://[email protected]/42");
+    let c: sentry::Client = sentry::Client::from_config("https://[email protected]/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://[email protected]/42",
+        "https://[email protected]/42%21",
         sentry::ClientOptions {
             release: Some("[email protected]".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(), &"[email protected]");
     }