diff --git a/server/cmd/backfill-excerpts/main.go b/server/cmd/backfill-excerpts/main.go new file mode 100644 index 0000000..5e58a87 --- /dev/null +++ b/server/cmd/backfill-excerpts/main.go @@ -0,0 +1,156 @@ +// Command backfill-excerpts repairs articles whose `excerpt` column was blanked +// by a save. +// +// The excerpt box is optional and the editor sends it on every save, so for a +// while PUT and PATCH stored the blank verbatim while POST derived one from the +// body. An article therefore got an excerpt on create and lost it on the first +// edit afterward. Listings select `excerpt` and never the body, so a blank +// column is a story that appears on the section pages and the homepage with no +// summary under it. +// +// The write paths now derive an excerpt from the body whenever the field +// arrives blank; this repairs the rows blanked before that. It derives through +// exactly the same db.ExcerptOrDerived the server uses, so a repaired row is +// byte-identical to what a save would write today. +// +// It reads DB_* from the environment, like the server, so it can be run inside +// the backend service's own environment rather than with credentials copied by +// hand. Dry run by default: pass -apply to write. +package main + +import ( + "context" + "database/sql" + "flag" + "fmt" + "log" + "os" + "strconv" + "strings" + "time" + + _ "github.com/go-sql-driver/mysql" + + db "server/internal/database" +) + +func main() { + apply := flag.Bool("apply", false, "write the derived excerpts; without it, report what would change") + limit := flag.Int("limit", 0, "stop after this many articles (0 = all)") + verbose := flag.Bool("verbose", false, "list every article, not just the first few of each kind") + flag.Parse() + + ctx := context.Background() + conn, err := connect(ctx) + if err != nil { + log.Fatalf("connect: %v", err) + } + defer conn.Close() + + // Read every blank row up front rather than streaming: the UPDATEs below run + // against the same table the cursor is walking, and the whole set is a few + // hundred rows. + type article struct { + id int64 + slug string + content string + } + query := "SELECT `id`, `slug`, COALESCE(`text`, '') FROM `articles` WHERE TRIM(COALESCE(`excerpt`, '')) = '' ORDER BY `id`" + if *limit > 0 { + query += " LIMIT " + strconv.Itoa(*limit) + } + rows, err := conn.QueryContext(ctx, query) + if err != nil { + log.Fatalf("select blank excerpts: %v", err) + } + var blank []article + for rows.Next() { + var a article + if err := rows.Scan(&a.id, &a.slug, &a.content); err != nil { + rows.Close() + log.Fatalf("scan: %v", err) + } + blank = append(blank, a) + } + if err := rows.Err(); err != nil { + rows.Close() + log.Fatalf("iterate: %v", err) + } + rows.Close() + + fmt.Printf("%d articles have a blank excerpt\n", len(blank)) + + var repaired, empty int + for _, a := range blank { + derived := db.ExcerptOrDerived("", a.content) + if strings.TrimSpace(derived) == "" { + // Nothing to derive from -- a body that is a bare shortcode, an + // image-only post, or a genuinely empty draft. Leave it alone: an + // empty excerpt is what it had, and there is no text to improve on. + if *verbose || empty < 10 { + fmt.Printf(" [skip] %s (body %d bytes: %s)\n", a.slug, len(a.content), truncate(strings.Join(strings.Fields(a.content), " "), 70)) + } + empty++ + continue + } + if !*apply { + if *verbose || repaired < 10 { + fmt.Printf(" %s -> %s\n", a.slug, truncate(derived, 80)) + } + repaired++ + continue + } + // mod_date is deliberately left alone. This is a repair of a value the + // CMS should have written at the time, not an edit: touching mod_date + // would move every repaired article's sitemap lastmod to today and queue + // the whole corpus for re-embedding. + if _, err := conn.ExecContext(ctx, + "UPDATE `articles` SET `excerpt` = ? WHERE `id` = ? AND TRIM(COALESCE(`excerpt`, '')) = ''", + derived, a.id, + ); err != nil { + log.Fatalf("update %s: %v", a.slug, err) + } + repaired++ + } + + verb := "would repair" + if *apply { + verb = "repaired" + } + fmt.Printf("%s %d articles; %d left alone (no text to derive from)\n", verb, repaired, empty) +} + +func connect(ctx context.Context) (*sql.DB, error) { + name := strings.TrimSpace(os.Getenv("DB_NAME")) + user := strings.TrimSpace(os.Getenv("DB_USER")) + password := os.Getenv("DB_PASSWORD") + host := strings.TrimSpace(os.Getenv("DB_HOST")) + if name == "" || user == "" { + return nil, fmt.Errorf("DB_NAME and DB_USER are required") + } + if host == "" { + host = "127.0.0.1" + } + port := 3306 + if raw := strings.TrimSpace(os.Getenv("DB_PORT")); raw != "" { + parsed, err := strconv.Atoi(raw) + if err != nil { + return nil, fmt.Errorf("DB_PORT %q: %w", raw, err) + } + port = parsed + } + // A one-off maintenance run has no business taking the server's share of a + // 200-connection budget, and it is single-threaded anyway. + os.Setenv("DB_MAX_OPEN_CONNS", "2") + + timeoutCtx, cancel := context.WithTimeout(ctx, 15*time.Second) + defer cancel() + return db.InitializeConnection(timeoutCtx, name, user, password, host, port) +} + +func truncate(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] + "..." +} diff --git a/server/internal/database/database.go b/server/internal/database/database.go index 794238d..f5b92d4 100644 --- a/server/internal/database/database.go +++ b/server/internal/database/database.go @@ -13,8 +13,22 @@ import ( _ "github.com/go-sql-driver/mysql" ) +// clientFoundRows makes an UPDATE report the rows it matched rather than the +// rows whose values it changed. +// +// Every handler that reads RowsAffected treats zero as "no such row" and +// answers 404 -- author update, patch, archive and restore, and both article +// write paths. Without this flag a save that sets every column to what it +// already holds matches its row, changes nothing, reports zero, and is answered +// as though the article had been deleted mid-edit. The editor sends the whole +// form on every save and autosaves on a timer, so two saves inside the same +// second -- a double-clicked Save, or an autosave landing on an untouched form +// after mod_date has already been stamped to the current second -- are enough. +// +// No call site uses RowsAffected to ask whether anything changed, so matched +// rows is what all six of them already mean. func buildDSN(dbName, user, password, host string, port int) string { - return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?parseTime=true", user, password, host, port, dbName) + return fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?parseTime=true&clientFoundRows=true", user, password, host, port, dbName) } // defaultMaxOpenConns is the size of the connection pool. diff --git a/server/internal/database/database_test.go b/server/internal/database/database_test.go index 636bab8..233d35e 100644 --- a/server/internal/database/database_test.go +++ b/server/internal/database/database_test.go @@ -1 +1,22 @@ package database + +import ( + "strings" + "testing" +) + +// Every handler that reads RowsAffected treats zero as "no such row" and answers +// 404. Without clientFoundRows an UPDATE that matches its row but changes none of +// its values reports zero, so a save that happened to change nothing is answered +// as though the article had been deleted mid-edit. +func TestBuildDSN_RequestsFoundRowsSemantics(t *testing.T) { + dsn := buildDSN("cms", "user", "pw", "db1.internal", 3306) + if !strings.Contains(dsn, "clientFoundRows=true") { + t.Errorf("DSN = %q, want clientFoundRows=true", dsn) + } + // parseTime drives every DATETIME column into time.Time; losing it would turn + // every date in the API into a []byte. + if !strings.Contains(dsn, "parseTime=true") { + t.Errorf("DSN = %q, want parseTime=true kept", dsn) + } +} diff --git a/server/internal/database/http_models.go b/server/internal/database/http_models.go index e396c54..c63a551 100644 --- a/server/internal/database/http_models.go +++ b/server/internal/database/http_models.go @@ -634,6 +634,21 @@ func ReplaceArticleAuthorsBySlug(ctx context.Context, conn *sql.DB, slug string, return ReplaceArticleAuthors(ctx, conn, articleID, authorIDs) } +// GetArticleContentBySlug reads just the body, for a patch that has to derive +// an excerpt from a body it did not carry. A missing article is "" and no +// error: the UPDATE that follows reports the 404 on its own row count. +func GetArticleContentBySlug(ctx context.Context, conn *sql.DB, slug string) (string, error) { + var text sql.NullString + err := conn.QueryRowContext(ctx, "SELECT `text` FROM `articles` WHERE `slug` = ?", strings.TrimSpace(slug)).Scan(&text) + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + if err != nil { + return "", err + } + return text.String, nil +} + func ParsePublishedAt(value string) *time.Time { if strings.TrimSpace(value) == "" { return nil @@ -751,6 +766,19 @@ func deriveExcerpt(content string) string { return strings.Join(words, " ") } +// ExcerptOrDerived is what every write that sets the `excerpt` column stores. +// An excerpt is optional in the editor, so a blank one means "derive it from +// the body", never "store nothing": listings render `excerpt` and nothing else +// -- the body is not selected for them -- so a stored empty string is an +// article that appears on every section page and the homepage with no summary +// under it. +func ExcerptOrDerived(excerpt, content string) string { + if trimmed := strings.TrimSpace(excerpt); trimmed != "" { + return trimmed + } + return deriveExcerpt(content) +} + func normalizeSlug(value string) string { s := strings.ToLower(strings.TrimSpace(value)) if s == "" { @@ -792,10 +820,7 @@ func ArticleInputToDBFields(body models.ArticleInput) []any { slug = normalizeSlug(body.Title) } - excerpt := strings.TrimSpace(body.Excerpt) - if excerpt == "" { - excerpt = deriveExcerpt(body.Content) - } + excerpt := ExcerptOrDerived(body.Excerpt, body.Content) tags := FormatTags(body.Tags) if tags == "" { @@ -858,7 +883,7 @@ func ArticleToDBFields(body models.Article) []any { return []any{ body.Title, normalizeSlug(body.Slug), - body.Excerpt, + ExcerptOrDerived(body.Excerpt, body.Content), body.Content, FormatTags(body.Categories), publishedAt, diff --git a/server/internal/database/http_models_test.go b/server/internal/database/http_models_test.go index 89a009c..cc48b52 100644 --- a/server/internal/database/http_models_test.go +++ b/server/internal/database/http_models_test.go @@ -182,6 +182,44 @@ func TestArticleToDBFields_StampsModifiedDate(t *testing.T) { } } +// The excerpt field is optional in the editor, and a PUT sends it blank when the +// author never filled it in. Storing that blank left the article with no summary +// on the section pages and the homepage, which render `excerpt` and never the +// body -- POST had derived one from the body all along, and a full replacement +// has the same body to derive from. +func TestArticleToDBFields_DerivesExcerptFromBodyWhenBlank(t *testing.T) { + fields := ArticleToDBFields(models.Article{ + Title: "Phillies reinforce team at the trade deadline", + Slug: "phillies-reinforce-team-at-the-trade-deadline", + Excerpt: " ", + Content: "

