mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
Drops the `tasks`, `task_workspace_apps` and `task_snapshots` tables, the `tasks_with_status` view and the `task_status` enum, and recreates `workspaces_expanded` without the `tasks` join and `task_id` column. Deletes `queries/tasks.sql` and every wrapper, fixture and test that existed only for those queries (dbauthz, dbmock, dbmetrics, dbfake, dbgen, querier_test), plus the `task_id` projection in `GetWorkspaceAgentAndWorkspaceByAuthToken`. **Breaking change:** migration 000594 deletes all task rows. The down migration restores the schema (tables, constraints, indexes, views), not the data. **Review changes:** the schema-only migration test was removed per review (`TestMigrate/UpDownUp` still exercises the down path), the test file no longer reorders existing tests, and the migration moved from 000593 to 000594. The `dump.sql` reorder is pg_dump dependency ordering: `tasks_with_status` pulled its four workspace tables ahead of it, and without the view they return to alphabetical positions. **Stack context:** Coder Tasks removal stack (CODAGT-642): #28743 frontend → #28788 CLI → #28789 API → #28790 provisioner → #29084 notifications → #29085 build reasons → **#29086 tables** → #28750 permissions. The last four PRs replace the single data-drop migration that used to live in #28750; each carries one concern and its own migration number. > Created by Xum (AI agent) on behalf of @ibetitsmike. ## Delivery record **Decision (2026-09-11 ~14:40Z): merge** through the stack merge endpoint, squash, pinned to head `f1d236a0c82f123b8a0d6878799c227ef9bd5776` (merge SHA appended below once landed). **Independent substantive reviews on this PR (3):** | # | Kind | Reviewed commit | Findings and disposition | Source | |---|---|---|---|---| | 1 | Codex code review (bot), PR opened | `24535f8410` | No findings, no threads | [summary](https://github.com/coder/coder/pull/29086#issuecomment-5583877627) | | 2 | Human review, spikecurtis (COMMENTED) | `24535f8410` | 4 findings: dump.sql reorder question (answered: pg_dump dependency hoisting by `tasks_with_status`); two moved tests (undone, the test file is no longer touched); schema-only migration test (deleted). All resolved. | [review](https://github.com/coder/coder/pull/29086#pullrequestreview-5142045164) | | 3 | Human review, spikecurtis (APPROVED) | `f1d236a0c8` (this head) | Approval on the exact final head | [review](https://github.com/coder/coder/pull/29086#pullrequestreview-5179353695) | The four COMMENTED reviews by @ibetitsmike on `f35688f40b` are thread-reply wrappers and are not counted. **Pre-split history relevant to this diff:** this layer's content lived in the monolithic #28750 until 2026-09-08, which received 10 completed Codex code reviews (2026-08-28 to 2026-09-01) and spikecurtis's CHANGES_REQUESTED review of 2026-09-04 that asked for the split. Counting those, the completed-review total relevant to this diff exceeds the six-review budget, so no further discretionary review or fresh final advisory was commissioned. The usable final recommendation for the final diff is review 3 above: the reviewer who authored every finding on this layer approved the exact head. **Exact-head checks:** `required` SUCCESS in [run 34580949353](https://github.com/coder/coder/actions/runs/34580949353/job/103205933212); rollup 15 success, 8 skipped, 1 failure. The failure is the non-required `Analyze PR for Documentation Updates Needed` job ([103204101137](https://github.com/coder/coder/actions/runs/34580949071/job/103204101137)), caused by the dormant `doc-check-pr-29086` review workspace (CODAGT-1025), not a code finding; GitHub flags only `required` as required for this PR. Merge-ref freshness: `refs/pull/29086/merge` has the exact head as second parent and `25af21b5d5d` (the #29085 squash) as first parent; main has since gained #29231, #28639 and #29218 (12 files, none overlapping this PR). The composed tree (`git merge-tree` of current main and this head) builds (`go build ./...`) and vets (`go vet` on `coderd`, `coderd/database/...`, `coderd/wsrelated/...`) cleanly. Layer diff parity with the previously pushed layer verified by numstat after both GitHub restacks. `gh stack push` did not run the local pre-push suite; CI is the validation evidence. **Deferred:** the dormant doc-check workspace (CODAGT-1025) is outside this PR. Next in the stack, #28750, stays blocked: CHANGES_REQUESTED, its approval and CI predate the restack, and its review budget is exhausted.
181 lines
6.8 KiB
Go
181 lines
6.8 KiB
Go
package database
|
|
|
|
import (
|
|
"reflect"
|
|
"regexp"
|
|
"slices"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/google/go-cmp/cmp"
|
|
"github.com/stretchr/testify/require"
|
|
|
|
"github.com/coder/coder/v2/codersdk"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
func TestIsAuthorizedQuery(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
query := `SELECT true;`
|
|
_, err := insertAuthorizedFilter(query, "")
|
|
require.ErrorContains(t, err, "does not contain authorized replace string", "ensure replace string")
|
|
}
|
|
|
|
// TestWorkspaceTableConvert verifies all workspace fields are converted
|
|
// when reducing a `Workspace` to a `WorkspaceTable`.
|
|
// This test is a guard rail to prevent developer oversight mistakes.
|
|
func TestWorkspaceTableConvert(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
staticRandoms := &testutil.Random{
|
|
String: func() string { return "foo" },
|
|
Bool: func() bool { return true },
|
|
Int: func() int64 { return 500 },
|
|
Uint: func() uint64 { return 126 },
|
|
Float: func() float64 { return 3.14 },
|
|
Complex: func() complex128 { return 6.24 },
|
|
Time: func() time.Time {
|
|
return time.Date(2020, 5, 2, 5, 19, 21, 30, time.UTC)
|
|
},
|
|
}
|
|
|
|
// This feels a bit janky, but it works.
|
|
// If you use 'PopulateStruct' to create 2 workspaces, using the same
|
|
// "random" values for each type. Then they should be identical.
|
|
//
|
|
// So if 'workspace.WorkspaceTable()' was missing any fields in its
|
|
// conversion, the comparison would fail.
|
|
|
|
var workspace Workspace
|
|
err := testutil.PopulateStruct(&workspace, staticRandoms)
|
|
require.NoError(t, err)
|
|
|
|
var subset WorkspaceTable
|
|
err = testutil.PopulateStruct(&subset, staticRandoms)
|
|
require.NoError(t, err)
|
|
|
|
require.Equal(t, workspace.WorkspaceTable(), subset,
|
|
"'workspace.WorkspaceTable()' is not missing at least 1 field when converting to 'WorkspaceTable'. "+
|
|
"To resolve this, go to the 'func (w Workspace) WorkspaceTable()' and ensure all fields are converted.")
|
|
}
|
|
|
|
// TestAuditLogsQueryConsistency ensures that GetAuditLogsOffset and CountAuditLogs
|
|
// have identical WHERE clauses to prevent filtering inconsistencies.
|
|
// This test is a guard rail to prevent developer oversight mistakes.
|
|
func TestAuditLogsQueryConsistency(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
getWhereClause := extractWhereClause(getAuditLogsOffset)
|
|
require.NotEmpty(t, getWhereClause, "failed to extract WHERE clause from GetAuditLogsOffset")
|
|
|
|
countWhereClause := extractWhereClause(countAuditLogs)
|
|
require.NotEmpty(t, countWhereClause, "failed to extract WHERE clause from CountAuditLogs")
|
|
|
|
// Compare the WHERE clauses
|
|
if diff := cmp.Diff(getWhereClause, countWhereClause); diff != "" {
|
|
t.Errorf("GetAuditLogsOffset and CountAuditLogs WHERE clauses must be identical to ensure consistent filtering.\nDiff:\n%s", diff)
|
|
}
|
|
}
|
|
|
|
// Same as TestAuditLogsQueryConsistency, but for connection logs.
|
|
func TestConnectionLogsQueryConsistency(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
getWhereClause := extractWhereClause(getConnectionLogsOffset)
|
|
require.NotEmpty(t, getWhereClause, "getConnectionLogsOffset query should have a WHERE clause")
|
|
|
|
countWhereClause := extractWhereClause(countConnectionLogs)
|
|
require.NotEmpty(t, countWhereClause, "countConnectionLogs query should have a WHERE clause")
|
|
|
|
require.Equal(t, getWhereClause, countWhereClause, "getConnectionLogsOffset and countConnectionLogs queries should have the same WHERE clause")
|
|
}
|
|
|
|
// TestFinalizeStaleChatDebugRows_TerminalStatusAlignment asserts that the
|
|
// NOT IN ('completed', 'error', 'interrupted') literals in the
|
|
// FinalizeStaleChatDebugRows SQL query match the terminal statuses
|
|
// defined by ChatDebugTerminalStatuses in codersdk. If a new terminal
|
|
// status is added to Go but not to the SQL, this test fails.
|
|
func TestFinalizeStaleChatDebugRows_TerminalStatusAlignment(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
// Extract all NOT IN (...) lists from the SQL constant.
|
|
re := regexp.MustCompile(`NOT IN\s*\(([^)]+)\)`)
|
|
matches := re.FindAllStringSubmatch(finalizeStaleChatDebugRows, -1)
|
|
require.NotEmpty(t, matches, "expected at least one NOT IN clause in finalizeStaleChatDebugRows")
|
|
|
|
// Parse the quoted status literals from each NOT IN clause.
|
|
literalRe := regexp.MustCompile(`'([^']+)'`)
|
|
goTerminal := codersdk.ChatDebugTerminalStatuses()
|
|
|
|
for _, match := range matches {
|
|
literals := literalRe.FindAllStringSubmatch(match[1], -1)
|
|
var sqlStatuses []string
|
|
for _, lit := range literals {
|
|
sqlStatuses = append(sqlStatuses, lit[1])
|
|
}
|
|
slices.Sort(sqlStatuses)
|
|
|
|
var goStatuses []string
|
|
for _, s := range goTerminal {
|
|
goStatuses = append(goStatuses, string(s))
|
|
}
|
|
slices.Sort(goStatuses)
|
|
|
|
require.Equal(t, goStatuses, sqlStatuses,
|
|
"terminal statuses in FinalizeStaleChatDebugRows SQL must match "+
|
|
"codersdk.ChatDebugTerminalStatuses(); update both when adding "+
|
|
"a new terminal status")
|
|
}
|
|
}
|
|
|
|
// TestInsertChatMessagesOrderContract guards the input-order guarantee that
|
|
// callers rely on when indexing the returned slice. A behavior test cannot:
|
|
// Postgres evaluates the id default in row order anyway, so a batch still looks
|
|
// ordered once the guarantee is removed.
|
|
func TestInsertChatMessagesOrderContract(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
require.Contains(t, insertChatMessages, "nextval('chat_messages_id_seq')",
|
|
"ids must be allocated explicitly so they can be correlated to input array position")
|
|
require.Contains(t, insertChatMessages, "ROW_NUMBER() OVER (ORDER BY id)",
|
|
"the k-th smallest allocated id must be assigned to input index k")
|
|
require.Regexp(t, `(?s)ORDER BY id\s*\z`, strings.TrimSpace(insertChatMessages),
|
|
"returned rows must be explicitly ordered by id rather than relying on RETURNING order")
|
|
|
|
// Every parallel input array must be read at the allocated ordinal. A column
|
|
// left on UNNEST would be positioned by the executor instead.
|
|
subscripted := regexp.MustCompile(`\)\[allocated\.ord\]`).FindAllString(insertChatMessages, -1)
|
|
require.Len(t, subscripted, reflect.TypeOf(InsertChatMessagesParams{}).NumField()-1,
|
|
"each InsertChatMessagesParams array field, all but ChatID, must be subscripted by allocated.ord")
|
|
}
|
|
|
|
// extractWhereClause extracts the WHERE clause from a SQL query string
|
|
func extractWhereClause(query string) string {
|
|
// Find WHERE and get everything after it
|
|
wherePattern := regexp.MustCompile(`(?is)WHERE\s+(.*)`)
|
|
whereMatches := wherePattern.FindStringSubmatch(query)
|
|
if len(whereMatches) < 2 {
|
|
return ""
|
|
}
|
|
|
|
whereClause := whereMatches[1]
|
|
|
|
// Remove ORDER BY, LIMIT, OFFSET clauses from the end
|
|
whereClause = regexp.MustCompile(`(?is)\s+(ORDER BY|LIMIT|OFFSET).*$`).ReplaceAllString(whereClause, "")
|
|
|
|
// Remove SQL comments
|
|
whereClause = regexp.MustCompile(`(?m)--.*$`).ReplaceAllString(whereClause, "")
|
|
|
|
// Normalize indentation so subquery wrapping doesn't cause
|
|
// mismatches.
|
|
lines := strings.Split(whereClause, "\n")
|
|
for i, line := range lines {
|
|
lines[i] = strings.TrimLeft(line, " \t")
|
|
}
|
|
whereClause = strings.Join(lines, "\n")
|
|
|
|
return strings.TrimSpace(whereClause)
|
|
}
|