From f4ca533fe10f7fa893e1953f8c8d9ed3e783486c Mon Sep 17 00:00:00 2001 From: Michael MurĂ© Date: Sat, 28 Mar 2020 21:09:36 +0100 Subject: gitlab: refactor the iterator, fix bugs Notably, properly reset sub iterators when changing to the next issue --- bridge/gitlab/import.go | 5 +- bridge/gitlab/iterator.go | 288 ----------------------------------- bridge/gitlab/iterator/issue.go | 79 ++++++++++ bridge/gitlab/iterator/iterator.go | 138 +++++++++++++++++ bridge/gitlab/iterator/labelEvent.go | 99 ++++++++++++ bridge/gitlab/iterator/note.go | 80 ++++++++++ 6 files changed, 399 insertions(+), 290 deletions(-) delete mode 100644 bridge/gitlab/iterator.go create mode 100644 bridge/gitlab/iterator/issue.go create mode 100644 bridge/gitlab/iterator/iterator.go create mode 100644 bridge/gitlab/iterator/labelEvent.go create mode 100644 bridge/gitlab/iterator/note.go diff --git a/bridge/gitlab/import.go b/bridge/gitlab/import.go index 0a47a783..8aeab1ce 100644 --- a/bridge/gitlab/import.go +++ b/bridge/gitlab/import.go @@ -10,6 +10,7 @@ import ( "github.com/MichaelMure/git-bug/bridge/core" "github.com/MichaelMure/git-bug/bridge/core/auth" + "github.com/MichaelMure/git-bug/bridge/gitlab/iterator" "github.com/MichaelMure/git-bug/bug" "github.com/MichaelMure/git-bug/cache" "github.com/MichaelMure/git-bug/entity" @@ -24,7 +25,7 @@ type gitlabImporter struct { client *gitlab.Client // iterator - iterator *iterator + iterator *iterator.Iterator // send only channel out chan<- core.ImportResult @@ -58,7 +59,7 @@ func (gi *gitlabImporter) Init(_ context.Context, repo *cache.RepoCache, conf co // ImportAll iterate over all the configured repository issues (notes) and ensure the creation // of the missing issues / comments / label events / title changes ... func (gi *gitlabImporter) ImportAll(ctx context.Context, repo *cache.RepoCache, since time.Time) (<-chan core.ImportResult, error) { - gi.iterator = NewIterator(ctx, gi.client, 10, gi.conf[confKeyProjectID], since) + gi.iterator = iterator.NewIterator(ctx, gi.client, 10, gi.conf[confKeyProjectID], since) out := make(chan core.ImportResult) gi.out = out diff --git a/bridge/gitlab/iterator.go b/bridge/gitlab/iterator.go deleted file mode 100644 index 07f9cce9..00000000 --- a/bridge/gitlab/iterator.go +++ /dev/null @@ -1,288 +0,0 @@ -package gitlab - -import ( - "context" - "sort" - "time" - - "github.com/xanzy/go-gitlab" -) - -type issueIterator struct { - page int - index int - cache []*gitlab.Issue -} - -type noteIterator struct { - page int - index int - cache []*gitlab.Note -} - -// Since Gitlab does not return the label events items in the correct order -// we need to sort the list our selfs and stop relying on the pagination model -// #BecauseGitlab -type labelEventIterator struct { - index int - cache []*gitlab.LabelEvent -} - -func (l *labelEventIterator) Len() int { - return len(l.cache) -} - -func (l *labelEventIterator) Swap(i, j int) { - l.cache[i], l.cache[j] = l.cache[j], l.cache[i] -} - -func (l *labelEventIterator) Less(i, j int) bool { - return l.cache[i].ID < l.cache[j].ID -} - -type iterator struct { - // gitlab api v4 client - gc *gitlab.Client - - // if since is given the iterator will query only the issues - // updated after this date - since time.Time - - // project id - project string - - // number of issues and notes to query at once - capacity int - - // shared context - ctx context.Context - - // sticky error - err error - - // issues iterator - issue *issueIterator - - // notes iterator - note *noteIterator - - // labelEvent iterator - labelEvent *labelEventIterator -} - -// NewIterator create a new iterator -func NewIterator(ctx context.Context, client *gitlab.Client, capacity int, projectID string, since time.Time) *iterator { - return &iterator{ - gc: client, - project: projectID, - since: since, - capacity: capacity, - ctx: ctx, - issue: &issueIterator{ - index: -1, - page: 1, - }, - note: ¬eIterator{ - index: -1, - page: 1, - }, - labelEvent: &labelEventIterator{ - index: -1, - }, - } -} - -// Error return last encountered error -func (i *iterator) Error() error { - return i.err -} - -func (i *iterator) getNextIssues() bool { - ctx, cancel := context.WithTimeout(i.ctx, defaultTimeout) - defer cancel() - - issues, _, err := i.gc.Issues.ListProjectIssues( - i.project, - &gitlab.ListProjectIssuesOptions{ - ListOptions: gitlab.ListOptions{ - Page: i.issue.page, - PerPage: i.capacity, - }, - Scope: gitlab.String("all"), - UpdatedAfter: &i.since, - Sort: gitlab.String("asc"), - }, - gitlab.WithContext(ctx), - ) - - if err != nil { - i.err = err - return false - } - - // if repository doesn't have any issues - if len(issues) == 0 { - return false - } - - i.issue.cache = issues - i.issue.index = 0 - i.issue.page++ - i.note.index = -1 - i.note.cache = nil - - return true -} - -func (i *iterator) NextIssue() bool { - if i.err != nil { - return false - } - - if i.ctx.Err() != nil { - return false - } - - // first query - if i.issue.cache == nil { - return i.getNextIssues() - } - - // move cursor index - if i.issue.index < len(i.issue.cache)-1 { - i.issue.index++ - return true - } - - return i.getNextIssues() -} - -func (i *iterator) IssueValue() *gitlab.Issue { - return i.issue.cache[i.issue.index] -} - -func (i *iterator) getNextNotes() bool { - ctx, cancel := context.WithTimeout(i.ctx, defaultTimeout) - defer cancel() - - notes, _, err := i.gc.Notes.ListIssueNotes( - i.project, - i.IssueValue().IID, - &gitlab.ListIssueNotesOptions{ - ListOptions: gitlab.ListOptions{ - Page: i.note.page, - PerPage: i.capacity, - }, - Sort: gitlab.String("asc"), - OrderBy: gitlab.String("created_at"), - }, - gitlab.WithContext(ctx), - ) - - if err != nil { - i.err = err - return false - } - - if len(notes) == 0 { - i.note.index = -1 - i.note.page = 1 - i.note.cache = nil - return false - } - - i.note.cache = notes - i.note.page++ - i.note.index = 0 - return true -} - -func (i *iterator) NextNote() bool { - if i.err != nil { - return false - } - - if i.ctx.Err() != nil { - return false - } - - if len(i.note.cache) == 0 { - return i.getNextNotes() - } - - // move cursor index - if i.note.index < len(i.note.cache)-1 { - i.note.index++ - return true - } - - return i.getNextNotes() -} - -func (i *iterator) NoteValue() *gitlab.Note { - return i.note.cache[i.note.index] -} - -func (i *iterator) getLabelEvents() bool { - ctx, cancel := context.WithTimeout(i.ctx, defaultTimeout) - defer cancel() - - // since order is not garanteed we should query all label events - // and sort them by ID - page := 1 - hasNextPage := true - for hasNextPage { - labelEvents, _, err := i.gc.ResourceLabelEvents.ListIssueLabelEvents( - i.project, - i.IssueValue().IID, - &gitlab.ListLabelEventsOptions{ - ListOptions: gitlab.ListOptions{ - Page: page, - PerPage: i.capacity, - }, - }, - gitlab.WithContext(ctx), - ) - if err != nil { - i.err = err - return false - } - - page++ - hasNextPage = len(labelEvents) != 0 - i.labelEvent.cache = append(i.labelEvent.cache, labelEvents...) - } - - i.labelEvent.index = 0 - sort.Sort(i.labelEvent) - - // if the label events list is empty return false - return len(i.labelEvent.cache) != 0 -} - -// because Gitlab -func (i *iterator) NextLabelEvent() bool { - if i.err != nil { - return false - } - - if i.ctx.Err() != nil { - return false - } - - if len(i.labelEvent.cache) == 0 { - return i.getLabelEvents() - } - - // move cursor index - if i.labelEvent.index < len(i.labelEvent.cache)-1 { - i.labelEvent.index++ - return true - } - - return false -} - -func (i *iterator) LabelEventValue() *gitlab.LabelEvent { - return i.labelEvent.cache[i.labelEvent.index] -} diff --git a/bridge/gitlab/iterator/issue.go b/bridge/gitlab/iterator/issue.go new file mode 100644 index 00000000..a6bcebf1 --- /dev/null +++ b/bridge/gitlab/iterator/issue.go @@ -0,0 +1,79 @@ +package iterator + +import ( + "context" + + "github.com/xanzy/go-gitlab" +) + +type issueIterator struct { + page int + index int + cache []*gitlab.Issue +} + +func newIssueIterator() *issueIterator { + ii := &issueIterator{} + ii.Reset() + return ii +} + +func (ii *issueIterator) Next(ctx context.Context, conf config) (bool, error) { + // first query + if ii.cache == nil { + return ii.getNext(ctx, conf) + } + + // move cursor index + if ii.index < len(ii.cache)-1 { + ii.index++ + return true, nil + } + + return ii.getNext(ctx, conf) +} + +func (ii *issueIterator) Value() *gitlab.Issue { + return ii.cache[ii.index] +} + +func (ii *issueIterator) getNext(ctx context.Context, conf config) (bool, error) { + ctx, cancel := context.WithTimeout(ctx, conf.timeout) + defer cancel() + + issues, _, err := conf.gc.Issues.ListProjectIssues( + conf.project, + &gitlab.ListProjectIssuesOptions{ + ListOptions: gitlab.ListOptions{ + Page: ii.page, + PerPage: conf.capacity, + }, + Scope: gitlab.String("all"), + UpdatedAfter: &conf.since, + Sort: gitlab.String("asc"), + }, + gitlab.WithContext(ctx), + ) + + if err != nil { + ii.Reset() + return false, err + } + + // if repository doesn't have any issues + if len(issues) == 0 { + return false, nil + } + + ii.cache = issues + ii.index = 0 + ii.page++ + + return true, nil +} + +func (ii *issueIterator) Reset() { + ii.index = -1 + ii.page = -1 + ii.cache = nil +} diff --git a/bridge/gitlab/iterator/iterator.go b/bridge/gitlab/iterator/iterator.go new file mode 100644 index 00000000..ee2090b0 --- /dev/null +++ b/bridge/gitlab/iterator/iterator.go @@ -0,0 +1,138 @@ +package iterator + +import ( + "context" + "time" + + "github.com/xanzy/go-gitlab" +) + +type Iterator struct { + // shared context + ctx context.Context + + // to pass to sub-iterators + conf config + + // sticky error + err error + + // issues iterator + issue *issueIterator + + // notes iterator + note *noteIterator + + // labelEvent iterator + labelEvent *labelEventIterator +} + +type config struct { + // gitlab api v4 client + gc *gitlab.Client + + timeout time.Duration + + // if since is given the iterator will query only the issues + // updated after this date + since time.Time + + // project id + project string + + // number of issues and notes to query at once + capacity int +} + +// NewIterator create a new iterator +func NewIterator(ctx context.Context, client *gitlab.Client, capacity int, projectID string, since time.Time) *Iterator { + return &Iterator{ + ctx: ctx, + conf: config{ + gc: client, + timeout: 60 * time.Second, + since: since, + project: projectID, + capacity: capacity, + }, + issue: newIssueIterator(), + note: newNoteIterator(), + labelEvent: newLabelEventIterator(), + } +} + +// Error return last encountered error +func (i *Iterator) Error() error { + return i.err +} + +func (i *Iterator) NextIssue() bool { + if i.err != nil { + return false + } + + if i.ctx.Err() != nil { + return false + } + + more, err := i.issue.Next(i.ctx, i.conf) + if err != nil { + i.err = err + return false + } + + // Also reset the other sub iterators as they would + // no longer be valid + i.note.Reset(i.issue.Value().IID) + i.labelEvent.Reset(i.issue.Value().IID) + + return more +} + +func (i *Iterator) IssueValue() *gitlab.Issue { + return i.issue.Value() +} + +func (i *Iterator) NextNote() bool { + if i.err != nil { + return false + } + + if i.ctx.Err() != nil { + return false + } + + more, err := i.note.Next(i.ctx, i.conf) + if err != nil { + i.err = err + return false + } + + return more +} + +func (i *Iterator) NoteValue() *gitlab.Note { + return i.note.Value() +} + +func (i *Iterator) NextLabelEvent() bool { + if i.err != nil { + return false + } + + if i.ctx.Err() != nil { + return false + } + + more, err := i.labelEvent.Next(i.ctx, i.conf) + if err != nil { + i.err = err + return false + } + + return more +} + +func (i *Iterator) LabelEventValue() *gitlab.LabelEvent { + return i.labelEvent.Value() +} diff --git a/bridge/gitlab/iterator/labelEvent.go b/bridge/gitlab/iterator/labelEvent.go new file mode 100644 index 00000000..7ee2604b --- /dev/null +++ b/bridge/gitlab/iterator/labelEvent.go @@ -0,0 +1,99 @@ +package iterator + +import ( + "context" + "sort" + + "github.com/xanzy/go-gitlab" +) + +// Since Gitlab does not return the label events items in the correct order +// we need to sort the list ourselves and stop relying on the pagination model +// #BecauseGitlab +type labelEventIterator struct { + issue int + index int + cache []*gitlab.LabelEvent +} + +func newLabelEventIterator() *labelEventIterator { + lei := &labelEventIterator{} + lei.Reset(-1) + return lei +} + +func (lei *labelEventIterator) Next(ctx context.Context, conf config) (bool, error) { + // first query + if lei.cache == nil { + return lei.getNext(ctx, conf) + } + + // move cursor index + if lei.index < len(lei.cache)-1 { + lei.index++ + return true, nil + } + + return false, nil +} + +func (lei *labelEventIterator) Value() *gitlab.LabelEvent { + return lei.cache[lei.index] +} + +func (lei *labelEventIterator) getNext(ctx context.Context, conf config) (bool, error) { + ctx, cancel := context.WithTimeout(ctx, conf.timeout) + defer cancel() + + // since order is not guaranteed we should query all label events + // and sort them by ID + page := 1 + for { + labelEvents, _, err := conf.gc.ResourceLabelEvents.ListIssueLabelEvents( + conf.project, + lei.issue, + &gitlab.ListLabelEventsOptions{ + ListOptions: gitlab.ListOptions{ + Page: page, + PerPage: conf.capacity, + }, + }, + gitlab.WithContext(ctx), + ) + if err != nil { + lei.Reset(-1) + return false, err + } + + if len(labelEvents) == 0 { + break + } + lei.cache = append(lei.cache, labelEvents...) + page++ + } + + sort.Sort(lei) + lei.index = 0 + + return len(lei.cache) > 0, nil +} + +func (lei *labelEventIterator) Reset(issue int) { + lei.issue = issue + lei.index = -1 + lei.cache = nil +} + +// ORDERING + +func (lei *labelEventIterator) Len() int { + return len(lei.cache) +} + +func (lei *labelEventIterator) Swap(i, j int) { + lei.cache[i], lei.cache[j] = lei.cache[j], lei.cache[i] +} + +func (lei *labelEventIterator) Less(i, j int) bool { + return lei.cache[i].ID < lei.cache[j].ID +} diff --git a/bridge/gitlab/iterator/note.go b/bridge/gitlab/iterator/note.go new file mode 100644 index 00000000..486ca94e --- /dev/null +++ b/bridge/gitlab/iterator/note.go @@ -0,0 +1,80 @@ +package iterator + +import ( + "context" + + "github.com/xanzy/go-gitlab" +) + +type noteIterator struct { + issue int + page int + index int + cache []*gitlab.Note +} + +func newNoteIterator() *noteIterator { + in := ¬eIterator{} + in.Reset(-1) + return in +} + +func (in *noteIterator) Next(ctx context.Context, conf config) (bool, error) { + // first query + if in.cache == nil { + return in.getNext(ctx, conf) + } + + // move cursor index + if in.index < len(in.cache)-1 { + in.index++ + return true, nil + } + + return in.getNext(ctx, conf) +} + +func (in *noteIterator) Value() *gitlab.Note { + return in.cache[in.index] +} + +func (in *noteIterator) getNext(ctx context.Context, conf config) (bool, error) { + ctx, cancel := context.WithTimeout(ctx, conf.timeout) + defer cancel() + + notes, _, err := conf.gc.Notes.ListIssueNotes( + conf.project, + in.issue, + &gitlab.ListIssueNotesOptions{ + ListOptions: gitlab.ListOptions{ + Page: in.page, + PerPage: conf.capacity, + }, + Sort: gitlab.String("asc"), + OrderBy: gitlab.String("created_at"), + }, + gitlab.WithContext(ctx), + ) + + if err != nil { + in.Reset(-1) + return false, err + } + + if len(notes) == 0 { + return false, nil + } + + in.cache = notes + in.index = 0 + in.page++ + + return true, nil +} + +func (in *noteIterator) Reset(issue int) { + in.issue = issue + in.index = -1 + in.page = -1 + in.cache = nil +} -- cgit From 903549cadf40ede3771053781eb6e9fd31aaa64e Mon Sep 17 00:00:00 2001 From: Michael MurĂ© Date: Sat, 4 Apr 2020 11:59:53 +0200 Subject: gitlab: fix iterator (paginate with first index 1) and avoid the trailing API call --- bridge/gitlab/iterator/issue.go | 20 +++++++++++++++----- bridge/gitlab/iterator/labelEvent.go | 8 +++++++- bridge/gitlab/iterator/note.go | 22 ++++++++++++++++------ 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/bridge/gitlab/iterator/issue.go b/bridge/gitlab/iterator/issue.go index a6bcebf1..9361b496 100644 --- a/bridge/gitlab/iterator/issue.go +++ b/bridge/gitlab/iterator/issue.go @@ -7,9 +7,10 @@ import ( ) type issueIterator struct { - page int - index int - cache []*gitlab.Issue + page int + lastPage bool + index int + cache []*gitlab.Issue } func newIssueIterator() *issueIterator { @@ -38,10 +39,14 @@ func (ii *issueIterator) Value() *gitlab.Issue { } func (ii *issueIterator) getNext(ctx context.Context, conf config) (bool, error) { + if ii.lastPage { + return false, nil + } + ctx, cancel := context.WithTimeout(ctx, conf.timeout) defer cancel() - issues, _, err := conf.gc.Issues.ListProjectIssues( + issues, resp, err := conf.gc.Issues.ListProjectIssues( conf.project, &gitlab.ListProjectIssuesOptions{ ListOptions: gitlab.ListOptions{ @@ -60,6 +65,10 @@ func (ii *issueIterator) getNext(ctx context.Context, conf config) (bool, error) return false, err } + if resp.TotalPages == ii.page { + ii.lastPage = true + } + // if repository doesn't have any issues if len(issues) == 0 { return false, nil @@ -74,6 +83,7 @@ func (ii *issueIterator) getNext(ctx context.Context, conf config) (bool, error) func (ii *issueIterator) Reset() { ii.index = -1 - ii.page = -1 + ii.page = 1 + ii.lastPage = false ii.cache = nil } diff --git a/bridge/gitlab/iterator/labelEvent.go b/bridge/gitlab/iterator/labelEvent.go index 7ee2604b..812e6646 100644 --- a/bridge/gitlab/iterator/labelEvent.go +++ b/bridge/gitlab/iterator/labelEvent.go @@ -49,7 +49,7 @@ func (lei *labelEventIterator) getNext(ctx context.Context, conf config) (bool, // and sort them by ID page := 1 for { - labelEvents, _, err := conf.gc.ResourceLabelEvents.ListIssueLabelEvents( + labelEvents, resp, err := conf.gc.ResourceLabelEvents.ListIssueLabelEvents( conf.project, lei.issue, &gitlab.ListLabelEventsOptions{ @@ -68,7 +68,13 @@ func (lei *labelEventIterator) getNext(ctx context.Context, conf config) (bool, if len(labelEvents) == 0 { break } + lei.cache = append(lei.cache, labelEvents...) + + if resp.TotalPages == page { + break + } + page++ } diff --git a/bridge/gitlab/iterator/note.go b/bridge/gitlab/iterator/note.go index 486ca94e..a1e0544c 100644 --- a/bridge/gitlab/iterator/note.go +++ b/bridge/gitlab/iterator/note.go @@ -7,10 +7,11 @@ import ( ) type noteIterator struct { - issue int - page int - index int - cache []*gitlab.Note + issue int + page int + lastPage bool + index int + cache []*gitlab.Note } func newNoteIterator() *noteIterator { @@ -39,10 +40,14 @@ func (in *noteIterator) Value() *gitlab.Note { } func (in *noteIterator) getNext(ctx context.Context, conf config) (bool, error) { + if in.lastPage { + return false, nil + } + ctx, cancel := context.WithTimeout(ctx, conf.timeout) defer cancel() - notes, _, err := conf.gc.Notes.ListIssueNotes( + notes, resp, err := conf.gc.Notes.ListIssueNotes( conf.project, in.issue, &gitlab.ListIssueNotesOptions{ @@ -61,6 +66,10 @@ func (in *noteIterator) getNext(ctx context.Context, conf config) (bool, error) return false, err } + if resp.TotalPages == in.page { + in.lastPage = true + } + if len(notes) == 0 { return false, nil } @@ -75,6 +84,7 @@ func (in *noteIterator) getNext(ctx context.Context, conf config) (bool, error) func (in *noteIterator) Reset(issue int) { in.issue = issue in.index = -1 - in.page = -1 + in.page = 1 + in.lastPage = false in.cache = nil } -- cgit