The Phillies added two arms before Thursday's deadline.

", + }) + + // Column order is title, slug, excerpt, text. + const excerptFieldIndex = 2 + got, ok := fields[excerptFieldIndex].(string) + if !ok { + t.Fatalf("excerpt field has type %T, want string", fields[excerptFieldIndex]) + } + if got != "The Phillies added two arms before Thursday's deadline." { + t.Errorf("excerpt = %q, want it derived from the body", got) + } +} + +func TestArticleToDBFields_KeepsSuppliedExcerpt(t *testing.T) { + fields := ArticleToDBFields(models.Article{ + Title: "Phillies reinforce team at the trade deadline", + Slug: "phillies-reinforce-team-at-the-trade-deadline", + Excerpt: " An editor's own summary. ", + Content: "

The Phillies added two arms before Thursday's deadline.

", + }) + + const excerptFieldIndex = 2 + if got := fields[excerptFieldIndex]; got != "An editor's own summary." { + t.Errorf("excerpt = %q, want the supplied text", got) + } +} + // An article normally opens with its featured image, so an excerpt derived by // stripping tags used to start with the photo credit in the caption instead of // the story. diff --git a/server/internal/handlers/article_patch_integration_test.go b/server/internal/handlers/article_patch_integration_test.go index 1f5282a..ba7302d 100644 --- a/server/internal/handlers/article_patch_integration_test.go +++ b/server/internal/handlers/article_patch_integration_test.go @@ -25,6 +25,17 @@ func articlePatchTestDB(t *testing.T) *sql.DB { if dsn == "" { t.Skip("CMS_TEST_DSN not set; skipping article patch integration test") } + // The server connects with clientFoundRows, and the handlers' 404-on-zero-rows + // checks only behave correctly under it. A test DSN without the flag would + // exercise semantics production never runs with, so add it rather than asking + // whoever sets the variable to remember. + if !strings.Contains(dsn, "clientFoundRows") { + separator := "?" + if strings.Contains(dsn, "?") { + separator = "&" + } + dsn += separator + "clientFoundRows=true" + } conn, err := sql.Open("mysql", dsn) if err != nil { @@ -363,6 +374,116 @@ func TestArticlePatchHTTP_PhotoAltRoundTrip(t *testing.T) { } } +// The excerpt box is optional, and the editor sends it on every save whether the +// author filled it in or not. Storing the blank verbatim published articles with +// no summary anywhere they are listed -- listings render `excerpt` and never the +// body -- so a blank means "derive one", the same fallback a POST applies. +func TestArticlePatchHTTP_BlankExcerptIsDerivedFromBody(t *testing.T) { + conn := articlePatchTestDB(t) + if _, err := conn.ExecContext(context.Background(), + "INSERT INTO articles (title, slug, `text`, excerpt, categories, pub_date) VALUES (?, ?, ?, ?, ?, UTC_TIMESTAMP())", + "Phillies reinforce team", "phillies-reinforce-team", + "

