Return an empty usage list for a user with no subscription

apitest/fixtures_test.go

@@ -99,10 +99,17 @@ const effectiveDate = "2026-01-15T12:00:00Z"
 // nested user object: the handler overwrites it from the path, and pinning that
 // is part of the point.
 func updateBody(resourceName, unit, operation string, value float64) string {
+	return updateBodyOfType("usages", resourceName, unit, operation, value)
+}
+
+// updateBodyOfType renders an update body for either tracked metric. The two
+// value types take different branches in the handler, and only the usages
+// branch is exercised by the rest of the suite.
+func updateBodyOfType(valueType, resourceName, unit, operation string, value float64) string {
 	return fmt.Sprintf(
-		`{"update": {"value_type": "usages", "value": %v, "effective_date": %q,`+
+		`{"update": {"value_type": %q, "value": %v, "effective_date": %q,`+
 			`"resource_type": {"name": %q, "unit": %q}, "operation": {"name": %q}, "user": {}}}`,
-		value, effectiveDate, resourceName, unit, operation,
+		valueType, value, effectiveDate, resourceName, unit, operation,
 	)
 }
 

apitest/log_fields_test.go

@@ -0,0 +1,142 @@
+package apitest
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+	"sync"
+	"testing"
+)
+
+// The handlers used to build their per-request logger by assigning to the
+// package-level one, so a username stayed attached to every line the process
+// logged afterwards. An operator reading the logs saw requests attributed to
+// whoever happened to come before them, which is how the uuid failure on
+// GET /users/{username}/usages was first misread as a fault in an unrelated
+// user's request. The concurrent case was also a data race on a shared package
+// variable, so this file is worth running under -race.
+
+// A username from one request must not appear in the logs of the next.
+func TestAUsernameDoesNotLeakIntoALaterRequest(t *testing.T) {
+	resetDB(t)
+
+	// A route that logs a username, and would have left it on the package logger.
+	var got response
+	logged := captureLogs(t, func() {
+		got = do(t, http.MethodPut, "/user/alice/updates", updateBody("cpu.hours", "cpu hours", "SET", 1))
+	})
+	if got.status != http.StatusOK {
+		t.Fatalf("the setup request failed: status %d, body %s", got.status, got.body)
+	}
+	if !strings.Contains(logged, "user=alice") {
+		t.Fatalf("the request for alice did not log its own username:\n%s", logged)
+	}
+
+	testCases := []struct {
+		name   string
+		method string
+		path   string
+	}{
+		// A request about a different user entirely.
+		{name: "a request about another user", method: http.MethodGet, path: "/users/bob/usages"},
+		// A request with no username in it at all.
+		{name: "a request about no one", method: http.MethodGet, path: "/addons"},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			logged := captureLogs(t, func() {
+				if got := do(t, testCase.method, testCase.path, ""); got.status != http.StatusOK {
+					t.Fatalf("status = %d, want %d; body: %s", got.status, http.StatusOK, got.body)
+				}
+			})
+
+			if strings.Contains(logged, "alice") {
+				t.Errorf("the earlier request's username leaked into this one's logs:\n%s", logged)
+			}
+		})
+	}
+}
+
+// The request log names the subject of the request, taken from the route, so
+// an operator can find every line for a user. Handlers must not be the source
+// of that field: they only run on some routes, and one that sets it leaves it
+// on requests about everyone else.
+func TestTheRequestLogNamesTheUserTheRequestIsAbout(t *testing.T) {
+	resetDB(t)
+
+	testCases := []struct {
+		name     string
+		path     string
+		wantUser string
+	}{
+		{name: "the username parameter", path: "/users/carol/usages", wantUser: "user=carol"},
+		{name: "the user parameter", path: "/summary/dave", wantUser: "user=dave"},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			logged := captureLogs(t, func() {
+				if got := do(t, http.MethodGet, testCase.path, ""); got.status != http.StatusOK {
+					t.Fatalf("status = %d, want %d; body: %s", got.status, http.StatusOK, got.body)
+				}
+			})
+
+			if !strings.Contains(logged, testCase.wantUser) {
+				t.Errorf("the request log does not carry %q:\n%s", testCase.wantUser, logged)
+			}
+		})
+	}
+
+	// A request that matched no route has no user to name, and must not borrow
+	// one from whatever ran before it.
+	logged := captureLogs(t, func() {
+		do(t, http.MethodGet, "/nonexistent-route", "")
+	})
+	if strings.Contains(logged, "user=") {
+		t.Errorf("a request with no user in it logged one:\n%s", logged)
+	}
+}
+
+// Concurrent requests each log their own username. The field is derived from
+// the request's own context, so this cannot catch a handler reintroducing the
+// package-logger assignment on its own -- its job is to run the shared logger
+// under -race, where that assignment shows up as a write to a shared variable.
+// The per-line assertion is still exact so that dropping the field, or deriving
+// it from anything request-independent, fails here.
+func TestConcurrentRequestsLogTheirOwnUsername(t *testing.T) {
+	resetDB(t)
+
+	const requests = 8
+
+	logged := captureLogs(t, func() {
+		var wg sync.WaitGroup
+		for i := range requests {
+			wg.Go(func() {
+				do(t, http.MethodGet, fmt.Sprintf("/users/user%d/usages", i), "")
+			})
+		}
+		wg.Wait()
+	})
+
+	// Every request line names the user from its own URI, and names one at all.
+	seen := 0
+	for line := range strings.SplitSeq(logged, "\n") {
+		if !strings.Contains(line, "uri=/users/") {
+			continue
+		}
+		for i := range requests {
+			user := fmt.Sprintf("user%d", i)
+			if !strings.Contains(line, "uri=/users/"+user+"/") {
+				continue
+			}
+			seen++
+			if !strings.Contains(line, "user="+user+" ") && !strings.HasSuffix(line, "user="+user) {
+				t.Errorf("a request for %s was not logged against %s:\n%s", user, user, line)
+			}
+		}
+	}
+	if seen != requests {
+		t.Errorf("found %d request log lines, want %d:\n%s", seen, requests, logged)
+	}
+}

