github: deal with the deleted user case where github return a null actor

Michael Muré created 7 years ago

Change summary

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(-)

Detailed changes

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

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)
 }

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
 }

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
 }
 

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)

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)"`
+}

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 {