mirror of https://github.com/coder/coder.git
fix: improve pagination parser (#12422)
This commit is contained in:
parent
61db293b33
commit
3e99c0373f
|
@ -79,6 +79,30 @@ func (p *QueryParamParser) Int(vals url.Values, def int, queryParam string) int
|
||||||
return v
|
return v
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// PositiveInt32 function checks if the given value is 32-bit and positive.
|
||||||
|
//
|
||||||
|
// We can't use `uint32` as the value must be within the range <0,2147483647>
|
||||||
|
// as database expects it. Otherwise, the database query fails with `pq: OFFSET must not be negative`.
|
||||||
|
func (p *QueryParamParser) PositiveInt32(vals url.Values, def int32, queryParam string) int32 {
|
||||||
|
v, err := parseQueryParam(p, vals, func(v string) (int32, error) {
|
||||||
|
intValue, err := strconv.ParseInt(v, 10, 32)
|
||||||
|
if err != nil {
|
||||||
|
return 0, err
|
||||||
|
}
|
||||||
|
if intValue < 0 {
|
||||||
|
return 0, xerrors.Errorf("value is negative")
|
||||||
|
}
|
||||||
|
return int32(intValue), nil
|
||||||
|
}, def, queryParam)
|
||||||
|
if err != nil {
|
||||||
|
p.Errors = append(p.Errors, codersdk.ValidationError{
|
||||||
|
Field: queryParam,
|
||||||
|
Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer (%s)", queryParam, err.Error()),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
return v
|
||||||
|
}
|
||||||
|
|
||||||
func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string) bool {
|
func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string) bool {
|
||||||
v, err := parseQueryParam(p, vals, strconv.ParseBool, def, queryParam)
|
v, err := parseQueryParam(p, vals, strconv.ParseBool, def, queryParam)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
|
@ -236,6 +236,50 @@ func TestParseQueryParams(t *testing.T) {
|
||||||
testQueryParams(t, expParams, parser, parser.Int)
|
testQueryParams(t, expParams, parser, parser.Int)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
t.Run("PositiveInt32", func(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
expParams := []queryParamTestCase[int32]{
|
||||||
|
{
|
||||||
|
QueryParam: "valid_integer",
|
||||||
|
Value: "100",
|
||||||
|
Expected: 100,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
QueryParam: "empty",
|
||||||
|
Value: "",
|
||||||
|
Expected: 0,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
QueryParam: "no_value",
|
||||||
|
NoSet: true,
|
||||||
|
Default: 5,
|
||||||
|
Expected: 5,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
QueryParam: "negative",
|
||||||
|
Value: "-1",
|
||||||
|
Expected: 0,
|
||||||
|
Default: 5,
|
||||||
|
ExpectedErrorContains: "must be a valid 32-bit positive integer",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
QueryParam: "invalid_integer",
|
||||||
|
Value: "bogus",
|
||||||
|
Expected: 0,
|
||||||
|
ExpectedErrorContains: "must be a valid 32-bit positive integer",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
QueryParam: "max_int_plus_one",
|
||||||
|
Value: "2147483648",
|
||||||
|
Expected: 0,
|
||||||
|
ExpectedErrorContains: "must be a valid 32-bit positive integer",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
parser := httpapi.NewQueryParamParser()
|
||||||
|
testQueryParams(t, expParams, parser, parser.PositiveInt32)
|
||||||
|
})
|
||||||
|
|
||||||
t.Run("UInt", func(t *testing.T) {
|
t.Run("UInt", func(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
expParams := []queryParamTestCase[uint64]{
|
expParams := []queryParamTestCase[uint64]{
|
||||||
|
|
|
@ -17,9 +17,8 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat
|
||||||
parser := httpapi.NewQueryParamParser()
|
parser := httpapi.NewQueryParamParser()
|
||||||
params := codersdk.Pagination{
|
params := codersdk.Pagination{
|
||||||
AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"),
|
AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"),
|
||||||
// Limit default to "-1" which returns all results
|
Limit: int(parser.PositiveInt32(queryParams, 0, "limit")),
|
||||||
Limit: parser.Int(queryParams, 0, "limit"),
|
Offset: int(parser.PositiveInt32(queryParams, 0, "offset")),
|
||||||
Offset: parser.Int(queryParams, 0, "offset"),
|
|
||||||
}
|
}
|
||||||
if len(parser.Errors) > 0 {
|
if len(parser.Errors) > 0 {
|
||||||
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
|
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
|
||||||
|
|
|
@ -46,11 +46,31 @@ func TestPagination(t *testing.T) {
|
||||||
Limit: "bogus",
|
Limit: "bogus",
|
||||||
ExpectedError: invalidValues,
|
ExpectedError: invalidValues,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
Name: "TooHighLimit",
|
||||||
|
Limit: "2147483648",
|
||||||
|
ExpectedError: invalidValues,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
Name: "NegativeLimit",
|
||||||
|
Limit: "-1",
|
||||||
|
ExpectedError: invalidValues,
|
||||||
|
},
|
||||||
{
|
{
|
||||||
Name: "BadOffset",
|
Name: "BadOffset",
|
||||||
Offset: "bogus",
|
Offset: "bogus",
|
||||||
ExpectedError: invalidValues,
|
ExpectedError: invalidValues,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
Name: "TooHighOffset",
|
||||||
|
Offset: "2147483648",
|
||||||
|
ExpectedError: invalidValues,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
Name: "NegativeOffset",
|
||||||
|
Offset: "-1",
|
||||||
|
ExpectedError: invalidValues,
|
||||||
|
},
|
||||||
|
|
||||||
// Valid values
|
// Valid values
|
||||||
{
|
{
|
||||||
|
|
|
@ -6,6 +6,7 @@ import (
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"math"
|
||||||
"net/http"
|
"net/http"
|
||||||
"os"
|
"os"
|
||||||
"strings"
|
"strings"
|
||||||
|
@ -1727,19 +1728,19 @@ func TestOffsetLimit(t *testing.T) {
|
||||||
_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
|
_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
|
||||||
_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
|
_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
|
||||||
|
|
||||||
// empty finds all workspaces
|
// Case 1: empty finds all workspaces
|
||||||
ws, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
|
ws, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.Len(t, ws.Workspaces, 3)
|
require.Len(t, ws.Workspaces, 3)
|
||||||
|
|
||||||
// offset 1 finds 2 workspaces
|
// Case 2: offset 1 finds 2 workspaces
|
||||||
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
||||||
Offset: 1,
|
Offset: 1,
|
||||||
})
|
})
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.Len(t, ws.Workspaces, 2)
|
require.Len(t, ws.Workspaces, 2)
|
||||||
|
|
||||||
// offset 1 limit 1 finds 1 workspace
|
// Case 3: offset 1 limit 1 finds 1 workspace
|
||||||
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
||||||
Offset: 1,
|
Offset: 1,
|
||||||
Limit: 1,
|
Limit: 1,
|
||||||
|
@ -1747,13 +1748,19 @@ func TestOffsetLimit(t *testing.T) {
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.Len(t, ws.Workspaces, 1)
|
require.Len(t, ws.Workspaces, 1)
|
||||||
|
|
||||||
// offset 3 finds no workspaces
|
// Case 4: offset 3 finds no workspaces
|
||||||
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
||||||
Offset: 3,
|
Offset: 3,
|
||||||
})
|
})
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.Len(t, ws.Workspaces, 0)
|
require.Len(t, ws.Workspaces, 0)
|
||||||
require.Equal(t, ws.Count, 3) // can't find workspaces, but count is non-zero
|
require.Equal(t, ws.Count, 3) // can't find workspaces, but count is non-zero
|
||||||
|
|
||||||
|
// Case 5: offset out of range
|
||||||
|
ws, err = client.Workspaces(ctx, codersdk.WorkspaceFilter{
|
||||||
|
Offset: math.MaxInt32 + 1, // Potential risk: pq: OFFSET must not be negative
|
||||||
|
})
|
||||||
|
require.Error(t, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestWorkspaceUpdateAutostart(t *testing.T) {
|
func TestWorkspaceUpdateAutostart(t *testing.T) {
|
||||||
|
|
Loading…
Reference in New Issue