From 6ed8faa06db6cca042f40ed12f5f8871e5672890 Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Sun, 26 Jun 2022 19:34:18 +0000 Subject: [PATCH 1/3] fix: Adjust pagination limit to be zero-based There isn't a use-case for querying a limit of zero. Using -1 led to issues when using default parameters for querying. --- coderd/database/queries.sql.go | 12 ++++++------ coderd/database/queries/templateversions.sql | 4 ++-- coderd/database/queries/users.sql | 4 ++-- coderd/database/queries/workspacebuilds.sql | 4 ++-- coderd/pagination.go | 2 +- coderd/pagination_internal_test.go | 2 -- 6 files changed, 13 insertions(+), 15 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 6811e93b8428a..e8589f4629937 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2242,8 +2242,8 @@ ORDER BY -- a timestamp. This is to ensure consistent pagination. (created_at, id) ASC OFFSET $3 LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF($4 :: int, -1) + -- A null limit means "no limit", so 0 means return all + NULLIF($4 :: int, 0) ` type GetTemplateVersionsByTemplateIDParams struct { @@ -2590,8 +2590,8 @@ ORDER BY -- a timestamp. This is to ensure consistent pagination. (created_at, id) ASC OFFSET $5 LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF($6 :: int, -1) + -- A null limit means "no limit", so 0 means return all + NULLIF($6 :: int, 0) ` type GetUsersParams struct { @@ -3573,8 +3573,8 @@ END ORDER BY build_number desc OFFSET $3 LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF($4 :: int, -1) + -- A null limit means "no limit", so 0 means return all + NULLIF($4 :: int, 0) ` type GetWorkspaceBuildByWorkspaceIDParams struct { diff --git a/coderd/database/queries/templateversions.sql b/coderd/database/queries/templateversions.sql index 231b3512553df..e9f9d4e00e8f4 100644 --- a/coderd/database/queries/templateversions.sql +++ b/coderd/database/queries/templateversions.sql @@ -29,8 +29,8 @@ ORDER BY -- a timestamp. This is to ensure consistent pagination. (created_at, id) ASC OFFSET @offset_opt LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF(@limit_opt :: int, -1); + -- A null limit means "no limit", so 0 means return all + NULLIF(@limit_opt :: int, 0); -- name: GetTemplateVersionByJobID :one SELECT diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 5359adc797d08..19fe8a7701744 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -126,8 +126,8 @@ ORDER BY -- a timestamp. This is to ensure consistent pagination. (created_at, id) ASC OFFSET @offset_opt LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF(@limit_opt :: int, -1); + -- A null limit means "no limit", so 0 means return all + NULLIF(@limit_opt :: int, 0); -- name: UpdateUserStatus :one UPDATE diff --git a/coderd/database/queries/workspacebuilds.sql b/coderd/database/queries/workspacebuilds.sql index 2e84616e99a33..85d154d0a7fbc 100644 --- a/coderd/database/queries/workspacebuilds.sql +++ b/coderd/database/queries/workspacebuilds.sql @@ -68,8 +68,8 @@ END ORDER BY build_number desc OFFSET @offset_opt LIMIT - -- A null limit means "no limit", so -1 means return all - NULLIF(@limit_opt :: int, -1); + -- A null limit means "no limit", so 0 means return all + NULLIF(@limit_opt :: int, 0); -- name: GetLatestWorkspaceBuildByWorkspaceID :one SELECT diff --git a/coderd/pagination.go b/coderd/pagination.go index 07f7b0fe743db..25d1465ef5092 100644 --- a/coderd/pagination.go +++ b/coderd/pagination.go @@ -17,7 +17,7 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat params := codersdk.Pagination{ AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"), // Limit default to "-1" which returns all results - Limit: parser.Int(queryParams, -1, "limit"), + Limit: parser.Int(queryParams, 0, "limit"), Offset: parser.Int(queryParams, 0, "offset"), } if len(parser.Errors) > 0 { diff --git a/coderd/pagination_internal_test.go b/coderd/pagination_internal_test.go index 5504ef267e165..978cfab417b2e 100644 --- a/coderd/pagination_internal_test.go +++ b/coderd/pagination_internal_test.go @@ -77,7 +77,6 @@ func TestPagination(t *testing.T) { ExpectedParams: codersdk.Pagination{ AfterID: uuid.Nil, Offset: 150, - Limit: -1, }, }, { @@ -85,7 +84,6 @@ func TestPagination(t *testing.T) { AfterID: "5f2005fc-acc4-4e5e-a7fa-be017359c60b", ExpectedParams: codersdk.Pagination{ AfterID: uuid.MustParse("5f2005fc-acc4-4e5e-a7fa-be017359c60b"), - Limit: -1, }, }, } From 11b4647f33529a608166e01ca9519de5e25a048b Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Sun, 26 Jun 2022 19:39:42 +0000 Subject: [PATCH 2/3] fix: Update database fake to check for nil time when streaming logs This caused a test flake seen here: https://github.com/coder/coder/runs/7056544834?check_suite_focus=true --- coderd/database/databasefake/databasefake.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index c4757075d397c..644d74b513436 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1360,10 +1360,10 @@ func (q *fakeQuerier) GetProvisionerLogsByIDBetween(_ context.Context, arg datab if jobLog.JobID.String() != arg.JobID.String() { continue } - if jobLog.CreatedAt.After(arg.CreatedBefore) { + if !arg.CreatedBefore.IsZero() && jobLog.CreatedAt.After(arg.CreatedBefore) { continue } - if jobLog.CreatedAt.Before(arg.CreatedAfter) { + if !arg.CreatedAfter.IsZero() && jobLog.CreatedAt.Before(arg.CreatedAfter) { continue } logs = append(logs, jobLog) From 9e2558dba5bd839820ccb52f22c071682a6e1ca2 Mon Sep 17 00:00:00 2001 From: kylecarbs Date: Sun, 26 Jun 2022 20:10:44 +0000 Subject: [PATCH 3/3] fix: Increase wait time for agent connection tests This was causing a test flake seen here: https://github.com/coder/coder/runs/7063032150?check_suite_focus=true --- coderd/coderdtest/coderdtest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 73451c8efa143..ea530c4de2c44 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -396,7 +396,7 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID } } return true - }, 5*time.Second, 25*time.Millisecond) + }, 15*time.Second, 50*time.Millisecond) return resources }