mirror of
https://github.com/go-gitea/gitea.git
synced 2026-05-14 17:27:39 +02:00
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 <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>
This commit is contained in:
parent
054eb6d8a5
commit
761b9d439b
@ -134,10 +134,25 @@ type PullRequestsConfig struct {
|
|||||||
DefaultTargetBranch string
|
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.
|
// FromDB fills up a PullRequestsConfig from serialized format.
|
||||||
func (cfg *PullRequestsConfig) FromDB(bs []byte) error {
|
func (cfg *PullRequestsConfig) FromDB(bs []byte) error {
|
||||||
// AllowRebaseUpdate = true as default for existing PullRequestConfig in DB
|
// set default values for existing PullRequestConfig in DB
|
||||||
cfg.AllowRebaseUpdate = true
|
*cfg = *DefaultPullRequestsConfig()
|
||||||
return json.UnmarshalHandleDoubleEncode(bs, &cfg)
|
return json.UnmarshalHandleDoubleEncode(bs, &cfg)
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -156,17 +171,8 @@ func (cfg *PullRequestsConfig) IsMergeStyleAllowed(mergeStyle MergeStyle) bool {
|
|||||||
mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge
|
mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge
|
||||||
}
|
}
|
||||||
|
|
||||||
// GetDefaultMergeStyle returns the default merge style for this pull request
|
func DefaultPullRequestsUnit(repoID int64) RepoUnit {
|
||||||
func (cfg *PullRequestsConfig) GetDefaultMergeStyle() MergeStyle {
|
return RepoUnit{RepoID: repoID, Type: unit.TypePullRequests, Config: DefaultPullRequestsConfig()}
|
||||||
if len(cfg.DefaultMergeStyle) != 0 {
|
|
||||||
return cfg.DefaultMergeStyle
|
|
||||||
}
|
|
||||||
|
|
||||||
if setting.Repository.PullRequest.DefaultMergeStyle != "" {
|
|
||||||
return MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle)
|
|
||||||
}
|
|
||||||
|
|
||||||
return MergeStyleMerge
|
|
||||||
}
|
}
|
||||||
|
|
||||||
type ActionsConfig struct {
|
type ActionsConfig struct {
|
||||||
|
|||||||
@ -67,3 +67,17 @@ func ParseBool(s string) Option[bool] {
|
|||||||
}
|
}
|
||||||
return Some(v)
|
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
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@ -884,77 +884,44 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if opts.HasPullRequests != nil && !unit_model.TypePullRequests.UnitGlobalDisabled() {
|
if !unit_model.TypePullRequests.UnitGlobalDisabled() {
|
||||||
if *opts.HasPullRequests {
|
mustDeletePullRequestUnit := opts.HasPullRequests != nil && !*opts.HasPullRequests
|
||||||
// We do allow setting individual PR settings through the API, so
|
mustInsertPullRequestUnit := opts.HasPullRequests != nil && *opts.HasPullRequests
|
||||||
// we get the config settings and then set them
|
if mustDeletePullRequestUnit {
|
||||||
// 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 {
|
|
||||||
deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
|
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,
|
||||||
|
})
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -905,9 +905,8 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
|
|||||||
// Check correct values and select default
|
// Check correct values and select default
|
||||||
if ms, ok := ctx.Data["MergeStyle"].(repo_model.MergeStyle); !ok ||
|
if ms, ok := ctx.Data["MergeStyle"].(repo_model.MergeStyle); !ok ||
|
||||||
!prConfig.IsMergeStyleAllowed(ms) {
|
!prConfig.IsMergeStyleAllowed(ms) {
|
||||||
defaultMergeStyle := prConfig.GetDefaultMergeStyle()
|
if prConfig.IsMergeStyleAllowed(prConfig.DefaultMergeStyle) && !ok {
|
||||||
if prConfig.IsMergeStyleAllowed(defaultMergeStyle) && !ok {
|
mergeStyle = prConfig.DefaultMergeStyle
|
||||||
mergeStyle = defaultMergeStyle
|
|
||||||
} else if prConfig.AllowMerge {
|
} else if prConfig.AllowMerge {
|
||||||
mergeStyle = repo_model.MergeStyleMerge
|
mergeStyle = repo_model.MergeStyleMerge
|
||||||
} else if prConfig.AllowRebase {
|
} else if prConfig.AllowRebase {
|
||||||
|
|||||||
@ -118,7 +118,7 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR
|
|||||||
allowManualMerge = config.AllowManualMerge
|
allowManualMerge = config.AllowManualMerge
|
||||||
autodetectManualMerge = config.AutodetectManualMerge
|
autodetectManualMerge = config.AutodetectManualMerge
|
||||||
defaultDeleteBranchAfterMerge = config.DefaultDeleteBranchAfterMerge
|
defaultDeleteBranchAfterMerge = config.DefaultDeleteBranchAfterMerge
|
||||||
defaultMergeStyle = config.GetDefaultMergeStyle()
|
defaultMergeStyle = config.DefaultMergeStyle
|
||||||
defaultAllowMaintainerEdit = config.DefaultAllowMaintainerEdit
|
defaultAllowMaintainerEdit = config.DefaultAllowMaintainerEdit
|
||||||
defaultTargetBranch = config.DefaultTargetBranch
|
defaultTargetBranch = config.DefaultTargetBranch
|
||||||
}
|
}
|
||||||
|
|||||||
@ -386,15 +386,7 @@ func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r
|
|||||||
},
|
},
|
||||||
})
|
})
|
||||||
case unit.TypePullRequests:
|
case unit.TypePullRequests:
|
||||||
units = append(units, repo_model.RepoUnit{
|
units = append(units, repo_model.DefaultPullRequestsUnit(repo.ID))
|
||||||
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,
|
|
||||||
},
|
|
||||||
})
|
|
||||||
case unit.TypeProjects:
|
case unit.TypeProjects:
|
||||||
units = append(units, repo_model.RepoUnit{
|
units = append(units, repo_model.RepoUnit{
|
||||||
RepoID: repo.ID,
|
RepoID: repo.ID,
|
||||||
|
|||||||
@ -279,10 +279,10 @@ func TestAPICreatePullSuccess(t *testing.T) {
|
|||||||
}).AddTokenAuth(token)
|
}).AddTokenAuth(token)
|
||||||
MakeRequest(t, req, http.StatusCreated)
|
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})
|
prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
|
||||||
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
|
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
|
MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
|
||||||
}
|
}
|
||||||
@ -304,7 +304,7 @@ func TestAPICreatePullBasePermission(t *testing.T) {
|
|||||||
Base: "master",
|
Base: "master",
|
||||||
Title: prTitle,
|
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)
|
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)
|
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)
|
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)
|
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})
|
prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
|
||||||
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
|
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) {
|
func TestAPICreatePullHeadPermission(t *testing.T) {
|
||||||
|
|||||||
@ -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).
|
req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name), &repoEditOption).
|
||||||
AddTokenAuth(token4)
|
AddTokenAuth(token4)
|
||||||
MakeRequest(t, req, http.StatusForbidden)
|
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)
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user