From 761b9d439b9c1f1bfe57e31ec74f7cf54cd4aef8 Mon Sep 17 00:00:00 2001 From: silverwind Date: Mon, 2 Mar 2026 23:08:53 +0100 Subject: [PATCH] Fix API not persisting pull request unit config when has_pull_requests is not set (#36718) The `PATCH /api/v1/repos/{owner}/{repo}` endpoint silently ignores pull request config fields (like `default_delete_branch_after_merge`, `allow_squash_merge`, etc.) unless `has_pull_requests: true` is also included in the request body. This is because the entire PR unit config block was gated behind `if opts.HasPullRequests != nil`. This PR restructures the logic so that PR config options are applied whenever the pull request unit already exists on the repo, without requiring `has_pull_requests` to be explicitly set. A new unit is only created when `has_pull_requests: true` is explicitly sent. Fixes https://github.com/go-gitea/gitea/issues/36466 Co-authored-by: Claude Opus 4.6 Co-authored-by: wxiaoguang Co-authored-by: Giteabot --- models/repo/repo_unit.go | 32 ++++--- modules/optional/option.go | 14 ++++ routers/api/v1/repo/repo.go | 107 ++++++++---------------- routers/web/repo/issue_view.go | 5 +- services/convert/repository.go | 2 +- services/repository/create.go | 10 +-- tests/integration/api_pull_test.go | 10 +-- tests/integration/api_repo_edit_test.go | 15 ++++ 8 files changed, 94 insertions(+), 101 deletions(-) diff --git a/models/repo/repo_unit.go b/models/repo/repo_unit.go index 1058a18a85..491e96770c 100644 --- a/models/repo/repo_unit.go +++ b/models/repo/repo_unit.go @@ -134,10 +134,25 @@ type PullRequestsConfig struct { DefaultTargetBranch string } +func DefaultPullRequestsConfig() *PullRequestsConfig { + cfg := &PullRequestsConfig{ + AllowMerge: true, + AllowRebase: true, + AllowRebaseMerge: true, + AllowSquash: true, + AllowFastForwardOnly: true, + AllowRebaseUpdate: true, + DefaultAllowMaintainerEdit: true, + } + cfg.DefaultMergeStyle = MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle) + cfg.DefaultMergeStyle = util.IfZero(cfg.DefaultMergeStyle, MergeStyleMerge) + return cfg +} + // FromDB fills up a PullRequestsConfig from serialized format. func (cfg *PullRequestsConfig) FromDB(bs []byte) error { - // AllowRebaseUpdate = true as default for existing PullRequestConfig in DB - cfg.AllowRebaseUpdate = true + // set default values for existing PullRequestConfig in DB + *cfg = *DefaultPullRequestsConfig() return json.UnmarshalHandleDoubleEncode(bs, &cfg) } @@ -156,17 +171,8 @@ func (cfg *PullRequestsConfig) IsMergeStyleAllowed(mergeStyle MergeStyle) bool { mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge } -// GetDefaultMergeStyle returns the default merge style for this pull request -func (cfg *PullRequestsConfig) GetDefaultMergeStyle() MergeStyle { - if len(cfg.DefaultMergeStyle) != 0 { - return cfg.DefaultMergeStyle - } - - if setting.Repository.PullRequest.DefaultMergeStyle != "" { - return MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle) - } - - return MergeStyleMerge +func DefaultPullRequestsUnit(repoID int64) RepoUnit { + return RepoUnit{RepoID: repoID, Type: unit.TypePullRequests, Config: DefaultPullRequestsConfig()} } type ActionsConfig struct { diff --git a/modules/optional/option.go b/modules/optional/option.go index cbecf86987..a278723bef 100644 --- a/modules/optional/option.go +++ b/modules/optional/option.go @@ -67,3 +67,17 @@ func ParseBool(s string) Option[bool] { } return Some(v) } + +func AssignPtrValue[T comparable](changed *bool, target, src *T) { + if src != nil && *src != *target { + *target = *src + *changed = true + } +} + +func AssignPtrString[TO, FROM ~string](changed *bool, target *TO, src *FROM) { + if src != nil && string(*src) != string(*target) { + *target = TO(*src) + *changed = true + } +} diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index bb6bda587d..cfdcf7b374 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -884,77 +884,44 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { } } - if opts.HasPullRequests != nil && !unit_model.TypePullRequests.UnitGlobalDisabled() { - if *opts.HasPullRequests { - // We do allow setting individual PR settings through the API, so - // we get the config settings and then set them - // if those settings were provided in the opts. - unit, err := repo.GetUnit(ctx, unit_model.TypePullRequests) - var config *repo_model.PullRequestsConfig - if err != nil { - // Unit type doesn't exist so we make a new config file with default values - config = &repo_model.PullRequestsConfig{ - IgnoreWhitespaceConflicts: false, - AllowMerge: true, - AllowRebase: true, - AllowRebaseMerge: true, - AllowSquash: true, - AllowFastForwardOnly: true, - AllowManualMerge: true, - AutodetectManualMerge: false, - AllowRebaseUpdate: true, - DefaultDeleteBranchAfterMerge: false, - DefaultMergeStyle: repo_model.MergeStyleMerge, - DefaultAllowMaintainerEdit: false, - } - } else { - config = unit.PullRequestsConfig() - } - - if opts.IgnoreWhitespaceConflicts != nil { - config.IgnoreWhitespaceConflicts = *opts.IgnoreWhitespaceConflicts - } - if opts.AllowMerge != nil { - config.AllowMerge = *opts.AllowMerge - } - if opts.AllowRebase != nil { - config.AllowRebase = *opts.AllowRebase - } - if opts.AllowRebaseMerge != nil { - config.AllowRebaseMerge = *opts.AllowRebaseMerge - } - if opts.AllowSquash != nil { - config.AllowSquash = *opts.AllowSquash - } - if opts.AllowFastForwardOnly != nil { - config.AllowFastForwardOnly = *opts.AllowFastForwardOnly - } - if opts.AllowManualMerge != nil { - config.AllowManualMerge = *opts.AllowManualMerge - } - if opts.AutodetectManualMerge != nil { - config.AutodetectManualMerge = *opts.AutodetectManualMerge - } - if opts.AllowRebaseUpdate != nil { - config.AllowRebaseUpdate = *opts.AllowRebaseUpdate - } - if opts.DefaultDeleteBranchAfterMerge != nil { - config.DefaultDeleteBranchAfterMerge = *opts.DefaultDeleteBranchAfterMerge - } - if opts.DefaultMergeStyle != nil { - config.DefaultMergeStyle = repo_model.MergeStyle(*opts.DefaultMergeStyle) - } - if opts.DefaultAllowMaintainerEdit != nil { - config.DefaultAllowMaintainerEdit = *opts.DefaultAllowMaintainerEdit - } - - units = append(units, repo_model.RepoUnit{ - RepoID: repo.ID, - Type: unit_model.TypePullRequests, - Config: config, - }) - } else { + if !unit_model.TypePullRequests.UnitGlobalDisabled() { + mustDeletePullRequestUnit := opts.HasPullRequests != nil && !*opts.HasPullRequests + mustInsertPullRequestUnit := opts.HasPullRequests != nil && *opts.HasPullRequests + if mustDeletePullRequestUnit { deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests) + } else { + // We do allow setting individual PR settings through the API, + // so we get the config settings and then set them if those settings were provided in the opts. + unit, err := repo.GetUnit(ctx, unit_model.TypePullRequests) + if err != nil && !errors.Is(err, util.ErrNotExist) { + return err + } + if unit == nil { + // Unit doesn't exist yet but is being enabled, create with defaults + unit = new(repo_model.DefaultPullRequestsUnit(repo.ID)) + } + + changed := new(false) + config := unit.PullRequestsConfig() + optional.AssignPtrValue(changed, &config.IgnoreWhitespaceConflicts, opts.IgnoreWhitespaceConflicts) + optional.AssignPtrValue(changed, &config.AllowMerge, opts.AllowMerge) + optional.AssignPtrValue(changed, &config.AllowRebase, opts.AllowRebase) + optional.AssignPtrValue(changed, &config.AllowRebaseMerge, opts.AllowRebaseMerge) + optional.AssignPtrValue(changed, &config.AllowSquash, opts.AllowSquash) + optional.AssignPtrValue(changed, &config.AllowFastForwardOnly, opts.AllowFastForwardOnly) + optional.AssignPtrValue(changed, &config.AllowManualMerge, opts.AllowManualMerge) + optional.AssignPtrValue(changed, &config.AutodetectManualMerge, opts.AutodetectManualMerge) + optional.AssignPtrValue(changed, &config.AllowRebaseUpdate, opts.AllowRebaseUpdate) + optional.AssignPtrValue(changed, &config.DefaultDeleteBranchAfterMerge, opts.DefaultDeleteBranchAfterMerge) + optional.AssignPtrValue(changed, &config.DefaultAllowMaintainerEdit, opts.DefaultAllowMaintainerEdit) + optional.AssignPtrString(changed, &config.DefaultMergeStyle, opts.DefaultMergeStyle) + if *changed || mustInsertPullRequestUnit { + units = append(units, repo_model.RepoUnit{ + RepoID: repo.ID, + Type: unit_model.TypePullRequests, + Config: config, + }) + } } } diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 1354c2d6f9..2cd8be4533 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -905,9 +905,8 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss // Check correct values and select default if ms, ok := ctx.Data["MergeStyle"].(repo_model.MergeStyle); !ok || !prConfig.IsMergeStyleAllowed(ms) { - defaultMergeStyle := prConfig.GetDefaultMergeStyle() - if prConfig.IsMergeStyleAllowed(defaultMergeStyle) && !ok { - mergeStyle = defaultMergeStyle + if prConfig.IsMergeStyleAllowed(prConfig.DefaultMergeStyle) && !ok { + mergeStyle = prConfig.DefaultMergeStyle } else if prConfig.AllowMerge { mergeStyle = repo_model.MergeStyleMerge } else if prConfig.AllowRebase { diff --git a/services/convert/repository.go b/services/convert/repository.go index 658d31d55c..3c9cc83ccb 100644 --- a/services/convert/repository.go +++ b/services/convert/repository.go @@ -118,7 +118,7 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR allowManualMerge = config.AllowManualMerge autodetectManualMerge = config.AutodetectManualMerge defaultDeleteBranchAfterMerge = config.DefaultDeleteBranchAfterMerge - defaultMergeStyle = config.GetDefaultMergeStyle() + defaultMergeStyle = config.DefaultMergeStyle defaultAllowMaintainerEdit = config.DefaultAllowMaintainerEdit defaultTargetBranch = config.DefaultTargetBranch } diff --git a/services/repository/create.go b/services/repository/create.go index e027d3b979..a8b57b6707 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -386,15 +386,7 @@ func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r }, }) case unit.TypePullRequests: - units = append(units, repo_model.RepoUnit{ - RepoID: repo.ID, - Type: tp, - Config: &repo_model.PullRequestsConfig{ - AllowMerge: true, AllowRebase: true, AllowRebaseMerge: true, AllowSquash: true, AllowFastForwardOnly: true, - DefaultMergeStyle: repo_model.MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle), - AllowRebaseUpdate: true, - }, - }) + units = append(units, repo_model.DefaultPullRequestsUnit(repo.ID)) case unit.TypeProjects: units = append(units, repo_model.RepoUnit{ RepoID: repo.ID, diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index 61a37ddd01..fcd3554c52 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -279,10 +279,10 @@ func TestAPICreatePullSuccess(t *testing.T) { }).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) - // Also test that AllowMaintainerEdit is false by default + // Also test that AllowMaintainerEdit is true by default, the "false" case is covered by TestAPICreatePullBasePermission prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID}) - assert.False(t, pr.AllowMaintainerEdit) + assert.True(t, pr.AllowMaintainerEdit) MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail } @@ -304,7 +304,7 @@ func TestAPICreatePullBasePermission(t *testing.T) { Base: "master", Title: prTitle, - AllowMaintainerEdit: new(true), + AllowMaintainerEdit: new(false), } req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token) MakeRequest(t, req, http.StatusForbidden) @@ -317,10 +317,10 @@ func TestAPICreatePullBasePermission(t *testing.T) { req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token) MakeRequest(t, req, http.StatusCreated) - // Also test that AllowMaintainerEdit is set to true + // Also test that AllowMaintainerEdit is set to false, the default "true" case is covered by TestAPICreatePullSuccess prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle}) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID}) - assert.True(t, pr.AllowMaintainerEdit) + assert.False(t, pr.AllowMaintainerEdit) } func TestAPICreatePullHeadPermission(t *testing.T) { diff --git a/tests/integration/api_repo_edit_test.go b/tests/integration/api_repo_edit_test.go index 34d6990497..215b48b64b 100644 --- a/tests/integration/api_repo_edit_test.go +++ b/tests/integration/api_repo_edit_test.go @@ -418,5 +418,20 @@ func TestAPIRepoEdit(t *testing.T) { req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name), &repoEditOption). AddTokenAuth(token4) MakeRequest(t, req, http.StatusForbidden) + + // Test updating pull request settings without setting has_pull_requests + repo1 = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + url = fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name) + req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{ + DefaultDeleteBranchAfterMerge: &bTrue, + }).AddTokenAuth(token2) + resp = MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &repo) + assert.True(t, repo.DefaultDeleteBranchAfterMerge) + // reset + req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{ + DefaultDeleteBranchAfterMerge: &bFalse, + }).AddTokenAuth(token2) + _ = MakeRequest(t, req, http.StatusOK) }) }