Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

chore: implement sane default pagination limit for audit logs#13676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 5 commits intomainfromstevenmasley/sane_audit_default_limit
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletionscoderd/audit.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -53,8 +53,8 @@ func (api *API) auditLogs(rw http.ResponseWriter, r *http.Request) {
})
return
}
filter.Offset = int32(page.Offset)
filter.Limit = int32(page.Limit)
filter.OffsetOpt = int32(page.Offset)
filter.LimitOpt = int32(page.Limit)

if filter.Username == "me" {
filter.UserID = apiKey.UserID
Expand Down
3 changes: 0 additions & 3 deletionscoderd/audit_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -343,9 +343,6 @@ func TestAuditLogsFilter(t *testing.T) {
t.Parallel()
auditLogs, err := client.AuditLogs(ctx, codersdk.AuditLogsRequest{
SearchQuery: testCase.SearchQuery,
Pagination: codersdk.Pagination{
Limit: 25,
},
})
if testCase.ExpectedError {
require.Error(t, err, "expected error")
Expand Down
2 changes: 1 addition & 1 deletioncoderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -263,7 +263,7 @@ func (s *MethodTestSuite) TestAuditLogs() {
_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
_ = dbgen.AuditLog(s.T(), db, database.AuditLog{})
check.Args(database.GetAuditLogsOffsetParams{
Limit: 10,
LimitOpt: 10,
}).Asserts(rbac.ResourceAuditLog, policy.ActionRead)
}))
}
Expand Down
2 changes: 1 addition & 1 deletioncoderd/database/dbgen/dbgen_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -19,7 +19,7 @@ func TestGenerator(t *testing.T) {
t.Parallel()
db := dbmem.New()
_ = dbgen.AuditLog(t, db, database.AuditLog{})
logs := must(db.GetAuditLogsOffset(context.Background(), database.GetAuditLogsOffsetParams{Limit: 1}))
logs := must(db.GetAuditLogsOffset(context.Background(), database.GetAuditLogsOffsetParams{LimitOpt: 1}))
require.Len(t, logs, 1)
})

Expand Down
13 changes: 9 additions & 4 deletionscoderd/database/dbmem/dbmem.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1920,12 +1920,17 @@ func (q *FakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAudi
q.mutex.RLock()
defer q.mutex.RUnlock()

logs := make([]database.GetAuditLogsOffsetRow, 0, arg.Limit)
if arg.LimitOpt == 0 {
// Default to 100 is set in the SQL query.
arg.LimitOpt = 100
}

logs := make([]database.GetAuditLogsOffsetRow, 0, arg.LimitOpt)

// q.auditLogs are already sorted by time DESC, so no need to sort after the fact.
for _, alog := range q.auditLogs {
if arg.Offset > 0 {
arg.Offset--
if arg.OffsetOpt > 0 {
arg.OffsetOpt--
continue
}
if arg.OrganizationID != uuid.Nil && arg.OrganizationID != alog.OrganizationID {
Expand DownExpand Up@@ -2002,7 +2007,7 @@ func (q *FakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAudi
Count: 0,
})

if len(logs) >= int(arg.Limit) {
if len(logs) >= int(arg.LimitOpt) {
break
}
}
Expand Down
23 changes: 23 additions & 0 deletionscoderd/database/querier_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -516,6 +516,29 @@ func TestDefaultOrg(t *testing.T) {
require.True(t, all[0].IsDefault, "first org should always be default")
}

func TestAuditLogDefaultLimit(t *testing.T) {
t.Parallel()
if testing.Short() {
t.SkipNow()
}

sqlDB := testSQLDB(t)
err := migrations.Up(sqlDB)
require.NoError(t, err)
db := database.New(sqlDB)

for i := 0; i < 110; i++ {
dbgen.AuditLog(t, db, database.AuditLog{})
}

ctx := testutil.Context(t, testutil.WaitShort)
rows, err := db.GetAuditLogsOffset(ctx, database.GetAuditLogsOffsetParams{})
require.NoError(t, err)
// The length should match the default limit of the SQL query.
// Updating the sql query requires changing the number below to match.
require.Len(t, rows, 100)
}

// TestReadCustomRoles tests the input params returns the correct set of roles.
func TestReadCustomRoles(t *testing.T) {
t.Parallel()
Expand Down
59 changes: 31 additions & 28 deletionscoderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

7 changes: 5 additions & 2 deletionscoderd/database/queries/auditlogs.sql
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -116,9 +116,12 @@ WHERE
ORDER BY
"time" DESC
LIMIT
$1
-- a limit of 0 means "no limit". The audit log table is unbounded
-- in size, and is expected to be quite large. Implement a default
-- limit of 100 to prevent accidental excessively large queries.
COALESCE(NULLIF(@limit_opt :: int, 0), 100)
OFFSET
$2;
@offset_opt;

-- name: InsertAuditLog :one
INSERT INTO
Expand Down
6 changes: 4 additions & 2 deletionscoderd/pagination.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,8 +17,10 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat
parser := httpapi.NewQueryParamParser()
params := codersdk.Pagination{
AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"),
Limit: int(parser.PositiveInt32(queryParams, 0, "limit")),
Offset: int(parser.PositiveInt32(queryParams, 0, "offset")),
// A limit of 0 should be interpreted by the SQL query as "null" or
// "no limit". Do not make this value anything besides 0.
Limit: int(parser.PositiveInt32(queryParams, 0, "limit")),
Offset: int(parser.PositiveInt32(queryParams, 0, "offset")),
Comment on lines +20 to +23
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Added this comment similar to the-1 one before. Should clear it up going forward 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Doesn't this comment directly contradict what happens inauditlogs.sql?

}
if len(parser.Errors) > 0 {
httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{
Expand Down
4 changes: 2 additions & 2 deletionsenterprise/audit/backends/postgres_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -30,8 +30,8 @@ func TestPostgresBackend(t *testing.T) {
require.NoError(t, err)

got, err := db.GetAuditLogsOffset(ctx, database.GetAuditLogsOffsetParams{
Offset: 0,
Limit: 1,
OffsetOpt: 0,
LimitOpt: 1,
})
require.NoError(t, err)
require.Len(t, got, 1)
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp