[Breaking] Reorg the user-group apis with a explicit google group spec field. #33
Conversation
| // The type of the group. | ||
| // Currently, the only supported type is 'google-group'. | ||
| // The type is immutable. Once set during creation, it cannot be changed. | ||
| string type = 3; | ||
| // The specification of the google group. | ||
| // This field is required when the group type is 'google-group'. | ||
| GoogleGroupSpec google_group = 4; |
There was a problem hiding this comment.
Can't the presence of google_group be enough to not need type? Would you consider a oneof? Same for the filter on the request.
There was a problem hiding this comment.
I think I will sight the discussion here:
#23 (comment)
We decided not to use one-off but rely on presence or absence of fields instead of a explicit type.
Let me remove the type field.
| // Filter groups by the group type - optional. | ||
| // Possible values: google-group | ||
| string group_type = 5; | ||
| // Filter groups by the google group email - optional. | ||
| // Only applicable when group_type filter is set to google group-type. | ||
| string google_group_email_address = 6; |
There was a problem hiding this comment.
Can you do the same thing here as you did in the spec? Specifically can you have a GoogleGroupFilter that can contain filterable fields and even just its empty presence is like group_type? My concern is that you're going to add a new top-level field for every field inside every spec type, and they only work if they are set as the same time as something else which is confusing.
There was a problem hiding this comment.
Just updated and removed the type field.
and even just its empty presence is like group_type
Proto3 does not distinguish between empty and unset scalars.
protocolbuffers/protobuf#1606 (comment)
I don't think we can implement a type filter with just a googleGroupFilter.
There was a problem hiding this comment.
Proto3 does not distinguish between empty and unset scalars.
That is unrelated to my concern now about top-level fields. Are you going to add a new top-level field for every field you want to filter on each group type? My suggestion is just making a a child object for all google group filter fields (and another for the next one) instead of moving them all top level. Same reason you have UserGroupSpec.google_group.email_address instead of UserGroupSpec.google_group_email_address, you should have GetUserGroupsRequest.google_group.email_address instead of GetUserGroupsRequest.google_group_email_address.
There was a problem hiding this comment.
Got it, makes sense. Added a child object as suggested.
| @@ -349,6 +349,11 @@ message DeleteApiKeyResponse { | |||
| temporal.api.cloud.operation.v1.AsyncOperation async_operation = 1; | |||
| } | |||
|
|
|||
| message GoogleGroupGetUserGroupsFilter { | |||
There was a problem hiding this comment.
Not the biggest fan of this name (I'd just have made a nested GoogleGroupFilter in GetUserGroupsRequest) but not that big of a deal
| // Filter groups by the display name - optional. | ||
| string display_name = 4; | ||
|
|
||
| message GoogleGroupFilter { |
There was a problem hiding this comment.
Personally I think it's best to put all nested messages at the bottom of the parent, though some proto authors put them at the top, but I don't think it's very common to intersperse between fields. (this is pedantic and doesn't affect my approval of the PR)
This change is backward incompatible. But we control who and how its being used. Should be safe to merge.