The Phillies added two arms before the deadline.

", "An older summary.", "Sports", + ); err != nil { + t.Fatalf("seed article: %v", err) + } + + // A second article, so the excerpt-only case below patches a row this test + // has not already written: an UPDATE that sets every column to what it + // already holds affects no rows, which the handler reports as a 404. + if _, err := conn.ExecContext(context.Background(), + "INSERT INTO articles (title, slug, `text`, excerpt, categories, pub_date) VALUES (?, ?, ?, ?, ?, UTC_TIMESTAMP())", + "FIFA experiences financial fiascos", "fifa-financial-fiascos", + "

FIFA closed the year short of its own projections.

", "A stale summary.", "Sports", + ); err != nil { + t.Fatalf("seed second article: %v", err) + } + + readExcerpt := func(slug string) string { + t.Helper() + var excerpt sql.NullString + if err := conn.QueryRowContext(context.Background(), + "SELECT excerpt FROM articles WHERE slug = ?", slug, + ).Scan(&excerpt); err != nil { + t.Fatalf("read excerpt: %v", err) + } + return excerpt.String + } + + // A save that carries a new body derives from that body, not the stored one. + rec := httptest.NewRecorder() + body := `{"title":"Phillies reinforce team","excerpt":"","content":"

The Phillies added two arms and a bat.

