Permalink
Browse files

githubapi: Skip cache if missing thread in MarkRead.

There are sometimes caching issues causing stale repository
notifications to be retrieved from cache. So fall back to skipping
cache if we can't find our thread.
  • Loading branch information...
dmitshur committed Aug 30, 2017
1 parent 5a67cc8 commit 97c8d1fa04c4d01bd75a49e8f30eb5f6919b5b4a
Showing with 85 additions and 28 deletions.
  1. +51 −28 githubapi/githubapi.go
  2. +34 −0 githubapi/githubapi_test.go
View
@@ -13,6 +13,7 @@ import (
"github.com/google/go-github/github"
"github.com/google/go-querystring/query"
"github.com/gregjones/httpcache"
"github.com/shurcooL/githubql"
"github.com/shurcooL/notifications"
"github.com/shurcooL/users"
@@ -27,6 +28,8 @@ import (
// Otherwise read notifications remain forever (until a new notification comes in).
//
// This service uses Cache-Control: no-cache request header to disable caching.
// Responses from cache must be marked with "X-From-Cache" header (i.e., the field
// MarkCachedResponses in httpcache.Transport must be set to true).
//
// If router is nil, GitHubRouter is used, which links to https://github.com.
func NewService(clientV3 *github.Client, clientV4 *githubql.Client, router Router) notifications.Service {
@@ -311,46 +314,66 @@ func (s service) MarkRead(ctx context.Context, rs notifications.RepoSpec, thread
}
// It's okay to use with-cache client here, because we don't mind seeing read notifications
// for the purposes of MarkRead. They'll be skipped if the notification ID doesn't match.
ns, _, err := s.clV3.Activity.ListRepositoryNotifications(ctx, repo.Owner, repo.Repo, nil)
if err != nil {
return fmt.Errorf("failed to ListRepositoryNotifications: %v", err)
}
for _, n := range ns {
if *n.Subject.Type != threadType {
continue
// However, there are sometimes caching issues causing stale repository notifications to
// be retrieved from cache. So fall back to skipping cache if we can't find our thread.
for _, cache := range []bool{true, false} {
ns, resp, err := ghListRepositoryNotifications(ctx, s.clV3, repo.Owner, repo.Repo, nil, cache)
if err != nil {
return fmt.Errorf("failed to ListRepositoryNotifications: %v", err)
}
for _, n := range ns {
if *n.Subject.Type != threadType {
continue
}
var id uint64
switch *n.Subject.Type {
case "Issue":
_, id, err = parseIssueSpec(*n.Subject.URL)
if err != nil {
return fmt.Errorf("failed to parseIssueSpec: %v", err)
var id uint64
switch *n.Subject.Type {
case "Issue":
_, id, err = parseIssueSpec(*n.Subject.URL)
if err != nil {
return fmt.Errorf("failed to parseIssueSpec: %v", err)
}
case "PullRequest":
_, id, err = parsePullRequestSpec(*n.Subject.URL)
if err != nil {
return fmt.Errorf("failed to parsePullRequestSpec: %v", err)
}
case "Commit", "Release", "RepositoryInvitation":
// These thread types are already handled on top, so skip them.
continue
default:
return fmt.Errorf("MarkRead: unsupported *n.Subject.Type: %v", *n.Subject.Type)
}
case "PullRequest":
_, id, err = parsePullRequestSpec(*n.Subject.URL)
if id != threadID {
continue
}
_, err = s.clV3.Activity.MarkThreadRead(ctx, *n.ID)
if err != nil {
return fmt.Errorf("failed to parsePullRequestSpec: %v", err)
return fmt.Errorf("MarkRead: failed to MarkThreadRead: %v", err)
}
case "Commit", "Release", "RepositoryInvitation":
// These thread types are already handled on top, so skip them.
continue
default:
return fmt.Errorf("MarkRead: unsupported *n.Subject.Type: %v", *n.Subject.Type)
}
if id != threadID {
continue
return nil
}
log.Printf("MarkRead: did not find notification %s/%d within %d notifications of repo %s:\n%v", threadType, threadID, len(ns), rs.URI, notificationsString(ns))
_, err = s.clV3.Activity.MarkThreadRead(ctx, *n.ID)
if err != nil {
return fmt.Errorf("MarkRead: failed to MarkThreadRead: %v", err)
// If the response we got is from origin server rather than from cache, don't try again.
_, fromCache := resp.Response.Header[httpcache.XFromCache]
if !fromCache {
break
}
return nil
}
return nil
}
// notificationsString returns a string representation of notifications ns.
func notificationsString(ns []*github.Notification) string {
var s string
for _, n := range ns {
s += "\t" + strings.TrimPrefix(*n.Subject.URL, "https://api.github.com/") + "\n"
}
return s
}
func (s service) MarkAllRead(ctx context.Context, rs notifications.RepoSpec) error {
repo, err := ghRepoSpec(rs)
if err != nil {
@@ -27,3 +27,37 @@ func TestAvatarURLSize(t *testing.T) {
t.Errorf("got %q, want %q", got, want)
}
}
func TestNotificationsString(t *testing.T) {
ns := []*github.Notification{
{
Subject: &github.NotificationSubject{
URL: github.String("https://api.github.com/repos/neugram/ng/issues/10"),
Type: github.String("Issue"),
},
ID: github.String("271670023"),
},
{
Subject: &github.NotificationSubject{
URL: github.String("https://api.github.com/repos/neugram/ng/pulls/22"),
Type: github.String("PullRequest"),
},
ID: github.String("271863360"),
},
{
Subject: &github.NotificationSubject{
URL: github.String("https://api.github.com/repos/neugram/ng/pulls/21"),
Type: github.String("PullRequest"),
},
ID: github.String("271857043"),
},
}
got := notificationsString(ns)
want := ` repos/neugram/ng/issues/10
repos/neugram/ng/pulls/22
repos/neugram/ng/pulls/21
`
if got != want {
t.Errorf("got %q, want %q", got, want)
}
}

0 comments on commit 97c8d1f

Please sign in to comment.