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 1 commit
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
6 changes: 3 additions & 3 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 @@ -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());
}

Expand Down
28 changes: 21 additions & 7 deletions sentry-types/src/project_id.rs
View file
Open in desktop
Expand Up @@ -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")]
relaxolotl marked this conversation as resolved.
Show resolved Hide resolved
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<u64> {
self.0.parse::<u64>().ok()
}
}

impl fmt::Display for ProjectId {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.value())
write!(f, "{}", self.0)
}
}

Expand Down Expand Up @@ -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<ProjectId> for u64 {
fn from(pid: ProjectId) -> Self {
match pid.value() {
Some(val) => val,
None => u64::MAX,
}
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
4 changes: 2 additions & 2 deletions sentry/tests/test_client.rs
View file
Open in desktop
Expand Up @@ -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((
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(), Some(42));
assert_eq!(&c.options().release.as_ref().unwrap(), &"foo@1.0");
}

Expand Down