maintner: start adding issue bodies to the corpus

When we get an issue from Github, add the issue and associated
details to the Corpus. Check whether the `Updated` timestamp on an
issue is newer than the one we have in the Corpus; if the timestamp
is the same, we can stop processing since we are sorting by the
updated date and no older issues will have new data.

If the mutation changes the Corpus, add it to an array of Mutations
that we will eventually flush to disk.

Adds a helper function to convert a github.Issue into a
maintpb.Mutation - this way we can reuse the logic for adding data to
the Corpus. Adds some more tests around data processing.

Change-Id: I9475e293941784179420ca7b638917943be1f29e
Reviewed-on: https://go-review.googlesource.com/37490
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: kevinburke

maintner/maintner.go

@@ -12,13 +12,15 @@ package maintner
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"log"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/golang/protobuf/ptypes"
 	"github.com/google/go-github/github"
 	"golang.org/x/build/maintner/maintpb"
 	"golang.org/x/oauth2"
@@ -75,6 +77,24 @@ func (c *Corpus) AddGithub(owner, repo, tokenFile string) {
 // such as "golang/go".
 type githubRepo string
 
+// Org finds "golang" in the githubRepo string "golang/go", or returns an empty
+// string if it is malformed.
+func (gr githubRepo) Org() string {
+	sep := strings.IndexByte(string(gr), '/')
+	if sep == -1 {
+		return ""
+	}
+	return string(gr[:sep])
+}
+
+func (gr githubRepo) Repo() string {
+	sep := strings.IndexByte(string(gr), '/')
+	if sep == -1 || sep == len(gr)-1 {
+		return ""
+	}
+	return string(gr[sep+1:])
+}
+
 // githubUser represents a github user.
 // It is a subset of https://developer.github.com/v3/users/#get-a-single-user
 type githubUser struct {
@@ -169,7 +189,80 @@ func (c *Corpus) getGithubUser(pu *maintpb.GithubUser) *githubUser {
 	return u
 }
 
-func (c *Corpus) processGithubIssueMutation(m *maintpb.GithubIssueMutation) {
+var errNoChanges = errors.New("No changes in this github.Issue")
+
+// newMutationFromIssue generates a GithubIssueMutation using the smallest
+// possible diff between ci (a corpus Issue) and gi (an external github issue).
+//
+// If newMutationFromIssue returns nil, the provided github.Issue is no newer
+// than the data we have in the corpus. ci may be nil.
+func newMutationFromIssue(ci *githubIssue, gi *github.Issue, rp githubRepo) *maintpb.GithubIssueMutation {
+	if gi == nil || gi.Number == nil {
+		panic(fmt.Sprintf("github issue with nil number: %#v", gi))
+	}
+	owner, repo := rp.Org(), rp.Repo()
+	// always need these fields to figure out which key to write to
+	m := &maintpb.GithubIssueMutation{
+		Owner:  owner,
+		Repo:   repo,
+		Number: int32(*gi.Number),
+	}
+	if ci == nil {
+		// We don't know about this github issue, so populate all fields in one
+		// mutation.
+		if gi.CreatedAt != nil {
+			tproto, err := ptypes.TimestampProto(*gi.CreatedAt)
+			if err != nil {
+				panic(err)
+			}
+			m.Created = tproto
+		}
+		if gi.UpdatedAt != nil {
+			tproto, err := ptypes.TimestampProto(*gi.UpdatedAt)
+			if err != nil {
+				panic(err)
+			}
+			m.Updated = tproto
+		}
+		if gi.Body != nil {
+			m.Body = *gi.Body
+		}
+		return m
+	}
+	if gi.UpdatedAt != nil {
+		if gi.UpdatedAt.Before(ci.Updated) {
+			// This data is stale, ignore it.
+			return nil
+		}
+		tproto, err := ptypes.TimestampProto(*gi.UpdatedAt)
+		if err != nil {
+			panic(err)
+		}
+		m.Updated = tproto
+	}
+	if gi.Body != nil && *gi.Body != ci.Body {
+		m.Body = *gi.Body
+	}
+	return m
+}
+
+// getIssue finds an issue in the Corpus or returns nil, false if it is not
+// present.
+func (c *Corpus) getIssue(rp githubRepo, number int32) (*githubIssue, bool) {
+	issueMap, ok := c.githubIssues[rp]
+	if !ok {
+		return nil, false
+	}
+	gi, ok := issueMap[number]
+	return gi, ok
+}
+
+// processGithubIssueMutation updates the corpus with the information in m, and
+// returns true if the Corpus was modified.
+func (c *Corpus) processGithubIssueMutation(m *maintpb.GithubIssueMutation) (changed bool) {
+	if c == nil {
+		panic("nil corpus")
+	}
 	k := c.repoKey(m.Owner, m.Repo)
 	if k == "" {
 		// TODO: errors? return false? skip for now.
@@ -188,16 +281,39 @@ func (c *Corpus) processGithubIssueMutation(m *maintpb.GithubIssueMutation) {
 	}
 	gi, ok := issueMap[m.Number]
 	if !ok {
+		created, err := ptypes.Timestamp(m.Created)
+		if err != nil {
+			panic(err)
+		}
 		gi = &githubIssue{
-			Number: m.Number,
-			User:   c.getGithubUser(m.User),
+			Number:  m.Number,
+			User:    c.getGithubUser(m.User),
+			Created: created,
 		}
 		issueMap[m.Number] = gi
+		changed = true
+	}
+	// Check Updated before all other fields so they don't update if this
+	// Mutation is stale
+	if m.Updated != nil {
+		updated, err := ptypes.Timestamp(m.Updated)
+		if err != nil {
+			panic(err)
+		}
+		if !updated.IsZero() && updated.Before(gi.Updated) {
+			// this mutation represents data older than the data we have in
+			// the corpus; ignore it.
+			return false
+		}
+		gi.Updated = updated
+		changed = changed || updated.After(gi.Updated)
 	}
 	if m.Body != "" {
 		gi.Body = m.Body
+		changed = changed || m.Body != gi.Body
 	}
-	// TODO: times, etc.
+	// ignoring Created since it *should* never update
+	return changed
 }
 
 // PopulateFromServer populates the corpus from a maintnerd server.
@@ -257,14 +373,16 @@ func (c *Corpus) pollGithub(ctx context.Context, rp githubRepo, ghc *github.Clie
 	log.Printf("Polling github for %s ...", rp)
 	page := 1
 	keepGoing := true
-	splits := strings.SplitN(string(rp), "/", 2)
-	owner, repo := splits[0], splits[1]
+	owner, repo := rp.Org(), rp.Repo()
 	for keepGoing {
 		// TODO: use https://godoc.org/github.com/google/go-github/github#ActivityService.ListIssueEventsForRepository probably
-		issues, res, err := ghc.Issues.ListByRepo(context.TODO(), owner, repo, &github.IssueListByRepoOptions{
+		issues, _, err := ghc.Issues.ListByRepo(ctx, owner, repo, &github.IssueListByRepoOptions{
 			State:     "all",
 			Sort:      "updated",
 			Direction: "desc",
+			// TODO: if an issue gets updated while we are paging, we might
+			// process the same issue twice - as item 100 on page 1 and then
+			// again as item 1 on page 2.
 			ListOptions: github.ListOptions{
 				Page:    page,
 				PerPage: 100,
@@ -273,15 +391,22 @@ func (c *Corpus) pollGithub(ctx context.Context, rp githubRepo, ghc *github.Clie
 		if err != nil {
 			return err
 		}
-		log.Printf("github %s/%s: page %d, num issues %d, res: %#v", owner, repo, page, len(issues), res)
-		keepGoing = false
+		log.Printf("github %s/%s: page %d, num issues %d", owner, repo, page, len(issues))
+		if len(issues) == 0 {
+			break
+		}
+		c.mu.Lock()
 		for _, is := range issues {
 			fmt.Printf("issue %d: %s\n", is.ID, *is.Title)
-			changes := false
-			if changes {
-				keepGoing = true
+			gi, _ := c.getIssue(rp, int32(*is.Number))
+			mp := newMutationFromIssue(gi, is, rp)
+			if mp == nil {
+				keepGoing = false
+				break
 			}
+			c.processGithubIssueMutation(mp)
 		}
+		c.mu.Unlock()
 		page++
 	}
 	return nil

maintner/maintner_test.go

@@ -7,7 +7,11 @@ package maintner
 import (
 	"reflect"
 	"testing"
+	"time"
 
+	"github.com/golang/protobuf/ptypes"
+	google_protobuf "github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/google/go-github/github"
 	"golang.org/x/build/maintner/maintpb"
 )
 
@@ -18,6 +22,9 @@ type mutationTest struct {
 
 func (mt mutationTest) test(t *testing.T, muts ...*maintpb.Mutation) {
 	c := mt.corpus
+	if c == nil {
+		c = NewCorpus()
+	}
 	for _, m := range muts {
 		c.processMutationLocked(m)
 	}
@@ -26,26 +33,71 @@ func (mt mutationTest) test(t *testing.T, muts ...*maintpb.Mutation) {
 	}
 }
 
+var t1, t2 time.Time
+var tp1, tp2 *google_protobuf.Timestamp
+
+func init() {
+	t1, _ = time.Parse(time.RFC3339, "2016-01-02T15:04:00Z")
+	t2, _ = time.Parse(time.RFC3339, "2016-01-02T15:30:00Z")
+	tp1, _ = ptypes.TimestampProto(t1)
+	tp2, _ = ptypes.TimestampProto(t2)
+}
+
 func TestProcessMutation_Github_NewIssue(t *testing.T) {
-	mutationTest{
-		want: &Corpus{
-			githubUsers: map[int64]*githubUser{
-				100: &githubUser{
-					Login: "gopherbot",
-					ID:    100,
-				},
+	c := NewCorpus()
+	c.githubUsers = map[int64]*githubUser{
+		100: &githubUser{
+			Login: "gopherbot",
+			ID:    100,
+		},
+	}
+	c.githubIssues = map[githubRepo]map[int32]*githubIssue{
+		"golang/go": map[int32]*githubIssue{
+			3: &githubIssue{
+				Number:  3,
+				User:    &githubUser{ID: 100, Login: "gopherbot"},
+				Body:    "some body",
+				Created: t1,
 			},
-			githubIssues: map[githubRepo]map[int32]*githubIssue{
-				"golang/go": map[int32]*githubIssue{
-					3: &githubIssue{
-						Number: 3,
-						User:   &githubUser{ID: 100, Login: "gopherbot"},
-						Body:   "some body",
-					},
-				},
+		},
+	}
+	mutationTest{want: c}.test(t, &maintpb.Mutation{
+		GithubIssue: &maintpb.GithubIssueMutation{
+			Owner:  "golang",
+			Repo:   "go",
+			Number: 3,
+			User: &maintpb.GithubUser{
+				Login: "gopherbot",
+				Id:    100,
 			},
+			Body:    "some body",
+			Created: tp1,
 		},
-	}.test(t, &maintpb.Mutation{
+	})
+}
+
+func TestProcessMutation_OldIssue(t *testing.T) {
+	// process a mutation with an Updated timestamp older than the existing
+	// issue.
+	c := NewCorpus()
+	c.githubUsers = map[int64]*githubUser{
+		100: &githubUser{
+			Login: "gopherbot",
+			ID:    100,
+		},
+	}
+	c.githubIssues = map[githubRepo]map[int32]*githubIssue{
+		"golang/go": map[int32]*githubIssue{
+			3: &githubIssue{
+				Number:  3,
+				User:    &githubUser{ID: 100, Login: "gopherbot"},
+				Body:    "some body",
+				Created: t2,
+				Updated: t2,
+			},
+		},
+	}
+	mutationTest{want: c}.test(t, &maintpb.Mutation{
 		GithubIssue: &maintpb.GithubIssueMutation{
 			Owner:  "golang",
 			Repo:   "go",
@@ -54,7 +106,71 @@ func TestProcessMutation_Github_NewIssue(t *testing.T) {
 				Login: "gopherbot",
 				Id:    100,
 			},
-			Body: "some body",
+			Body:    "some body",
+			Created: tp2,
+			Updated: tp2,
+		},
+	}, &maintpb.Mutation{
+		// The second issue is older than the first.
+		GithubIssue: &maintpb.GithubIssueMutation{
+			Owner:  "golang",
+			Repo:   "go",
+			Number: 3,
+			User: &maintpb.GithubUser{
+				Login: "gopherbot",
+				Id:    100,
+			},
+			Body:    "issue body changed",
+			Created: tp1,
+			Updated: tp1,
 		},
 	})
 }
+
+func TestNewMutationsFromIssue(t *testing.T) {
+	gh := &github.Issue{
+		Number:    github.Int(5),
+		CreatedAt: &t1,
+		UpdatedAt: &t2,
+		Body:      github.String("body of the issue"),
+		State:     github.String("closed"),
+	}
+	is := newMutationFromIssue(nil, gh, githubRepo("golang/go"))
+	want := &maintpb.GithubIssueMutation{
+		Owner:   "golang",
+		Repo:    "go",
+		Number:  5,
+		Body:    "body of the issue",
+		Created: tp1,
+		Updated: tp2,
+	}
+	if !reflect.DeepEqual(is, want) {
+		t.Errorf("issue mismatch\n got: %#v\nwant: %#v", is, want)
+	}
+}
+
+func TestGithubRepoOrg(t *testing.T) {
+	gr := githubRepo("golang/go")
+	want := "golang"
+	if org := gr.Org(); org != want {
+		t.Errorf("githubRepo(\"%s\").Org(): got %s, want %s", gr, org, want)
+	}
+	gr = githubRepo("unknown format")
+	want = ""
+	if org := gr.Org(); org != want {
+		t.Errorf("githubRepo(\"%s\").Org(): got %s, want %s", gr, org, want)
+	}
+}
+
+func TestGithubRepo(t *testing.T) {
+	gr := githubRepo("golang/go")
+	want := "go"
+	if repo := gr.Repo(); repo != want {
+		t.Errorf("githubRepo(\"%s\").Repo(): got %s, want %s", gr, repo, want)
+	}
+	gr = githubRepo("bad/")
+	want = ""
+	if repo := gr.Repo(); repo != want {
+		t.Errorf("githubRepo(\"%s\").Repo(): got %s, want %s", gr, repo, want)
+	}
+}