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

Better represent/document uniqueness of name on User and Organization #2

Closed
nicksnyder opened this issue Oct 26, 2023 · 0 comments · Fixed by #7
Closed

Better represent/document uniqueness of name on User and Organization #2

nicksnyder opened this issue Oct 26, 2023 · 0 comments · Fixed by #7

Comments

@nicksnyder
Copy link
Member

nicksnyder commented Oct 26, 2023

I was looking at CreateUsers and wondering how it was idempotent because it wasn't obvious to me that the name field was unique.
https://github.com/bufbuild/registry-proto/blob/601ec088649be87ee6a683602fe1e6ecc587a665/buf/registry/owner/v1beta1/user_service.proto#L82-L89

I went to look at the definition of User and still wasn't obvious the name was intended to be unique since the field documentation didn't indicate that either. I did eventually find the docs on the User type itself, but those docs are actually not very close to the field documentation:
https://github.com/bufbuild/registry-proto/blob/601ec088649be87ee6a683602fe1e6ecc587a665/buf/registry/owner/v1beta1/user.proto#L23-L42

Furthermore, I kind of expect "id" like fields to be first in the definition of a type.

Similar situation for Organization.name too.

Here are some things that we could do to improve the situation for both User and Organization (not mutually exclusive, could to some or all of them).

Move the uniqueness documentation to the `name` field on `User` and `CreateUsersRequest.Value`
diff --git a/buf/registry/owner/v1beta1/user.proto b/buf/registry/owner/v1beta1/user.proto
index 366b31e..77c444a 100644
--- a/buf/registry/owner/v1beta1/user.proto
+++ b/buf/registry/owner/v1beta1/user.proto
@@ -21,8 +21,6 @@ import "buf/validate/validate.proto";
 import "google/protobuf/timestamp.proto";
 
 // A user on the BSR.
-//
-// A name uniquely identifies a User, however name is mutable.
 message User {
   option (buf.registry.priv.extension.v1beta1.message).response_only = true;
 
@@ -36,6 +34,7 @@ message User {
   // The last time the User was updated.
   google.protobuf.Timestamp update_time = 3 [(buf.validate.field).required = true];
   // The name of the User.
+  // A name uniquely identifies a User, however name is mutable.
   string name = 4 [
     (buf.validate.field).required = true,
     (buf.validate.field).string.max_len = 255
diff --git a/buf/registry/owner/v1beta1/user_service.proto b/buf/registry/owner/v1beta1/user_service.proto
index 969f28f..a964ab1 100644
--- a/buf/registry/owner/v1beta1/user_service.proto
+++ b/buf/registry/owner/v1beta1/user_service.proto
@@ -83,6 +83,7 @@ message CreateUsersRequest {
   // An individual request to create a User.
   message Value {
     // The name of the User.
+    // A name uniquely identifies a User, however name is mutable.
     string name = 1 [
       (buf.validate.field).required = true,
       (buf.validate.field).string.max_len = 255
Move `name` to be field 2 so it is next to `id`
diff --git a/buf/registry/owner/v1beta1/user.proto b/buf/registry/owner/v1beta1/user.proto
index 366b31e..1402edf 100644
--- a/buf/registry/owner/v1beta1/user.proto
+++ b/buf/registry/owner/v1beta1/user.proto
@@ -21,8 +21,6 @@ import "buf/validate/validate.proto";
 import "google/protobuf/timestamp.proto";
 
 // A user on the BSR.
-//
-// A name uniquely identifies a User, however name is mutable.
 message User {
   option (buf.registry.priv.extension.v1beta1.message).response_only = true;
 
@@ -31,15 +29,16 @@ message User {
     (buf.validate.field).required = true,
     (buf.validate.field).string.uuid = true
   ];
-  // The time the User was created.
-  google.protobuf.Timestamp create_time = 2 [(buf.validate.field).required = true];
-  // The last time the User was updated.
-  google.protobuf.Timestamp update_time = 3 [(buf.validate.field).required = true];
   // The name of the User.
-  string name = 4 [
+  // A name uniquely identifies a User, however name is mutable.
+  string name = 2 [
     (buf.validate.field).required = true,
     (buf.validate.field).string.max_len = 255
   ];
+  // The time the User was created.
+  google.protobuf.Timestamp create_time = 3 [(buf.validate.field).required = true];
+  // The last time the User was updated.
+  google.protobuf.Timestamp update_time = 4 [(buf.validate.field).required = true];
   // The type of the User.
   UserType type = 5 [
     (buf.validate.field).required = true,
Consider naming the field `username` or `handle` instead of `name`
  • Especially for a User which is like a person, it isn't natural to think of someone's name as being unique.
  • handle is potentially better because it works for organizations too (username would be a more awkward on an Organization).
nicksnyder added a commit that referenced this issue Nov 5, 2023
Document the uniqueness of the `name` field on the field itself.

Closes #2
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 a pull request may close this issue.

1 participant