"}` + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("phillies-reinforce-team", body)) + if rec.Code != http.StatusNoContent { + t.Fatalf("patch status = %d, want 204; body = %s", rec.Code, rec.Body.String()) + } + if got := readExcerpt("phillies-reinforce-team"); got != "The Phillies added two arms and a bat." { + t.Fatalf("excerpt = %q, want it derived from the patched body", got) + } + + // An excerpt-only patch has no body of its own, so it derives from the stored + // one rather than falling back to empty. + rec = httptest.NewRecorder() + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("fifa-financial-fiascos", `{"excerpt":" "}`)) + if rec.Code != http.StatusNoContent { + t.Fatalf("excerpt-only patch status = %d, want 204; body = %s", rec.Code, rec.Body.String()) + } + if got := readExcerpt("fifa-financial-fiascos"); got != "FIFA closed the year short of its own projections." { + t.Fatalf("excerpt = %q, want it derived from the stored body", got) + } + + // A supplied excerpt still wins over anything derivable. + rec = httptest.NewRecorder() + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("phillies-reinforce-team", `{"excerpt":" An editor's own summary. "}`)) + if rec.Code != http.StatusNoContent { + t.Fatalf("supplied-excerpt patch status = %d, want 204; body = %s", rec.Code, rec.Body.String()) + } + if got := readExcerpt("phillies-reinforce-team"); got != "An editor's own summary." { + t.Fatalf("excerpt = %q, want the supplied text", got) + } + + // A non-string is a client bug, not an excerpt. + rec = httptest.NewRecorder() + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("phillies-reinforce-team", `{"excerpt":42}`)) + if rec.Code != http.StatusBadRequest { + t.Fatalf("numeric excerpt status = %d, want 400; body = %s", rec.Code, rec.Body.String()) + } +} + +// A save that changes nothing still matched its row, so it is a success. It used +// to be answered 404 -- the UPDATE changed no values, MySQL reported zero rows +// affected, and the handler reads zero as "no such article". The editor sends the +// whole form on every save and autosaves on a timer, so two saves inside the same +// second are enough to hit it: mod_date is stamped to the second, so the second +// save rewrites every column to what it already holds. To an author, an article +// they are editing reports that it does not exist. +func TestArticlePatchHTTP_NoOpSaveIsNotAMissingArticle(t *testing.T) { + conn := articlePatchTestDB(t) + if _, err := conn.ExecContext(context.Background(), + "INSERT INTO articles (title, slug, `text`, excerpt, categories, pub_date) VALUES (?, ?, ?, ?, ?, UTC_TIMESTAMP())", + "Unchanged story", "unchanged-story", "

