Create delta webhooks in GithubService.Register()

Remove PostRegister() as it isn't used for anything anymore. Add TODO marker
for guarding against race conditions.

src/github.com/matrix-org/go-neb/api.go

@@ -211,29 +211,19 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac
 		return nil, &errors.HTTPError{nil, "Unsupported Method", 405}
 	}
 
-	var body struct {
-		ID     string
-		Type   string
-		UserID string
-		Config json.RawMessage
-	}
-	if err := json.NewDecoder(req.Body).Decode(&body); err != nil {
-		return nil, &errors.HTTPError{err, "Error parsing request JSON", 400}
+	service, httpErr := s.createService(req)
+	if httpErr != nil {
+		return nil, httpErr
 	}
 
-	if body.ID == "" || body.Type == "" || body.UserID == "" || body.Config == nil {
-		return nil, &errors.HTTPError{
-			nil, `Must supply an "ID", a "Type", a "UserID" and a "Config"`, 400,
-		}
-	}
+	// TODO mutex lock keyed off service ID
 
-	service, err := types.CreateService(body.ID, body.Type, body.UserID, body.Config)
-	if err != nil {
-		return nil, &errors.HTTPError{err, "Error parsing config JSON", 400}
+	old, err := s.db.LoadService(service.ServiceID())
+	if err != nil && err != sql.ErrNoRows {
+		return nil, &errors.HTTPError{err, "Error loading old service", 500}
 	}
 
-	err = service.Register()
-	if err != nil {
+	if err = service.Register(old); err != nil {
 		return nil, &errors.HTTPError{err, "Failed to register service: " + err.Error(), 500}
 	}
 
@@ -247,14 +237,38 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac
 		return nil, &errors.HTTPError{err, "Error storing service", 500}
 	}
 
-	service.PostRegister(oldService)
+	// TODO mutex unlock keyed off service ID
 
 	return &struct {
 		ID        string
 		Type      string
 		OldConfig types.Service
 		NewConfig types.Service
-	}{body.ID, body.Type, oldService, service}, nil
+	}{service.ServiceID(), service.ServiceType(), oldService, service}, nil
+}
+
+func (s *configureServiceHandler) createService(req *http.Request) (types.Service, *errors.HTTPError) {
+	var body struct {
+		ID     string
+		Type   string
+		UserID string
+		Config json.RawMessage
+	}
+	if err := json.NewDecoder(req.Body).Decode(&body); err != nil {
+		return nil, &errors.HTTPError{err, "Error parsing request JSON", 400}
+	}
+
+	if body.ID == "" || body.Type == "" || body.UserID == "" || body.Config == nil {
+		return nil, &errors.HTTPError{
+			nil, `Must supply an "ID", a "Type", a "UserID" and a "Config"`, 400,
+		}
+	}
+
+	service, err := types.CreateService(body.ID, body.Type, body.UserID, body.Config)
+	if err != nil {
+		return nil, &errors.HTTPError{err, "Error parsing config JSON", 400}
+	}
+	return service, nil
 }
 
 type getServiceHandler struct {

src/github.com/matrix-org/go-neb/services/echo/echo.go

@@ -13,11 +13,10 @@ type echoService struct {
 	serviceUserID string
 }
 
-func (e *echoService) ServiceUserID() string          { return e.serviceUserID }
-func (e *echoService) ServiceID() string              { return e.id }
-func (e *echoService) ServiceType() string            { return "echo" }
-func (e *echoService) Register() error                { return nil }
-func (e *echoService) PostRegister(old types.Service) {}
+func (e *echoService) ServiceUserID() string                   { return e.serviceUserID }
+func (e *echoService) ServiceID() string                       { return e.id }
+func (e *echoService) ServiceType() string                     { return "echo" }
+func (e *echoService) Register(oldService types.Service) error { return nil }
 func (e *echoService) Plugin(roomID string) plugin.Plugin {
 	return plugin.Plugin{
 		Commands: []plugin.Command{

src/github.com/matrix-org/go-neb/services/github/github.go

@@ -11,6 +11,7 @@ import (
 	"github.com/matrix-org/go-neb/services/github/client"
 	"github.com/matrix-org/go-neb/services/github/webhook"
 	"github.com/matrix-org/go-neb/types"
+	"github.com/matrix-org/go-neb/util"
 	"net/http"
 	"regexp"
 	"sort"
@@ -203,7 +204,7 @@ func (s *githubService) OnReceiveWebhook(w http.ResponseWriter, req *http.Reques
 //
 // Hooks can get out of sync if a user manually deletes a hook in the Github UI. In this case, toggling the repo configuration will
 // force NEB to recreate the hook.
-func (s *githubService) Register() error {
+func (s *githubService) Register(oldService types.Service) error {
 	if s.RealmID == "" {
 		return fmt.Errorf("RealmID is required")
 	}
@@ -225,108 +226,88 @@ func (s *githubService) Register() error {
 				"User %s does not have a Github auth session with realm %s.", s.ClientUserID, realm.ID())
 		}
 		// Make sure they have specified some webhooks (it makes no sense otherwise)
-
-		// Work out
-
-	}
-
-	log.Infof("%+v", s)
-
-	return nil
-}
-
-func (s *githubService) PostRegister(oldService types.Service) {
-	// PostRegister handles creating/destroying webhooks, which is only valid if this service
-	// is configured on behalf of a client.
-	if s.ClientUserID == "" {
-		if len(s.Rooms) != 0 {
-			log.WithFields(log.Fields{
-				"Rooms": s.Rooms,
-			}).Error("Empty ClientUserID but a webhook config was supplied.")
+		reposForWebhooks := s.repoList()
+		if len(reposForWebhooks) == 0 {
+			return fmt.Errorf("No repos for webhooks specified")
 		}
-		return
-	}
 
-	cli := s.githubClientFor(s.ClientUserID, false)
-	if cli == nil {
-		log.Errorf("PostRegister: %s does not have a github session", s.ClientUserID)
-		return
-	}
-
-	if oldService != nil {
+		// Fetch the old service list and work out the difference between the two.
+		var oldRepos []string
 		old, ok := oldService.(*githubService)
 		if !ok {
-			log.Error("PostRegister: Provided old service is not of type GithubService")
-			return
+			log.WithFields(log.Fields{
+				"service_id":   oldService.ServiceID(),
+				"service_type": oldService.ServiceType(),
+			}).Print("Cannot case old github service to GithubService")
+			// non-fatal though, we'll just make the hooks
+		} else {
+			oldRepos = old.repoList()
 		}
 
-		// Don't spam github webhook requests if we can help it.
-		if sameRepos(s, old) {
-			log.Print("PostRegister: old and new services have the same repo set. Nooping.")
-			return
+		// Add the repos in the new service but not the old service
+		newRepos, _ := util.Difference(reposForWebhooks, oldRepos)
+		for _, r := range newRepos {
+			logger := log.WithField("repo", r)
+			err := s.createHook(cli, r)
+			if err != nil {
+				logger.WithError(err).Error("Failed to create webhook")
+				return err
+			}
+			logger.Info("Created webhook")
 		}
-
-		// TODO: We should be adding webhooks in Register() then removing old hooks in PostRegister()
-		//
-		// By doing both operations in PostRegister(), if some of the requests fail we can end up in
-		// an inconsistent state. It is a lot simpler and easy to reason about this way though, so
-		// for now it will do.
-
-		// remove any existing webhooks this service created on the user's behalf
-		modifyWebhooks(old, cli, true)
 	}
 
-	// make new webhooks according to service config
-	modifyWebhooks(s, cli, false)
+	log.Infof("%+v", s)
+
+	return nil
 }
 
-func modifyWebhooks(s *githubService, cli *github.Client, removeHooks bool) {
-	ownerRepoSet := make(map[string]bool)
-	for _, roomCfg := range s.Rooms {
-		for ownerRepo := range roomCfg.Repos {
-			// sanity check that it looks like 'owner/repo' as we'll split on / later
+func (s *githubService) repoList() []string {
+	var repos []string
+	if s.Rooms == nil {
+		return repos
+	}
+	for _, roomConfig := range s.Rooms {
+		for ownerRepo := range roomConfig.Repos {
 			if strings.Count(ownerRepo, "/") != 1 {
-				log.WithField("owner_repo", ownerRepo).Print("Bad owner/repo value.")
+				log.WithField("repo", ownerRepo).Error("Bad owner/repo key in config")
 				continue
 			}
-			ownerRepoSet[ownerRepo] = true
-		}
-	}
-
-	for ownerRepo := range ownerRepoSet {
-		o := strings.Split(ownerRepo, "/")
-		owner := o[0]
-		repo := o[1]
-		logger := log.WithFields(log.Fields{
-			"owner": owner,
-			"repo":  repo,
-		})
-		if removeHooks {
-			removeHook(logger, cli, owner, repo, s.webhookEndpointURL)
-		} else {
-			// make a hook for all GH events since we'll filter it when we receive webhook requests
-			name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook
-			cfg := map[string]interface{}{
-				"content_type": "json",
-				"url":          s.webhookEndpointURL,
-			}
-			if s.SecretToken != "" {
-				cfg["secret"] = s.SecretToken
+			exists := false
+			for _, r := range repos {
+				if r == ownerRepo {
+					exists = true
+					break
+				}
 			}
-			events := []string{"push", "pull_request", "issues", "issue_comment", "pull_request_review_comment"}
-			_, _, err := cli.Repositories.CreateHook(owner, repo, &github.Hook{
-				Name:   &name,
-				Config: cfg,
-				Events: events,
-			})
-			if err != nil {
-				logger.WithError(err).Print("Failed to create webhook")
-				// continue as others may succeed
-			} else {
-				logger.WithField("endpoint", s.webhookEndpointURL).Print("Created hook with endpoint")
+			if !exists {
+				repos = append(repos, ownerRepo)
 			}
 		}
 	}
+	return repos
+}
+
+func (s *githubService) createHook(cli *github.Client, ownerRepo string) error {
+	o := strings.Split(ownerRepo, "/")
+	owner := o[0]
+	repo := o[1]
+	// make a hook for all GH events since we'll filter it when we receive webhook requests
+	name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook
+	cfg := map[string]interface{}{
+		"content_type": "json",
+		"url":          s.webhookEndpointURL,
+	}
+	if s.SecretToken != "" {
+		cfg["secret"] = s.SecretToken
+	}
+	events := []string{"push", "pull_request", "issues", "issue_comment", "pull_request_review_comment"}
+	_, _, err := cli.Repositories.CreateHook(owner, repo, &github.Hook{
+		Name:   &name,
+		Config: cfg,
+		Events: events,
+	})
+	return err
 }
 
 func sameRepos(a *githubService, b *githubService) bool {

src/github.com/matrix-org/go-neb/services/jira/jira.go

@@ -41,7 +41,7 @@ type jiraService struct {
 func (s *jiraService) ServiceUserID() string { return s.serviceUserID }
 func (s *jiraService) ServiceID() string     { return s.id }
 func (s *jiraService) ServiceType() string   { return "jira" }
-func (s *jiraService) Register() error {
+func (s *jiraService) Register(oldService types.Service) error {
 	// We only ever make 1 JIRA webhook which listens for all projects and then filter
 	// on receive. So we simply need to know if we need to make a webhook or not. We
 	// need to do this for each unique realm.
@@ -61,9 +61,6 @@ func (s *jiraService) Register() error {
 	}
 	return nil
 }
-func (s *jiraService) PostRegister(old types.Service) {
-	// TODO: We don't remove old JIRA webhooks for now. Let the admin sort it out.
-}
 
 func (s *jiraService) cmdJiraCreate(roomID, userID string, args []string) (interface{}, error) {
 	// E.g jira create PROJ "Issue title" "Issue desc"

src/github.com/matrix-org/go-neb/types/types.go

@@ -38,8 +38,7 @@ type Service interface {
 	ServiceType() string
 	Plugin(roomID string) plugin.Plugin
 	OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client)
-	Register() error
-	PostRegister(oldService Service)
+	Register(oldService Service) error
 }
 
 var baseURL = ""

src/github.com/matrix-org/go-neb/util/util.go

@@ -0,0 +1,35 @@
+package util
+
+import (
+	"sort"
+)
+
+// Difference returns the elements that are only in the first list and
+// the elements that are only in the second. As a side-effect this sorts
+// the input lists in-place.
+func Difference(a, b []string) (onlyA, onlyB []string) {
+	sort.Strings(a)
+	sort.Strings(b)
+	for {
+		if len(b) == 0 {
+			onlyA = append(onlyA, a...)
+			return
+		}
+		if len(a) == 0 {
+			onlyB = append(onlyB, b...)
+			return
+		}
+		xA := a[0]
+		xB := b[0]
+		if xA < xB {
+			onlyA = append(onlyA, xA)
+			a = a[1:]
+		} else if xA > xB {
+			onlyB = append(onlyB, xB)
+			b = b[1:]
+		} else {
+			a = a[1:]
+			b = b[1:]
+		}
+	}
+}