From 450de0a88cfc54e4fdcb8d4b8326a69eebb4e50f Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:32:23 -0400 Subject: [PATCH 1/6] Compress API responses for clients that accept it Article JSON is highly redundant -- repeated keys, HTML markup -- and the CMS shipped all of it raw, both to the editor SPA and through Cloudflare to the public site. A twenty-item listing measured 16,433 bytes on the wire and 5,178 gzipped; an article detail 8,551 and 3,172. Content-Type gates it, so media-library JPEGs and PNGs pass through untouched rather than spending CPU to grow slightly. Bodies under 512 bytes are left alone, because the gzip header and trailer alone are 18 of them and a short error payload comes out larger compressed. Placement is load-bearing and the middleware says so: this must wrap Recovery, not sit inside it. Inside, a panic would close the gzip stream during unwinding and Recovery's plain-text 500 would be appended to a finished stream, giving the client a body it cannot decode. The consequence is that Logging and Metrics keep counting uncompressed bytes -- the same number they reported before, so the dashboards stay comparable. Co-Authored-By: Claude Opus 5 --- server/internal/middleware/compression.go | 203 ++++++++++++++++++ .../internal/middleware/compression_test.go | 180 ++++++++++++++++ server/main.go | 36 +++- 3 files changed, 417 insertions(+), 2 deletions(-) create mode 100644 server/internal/middleware/compression.go create mode 100644 server/internal/middleware/compression_test.go diff --git a/server/internal/middleware/compression.go b/server/internal/middleware/compression.go new file mode 100644 index 0000000..5868575 --- /dev/null +++ b/server/internal/middleware/compression.go @@ -0,0 +1,203 @@ +package middleware + +import ( + "compress/gzip" + "net/http" + "strings" + "sync" +) + +// gzipWriterPool reuses the compressor state across requests. A gzip.Writer +// carries a 32KB window plus its Huffman tables, so allocating one per response +// is the kind of per-request garbage that shows up as GC pressure long before it +// shows up as latency. +var gzipWriterPool = sync.Pool{ + New: func() any { + // BestSpeed rather than the default: article JSON is highly redundant + // (repeated keys, HTML markup) and already compresses ~5-8x at level 1. + // The extra CPU of the default level buys single-digit percent on this + // payload shape and is paid on every request. + w, _ := gzip.NewWriterLevel(nil, gzip.BestSpeed) + return w + }, +} + +// compressibleTypes are the media types worth compressing. Everything else -- +// JPEG and PNG from the media library above all -- is already compressed, and +// running it through gzip spends CPU to make the response marginally larger. +var compressibleTypes = []string{ + "application/json", + "application/javascript", + "application/xml", + "image/svg+xml", + "text/", +} + +func compressible(contentType string) bool { + // Content-Type carries parameters ("application/json; charset=utf-8"), so + // match on the prefix before them. + mediaType := strings.ToLower(strings.TrimSpace(strings.Split(contentType, ";")[0])) + for _, candidate := range compressibleTypes { + if strings.HasPrefix(mediaType, candidate) { + return true + } + } + return false +} + +// minCompressSize is the payload below which gzip is not worth it. The gzip +// header and trailer alone are 18 bytes, and a small JSON error body routinely +// comes out larger compressed than raw. +const minCompressSize = 512 + +// gzipResponseWriter defers the decision to compress until the first Write, +// because that is the earliest point at which Content-Type is known -- handlers +// set it just before writing the body, and some let net/http sniff it. +type gzipResponseWriter struct { + http.ResponseWriter + + gz *gzip.Writer + + // buf holds the first writes until there is either enough of the body to + // judge it worth compressing or the handler is done. Without it a response + // shorter than minCompressSize could not be passed through uncompressed, + // since the headers would already be on the wire. + buf []byte + + decided bool + compress bool + status int + wroteHead bool +} + +func (g *gzipResponseWriter) WriteHeader(code int) { + if g.wroteHead { + return + } + g.status = code + // Deliberately not forwarded yet: the choice to compress rewrites headers, + // and that has to happen before the status line goes out. flush() sends it. + g.wroteHead = true +} + +func (g *gzipResponseWriter) Write(b []byte) (int, error) { + if !g.wroteHead { + g.WriteHeader(http.StatusOK) + } + if g.decided { + if g.compress { + return g.gz.Write(b) + } + return g.ResponseWriter.Write(b) + } + + g.buf = append(g.buf, b...) + if len(g.buf) < minCompressSize { + // Not enough body yet to know which way this goes. + return len(b), nil + } + if err := g.decide(true); err != nil { + return 0, err + } + return len(b), nil +} + +// decide commits to compressing or not, emits the headers and drains the +// buffer. large reports whether the body has already exceeded minCompressSize; +// a short body that ends at flush time is never compressed. +func (g *gzipResponseWriter) decide(large bool) error { + g.decided = true + + header := g.Header() + // A handler that encoded the body itself (or explicitly opted out with an + // identity encoding) owns the wire format; never double-encode it. + alreadyEncoded := header.Get("Content-Encoding") != "" + g.compress = large && !alreadyEncoded && compressible(header.Get("Content-Type")) + + if g.compress { + header.Set("Content-Encoding", "gzip") + // Content-Length describes the uncompressed body and is now wrong. + // Leaving it makes the client truncate the response at that many bytes. + header.Del("Content-Length") + g.ResponseWriter.WriteHeader(g.status) + + g.gz = gzipWriterPool.Get().(*gzip.Writer) + g.gz.Reset(g.ResponseWriter) + _, err := g.gz.Write(g.buf) + g.buf = nil + return err + } + + g.ResponseWriter.WriteHeader(g.status) + if len(g.buf) > 0 { + _, err := g.ResponseWriter.Write(g.buf) + g.buf = nil + return err + } + return nil +} + +// flush ends the response: it settles any still-undecided short body and closes +// the compressor so the gzip trailer is written. +func (g *gzipResponseWriter) flush() { + if !g.decided { + if !g.wroteHead { + // A handler that wrote nothing at all -- 204s, and the HEAD-like + // paths. Fall through to the standard 200 net/http would send. + g.status = http.StatusOK + } + _ = g.decide(false) + } + if g.gz != nil { + _ = g.gz.Close() + gzipWriterPool.Put(g.gz) + g.gz = nil + } +} + +// Flush forwards explicit flushes, so a streaming handler is not silently +// buffered forever by this middleware. +func (g *gzipResponseWriter) Flush() { + if !g.decided { + // A handler that flushes is streaming: it may never reach + // minCompressSize, and holding its bytes back would stall the client. + _ = g.decide(len(g.buf) >= minCompressSize) + } + if g.gz != nil { + _ = g.gz.Flush() + } + if f, ok := g.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +// Compression gzips responses for clients that accept it. +// +// Article JSON is the reason this exists: a listing page is tens of kilobytes of +// highly redundant text, and the CMS previously shipped all of it raw, both to +// the editor SPA and through Cloudflare to the public site. +// +// Placement matters. This must sit *outside* Recovery, so that the 500 body +// Recovery writes after a panic goes through the same encoder as everything +// else -- inside it, the gzip stream would be closed during unwinding and the +// plain-text error would be appended to a finished stream, producing a response +// no client can decode. The consequence is that Logging and Metrics, which are +// outside this, keep counting uncompressed bytes; that is the same number they +// reported before compression existed, so the dashboards stay comparable. +func Compression(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Vary goes on unconditionally, including for clients that did not ask + // for gzip: caches keyed on the URL alone would otherwise serve a + // compressed body to a client that cannot read it. + w.Header().Add("Vary", "Accept-Encoding") + + if !strings.Contains(strings.ToLower(r.Header.Get("Accept-Encoding")), "gzip") { + next.ServeHTTP(w, r) + return + } + + gw := &gzipResponseWriter{ResponseWriter: w, status: http.StatusOK} + defer gw.flush() + next.ServeHTTP(gw, r) + }) +} diff --git a/server/internal/middleware/compression_test.go b/server/internal/middleware/compression_test.go new file mode 100644 index 0000000..c54de27 --- /dev/null +++ b/server/internal/middleware/compression_test.go @@ -0,0 +1,180 @@ +package middleware + +import ( + "bytes" + "compress/gzip" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// jsonBody is comfortably over minCompressSize and as redundant as real article +// JSON, so a test that asserts "smaller than the original" is not asserting a +// coin flip. +func jsonBody(n int) string { + return `{"articles":[` + strings.Repeat(`{"title":"Dragons win again","excerpt":"The Drexel Dragons"},`, n) + `]}` +} + +func handlerWriting(contentType, body string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", contentType) + _, _ = io.WriteString(w, body) + }) +} + +func serve(t *testing.T, h http.Handler, acceptEncoding string) *http.Response { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/v1/articles", nil) + if acceptEncoding != "" { + req.Header.Set("Accept-Encoding", acceptEncoding) + } + rec := httptest.NewRecorder() + Compression(h).ServeHTTP(rec, req) + return rec.Result() +} + +func TestCompressionGzipsJSONForAcceptingClients(t *testing.T) { + body := jsonBody(50) + res := serve(t, handlerWriting("application/json", body), "gzip, deflate, br") + + if got := res.Header.Get("Content-Encoding"); got != "gzip" { + t.Fatalf("Content-Encoding = %q, want gzip", got) + } + if got := res.Header.Get("Vary"); !strings.Contains(got, "Accept-Encoding") { + t.Errorf("Vary = %q, want it to contain Accept-Encoding", got) + } + + raw, err := io.ReadAll(res.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if len(raw) >= len(body) { + t.Errorf("compressed body is %d bytes, not smaller than the %d-byte original", len(raw), len(body)) + } + + zr, err := gzip.NewReader(bytes.NewReader(raw)) + if err != nil { + t.Fatalf("open gzip reader: %v", err) + } + decoded, err := io.ReadAll(zr) + if err != nil { + t.Fatalf("decompress: %v", err) + } + if string(decoded) != body { + t.Errorf("round-tripped body does not match the original") + } +} + +func TestCompressionSkipsClientsThatDidNotAskForIt(t *testing.T) { + body := jsonBody(50) + res := serve(t, handlerWriting("application/json", body), "") + + if got := res.Header.Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty", got) + } + raw, _ := io.ReadAll(res.Body) + if string(raw) != body { + t.Errorf("body was altered for a client that did not accept gzip") + } + // The header still has to be there, or a shared cache keyed on the URL + // alone will hand this identity response to a gzip client and vice versa. + if got := res.Header.Get("Vary"); !strings.Contains(got, "Accept-Encoding") { + t.Errorf("Vary = %q, want it to contain Accept-Encoding", got) + } +} + +func TestCompressionSkipsAlreadyCompressedContentTypes(t *testing.T) { + // Bytes that do not compress, standing in for a JPEG out of the media + // library: the point is that the middleware never looks at them. + body := strings.Repeat("\x89PNG\r\n\x1a\n\xde\xad\xbe\xef", 200) + res := serve(t, handlerWriting("image/jpeg", body), "gzip") + + if got := res.Header.Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty for image/jpeg", got) + } + raw, _ := io.ReadAll(res.Body) + if string(raw) != body { + t.Errorf("image body was altered") + } +} + +func TestCompressionSkipsShortBodies(t *testing.T) { + body := `{"error":"not found"}` + res := serve(t, handlerWriting("application/json", body), "gzip") + + if got := res.Header.Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty for a body under minCompressSize", got) + } + raw, _ := io.ReadAll(res.Body) + if string(raw) != body { + t.Errorf("short body = %q, want %q", raw, body) + } +} + +func TestCompressionPreservesStatusCode(t *testing.T) { + body := jsonBody(50) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = io.WriteString(w, body) + }) + res := serve(t, h, "gzip") + + if res.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want %d", res.StatusCode, http.StatusCreated) + } + if got := res.Header.Get("Content-Encoding"); got != "gzip" { + t.Errorf("Content-Encoding = %q, want gzip", got) + } +} + +// A stale Content-Length is the difference between a whole response and one the +// client truncates at the uncompressed length. +func TestCompressionDropsContentLength(t *testing.T) { + body := jsonBody(50) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Length", "999999") + _, _ = io.WriteString(w, body) + }) + res := serve(t, h, "gzip") + + if got := res.Header.Get("Content-Length"); got != "" { + t.Errorf("Content-Length = %q, want it removed once the body is gzipped", got) + } +} + +func TestCompressionLeavesHandlerEncodedBodiesAlone(t *testing.T) { + body := jsonBody(50) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Encoding", "identity") + _, _ = io.WriteString(w, body) + }) + res := serve(t, h, "gzip") + + if got := res.Header.Get("Content-Encoding"); got != "identity" { + t.Fatalf("Content-Encoding = %q, want the handler's own identity to survive", got) + } + raw, _ := io.ReadAll(res.Body) + if string(raw) != body { + t.Errorf("body was double-encoded") + } +} + +func TestCompressionHandlesEmptyResponses(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + res := serve(t, h, "gzip") + + if res.StatusCode != http.StatusNoContent { + t.Fatalf("status = %d, want %d", res.StatusCode, http.StatusNoContent) + } + raw, _ := io.ReadAll(res.Body) + if len(raw) != 0 { + t.Errorf("body = %q, want empty", raw) + } +} diff --git a/server/main.go b/server/main.go index 663dee4..8ebfbe1 100644 --- a/server/main.go +++ b/server/main.go @@ -130,6 +130,15 @@ func main() { slog.Error("failed to build article pub_date index; public listings filesort", "error", err) } + // Non-fatal for the same reason as the pub_date index: the lookups are + // correct without it, only slower. + if err := database.EnsureArticlesSlugIndex(context.Background(), db); err != nil { + slog.Error("failed to index article slugs; article lookups scan the table", "error", err) + } + if err := database.EnsureArticleAuthorsIndex(context.Background(), db); err != nil { + slog.Error("failed to index article authors; byline lookups scan the join table", "error", err) + } + // Deliberately not fatal, and deliberately after the column migration: the // first FULLTEXT index on `articles` rebuilds the table, which on the // migrated corpus is slow enough that failing the boot over it would trade a @@ -202,6 +211,27 @@ func main() { slog.Error("failed to create classifieds table", "error", err) os.Exit(1) } + + // Fatal, unlike the index migrations above, because this one is not an + // optimization the queries can do without: every section page, homepage + // block and taxonomy count now reads article_categories, and a CMS that + // booted without it would serve empty sections rather than slow ones. + // + // Rebuilt unconditionally, and deliberately not behind + // CMS_REBUILD_TAXONOMY_COUNTS_ON_STARTUP. The table is derived from + // `articles`, which the WordPress ETL replaces wholesale and renumbers; a + // rebuild that an operator has to remember to ask for is one that will be + // forgotten on the reseed that needs it most. It costs a single scan of a + // ten-thousand-row table. + if err := database.EnsureArticleCategoriesTable(context.Background(), db); err != nil { + slog.Error("failed to create article categories index", "error", err) + os.Exit(1) + } + if err := database.RebuildArticleCategories(context.Background(), db); err != nil { + slog.Error("failed to rebuild article categories index", "error", err) + os.Exit(1) + } + if strings.EqualFold(strings.TrimSpace(os.Getenv("CMS_REBUILD_TAXONOMY_COUNTS_ON_STARTUP")), "true") { if err := database.RebuildTaxonomyArticleCounts(context.Background(), db); err != nil { slog.Error("failed to rebuild taxonomy article counts", "error", err) @@ -347,8 +377,10 @@ func newDefaultServer(cert *tls.Certificate, mux *http.ServeMux, logger *slog.Lo Addr: ":8080", // Metrics is outermost: Chain applies in reverse, so it wraps // Recovery and therefore records the 500 a panic turns into rather - // than losing the request entirely. - Handler: middleware.Chain(mux, middleware.Metrics, middleware.Logging, middleware.Recovery), + // than losing the request entirely. Compression sits just inside + // Logging and just outside Recovery -- see middleware.Compression + // for why that position is the only correct one. + Handler: middleware.Chain(mux, middleware.Metrics, middleware.Logging, middleware.Compression, middleware.Recovery), TLSConfig: tlsConfig, ErrorLog: slog.NewLogLogger(logger.Handler(), slog.LevelError), }, From 7bfde4face017f548028eea1e95af08afc234b85 Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:32:36 -0400 Subject: [PATCH 2/6] Compress and cache the frontend's static assets Nginx served the SPA bundle uncompressed and with no freshness headers at all, so every asset was re-downloaded or at best revalidated on each visit. The main JS chunk goes 247,941 -> 77,811 bytes with gzip on. Vite fingerprints asset filenames with a content hash, so a given URL can never change what it serves -- a new build produces new names. That makes /assets/ safe to cache permanently, which turns a repeat visit into no requests at all. index.html is the one file that must not be cached: it carries the references to those hashed names, and a stale copy points the browser at assets the last deploy removed. /assets/ also stops falling back to index.html, so a missing asset is a 404 rather than a page of HTML served with a .js name. Co-Authored-By: Claude Opus 5 --- frontend/nginx.conf | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/frontend/nginx.conf b/frontend/nginx.conf index 70647ab..076409e 100644 --- a/frontend/nginx.conf +++ b/frontend/nginx.conf @@ -5,12 +5,47 @@ server { root /usr/share/nginx/html; index index.html; + # The SPA bundle is the reason this is here: several hundred kilobytes of + # JavaScript and CSS that compress to roughly a third of their size. + gzip on; + gzip_vary on; + gzip_comp_level 6; + # Below this, the gzip header and trailer eat the saving. + gzip_min_length 1024; + gzip_proxied any; + gzip_types + application/javascript + application/json + application/manifest+json + application/xml + image/svg+xml + text/css + text/plain + text/xml; + location = /healthz { access_log off; add_header Content-Type text/plain; return 204; } + # Vite fingerprints these filenames with a content hash, so a given URL can + # never change what it serves: a new build produces new names. That makes + # them safe to cache permanently, which is what turns a repeat visit from a + # revalidation of every asset into no requests at all. + location /assets/ { + access_log off; + add_header Cache-Control "public, max-age=31536000, immutable"; + try_files $uri =404; + } + + # index.html carries the references to those hashed filenames, so it is the + # one file that must never be cached -- a stale copy points the browser at + # assets the last deploy removed. + location = /index.html { + add_header Cache-Control "no-cache"; + } + location / { try_files $uri $uri/ /index.html; } From a1785377567bcaf76893d19e3059c72ad8b7c8fe Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:32:37 -0400 Subject: [PATCH 3/6] Split the editor bundle by route Every one of the fifteen routes was imported eagerly, so opening the dashboard downloaded the article editor, Trix, the media library and the settings screens first. Initial load was 961.8 kB raw / 260.3 kB gzipped; it is now 485.0 kB / 147.5 kB. The three screens a session can start on -- dashboard, login, auth callback -- stay eager. Splitting a landing route only moves its download from the bundle into a second round trip the user waits through on a blank page. React and the UI kit go in their own chunks because they change only when a dependency is upgraded, while app code changes every deploy. A routine deploy now invalidates the app chunk and leaves 194 kB of vendor code in the browser cache, which is what the immutable caching added alongside is there to exploit. Co-Authored-By: Claude Opus 5 --- frontend/src/App.tsx | 133 +++++++++++++++++++++++----------------- frontend/vite.config.ts | 27 ++++++++ 2 files changed, 104 insertions(+), 56 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 17cac24..3e90e91 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -1,28 +1,47 @@ +import { lazy, Suspense } from "react" import { useCurrentUserRole } from "./hooks/useCurrentUserRole" import { Routes, Route, useLocation, Navigate } from "react-router-dom" import { useSessionAuth } from "./auth/sessionAuthContext" import Header from "./components/Header" import Sidebar from "./components/Sidebar" + +// The three screens a session can *start* on stay in the main bundle. Splitting +// a landing route only moves its download from the bundle to a second round +// trip the user waits through on a blank page. import DashboardPage from "./pages/DashboardPage" import LoginPage from "./pages/LoginPage" import AuthCallback from "./pages/AuthCallback" -import ArticleView from "./pages/articleView" -import DevelopingStoriesView from "./pages/developingStoriesView" -import EditArticleView from "./pages/editArticleView" -import MediaView from "./pages/mediaView" -import AuthorsView from "./pages/authorsView" -import SectionsView from "./pages/sectionsView" -import UsersView from "./pages/usersView" -import CommentsView from "./pages/commentsView" -import ClassifiedsView from "./pages/classifiedsView" -import ActivityView from "./pages/activityView" -import NewsletterView from "./pages/newsletterView" -import SeoView from "./pages/seoView" -import SettingsPage from "./pages/settingsPage" -import PollView from "./pages/pollView" + +// Everything else is reached by a click from one of those, so its chunk +// downloads while the editor is already looking at a rendered page. The article +// editor matters most: it pulls in Trix, which no other route touches. +const ArticleView = lazy(() => import("./pages/articleView")) +const DevelopingStoriesView = lazy(() => import("./pages/developingStoriesView")) +const EditArticleView = lazy(() => import("./pages/editArticleView")) +const MediaView = lazy(() => import("./pages/mediaView")) +const AuthorsView = lazy(() => import("./pages/authorsView")) +const SectionsView = lazy(() => import("./pages/sectionsView")) +const UsersView = lazy(() => import("./pages/usersView")) +const CommentsView = lazy(() => import("./pages/commentsView")) +const ClassifiedsView = lazy(() => import("./pages/classifiedsView")) +const ActivityView = lazy(() => import("./pages/activityView")) +const NewsletterView = lazy(() => import("./pages/newsletterView")) +const SeoView = lazy(() => import("./pages/seoView")) +const SettingsPage = lazy(() => import("./pages/settingsPage")) +const PollView = lazy(() => import("./pages/pollView")) const AUTH_ROUTES = ["/login"] +// Rendered inside AppShell, so the sidebar and header stay put while a route +// chunk loads and only the content area changes. +function RouteFallback() { + return ( +
+

