feat: add sharing info to /workspaces endpoint #21049
Conversation
f0eda46 to
9e33549
Compare
Emyrk
left a comment
There was a problem hiding this comment.
Do you think we could do this all in the postgres view?
We have the view
coder/coderd/database/dump.sql
Line 2911 in 9e33549
I know the ACLs are jsonb. If we add this to the view though, that would save us work downstream.
There are api endpoints in this PR that return workspaces without the shared info. I'd rather us be consistent what fields are returned when fetching a workspace.
9e33549 to
77ea00b
Compare
aslilac
left a comment
There was a problem hiding this comment.
the go code all looks largely fine but I'd love to have a second pair of eyes on the sql view because it's quite involved.
coderd/database/migrations/000403_workspaces_expanded_acl_actor_info.up.sql
Outdated
Show resolved
Hide resolved
Also fix bug in the deserialization code.
…paces_expanded view Instead of N+1 in coderd.
…_with: []` in response if experiment was enabled
a73d47f to
3777de0
Compare
…r/group acl in workspace test table
Because such nulls break tests fetching the new workspace_expanded
view, with the following error:
`cannot call jsonb_each on a non-object`
(null is a non-object, that's why we use `{}`as default).
This begs the question if we want to adopt check constraints for JSONB
columns across our schema, e.g.:
`ALTER TABLE workspaces
ADD CONSTRAINT group_acl_is_object CHECK (jsonb_typeof(group_acl) = 'object');`
aslilac
left a comment
There was a problem hiding this comment.
sorry, noticed something on a second look that I think we should address
| return json.Marshal(t) | ||
| } | ||
|
|
||
| type WorkspaceACLEntry struct { |
There was a problem hiding this comment.
this type is used by the UpdateWorkspaceACLByID query. I don't think we want to add these extra fields here, because they're not valid to set when calling that query.
in the RFC this used a type called SharedWorkspaceActor https://www.notion.so/coderhq/Shared-Workspaces-214d579be592803697cdef7a662d35b5?source=copy_link#224d579be59280688617ed6001e46377
| Role WorkspaceRole `json:"role" enums:"admin,use"` | ||
| } | ||
|
|
||
| type SharedWorkspaceActor struct { |
There was a problem hiding this comment.
ah, yeah, we do define that here still. can we avoid polluting the db type and either convert to this earlier or use a different type as the intermediate?
There was a problem hiding this comment.
Good point, thanks! Pushed an update that reads the ACL actors' display info as separate columns on the workspaces_expanded view.
The query behind this version of the view is slightly (up to 20%) faster, likely because it doesn't need to parse saved ACL JSON (just extracts the keys).
The downside is more data on the wire and extra boilerplate.
629e5b4 to
73a619c
Compare
…e acl actors display info
73a619c to
a50b597
Compare
Emyrk
left a comment
There was a problem hiding this comment.
The split into 2 columns is fine. Not ideal, but if it's more efficient, than that is a win.
| -- Workspace ACL actors' display info | ||
| COALESCE(( | ||
| SELECT jsonb_object_agg( | ||
| acl.key, | ||
| jsonb_build_object( | ||
| 'name', COALESCE(g.name, ''), | ||
| 'avatar_url', COALESCE(g.avatar_url, '') | ||
| ) | ||
| ) | ||
| FROM jsonb_each(workspaces.group_acl) AS acl | ||
| LEFT JOIN groups g ON g.id = acl.key::uuid | ||
| ), '{}'::jsonb) AS group_acl_display_info, | ||
| COALESCE(( | ||
| SELECT jsonb_object_agg( | ||
| acl.key, | ||
| jsonb_build_object( | ||
| 'name', COALESCE(vu.name, ''), | ||
| 'avatar_url', COALESCE(vu.avatar_url, '') | ||
| ) | ||
| ) | ||
| FROM jsonb_each(workspaces.user_acl) AS acl | ||
| LEFT JOIN visible_users vu ON vu.id = acl.key::uuid | ||
| ), '{}'::jsonb) AS user_acl_display_info |
There was a problem hiding this comment.
Interesting this saved some time. Although I'm sus with the planner now 😄
| type WorkspaceACLDisplayInfo map[string]struct { | ||
| Name string `json:"name"` | ||
| AvatarURL string `json:"avatar_url"` | ||
| } |
There was a problem hiding this comment.
It is unfortunate we cannot type the key as a uuid.UUID. The rbac stuff does not enforce that type though, so string is safer.
Can we just add a comment to that effect?
Similar to #19375, this one uses system permissions for fetching actual user and group data.
An implementation that doesn't do that is also buried in the commits, so it should be easy to switch to if needed.
closes: coder/internal#858