ref: Change the internal representation of project IDs from u64s to strings#452
ref: Change the internal representation of project IDs from u64s to strings#452relaxolotl merged 3 commits intomasterfrom
Conversation
| /// Returns the project_id | ||
| pub fn project_id(&self) -> ProjectId { | ||
| self.project_id | ||
| pub fn project_id(&self) -> &ProjectId { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I’m wondering if it might be simpler to just return &str here, and completely remove the ProjectId type.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙃
Codecov Report
@@ Coverage Diff @@
## master #452 +/- ##
==========================================
+ Coverage 80.15% 80.25% +0.10%
==========================================
Files 73 73
Lines 8355 8353 -2
==========================================
+ Hits 6697 6704 +7
+ Misses 1658 1649 -9 |
|
Updated to reflect the original expectations that motivated this change (zero validation of the contents of the project ID). Reasonable checks have been kept (i.e. emptiness) but all other validation has been stripped away. All of the conversions from and to various integer values have been removed as the project ID should no longer be considered a newtype that thinly wraps around a string. This should be ready for another review. |
Swatinem
left a comment
Swatinem
left a comment
There was a problem hiding this comment.
lgtm. You can land this as-is, or consider removing the type completely. TBH, the Rust SDK went overboard with strict typing in some places, leading to a huge and unwieldy API surface, so it might as well be a good idea to cut back on that.
| }; | ||
| } | ||
|
|
||
| impl_from!(u8); |
There was a problem hiding this comment.
IMO, we could keep the Try/From impls, as they would just stringify the given int. But its also fine to just remove it.
There was a problem hiding this comment.
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?
| /// Returns the project_id | ||
| pub fn project_id(&self) -> ProjectId { | ||
| self.project_id | ||
| pub fn project_id(&self) -> &ProjectId { |
There was a problem hiding this comment.
I’m wondering if it might be simpler to just return &str here, and completely remove the ProjectId type.
4 participants
Does what it says on the tin. AFAIK this is because project IDs aren't guaranteed to be numbers forever, and there's a shared goal between multiple SDKs to switch their types over from some integer-y representation to a string representation, in preparation for a possible switch to a more sophisticated ID system.
Effort has been put in to ensure that most of the API remains the same, however there have been some changes to signatures whose original type would be expensive to retain given the unclonability of strings.
jira breadcrumb: NATIVE-503