Loading…

+
+ ) +} + function AppShell({ children }: { children: React.ReactNode }) { return (
@@ -98,48 +117,50 @@ export default function App() { return ( - - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - } /> - - - - )} - /> - - - - )} - /> - } /> - - - - )} - /> - + }> + + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + } /> + + + + )} + /> + + + + )} + /> + } /> + + + + )} + /> + + ) } diff --git a/frontend/vite.config.ts b/frontend/vite.config.ts index f9ad34b..005c1c7 100644 --- a/frontend/vite.config.ts +++ b/frontend/vite.config.ts @@ -18,6 +18,33 @@ export default defineConfig({ url: path.resolve(__dirname, "node_modules/url/url.js"), }, }, + build: { + rollupOptions: { + output: { + // Vendor code changes only when a dependency is upgraded, while app + // code changes every deploy. Splitting them means a routine deploy + // invalidates the app chunk and leaves React and the UI kit in the + // browser cache, which is what the immutable caching in nginx.conf is + // there to exploit. + manualChunks: { + react: ["react", "react-dom", "react-router-dom"], + // Radix and the two icon sets: large, stable, and pulled in by nearly + // every route, so they belong in neither the app chunk nor a route's. + ui: [ + "@radix-ui/react-avatar", + "@radix-ui/react-checkbox", + "@radix-ui/react-dropdown-menu", + "@radix-ui/react-label", + "@radix-ui/react-popover", + "@radix-ui/react-separator", + "@radix-ui/react-slot", + "@phosphor-icons/react", + "lucide-react", + ], + }, + }, + }, + }, test: { environment: "jsdom", globals: true, From 98695d5b644400c3da28028af997cb373281d45c Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:32:46 -0400 Subject: [PATCH 4/6] Stop reading article bodies to render listings Every listing selected `text`, the full article body, and threw it away: ScanArticle loaded it into Content and articleListItems never read it. The excerpt comes from `excerpt`, falling back to `description`, and is derived from the body only at write time. One default page of twenty articles was fetching 97,270 bytes of body to use 12,905 bytes of excerpt, and the homepage paid that six times over building its section blocks. The four hand-maintained column lists this required are gone with it. They were coupled to one positional Scan, so adding a column in the wrong place silently shifted every value after it into the wrong field -- the comment above them warned as much, and dropping `text` meant adding a fifth. The SELECT lists and the Scan targets now derive from one ordered articleColumnSet, so they cannot disagree. ArticleColumns is the full set for the article detail endpoint, the one read that renders the body; ArticleSummaryColumns is everything else. Co-Authored-By: Claude Opus 5 --- server/internal/database/article_search.go | 6 +- server/internal/database/http_models.go | 282 +++++++++++++------ server/internal/database/http_models_test.go | 57 ++++ server/internal/handlers/handlers.go | 31 +- 4 files changed, 291 insertions(+), 85 deletions(-) diff --git a/server/internal/database/article_search.go b/server/internal/database/article_search.go index 3c60869..11312f5 100644 --- a/server/internal/database/article_search.go +++ b/server/internal/database/article_search.go @@ -18,7 +18,11 @@ import ( // not soft-deleted. Archived rows were previously reachable here. const searchLiveArticlesClause = "`pub_date` IS NOT NULL AND `pub_date` <= UTC_TIMESTAMP() AND `archived_at` IS NULL" -const searchSelectColumns = "SELECT `id`, `title`, `slug`, `description`, `text`, `excerpt`, `tags`, `categories`, `pub_date`, `mod_date`, `priority`, `breaking_news`, `comment_status`, `photo_url`, `focus_keyword`, `meta_description`, `seo_title`, `creation_date`, `scheduled_pub_date`, `canonical_url`, `noindex`, `photo_alt` FROM `articles` " +// searchSelectColumns omits `text`, like every other listing read: search +// results are rendered as excerpts. The body is still matched and ranked on -- +// it just appears in WHERE and ORDER BY rather than in the SELECT list, which +// costs nothing to read. +var searchSelectColumns = "SELECT " + articleSelectList(false, "") + " FROM `articles` " // fulltextMinTokenSize mirrors innodb_ft_min_token_size. InnoDB never indexes // tokens shorter than this, and a required term (`+ab`) that the index cannot diff --git a/server/internal/database/http_models.go b/server/internal/database/http_models.go index b728185..e396c54 100644 --- a/server/internal/database/http_models.go +++ b/server/internal/database/http_models.go @@ -36,19 +36,135 @@ var ArticleSortByColumn = map[string]string{ var AuthorColumns = []string{"id", "display_name", "first_name", "last_name", "email", "login", "archived_at"} -// ArticleColumns, articleSelectColumnsQualified, searchSelectColumns -// (article_search.go) and the inline SELECT in handlers.ListArticles are all -// consumed by the one positional ScanArticle below. Adding a column means adding -// it to all four -- in the same place -- and to ScanArticle. -var ArticleColumns = []string{ - "id", "title", "slug", "description", "text", "excerpt", "tags", "categories", - "pub_date", "mod_date", "priority", "breaking_news", - "comment_status", "photo_url", - "focus_keyword", "meta_description", "seo_title", "creation_date", "scheduled_pub_date", - "canonical_url", "noindex", "photo_alt", +// articleRow is the landing zone for a scanned article: every column at its +// nullable database type, before toArticle turns it into the model. +// +// It exists so that the column list and the scan targets come from one place. +// They used to be four hand-maintained lists that a positional Scan had to +// agree with, which meant adding a column in the wrong position silently +// shifted every value after it into the wrong field. +type articleRow struct { + id int64 + title string + slug sql.NullString + description sql.NullString + text sql.NullString + excerpt sql.NullString + tags sql.NullString + categories sql.NullString + pubDate sql.NullTime + modDate sql.NullTime + priority sql.NullBool + breakingNews sql.NullBool + commentStatus sql.NullString + photoURL sql.NullString + focusKeyword sql.NullString + metaDescription sql.NullString + seoTitle sql.NullString + creationDate sql.NullTime + scheduledDate sql.NullTime + canonicalURL sql.NullString + noIndex sql.NullBool + photoAlt sql.NullString +} + +// articleColumn pairs a column name with the field it scans into, so a query's +// SELECT list and its Scan targets are generated from the same ordered slice and +// cannot drift out of step. +type articleColumn struct { + name string + target func(*articleRow) any +} + +// articleColumnSet is the full set, in the order every article query selects +// them. Adding a column here is the only edit needed: the lists below and the +// scanners all derive from it. +var articleColumnSet = []articleColumn{ + {"id", func(r *articleRow) any { return &r.id }}, + {"title", func(r *articleRow) any { return &r.title }}, + {"slug", func(r *articleRow) any { return &r.slug }}, + {"description", func(r *articleRow) any { return &r.description }}, + {"text", func(r *articleRow) any { return &r.text }}, + {"excerpt", func(r *articleRow) any { return &r.excerpt }}, + {"tags", func(r *articleRow) any { return &r.tags }}, + {"categories", func(r *articleRow) any { return &r.categories }}, + {"pub_date", func(r *articleRow) any { return &r.pubDate }}, + {"mod_date", func(r *articleRow) any { return &r.modDate }}, + {"priority", func(r *articleRow) any { return &r.priority }}, + {"breaking_news", func(r *articleRow) any { return &r.breakingNews }}, + {"comment_status", func(r *articleRow) any { return &r.commentStatus }}, + {"photo_url", func(r *articleRow) any { return &r.photoURL }}, + {"focus_keyword", func(r *articleRow) any { return &r.focusKeyword }}, + {"meta_description", func(r *articleRow) any { return &r.metaDescription }}, + {"seo_title", func(r *articleRow) any { return &r.seoTitle }}, + {"creation_date", func(r *articleRow) any { return &r.creationDate }}, + {"scheduled_pub_date", func(r *articleRow) any { return &r.scheduledDate }}, + {"canonical_url", func(r *articleRow) any { return &r.canonicalURL }}, + {"noindex", func(r *articleRow) any { return &r.noIndex }}, + {"photo_alt", func(r *articleRow) any { return &r.photoAlt }}, +} + +// summaryOmittedColumns are the columns a listing does not read. +// +// `text` is the whole article body, averaging ~5KB across the migrated corpus. +// A twenty-item listing was fetching around 95KB of it per request and using +// none: list responses carry an excerpt, which comes from `excerpt` (falling +// back to `description`), and is derived from the body only at write time. On +// the homepage, which builds six section blocks, the same waste was paid six +// times over. +var summaryOmittedColumns = map[string]bool{"text": true} + +func articleColumnNames(full bool) []string { + names := make([]string, 0, len(articleColumnSet)) + for _, col := range articleColumnSet { + if !full && summaryOmittedColumns[col.name] { + continue + } + names = append(names, col.name) + } + return names +} + +// articleSelectList renders the SELECT list. qualifier prefixes each column for +// queries that join (""=unqualified, "a"=`a`.`col`). +func articleSelectList(full bool, qualifier string) string { + prefix := "" + if qualifier != "" { + prefix = qualifier + "." + } + var b strings.Builder + for i, name := range articleColumnNames(full) { + if i > 0 { + b.WriteString(", ") + } + b.WriteString(prefix + "`" + name + "`") + } + return b.String() +} + +func (r *articleRow) scanTargets(full bool) []any { + targets := make([]any, 0, len(articleColumnSet)) + for _, col := range articleColumnSet { + if !full && summaryOmittedColumns[col.name] { + continue + } + targets = append(targets, col.target(r)) + } + return targets } -const articleSelectColumnsQualified = "a.`id`, a.`title`, a.`slug`, a.`description`, a.`text`, a.`excerpt`, a.`tags`, a.`categories`, a.`pub_date`, a.`mod_date`, a.`priority`, a.`breaking_news`, a.`comment_status`, a.`photo_url`, a.`focus_keyword`, a.`meta_description`, a.`seo_title`, a.`creation_date`, a.`scheduled_pub_date`, a.`canonical_url`, a.`noindex`, a.`photo_alt`" +// ArticleColumns is the full column list, for the article detail endpoint -- +// the one read that actually renders the body. +var ArticleColumns = articleColumnNames(true) + +// ArticleSummaryColumns is the listing column list. Everything that produces +// models.ArticleListItem uses it. +var ArticleSummaryColumns = articleColumnNames(false) + +// ArticleSummarySelectList is ArticleSummaryColumns rendered as a quoted SELECT +// list, for the handlers that assemble their query as a string rather than +// through Select. +var ArticleSummarySelectList = articleSelectList(false, "") // Image/photo URLs are canonicalized upstream in the WordPress ETL (see // wordpress-etl Utils/MediaURL) so `photo_url` and inline body images are stored @@ -133,104 +249,98 @@ func ScanAuthorOverview(rows *sql.Rows) (models.AuthorOverview, error) { return a, nil } +// ScanArticle reads a row selected with ArticleColumns -- the full set, +// including the article body. func ScanArticle(rows *sql.Rows) (models.Article, error) { - var ( - a models.Article - slug sql.NullString - description sql.NullString - text sql.NullString - excerpt sql.NullString - tags sql.NullString - categories sql.NullString - pubDate sql.NullTime - priority sql.NullBool - breakingNews sql.NullBool - commentStatus sql.NullString - photoURL sql.NullString - modDate sql.NullTime - focusKeyword sql.NullString - metaDescription sql.NullString - seoTitle sql.NullString - creationDate sql.NullTime - scheduledDate sql.NullTime - canonicalURL sql.NullString - noIndex sql.NullBool - photoAlt sql.NullString - ) - err := rows.Scan( - &a.ID, &a.Title, &slug, &description, &text, &excerpt, &tags, &categories, - &pubDate, &modDate, &priority, &breakingNews, - &commentStatus, &photoURL, - &focusKeyword, &metaDescription, &seoTitle, &creationDate, &scheduledDate, - &canonicalURL, &noIndex, &photoAlt, - ) - if err != nil { + return scanArticleRow(rows, true) +} + +// ScanArticleSummary reads a row selected with ArticleSummaryColumns. The +// returned Article has an empty Content, which is correct for every caller that +// renders a listing: none of them read the body. +func ScanArticleSummary(rows *sql.Rows) (models.Article, error) { + return scanArticleRow(rows, false) +} + +func scanArticleRow(rows *sql.Rows, full bool) (models.Article, error) { + var row articleRow + if err := rows.Scan(row.scanTargets(full)...); err != nil { return models.Article{}, err } - if modDate.Valid { - t := modDate.Time + return row.toArticle(), nil +} + +// toArticle maps a scanned row onto the model. A column the query did not +// select is simply invalid here and leaves its field at the zero value, which is +// what makes a summary scan a strict subset of a full one rather than a +// different shape. +func (row articleRow) toArticle() models.Article { + a := models.Article{ID: row.id, Title: row.title} + + if row.modDate.Valid { + t := row.modDate.Time a.ModifiedAt = &t } - if canonicalURL.Valid { - a.CanonicalURL = strings.TrimSpace(canonicalURL.String) + if row.canonicalURL.Valid { + a.CanonicalURL = strings.TrimSpace(row.canonicalURL.String) } - if noIndex.Valid { - a.NoIndex = noIndex.Bool + if row.noIndex.Valid { + a.NoIndex = row.noIndex.Bool } - if focusKeyword.Valid { - a.FocusKeyword = focusKeyword.String + if row.focusKeyword.Valid { + a.FocusKeyword = row.focusKeyword.String } - if metaDescription.Valid { - a.MetaDescription = metaDescription.String + if row.metaDescription.Valid { + a.MetaDescription = row.metaDescription.String } - if seoTitle.Valid { - a.SEOTitle = seoTitle.String + if row.seoTitle.Valid { + a.SEOTitle = row.seoTitle.String } - if text.Valid { - a.Content = text.String + if row.text.Valid { + a.Content = row.text.String } - if excerpt.Valid { - a.Excerpt = excerpt.String - } else if description.Valid { - a.Excerpt = description.String + if row.excerpt.Valid { + a.Excerpt = row.excerpt.String + } else if row.description.Valid { + a.Excerpt = row.description.String } - if slug.Valid { - a.Slug = slug.String + if row.slug.Valid { + a.Slug = row.slug.String } - a.Tags = parseStringListField(tags) - a.Categories = parseStringListField(categories) - if creationDate.Valid { - t := creationDate.Time + a.Tags = parseStringListField(row.tags) + a.Categories = parseStringListField(row.categories) + if row.creationDate.Valid { + t := row.creationDate.Time a.CreatedAt = &t } - if pubDate.Valid { - t := pubDate.Time + if row.pubDate.Valid { + t := row.pubDate.Time a.PublishedAt = &t a.Status = models.ArticleStatusPublished - } else if scheduledDate.Valid { - t := scheduledDate.Time + } else if row.scheduledDate.Valid { + t := row.scheduledDate.Time a.PublishedAt = &t a.ScheduledAt = &t a.Status = models.ArticleStatusScheduled } else { a.Status = models.ArticleStatusDraft } - if priority.Valid { - a.IsFeatured = priority.Bool + if row.priority.Valid { + a.IsFeatured = row.priority.Bool } - if breakingNews.Valid { - a.BreakingNews = breakingNews.Bool + if row.breakingNews.Valid { + a.BreakingNews = row.breakingNews.Bool } - if commentStatus.Valid { - a.CommentStatus = normalizeCommentStatus(commentStatus.String) + if row.commentStatus.Valid { + a.CommentStatus = normalizeCommentStatus(row.commentStatus.String) } - if photoURL.Valid { - a.PhotoURL = photoURL.String + if row.photoURL.Valid { + a.PhotoURL = row.photoURL.String } - if photoAlt.Valid { - a.PhotoAlt = photoAlt.String + if row.photoAlt.Valid { + a.PhotoAlt = row.photoAlt.String } - return a, nil + return a } // NormalizeCanonicalURL trims a canonical-URL override and reports whether it is @@ -268,10 +378,16 @@ func parseStringListField(value sql.NullString) []string { return parsed } +// CollectArticles drains rows selected with ArticleSummaryColumns. +// +// It is the listing path, and there is deliberately no full-column counterpart: +// every caller that collects more than one article is building a list of +// excerpts, and the single read that needs the body -- the article detail +// endpoint -- scans its one row with ScanArticle directly. func CollectArticles(rows *sql.Rows) ([]models.Article, error) { var articles []models.Article for rows.Next() { - a, err := ScanArticle(rows) + a, err := ScanArticleSummary(rows) if err != nil { return nil, err } @@ -379,7 +495,7 @@ func GetRelatedArticlesBySlug(ctx context.Context, conn *sql.DB, slug string, k return nil, fmt.Errorf("k must be greater than 0") } - query := "SELECT " + articleSelectColumnsQualified + " " + + query := "SELECT " + articleSelectList(false, "a") + " " + "FROM articles AS src " + "JOIN article_embeddings AS src_vec ON src_vec.article_id = src.id " + "JOIN article_embeddings AS cand_vec ON cand_vec.article_id <> src.id " + diff --git a/server/internal/database/http_models_test.go b/server/internal/database/http_models_test.go index 6b7ff0f..89a009c 100644 --- a/server/internal/database/http_models_test.go +++ b/server/internal/database/http_models_test.go @@ -255,3 +255,60 @@ func TestDeriveExcerpt_KeepsEditorialBrackets(t *testing.T) { t.Errorf("deriveExcerpt = %q, want %q", got, want) } } + +// The listing column set must be the full set minus exactly the body. If a new +// column is added to articleColumnSet and quietly lands in summaryOmittedColumns +// too, listings stop returning it and the public site renders a blank field. +func TestArticleSummaryColumns_OmitOnlyTheBody(t *testing.T) { + full := map[string]bool{} + for _, name := range ArticleColumns { + full[name] = true + } + summary := map[string]bool{} + for _, name := range ArticleSummaryColumns { + summary[name] = true + } + + for name := range full { + if name == "text" { + continue + } + if !summary[name] { + t.Errorf("column %q is missing from ArticleSummaryColumns", name) + } + } + if summary["text"] { + t.Error("ArticleSummaryColumns still selects `text`; listings do not read the body") + } + if len(ArticleSummaryColumns) != len(ArticleColumns)-1 { + t.Errorf("ArticleSummaryColumns has %d columns, want %d", len(ArticleSummaryColumns), len(ArticleColumns)-1) + } +} + +// scanTargets is what makes the column list and the Scan positional agreement +// one fact rather than two. If they can disagree, every value after the +// mismatch lands in the wrong field. +func TestArticleScanTargets_MatchColumnCounts(t *testing.T) { + var row articleRow + if got, want := len(row.scanTargets(true)), len(ArticleColumns); got != want { + t.Errorf("full scan targets = %d, want %d", got, want) + } + if got, want := len(row.scanTargets(false)), len(ArticleSummaryColumns); got != want { + t.Errorf("summary scan targets = %d, want %d", got, want) + } +} + +func TestArticleSelectList_QualifiesForJoins(t *testing.T) { + if got := articleSelectList(false, "a"); !strings.HasPrefix(got, "a.`id`, a.`title`") { + t.Errorf("qualified select list = %q, want it to start with a.`id`, a.`title`", got) + } + if got := articleSelectList(false, ""); !strings.HasPrefix(got, "`id`, `title`") { + t.Errorf("unqualified select list = %q, want it to start with `id`, `title`", got) + } + if strings.Contains(articleSelectList(false, "a"), "`text`") { + t.Error("summary select list names `text`") + } + if !strings.Contains(articleSelectList(true, "a"), "`text`") { + t.Error("full select list is missing `text`") + } +} diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 9b91a04..26f8915 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -1227,7 +1227,7 @@ type ArticleParams struct { func queryArticles(r *http.Request, conn *sql.DB, params ArticleParams, limit, offset int) (*sql.Rows, error) { q := r.URL.Query() conditions, args := articleQueryFilters(r, params) - query := "SELECT `id`, `title`, `slug`, `description`, `text`, `excerpt`, `tags`, `categories`, `pub_date`, `mod_date`, `priority`, `breaking_news`, `comment_status`, `photo_url`, `focus_keyword`, `meta_description`, `seo_title`, `creation_date`, `scheduled_pub_date`, `canonical_url`, `noindex`, `photo_alt` FROM `articles`" + query := "SELECT " + db.ArticleSummarySelectList + " FROM `articles`" if len(conditions) > 0 { query += " WHERE " + strings.Join(conditions, " AND ") } @@ -1316,6 +1316,15 @@ func articleQueryFilters(r *http.Request, params ArticleParams) ([]string, []any // A brand-new CMS draft looks exactly like one of those until an author or a // category is attached, so editors keep unpublished rows regardless — without // the exemption a draft is invisible in the CMS listing until it is filed. + // + // Kept as a predicate over the columns rather than over the derived + // article_categories / articles_authors indexes. Rewriting it as two EXISTS + // was measured: it saves 3.8ms on the paging COUNT and costs 0.1ms on the + // listing itself, and in exchange the definition of "filed" would move from + // the columns to two indexes that agree with them by data rather than by + // construction. Four milliseconds does not buy that -- the failure mode is + // articles silently vanishing from every listing while still resolving by + // direct link. artifactFilter := "((TRIM(COALESCE(`authors`, '')) <> '' AND TRIM(`authors`) <> '[]') OR (TRIM(COALESCE(`categories`, '')) <> '' AND TRIM(`categories`) <> '[]'))" if isEditor { artifactFilter = "(" + artifactFilter + " OR `pub_date` IS NULL)" @@ -2136,6 +2145,10 @@ func PostArticles(conn *sql.DB) http.HandlerFunc { writeError(w, http.StatusInternalServerError, err.Error()) return } + if err := db.ReplaceArticleCategories(r.Context(), conn, articleID, body.Categories); err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } if err := incrementTaxonomyArticleCounts(r.Context(), conn, body.Categories); err != nil { writeError(w, http.StatusInternalServerError, err.Error()) return @@ -2228,6 +2241,15 @@ func PutArticle(conn *sql.DB) http.HandlerFunc { writeError(w, http.StatusInternalServerError, err.Error()) return } + // Keyed on the new slug, because the UPDATE above may have renamed it. + // Unconditional, unlike the taxonomy counts: the index describes where + // the article is filed, which is true of archived rows too -- they are + // excluded by the listing's own archived_at predicate, not by being + // missing from here. + if err := db.ReplaceArticleCategoriesBySlug(r.Context(), conn, body.Slug, body.Categories); err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } if isActiveArticle { if err := reconcileTaxonomyArticleCounts(r.Context(), conn, oldCategories, body.Categories); err != nil { writeError(w, http.StatusInternalServerError, err.Error()) @@ -2495,6 +2517,13 @@ func PatchArticle(conn *sql.DB) http.HandlerFunc { return } } + // nextCategories is oldCategories unless this patch carried a categories + // field, so an unrelated patch rewrites the index to what it already + // held rather than clearing it. + if err := db.ReplaceArticleCategoriesBySlug(r.Context(), conn, targetSlug, nextCategories); err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } if isActiveArticle { if err := reconcileTaxonomyArticleCounts(r.Context(), conn, oldCategories, nextCategories); err != nil { writeError(w, http.StatusInternalServerError, err.Error()) From 5bf739b2da5b52d748c9ff2b1d24bd617bb604a8 Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:32:59 -0400 Subject: [PATCH 5/6] Index the columns article lookups actually search on `slug` is how every article is addressed -- the detail endpoint, the comment thread, the permalink check, the featured-article write -- and nothing indexed it, so each of those scanned the whole corpus to return one row. `articles_authors` had only a primary key on its own id, so resolving a page of bylines scanned the entire join table, once per listing and once per homepage block. Both are prefix/secondary ADD INDEX, deliberately. The obvious fix for slug is to narrow the LONGTEXT to VARCHAR first, and that is a trap: retyping a column rewrites the table, and rewriting `articles` re-tokenizes 44MB of bodies into the two FULLTEXT indexes. Measured at 8m53s for one ALTER on a corpus this size, during which a second connection running the CMS's own startup migration sat in "Waiting for table metadata lock" -- in production that is the newsroom's writes queued behind a container that is not yet serving traffic. The prefix index takes 0.31s on the same shape and yields the same plan. 191 characters is above the longest slug in the corpus (154) and stays under the 767-byte limit older row formats impose. A prefix index only narrows candidates -- InnoDB rechecks the full value -- so it can cost an extra row read but never return a wrong article. Not UNIQUE: uniqueness is a property of the data, and the corpus belongs to the ETL. Production already carries one duplicated slug from an archived pair, which a UNIQUE index would turn into a CMS that will not start. Co-Authored-By: Claude Opus 5 --- server/internal/database/users.go | 50 +++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/internal/database/users.go b/server/internal/database/users.go index c4645f4..8550378 100644 --- a/server/internal/database/users.go +++ b/server/internal/database/users.go @@ -45,6 +45,56 @@ func EnsureArticlesPublishedIndex(ctx context.Context, conn *sql.DB) error { return err } +// EnsureArticlesSlugIndex indexes the column every article is addressed by: +// the detail endpoint, the comment thread, the permalink check, the +// featured-article write. Without it each of those is a full scan of the +// migrated corpus -- ten thousand rows read to return one. +// +// It is a PREFIX index, and that detail is the whole design. The ETL ships +// `slug` as LONGTEXT, which cannot be indexed whole; the obvious fix is to +// narrow the column to VARCHAR first. Do not. Retyping a column rewrites the +// table, and rewriting `articles` means re-tokenizing 44MB of article bodies +// into the two FULLTEXT indexes -- measured at over six minutes on a corpus +// this size, holding a lock the newsroom's writes would queue behind, and on a +// blue/green deploy the container doing it is not even the one serving. Adding +// a secondary index is an in-place operation that does none of that. +// +// 191 characters is a prefix no slug in the corpus reaches (the longest is 154) +// and stays under the 767-byte limit older row formats impose, so it is exact +// in practice and safe on any table layout. Even where it were not, a prefix +// index only narrows the candidates -- InnoDB rechecks the full value -- so +// this can cost an extra row read but can never return a wrong article. +// +// The index is deliberately not UNIQUE. Uniqueness is a property of the data, +// and the corpus belongs to the ETL, not the CMS: production already carries +// one duplicated slug from an archived pair, which a UNIQUE index would turn +// into a CMS that will not start. +func EnsureArticlesSlugIndex(ctx context.Context, conn *sql.DB) error { + _, err := conn.ExecContext(ctx, ` + ALTER TABLE articles + ADD INDEX IF NOT EXISTS idx_articles_slug (`+"`slug`"+`(191)) + `) + return err +} + +// EnsureArticleAuthorsIndex indexes the join column that every listing reads. +// +// LoadAuthorsByArticleIDs resolves a page of articles' bylines with one +// `WHERE articles_id IN (...)`, but the ETL creates `articles_authors` with +// only a primary key on its own id, so that lookup scanned the entire join +// table on every listing request -- including each of the homepage's six +// section blocks. author_id rides along so the index covers the join. +// +// Cheap and safe to run at boot, unlike anything touching `articles`: the table +// is a few hundred kilobytes and carries no FULLTEXT index to rebuild. +func EnsureArticleAuthorsIndex(ctx context.Context, conn *sql.DB) error { + _, err := conn.ExecContext(ctx, ` + ALTER TABLE articles_authors + ADD INDEX IF NOT EXISTS idx_articles_authors_article (`+"`articles_id`, `author_id`"+`) + `) + return err +} + // EnsureArticlesSearchIndex adds the FULLTEXT indexes public search ranks on. // // Two indexes, not one: the combined index answers "does this article match at From fcdafe53a3e23e405a6f7e309603ab58459d6c4d Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 7 Aug 2026 21:33:14 -0400 Subject: [PATCH 6/6] Match articles to sections through an index, not a LIKE scan Section filtering ran `LOWER(categories) LIKE '%"news"%'` against a LONGTEXT column. A leading wildcard rules out every index, so each section page read the whole articles table -- twice, since the listing pages with a COUNT(*) alongside -- and so did each of the homepage's six blocks and every taxonomy recount. article_categories is a derived index of that column: one row per (article, category title), so the question becomes an equality join. The optimizer flips it, driving from the index and doing eq_ref primary lookups instead of applying REPLACE/LOWER to a LONGTEXT for 6,038 rows. A section page goes from 76.2ms to 40.2ms, search from 111.6ms to 66.9ms. It is strictly derived, never authoritative. `articles`.`categories` remains the source of truth, article writes keep the index in step, and it is rebuilt at every startup -- deliberately unconditional, and deliberately not behind CMS_REBUILD_TAXONOMY_COUNTS_ON_STARTUP. The corpus is periodically reloaded wholesale by the ETL, which renumbers article ids: stale contents would not merely be old, they would point at the wrong articles. A rebuild an operator has to remember is one that gets forgotten on the reseed that needs it most, and it costs one scan of a ten-thousand-row table. Creating it is fatal on failure, unlike the index migrations, because the queries now depend on it: a CMS that booted without it would serve empty sections rather than slow ones. EXISTS rather than IN because callers negate the fragment (see ReportOrphanedArticles), and NOT IN over a subquery that can yield NULL evaluates to UNKNOWN and silently matches nothing. The LIKE mechanism is deleted rather than left dead. Its rules -- the anchoring that keeps Women's Basketball out of mens-basketball, the alias map, the restored possessives -- move intact to CategoryMatchValues, and their tests move with them: matchesCategories now composes the two real halves, what the rebuild would index and what the query would ask for. Verified against the full corpus: for all 76 categories the old and new predicates select identical article sets, and every one of the 132 unindexed articles genuinely has no categories. Co-Authored-By: Claude Opus 5 --- .../internal/database/article_categories.go | 235 ++++++++++++++++++ .../database/article_categories_test.go | 71 ++++++ .../database/schema/article_categories.sql | 11 + server/internal/database/taxonomy.go | 88 ++++--- .../database/taxonomy_integration_test.go | 30 +-- server/internal/database/taxonomy_test.go | 88 ++++--- .../article_default_order_integration_test.go | 6 +- .../article_patch_integration_test.go | 8 + .../featured_article_integration_test.go | 4 + server/internal/handlers/handlers_test.go | 12 +- .../handlers/taxonomy_integration_test.go | 42 +++- 11 files changed, 495 insertions(+), 100 deletions(-) create mode 100644 server/internal/database/article_categories.go create mode 100644 server/internal/database/article_categories_test.go create mode 100644 server/internal/database/schema/article_categories.sql diff --git a/server/internal/database/article_categories.go b/server/internal/database/article_categories.go new file mode 100644 index 0000000..ab37d1f --- /dev/null +++ b/server/internal/database/article_categories.go @@ -0,0 +1,235 @@ +package database + +import ( + "context" + "database/sql" + "encoding/json" + "strings" +) + +// article_categories is a derived index of `articles`.`categories`: one row per +// (article, category title), so "which articles are in this section" is an index +// lookup instead of a scan. +// +// It replaces a `categories LIKE '%"news"%'` predicate. That predicate could +// never use an index -- a leading wildcard on a LONGTEXT column rules it out -- +// so every section page, every homepage block and every taxonomy count read the +// whole articles table, twice per request once the paging COUNT(*) is included. +// +// The table is strictly derived, never authoritative. `articles`.`categories` +// remains the source of truth, this is rebuilt from it at every startup, and +// article writes keep it in step in between. That combination is deliberate: +// the corpus is periodically reloaded wholesale by the WordPress ETL, which +// knows nothing about this table, and a derived index that silently survives a +// reseed would serve empty section pages with no indication why. + +// maxCategoryLength matches the VARCHAR in schema/article_categories.sql. A +// longer title is skipped rather than truncated, because a truncated key would +// match a section it does not belong to. +const maxCategoryLength = 191 + +// A note for the next person who looks at the listing's paging COUNT(*), which +// evaluates the artifact filter (see handlers.articleQueryFilters) over every +// published row and is the largest remaining cost on /v1/articles. +// +// Two rewrites were measured against this corpus and both rejected: +// +// - Two EXISTS over articles_authors and this table. Correct on today's data +// -- zero disagreements across all 9,371 development and 10,113 production +// rows -- and worth 13.1ms -> 9.4ms on the COUNT, but it also costs 0.1ms +// on the listing query, which materializes both subqueries to return 21 +// rows. Under 4ms net, in exchange for moving the definition of "filed" +// onto indexes that match the columns by data rather than by construction. +// - A PERSISTENT generated column with an index, which would make the COUNT +// index-only. Adding one rewrites `articles`, and rewriting `articles` +// re-tokenizes the bodies into the two FULLTEXT indexes: 8m53s for a single +// ALTER here, during which the CMS's own startup migration sat in "Waiting +// for table metadata lock". In production that is the newsroom's writes +// queued behind a container that is not yet serving traffic. +// +// The cheap wins are already taken (see EnsureArticlesPublishedIndex and +// EnsureArticleAuthorsIndex). What is left needs either a schema the ETL owns +// or an approximate count, and neither is worth four milliseconds. + +func EnsureArticleCategoriesTable(ctx context.Context, conn *sql.DB) error { + _, err := conn.ExecContext(ctx, TableSchema("article_categories")) + return err +} + +// normalizeCategoryValues turns the stored `categories` JSON into the keys this +// table is indexed on. +// +// Only a well-formed JSON array counts. parseStringListField falls back to +// splitting on commas for rows that are not JSON, but the LIKE predicate this +// replaces anchored on JSON quotes and so never matched those rows either; +// indexing them here would quietly add articles to sections that have never +// shown them. +func normalizeCategoryValues(raw string) []string { + trimmed := strings.TrimSpace(raw) + if trimmed == "" { + return nil + } + var parsed []string + if err := json.Unmarshal([]byte(trimmed), &parsed); err != nil { + return nil + } + return normalizeCategoryList(parsed) +} + +// normalizeCategoryList lowercases, trims and dedupes category titles. The +// lowercasing mirrors the LOWER() the old predicate applied to the column, and +// is what lets CategoryMatchValues compare against these rows directly. +func normalizeCategoryList(categories []string) []string { + out := make([]string, 0, len(categories)) + for _, category := range categories { + value := strings.ToLower(strings.TrimSpace(category)) + if value == "" || len(value) > maxCategoryLength { + continue + } + duplicate := false + for _, existing := range out { + if existing == value { + duplicate = true + break + } + } + if !duplicate { + out = append(out, value) + } + } + return out +} + +// execer is satisfied by both *sql.DB and *sql.Tx, so the write-path helpers can +// join a caller's transaction. +type execer interface { + ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error) +} + +// ReplaceArticleCategories rewrites one article's rows. Delete-then-insert +// rather than a diff: an article carries a handful of categories, and the +// simpler operation cannot leave a stale row behind. +func ReplaceArticleCategories(ctx context.Context, conn execer, articleID int64, categories []string) error { + if _, err := conn.ExecContext(ctx, "DELETE FROM `article_categories` WHERE `article_id` = ?", articleID); err != nil { + return err + } + values := normalizeCategoryList(categories) + if len(values) == 0 { + return nil + } + + placeholders := make([]string, 0, len(values)) + args := make([]any, 0, len(values)*2) + for _, value := range values { + placeholders = append(placeholders, "(?, ?)") + args = append(args, articleID, value) + } + _, err := conn.ExecContext(ctx, + "INSERT INTO `article_categories` (`article_id`, `category`) VALUES "+strings.Join(placeholders, ", "), + args..., + ) + return err +} + +// ReplaceArticleCategoriesBySlug is ReplaceArticleCategories for the update +// paths, which address an article by slug. A slug that matches nothing is not an +// error here: the caller's own UPDATE already reported that as a 404, and +// failing again would turn one missing article into a 500. +func ReplaceArticleCategoriesBySlug(ctx context.Context, conn *sql.DB, slug string, categories []string) error { + var articleID int64 + switch err := conn.QueryRowContext(ctx, + "SELECT `id` FROM `articles` WHERE `slug` = ? LIMIT 1", strings.TrimSpace(slug), + ).Scan(&articleID); err { + case nil: + case sql.ErrNoRows: + return nil + default: + return err + } + return ReplaceArticleCategories(ctx, conn, articleID, categories) +} + +// RebuildArticleCategories rebuilds the whole table from `articles`. +// +// Run at every startup. It is cheap -- the corpus is under ten thousand rows and +// two or three categories each -- and running it unconditionally is what makes +// the table safe to depend on: after a WordPress ETL reseed, which replaces +// `articles` wholesale and renumbers its ids, the previous contents are not +// merely stale but point at the wrong articles. +func RebuildArticleCategories(ctx context.Context, conn *sql.DB) error { + rows, err := conn.QueryContext(ctx, "SELECT `id`, `categories` FROM `articles`") + if err != nil { + return err + } + defer rows.Close() + + type articleCategories struct { + id int64 + categories []string + } + var pending []articleCategories + for rows.Next() { + var id int64 + var raw sql.NullString + if err := rows.Scan(&id, &raw); err != nil { + return err + } + values := normalizeCategoryValues(raw.String) + if len(values) == 0 { + continue + } + pending = append(pending, articleCategories{id: id, categories: values}) + } + if err := rows.Err(); err != nil { + return err + } + // The scan is finished before the writes start: holding a result set open + // across them would need a second connection from the pool, and the pool is + // the resource this whole change is trying to spend less of. + + tx, err := conn.BeginTx(ctx, nil) + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + + // TRUNCATE rather than DELETE: this is a full replacement, and it leaves no + // undo log to write for rows that are about to be re-inserted anyway. + if _, err := tx.ExecContext(ctx, "TRUNCATE TABLE `article_categories`"); err != nil { + return err + } + + // Batched, because one INSERT per article is ten thousand round trips on + // every boot. + const batchSize = 500 + placeholders := make([]string, 0, batchSize) + args := make([]any, 0, batchSize*2) + flush := func() error { + if len(placeholders) == 0 { + return nil + } + _, err := tx.ExecContext(ctx, + "INSERT INTO `article_categories` (`article_id`, `category`) VALUES "+strings.Join(placeholders, ", "), + args..., + ) + placeholders = placeholders[:0] + args = args[:0] + return err + } + + for _, article := range pending { + for _, category := range article.categories { + placeholders = append(placeholders, "(?, ?)") + args = append(args, article.id, category) + if len(placeholders) >= batchSize { + if err := flush(); err != nil { + return err + } + } + } + } + if err := flush(); err != nil { + return err + } + return tx.Commit() +} diff --git a/server/internal/database/article_categories_test.go b/server/internal/database/article_categories_test.go new file mode 100644 index 0000000..ac5faf6 --- /dev/null +++ b/server/internal/database/article_categories_test.go @@ -0,0 +1,71 @@ +package database + +import ( + "reflect" + "strings" + "testing" +) + +func TestNormalizeCategoryValues(t *testing.T) { + cases := map[string]struct { + raw string + want []string + }{ + "json array": {`["News","Campus"]`, []string{"news", "campus"}}, + // The escape encoding/json used to emit for "&". Decoding resolves it, + // which is what makes the SQL-side REPLACE unnecessary. + "escaped ampersand": {`["Comics & Puzzles"]`, []string{"comics & puzzles"}}, + "whitespace": {`[" News "]`, []string{"news"}}, + "duplicates": {`["News","news"]`, []string{"news"}}, + "empty members": {`["News",""]`, []string{"news"}}, + "empty array": {`[]`, nil}, + "blank": {" ", nil}, + // A row that is not a JSON array never matched the LIKE predicate this + // replaced, so indexing it would add articles to sections that have + // never shown them. + "not json": {`News,Campus`, nil}, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := normalizeCategoryValues(tc.raw) + if len(got) == 0 && len(tc.want) == 0 { + return + } + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("normalizeCategoryValues(%q) = %v, want %v", tc.raw, got, tc.want) + } + }) + } +} + +// A title longer than the column is skipped rather than truncated: a truncated +// key would compare equal to a section it does not belong to. +func TestNormalizeCategoryListSkipsOverlongTitles(t *testing.T) { + long := strings.Repeat("a", maxCategoryLength+1) + got := normalizeCategoryList([]string{"News", long}) + if !reflect.DeepEqual(got, []string{"news"}) { + t.Errorf("got %v, want just [news]", got) + } +} + +// The index stores what CategoryMatchValues asks for. If the two normalizations +// ever diverge, every section page silently empties. +func TestIndexedCategoriesMatchTheValuesQueriedFor(t *testing.T) { + withCategoryAliases(t, map[string][]string{"entertainment": {"Arts & Entertainment"}}) + + indexed := normalizeCategoryValues(`["Arts & Entertainment"]`) + wanted := CategoryMatchValues("entertainment") + + found := false + for _, have := range indexed { + for _, want := range wanted { + if have == want { + found = true + } + } + } + if !found { + t.Errorf("indexed %v matches none of the queried values %v", indexed, wanted) + } +} diff --git a/server/internal/database/schema/article_categories.sql b/server/internal/database/schema/article_categories.sql new file mode 100644 index 0000000..dd9e2e2 --- /dev/null +++ b/server/internal/database/schema/article_categories.sql @@ -0,0 +1,11 @@ +CREATE TABLE IF NOT EXISTS article_categories ( + article_id BIGINT NOT NULL, + -- The category title as it appears in `articles`.`categories`, lowercased and + -- trimmed. 191 is the longest utf8mb4 VARCHAR that fits a legacy 767-byte + -- index prefix; the longest category in the corpus is under 30. + category VARCHAR(191) NOT NULL, + PRIMARY KEY (article_id, category), + -- The section-page lookup: given a handful of category titles, find the + -- articles. article_id rides along so the index covers the whole subquery. + KEY idx_article_categories_category (category, article_id) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 diff --git a/server/internal/database/taxonomy.go b/server/internal/database/taxonomy.go index f61e33d..04483e2 100644 --- a/server/internal/database/taxonomy.go +++ b/server/internal/database/taxonomy.go @@ -290,19 +290,9 @@ func categoryAliasesFor(slug string) []string { return categoryAliasBySlug[slug] } -// CategoryColumnExpr is the SQL expression every category match runs against. -// -// `articles`.`categories` is a JSON array of category titles, but it is written -// by two producers that escape it differently: the ETL emits a plain "&", while -// Go's encoding/json HTML-escapes it to a backslash-u escape (see FormatTags), -// so an article edited in the CMS stops matching the very section it is filed -// under. Folding that escape away here means callers only ever reason about one -// spelling. FormatTags no longer produces it, but rows written before that fix -// still carry it. -const CategoryColumnExpr = "REPLACE(LOWER(`categories`), '\\\\u0026', '&')" - -// CategoryMatchPatterns returns the CategoryColumnExpr LIKE patterns that -// identify articles filed under a taxonomy slug. +// CategoryMatchValues returns the category titles that identify articles filed +// under a taxonomy slug, normalized the same way article_categories stores +// them: lowercased and trimmed. // // WordPress category text does not match our slugs literally, so a slug stands // in for several spellings: "comics-puzzles" has to find "Comics & Puzzles". @@ -310,35 +300,40 @@ const CategoryColumnExpr = "REPLACE(LOWER(`categories`), '\\\\u0026', '&')" // article listing and the count rebuild call it, because when they disagreed a // section could list 2545 articles while reporting 8. // -// Every pattern is anchored on the JSON quotes around a member, so a slug -// matches a WHOLE category and never a fragment of a longer one. Unanchored -// patterns silently merged sibling taxonomies: `%puzzles%` matched the parent +// Every value is a WHOLE category title, compared for equality, so a slug never +// matches a fragment of a longer one. This used to be a LIKE pattern anchored on +// the JSON quotes around an array member, for the same reason: unanchored +// patterns silently merged sibling taxonomies -- `%puzzles%` matched the parent // title "Comics & Puzzles" and so pulled all 219 comics into the Puzzles // subsection, and `%men's basketball%` is a substring of "Women's Basketball", // which folded the women's team into the men's. Both read as plausible-but-wrong -// pages rather than as errors, so anchoring is load-bearing, not cosmetic. +// pages rather than as errors, so exactness is load-bearing, not cosmetic. An +// equality join gives it for free, and can use an index besides. // // Because the match is exact, a slug that is not the canonicalized category // string resolves to nothing on its own -- it needs an alias row. Those come // from the cache, so callers must have run RefreshCategoryAliases first; // EnsureTaxonomyTable does it at startup. -func CategoryMatchPatterns(slug string) []string { +// +// Ampersand escaping used to be folded in SQL here too: the ETL emits a plain +// "&" while Go's encoding/json HTML-escapes it to & (see FormatTags), so an +// article edited in the CMS stopped matching its own section. That fold is now +// implicit -- article_categories is built by JSON-decoding the column, which +// resolves the escape to the character it stands for. +func CategoryMatchValues(slug string) []string { normalized := strings.ToLower(strings.TrimSpace(slug)) if normalized == "" { return nil } - patterns := make([]string, 0, 4) + values := make([]string, 0, 4) add := func(value string) { - // The JSON quotes are what make the match exact; without them a - // pattern matches any category containing the phrase. - pattern := `%"` + value + `"%` - for _, existing := range patterns { - if existing == pattern { + for _, existing := range values { + if existing == value { return } } - patterns = append(patterns, pattern) + values = append(values, value) } add(normalized) @@ -353,7 +348,7 @@ func CategoryMatchPatterns(slug string) []string { for _, alias := range categoryAliasesFor(normalized) { add(strings.ToLower(strings.TrimSpace(alias))) } - return patterns + return values } // possessiveStems are the words a slug can only have lost an apostrophe from. @@ -438,19 +433,46 @@ func taxonomyMatchSlugs(ctx context.Context, conn *sql.DB) (map[string][]string, // TaxonomyCountCondition builds the WHERE fragment matching articles in any of // the given slugs, along with its arguments. +// +// It reads the article_categories index rather than the `categories` column. +// The predicate it replaced was a chain of `LOWER(categories) LIKE '%"news"%'` +// -- a leading wildcard, so no index could ever serve it, and every section page +// scanned the whole table to answer it (twice, since the listing pages with a +// COUNT(*) alongside). +// +// EXISTS rather than IN: callers negate this fragment (see +// ReportOrphanedArticles), and NOT IN against a subquery that can yield NULL +// evaluates to UNKNOWN and quietly returns nothing. NOT EXISTS has no such +// edge. +// +// The correlation name is the bare `articles` table, which every caller selects +// from unaliased. func TaxonomyCountCondition(slugs []string) (string, []any) { - var clauses []string - var args []any + var values []string + seen := make(map[string]bool) for _, slug := range slugs { - for _, pattern := range CategoryMatchPatterns(slug) { - clauses = append(clauses, CategoryColumnExpr+" LIKE ?") - args = append(args, pattern) + for _, value := range CategoryMatchValues(slug) { + if seen[value] { + continue + } + seen[value] = true + values = append(values, value) } } - if len(clauses) == 0 { + if len(values) == 0 { return "", nil } - return "(" + strings.Join(clauses, " OR ") + ")", args + + placeholders := make([]string, len(values)) + args := make([]any, len(values)) + for i, value := range values { + placeholders[i] = "?" + args[i] = value + } + + condition := "EXISTS (SELECT 1 FROM `article_categories` `ac` WHERE `ac`.`article_id` = `articles`.`id` " + + "AND `ac`.`category` IN (" + strings.Join(placeholders, ", ") + "))" + return condition, args } // countArticlesForSlugs counts the articles a taxonomy row matches, over the diff --git a/server/internal/database/taxonomy_integration_test.go b/server/internal/database/taxonomy_integration_test.go index 9561961..6acbb2f 100644 --- a/server/internal/database/taxonomy_integration_test.go +++ b/server/internal/database/taxonomy_integration_test.go @@ -91,11 +91,11 @@ func TestTaxonomyAliasSeedAndCacheRoundTrip(t *testing.T) { // defaultCategoryAliases rather than a literal list: the map grows every // time another orphaned category is filed, and a test that pins the // contents fails on the filing rather than on anything being broken. - patterns := CategoryMatchPatterns("entertainment") + values := CategoryMatchValues("entertainment") for _, alias := range defaultCategoryAliases["entertainment"] { - want := `%"` + strings.ToLower(alias) + `"%` - if !containsPattern(patterns, want) { - t.Errorf("entertainment patterns %v missing the seeded alias %s", patterns, want) + want := strings.ToLower(alias) + if !containsValue(values, want) { + t.Errorf("entertainment values %v missing the seeded alias %s", values, want) } } // The aliases must not be HTML-escaped in the column, or one holding an @@ -118,8 +118,8 @@ func TestTaxonomyAliasSeedAndCacheRoundTrip(t *testing.T) { } // A slug with no default keeps no aliases. - if got := CategoryMatchPatterns(unseededSlug); len(got) != 1 { - t.Errorf("%s patterns = %v, want just its own", unseededSlug, got) + if got := CategoryMatchValues(unseededSlug); len(got) != 1 { + t.Errorf("%s values = %v, want just its own", unseededSlug, got) } } @@ -169,8 +169,8 @@ func TestTaxonomyAliasRefreshPicksUpWrites(t *testing.T) { if err := RefreshCategoryAliases(ctx, conn); err != nil { t.Fatalf("refresh: %v", err) } - if got := CategoryMatchPatterns("movies"); len(got) != 1 { - t.Fatalf("movies patterns = %v, want just its own", got) + if got := CategoryMatchValues("movies"); len(got) != 1 { + t.Fatalf("movies values = %v, want just its own", got) } if _, err := conn.ExecContext(ctx, @@ -180,20 +180,20 @@ func TestTaxonomyAliasRefreshPicksUpWrites(t *testing.T) { } // Still stale until the cache is told, which is why every write path calls // RefreshCategoryAliases. - if got := CategoryMatchPatterns("movies"); len(got) != 1 { - t.Errorf("patterns changed before a refresh: %v", got) + if got := CategoryMatchValues("movies"); len(got) != 1 { + t.Errorf("values changed before a refresh: %v", got) } if err := RefreshCategoryAliases(ctx, conn); err != nil { t.Fatalf("refresh after write: %v", err) } - if got := CategoryMatchPatterns("movies"); !containsPattern(got, `%"movies i've seen"%`) { - t.Errorf("movies patterns %v missing the written alias", got) + if got := CategoryMatchValues("movies"); !containsValue(got, "movies i've seen") { + t.Errorf("movies values %v missing the written alias", got) } } -func containsPattern(patterns []string, want string) bool { - for _, pattern := range patterns { - if pattern == want { +func containsValue(values []string, want string) bool { + for _, value := range values { + if value == want { return true } } diff --git a/server/internal/database/taxonomy_test.go b/server/internal/database/taxonomy_test.go index 3765cc8..ba8e970 100644 --- a/server/internal/database/taxonomy_test.go +++ b/server/internal/database/taxonomy_test.go @@ -7,49 +7,49 @@ import ( func assertPatterns(t *testing.T, slug string, want []string) { t.Helper() - got := CategoryMatchPatterns(slug) + got := CategoryMatchValues(slug) if len(got) != len(want) { - t.Fatalf("%s: got %d patterns %v, want %d %v", slug, len(got), got, len(want), want) + t.Fatalf("%s: got %d values %v, want %d %v", slug, len(got), got, len(want), want) } for i := range want { if got[i] != want[i] { - t.Errorf("%s: pattern %d = %q, want %q", slug, i, got[i], want[i]) + t.Errorf("%s: value %d = %q, want %q", slug, i, got[i], want[i]) } } } -func TestCategoryMatchPatternsExpandsDashedSlugs(t *testing.T) { +func TestCategoryMatchValuesExpandsDashedSlugs(t *testing.T) { // A dashed slug has to find the WordPress spelling, which uses spaces and // often an ampersand: "comics-puzzles" must match "Comics & Puzzles". assertPatterns(t, "comics-puzzles", []string{ - `%"comics-puzzles"%`, `%"comics puzzles"%`, `%"comics & puzzles"%`, + "comics-puzzles", "comics puzzles", "comics & puzzles", }) } -func TestCategoryMatchPatternsRestoresPossessiveApostrophe(t *testing.T) { +func TestCategoryMatchValuesRestoresPossessiveApostrophe(t *testing.T) { // The category text is "Men's Basketball"; no slug can carry the // apostrophe, so without this pattern the subsection matched nothing. assertPatterns(t, "mens-basketball", []string{ - `%"mens-basketball"%`, `%"mens basketball"%`, `%"mens & basketball"%`, `%"men's basketball"%`, + "mens-basketball", "mens basketball", "mens & basketball", "men's basketball", }) } -func TestCategoryMatchPatternsLeavesPluralsAlone(t *testing.T) { +func TestCategoryMatchValuesLeavesPluralsAlone(t *testing.T) { // "philly-sports" is a plural, not a possessive: guessing "philly sport's" // would add a pattern that matches nothing. - for _, pattern := range CategoryMatchPatterns("philly-sports") { - if strings.Contains(pattern, "'") { - t.Fatalf("unexpected possessive pattern %q", pattern) + for _, value := range CategoryMatchValues("philly-sports") { + if strings.Contains(value, "'") { + t.Fatalf("unexpected possessive value %q", value) } } } -func TestCategoryMatchPatternsSingleWordSlug(t *testing.T) { - assertPatterns(t, "comics", []string{`%"comics"%`}) +func TestCategoryMatchValuesSingleWordSlug(t *testing.T) { + assertPatterns(t, "comics", []string{"comics"}) } -func TestCategoryMatchPatternsEmpty(t *testing.T) { - if got := CategoryMatchPatterns(" "); got != nil { +func TestCategoryMatchValuesEmpty(t *testing.T) { + if got := CategoryMatchValues(" "); got != nil { t.Fatalf("got %v, want nil for a blank slug", got) } } @@ -75,14 +75,17 @@ func withCategoryAliases(t *testing.T, aliases map[string][]string) { // editor or a copy-paste, which would make the tests below silently vacuous. var escapedAmp = `\` + "u0026" -// matchesCategories reports whether any pattern for slug would match a -// `categories` JSON array, mirroring what CategoryColumnExpr + LIKE does in -// SQL: lowercase the column, unescape the ampersand, then substring-match. +// matchesCategories reports whether slug matches a `categories` JSON array. It +// composes the two halves of the real lookup -- the rows RebuildArticleCategories +// would index for the article, and the values TaxonomyCountCondition would ask +// for -- so these assertions describe what the database will actually answer. func matchesCategories(slug, categoriesJSON string) bool { - column := strings.ReplaceAll(strings.ToLower(categoriesJSON), escapedAmp, "&") - for _, pattern := range CategoryMatchPatterns(slug) { - if strings.Contains(column, strings.Trim(pattern, "%")) { - return true + indexed := normalizeCategoryValues(categoriesJSON) + for _, want := range CategoryMatchValues(slug) { + for _, have := range indexed { + if have == want { + return true + } } } return false @@ -165,24 +168,22 @@ func TestCategoryMatchToleratesEscapedAmpersand(t *testing.T) { } } -func TestTaxonomyCountConditionORsEverySlug(t *testing.T) { +func TestTaxonomyCountConditionMatchesEverySlug(t *testing.T) { // A section matches its own slug OR any child's, so a container section // with no category of its own still resolves to its subsections' articles. + // Every spelling lands in one IN list, which is that OR. condition, args := TaxonomyCountCondition([]string{"special-editions", "welcome-week"}) if condition == "" { t.Fatal("expected a condition") } - if strings.Contains(condition, " AND ") { - t.Errorf("condition must OR its slugs, not AND them: %s", condition) - } - // 3 patterns per dashed slug, two slugs. - if got := strings.Count(condition, "LIKE ?"); got != 6 { + // 3 spellings per dashed slug, two slugs. + if got := strings.Count(condition, "?"); got != 6 { t.Errorf("got %d placeholders, want 6: %s", got, condition) } if len(args) != 6 { t.Errorf("got %d args, want 6: %v", len(args), args) } - for _, want := range []string{`%"welcome week"%`, `%"special editions"%`} { + for _, want := range []string{"welcome week", "special editions"} { found := false for _, arg := range args { if arg == want { @@ -190,16 +191,31 @@ func TestTaxonomyCountConditionORsEverySlug(t *testing.T) { } } if !found { - t.Errorf("missing pattern %q in %v", want, args) + t.Errorf("missing category %q in %v", want, args) } } } -func TestTaxonomyCountConditionMatchesOnTheNormalizedColumn(t *testing.T) { - // The escape-folding has to be in the SQL, not just in the patterns. +// The predicate has to be correlated on the outer articles row, and it has to +// be EXISTS: callers negate it, and NOT IN over a subquery that can produce +// NULL evaluates to UNKNOWN and silently matches nothing. +func TestTaxonomyCountConditionIsCorrelatedExists(t *testing.T) { condition, _ := TaxonomyCountCondition([]string{"comics"}) - if !strings.Contains(condition, CategoryColumnExpr) { - t.Errorf("condition must match on %s, got %s", CategoryColumnExpr, condition) + if !strings.HasPrefix(condition, "EXISTS (") { + t.Errorf("condition must be an EXISTS, got %s", condition) + } + if !strings.Contains(condition, "`ac`.`article_id` = `articles`.`id`") { + t.Errorf("condition must correlate on the outer articles row, got %s", condition) + } +} + +// Slugs resolving to the same spelling must not each contribute a placeholder; +// a section and its identically-named subsection would otherwise double the +// argument list on every section page. +func TestTaxonomyCountConditionDeduplicatesValues(t *testing.T) { + _, args := TaxonomyCountCondition([]string{"comics", "comics"}) + if len(args) != 1 { + t.Errorf("got %d args, want 1: %v", len(args), args) } } @@ -281,11 +297,11 @@ func TestCategoryAliasesAreCaseInsensitiveOnTheSlug(t *testing.T) { } } -func TestCategoryMatchPatternsDeduplicatesAliases(t *testing.T) { +func TestCategoryMatchValuesDeduplicatesAliases(t *testing.T) { // An alias that merely restates a derived pattern must not double the // placeholders in every query that uses the slug. withCategoryAliases(t, map[string][]string{"comics": {"Comics"}}) - assertPatterns(t, "comics", []string{`%"comics"%`}) + assertPatterns(t, "comics", []string{"comics"}) } func TestDefaultCategoryAliasesCoverTheKnownMismatches(t *testing.T) { diff --git a/server/internal/handlers/article_default_order_integration_test.go b/server/internal/handlers/article_default_order_integration_test.go index f83dd83..a204ec7 100644 --- a/server/internal/handlers/article_default_order_integration_test.go +++ b/server/internal/handlers/article_default_order_integration_test.go @@ -33,9 +33,13 @@ func seedDefaultOrderArticles(t *testing.T, conn *sql.DB) { {10020, "an-archive-story", "2011-04-08 15:04:51"}, } for _, row := range rows { + // `categories` is a JSON array, which is what both producers write -- + // the ETL json.dumps its list, and the CMS goes through FormatTags. The + // bare string this fixture used before matched no shape in the corpus + // and quietly exempted itself from the category index. if _, err := conn.ExecContext(ctx, "INSERT INTO articles (id, title, slug, `text`, authors, categories, pub_date, creation_date) VALUES (?, ?, ?, ?, ?, ?, ?, UTC_TIMESTAMP())", - row.id, row.slug, row.slug, "Body", `["Rui Zhao"]`, "News", row.pub, + row.id, row.slug, row.slug, "Body", `["Rui Zhao"]`, `["News"]`, row.pub, ); err != nil { t.Fatalf("seed %s: %v", row.slug, err) } diff --git a/server/internal/handlers/article_patch_integration_test.go b/server/internal/handlers/article_patch_integration_test.go index 2adbbcc..1f5282a 100644 --- a/server/internal/handlers/article_patch_integration_test.go +++ b/server/internal/handlers/article_patch_integration_test.go @@ -107,6 +107,14 @@ func articlePatchTestDB(t *testing.T) *sql.DB { if err := db.EnsureTaxonomyTable(ctx, conn); err != nil { t.Fatalf("ensure taxonomy table: %v", err) } + // The write handlers keep this index in step with `articles`.`categories`, + // so every save touches it. + if _, err := conn.ExecContext(ctx, "DROP TABLE IF EXISTS article_categories"); err != nil { + t.Fatalf("drop article_categories: %v", err) + } + if err := db.EnsureArticleCategoriesTable(ctx, conn); err != nil { + t.Fatalf("ensure article_categories: %v", err) + } return conn } diff --git a/server/internal/handlers/featured_article_integration_test.go b/server/internal/handlers/featured_article_integration_test.go index f9342a4..d73ae40 100644 --- a/server/internal/handlers/featured_article_integration_test.go +++ b/server/internal/handlers/featured_article_integration_test.go @@ -147,6 +147,10 @@ func TestFeaturedArticleHTTP_HomepageLeadsWithTheFeaturedArticle(t *testing.T) { ); err != nil { t.Fatalf("seed articles: %v", err) } + // The homepage blocks are section-filtered, which reads the derived + // category index; these rows were inserted straight into `articles`, so + // nothing has indexed them yet. + indexArticleCategories(t, conn) homepageNewsSlugs := func() []string { rec := httptest.NewRecorder() diff --git a/server/internal/handlers/handlers_test.go b/server/internal/handlers/handlers_test.go index 77a98c2..cb190db 100644 --- a/server/internal/handlers/handlers_test.go +++ b/server/internal/handlers/handlers_test.go @@ -118,13 +118,13 @@ func TestAppendCategorySlugCondition(t *testing.T) { t.Fatalf("expected 3 args, got %d", len(args)) } - // Patterns are anchored on the JSON quotes so a slug matches a whole - // category, never a fragment of a longer one -- see - // db.CategoryMatchPatterns. + // Whole category titles, compared for equality against the + // article_categories index -- so a slug matches a whole category and never + // a fragment of a longer one. See db.CategoryMatchValues. wantArgs := []string{ - `%"comics-puzzles"%`, - `%"comics puzzles"%`, - `%"comics & puzzles"%`, + "comics-puzzles", + "comics puzzles", + "comics & puzzles", } for i, want := range wantArgs { got, ok := args[i].(string) diff --git a/server/internal/handlers/taxonomy_integration_test.go b/server/internal/handlers/taxonomy_integration_test.go index 5856cff..834cb6b 100644 --- a/server/internal/handlers/taxonomy_integration_test.go +++ b/server/internal/handlers/taxonomy_integration_test.go @@ -72,6 +72,16 @@ func taxonomyHTTPTestDB(t *testing.T) *sql.DB { t.Fatalf("create articles: %v", err) } t.Cleanup(func() { _, _ = conn.ExecContext(context.Background(), "DROP TABLE IF EXISTS articles") }) + + // Section matching reads this index rather than the categories column, so a + // harness without it answers every count with an error. + if _, err := conn.ExecContext(ctx, "DROP TABLE IF EXISTS article_categories"); err != nil { + t.Fatalf("drop article_categories: %v", err) + } + if err := db.EnsureArticleCategoriesTable(ctx, conn); err != nil { + t.Fatalf("ensure article_categories: %v", err) + } + t.Cleanup(func() { _, _ = conn.ExecContext(context.Background(), "DROP TABLE IF EXISTS article_categories") }) return conn } @@ -146,8 +156,8 @@ func TestTaxonomyHTTPAliasFixesAnEmptySection(t *testing.T) { } // Nothing matches "Food" yet, because the articles say "Restaurant Reviews". - if got := db.CategoryMatchPatterns("food"); len(got) != 1 { - t.Fatalf("patterns = %v, want only the slug's own", got) + if got := db.CategoryMatchValues("food"); len(got) != 1 { + t.Fatalf("values = %v, want only the slug's own", got) } // The editor saves the real category name. This is the exact payload the @@ -164,16 +174,16 @@ func TestTaxonomyHTTPAliasFixesAnEmptySection(t *testing.T) { // The matcher must see it immediately -- no restart. This is what the // cache refresh on write buys. - patterns := db.CategoryMatchPatterns("food") - for _, want := range []string{`%"restaurant reviews"%`, `%"beer reviews"%`} { + values := db.CategoryMatchValues("food") + for _, want := range []string{"restaurant reviews", "beer reviews"} { found := false - for _, pattern := range patterns { - if pattern == want { + for _, value := range values { + if value == want { found = true } } if !found { - t.Errorf("patterns %v missing %q right after the save", patterns, want) + t.Errorf("values %v missing %q right after the save", values, want) } } @@ -243,8 +253,8 @@ func TestTaxonomyHTTPExplicitEmptyClearsAliases(t *testing.T) { if len(item.CategoryAliases) != 0 { t.Errorf("aliases = %v, want cleared", item.CategoryAliases) } - if got := db.CategoryMatchPatterns("food"); len(got) != 1 { - t.Errorf("patterns = %v, want the alias gone from matching too", got) + if got := db.CategoryMatchValues("food"); len(got) != 1 { + t.Errorf("values = %v, want the alias gone from matching too", got) } } @@ -309,6 +319,7 @@ func TestTaxonomyHTTPDeleteRefusesWhenArticlesExist(t *testing.T) { ); err != nil { t.Fatalf("insert article: %v", err) } + indexArticleCategories(t, conn) var storedCount int64 if err := conn.QueryRowContext(ctx, @@ -372,6 +383,7 @@ func TestTaxonomyHTTPAliasSaveUpdatesTheDisplayedCount(t *testing.T) { ); err != nil { t.Fatalf("insert articles: %v", err) } + indexArticleCategories(t, conn) if code := taxonomyRequest(t, PostTaxonomy(conn), http.MethodPost, "/v1/taxonomy", map[string]any{ "type": "section", @@ -416,6 +428,7 @@ func TestTaxonomyHTTPSubsectionSaveRecountsItsParent(t *testing.T) { ); err != nil { t.Fatalf("insert articles: %v", err) } + indexArticleCategories(t, conn) for _, body := range []map[string]any{ {"type": "section", "slug": "food", "canonical_title": "Food"}, @@ -468,6 +481,7 @@ func TestTaxonomyHTTPDeleteRecountsTheParent(t *testing.T) { ); err != nil { t.Fatalf("insert article: %v", err) } + indexArticleCategories(t, conn) if _, err := conn.ExecContext(ctx, "UPDATE site_taxonomy SET article_count = 99 WHERE slug = 'food'", ); err != nil { @@ -482,3 +496,13 @@ func TestTaxonomyHTTPDeleteRecountsTheParent(t *testing.T) { t.Errorf("parent count after deleting a child = %d, want 1 -- the parent was not recounted", got) } } + +// indexArticleCategories rebuilds the derived category index, standing in for +// the startup rebuild. These tests seed `articles` with raw INSERTs rather than +// through the write handlers, so nothing else would populate it. +func indexArticleCategories(t *testing.T, conn *sql.DB) { + t.Helper() + if err := db.RebuildArticleCategories(context.Background(), conn); err != nil { + t.Fatalf("rebuild article categories: %v", err) + } +}