From eb994a4502baa8ab9fff9b476efe580f0d4e50a0 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Wed, 1 Jan 2020 23:51:10 +0100
Subject: [PATCH 1/4] Fix #9189 - API Allow only specific Colums to be updated
on Issue (#9539)
* dont insert "-1" in any case to issue.poster_id
* Make sure API cant override importand fields
* code format
* add Test for IssueEdit
* load milestone and return it on IssueEdit via API
* extend Test for TestAPIEditIssue
---
integrations/api_issue_test.go | 58 ++++++++++++++++++++++++++++++++++
models/fixtures/issue.yml | 15 ++++++++-
models/fixtures/milestone.yml | 8 +++++
models/fixtures/repository.yml | 3 +-
models/issue.go | 45 +++++++++++++++-----------
routers/api/v1/repo/issue.go | 10 ++++--
routers/api/v1/repo/pull.go | 4 +--
7 files changed, 117 insertions(+), 26 deletions(-)
diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go
index 24535057e2475..65d9369c533f0 100644
--- a/integrations/api_issue_test.go
+++ b/integrations/api_issue_test.go
@@ -62,3 +62,61 @@ func TestAPICreateIssue(t *testing.T) {
Title: title,
})
}
+
+func TestAPIEditIssue(t *testing.T) {
+ defer prepareTestEnv(t)()
+
+ issueBefore := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 9}).(*models.Issue)
+ repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: issueBefore.RepoID}).(*models.Repository)
+ owner := models.AssertExistsAndLoadBean(t, &models.User{ID: repo.OwnerID}).(*models.User)
+ assert.NoError(t, issueBefore.LoadAttributes())
+ assert.Equal(t, int64(1019307200), int64(issueBefore.DeadlineUnix))
+ assert.Equal(t, api.StateOpen, issueBefore.State())
+
+ session := loginUser(t, owner.Name)
+ token := getTokenForLoggedInUser(t, session)
+
+ // update values of issue
+ issueState := "closed"
+ removeDeadline := true
+ milestone := int64(4)
+ body := "new content!"
+ title := "new title from api set"
+
+ urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d?token=%s", owner.Name, repo.Name, issueBefore.Index, token)
+ req := NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
+ State: &issueState,
+ RemoveDeadline: &removeDeadline,
+ Milestone: &milestone,
+ Body: &body,
+ Title: title,
+
+ // ToDo change more
+ })
+ resp := session.MakeRequest(t, req, http.StatusCreated)
+ var apiIssue api.Issue
+ DecodeJSON(t, resp, &apiIssue)
+
+ issueAfter := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 10}).(*models.Issue)
+
+ // check deleted user
+ assert.Equal(t, int64(500), issueAfter.PosterID)
+ assert.NoError(t, issueAfter.LoadAttributes())
+ assert.Equal(t, int64(-1), issueAfter.PosterID)
+ assert.Equal(t, int64(-1), issueBefore.PosterID)
+ assert.Equal(t, int64(-1), apiIssue.Poster.ID)
+
+ // API response
+ assert.Equal(t, api.StateClosed, apiIssue.State)
+ assert.Equal(t, milestone, apiIssue.Milestone.ID)
+ assert.Equal(t, body, apiIssue.Body)
+ assert.True(t, apiIssue.Deadline == nil)
+ assert.Equal(t, title, apiIssue.Title)
+
+ // in database
+ assert.Equal(t, api.StateClosed, issueAfter.State())
+ assert.Equal(t, milestone, issueAfter.MilestoneID)
+ assert.Equal(t, int64(0), int64(issueAfter.DeadlineUnix))
+ assert.Equal(t, body, issueAfter.Content)
+ assert.Equal(t, title, issueAfter.Title)
+}
diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml
index 585242a14d139..e196cdd577311 100644
--- a/models/fixtures/issue.yml
+++ b/models/fixtures/issue.yml
@@ -96,4 +96,17 @@
is_closed: false
is_pull: true
created_unix: 946684820
- updated_unix: 978307180
\ No newline at end of file
+ updated_unix: 978307180
+
+-
+ id: 9
+ repo_id: 42
+ index: 1
+ poster_id: 500
+ name: issue from deleted account
+ content: content from deleted account
+ is_closed: false
+ is_pull: false
+ created_unix: 946684830
+ updated_unix: 999307200
+ deadline_unix: 1019307200
diff --git a/models/fixtures/milestone.yml b/models/fixtures/milestone.yml
index 15f422fc3b5c2..a9ecb4ee6a6ef 100644
--- a/models/fixtures/milestone.yml
+++ b/models/fixtures/milestone.yml
@@ -21,3 +21,11 @@
content: content3
is_closed: true
num_issues: 0
+
+-
+ id: 4
+ repo_id: 42
+ name: milestone of repo42
+ content: content random
+ is_closed: false
+ num_issues: 0
diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml
index cf7d24c6cdb3c..a084339cc21ed 100644
--- a/models/fixtures/repository.yml
+++ b/models/fixtures/repository.yml
@@ -547,7 +547,8 @@
is_private: false
num_stars: 0
num_forks: 0
- num_issues: 0
+ num_issues: 1
+ num_milestones: 1
is_mirror: false
-
diff --git a/models/issue.go b/models/issue.go
index a129bef7db6b3..4984eb5749954 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -1,4 +1,5 @@
// Copyright 2014 The Gogs Authors. All rights reserved.
+// Copyright 2020 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
@@ -238,6 +239,16 @@ func (issue *Issue) loadReactions(e Engine) (err error) {
return nil
}
+func (issue *Issue) loadMilestone(e Engine) (err error) {
+ if issue.Milestone == nil && issue.MilestoneID > 0 {
+ issue.Milestone, err = getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
+ if err != nil && !IsErrMilestoneNotExist(err) {
+ return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %v", issue.RepoID, issue.MilestoneID, err)
+ }
+ }
+ return nil
+}
+
func (issue *Issue) loadAttributes(e Engine) (err error) {
if err = issue.loadRepo(e); err != nil {
return
@@ -251,11 +262,8 @@ func (issue *Issue) loadAttributes(e Engine) (err error) {
return
}
- if issue.Milestone == nil && issue.MilestoneID > 0 {
- issue.Milestone, err = getMilestoneByRepoID(e, issue.RepoID, issue.MilestoneID)
- if err != nil && !IsErrMilestoneNotExist(err) {
- return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %v", issue.RepoID, issue.MilestoneID, err)
- }
+ if err = issue.loadMilestone(e); err != nil {
+ return
}
if err = issue.loadAssignees(e); err != nil {
@@ -295,6 +303,11 @@ func (issue *Issue) LoadAttributes() error {
return issue.loadAttributes(x)
}
+// LoadMilestone load milestone of this issue.
+func (issue *Issue) LoadMilestone() error {
+ return issue.loadMilestone(x)
+}
+
// GetIsRead load the `IsRead` field of the issue
func (issue *Issue) GetIsRead(userID int64) error {
issueUser := &IssueUser{IssueID: issue.ID, UID: userID}
@@ -1730,30 +1743,24 @@ func SearchIssueIDsByKeyword(kw string, repoID int64, limit, start int) (int64,
return total, ids, nil
}
-func updateIssue(e Engine, issue *Issue) error {
- _, err := e.ID(issue.ID).AllCols().Update(issue)
- if err != nil {
- return err
- }
- return nil
-}
-
-// UpdateIssue updates all fields of given issue.
-func UpdateIssue(issue *Issue) error {
+// UpdateIssueByAPI updates all allowed fields of given issue.
+func UpdateIssueByAPI(issue *Issue) error {
sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return err
}
- if err := updateIssue(sess, issue); err != nil {
+
+ if _, err := sess.ID(issue.ID).Cols(
+ "name", "is_closed", "content", "milestone_id", "priority",
+ "deadline_unix", "updated_unix", "closed_unix", "is_locked").
+ Update(issue); err != nil {
return err
}
+
if err := issue.neuterCrossReferences(sess); err != nil {
return err
}
- if err := issue.loadPoster(sess); err != nil {
- return err
- }
if err := issue.addCrossReferences(sess, issue.Poster); err != nil {
return err
}
diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go
index 6fec1c66b8c8c..756b2c35f218c 100644
--- a/routers/api/v1/repo/issue.go
+++ b/routers/api/v1/repo/issue.go
@@ -352,8 +352,8 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
}
}
- if err = models.UpdateIssue(issue); err != nil {
- ctx.Error(500, "UpdateIssue", err)
+ if err = models.UpdateIssueByAPI(issue); err != nil {
+ ctx.InternalServerError(err)
return
}
if form.State != nil {
@@ -372,7 +372,11 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
// Refetch from database to assign some automatic values
issue, err = models.GetIssueByID(issue.ID)
if err != nil {
- ctx.Error(500, "GetIssueByID", err)
+ ctx.InternalServerError(err)
+ return
+ }
+ if err = issue.LoadMilestone(); err != nil {
+ ctx.InternalServerError(err)
return
}
ctx.JSON(201, issue.APIFormat())
diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 2601ab8339e41..0f1be9a6f6bce 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -420,8 +420,8 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) {
}
}
- if err = models.UpdateIssue(issue); err != nil {
- ctx.Error(500, "UpdateIssue", err)
+ if err = models.UpdateIssueByAPI(issue); err != nil {
+ ctx.InternalServerError(err)
return
}
if form.State != nil {
From 21187b208cc14484bfca77a2e54bb7f91975a31e Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 2 Jan 2020 00:38:56 +0100
Subject: [PATCH 2/4] fix TEST
---
integrations/api_issue_test.go | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go
index 65d9369c533f0..15510fcfc1384 100644
--- a/integrations/api_issue_test.go
+++ b/integrations/api_issue_test.go
@@ -8,6 +8,7 @@ import (
"fmt"
"net/http"
"testing"
+ "time"
"code.gitea.io/gitea/models"
api "code.gitea.io/gitea/modules/structs"
@@ -64,7 +65,7 @@ func TestAPICreateIssue(t *testing.T) {
}
func TestAPIEditIssue(t *testing.T) {
- defer prepareTestEnv(t)()
+ prepareTestEnv(t)
issueBefore := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 9}).(*models.Issue)
repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: issueBefore.RepoID}).(*models.Repository)
@@ -78,18 +79,18 @@ func TestAPIEditIssue(t *testing.T) {
// update values of issue
issueState := "closed"
- removeDeadline := true
+ removeDeadline := time.Unix(0, 0)
milestone := int64(4)
body := "new content!"
title := "new title from api set"
urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d?token=%s", owner.Name, repo.Name, issueBefore.Index, token)
req := NewRequestWithJSON(t, "PATCH", urlStr, api.EditIssueOption{
- State: &issueState,
- RemoveDeadline: &removeDeadline,
- Milestone: &milestone,
- Body: &body,
- Title: title,
+ State: &issueState,
+ Deadline: &removeDeadline,
+ Milestone: &milestone,
+ Body: &body,
+ Title: title,
// ToDo change more
})
@@ -97,7 +98,7 @@ func TestAPIEditIssue(t *testing.T) {
var apiIssue api.Issue
DecodeJSON(t, resp, &apiIssue)
- issueAfter := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 10}).(*models.Issue)
+ issueAfter := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 9}).(*models.Issue)
// check deleted user
assert.Equal(t, int64(500), issueAfter.PosterID)
From fa4cad37650ee9345e8e451c040bef1ffbfc6089 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 2 Jan 2020 06:16:11 +0100
Subject: [PATCH 3/4] make sure Poster is loaded
---
models/issue.go | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/models/issue.go b/models/issue.go
index 4984eb5749954..fec210d2bcc90 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -1758,6 +1758,10 @@ func UpdateIssueByAPI(issue *Issue) error {
return err
}
+ if err := issue.loadPoster(sess); err != nil {
+ return err
+ }
+
if err := issue.neuterCrossReferences(sess); err != nil {
return err
}
From de4bc8bb787a7c5eb6c0d11f5f50a57792a82f50 Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Thu, 2 Jan 2020 07:11:11 +0100
Subject: [PATCH 4/4] keep code format on backport as it is
---
models/issue.go | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/models/issue.go b/models/issue.go
index fec210d2bcc90..7c2eecadbcf35 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -1750,19 +1750,16 @@ func UpdateIssueByAPI(issue *Issue) error {
if err := sess.Begin(); err != nil {
return err
}
-
if _, err := sess.ID(issue.ID).Cols(
"name", "is_closed", "content", "milestone_id", "priority",
"deadline_unix", "updated_unix", "closed_unix", "is_locked").
Update(issue); err != nil {
return err
}
-
- if err := issue.loadPoster(sess); err != nil {
+ if err := issue.neuterCrossReferences(sess); err != nil {
return err
}
-
- if err := issue.neuterCrossReferences(sess); err != nil {
+ if err := issue.loadPoster(sess); err != nil {
return err
}
if err := issue.addCrossReferences(sess, issue.Poster); err != nil {