diff --git a/.gopmfile b/.gopmfile index a30329171..71cbe87a5 100644 --- a/.gopmfile +++ b/.gopmfile @@ -20,7 +20,7 @@ github.com/go-xorm/xorm = commit:3ad0b42 github.com/gogits/chardet = commit:2404f77 github.com/gogits/cron = commit:7f3990a github.com/gogits/git-module = commit:7129215 -github.com/gogits/go-gogs-client = commit:c52f7ee +github.com/gogits/go-gogs-client = commit:3693f6a github.com/gogits/go-libravatar = commit:cd1abbd github.com/issue9/identicon = commit:d36b545 github.com/jaytaylor/html2text = commit:52d9b78 diff --git a/glide.lock b/glide.lock index dee3f6be7..9b6603c3e 100644 --- a/glide.lock +++ b/glide.lock @@ -45,7 +45,7 @@ imports: - name: github.com/gogits/git-module version: 71292151e50d262429f29515dd077d7f5beb8c66 - name: github.com/gogits/go-gogs-client - version: c52f7ee0cc58d3cd6e379025552873a8df6de322 + version: 3693f6a1a71302d146590845e35fc7fd530aae4e - name: github.com/gogits/go-libravatar version: cd1abbd55d09b793672732a7a1dfdaa12a40dfd0 - name: github.com/issue9/identicon diff --git a/models/issue.go b/models/issue.go index 208204b07..55d3253ad 100644 --- a/models/issue.go +++ b/models/issue.go @@ -98,7 +98,7 @@ func (issue *Issue) loadAttributes(e Engine) (err error) { issue.PosterID = -1 issue.Poster = NewGhostUser() } else { - return fmt.Errorf("getUserByID.(poster) [%d]: %v", issue.PosterID, err) + return fmt.Errorf("getUserByID.(Poster) [%d]: %v", issue.PosterID, err) } return } @@ -780,7 +780,7 @@ func GetIssueByIndex(repoID, index int64) (*Issue, error) { return issue, issue.LoadAttributes() } -func getIssueByID(e Engine, id int64) (*Issue, error) { +func getRawIssueByID(e Engine, id int64) (*Issue, error) { issue := new(Issue) has, err := e.Id(id).Get(issue) if err != nil { @@ -788,7 +788,15 @@ func getIssueByID(e Engine, id int64) (*Issue, error) { } else if !has { return nil, ErrIssueNotExist{id, 0, 0} } - return issue, issue.LoadAttributes() + return issue, nil +} + +func getIssueByID(e Engine, id int64) (*Issue, error) { + issue, err := getRawIssueByID(e, id) + if err != nil { + return nil, err + } + return issue, issue.loadAttributes(e) } // GetIssueByID returns an issue by given ID. @@ -1745,10 +1753,14 @@ func GetAttachmentsByIssueID(issueID int64) ([]*Attachment, error) { return getAttachmentsByIssueID(x, issueID) } +func getAttachmentsByCommentID(e Engine, commentID int64) ([]*Attachment, error) { + attachments := make([]*Attachment, 0, 10) + return attachments, e.Where("comment_id=?", commentID).Find(&attachments) +} + // GetAttachmentsByCommentID returns all attachments if comment by given ID. func GetAttachmentsByCommentID(commentID int64) ([]*Attachment, error) { - attachments := make([]*Attachment, 0, 10) - return attachments, x.Where("comment_id=?", commentID).Find(&attachments) + return getAttachmentsByCommentID(x, commentID) } // DeleteAttachment deletes the given attachment and optionally the associated file. diff --git a/models/issue_comment.go b/models/issue_comment.go index 63d4cbf66..091bfc3b2 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -51,8 +51,9 @@ type Comment struct { ID int64 `xorm:"pk autoincr"` Type CommentType PosterID int64 - Poster *User `xorm:"-"` - IssueID int64 `xorm:"INDEX"` + Poster *User `xorm:"-"` + IssueID int64 `xorm:"INDEX"` + Issue *Issue `xorm:"-"` CommitID int64 Line int64 Content string `xorm:"TEXT"` @@ -82,29 +83,46 @@ func (c *Comment) BeforeUpdate() { } func (c *Comment) AfterSet(colName string, _ xorm.Cell) { - var err error switch colName { - case "id": - c.Attachments, err = GetAttachmentsByCommentID(c.ID) - if err != nil { - log.Error(3, "GetAttachmentsByCommentID[%d]: %v", c.ID, err) - } + case "created_unix": + c.Created = time.Unix(c.CreatedUnix, 0).Local() + case "updated_unix": + c.Updated = time.Unix(c.UpdatedUnix, 0).Local() + } +} - case "poster_id": +func (c *Comment) loadAttributes(e Engine) (err error) { + if c.Poster == nil { c.Poster, err = GetUserByID(c.PosterID) if err != nil { if IsErrUserNotExist(err) { c.PosterID = -1 c.Poster = NewGhostUser() } else { - log.Error(3, "GetUserByID[%d]: %v", c.ID, err) + return fmt.Errorf("getUserByID.(Poster) [%d]: %v", c.PosterID, err) } } - case "created_unix": - c.Created = time.Unix(c.CreatedUnix, 0).Local() - case "updated_unix": - c.Updated = time.Unix(c.UpdatedUnix, 0).Local() } + + if c.Issue == nil { + c.Issue, err = getRawIssueByID(e, c.IssueID) + if err != nil { + return fmt.Errorf("getIssueByID [%d]: %v", c.IssueID, err) + } + } + + if c.Attachments == nil { + c.Attachments, err = getAttachmentsByCommentID(e, c.ID) + if err != nil { + return fmt.Errorf("getAttachmentsByCommentID [%d]: %v", c.ID, err) + } + } + + return nil +} + +func (c *Comment) LoadAttributes() error { + return c.loadAttributes(x) } func (c *Comment) AfterDelete() { @@ -116,50 +134,19 @@ func (c *Comment) AfterDelete() { } func (c *Comment) HTMLURL() string { - issue, err := GetIssueByID(c.IssueID) - if err != nil { // Silently dropping errors :unamused: - log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err) - return "" - } - return fmt.Sprintf("%s#issuecomment-%d", issue.HTMLURL(), c.ID) -} - -func (c *Comment) IssueURL() string { - issue, err := GetIssueByID(c.IssueID) - if err != nil { // Silently dropping errors :unamused: - log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err) - return "" - } - - if issue.IsPull { - return "" - } - return issue.HTMLURL() -} - -func (c *Comment) PRURL() string { - issue, err := GetIssueByID(c.IssueID) - if err != nil { // Silently dropping errors :unamused: - log.Error(4, "GetIssueByID(%d): %v", c.IssueID, err) - return "" - } - - if !issue.IsPull { - return "" - } - return issue.HTMLURL() + return fmt.Sprintf("%s#issuecomment-%d", c.Issue.HTMLURL(), c.ID) } +// This method assumes following fields have been assigned with valid values: +// Required - Poster, Issue func (c *Comment) APIFormat() *api.Comment { return &api.Comment{ - ID: c.ID, - Poster: c.Poster.APIFormat(), - HTMLURL: c.HTMLURL(), - IssueURL: c.IssueURL(), - PRURL: c.PRURL(), - Body: c.Content, - Created: c.Created, - Updated: c.Updated, + ID: c.ID, + HTMLURL: c.HTMLURL(), + Poster: c.Poster.APIFormat(), + Body: c.Content, + Created: c.Created, + Updated: c.Updated, } } @@ -294,7 +281,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err comment.MailParticipants(act.OpType, opts.Issue) } - return comment, nil + return comment, comment.loadAttributes(e) } func createStatusComment(e *xorm.Session, doer *User, repo *Repository, issue *Issue) (*Comment, error) { @@ -389,7 +376,18 @@ func GetCommentByID(id int64) (*Comment, error) { } else if !has { return nil, ErrCommentNotExist{id, 0} } - return c, nil + return c, c.LoadAttributes() +} + +// FIXME: use CommentList to improve performance. +func loadCommentsAttributes(e Engine, comments []*Comment) (err error) { + for i := range comments { + if err = comments[i].loadAttributes(e); err != nil { + return fmt.Errorf("loadAttributes [%d]: %v", comments[i].ID, err) + } + } + + return nil } func getCommentsByIssueIDSince(e Engine, issueID, since int64) ([]*Comment, error) { @@ -398,7 +396,11 @@ func getCommentsByIssueIDSince(e Engine, issueID, since int64) ([]*Comment, erro if since > 0 { sess.And("updated_unix >= ?", since) } - return comments, sess.Find(&comments) + + if err := sess.Find(&comments); err != nil { + return nil, err + } + return comments, loadCommentsAttributes(e, comments) } func getCommentsByRepoIDSince(e Engine, repoID, since int64) ([]*Comment, error) { @@ -407,7 +409,10 @@ func getCommentsByRepoIDSince(e Engine, repoID, since int64) ([]*Comment, error) if since > 0 { sess.And("updated_unix >= ?", since) } - return comments, sess.Find(&comments) + if err := sess.Find(&comments); err != nil { + return nil, err + } + return comments, loadCommentsAttributes(e, comments) } func getCommentsByIssueID(e Engine, issueID int64) ([]*Comment, error) { diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index eaa9c0c47..fe94fd71b 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -84,7 +84,7 @@ func EditIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) return } - if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin() { ctx.Status(403) return } else if comment.Type != models.COMMENT_TYPE_COMMENT { @@ -111,7 +111,7 @@ func DeleteIssueComment(ctx *context.APIContext) { return } - if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin() { ctx.Status(403) return } else if comment.Type != models.COMMENT_TYPE_COMMENT {