Skip to content

Conversation

kerwin612
Copy link
Member

This PR adds webhook payload size optimization options to help reduce payload size for large commits or pushes with many commits. This feature is particularly useful for target URLs that restrict payload size or have performance problems handling large JSON parsing.
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 19, 2025
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Jul 19, 2025
@wxiaoguang
Copy link
Contributor

It should be more flexible to use "limit number" instead of "bool"

@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch 6 times, most recently from f249f67 to 47c395d Compare July 23, 2025 08:09
@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from 47c395d to 5472d66 Compare July 23, 2025 08:37
@kerwin612
Copy link
Member Author

kerwin612 commented Jul 23, 2025

It should be more flexible to use "limit number" instead of "bool"

image Is this OK?

@kerwin612 kerwin612 requested a review from lunny July 30, 2025 04:39
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 30, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 30, 2025

  1. IIRC all other places use "-1" for "no-limit" when "0" is used for "limit to zero".
    • For example: the limit of the repo number
  2. Maybe we need a JSON column but not many new separate columns.
  3. There are some issues asking about "commit order", so some users want the first N commits, while some other users want the last N commits

@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Jul 30, 2025
@kerwin612
Copy link
Member Author

image

@kerwin612 kerwin612 force-pushed the feature/webhook-payload-optimization branch from 5e828e9 to 258d8ee Compare August 4, 2025 07:07
@kerwin612 kerwin612 requested a review from wxiaoguang August 24, 2025 14:02
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 28, 2025

func AddWebhookPayloadOptimizationColumns(x *xorm.Engine) error {
type Webhook struct {
MetaSettings string `xorm:"meta_settings TEXT"`
Copy link
Contributor

Choose a reason for hiding this comment

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

XORM natively supports JSON type IIRC

}

// getPayloadConfigLimit extracts the "limit" integer value from a payload config map
func getPayloadConfigLimit(m map[string]any) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems fragile, I don't think we need these tricks to parse form input

Comment on lines +1312 to +1315
"files": map[string]any{
"enable": true,
"limit": 2,
},
Copy link
Contributor

@wxiaoguang wxiaoguang Aug 28, 2025

Choose a reason for hiding this comment

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

What does "enable" mean?

If "enable=false", then "no files" or "no commits"? Or "enable=false" means "disable the limit"?

@wxiaoguang wxiaoguang marked this pull request as draft August 28, 2025 03:39
Co-authored-by: wxiaoguang <[email protected]>
Signed-off-by: Kerwin Bryant <[email protected]>
@lunny lunny modified the milestones: 1.25.0, 1.26.0 Sep 18, 2025
…win612/gitea into kerwin612-feature/webhook-payload-optimization
@github-actions github-actions bot added modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/internal modifies/dependencies labels Oct 15, 2025
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

JS changes look reasonable

@wxiaoguang
Copy link
Contributor

JS changes look reasonable

Actually no, there are other webhook pages like page-content admin settings new webhook, the JS code won't work.

The code is still fragile and needs to be improved (not only JS, but also the problems I commented above)

@silverwind
Copy link
Member

But it's matching on .page-content.repository.settings.webhook which is unique, right?

@wxiaoguang
Copy link
Contributor

But it's matching on .page-content.repository.settings.webhook which is unique, right?

All webhook pages use the same template. All pages need the JS. unique is wrong.

@silverwind
Copy link
Member

silverwind commented Oct 16, 2025

Hmm, we should probably split the webhook settings template into two files with distinct classes, possibly sharing parts via subtemplates.

@silverwind silverwind self-requested a review October 16, 2025 04:43
@silverwind
Copy link
Member

BTW instead of these class queries, checking the URL would be more accurate and not have issues when templates are re-used.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 16, 2025

Hmm, we should probably split the webhook settings template into two files with distinct classes, possibly sharing parts via subtemplates.

They already used shared templates. The page-content admin settings new webhook is the page container's class, and it is abused, that why I said it is wrong.


BTW instead of these class queries, checking the URL would be more accurate and not have issues when templates are re-used.

I believe "checking the URL" is more fragile and infeasible.

Actually our data-global-init framework should already be able to handle this case properly.

<input data-global-init="checked-switch-display" data-checked-true-target=".some-elements" data-checked-false-target=".some-other-elements">

And it also helps #35129 (comment) to make code more maintainable and readable.

<input data-global-init="checked-switch-enabled" data-checked-true-target=".some-elements" data-checked-false-target=".some-other-elements">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/dependencies modifies/frontend modifies/go Pull requests that update Go code modifies/internal modifies/migrations modifies/templates This PR modifies the template files modifies/translation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants