Let API create and edit system webhooks #26418

kousu · 2023-08-09T21:50:03Z

"System" webhooks -- webhooks run on all repos on a Gitea instance -- were added in #14537 but managing them by the API is buggy:

PATCH /api/v1/admin/hooks/:id edits either kind (?)
DELETE /api/v1/admin/hooks/:id deletes either kind

but

POST /api/v1/admin/hooks/ creates "default" webhooks
GET /api/v1/admin/hooks/ and GET/api/v1/admin/hooks/:id return "system" webhooks

To fix the confusion:

In routers/api/v1/utils/hook.go, correctly handle the distinction between system and default webhooks. This allows actually creating, editing and deleting both kinds.
In routers/api/, split /api/v1/admin/hooks to /api/v1/admin/hooks/{system,default}.
In routers/web/, move /admin/{system,default}-hooks and most of /admin/hooks/ into /admin/hooks/{system,default} to match API.
In model/, normalize vocabulary. Since the new sub-type, the terminology has been a confusing mix of "SystemWebhook", "DefaultSystemWebhook", "SystemOrDefaultWebhook" and "DefaultWebhook". Standardize on "AdminWebhook" everywhere with a isSystemWebhook bool parameter to separate the two sub-types.
- Using a bool made it easier to handle both cases without duplicating the router endpoints. I hope that's okay.
Make PATCH /admin/hooks/{system,default}/:id return 404 instead of 500 when appropriate to do so.

Fixes #23139.

Supersedes #23142.

kousu · 2023-08-09T21:53:23Z

routers/api/v1/api.go

@@ -1381,7 +1381,7 @@ func Routes() *web.Route {
 				m.Post("/{username}/{reponame}", admin.AdoptRepository)
 				m.Delete("/{username}/{reponame}", admin.DeleteUnadoptedRepository)
 			})
