Skip to content

Commit 1b7edde

Browse files
johnstcnjohnstcn
committed
Merge remote-tracking branch 'origin/main' into cj/flake-WorkspaceApps
2 parents ea99e19 + b4492ff commit 1b7edde

File tree

5 files changed

+187
-44
lines changed

5 files changed

+187
-44
lines changed

‎coderd/httpapi/queryparams.go

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (p *QueryParamParser) UInt(vals url.Values, def uint64, queryParam string)
6161
if err != nil {
6262
p.Errors = append(p.Errors, codersdk.ValidationError{
6363
Field: queryParam,
64-
Detail: fmt.Sprintf("Query param %q must be a valid positive integer (%s)", queryParam, err.Error()),
64+
Detail: fmt.Sprintf("Query param %q must be a valid positive integer: %s", queryParam, err.Error()),
6565
})
6666
return 0
6767
}
@@ -73,7 +73,7 @@ func (p *QueryParamParser) Int(vals url.Values, def int, queryParam string) int
7373
if err != nil {
7474
p.Errors = append(p.Errors, codersdk.ValidationError{
7575
Field: queryParam,
76-
Detail: fmt.Sprintf("Query param %q must be a valid integer (%s)", queryParam, err.Error()),
76+
Detail: fmt.Sprintf("Query param %q must be a valid integer: %s", queryParam, err.Error()),
7777
})
7878
}
7979
return v
@@ -97,7 +97,7 @@ func (p *QueryParamParser) PositiveInt32(vals url.Values, def int32, queryParam
9797
if err != nil {
9898
p.Errors = append(p.Errors, codersdk.ValidationError{
9999
Field: queryParam,
100-
Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer (%s)", queryParam, err.Error()),
100+
Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer: %s", queryParam, err.Error()),
101101
})
102102
}
103103
return v
@@ -108,7 +108,7 @@ func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string)
108108
if err != nil {
109109
p.Errors = append(p.Errors, codersdk.ValidationError{
110110
Field: queryParam,
111-
Detail: fmt.Sprintf("Query param %q must be a valid boolean (%s)", queryParam, err.Error()),
111+
Detail: fmt.Sprintf("Query param %q must be a valid boolean: %s", queryParam, err.Error()),
112112
})
113113
}
114114
return v
@@ -203,9 +203,15 @@ func (p *QueryParamParser) timeWithMutate(vals url.Values, def time.Time, queryP
203203
}
204204