apitest/missing_subscription_test.go

@@ -0,0 +1,141 @@
+package apitest
+
+import (
+	"net/http"
+	"testing"
+)
+
+// A user with no subscription is the ordinary case for anyone the service has
+// not seen yet, and the routes below all reach for their active subscription
+// before doing anything else. Reaching for one that isn't there used to hand an
+// empty string to a query comparing it against a uuid column, so PostgreSQL
+// rejected the statement and the caller saw a 500 that said nothing about the
+// cause. resource-usage-api reads GET /users/{username}/usages to decide whether
+// a first data-usage reading exists, and could not tell "none recorded" apart
+// from a genuine fault.
+
+// newUser is a name no fixture creates, so the service has never seen it.
+const newUser = "zzznewuser"
+
+func TestRoutesCopeWithAUserWithoutASubscription(t *testing.T) {
+	testCases := []struct {
+		name   string
+		method string
+		path   string
+		body   string
+	}{
+		{
+			name:   "listing usages",
+			method: http.MethodGet,
+			path:   "/users/" + newUser + "/usages",
+		},
+		{
+			name:   "recording a usage",
+			method: http.MethodPut,
+			path:   "/users/" + newUser + "/usages",
+			body:   `{"resource_name": "cpu.hours", "usage_value": 10, "update_type": "SET"}`,
+		},
+		{
+			name:   "a usage update",
+			method: http.MethodPut,
+			path:   "/user/" + newUser + "/updates",
+			body:   updateBodyOfType("usages", "cpu.hours", "cpu hours", "SET", 10),
+		},
+		{
+			name:   "a quota update",
+			method: http.MethodPut,
+			path:   "/user/" + newUser + "/updates",
+			body:   updateBodyOfType("quotas", "cpu.hours", "cpu hours", "SET", 10),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			resetDB(t)
+
+			got := do(t, testCase.method, testCase.path, testCase.body)
+			if got.status != http.StatusOK {
+				t.Fatalf("status = %d, want %d; body: %s", got.status, http.StatusOK, got.body)
+			}
+			if decoded := mustDecode(t, got); decoded["error"] != nil {
+				t.Errorf("the response carries an error: %v", decoded["error"])
+			}
+		})
+	}
+}
+
+// Listing usages reports that there are none rather than failing, which is the
+// answer resource-usage-api needs to tell "no reading yet" from a server fault.
+func TestUsagesForAUserWithoutASubscriptionAreEmpty(t *testing.T) {
+	resetDB(t)
+
+	got := do(t, http.MethodGet, "/users/"+newUser+"/usages", "")
+	assertGolden(t, "usages_unknown_user", got, http.StatusOK)
+}
+
+// Reading usages does not subscribe anyone. The write paths enrol a user who
+// has no subscription, but a GET that quietly created one would bill a plan to
+// anybody whose name was typed into a URL.
+func TestListingUsagesDoesNotCreateASubscription(t *testing.T) {
+	resetDB(t)
+
+	if got := do(t, http.MethodGet, "/users/"+newUser+"/usages", ""); got.status != http.StatusOK {
+		t.Fatalf("status = %d, want %d; body: %s", got.status, http.StatusOK, got.body)
+	}
+
+	subscriptions := queryInt(t, `SELECT count(*) FROM subscriptions`)
+	if subscriptions != 0 {
+		t.Errorf("the request created %d subscription(s), want 0", subscriptions)
+	}
+}
+
+// The write paths do enrol the user, so the usage they just recorded has a
+// subscription to hang off and a later read can find it.
+func TestRecordingUsageSubscribesAUserWithoutASubscription(t *testing.T) {
+	testCases := []struct {
+		name string
+		path string
+		body string
+	}{
+		{
+			name: "through the usages route",
+			path: "/users/" + newUser + "/usages",
+			body: `{"resource_name": "cpu.hours", "usage_value": 10, "update_type": "SET"}`,
+		},
+		{
+			name: "through the updates route",
+			path: "/user/" + newUser + "/updates",
+			body: updateBodyOfType("usages", "cpu.hours", "cpu hours", "SET", 10),
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			resetDB(t)
+
+			if got := do(t, http.MethodPut, testCase.path, testCase.body); got.status != http.StatusOK {
+				t.Fatalf("status = %d, want %d; body: %s", got.status, http.StatusOK, got.body)
+			}
+
+			plan := queryString(t, `
+				SELECT p.name
+				  FROM subscriptions s
+				  JOIN users u ON s.user_id = u.id
+				  JOIN plans p ON s.plan_id = p.id
+				 WHERE u.username = $1`, newUser)
+			if plan != "Basic" {
+				t.Errorf("the user was subscribed to the %s plan, want Basic", plan)
+			}
+
+			usage := queryFloat(t, `
+				SELECT us.usage
+				  FROM usages us
+				  JOIN subscriptions s ON us.subscription_id = s.id
+				  JOIN users u ON s.user_id = u.id
+				 WHERE u.username = $1`, newUser)
+			if usage != 10 {
+				t.Errorf("the recorded usage is %v, want 10", usage)
+			}
+		})
+	}
+}