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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ref: Change the internal representation of project IDs from u64s to strings #452

Merged
merged 3 commits into from Mar 31, 2022
Merged

ref: Change the internal representation of project IDs from u64s to strings #452

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61ae594
ref: Change the internal representation of project IDs from u64s to s…
relaxolotl Mar 29, 2022
8e62a43
remove all but emptiness checks on project IDs
relaxolotl Mar 30, 2022
605b71b
Merge branch 'master' into ref/string-project-ids
relaxolotl Mar 31, 2022
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
21 changes: 15 additions & 6 deletions sentry-types/src/dsn.rs
View file
Open in desktop
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted in the description, this signature change may impact some users. I'm actually not too sure if too many folks actually make active use of this value seeing as it isn't required as a parameter or anything like that in the rest of the SDK's API. If I had to guess, this change is probably relatively low-impact but I could be wrong about this.

Copy link
Member

Choose a reason for hiding this comment

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

I’m wondering if it might be simpler to just return &str here, and completely remove the ProjectId type.

Copy link
Contributor

Choose a reason for hiding this comment

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

leaving it as an explicit type makes sure it's opaque and user's can't be tempted to try and parse things out of it, so I'd keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was thinking about having this return &str initially and as suggested, to get rid of the type completely.

if we were to switch to something like a UUID or otherwise well-structured unique identifier, we could add back in validation to this type that we'd removed. in an ideal situation this would mean not having to rewrite and convert all of the project ID code back to using a newtype.

even if we're unable to reuse the code at all, i think it would make it easier for us to easily find and replace instances of project IDs with their replacements in the future. as @flub has mentioned, having a separate type makes it harder for users to do too much with the ID, but i suppose it's not like we're trying particularly hard to prevent them from doing so either by exposing ProjectId::value.

still, i'm tempted to keep the type because it helps explicitly express an opinion about what ProjectIds are and how to work with them. we'll see if this burns us sometime in the future 🙃

&self.project_id
}
}

Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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]
Expand Down
86 changes: 21 additions & 65 deletions sentry-types/src/project_id.rs
View file
Open in desktop
@@ -1,4 +1,3 @@
use std::convert::TryFrom;
use std::fmt;
use std::str::FromStr;

Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we could keep the Try/From impls, as they would just stringify the given int. But its also fine to just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll follow up on this in a different place, but i'm curious to hear the argument for keeping the Try/From impls. i've got an inkling of an idea, but it could just be completely wrong.

i'm open to adding these back in a separate PR though. right now it feels like keeping impls just for integer values looks a little weird: why are there Try/From impls for integers but parse for &strs? why aren't there try_from impls for other primitives, why specifically all of the ints?

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

Expand All @@ -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")
);
}
}
8 changes: 4 additions & 4 deletions sentry/tests/test_client.rs
View file
Open in desktop
Expand Up @@ -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()
Expand All @@ -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");
}

Expand Down