refactor: Extract AppEngine Cron source check into middleware (#99)

This de-duplicates the code we have to check the AppEngine Cron request
header and pushes it into the HTTP router and out of the app logic.

As a convenient side effect, this also ensures that any cron job failure
produces an HTTP 500 response (indicating failure to AppEngine) and an
ERROR-severity log line (which we're starting to use for alerts).

main.go

@@ -127,12 +127,12 @@ func main() {
 		welcomeStream: welcomeStream,
 	}
 
-	http.HandleFunc("/", http.NotFound)           // will this handle anything that's not defined?
-	http.HandleFunc("/webhooks", pl.handle)       // from zulip
-	http.HandleFunc("/match", pl.match)           // from GCP- daily
-	http.HandleFunc("/endofbatch", pl.endofbatch) // from GCP- weekly
-	http.HandleFunc("/welcome", pl.welcome)       // from GCP- weekly
-	http.HandleFunc("/checkin", pl.checkin)       // from GCP- weekly
+	http.HandleFunc("/", http.NotFound)                 // will this handle anything that's not defined?
+	http.HandleFunc("/webhooks", pl.handle)             // from zulip
+	http.HandleFunc("/match", cron(pl.Match))           // from GCP- daily
+	http.HandleFunc("/endofbatch", cron(pl.EndOfBatch)) // from GCP- weekly
+	http.HandleFunc("/welcome", cron(pl.Welcome))       // from GCP- weekly
+	http.HandleFunc("/checkin", cron(pl.Checkin))       // from GCP- weekly
 
 	port := os.Getenv("PORT")
 	if port == "" {

middleware.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"context"
+	"log/slog"
+	"net/http"
+)
+
+// JobFunc is the type of function that can run as a cron job.
+type JobFunc func(context.Context) error
+
+// cron wraps a job function to make it an HTTP handler. The handler enforces
+// that requests originate from App Engine's Cron scheduler.
+func cron(job JobFunc) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		// Check that the request is originating from within app engine
+		// https://cloud.google.com/appengine/docs/standard/go/scheduling-jobs-with-cron-yaml#validating_cron_requests
+		if r.Header.Get("X-Appengine-Cron") != "true" {
+			http.NotFound(w, r)
+			return
+		}
+
+		err := job(r.Context())
+		if err != nil {
+			slog.Error("Job failed", slog.Any("error", err))
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+	}
+}

middleware_test.go

@@ -0,0 +1,76 @@
+package main
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/recursecenter/pairing-bot/internal/assert"
+)
+
+func Test_cron(t *testing.T) {
+	t.Run("run job for AppEngine", func(t *testing.T) {
+		// Arrange a cron job that tells us whether it ran.
+		ran := false
+		handler := cron(func(context.Context) error {
+			ran = true
+			return nil
+		})
+
+		// Prepare an AppEngine-sourced request.
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.Header.Set("X-Appengine-Cron", "true")
+
+		// Run it!
+		w := httptest.NewRecorder()
+		handler(w, req)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+
+		assert.Equal(t, ran, true)
+		assert.Equal(t, resp.StatusCode, 200)
+	})
+
+	t.Run("deny request outside of cron", func(t *testing.T) {
+		// Arrange a cron job that fails the test if it runs.
+		handler := cron(func(context.Context) error {
+			t.Error("handler should not have run")
+			return nil
+		})
+
+		// Prepare a request from outside of AppEngine (no custom header).
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+
+		// Run it!
+		w := httptest.NewRecorder()
+		handler(w, req)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+
+		assert.Equal(t, resp.StatusCode, 404)
+	})
+
+	t.Run("report job failure", func(t *testing.T) {
+		// Arrange a cron job that errors.
+		handler := cron(func(context.Context) error {
+			return errors.New("test error")
+		})
+
+		// Prepare an AppEngine-sourced request.
+		req := httptest.NewRequest(http.MethodGet, "/", nil)
+		req.Header.Set("X-Appengine-Cron", "true")
+
+		// Run it!
+		w := httptest.NewRecorder()
+		handler(w, req)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+
+		assert.Equal(t, resp.StatusCode, 500)
+	})
+}

pairing_bot.go

@@ -1,10 +1,10 @@
 package main
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"log"
-	"log/slog"
 	"math/rand"
 	"net/http"
 	"slices"
