From 64133ee5ba4b03e2f7f2f2161b1d551a97bd0d80 Mon Sep 17 00:00:00 2001 From: Michael Muré Date: Sat, 6 Oct 2018 11:55:16 +0200 Subject: github: deal with the deleted user case where github return a null actor --- bridge/bridges.go | 4 +- bridge/core/bridge.go | 119 ++++++++++++++++++++++++++++++++---------- bridge/core/interfaces.go | 18 ++++--- bridge/github/github.go | 4 +- bridge/github/import.go | 102 +++++++++++++++++++++++------------- bridge/github/import_query.go | 8 ++- commands/bridge_pull.go | 2 +- 7 files changed, 180 insertions(+), 77 deletions(-) diff --git a/bridge/bridges.go b/bridge/bridges.go index 1ffa74b6..4a0ef9e2 100644 --- a/bridge/bridges.go +++ b/bridge/bridges.go @@ -20,8 +20,8 @@ func NewBridge(repo *cache.RepoCache, target string, name string) (*core.Bridge, // Instantiate a new bridge for a repo, from the combined target and name contained // in the full name -func NewBridgeFullName(repo *cache.RepoCache, fullName string) (*core.Bridge, error) { - return core.NewBridgeFullName(repo, fullName) +func NewBridgeFromFullName(repo *cache.RepoCache, fullName string) (*core.Bridge, error) { + return core.NewBridgeFromFullName(repo, fullName) } // Attempt to retrieve a default bridge for the given repo. If zero or multiple diff --git a/bridge/core/bridge.go b/bridge/core/bridge.go index 69e9e522..112c56ea 100644 --- a/bridge/core/bridge.go +++ b/bridge/core/bridge.go @@ -20,10 +20,13 @@ var bridgeImpl map[string]reflect.Type // Bridge is a wrapper around a BridgeImpl that will bind low-level // implementation with utility code to provide high-level functions. type Bridge struct { - Name string - repo *cache.RepoCache - impl BridgeImpl - conf Configuration + Name string + repo *cache.RepoCache + impl BridgeImpl + importer Importer + exporter Exporter + conf Configuration + initDone bool } // Register will register a new BridgeImpl @@ -65,7 +68,7 @@ func NewBridge(repo *cache.RepoCache, target string, name string) (*Bridge, erro // Instantiate a new bridge for a repo, from the combined target and name contained // in the full name -func NewBridgeFullName(repo *cache.RepoCache, fullName string) (*Bridge, error) { +func NewBridgeFromFullName(repo *cache.RepoCache, fullName string) (*Bridge, error) { target, name, err := splitFullName(fullName) if err != nil { return nil, err @@ -184,19 +187,19 @@ func (b *Bridge) storeConfig(conf Configuration) error { return nil } -func (b Bridge) getConfig() (Configuration, error) { - var err error +func (b *Bridge) ensureConfig() error { if b.conf == nil { - b.conf, err = b.loadConfig() + conf, err := b.loadConfig() if err != nil { - return nil, err + return err } + b.conf = conf } - return b.conf, nil + return nil } -func (b Bridge) loadConfig() (Configuration, error) { +func (b *Bridge) loadConfig() (Configuration, error) { keyPrefix := fmt.Sprintf("git-bug.bridge.%s.%s.", b.impl.Target(), b.Name) pairs, err := b.repo.ReadConfigs(keyPrefix) @@ -218,58 +221,120 @@ func (b Bridge) loadConfig() (Configuration, error) { return result, nil } -func (b Bridge) ImportAll() error { - importer := b.impl.Importer() +func (b *Bridge) getImporter() Importer { + if b.importer == nil { + b.importer = b.impl.NewImporter() + } + + return b.importer +} + +func (b *Bridge) getExporter() Exporter { + if b.exporter == nil { + b.exporter = b.impl.NewExporter() + } + + return b.exporter +} + +func (b *Bridge) ensureInit() error { + if b.initDone { + return nil + } + + importer := b.getImporter() + if importer != nil { + err := importer.Init(b.conf) + if err != nil { + return err + } + } + + exporter := b.getExporter() + if exporter != nil { + err := exporter.Init(b.conf) + if err != nil { + return err + } + } + + b.initDone = true + + return nil +} + +func (b *Bridge) ImportAll() error { + importer := b.getImporter() if importer == nil { return ErrImportNorSupported } - conf, err := b.getConfig() + err := b.ensureConfig() + if err != nil { + return err + } + + err = b.ensureInit() if err != nil { return err } - return b.impl.Importer().ImportAll(b.repo, conf) + return importer.ImportAll(b.repo) } -func (b Bridge) Import(id string) error { - importer := b.impl.Importer() +func (b *Bridge) Import(id string) error { + importer := b.getImporter() if importer == nil { return ErrImportNorSupported } - conf, err := b.getConfig() + err := b.ensureConfig() if err != nil { return err } - return b.impl.Importer().Import(b.repo, conf, id) + err = b.ensureInit() + if err != nil { + return err + } + + return importer.Import(b.repo, id) } -func (b Bridge) ExportAll() error { - exporter := b.impl.Exporter() +func (b *Bridge) ExportAll() error { + exporter := b.getExporter() if exporter == nil { return ErrExportNorSupported } - conf, err := b.getConfig() + err := b.ensureConfig() + if err != nil { + return err + } + + err = b.ensureInit() if err != nil { return err } - return b.impl.Exporter().ExportAll(b.repo, conf) + return exporter.ExportAll(b.repo) } -func (b Bridge) Export(id string) error { - exporter := b.impl.Exporter() +func (b *Bridge) Export(id string) error { + exporter := b.getExporter() if exporter == nil { return ErrExportNorSupported } - conf, err := b.getConfig() + err := b.ensureConfig() + if err != nil { + return err + } + + err = b.ensureInit() if err != nil { return err } - return b.impl.Exporter().Export(b.repo, conf, id) + return exporter.Export(b.repo, id) } diff --git a/bridge/core/interfaces.go b/bridge/core/interfaces.go index 79b75606..4836dab3 100644 --- a/bridge/core/interfaces.go +++ b/bridge/core/interfaces.go @@ -18,19 +18,21 @@ type BridgeImpl interface { // ValidateConfig check the configuration for error ValidateConfig(conf Configuration) error - // Importer return an Importer implementation if the import is supported - Importer() Importer + // NewImporter return an Importer implementation if the import is supported + NewImporter() Importer - // Exporter return an Exporter implementation if the export is supported - Exporter() Exporter + // NewExporter return an Exporter implementation if the export is supported + NewExporter() Exporter } type Importer interface { - ImportAll(repo *cache.RepoCache, conf Configuration) error - Import(repo *cache.RepoCache, conf Configuration, id string) error + Init(conf Configuration) error + ImportAll(repo *cache.RepoCache) error + Import(repo *cache.RepoCache, id string) error } type Exporter interface { - ExportAll(repo *cache.RepoCache, conf Configuration) error - Export(repo *cache.RepoCache, conf Configuration, id string) error + Init(conf Configuration) error + ExportAll(repo *cache.RepoCache) error + Export(repo *cache.RepoCache, id string) error } diff --git a/bridge/github/github.go b/bridge/github/github.go index b70a89db..b3f8d763 100644 --- a/bridge/github/github.go +++ b/bridge/github/github.go @@ -19,11 +19,11 @@ func (*Github) Target() string { return "github" } -func (*Github) Importer() core.Importer { +func (*Github) NewImporter() core.Importer { return &githubImporter{} } -func (*Github) Exporter() core.Exporter { +func (*Github) NewExporter() core.Exporter { return nil } diff --git a/bridge/github/import.go b/bridge/github/import.go index 4ecd4876..ed0135e7 100644 --- a/bridge/github/import.go +++ b/bridge/github/import.go @@ -16,15 +16,24 @@ const keyGithubId = "github-id" const keyGithubUrl = "github-url" // githubImporter implement the Importer interface -type githubImporter struct{} +type githubImporter struct { + client *githubv4.Client + conf core.Configuration + ghost bug.Person +} + +func (gi *githubImporter) Init(conf core.Configuration) error { + gi.conf = conf + gi.client = buildClient(conf) -func (*githubImporter) ImportAll(repo *cache.RepoCache, conf core.Configuration) error { - client := buildClient(conf) + return gi.fetchGhost() +} +func (gi *githubImporter) ImportAll(repo *cache.RepoCache) error { q := &issueTimelineQuery{} variables := map[string]interface{}{ - "owner": githubv4.String(conf[keyUser]), - "name": githubv4.String(conf[keyProject]), + "owner": githubv4.String(gi.conf[keyUser]), + "name": githubv4.String(gi.conf[keyProject]), "issueFirst": githubv4.Int(1), "issueAfter": (*githubv4.String)(nil), "timelineFirst": githubv4.Int(10), @@ -41,7 +50,7 @@ func (*githubImporter) ImportAll(repo *cache.RepoCache, conf core.Configuration) var b *cache.BugCache for { - err := client.Query(context.TODO(), &q, variables) + err := gi.client.Query(context.TODO(), &q, variables) if err != nil { return err } @@ -53,14 +62,14 @@ func (*githubImporter) ImportAll(repo *cache.RepoCache, conf core.Configuration) issue := q.Repository.Issues.Nodes[0] if b == nil { - b, err = ensureIssue(repo, issue, client, variables) + b, err = gi.ensureIssue(repo, issue, variables) if err != nil { return err } } for _, itemEdge := range q.Repository.Issues.Nodes[0].Timeline.Edges { - ensureTimelineItem(b, itemEdge.Cursor, itemEdge.Node, client, variables) + gi.ensureTimelineItem(b, itemEdge.Cursor, itemEdge.Node, variables) } if !issue.Timeline.PageInfo.HasNextPage { @@ -86,14 +95,13 @@ func (*githubImporter) ImportAll(repo *cache.RepoCache, conf core.Configuration) return nil } -func (*githubImporter) Import(repo *cache.RepoCache, conf core.Configuration, id string) error { - fmt.Println(conf) +func (gi *githubImporter) Import(repo *cache.RepoCache, id string) error { fmt.Println("IMPORT") return nil } -func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Client, rootVariables map[string]interface{}) (*cache.BugCache, error) { +func (gi *githubImporter) ensureIssue(repo *cache.RepoCache, issue issueTimeline, rootVariables map[string]interface{}) (*cache.BugCache, error) { fmt.Printf("import issue: %s\n", issue.Title) b, err := repo.ResolveBugCreateMetadata(keyGithubId, parseId(issue.Id)) @@ -115,7 +123,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl if len(issue.UserContentEdits.Nodes) == 0 { if err == bug.ErrBugNotExist { b, err = repo.NewBugRaw( - makePerson(issue.Author), + gi.makePerson(issue.Author), issue.CreatedAt.Unix(), // Todo: this might not be the initial title, we need to query the // timeline to be sure @@ -153,7 +161,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl // we create the bug as soon as we have a legit first edition b, err = repo.NewBugRaw( - makePerson(issue.Author), + gi.makePerson(issue.Author), issue.CreatedAt.Unix(), // Todo: this might not be the initial title, we need to query the // timeline to be sure @@ -176,7 +184,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl return nil, err } - err = ensureCommentEdit(b, target, edit) + err = gi.ensureCommentEdit(b, target, edit) if err != nil { return nil, err } @@ -186,7 +194,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl // if we still didn't get a legit edit, create the bug from the issue data if b == nil { return repo.NewBugRaw( - makePerson(issue.Author), + gi.makePerson(issue.Author), issue.CreatedAt.Unix(), // Todo: this might not be the initial title, we need to query the // timeline to be sure @@ -215,7 +223,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl } for { - err := client.Query(context.TODO(), &q, variables) + err := gi.client.Query(context.TODO(), &q, variables) if err != nil { return nil, err } @@ -235,7 +243,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl // we create the bug as soon as we have a legit first edition b, err = repo.NewBugRaw( - makePerson(issue.Author), + gi.makePerson(issue.Author), issue.CreatedAt.Unix(), // Todo: this might not be the initial title, we need to query the // timeline to be sure @@ -258,7 +266,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl return nil, err } - err = ensureCommentEdit(b, target, edit) + err = gi.ensureCommentEdit(b, target, edit) if err != nil { return nil, err } @@ -276,7 +284,7 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl // if we still didn't get a legit edit, create the bug from the issue data if b == nil { return repo.NewBugRaw( - makePerson(issue.Author), + gi.makePerson(issue.Author), issue.CreatedAt.Unix(), // Todo: this might not be the initial title, we need to query the // timeline to be sure @@ -293,12 +301,12 @@ func ensureIssue(repo *cache.RepoCache, issue issueTimeline, client *githubv4.Cl return b, nil } -func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timelineItem, client *githubv4.Client, rootVariables map[string]interface{}) error { +func (gi *githubImporter) ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timelineItem, rootVariables map[string]interface{}) error { fmt.Printf("import %s\n", item.Typename) switch item.Typename { case "IssueComment": - return ensureComment(b, cursor, item.IssueComment, client, rootVariables) + return gi.ensureComment(b, cursor, item.IssueComment, rootVariables) case "LabeledEvent": id := parseId(item.LabeledEvent.Id) @@ -307,7 +315,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return err } _, err = b.ChangeLabelsRaw( - makePerson(item.LabeledEvent.Actor), + gi.makePerson(item.LabeledEvent.Actor), item.LabeledEvent.CreatedAt.Unix(), []string{ string(item.LabeledEvent.Label.Name), @@ -324,7 +332,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return err } _, err = b.ChangeLabelsRaw( - makePerson(item.UnlabeledEvent.Actor), + gi.makePerson(item.UnlabeledEvent.Actor), item.UnlabeledEvent.CreatedAt.Unix(), nil, []string{ @@ -341,7 +349,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return err } return b.CloseRaw( - makePerson(item.ClosedEvent.Actor), + gi.makePerson(item.ClosedEvent.Actor), item.ClosedEvent.CreatedAt.Unix(), map[string]string{keyGithubId: id}, ) @@ -353,7 +361,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return err } return b.OpenRaw( - makePerson(item.ReopenedEvent.Actor), + gi.makePerson(item.ReopenedEvent.Actor), item.ReopenedEvent.CreatedAt.Unix(), map[string]string{keyGithubId: id}, ) @@ -365,7 +373,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return err } return b.SetTitleRaw( - makePerson(item.RenamedTitleEvent.Actor), + gi.makePerson(item.RenamedTitleEvent.Actor), item.RenamedTitleEvent.CreatedAt.Unix(), string(item.RenamedTitleEvent.CurrentTitle), map[string]string{keyGithubId: id}, @@ -378,7 +386,7 @@ func ensureTimelineItem(b *cache.BugCache, cursor githubv4.String, item timeline return nil } -func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComment, client *githubv4.Client, rootVariables map[string]interface{}) error { +func (gi *githubImporter) ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComment, rootVariables map[string]interface{}) error { target, err := b.ResolveTargetWithMetadata(keyGithubId, parseId(comment.Id)) if err != nil && err != cache.ErrNoMatchingOp { // real error @@ -399,7 +407,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme if len(comment.UserContentEdits.Nodes) == 0 { if err == cache.ErrNoMatchingOp { err = b.AddCommentRaw( - makePerson(comment.Author), + gi.makePerson(comment.Author), comment.CreatedAt.Unix(), cleanupText(string(comment.Body)), nil, @@ -432,7 +440,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme } err = b.AddCommentRaw( - makePerson(comment.Author), + gi.makePerson(comment.Author), comment.CreatedAt.Unix(), cleanupText(string(*edit.Diff)), nil, @@ -446,7 +454,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme } } - err := ensureCommentEdit(b, target, edit) + err := gi.ensureCommentEdit(b, target, edit) if err != nil { return err } @@ -471,7 +479,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme } for { - err := client.Query(context.TODO(), &q, variables) + err := gi.client.Query(context.TODO(), &q, variables) if err != nil { return err } @@ -488,7 +496,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme continue } - err := ensureCommentEdit(b, target, edit) + err := gi.ensureCommentEdit(b, target, edit) if err != nil { return err } @@ -506,7 +514,7 @@ func ensureComment(b *cache.BugCache, cursor githubv4.String, comment issueComme return nil } -func ensureCommentEdit(b *cache.BugCache, target git.Hash, edit userContentEdit) error { +func (gi *githubImporter) ensureCommentEdit(b *cache.BugCache, target git.Hash, edit userContentEdit) error { if edit.Diff == nil { // this happen if the event is older than early 2018, Github doesn't have the data before that. // Best we can do is to ignore the event. @@ -536,7 +544,7 @@ func ensureCommentEdit(b *cache.BugCache, target git.Hash, edit userContentEdit) case edit.DeletedAt == nil: // comment edition err := b.EditCommentRaw( - makePerson(*edit.Editor), + gi.makePerson(edit.Editor), edit.CreatedAt.Unix(), target, cleanupText(string(*edit.Diff)), @@ -553,13 +561,37 @@ func ensureCommentEdit(b *cache.BugCache, target git.Hash, edit userContentEdit) } // makePerson create a bug.Person from the Github data -func makePerson(actor actor) bug.Person { +func (gi *githubImporter) makePerson(actor *actor) bug.Person { + if actor == nil { + return gi.ghost + } + return bug.Person{ Name: string(actor.Login), AvatarUrl: string(actor.AvatarUrl), } } +func (gi *githubImporter) fetchGhost() error { + var q userQuery + + variables := map[string]interface{}{ + "login": githubv4.String("ghost"), + } + + err := gi.client.Query(context.TODO(), &q, variables) + if err != nil { + return err + } + + gi.ghost = bug.Person{ + Name: string(q.User.Login), + AvatarUrl: string(q.User.AvatarUrl), + } + + return nil +} + // parseId convert the unusable githubv4.ID (an interface{}) into a string func parseId(id githubv4.ID) string { return fmt.Sprintf("%v", id) diff --git a/bridge/github/import_query.go b/bridge/github/import_query.go index 89eeb553..e1dcff64 100644 --- a/bridge/github/import_query.go +++ b/bridge/github/import_query.go @@ -17,13 +17,13 @@ type actor struct { type actorEvent struct { Id githubv4.ID CreatedAt githubv4.DateTime - Actor actor + Actor *actor } type authorEvent struct { Id githubv4.ID CreatedAt githubv4.DateTime - Author actor + Author *actor } type userContentEdit struct { @@ -150,3 +150,7 @@ type commentEditQuery struct { } `graphql:"issues(first: $issueFirst, after: $issueAfter, orderBy: {field: CREATED_AT, direction: ASC})"` } `graphql:"repository(owner: $owner, name: $name)"` } + +type userQuery struct { + User actor `graphql:"user(login: $login)"` +} diff --git a/commands/bridge_pull.go b/commands/bridge_pull.go index fc12d369..521f2d9c 100644 --- a/commands/bridge_pull.go +++ b/commands/bridge_pull.go @@ -19,7 +19,7 @@ func runBridgePull(cmd *cobra.Command, args []string) error { if len(args) == 0 { b, err = bridge.DefaultBridge(backend) } else { - b, err = bridge.NewBridgeFullName(backend, args[0]) + b, err = bridge.NewBridgeFromFullName(backend, args[0]) } if err != nil { -- cgit