diff --git a/internal/api/controller/pullreq/create.go b/internal/api/controller/pullreq/create.go index 59ca689c2..54d317f86 100644 --- a/internal/api/controller/pullreq/create.go +++ b/internal/api/controller/pullreq/create.go @@ -33,8 +33,6 @@ func (c *Controller) Create( repoRef string, in *CreateInput, ) (*types.PullReq, error) { - now := time.Now().UnixMilli() - in.Title = strings.TrimSpace(in.Title) if in.Title == "" { return nil, usererror.BadRequest("pull request title can't be empty") @@ -89,31 +87,7 @@ func (c *Controller) Create( return err } - // create new pull request object - pr = &types.PullReq{ - ID: 0, // the ID will be populated in the data layer - CreatedBy: session.Principal.ID, - Created: now, - Updated: now, - Number: lastNumber + 1, - State: enum.PullReqStateOpen, - Title: in.Title, - Description: in.Description, - SourceRepoID: sourceRepo.ID, - SourceBranch: in.SourceBranch, - TargetRepoID: targetRepo.ID, - TargetBranch: in.TargetBranch, - MergedBy: nil, - Merged: nil, - MergeStrategy: nil, - Author: types.PrincipalInfo{ - ID: session.Principal.ID, - UID: session.Principal.UID, - Name: session.Principal.DisplayName, - Email: session.Principal.Email, - }, - Merger: nil, - } + pr = newPullReq(session, lastNumber+1, sourceRepo, targetRepo, in) return c.pullreqStore.Create(ctx, pr) }) @@ -123,3 +97,36 @@ func (c *Controller) Create( return pr, nil } + +// newPullReq creates new pull request object. +func newPullReq(session *auth.Session, number int64, + sourceRepo, targetRepo *types.Repository, in *CreateInput) *types.PullReq { + now := time.Now().UnixMilli() + return &types.PullReq{ + ID: 0, // the ID will be populated in the data layer + Version: 0, + Number: number, + CreatedBy: session.Principal.ID, + Created: now, + Updated: now, + Edited: now, + State: enum.PullReqStateOpen, + Title: in.Title, + Description: in.Description, + SourceRepoID: sourceRepo.ID, + SourceBranch: in.SourceBranch, + TargetRepoID: targetRepo.ID, + TargetBranch: in.TargetBranch, + PullReqActivitySeq: 0, + MergedBy: nil, + Merged: nil, + MergeStrategy: nil, + Author: types.PrincipalInfo{ + ID: session.Principal.ID, + UID: session.Principal.UID, + Name: session.Principal.DisplayName, + Email: session.Principal.Email, + }, + Merger: nil, + } +} diff --git a/internal/api/controller/pullreq/update.go b/internal/api/controller/pullreq/update.go index dc5fb93dd..b4d5fec0a 100644 --- a/internal/api/controller/pullreq/update.go +++ b/internal/api/controller/pullreq/update.go @@ -64,7 +64,7 @@ func (c *Controller) Update(ctx context.Context, pr.Title = in.Title pr.Description = in.Description - pr.Updated = time.Now().UnixMilli() + pr.Edited = time.Now().UnixMilli() err = c.pullreqStore.Update(ctx, pr) if err != nil { @@ -77,5 +77,7 @@ func (c *Controller) Update(ctx context.Context, return nil, err } + // TODO: Write a row to the pull request activity + return pr, nil } diff --git a/internal/store/database/migrate/postgres/0003_create_table_pullreq.up.sql b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql similarity index 89% rename from internal/store/database/migrate/postgres/0003_create_table_pullreq.up.sql rename to internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql index 8aaa0fece..d63458ad3 100644 --- a/internal/store/database/migrate/postgres/0003_create_table_pullreq.up.sql +++ b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql @@ -1,8 +1,10 @@ -CREATE TABLE pullreq ( +CREATE TABLE pullreqs ( pullreq_id SERIAL PRIMARY KEY +,pullreq_version INTEGER NOT NULL DEFAULT 0 ,pullreq_created_by INTEGER NOT NULL ,pullreq_created BIGINT NOT NULL ,pullreq_updated BIGINT NOT NULL +,pullreq_edited BIGINT NOT NULL ,pullreq_number INTEGER NOT NULL ,pullreq_state TEXT NOT NULL ,pullreq_title TEXT NOT NULL @@ -11,6 +13,7 @@ pullreq_id SERIAL PRIMARY KEY ,pullreq_source_branch TEXT NOT NULL ,pullreq_target_repo_id INTEGER NOT NULL ,pullreq_target_branch TEXT NOT NULL +,pullreq_activity_seq INTEGER DEFAULT 0 ,pullreq_merged_by INTEGER ,pullreq_merged BIGINT ,pullreq_merge_strategy TEXT diff --git a/internal/store/database/migrate/postgres/0004_create_index_pullreq_target_repo_id_number.up.sql b/internal/store/database/migrate/postgres/0004_create_index_pullreq_target_repo_id_number.up.sql index 33987370a..d6691829f 100644 --- a/internal/store/database/migrate/postgres/0004_create_index_pullreq_target_repo_id_number.up.sql +++ b/internal/store/database/migrate/postgres/0004_create_index_pullreq_target_repo_id_number.up.sql @@ -1,2 +1,2 @@ CREATE UNIQUE INDEX IF NOT EXISTS index_pullreq_target_repo_id_number -ON pullreq(pullreq_target_repo_id, pullreq_number); +ON pullreqs(pullreq_target_repo_id, pullreq_number); diff --git a/internal/store/database/migrate/sqlite/0003_create_table_pullreq.up.sql b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql similarity index 89% rename from internal/store/database/migrate/sqlite/0003_create_table_pullreq.up.sql rename to internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql index 013a89eb2..219e36f98 100644 --- a/internal/store/database/migrate/sqlite/0003_create_table_pullreq.up.sql +++ b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql @@ -1,8 +1,10 @@ -CREATE TABLE pullreq ( +CREATE TABLE pullreqs ( pullreq_id INTEGER PRIMARY KEY AUTOINCREMENT +,pullreq_version INTEGER NOT NULL DEFAULT 0 ,pullreq_created_by INTEGER NOT NULL ,pullreq_created BIGINT NOT NULL ,pullreq_updated BIGINT NOT NULL +,pullreq_edited BIGINT NOT NULL ,pullreq_number INTEGER NOT NULL ,pullreq_state TEXT NOT NULL ,pullreq_title TEXT NOT NULL @@ -11,6 +13,7 @@ pullreq_id INTEGER PRIMARY KEY AUTOINCREMENT ,pullreq_source_branch TEXT NOT NULL ,pullreq_target_repo_id INTEGER NOT NULL ,pullreq_target_branch TEXT NOT NULL +,pullreq_activity_seq INTEGER DEFAULT 0 ,pullreq_merged_by INTEGER ,pullreq_merged BIGINT ,pullreq_merge_strategy TEXT diff --git a/internal/store/database/migrate/sqlite/0004_create_index_pullreq_target_repo_id_number.up.sql b/internal/store/database/migrate/sqlite/0004_create_index_pullreq_target_repo_id_number.up.sql index 33987370a..d6691829f 100644 --- a/internal/store/database/migrate/sqlite/0004_create_index_pullreq_target_repo_id_number.up.sql +++ b/internal/store/database/migrate/sqlite/0004_create_index_pullreq_target_repo_id_number.up.sql @@ -1,2 +1,2 @@ CREATE UNIQUE INDEX IF NOT EXISTS index_pullreq_target_repo_id_number -ON pullreq(pullreq_target_repo_id, pullreq_number); +ON pullreqs(pullreq_target_repo_id, pullreq_number); diff --git a/internal/store/database/pullreq.go b/internal/store/database/pullreq.go index 2777482f8..2f5c9cf03 100644 --- a/internal/store/database/pullreq.go +++ b/internal/store/database/pullreq.go @@ -6,6 +6,7 @@ package database import ( "context" + "time" "github.com/harness/gitness/internal/store" "github.com/harness/gitness/internal/store/database/dbtx" @@ -35,12 +36,16 @@ type PullReqStore struct { // pullReq is used to fetch pull request data from the database. // The object should be later re-packed into a different struct to return it as an API response. type pullReq struct { - ID int64 `db:"pullreq_id"` - CreatedBy int64 `db:"pullreq_created_by"` - Created int64 `db:"pullreq_created"` - Updated int64 `db:"pullreq_updated"` - Number int64 `db:"pullreq_number"` - State enum.PullReqState `db:"pullreq_state"` + ID int64 `db:"pullreq_id"` + Version int64 `db:"pullreq_version"` + Number int64 `db:"pullreq_number"` + + CreatedBy int64 `db:"pullreq_created_by"` + Created int64 `db:"pullreq_created"` + Updated int64 `db:"pullreq_updated"` + Edited int64 `db:"pullreq_edited"` + + State enum.PullReqState `db:"pullreq_state"` Title string `db:"pullreq_title"` Description string `db:"pullreq_description"` @@ -50,6 +55,8 @@ type pullReq struct { TargetRepoID int64 `db:"pullreq_target_repo_id"` TargetBranch string `db:"pullreq_target_branch"` + PullReqActivitySeq int64 `db:"pullreq_activity_seq"` + MergedBy null.Int `db:"pullreq_merged_by"` Merged null.Int `db:"pullreq_merged"` MergeStrategy null.String `db:"pullreq_merge_strategy"` @@ -65,10 +72,12 @@ type pullReq struct { const ( pullReqColumns = ` pullreq_id + ,pullreq_version + ,pullreq_number ,pullreq_created_by ,pullreq_created ,pullreq_updated - ,pullreq_number + ,pullreq_edited ,pullreq_state ,pullreq_title ,pullreq_description @@ -76,6 +85,7 @@ const ( ,pullreq_source_branch ,pullreq_target_repo_id ,pullreq_target_branch + ,pullreq_activity_seq ,pullreq_merged_by ,pullreq_merged ,pullreq_merge_strategy @@ -88,7 +98,7 @@ const ( pullReqSelectBase = ` SELECT` + pullReqColumns + ` - FROM pullreq + FROM pullreqs INNER JOIN principals author on author.principal_id = pullreq_created_by LEFT JOIN principals merger on merger.principal_id = pullreq_merged_by` ) @@ -126,33 +136,39 @@ func (s *PullReqStore) FindByNumber(ctx context.Context, repoID, number int64) ( // Create creates a new pull request. func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { const sqlQuery = ` - INSERT INTO pullreq ( - pullreq_created_by + INSERT INTO pullreqs ( + pullreq_version + ,pullreq_number + ,pullreq_created_by ,pullreq_created ,pullreq_updated + ,pullreq_edited ,pullreq_state - ,pullreq_number ,pullreq_title ,pullreq_description ,pullreq_source_repo_id ,pullreq_source_branch ,pullreq_target_repo_id ,pullreq_target_branch + ,pullreq_activity_seq ,pullreq_merged_by ,pullreq_merged ,pullreq_merge_strategy ) values ( - :pullreq_created_by + :pullreq_version + ,:pullreq_number + ,:pullreq_created_by ,:pullreq_created ,:pullreq_updated + ,:pullreq_edited ,:pullreq_state - ,:pullreq_number ,:pullreq_title ,:pullreq_description ,:pullreq_source_repo_id ,:pullreq_source_branch ,:pullreq_target_repo_id ,:pullreq_target_branch + ,:pullreq_activity_seq ,:pullreq_merged_by ,:pullreq_merged ,:pullreq_merge_strategy @@ -175,34 +191,56 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { // Update updates the pull request. func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { const sqlQuery = ` - UPDATE pullreq + UPDATE pullreqs SET - pullreq_updated = :pullreq_updated + pullreq_version = :pullreq_version + ,pullreq_updated = :pullreq_updated + ,pullreq_edited = :pullreq_edited ,pullreq_state = :pullreq_state ,pullreq_title = :pullreq_title ,pullreq_description = :pullreq_description + ,pullreq_activity_seq = :pullreq_activity_seq ,pullreq_merged_by = :pullreq_merged_by ,pullreq_merged = :pullreq_merged ,pullreq_merge_strategy = :pullreq_merge_strategy - WHERE pullreq_id = :pullreq_id` + WHERE pullreq_id = :pullreq_id AND pullreq_version = :pullreq_version - 1` db := dbtx.GetAccessor(ctx, s.db) - query, arg, err := db.BindNamed(sqlQuery, mapInternalPullReq(pr)) + updatedAt := time.Now() + + dbPR := mapInternalPullReq(pr) + dbPR.Version++ + dbPR.Updated = updatedAt.UnixMilli() + + query, arg, err := db.BindNamed(sqlQuery, dbPR) if err != nil { return processSQLErrorf(err, "Failed to bind pull request object") } - if _, err = db.ExecContext(ctx, query, arg...); err != nil { - return processSQLErrorf(err, "Update query failed") + result, err := db.ExecContext(ctx, query, arg...) + if err != nil { + return processSQLErrorf(err, "Failed to update pull request") } + count, err := result.RowsAffected() + if err != nil { + return processSQLErrorf(err, "Failed to get number of updated rows") + } + + if count == 0 { + return store.ErrConflict + } + + pr.Version = dbPR.Version + pr.Updated = dbPR.Updated + return nil } // Delete the pull request. func (s *PullReqStore) Delete(ctx context.Context, id int64) error { - const pullReqDelete = `DELETE FROM pullreq WHERE pullreq_id = $1` + const pullReqDelete = `DELETE FROM pullreqs WHERE pullreq_id = $1` db := dbtx.GetAccessor(ctx, s.db) @@ -215,7 +253,7 @@ func (s *PullReqStore) Delete(ctx context.Context, id int64) error { // LastNumber return the number of the most recent pull request. func (s *PullReqStore) LastNumber(ctx context.Context, repoID int64) (int64, error) { - const sqlQuery = `select coalesce(max(pullreq_number), 0) from pullreq where pullreq_target_repo_id = $1 limit 1` + const sqlQuery = `select coalesce(max(pullreq_number), 0) from pullreqs where pullreq_target_repo_id = $1 limit 1` db := dbtx.GetAccessor(ctx, s.db) @@ -236,7 +274,7 @@ func (s *PullReqStore) LastNumber(ctx context.Context, repoID int64) (int64, err func (s *PullReqStore) Count(ctx context.Context, repoID int64, opts *types.PullReqFilter) (int64, error) { stmt := builder. Select("count(*)"). - From("pullreq"). + From("pullreqs"). Where("pullreq_target_repo_id = ?", repoID) if len(opts.States) == 1 { @@ -281,7 +319,7 @@ func (s *PullReqStore) Count(ctx context.Context, repoID int64, opts *types.Pull func (s *PullReqStore) List(ctx context.Context, repoID int64, opts *types.PullReqFilter) ([]*types.PullReq, error) { stmt := builder. Select(pullReqColumns). - From("pullreq"). + From("pullreqs"). InnerJoin("principals author on author.principal_id = pullreq_created_by"). LeftJoin("principals merger on merger.principal_id = pullreq_merged_by"). Where("pullreq_target_repo_id = ?", repoID) @@ -345,23 +383,26 @@ func (s *PullReqStore) List(ctx context.Context, repoID int64, opts *types.PullR func mapPullReq(pr *pullReq) *types.PullReq { m := &types.PullReq{ - ID: pr.ID, - CreatedBy: pr.CreatedBy, - Created: pr.Created, - Updated: pr.Updated, - Number: pr.Number, - State: pr.State, - Title: pr.Title, - Description: pr.Description, - SourceRepoID: pr.SourceRepoID, - SourceBranch: pr.SourceBranch, - TargetRepoID: pr.TargetRepoID, - TargetBranch: pr.TargetBranch, - MergedBy: pr.MergedBy.Ptr(), - Merged: pr.Merged.Ptr(), - MergeStrategy: pr.MergeStrategy.Ptr(), - Author: types.PrincipalInfo{}, - Merger: nil, + ID: pr.ID, + Version: pr.Version, + Number: pr.Number, + CreatedBy: pr.CreatedBy, + Created: pr.Created, + Updated: pr.Updated, + Edited: pr.Edited, + State: pr.State, + Title: pr.Title, + Description: pr.Description, + SourceRepoID: pr.SourceRepoID, + SourceBranch: pr.SourceBranch, + TargetRepoID: pr.TargetRepoID, + TargetBranch: pr.TargetBranch, + PullReqActivitySeq: pr.PullReqActivitySeq, + MergedBy: pr.MergedBy.Ptr(), + Merged: pr.Merged.Ptr(), + MergeStrategy: pr.MergeStrategy.Ptr(), + Author: types.PrincipalInfo{}, + Merger: nil, } m.Author = types.PrincipalInfo{ ID: pr.CreatedBy, @@ -383,21 +424,24 @@ func mapPullReq(pr *pullReq) *types.PullReq { func mapInternalPullReq(pr *types.PullReq) *pullReq { m := &pullReq{ - ID: pr.ID, - CreatedBy: pr.CreatedBy, - Created: pr.Created, - Updated: pr.Updated, - Number: pr.Number, - State: pr.State, - Title: pr.Title, - Description: pr.Description, - SourceRepoID: pr.SourceRepoID, - SourceBranch: pr.SourceBranch, - TargetRepoID: pr.TargetRepoID, - TargetBranch: pr.TargetBranch, - MergedBy: null.IntFromPtr(pr.MergedBy), - Merged: null.IntFromPtr(pr.Merged), - MergeStrategy: null.StringFromPtr(pr.MergeStrategy), + ID: pr.ID, + Version: pr.Version, + Number: pr.Number, + CreatedBy: pr.CreatedBy, + Created: pr.Created, + Updated: pr.Updated, + Edited: pr.Edited, + State: pr.State, + Title: pr.Title, + Description: pr.Description, + SourceRepoID: pr.SourceRepoID, + SourceBranch: pr.SourceBranch, + TargetRepoID: pr.TargetRepoID, + TargetBranch: pr.TargetBranch, + PullReqActivitySeq: pr.PullReqActivitySeq, + MergedBy: null.IntFromPtr(pr.MergedBy), + Merged: null.IntFromPtr(pr.Merged), + MergeStrategy: null.StringFromPtr(pr.MergeStrategy), } return m diff --git a/internal/store/errors.go b/internal/store/errors.go index 155df24ef..dd832f289 100644 --- a/internal/store/errors.go +++ b/internal/store/errors.go @@ -9,6 +9,7 @@ import "errors" var ( ErrResourceNotFound = errors.New("resource not found") ErrDuplicate = errors.New("resource is a duplicate") + ErrConflict = errors.New("resource version conflict") ErrPathTooLong = errors.New("the path is too long") ErrPrimaryPathAlreadyExists = errors.New("primary path already exists for resource") ErrPrimaryPathRequired = errors.New("path has to be primary") diff --git a/internal/store/store.go b/internal/store/store.go index 28fa82116..cacba82a0 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -209,7 +209,7 @@ type ( // Create a new pull request. Create(ctx context.Context, pullreq *types.PullReq) error - // Update the pull request. + // Update the pull request. It will set new values to the Version and Updated fields. Update(ctx context.Context, repo *types.PullReq) error // Delete the pull request. diff --git a/types/pullreq.go b/types/pullreq.go index 454d98dcc..3d01b1b95 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -10,11 +10,14 @@ import ( // PullReq represents a pull request. type PullReq struct { - ID int64 `json:"id"` - CreatedBy int64 `json:"-"` + ID int64 `json:"id"` + Version int64 `json:"version"` + Number int64 `json:"number"` + + CreatedBy int64 `json:"-"` // not returned, because the author info is in the Author field Created int64 `json:"created"` Updated int64 `json:"updated"` - Number int64 `json:"number"` + Edited int64 `json:"edited"` State enum.PullReqState `json:"state"` @@ -26,7 +29,9 @@ type PullReq struct { TargetRepoID int64 `json:"target_repo_id"` TargetBranch string `json:"target_branch"` - MergedBy *int64 `json:"-"` + PullReqActivitySeq int64 `json:"-"` // not returned, because it's a server internal field + + MergedBy *int64 `json:"-"` // not returned, because the merger info is in the Merger field Merged *int64 `json:"merged"` MergeStrategy *string `json:"merge_strategy"` @@ -48,3 +53,33 @@ type PullReqFilter struct { Sort enum.PullReqSort `json:"sort"` Order enum.Order `json:"direction"` } + +// PullReqActivity represents a pull request activity. +type PullReqActivity struct { + ID int64 `json:"id"` + Version int64 `json:"version"` + + CreatedBy int64 `json:"-"` // not returned, because the author info is in the Author field + Created int64 `json:"created"` + Updated int64 `json:"updated"` + Edited int64 `json:"edited"` + Deleted int64 `json:"deleted"` + + RepoID int64 `json:"repo_id"` + PullReqID int64 `json:"pullreq_id"` + + Seq int64 `json:"seq"` + SubSeq int64 `json:"subseq"` + + Type int64 `json:"type"` + Kind int64 `json:"kind"` + + Text string `json:"title"` + Payload map[string]interface{} `json:"payload"` + + ResolvedBy *int64 `json:"-"` // not returned, because the resolver info is in the Resolver field + Resolved *int64 `json:"resolved"` + + Author PrincipalInfo `json:"author"` + Resolver *PrincipalInfo `json:"resolver"` +}