205205
func (p *QueryParamParser) String(vals url.Values, def string, queryParam string) string {
206-
v, _ := parseQueryParam(p, vals, func(v string) (string, error) {
206+
v, err := parseQueryParam(p, vals, func(v string) (string, error) {
207207
return v, nil
208208
}, def, queryParam)
209+
if err != nil {
210+
p.Errors = append(p.Errors, codersdk.ValidationError{
211+
Field: queryParam,
212+
Detail: fmt.Sprintf("Query param %q must be a valid string: %s", queryParam, err.Error()),
213+
})
214+
}
209215
return v
210216
}
211217

@@ -248,13 +254,23 @@ func ParseCustom[T any](parser *QueryParamParser, vals url.Values, def T, queryP
248254
return v
249255
}
250256

251-
// ParseCustomList is a function that handles csv query params.
257+
// ParseCustomList is a function that handles csv query params or multiple values
258+
// for a query param.
259+
// Csv is supported as it is a common way to pass multiple values in a query param.
260+
// Multiple values is supported (key=value&key=value2) for feature parity with GitHub issue search.
252261
func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, queryParam string, parseFunc func(v string) (T, error)) []T {
253-
v, err := parseQueryParam(parser, vals, func(v string) ([]T, error) {
254-
terms := strings.Split(v, ",")
262+
v, err := parseQueryParamSet(parser, vals, func(set []string) ([]T, error) {
263+
// Gather all terms.
264+
allTerms := make([]string, 0, len(set))
265+
for _, s := range set {
266+
// If a term is a csv, break it out into individual terms.
267+
terms := strings.Split(s, ",")
268+
allTerms = append(allTerms, terms...)
269+
}
270+
255271
var badValues []string
256272
var output []T
257-
for _, s := range terms {
273+
for _, s := range allTerms {
258274
good, err := parseFunc(s)
259275
if err != nil {
260276
badValues = append(badValues, s)
@@ -277,7 +293,27 @@ func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T,
277293
return v
278294
}
279295

296+
// parseQueryParam expects just 1 value set for the given query param.
280297
func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse func(v string) (T, error), def T, queryParam string) (T, error) {
298+
setParse := func(set []string) (T, error) {
299+
if len(set) > 1 {
300+
// Set as a parser.Error rather than return an error.
301+
// Returned errors are errors from the passed in `parse` function, and
302+
// imply the query param value had attempted to be parsed.
303+
// By raising the error this way, we can also more easily control how it
304+
// is presented to the user. A returned error is wrapped with more text.
305+
parser.Errors = append(parser.Errors, codersdk.ValidationError{
306+
Field: queryParam,
307+
Detail: fmt.Sprintf("Query param %q provided more than once, found %d times. Only provide 1 instance of this query param.", queryParam, len(set)),
308+
})
309+
return def, nil
310+
}
311+
return parse(set[0])
312+
}
313+
return parseQueryParamSet(parser, vals, setParse, def, queryParam)
314+
}
315+
316+
func parseQueryParamSet[T any](parser *QueryParamParser, vals url.Values, parse func(set []string) (T, error), def T, queryParam string) (T, error) {
281317
parser.addParsed(queryParam)
282318
// If the query param is required and not present, return an error.
283319
if parser.RequiredNotEmptyParams[queryParam] && (!vals.Has(queryParam) || vals.Get(queryParam) == "") {
@@ -293,6 +329,5 @@ func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse fun
293329
return def, nil
294330
}
295331

296-
str := vals.Get(queryParam)
297-
return parse(str)
332+
return parse(vals[queryParam])
298333
}

‎coderd/httpapi/queryparams_test.go

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,13 @@ import (
1717
type queryParamTestCase[T any] struct {
1818
QueryParam string
1919
// No set does not set the query param, rather than setting the empty value
20-
NoSet bool
21-
Value string
20+
NoSet bool
21+
// Value vs values is the difference between a single query param and multiple
22+
// to the same key.
23+
// -> key=value
24+
Value string
25+
// -> key=value1 key=value2
26+
Values []string
2227
Default T
2328
Expected T
2429
ExpectedErrorContains string
@@ -27,6 +32,7 @@ type queryParamTestCase[T any] struct {
2732

2833
func TestParseQueryParams(t *testing.T) {
2934
t.Parallel()
35+
const multipleValuesError = "provided more than once"
3036

3137
t.Run("Enum", func(t *testing.T) {
3238
t.Parallel()
@@ -59,6 +65,11 @@ func TestParseQueryParams(t *testing.T) {
5965
Value: fmt.Sprintf("%s,%s", database.ResourceTypeWorkspace, database.ResourceTypeApiKey),
6066
Expected: []database.ResourceType{database.ResourceTypeWorkspace, database.ResourceTypeApiKey},
6167
},
68+
{
69+
QueryParam: "resource_type_as_list",
70+
Values: []string{string(database.ResourceTypeWorkspace), string(database.ResourceTypeApiKey)},
71+
Expected: []database.ResourceType{database.ResourceTypeWorkspace, database.ResourceTypeApiKey},
72+
},
6273
}
6374

6475
parser := httpapi.NewQueryParamParser()
@@ -151,6 +162,11 @@ func TestParseQueryParams(t *testing.T) {
151162
Default: "default",
152163
Expected: "default",
153164
},
165+
{
166+
QueryParam: "unexpected_list",
167+
Values: []string{"one", "two"},
168+
ExpectedErrorContains: multipleValuesError,
169+
},
154170
}
155171

156172
parser := httpapi.NewQueryParamParser()
@@ -193,6 +209,11 @@ func TestParseQueryParams(t *testing.T) {
193209
Expected: false,
194210
ExpectedErrorContains: "must be a valid boolean",
195211
},
212+
{
213+
QueryParam: "unexpected_list",
214+
Values: []string{"true", "false"},
215+
ExpectedErrorContains: multipleValuesError,
216+
},
196217
}
197218

198219
parser := httpapi.NewQueryParamParser()
@@ -230,6 +251,11 @@ func TestParseQueryParams(t *testing.T) {
230251
Expected: 0,
231252
ExpectedErrorContains: "must be a valid integer",
232253
},
254+
{
255+
QueryParam: "unexpected_list",
256+
Values: []string{"5", "10"},
257+
ExpectedErrorContains: multipleValuesError,
258+
},
233259
}
234260

235261
parser := httpapi.NewQueryParamParser()
@@ -274,6 +300,11 @@ func TestParseQueryParams(t *testing.T) {
274300
Expected: 0,
275301
ExpectedErrorContains: "must be a valid 32-bit positive integer",
276302
},
303+
{
304+
QueryParam: "unexpected_list",
305+
Values: []string{"5", "10"},
306+
ExpectedErrorContains: multipleValuesError,
307+
},
277308
}
278309

279310
parser := httpapi.NewQueryParamParser()
@@ -311,6 +342,11 @@ func TestParseQueryParams(t *testing.T) {
311342
Expected: 0,
312343
ExpectedErrorContains: "must be a valid positive integer",
313344
},
345+
{
346+
QueryParam: "unexpected_list",
347+
Values: []string{"5", "10"},
348+
ExpectedErrorContains: multipleValuesError,
349+
},
314350
}
315351

316352
parser := httpapi.NewQueryParamParser()
@@ -354,6 +390,23 @@ func TestParseQueryParams(t *testing.T) {
354390
Default: []uuid.UUID{},
355391
ExpectedErrorContains: "bogus",
356392
},
393+
{
394+
QueryParam: "multiple_keys",
395+
Values: []string{"6c8ef17d-5dd8-4b92-bac9-41944f90f237", "65fb05f3-12c8-4a0a-801f-40439cf9e681"},
396+
Expected: []uuid.UUID{
397+
uuid.MustParse("6c8ef17d-5dd8-4b92-bac9-41944f90f237"),
398+
uuid.MustParse("65fb05f3-12c8-4a0a-801f-40439cf9e681"),
399+
},
400+
},
401+
{
402+
QueryParam: "multiple_and_csv",
403+
Values: []string{"6c8ef17d-5dd8-4b92-bac9-41944f90f237", "65fb05f3-12c8-4a0a-801f-40439cf9e681, 01b94888-1eab-4bbf-aed0-dc7a8010da97"},
404+
Expected: []uuid.UUID{
405+
uuid.MustParse("6c8ef17d-5dd8-4b92-bac9-41944f90f237"),
406+
uuid.MustParse("65fb05f3-12c8-4a0a-801f-40439cf9e681"),
407+
uuid.MustParse("01b94888-1eab-4bbf-aed0-dc7a8010da97"),
408+
},
409+
},
357410
}
358411

359412
parser := httpapi.NewQueryParamParser()
@@ -381,7 +434,17 @@ func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], par
381434
if c.NoSet {
382435
continue
383436
}
384-
v.Set(c.QueryParam, c.Value)
437+
if len(c.Values) > 0 && c.Value != "" {
438+
t.Errorf("test case %q has both value and values, choose one, not both!", c.QueryParam)
439+
t.FailNow()
440+
}
441+
if c.Value != "" {
442+
c.Values = append(c.Values, c.Value)
443+
}
444+
445+
for _, value := range c.Values {
446+
v.Add(c.QueryParam, value)
447+
}
385448
}
386449

387450
for _, c := range testCases {

‎coderd/searchquery/search.go

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -163,17 +163,6 @@ func searchTerms(query string, defaultKey func(term string, values url.Values) e
163163
}
164164
}
165165

166-
for k := range searchValues {
167-
if len(searchValues[k]) > 1 {
168-
return nil, []codersdk.ValidationError{
169-
{
170-
Field: "q",
171-
Detail: fmt.Sprintf("Query parameter %q provided more than once, found %d times", k, len(searchValues[k])),
172-
},
173-
}
174-
}
175-
}
176-
177166
return searchValues, nil
178167
}
179168

‎site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsForm.tsx

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -236,24 +236,11 @@ export const TemplateSettingsForm: FC<TemplateSettingsForm> = ({
236236
description="Deprecating a template prevents any new workspaces from being created. Existing workspaces will continue to function."
237237
>
238238
<FormFields>
239-
<Stack direction="column" spacing={0.5}>
240-
<Stack
241-
direction="row"
242-
alignItems="center"
243-
spacing={0.5}
244-
css={styles.optionText}
245-
>
246-
Deprecation Message
247-
</Stack>
248-
<span css={styles.optionHelperText}>
249-
Leave the message empty to keep the template active. Any message
250-
provided will mark the template as deprecated. Use this message to
251-
inform users of the deprecation and how to migrate to a new
252-
template.
253-
</span>
254-
</Stack>
255239
<TextField
256-
{...getFieldHelpers("deprecation_message")}
240+
{...getFieldHelpers("deprecation_message", {
241+
helperText:
242+
"Leave the message empty to keep the template active. Any message provided will mark the template as deprecated. Use this message to inform users of the deprecation and how to migrate to a new template.",
243+
})}
257244
disabled={isSubmitting || !accessControlEnabled}
258245
fullWidth
259246
label="Deprecation Message"

‎site/src/pages/TemplateSettingsPage/TemplateGeneralSettingsPage/TemplateSettingsPage.test.tsx

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import { screen, waitFor } from "@testing-library/react";
22
import userEvent from "@testing-library/user-event";
3+
import { http, HttpResponse } from "msw";
34
import * as API from "api/api";
4-
import type { UpdateTemplateMeta } from "api/typesGenerated";
5+
import type { Template, UpdateTemplateMeta } from "api/typesGenerated";
56
import { Language as FooterFormLanguage } from "components/FormFooter/FormFooter";
6-
import { MockTemplate } from "testHelpers/entities";
7+
import { MockEntitlements, MockTemplate } from "testHelpers/entities";
78
import {
89
renderWithTemplateSettingsLayout,
910
waitForLoaderToBeRemoved,
1011
} from "testHelpers/renderHelpers";
12+
import { server } from "testHelpers/server";
1113
import { getValidationSchema } from "./TemplateSettingsForm";
1214
import { TemplateSettingsPage } from "./TemplateSettingsPage";
1315

@@ -55,6 +57,9 @@ const renderTemplateSettingsPage = async () => {
5557
renderWithTemplateSettingsLayout(<TemplateSettingsPage />, {
5658
route: `/templates/${MockTemplate.name}/settings`,
5759
path: `/templates/:template/settings`,
60+
extraRoutes: [
61+
{ path: "/templates/:template", element: <div>Template</div> },
62+
],
5863
});
5964
await waitForLoaderToBeRemoved();
6065
};
@@ -126,4 +131,68 @@ describe("TemplateSettingsPage", () => {
126131
const validate = () => getValidationSchema().validateSync(values);
127132
expect(validate).toThrowError();
128133
});
134+
135+
describe("Deprecate template", () => {
136+
it("deprecates a template when has access control", async () => {
137+
server.use(
138+
http.get("/api/v2/entitlements", () => {
139+
return HttpResponse.json({
140+
...MockEntitlements,
141+
features: API.withDefaultFeatures({
142+
access_control: { enabled: true, entitlement: "entitled" },
143+
}),
144+
});
145+
}),
146+
);
147+
const updateTemplateMetaSpy = jest.spyOn(API, "updateTemplateMeta");
148+
const deprecationMessage = "This template is deprecated";
149+
150+
await renderTemplateSettingsPage();
151+
await deprecateTemplate(MockTemplate, deprecationMessage);
152+
153+
const [templateId, data] = updateTemplateMetaSpy.mock.calls[0];
154+
155+
expect(templateId).toEqual(MockTemplate.id);
156+
expect(data).toEqual(
157+
expect.objectContaining({ deprecation_message: deprecationMessage }),
158+
);
159+
});
160+
161+
it("does not deprecate a template when does not have access control", async () => {
162+
server.use(
163+
http.get("/api/v2/entitlements", () => {
164+
return HttpResponse.json({
165+
...MockEntitlements,
166+
features: API.withDefaultFeatures({
167+
access_control: { enabled: false, entitlement: "not_entitled" },
168+
}),
169+
});
170+
}),
171+
);
172+
const updateTemplateMetaSpy = jest.spyOn(API, "updateTemplateMeta");
173+
174+
await renderTemplateSettingsPage();
175+
await deprecateTemplate(
176+
MockTemplate,
177+
"This template should not be able to deprecate",
178+
);
179+
180+
const [templateId, data] = updateTemplateMetaSpy.mock.calls[0];
181+
182+
expect(templateId).toEqual(MockTemplate.id);
183+
expect(data).toEqual(
184+
expect.objectContaining({ deprecation_message: "" }),
185+
);
186+
});
187+
});
129188
});
189+
190+
async function deprecateTemplate(template: Template, message: string) {
191+
const deprecationField = screen.getByLabelText("Deprecation Message");
192+
await userEvent.type(deprecationField, message);
193+
194+
const submitButton = await screen.findByText(
195+
FooterFormLanguage.defaultSubmitLabel,
196+
);
197+
await userEvent.click(submitButton);
198+
}

0 commit comments

Comments
 (0)