fix: paginate GET /api/v1/admin/hooks response (#9915)

Fixes #9911

The endpoint was documented as taking `page` and `limit` parameters but did not actually use then and just returned the full list. Now it does use them!

Co-authored-by: Gusted <postmaster@gusted.xyz>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9915
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Cyborus <cyborus@cyborus.xyz>
Co-committed-by: Cyborus <cyborus@cyborus.xyz>

models/webhook/fixtures/TestListDefaultWebhooks/webhook.yml

@@ -0,0 +1,66 @@
+-
+  id: 5
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook5
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: false
+-
+  id: 6
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook6
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: false
+-
+  id: 7
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook7
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: false
+-
+  id: 8
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook8
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: false
+-
+  id: 9
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook9
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: false
+-
+  id: 10
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook10
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: true

models/webhook/fixtures/TestListSystemWebhooks/webhook.yml

@@ -0,0 +1,66 @@
+-
+  id: 5
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook5
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: true
+-
+  id: 6
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook6
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: true
+-
+  id: 7
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook7
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: true
+  is_system_webhook: true
+-
+  id: 8
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook8
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: true
+-
+  id: 9
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook9
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: true
+-
+  id: 10
+  repo_id: 0
+  owner_id: 0
+  url: http://www.example.com/systemhook
+  http_method: POST
+  type: forgejo
+  content_type: 1 # json
+  events: '{"push_only":true,"send_everything":false,"choose_events":false,"events":{"create":false,"push":true,"pull_request":false}}'
+  is_active: false
+  is_system_webhook: false

models/webhook/webhook_system.go

@@ -11,8 +11,8 @@ import (
 )
 
 // GetDefaultWebhooks returns all admin-default webhooks.
-func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, error) {
-	return getAdminWebhooks(ctx, false)
+func GetDefaultWebhooks(ctx context.Context) ([]*Webhook, int64, error) {
+	return getAdminWebhooks(ctx, false, db.ListOptions{ListAll: true})
 }
 
 // GetSystemOrDefaultWebhook returns admin system or default webhook by given ID.
@@ -30,22 +30,24 @@ func GetSystemOrDefaultWebhook(ctx context.Context, id int64) (*Webhook, error)
 }
 
 // GetSystemWebhooks returns all admin system webhooks.
