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');`
| groupACL := orig.GroupACL | ||
| if groupACL == nil { | ||
| groupACL = database.WorkspaceACL{} | ||
| } |
There was a problem hiding this comment.
@Emyrk the new version of workspaces_expanded revealed an issue in a (test) helper function update merged yesterday (by yours truly 🤕).
This hunk fixes the issue so that the tests using the helper don't break when passing in only one from the UserACL/GroupACL pair.
With the old workspace_expanded view the issue (JSON null being saved in DB instead of the default {}) wasn't surfacing, but with the new version of the view, fetching results in this error:
pq: cannot call jsonb_each on a non-object
As I said, this commit fixes the issue for tests, but it begs a bigger question: should we consider adopting 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');
so that JSON nulls never get saved by faulty Go code?
There was a problem hiding this comment.
Ah, do we need a db migration and constraint to fix it?
That would be ok
There was a problem hiding this comment.
If that's OK, I'll submit a separate PR when I have a little more bandwidth.
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
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