@@ -142,27 +142,17 @@ func (pl *PairingLogic) handle(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-// "match" makes matches for pairing, and messages those people to notify them of their match
-// it runs once per day (it's triggered with app engine's cron service)
-func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) {
-	// Check that the request is originating from within app engine
-	// https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests
-	if r.Header.Get("X-Appengine-Cron") != "true" {
-		http.NotFound(w, r)
-		return
-	}
-
-	ctx := r.Context()
-
+// Match generates new pairs for today and sends notifications for them.
+func (pl *PairingLogic) Match(ctx context.Context) error {
 	recursersList, err := pl.rdb.ListPairingTomorrow(ctx)
 	log.Println(recursersList)
 	if err != nil {
-		log.Printf("Could not get list of recursers from DB: %s\n", err)
+		return fmt.Errorf("get today's recursers from DB: %w", err)
 	}
 
 	skippersList, err := pl.rdb.ListSkippingTomorrow(ctx)
 	if err != nil {
-		log.Printf("Could not get list of skippers from DB: %s\n", err)
+		return fmt.Errorf("get today's skippers from DB: %w", err)
 	}
 
 	// get everyone who was set to skip today and set them back to isSkippingTomorrow = false
@@ -187,7 +177,7 @@ func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) {
 	// if for some reason there's no matches today, we're done
 	if len(recursersList) == 0 {
 		log.Println("No one was signed up to pair today -- so there were no matches")
-		return
+		return nil
 	}
 
 	// message the peeps!
@@ -229,30 +219,21 @@ func (pl *PairingLogic) match(w http.ResponseWriter, r *http.Request) {
 	if err := pl.pdb.SetNumPairings(ctx, pairing); err != nil {
 		log.Printf("Failed to record today's pairings: %s", err)
 	}
-}
 
-// Unsubscribe people from Pairing Bot when their batch is over. They're always welcome to re-subscribe manually!
-func (pl *PairingLogic) endofbatch(w http.ResponseWriter, r *http.Request) {
-	// Check that the request is originating from within app engine
-	// https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests
-	if r.Header.Get("X-Appengine-Cron") != "true" {
-		http.NotFound(w, r)
-		return
-	}
+	return nil
+}
 
+// EndOfBatch unsubscribes everyone who just never-graduated with this batch.
+func (pl *PairingLogic) EndOfBatch(ctx context.Context) error {
 	// getting all the recursers
-	ctx := r.Context()
 	recursersList, err := pl.rdb.GetAllUsers(ctx)
 	if err != nil {
 		log.Println("Could not get list of recursers from DB: ", err)
 	}
 
 	profiles, err := pl.recurse.ActiveRecursers(ctx)
 	if err != nil {
-		log.Println("Encountered error while getting currently-active Recursers: ", err)
-		slog.Error("Aborting end-of-batch processing!")
-		w.WriteHeader(http.StatusInternalServerError)
-		return
+		return fmt.Errorf("get active Recursers: %w", err)
 	}
 
 	var idsOfPeopleAtRc []int64
@@ -297,13 +278,13 @@ func (pl *PairingLogic) endofbatch(w http.ResponseWriter, r *http.Request) {
 			}
 		}
 	}
-}
 
-func (pl *PairingLogic) checkin(w http.ResponseWriter, r *http.Request) {
-	ctx := r.Context()
+	return nil
+}
 
+// Checkin posts a message to Pairing Bot's checkin topic.
+func (pl *PairingLogic) Checkin(ctx context.Context) error {
 	numPairings, err := pl.pdb.GetTotalPairingsDuringLastWeek(ctx)
-
 	if err != nil {
 		log.Println("Unable to get the total number of pairings during the last week: : ", err)
 	}
@@ -320,36 +301,26 @@ func (pl *PairingLogic) checkin(w http.ResponseWriter, r *http.Request) {
 
 	checkinMessage, err := renderCheckin(time.Now(), numPairings, len(recursersList), review.Content)
 	if err != nil {
-		log.Printf("Error when trying to render Pairing Bot checkin: %s", err)
-		return
+		return fmt.Errorf("render checkin: %w", err)
 	}
 
 	if err := pl.zulip.PostToTopic(ctx, "checkins", "Pairing Bot", checkinMessage); err != nil {
-		log.Printf("Error when trying to submit Pairing Bot checkins stream message: %s\n", err)
+		return fmt.Errorf("send checkin: %w", err)
 	}
+
+	return nil
 }
 
-// welcome sends a "Welcome to Pairing Bot" message to introduce the new batch
+// Welcome sends a "Welcome to Pairing Bot" message to introduce the new batch
 // to Pairing Bot.
 //
 // We send this message during the second week of batch. The first week is a
 // bit overwhelming with all of the orientation meetings and messages, and
 // people haven't had time to think too much about their projects.
-func (pl *PairingLogic) welcome(w http.ResponseWriter, r *http.Request) {
-	// Check that the request is originating from within app engine
-	// https://cloud.google.com/appengine/docs/flexible/go/scheduling-jobs-with-cron-yaml#validating_cron_requests
-	if r.Header.Get("X-Appengine-Cron") != "true" {
-		http.NotFound(w, r)
-		return
-	}
-
-	ctx := r.Context()
-
+func (pl *PairingLogic) Welcome(ctx context.Context) error {
 	batches, err := pl.recurse.AllBatches(ctx)
 	if err != nil {
-		log.Printf("Error when fetching batches: %s", err)
-		w.WriteHeader(http.StatusInternalServerError)
-		return
+		return fmt.Errorf("get list of batches: %w", err)
 	}
 
 	// Loop through the batches until we find the first non-mini batch. Mini
@@ -369,12 +340,13 @@ func (pl *PairingLogic) welcome(w http.ResponseWriter, r *http.Request) {
 	if currentBatch.IsSecondWeek(now) {
 		msg, err := renderWelcome(now)
 		if err != nil {
-			log.Printf("Error when trying to send welcome message about Pairing Bot %s\n", err)
-			return
+			return fmt.Errorf("render welcome message: %w", err)
 		}
 
 		if err := pl.zulip.PostToTopic(ctx, pl.welcomeStream, "🍐🤖", msg); err != nil {
-			log.Printf("Error when trying to send welcome message about Pairing Bot %s\n", err)
+			return fmt.Errorf("send welcome message: %w", err)
 		}
 	}
+
+	return nil
 }