feat: workspace filter query supported in backend #2232
Conversation
…e-template-search
| // functions. The fake database is a manual implementation, so it is possible | ||
| // we forget to delete functions that we remove. This unit test just ensures | ||
| // we remove the extra methods. | ||
| func TestExactMethods(t *testing.T) { |
There was a problem hiding this comment.
This helps find which methods are not required on the fake. Just good for keeping that struct clean.
coderd/workspaces.go
Outdated
| if !(filter.OwnerID == uuid.Nil || filter.OwnerID == apiKey.UserID) { | ||
| httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ | ||
| Message: "Cannot set both \"me\" in \"owner_name\" and use \"owner_id\".", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
I'm kinda hesitant to return 4xx errors in this route since it's powered by a filter. We can add support for validation error in the query bar on the dashboard, but right now we the behavior is we just take the first.
Could we just drop ownerID? I think we use owner name on the app everywhere anyways?
There was a problem hiding this comment.
I'm ok with dropping owner_id for now 👍
There was a problem hiding this comment.
I'm keeping it in the database filter because apiKey has userID, not their name. But I'll drop it from the query params.
If we don't allow type WorkspaceFilter struct {
// Owner can be "me" or a username
Owner string `json:"owner,omitempty"`
// Template is a template name
Template string `json:"template,omitempty"`
// Name will return partial matches
Name string `json:"name,omitempty"`
}I'll need to add a function to convert these fields into the Do you prefer to do: client.Workspaces(ctx, codersdk.WorkspaceFilter{
FilterQuery: fmt.Sprintf("owner:%s name:%s", codersdk.Me, workspaceName),
}
// OR
client.Workspaces(ctx, codersdk.WorkspaceFilter{
Owner: codersdk.Me,
Name: workspaceName,
}To me, it feels awkward to use the |
|
Also github issues does support But it seems some filter strings are not their own query params. |
coderd/workspacesearch.go
Outdated
|
|
||
| // WorkspaceSearchQuery takes a query string and breaks it into its queryparams | ||
| // as a set of key=value. | ||
| func WorkspaceSearchQuery(query string) (map[string]string, error) { |
There was a problem hiding this comment.
It seems odd that this would be exported from the coderd package. I'd personally prefer this be internal and tested from the endpoint itself to prevent any implementation mismatches. I'm open to thoughts here!
There was a problem hiding this comment.
nit: I noticed AuthorizeFilter is exposed too, and it'd be nice to make that internal as part of this PR.
There was a problem hiding this comment.
Similar comment for the Authorize function. It's exposed on the primary API struct, but it doesn't need to be from what it seems.
There was a problem hiding this comment.
I can unexport it, but I really much rather just test this pure function. Having to interpret what the results of this is based on the api feedback of the endpoint sound challenging. Testing this function as is is trivial.
- Unexport workspace search query function - move code into workspaces.go - use strings.SplitFields
|
@kylecarbs I'll just drop non |
| @@ -8,13 +8,33 @@ WHERE | |||
| LIMIT | |||
| 1; | |||
|
|
|||
| -- name: GetTemplatesByIDs :many | |||
| -- name: GetTemplatesWithFilter :many | |||
There was a problem hiding this comment.
nit: This is really just GetTemplates (and that's how we use it from our API)
Continue of #2134
What this does
Adds workspace filter query support in backend. Adds template_name query param support as well.
Extra
GetTemplatesWithFilterrather than multiple sql queries with different paramsQueryParamParserto handle parsing query params better.Future Work
Make the FE pass the query string as is to the BE. @f0ssel ?