fix(playlist): create a smart playlist through the writer
`CreateSmartPlaylist` issued its `INSERT ... RETURNING` through `QueryContext`, which routes to the query-only read pool, and failed with "attempt to write a readonly database (8)". No smart playlist could be created at all, in any real build. It was invisible because `NewTestDB` shares one in-memory connection and leaves `readDB` nil, so `reader()` hands back the *writer* under test: every unit test of that path exercised a handle production does not have. `TestNoWritesOnTheReadPool` walks the tree for the whole class, in the same spirit as `TestNoDirectRuntimeEmits` and for the same reason — a lint pass only sees one build configuration.
This commit is contained in:
@@ -0,0 +1,109 @@
|
||||
package database_test
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"regexp"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// writeVerb matches the first SQL keyword of a statement that mutates.
|
||||
// Anchored to the start of the trimmed line, because a subquery or a
|
||||
// column named "update" is not a write.
|
||||
var writeVerb = regexp.MustCompile(
|
||||
`^\s*` + "`" + `?\s*(?i:INSERT|UPDATE|DELETE|REPLACE|CREATE|DROP|ALTER)\s`,
|
||||
)
|
||||
|
||||
// queryCall matches a call to one of the read-pool helpers. These
|
||||
// route to DB.reader(), which in a real app is a second sql.DB opened
|
||||
// query-only over the same file.
|
||||
var queryCall = regexp.MustCompile(
|
||||
`\.Query(?:Context|ContextWith|Row)\s*\(`,
|
||||
)
|
||||
|
||||
// TestNoWritesOnTheReadPool fails if a mutating statement is issued
|
||||
// through one of the query-only read helpers.
|
||||
//
|
||||
// This is worth a test of its own because the failure mode is invisible
|
||||
// to every other tier. `CreateSmartPlaylist` ran an
|
||||
// `INSERT ... RETURNING` through `QueryContext` — a write wearing a
|
||||
// query's shape — and failed at runtime with "attempt to write a
|
||||
// readonly database (8)", i.e. no smart playlist could be created at
|
||||
// all. Nothing caught it: `NewTestDB` shares one in-memory connection
|
||||
// and sets `readDB` to nil, so `reader()` returns the *writer* there
|
||||
// and every unit test of that path passed against a handle the app does
|
||||
// not have.
|
||||
//
|
||||
// A text walk rather than a lint rule, for the same reason as
|
||||
// TestNoDirectRuntimeEmits: golangci-lint runs once per build
|
||||
// configuration and would not see a call in a tagged file.
|
||||
func TestNoWritesOnTheReadPool(t *testing.T) {
|
||||
root := filepath.Join("..", "..")
|
||||
|
||||
skipDirs := map[string]bool{
|
||||
".git": true,
|
||||
"node_modules": true,
|
||||
"frontend": true,
|
||||
"build": true,
|
||||
".dev": true,
|
||||
}
|
||||
|
||||
err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if d.IsDir() {
|
||||
if skipDirs[d.Name()] {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
if filepath.Ext(path) != ".go" ||
|
||||
strings.HasSuffix(path, "_test.go") {
|
||||
return nil
|
||||
}
|
||||
|
||||
rel, relErr := filepath.Rel(root, path)
|
||||
if relErr != nil {
|
||||
return relErr
|
||||
}
|
||||
|
||||
src, readErr := os.ReadFile(path)
|
||||
if readErr != nil {
|
||||
return readErr
|
||||
}
|
||||
|
||||
lines := strings.Split(string(src), "\n")
|
||||
|
||||
for i, line := range lines {
|
||||
if !queryCall.MatchString(line) ||
|
||||
strings.Contains(line, "QueryRowWriter") {
|
||||
continue
|
||||
}
|
||||
|
||||
// The statement is usually on the following line, in a raw
|
||||
// string literal. Look a little way ahead rather than only
|
||||
// at the call itself.
|
||||
for j := i; j < min(i+3, len(lines)); j++ {
|
||||
if writeVerb.MatchString(lines[j]) {
|
||||
t.Errorf(
|
||||
"%s:%d issues a write through a read-pool helper; "+
|
||||
"use ExecContext or QueryRowWriter\n\t%s",
|
||||
rel, j+1, strings.TrimSpace(lines[j]),
|
||||
)
|
||||
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("walking %s: %v", root, err)
|
||||
}
|
||||
}
|
||||
@@ -2565,35 +2565,6 @@ func (s *Service) CreateSmartPlaylist(
|
||||
)
|
||||
}
|
||||
|
||||
// SAFETY: Hand-crafted INSERT for smart playlist with
|
||||
// is_smart and smart_rules columns not yet in sqlc schema.
|
||||
// All values are parameterized.
|
||||
rows, err := s.db.QueryContext(
|
||||
`INSERT INTO playlists (name, is_smart, smart_rules)
|
||||
VALUES (?, 1, ?)
|
||||
RETURNING id, name, created_at, updated_at`,
|
||||
trimmed, rulesJSON,
|
||||
)
|
||||
if err != nil {
|
||||
s.logger.Error(
|
||||
"Failed to create smart playlist",
|
||||
"name", trimmed, "err", err,
|
||||
)
|
||||
|
||||
return Summary{}, fmt.Errorf(
|
||||
"failed to create smart playlist: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
if !rows.Next() {
|
||||
_ = rows.Close()
|
||||
|
||||
return Summary{}, fmt.Errorf(
|
||||
"failed to create smart playlist: %w",
|
||||
errNoRowReturned,
|
||||
)
|
||||
}
|
||||
|
||||
var (
|
||||
id int64
|
||||
retName string
|
||||
@@ -2601,25 +2572,38 @@ func (s *Service) CreateSmartPlaylist(
|
||||
updatedAt string
|
||||
)
|
||||
|
||||
if err := rows.Scan(
|
||||
&id, &retName, &createdAt, &updatedAt,
|
||||
); err != nil {
|
||||
_ = rows.Close()
|
||||
|
||||
// SAFETY: Hand-crafted INSERT for smart playlist with
|
||||
// is_smart and smart_rules columns not yet in sqlc schema.
|
||||
// All values are parameterized.
|
||||
//
|
||||
// QueryRowWriter, not QueryContext: this is an INSERT wearing a
|
||||
// query's shape, and QueryContext routes to the query-only read
|
||||
// pool. Through that handle it failed with "attempt to write a
|
||||
// readonly database", i.e. no smart playlist could be created at
|
||||
// all. A RETURNING clause does not make a write a read.
|
||||
if err := s.db.QueryRowWriter(
|
||||
`INSERT INTO playlists (name, is_smart, smart_rules)
|
||||
VALUES (?, 1, ?)
|
||||
RETURNING id, name, created_at, updated_at`,
|
||||
trimmed, rulesJSON,
|
||||
).Scan(&id, &retName, &createdAt, &updatedAt); err != nil {
|
||||
s.logger.Error(
|
||||
"Failed to create smart playlist",
|
||||
"name", trimmed, "err", err,
|
||||
)
|
||||
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
return Summary{}, fmt.Errorf(
|
||||
"failed to create smart playlist: %w",
|
||||
errNoRowReturned,
|
||||
)
|
||||
}
|
||||
|
||||
return Summary{}, fmt.Errorf(
|
||||
"failed to create smart playlist: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
// Close before RefreshSmartPlaylist issues its own queries
|
||||
// (MaxOpenConns=1 test DBs would deadlock).
|
||||
_ = rows.Close()
|
||||
|
||||
s.logger.Info(
|
||||
"Smart playlist created",
|
||||
"id", id, "name", retName,
|
||||
|
||||
Reference in New Issue
Block a user