[REFACTOR] store webhook event in database

[oliverpool]

Refactor the webhook logic, to have the type-dependent processing happen only in one place.

---

Current webhook flow

1. An event happens
2. It is pre-processed (depending on the webhook type) and its body is added to a task queue
3. When the task is processed, some more logic (depending on the webhook type as well) is applied to make an HTTP request

This means that webhook-type dependant logic is needed in step 2 and 3. This is cumbersome and brittle to maintain.

Updated webhook flow with this PR:

1. An event happens
2. It is stored as-is and added to a task queue
3. When the task is processed, the event is processed (depending on the webhook type) to make an HTTP request

So the only webhook-type dependent logic happens in one place (step 3) which should be much more robust.

Consequences of the refactor

• the raw event must be stored in the hooktask (until now, the pre-processed body was stored)
• to ensure that previous hooktasks are correctly sent, a payload_version is added (version 1: the body has already been pre-process / version 2: the body is the raw event)

So future webhook additions will only have to deal with creating an http.Request based on the raw event (no need to adjust the code in multiple places, like currently).

Moreover since this processing happens when fetching from the task queue, it ensures that the queuing of new events (upon a git push for instance) does not get slowed down by a slow webhook.

As a concrete example, the PR #19307 for custom webhooks, should be substantially smaller:

• no need to change services/webhook/deliver.go
• minimal change in services/webhook/webhook.go (add the new webhook to the map)
• no need to change all the individual webhook files (since with this refactor the *webhook_model.Webhook is provided as argument)

[oliverpool]

@jolheiser do you know if/when you will have some time to look at this PR? (especially since it should reduce the changes needed for #19307)

[oliverpool]

@silverwind @KN4CK3R thank you for your initial comments. Do you have any other feedback?

Do you think that there is a chance for this PR to make it before 1.22 ? (cc @delvh)

[delvh]

If we merge it this week, then yes.

[silverwind]

I'm not well versed around these parts but what is the deal with this PayloadVersion? Is it a breaking change in the webhook payload?

[lunny]

Could you have more description on the issue body about the plan? Why you add the new version column and how will the new version support customized webhooks in the future?

[oliverpool]

@silverwind & @lunny I haved updated my initial description to emphasize:

• the difference in the logic for processing webhook events
• the consequences it should have on the code (and how it should simplify the PR WIP: Custom Webhooks #19307 by @jolheiser)

Does it addresses your concerns?

[delvh]

What I haven't understood yet:
You introduce the new version, 2.
However, I don't see this version being set anywhere, except in the tests.
Did I miss something?

[oliverpool]

The version 1 is set to the hooktask which were generated before the upgrade (set to 1 in the migration, as you suggested).
Either the hooktask was not sent before the upgrade or the user requested an old hooktask to be sent again.

So the "new" codebase should only create version 2 hooktasks and after a while, no more version 1 should be present in the database.

[delvh]

I think we are talking about different things:

I expected to see the version 2 being set somewhere (apart from the tests).
And I don't see it anywhere.

So the "new" codebase should only create version 2 hooktask

Yes, that's exactly what I thought.
However, is that the job of other PRs?
Because I expected that it is done here…

[oliverpool]

@delvh it is done upon hooktask creation in services/webhook/webhook.go:172

webhook_model.CreateHookTask(ctx, &webhook_model.HookTask{
		HookID:         w.ID,
		PayloadContent: string(payload),
		EventType:      event,
		PayloadVersion: 2,
	})

[delvh]

Now it makes sense why I don't find anything when I'm searching for [vV]ersion.*=.*2… 🤦

[jolheiser]

Overall looks good from initial look. I'll try and take some time to toy around with it more.

[oliverpool]

As soon as there is willingness for this PR to be merged, I can solve the merge conflicts.

[6543]

As soon as there is willingness for this PR to be merged, I can solve the merge conflicts.

I guess that would be now

[techknowlogick]

I brought this up on the TOC call, and how this PR would be great to get in, Im just wondering if it'll still include a raw copy of the body and headers that are sent to the endpoints? As having that information has been helpful to be able to copy/paste it into a curl to debug issues.

(Not blocking getting this in, I'm just asking to make sure no existing functionality is removed by this PR)

[jolheiser]

Yes, that part appears to be largely unchanged https://github.com/go-gitea/gitea/pull/29145/files#diff-2402142c5412a93d0288dee23e4fa6d8d5b1e8a07b627714820a2344ff205125R161

[oliverpool]

if it'll still include a raw copy of the body and headers that are sent to the endpoints

Yes it does, a little bit adapted, because the body must be saved as well (since the payloadBody is now the raw event):

	// Record delivery information.
	t.RequestInfo = &webhook_model.HookRequest{
		URL:        req.URL.String(),
		HTTPMethod: req.Method,
		Headers:    map[string]string{},
		Body:       string(body),
	}
	for k, vals := range req.Header {
		t.RequestInfo.Headers[k] = strings.Join(vals, ",")
	}

I have solved the merge conflicts.