-			m.Group("/hooks", func() {
+			m.Group("/hooks/{configType:system|default}", func() {


This is the core change. Almost everything else is fall-out from making this one line change.

In #23142 (comment), lunny suggested what I added as /api/v1/admin/hooks/default, but also wanted to keep the old behaviour too.

Respectfully, I think we must not. The old behaviour was inconsistent:

GET /hooks -> return system hooks (because it calls GetSystemWebhooks)

POST /hooks -> create a default hook (because isSystemWebhook is missing therefore false).

For "backwards compatibility", we could chose to keep the POST /hooks == POST /hooks/default, but then we would have to break GET /hooks; we could choose to keep GET /hooks == GET /hooks/system, but then we would have to break POST /hooks.

The confused behaviour was in the original version of /api/v1/admin/hooks, so I doubt anyone is using it in the wild -- I was probably the first person to try and immediately hit this bug. Anyone else who tried would have hit it too.

kousu · 2023-08-09T21:54:03Z

routers/api/v1/utils/hook.go

+		URL:             form.Config["url"],
+		ContentType:     webhook.ToHookContentType(form.Config["content_type"]),
+		Secret:          form.Config["secret"],
+		IsSystemWebhook: isSystemWebhook,


This line is what communicates the one line change above into the backend.

routers/web/admin/hooks.go

kousu · 2023-08-11T14:02:04Z

routers/api/v1/utils/hook.go

+		if errors.Is(err, util.ErrNotExist) {
+			ctx.NotFound()


This is what adds the correct 404 to the PATCH method.

kousu · 2023-08-11T14:02:37Z

routers/web/admin/hooks.go

 	if err != nil {
-		ctx.ServerError("GetWebhooksAdmin", err)
+		ctx.ServerError("GetSystemWebhooks", err)


This error message was mislabelled.

kousu · 2023-08-11T14:02:44Z

routers/web/admin/hooks.go

 	if err != nil {
-		ctx.ServerError("GetWebhooksAdmin", err)
+		ctx.ServerError("GetDefaultWebhooks", err)


also mislabelled

kousu · 2023-08-11T14:09:06Z

templates/repo/settings/webhook/base_list.tmpl

@@ -62,7 +62,7 @@
 				<span class="text {{if eq .LastStatus 1}}green{{else if eq .LastStatus 2}}red{{else}}grey{{end}} gt-mr-3">{{svg "octicon-dot-fill" 22}}</span>
 				<a class="text truncate gt-f1 gt-mr-3" title="{{.URL}}" href="{{$.BaseLink}}/{{.ID}}">{{.URL}}</a>
 				<a class="muted gt-p-3" href="{{$.BaseLink}}/{{.ID}}">{{svg "octicon-pencil"}}</a>
-				<a class="delete-button gt-p-3" data-url="{{$.Link}}/delete" data-id="{{.ID}}">{{svg "octicon-trash"}}</a>
+				<a class="delete-button gt-p-3" data-url="{{$.BaseLink}}/delete" data-id="{{.ID}}">{{svg "octicon-trash"}}</a>


This is because POST /admin/hooks/delete was moved to POST /admin/hooks/{configType:system|default}/delete

"System" webhooks -- webhooks run on all repos on a Gitea instance -- were added in go-gitea#14537 (I believe?) but managing them by the API is buggy. - In routers/api/v1/utils/hook.go, correctly handle the distinction between system and default webhooks. This enables actually creating, editing and deleting both kinds. - In routers/api/, move `/api/v1/admin/hooks` to `/api/v1/admin/hooks/{system,default}`. This allows users to access the code in the previous point. - In routers/web/, move `/admin/{system,default}-hooks` and most of `/admin/hooks/` into `/admin/hooks/{system,default}` to match API. - In model/, normalize vocabulary. Since the new sub-type, the terminology has been a confusing mix of "SystemWebhook", "DefaultSystemWebhook", "SystemOrDefaultWebhook" and "DefaultWebhook". Standardize on "AdminWebhook" everywhere with `isSystemWebhook bool` to separate the two sub-types. - Using a bool made it easier to handle both cases without duplicating the router endpoints - Make PATCH /admin/hooks/{system,default}/:id appropriately return 404. Fixes go-gitea#23139. Supersedes go-gitea#23142.

kousu · 2023-10-02T21:43:42Z

I see that there's a lot of other issues competing for attention in https://github.com/go-gitea/gitea/milestone/148. Is there anything I can do to make it easier to review? I'm hoping to help get this in before the original bug hits its first birthday 🍰

lunny · 2023-11-03T13:26:23Z

routers/api/v1/admin/hooks.go

-	//   in: query
-	//   description: page size of results
-	//   type: integer
+	// - name: configType


I think this is a break change and with no deprecated mark?

lunny · 2023-11-03T13:28:09Z

Maybe we can use /admin/hooks as global hooks but /admin/hooks/{configType} as default hooks?
Is that compatible with before? Otherwise, we need a breaking change. For API breaking change, we need to do more things.

philipparndt · 2024-11-27T09:02:49Z

Hi @lunny,

My team would like to have this issue fixed so that it is possible to set up Gitea in an automated way. Is there a good chance to get it merged if we find a way to fix it in a manner that keeps the current API and has some additional API to define the default hook type? Or would you prefer to have a breaking change that also removes the old behavior, like this one? What additional work needs to be done, and how can I (or someone from my scrum team) help fix this bug?

@kousu is this issue still relevant for you?

lunny · 2024-11-27T18:54:57Z

Hi @lunny,

My team would like to have this issue fixed so that it is possible to set up Gitea in an automated way. Is there a good chance to get it merged if we find a way to fix it in a manner that keeps the current API and has some additional API to define the default hook type? Or would you prefer to have a breaking change that also removes the old behavior, like this one? What additional work needs to be done, and how can I (or someone from my scrum team) help fix this bug?

@kousu is this issue still relevant for you?

I would like to continue the review if it can resolve the API-breaking problem. If the breaking is necessary, we at least need to add deprecated for several versions until we remove it.

kousu · 2024-11-28T00:54:37Z

Yes I would like to still see this for the same reason as @philipparndt. But the code has drifted very far now and I suspect it needs a full rewrite, especially if the purpose of the surrounding APIs may have changed. I won't be offended if you close this and submit a replacement.

philipparndt · 2024-11-29T07:17:26Z

All right, I have planned to implement this and let you know when it is ready.

mbollmann-v · 2025-01-03T10:42:55Z

Hi @lunny & @kousu,
Do you have any objections to moving the configType to an optional query parameter (system/default/any)?
This way we should be able to configure the default value on the various endpoints to match the old inconsistent behaviour avoiding a breaking change while also allowing api users to explicitly configure which kind of hook they want to interact with.
It's probably not the prettiest solution but maybe an appropriate tradeoff.

kousu · 2025-01-06T02:39:52Z

Not the prettiest no, but I think if it gets things going then it's good 🌠

mbollmann-v · 2025-01-09T15:26:08Z

Hi @lunny & @kousu,

After reading the source I think I found a simple and compatible way to fix the issue.
I've opened another PR for it at #33180.
I hope that enables us to finally resolve this topic.

wxiaoguang · 2025-01-13T16:54:56Z

Replaced by #33180

This PR fixes inconsistencies between system and default webhooks in the Gitea API. (See also #26418) - A system webhook is a webhook that captures events for all repositories. - A default webhook is copied to a new repository when it is created. Before this PR `POST /api/v1/admin/hooks/` creates default webhooks (if not configured otherwise) and `GET /api/v1/admin/hooks/` returns system webhooks. The PR introduces an optional query parameter to `GET /api/v1/admin/hooks/` to enable selecting if either default, system or both kind of webhooks should be retrieved. By default the flag is set to return system webhooks keep current behaviour. ## Examples ### System Webhooks #### Create ``` POST /api/v1/admin/hooks/ { "type": "gitea", "active": false, "branch_filter": "*", "events": [ "create", "..." ], "config": { "url": "http://...", "content_type": "json", "secret": "secret", "is_system_webhook": true // <-- controls hook type } } ``` #### List ``` GET/api/v1/admin/hooks?type=system //type argument is optional here since it's the default ``` #### Others The other relevant endpoints work as expected by referencing the hook by id ``` GET /api/v1/admin/hooks/:id PATCH /api/v1/admin/hooks/:id DELETE /api/v1/admin/hooks/:id ``` ### Default Webhooks #### Create ``` POST /api/v1/admin/hooks/ { "type": "gitea", "active": false, "branch_filter": "*", "events": [ "create", "..." ], "config": { "url": "http://...", "content_type": "json", "secret": "secret", "is_system_webhook": false // optional, as false is the default value } } ``` #### List ``` GET/api/v1/admin/hooks?type=default ``` #### Others The other relevant endpoints work as expected by referencing the hook by id ``` GET /api/v1/admin/hooks/:id PATCH /api/v1/admin/hooks/:id DELETE /api/v1/admin/hooks/:id ```

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 9, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2023

kousu mentioned this pull request Aug 9, 2023

Allow creating and editing system webhooks. #23142

Closed

kousu commented Aug 9, 2023

View reviewed changes

routers/web/admin/hooks.go Outdated Show resolved Hide resolved

kousu force-pushed the system-webhooks-api-2 branch 5 times, most recently from edebde3 to 9344314 Compare August 11, 2023 13:58

kousu commented Aug 11, 2023

View reviewed changes

kousu force-pushed the system-webhooks-api-2 branch from 9344314 to bfd6258 Compare August 11, 2023 14:06

kousu commented Aug 11, 2023

View reviewed changes

kousu force-pushed the system-webhooks-api-2 branch from bfd6258 to b4f3909 Compare August 11, 2023 14:10

kousu force-pushed the system-webhooks-api-2 branch from b4f3909 to 8d6a9a0 Compare August 11, 2023 14:24

kousu marked this pull request as ready for review August 11, 2023 15:31

lunny added the modifies/api This PR adds API routes or modifies them label Aug 12, 2023

lunny added this to the 1.21.0 milestone Aug 12, 2023

Merge branch 'main' into system-webhooks-api-2

606e5da

lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023

Merge branch 'main' into system-webhooks-api-2

0e251f6

Merge branch 'main' into system-webhooks-api-2

6d1b6fa

lunny reviewed Nov 3, 2023

View reviewed changes

lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024

lunny removed this from the 1.23.0 milestone Sep 24, 2024

mbollmann-v mentioned this pull request Jan 9, 2025

Let API create and edit system webhooks, attempt 2 #33180

Merged

wxiaoguang closed this Jan 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let API create and edit system webhooks #26418

Let API create and edit system webhooks #26418

kousu commented Aug 9, 2023 •

edited

Loading

kousu Aug 9, 2023

kousu Aug 11, 2023 •

edited

Loading

kousu Aug 9, 2023

kousu Aug 11, 2023

kousu Aug 11, 2023

kousu Aug 11, 2023

kousu Aug 11, 2023

kousu commented Oct 2, 2023 •

edited

Loading

lunny Nov 3, 2023

lunny commented Nov 3, 2023

philipparndt commented Nov 27, 2024

lunny commented Nov 27, 2024

kousu commented Nov 28, 2024 •

edited

Loading

philipparndt commented Nov 29, 2024

mbollmann-v commented Jan 3, 2025

kousu commented Jan 6, 2025

mbollmann-v commented Jan 9, 2025

wxiaoguang commented Jan 13, 2025

Let API create and edit system webhooks #26418

Let API create and edit system webhooks #26418

Conversation

kousu commented Aug 9, 2023 • edited Loading

kousu Aug 9, 2023

Choose a reason for hiding this comment

kousu Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

kousu Aug 9, 2023

Choose a reason for hiding this comment

kousu Aug 11, 2023

Choose a reason for hiding this comment

kousu Aug 11, 2023

Choose a reason for hiding this comment

kousu Aug 11, 2023

Choose a reason for hiding this comment

kousu Aug 11, 2023

Choose a reason for hiding this comment

kousu commented Oct 2, 2023 • edited Loading

lunny Nov 3, 2023

Choose a reason for hiding this comment

lunny commented Nov 3, 2023

philipparndt commented Nov 27, 2024

lunny commented Nov 27, 2024

kousu commented Nov 28, 2024 • edited Loading

philipparndt commented Nov 29, 2024

mbollmann-v commented Jan 3, 2025

kousu commented Jan 6, 2025

mbollmann-v commented Jan 9, 2025

wxiaoguang commented Jan 13, 2025

kousu commented Aug 9, 2023 •

edited

Loading

kousu Aug 11, 2023 •

edited

Loading

kousu commented Oct 2, 2023 •

edited

Loading

kousu commented Nov 28, 2024 •

edited

Loading