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.

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

Update Commit.id to not validate as a UUID #45

Closed
wants to merge 2 commits into from

Conversation

saquibmian
Copy link
Contributor

Commit.id <=> RepositoryCommit.name, which is a dashless UUID.

This also implies that ResourceRef.value.id is not a UUID.

TODO:

  • This is now inconsistent across the API surface. Should we drop the UUID validation on all IDs? Ideally our IDs are opaque to users and we are not expressing their shape in the API. Alternatively, we add Commit.name back and Commit.id <=> RepositoryCommit.id and Commit.name <=> RepositoryCommit.name.

`Commit.id` <=> `RepositoryCommit.name`, which is a dashless UUID.

This also implies that `ResourceRef.value.id` is not a UUID.

TODO:
- [ ] Should we drop the UUID validation on all IDs? Ideally our IDs are opaque to users and we are not expressing their shape in the API.
@saquibmian saquibmian requested a review from bufdev as a code owner December 4, 2023 17:23
@saquibmian saquibmian changed the title Commit.id is not a UUID Update Commit.id to not validate as a UUID Dec 4, 2023
@nicksnyder
Copy link
Member

The inconsistency between some IDs are UUIDs and other aren’t is a bit unfortunate.

I think we should leverage Protovalidate for basic sanity checking of input as much as possible, so I don't think we should remove (buf.validate.field).string.uuid = true from id fields where it is valid.

For fields that can't use the build-in (buf.validate.field).string.uuid = true rule, we shouldn't just remove it, we should replace it with actual validation. It sounds like they are in fact UUIDs without the dashes, so let's validate that.

@saquibmian
Copy link
Contributor Author

It sounds like they are in fact UUIDs without the dashes, so let's validate that.

Are we sure we want to go down that path? We are exposing the shape of our IDs, and we cannot change this later.

In the meantime, I'll update the validation rules to validate a dashless UUID.

@@ -95,7 +95,7 @@ message ResourceRef {
oneof value {
option (buf.validate.oneof).required = true;
// The id of the resource.
string id = 1 [(buf.validate.field).string.uuid = true];
Copy link
Member

Choose a reason for hiding this comment

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

I assume we could write a CEL expression that essentially validates this field is either a UUID or whatever the validation is for a commit id (sounds like it is a UUID without dashes).

@nicksnyder
Copy link
Member

Are we sure we want to go down that path? We are exposing the shape of our IDs, and we cannot change this later.

This is an interesting philosophical question. My two arguments in favor are:

  1. In practice, it is nice to be able to rely on protovalidate to validate basic things like this.
  2. In theory, even if we remove these validations from the schema, Hyrum's law will apply (people will see that in practice we are using UUIDs everywhere).

Therefore might as well put it in the schema and accept that if we are compelled to change this later it would be a breaking change in that sense.

@nicksnyder
Copy link
Member

Discussed in person. Consistently using UUIDs is important enough that we should first consider what work we would need to do to make the implementation consistent so commits use UUIDs.

@saquibmian
Copy link
Contributor Author

Closing this PR, the BSR will take care to translate name to id and back as needed. We discussed this as a team and would like to prioritize the best API possible. Decision doc here.

@saquibmian saquibmian closed this Dec 6, 2023
@saquibmian saquibmian deleted the smian/commit-id-not-uuid branch December 6, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants