Skip to content

Commit dedc32f

Browse files
SasSwartSasSwart
authored
fix(coderd): avoid fetching extra parameters for a preset (coder#16642)
This pull request fixes a bug in presets and adds tests to ensure it doesn't happen again. Due to an oversight in refactoring, we returned extra and incorrect parameters from other presets in the same template version when calling `/templateversions/{templateversion}/presets`.
1 parent 9469b78 commit dedc32f

File tree

2 files changed

+115
-41
lines changed

2 files changed

+115
-41
lines changed

‎coderd/presets.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ func (api *API) templateVersionPresets(rw http.ResponseWriter, r *http.Request)
4545
Name: preset.Name,
4646
}
4747
for _, presetParam := range presetParams {
48+
if presetParam.TemplateVersionPresetID != preset.ID {
49+
continue
50+
}
51+
4852
sdkPreset.Parameters = append(sdkPreset.Parameters, codersdk.PresetParameter{
4953
Name: presetParam.Name,
5054
Value: presetParam.Value,

‎coderd/presets_test.go

Lines changed: 111 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,58 +17,128 @@ import (
1717
func TestTemplateVersionPresets(t *testing.T) {
1818
t.Parallel()
1919

20-
givenPreset := codersdk.Preset{
21-
Name: "My Preset",
22-
Parameters: []codersdk.PresetParameter{
23-
{
24-
Name: "preset_param1",
25-
Value: "A1B2C3",
20+
testCases := []struct {
21+
name string
22+
presets []codersdk.Preset
23+
}{
24+
{
25+
name: "no presets",
26+
presets: []codersdk.Preset{},
27+
},
28+
{
29+
name: "single preset with parameters",
30+
presets: []codersdk.Preset{
31+
{
32+
Name: "My Preset",
33+
Parameters: []codersdk.PresetParameter{
34+
{
35+
Name: "preset_param1",
36+
Value: "A1B2C3",
37+
},
38+
{
39+
Name: "preset_param2",
40+
Value: "D4E5F6",
41+
},
42+
},
43+
},
2644
},
27-
{
28-
Name: "preset_param2",
29-
Value: "D4E5F6",
45+
},
46+
{
47+
name: "multiple presets with overlapping parameters",
48+
presets: []codersdk.Preset{
49+
{
50+
Name: "Preset 1",
51+
Parameters: []codersdk.PresetParameter{
52+
{
53+
Name: "shared_param",
54+
Value: "value1",
55+
},
56+
{
57+
Name: "unique_param1",
58+
Value: "unique1",
59+
},
60+
},
61+
},
62+
{
63+
Name: "Preset 2",
64+
Parameters: []codersdk.PresetParameter{
65+
{
66+
Name: "shared_param",
67+
Value: "value2",
68+
},
69+
{
70+
Name: "unique_param2",
71+
Value: "unique2",
72+
},
73+
},
74+
},
3075
},
3176
},
3277
}
33-
ctx := testutil.Context(t, testutil.WaitShort)
3478

35-
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
36-
user := coderdtest.CreateFirstUser(t, client)
37-
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
79+
for _, tc := range testCases {
80+
tc := tc
81+
t.Run(tc.name, func(t *testing.T) {
82+
t.Parallel()
83+
ctx := testutil.Context(t, testutil.WaitShort)
3884

39-
// nolint:gocritic // This is a test
40-
provisionerCtx := dbauthz.AsProvisionerd(ctx)
85+
client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
86+
user := coderdtest.CreateFirstUser(t, client)
87+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
4188

42-
dbPreset, err := db.InsertPreset(provisionerCtx, database.InsertPresetParams{
43-
Name: givenPreset.Name,
44-
TemplateVersionID: version.ID,
45-
})
46-
require.NoError(t, err)
89+
// nolint:gocritic // This is a test
90+
provisionerCtx := dbauthz.AsProvisionerd(ctx)
4791

48-
var presetParameterNames []string
49-
var presetParameterValues []string
50-
for _, presetParameter := range givenPreset.Parameters {
51-
presetParameterNames = append(presetParameterNames, presetParameter.Name)
52-
presetParameterValues = append(presetParameterValues, presetParameter.Value)
53-
}
54-
_, err = db.InsertPresetParameters(provisionerCtx, database.InsertPresetParametersParams{
55-
TemplateVersionPresetID: dbPreset.ID,
56-
Names: presetParameterNames,
57-
Values: presetParameterValues,
58-
})
59-
require.NoError(t, err)
92+
// Insert all presets for this test case
93+
for _, givenPreset := range tc.presets {
94+
dbPreset, err := db.InsertPreset(provisionerCtx, database.InsertPresetParams{
95+
Name: givenPreset.Name,
96+
TemplateVersionID: version.ID,
97+
})
98+
require.NoError(t, err)
99+
100+
if len(givenPreset.Parameters) > 0 {
101+
var presetParameterNames []string
102+
var presetParameterValues []string
103+
for _, presetParameter := range givenPreset.Parameters {
104+
presetParameterNames = append(presetParameterNames, presetParameter.Name)
105+
presetParameterValues = append(presetParameterValues, presetParameter.Value)
106+
}
107+
_, err = db.InsertPresetParameters(provisionerCtx, database.InsertPresetParametersParams{
108+
TemplateVersionPresetID: dbPreset.ID,
109+
Names: presetParameterNames,
110+
Values: presetParameterValues,
111+
})
112+
require.NoError(t, err)
113+
}
114+
}
115+
116+
userSubject, _, err := httpmw.UserRBACSubject(ctx, db, user.UserID, rbac.ScopeAll)
117+
require.NoError(t, err)
118+
userCtx := dbauthz.As(ctx, userSubject)
60119

61-
userSubject, _, err := httpmw.UserRBACSubject(ctx, db, user.UserID, rbac.ScopeAll)
62-
require.NoError(t, err)
63-
userCtx := dbauthz.As(ctx, userSubject)
120+
gotPresets, err := client.TemplateVersionPresets(userCtx, version.ID)
121+
require.NoError(t, err)
64122

65-
gotPresets, err := client.TemplateVersionPresets(userCtx, version.ID)
66-
require.NoError(t, err)
123+
require.Equal(t, len(tc.presets), len(gotPresets))
67124

68-
require.Equal(t, 1, len(gotPresets))
69-
require.Equal(t, givenPreset.Name, gotPresets[0].Name)
125+
for _, expectedPreset := range tc.presets {
126+
found := false
127+
for _, gotPreset := range gotPresets {
128+
if gotPreset.Name == expectedPreset.Name {
129+
found = true
70130

71-
for _, presetParameter := range givenPreset.Parameters {
72-
require.Contains(t, gotPresets[0].Parameters, presetParameter)
131+
// verify not only that we get the right number of parameters, but that we get the right parameters
132+
// This ensures that we don't get extra parameters from other presets
133+
require.Equal(t, len(expectedPreset.Parameters), len(gotPreset.Parameters))
134+
for _, expectedParam := range expectedPreset.Parameters {
135+
require.Contains(t, gotPreset.Parameters, expectedParam)
136+
}
137+
break
138+
}
139+
}
140+
require.True(t, found, "Expected preset %s not found in results", expectedPreset.Name)
141+
}
142+
})
73143
}
74144
}

0 commit comments

Comments
 (0)