Body.

", "A summary.", "News", + ); err != nil { + t.Fatalf("seed article: %v", err) + } + + body := `{"title":"Unchanged story","excerpt":"A summary.","content":"

Body.

","comment_status":"open"}` + for attempt := 1; attempt <= 2; attempt++ { + rec := httptest.NewRecorder() + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("unchanged-story", body)) + if rec.Code != http.StatusNoContent { + t.Fatalf("save %d status = %d, want 204; body = %s", attempt, rec.Code, rec.Body.String()) + } + } + + // A slug that genuinely is not there must still be a 404 -- the fix must not + // turn "no such article" into a silent success. + rec := httptest.NewRecorder() + PatchArticle(conn).ServeHTTP(rec, patchArticleRequest("no-such-story", body)) + if rec.Code != http.StatusNotFound { + t.Fatalf("missing-article status = %d, want 404; body = %s", rec.Code, rec.Body.String()) + } +} + // A noindexed article must drop out of the sitemap too: listing a URL whose page // says noindex is a contradiction search engines report as an error. func TestSitemapSlugsOmitsNoIndexedArticles(t *testing.T) { diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 2319349..bd155ab 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -2474,6 +2474,30 @@ func PatchArticle(conn *sql.DB) http.HandlerFunc { } setCols = append(setCols, column) setArgs = append(setArgs, strings.TrimSpace(s)) + case "excerpt": + s, ok := v.(string) + if !ok { + writeError(w, http.StatusBadRequest, "excerpt must be a string") + return + } + // The editor sends the excerpt field on every save, blank or not, + // so a patch that clears it is the ordinary case of an author who + // never filled it in -- not a request for an article with no + // summary anywhere it is listed. Derive one from the body, the + // same fallback POST applies. The patch's own content wins if it + // carries one; the map is iterated in random order, so read it + // from the body rather than from whatever the loop has processed. + content, _ := body["content"].(string) + if strings.TrimSpace(content) == "" { + stored, err := db.GetArticleContentBySlug(r.Context(), conn, slug) + if err != nil { + writeError(w, http.StatusInternalServerError, err.Error()) + return + } + content = stored + } + setCols = append(setCols, column) + setArgs = append(setArgs, db.ExcerptOrDerived(s, content)) case "canonical_url": s, ok := v.(string) if !ok {