-func GetSystemWebhooks(ctx context.Context, onlyActive bool) ([]*Webhook, error) {
-	return getAdminWebhooks(ctx, true, onlyActive)
+func GetSystemWebhooks(ctx context.Context, listOptions db.ListOptions, onlyActive bool) ([]*Webhook, int64, error) {
+	return getAdminWebhooks(ctx, true, listOptions, onlyActive)
 }
 
-func getAdminWebhooks(ctx context.Context, systemWebhooks bool, onlyActive ...bool) ([]*Webhook, error) {
+func getAdminWebhooks(ctx context.Context, systemWebhooks bool, listOptions db.ListOptions, onlyActive ...bool) ([]*Webhook, int64, error) {
 	webhooks := make([]*Webhook, 0, 5)
+	sess := db.GetEngine(ctx).
+		Where("repo_id=?", 0).
+		And("owner_id=?", 0).
+		And("is_system_webhook=?", systemWebhooks)
 	if len(onlyActive) > 0 && onlyActive[0] {
-		return webhooks, db.GetEngine(ctx).
-			Where("repo_id=? AND owner_id=? AND is_system_webhook=? AND is_active = ?", 0, 0, systemWebhooks, true).
-			OrderBy("id").
-			Find(&webhooks)
+		sess = sess.And("is_active=?", true)
 	}
-	return webhooks, db.GetEngine(ctx).
-		Where("repo_id=? AND owner_id=? AND is_system_webhook=?", 0, 0, systemWebhooks).
-		OrderBy("id").
-		Find(&webhooks)
+	if listOptions.Page > 0 {
+		sess = db.SetSessionPagination(sess, &listOptions)
+	}
+	total, err := sess.OrderBy("id").FindAndCount(&webhooks)
+	return webhooks, total, err
 }
 
 // DeleteDefaultSystemWebhook deletes an admin-configured default or system webhook (where Org and Repo ID both 0)
@@ -67,7 +69,7 @@ func DeleteDefaultSystemWebhook(ctx context.Context, id int64) error {
 
 // CopyDefaultWebhooksToRepo creates copies of the default webhooks in a new repo
 func CopyDefaultWebhooksToRepo(ctx context.Context, repoID int64) error {
-	ws, err := GetDefaultWebhooks(ctx)
+	ws, _, err := GetDefaultWebhooks(ctx)
 	if err != nil {
 		return fmt.Errorf("GetDefaultWebhooks: %v", err)
 	}

models/webhook/webhook_system_test.go

@@ -0,0 +1,65 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package webhook
+
+import (
+	"testing"
+
+	"forgejo.org/models/db"
+	"forgejo.org/models/unittest"
+
+	"github.com/stretchr/testify/require"
+	"xorm.io/builder"
+)
+
+func TestListSystemWebhooks(t *testing.T) {
+	defer unittest.OverrideFixtures("models/webhook/fixtures/TestListSystemWebhooks")()
+	require.NoError(t, unittest.PrepareTestDatabase())
+	testGetAdminWebhooks(t, true)
+}
+
+func TestListDefaultWebhooks(t *testing.T) {
+	defer unittest.OverrideFixtures("models/webhook/fixtures/TestListDefaultWebhooks")()
+	require.NoError(t, unittest.PrepareTestDatabase())
+	testGetAdminWebhooks(t, false)
+}
+
+func testGetAdminWebhooks(t *testing.T, systemHooks bool) {
+	hookCond := builder.Eq{"is_system_webhook": systemHooks}.And(builder.Eq{"repo_id": 0})
+	unittest.AssertCountByCond(t, "webhook", hookCond, 5)
+
+	t.Run("All hooks (Implicit)", func(t *testing.T) {
+		hooks, count, err := getAdminWebhooks(t.Context(), systemHooks, db.ListOptionsAll)
+		require.NoError(t, err)
+		unittest.AssertCountByCond(t, "webhook", hookCond, int(count))
+		require.Len(t, hooks, int(count))
+		require.Equal(t, 5, int(count))
+		for i := range hooks {
+			require.Equal(t, systemHooks, hooks[i].IsSystemWebhook)
+		}
+	})
+
+	t.Run("All hooks (Explicit)", func(t *testing.T) {
+		hooks, count, err := getAdminWebhooks(t.Context(), systemHooks, db.ListOptionsAll, false)
+		require.NoError(t, err)
+		unittest.AssertCountByCond(t, "webhook", hookCond, int(count))
+		require.Len(t, hooks, int(count))
+		require.Equal(t, 5, int(count))
+		for i := range hooks {
+			require.Equal(t, systemHooks, hooks[i].IsSystemWebhook)
+		}
+	})
+
+	t.Run("Active hooks", func(t *testing.T) {
+		hooks, count, err := getAdminWebhooks(t.Context(), systemHooks, db.ListOptionsAll, true)
+		require.NoError(t, err)
+		unittest.AssertCountByCond(t, "webhook", hookCond.And(builder.Eq{"is_active": true}), int(count))
+		require.Len(t, hooks, int(count))
+		require.Equal(t, 3, int(count))
+		for i := range hooks {
+			require.Equal(t, systemHooks, hooks[i].IsSystemWebhook)
+			require.True(t, hooks[i].IsActive)
+		}
+	})
+}

routers/api/v1/admin/hooks.go

@@ -37,7 +37,7 @@ func ListHooks(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/HookList"
 
-	sysHooks, err := webhook.GetSystemWebhooks(ctx, false)
+	sysHooks, total, err := webhook.GetSystemWebhooks(ctx, utils.GetListOptions(ctx), false)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "GetSystemWebhooks", err)
 		return
@@ -51,6 +51,7 @@ func ListHooks(ctx *context.APIContext) {
 		}
 		hooks[i] = h
 	}
+	ctx.SetTotalCountHeader(total)
 	ctx.JSON(http.StatusOK, hooks)
 }
 

routers/web/admin/hooks.go

@@ -6,6 +6,7 @@ package admin
 import (
 	"net/http"
 
+	"forgejo.org/models/db"
 	"forgejo.org/models/webhook"
 	"forgejo.org/modules/base"
 	"forgejo.org/modules/setting"
@@ -35,7 +36,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 
 	sys["Title"] = ctx.Tr("admin.systemhooks")
 	sys["Description"] = ctx.Tr("admin.systemhooks.desc", "https://forgejo.org/docs/latest/user/webhooks/")
-	sys["Webhooks"], err = webhook.GetSystemWebhooks(ctx, false)
+	sys["Webhooks"], _, err = webhook.GetSystemWebhooks(ctx, db.ListOptions{ListAll: true}, false)
 	sys["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 	sys["BaseLinkNew"] = setting.AppSubURL + "/admin/system-hooks"
 	sys["WebhookList"] = webhook_service.List()
@@ -46,7 +47,7 @@ func DefaultOrSystemWebhooks(ctx *context.Context) {
 
 	def["Title"] = ctx.Tr("admin.defaulthooks")
 	def["Description"] = ctx.Tr("admin.defaulthooks.desc", "https://forgejo.org/docs/latest/user/webhooks/")
-	def["Webhooks"], err = webhook.GetDefaultWebhooks(ctx)
+	def["Webhooks"], _, err = webhook.GetDefaultWebhooks(ctx)
 	def["BaseLink"] = setting.AppSubURL + "/admin/hooks"
 	def["BaseLinkNew"] = setting.AppSubURL + "/admin/default-hooks"
 	def["WebhookList"] = webhook_service.List()

services/webhook/webhook.go

@@ -241,7 +241,7 @@ func PrepareWebhooks(ctx context.Context, source EventSource, event webhook_modu
 	}
 
 	// Add any admin-defined system webhooks
-	systemHooks, err := webhook_model.GetSystemWebhooks(ctx, true)
+	systemHooks, _, err := webhook_model.GetSystemWebhooks(ctx, db.ListOptions{ListAll: true}, true)
 	if err != nil {
 		return fmt.Errorf("GetSystemWebhooks: %w", err)
 	}

tests/integration/api_admin_test.go

@@ -541,3 +541,29 @@ func TestAPIAdminDeleteUserEmailsMultiple(t *testing.T) {
 	assert.Equal(t, "user1@example.com", remainingEmails[0].Email, "Only primary email should remain")
 	assert.True(t, remainingEmails[0].Primary, "Remaining email should be primary")
 }
+
+func TestAPIAdminListHooksPagination(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	token := getUserToken(t, "user1", auth_model.AccessTokenScopeWriteAdmin)
+
+	for i := range 20 {
+		req := NewRequestWithJSON(t, "POST", "/api/v1/admin/hooks", api.CreateHookOption{
+			Config: api.CreateHookOptionConfig{
+				"url":               fmt.Sprintf("http://localhost/hook-%d", i),
+				"content_type":      "json",
+				"is_system_webhook": "true",
+			},
+			Type: "forgejo",
+		}).AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusCreated)
+	}
+
+	req := NewRequest(t, "GET", "/api/v1/admin/hooks?page=1&limit=10").AddTokenAuth(token)
+	resp := MakeRequest(t, req, http.StatusOK)
+	var hooksList []api.Hook
+	t.Logf("got response %s", resp.Body)
+	DecodeJSON(t, resp, &hooksList)
+	assert.Len(t, hooksList, 10, "page length should equal `limit` param")
+	assert.Equal(t, "20", resp.Header().Get("X-Total-Count"))
+}