Skip to content

Commit 6b69abf

Browse files
mafredrimafredri
authored
fix(coderd): use stable sorting for insights and improve test coverage (coder#9250)
Fixes coder#9213
1 parent 970072f commit 6b69abf

19 files changed

+1974
-379
lines changed

‎Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,7 @@ coderd/apidoc/swagger.json: $(shell find ./scripts/apidocgen $(FIND_EXCLUSIONS)
564564
./scripts/apidocgen/generate.sh
565565
pnpm run format:write:only ./docs/api ./docs/manifest.json ./coderd/apidoc/swagger.json
566566

567-
update-golden-files: cli/testdata/.gen-golden helm/coder/tests/testdata/.gen-golden helm/provisioner/tests/testdata/.gen-golden scripts/ci-report/testdata/.gen-golden enterprise/cli/testdata/.gen-golden
567+
update-golden-files: cli/testdata/.gen-golden helm/coder/tests/testdata/.gen-golden helm/provisioner/tests/testdata/.gen-golden scripts/ci-report/testdata/.gen-golden enterprise/cli/testdata/.gen-golden coderd/.gen-golden
568568
.PHONY: update-golden-files
569569

570570
cli/testdata/.gen-golden: $(wildcard cli/testdata/*.golden) $(wildcard cli/*.tpl) $(GO_SRC_FILES) $(wildcard cli/*_test.go)
@@ -583,6 +583,10 @@ helm/provisioner/tests/testdata/.gen-golden: $(wildcard helm/provisioner/tests/t
583583
go test ./helm/provisioner/tests -run=TestUpdateGoldenFiles -update
584584
touch "$@"
585585

586+
coderd/.gen-golden: $(wildcard coderd/testdata/*/*.golden) $(GO_SRC_FILES) $(wildcard coderd/*_test.go)
587+
go test ./coderd -run="Test.*Golden$$" -update
588+
touch "$@"
589+
586590
scripts/ci-report/testdata/.gen-golden: $(wildcard scripts/ci-report/testdata/*) $(wildcard scripts/ci-report/*.go)
587591
go test ./scripts/ci-report -run=TestOutputMatchesGoldenFile -update
588592
touch "$@"

‎coderd/coderd_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package coderd_test
22

33
import (
44
"context"
5+
"flag"
56
"io"
67
"net/http"
78
"net/netip"
@@ -23,6 +24,9 @@ import (
2324
"github.com/coder/coder/v2/testutil"
2425
)
2526

27+
// updateGoldenFiles is a flag that can be set to update golden files.
28+
var updateGoldenFiles = flag.Bool("update", false, "Update golden files")
29+
2630
func TestMain(m *testing.M) {
2731
goleak.VerifyTestMain(m)
2832
}

‎coderd/database/db2sdk/db2sdk.go

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ package db2sdk
33

44
import (
55
"encoding/json"
6-
"sort"
6+
"strings"
77

88
"github.com/google/uuid"
9+
"golang.org/x/exp/slices"
910

1011
"github.com/coder/coder/v2/coderd/database"
1112
"github.com/coder/coder/v2/coderd/parameter"
@@ -125,9 +126,34 @@ func Role(role rbac.Role) codersdk.Role {
125126
}
126127

127128
func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) {
128-
parametersByNum := make(map[int64]*codersdk.TemplateParameterUsage)
129+
// Use a stable sort, similarly to how we would sort in the query, note that
130+
// we don't sort in the query because order varies depending on the table
131+
// collation.
132+
//
133+
// ORDER BY utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value
134+
slices.SortFunc(parameterRows, func(a, b database.GetTemplateParameterInsightsRow) int {
135+
if a.Name != b.Name {
136+
return strings.Compare(a.Name, b.Name)
137+
}
138+
if a.Type != b.Type {
139+
return strings.Compare(a.Type, b.Type)
140+
}
141+
if a.DisplayName != b.DisplayName {
142+
return strings.Compare(a.DisplayName, b.DisplayName)
143+
}
144+
if a.Description != b.Description {
145+
return strings.Compare(a.Description, b.Description)
146+
}
147+
if string(a.Options) != string(b.Options) {
148+
return strings.Compare(string(a.Options), string(b.Options))
149+
}
150+
return strings.Compare(a.Value, b.Value)
151+
})
152+
153+
parametersUsage := []codersdk.TemplateParameterUsage{}
154+
indexByNum := make(map[int64]int)
129155
for _, param := range parameterRows {
130-
if _, ok := parametersByNum[param.Num]; !ok {
156+
if _, ok := indexByNum[param.Num]; !ok {
131157
var opts []codersdk.TemplateVersionParameterOption
132158
err := json.Unmarshal(param.Options, &opts)
133159
if err != nil {
@@ -139,28 +165,24 @@ func TemplateInsightsParameters(parameterRows []database.GetTemplateParameterIns
139165
return nil, err
140166
}
141167

142-
parametersByNum[param.Num] = &codersdk.TemplateParameterUsage{
168+
parametersUsage = append(parametersUsage, codersdk.TemplateParameterUsage{
143169
TemplateIDs: param.TemplateIDs,
144170
Name: param.Name,
145171
Type: param.Type,
146172
DisplayName: param.DisplayName,
147173
Description: plaintextDescription,
148174
Options: opts,
149-
}
175+
})
176+
indexByNum[param.Num] = len(parametersUsage) - 1
150177
}
151-
parametersByNum[param.Num].Values = append(parametersByNum[param.Num].Values, codersdk.TemplateParameterValue{
178+
179+
i := indexByNum[param.Num]
180+
parametersUsage[i].Values = append(parametersUsage[i].Values, codersdk.TemplateParameterValue{
152181
Value: param.Value,
153182
Count: param.Count,
154183
})
155184
}
156-
parametersUsage := []codersdk.TemplateParameterUsage{}
157-
for _, param := range parametersByNum {
158-
parametersUsage = append(parametersUsage, *param)
159-
}
160185

161-
sort.Slice(parametersUsage, func(i, j int) bool {
162-
return parametersUsage[i].Name < parametersUsage[j].Name
163-
})
164186
return parametersUsage, nil
165187
}
166188

‎coderd/database/dbfake/dbfake.go

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2018,6 +2018,10 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G
20182018
return nil, err
20192019
}
20202020

2021+
if len(arg.TemplateIDs) > 0 && !slices.Contains(arg.TemplateIDs, w.TemplateID) {
2022+
continue
2023+
}
2024+
20212025
app, _ := q.getWorkspaceAppByAgentIDAndSlugNoLock(ctx, database.GetWorkspaceAppByAgentIDAndSlugParams{
20222026
AgentID: s.AgentID,
20232027
Slug: s.SlugOrPort,
@@ -2095,6 +2099,8 @@ func (q *FakeQuerier) GetTemplateAppInsights(ctx context.Context, arg database.G
20952099
})
20962100
}
20972101

2102+
// NOTE(mafredri): Add sorting if we decide on how to handle PostgreSQL collations.
2103+
// ORDER BY access_method, slug_or_port, display_name, icon, is_app
20982104
return rows, nil
20992105
}
21002106

@@ -2264,7 +2270,6 @@ func (q *FakeQuerier) GetTemplateDailyInsights(ctx context.Context, arg database
22642270
}
22652271
ds.userSet[s.UserID] = struct{}{}
22662272
ds.templateIDSet[s.TemplateID] = struct{}{}
2267-
break
22682273
}
22692274
}
22702275

@@ -2278,24 +2283,27 @@ func (q *FakeQuerier) GetTemplateDailyInsights(ctx context.Context, arg database
22782283
continue
22792284
}
22802285

2286+
w, err := q.getWorkspaceByIDNoLock(ctx, s.WorkspaceID)
2287+
if err != nil {
2288+
return nil, err
2289+
}
2290+
2291+
if len(arg.TemplateIDs) > 0 && !slices.Contains(arg.TemplateIDs, w.TemplateID) {
2292+
continue
2293+
}
2294+
22812295
for _, ds := range dailyStats {
22822296
// (was.session_started_at >= ts.from_ AND was.session_started_at < ts.to_)
22832297
// OR (was.session_ended_at > ts.from_ AND was.session_ended_at < ts.to_)
22842298
// OR (was.session_started_at < ts.from_ AND was.session_ended_at >= ts.to_)
2285-
if !(((s.SessionStartedAt.After(arg.StartTime) || s.SessionStartedAt.Equal(arg.StartTime)) && s.SessionStartedAt.Before(arg.EndTime)) ||
2286-
(s.SessionEndedAt.After(arg.StartTime) && s.SessionEndedAt.Before(arg.EndTime)) ||
2287-
(s.SessionStartedAt.Before(arg.StartTime) && (s.SessionEndedAt.After(arg.EndTime) || s.SessionEndedAt.Equal(arg.EndTime)))) {
2299+
if !(((s.SessionStartedAt.After(ds.startTime) || s.SessionStartedAt.Equal(ds.startTime)) && s.SessionStartedAt.Before(ds.endTime)) ||
2300+
(s.SessionEndedAt.After(ds.startTime) && s.SessionEndedAt.Before(ds.endTime)) ||
2301+
(s.SessionStartedAt.Before(ds.startTime) && (s.SessionEndedAt.After(ds.endTime) || s.SessionEndedAt.Equal(ds.endTime)))) {
22882302
continue
22892303
}
22902304

2291-
w, err := q.getWorkspaceByIDNoLock(ctx, s.WorkspaceID)
2292-
if err != nil {
2293-
return nil, err
2294-
}
2295-
22962305
ds.userSet[s.UserID] = struct{}{}
22972306
ds.templateIDSet[w.TemplateID] = struct{}{}
2298-
break
22992307
}
23002308
}
23012309

@@ -2430,7 +2438,8 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data
24302438
if tvp.TemplateVersionID != tv.ID {
24312439
continue
24322440
}
2433-
key := fmt.Sprintf("%s:%s:%s:%s", tvp.Name, tvp.DisplayName, tvp.Description, tvp.Options)
2441+
// GROUP BY tvp.name, tvp.type, tvp.display_name, tvp.description, tvp.options
2442+
key := fmt.Sprintf("%s:%s:%s:%s:%s", tvp.Name, tvp.Type, tvp.DisplayName, tvp.Description, tvp.Options)
24342443
if _, ok := uniqueTemplateParams[key]; !ok {
24352444
num++
24362445
uniqueTemplateParams[key] = &database.GetTemplateParameterInsightsRow{
@@ -2480,6 +2489,8 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data
24802489
}
24812490
}
24822491

2492+
// NOTE(mafredri): Add sorting if we decide on how to handle PostgreSQL collations.
2493+
// ORDER BY utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value
24832494
return rows, nil
24842495
}
24852496

‎coderd/database/queries.sql.go

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/insights.sql

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,13 @@ WITH latest_workspace_builds AS (
230230
array_agg(DISTINCT wb.template_id)::uuid[] AS template_ids,
231231
array_agg(wb.id)::uuid[] AS workspace_build_ids,
232232
tvp.name,
233+
tvp.type,
233234
tvp.display_name,
234235
tvp.description,
235-
tvp.options,
236-
tvp.type
236+
tvp.options
237237
FROM latest_workspace_builds wb
238238
JOIN template_version_parameters tvp ON (tvp.template_version_id = wb.template_version_id)
239-
GROUP BY tvp.name, tvp.display_name, tvp.description, tvp.options, tvp.type
239+
GROUP BY tvp.name, tvp.type, tvp.display_name, tvp.description, tvp.options
240240
)
241241

242242
SELECT
@@ -251,4 +251,4 @@ SELECT
251251
COUNT(wbp.value) AS count
252252
FROM unique_template_params utp
253253
JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name)
254-
GROUP BY utp.num, utp.name, utp.display_name, utp.description, utp.options, utp.template_ids, utp.type, wbp.value;
254+
GROUP BY utp.num, utp.template_ids, utp.name, utp.type, utp.display_name, utp.description, utp.options, wbp.value;

‎coderd/insights.go

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"net/http"
7+
"strings"
78
"time"
89

910
"github.com/google/uuid"
@@ -288,8 +289,10 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) {
288289
}
289290
for _, row := range dailyUsage {
290291
resp.IntervalReports = append(resp.IntervalReports, codersdk.TemplateInsightsIntervalReport{
291-
StartTime: row.StartTime,
292-
EndTime: row.EndTime,
292+
// NOTE(mafredri): This might not be accurate over DST since the
293+
// parsed location only contains the offset.
294+
StartTime: row.StartTime.In(startTime.Location()),
295+
EndTime: row.EndTime.In(startTime.Location()),
293296
Interval: interval,
294297
TemplateIDs: row.TemplateIDs,
295298
ActiveUsers: row.ActiveUsers,
@@ -377,6 +380,32 @@ func convertTemplateInsightsApps(usage database.GetTemplateInsightsRow, appUsage
377380
},
378381
}
379382

383+
// Use a stable sort, similarly to how we would sort in the query, note that
384+
// we don't sort in the query because order varies depending on the table
385+
// collation.
386+
//
387+
// ORDER BY access_method, slug_or_port, display_name, icon, is_app
388+
slices.SortFunc(appUsage, func(a, b database.GetTemplateAppInsightsRow) int {
389+
if a.AccessMethod != b.AccessMethod {
390+
return strings.Compare(a.AccessMethod, b.AccessMethod)
391+
}
392+
if a.SlugOrPort != b.SlugOrPort {
393+
return strings.Compare(a.SlugOrPort, b.SlugOrPort)
394+
}
395+
if a.DisplayName.String != b.DisplayName.String {
396+
return strings.Compare(a.DisplayName.String, b.DisplayName.String)
397+
}
398+
if a.Icon.String != b.Icon.String {
399+
return strings.Compare(a.Icon.String, b.Icon.String)
400+
}
401+
if !a.IsApp && b.IsApp {
402+
return -1
403+
} else if a.IsApp && !b.IsApp {
404+
return 1
405+
}
406+
return 0
407+
})
408+
380409
// Template apps.
381410
for _, app := range appUsage {
382411
if !app.IsApp {

0 commit comments

Comments
 (0)