REFACTOR: Deleted Redundant Repo Interfaces

CLAUDE.md

@@ -19,6 +19,7 @@ deployable.
 ## Commands
 
 ### cms (Go 1.25, module `thamanyah/cms/v2`)
+
 ```bash
 cd cms
 go run .                    # serves on :8081 (requires DB_*/S3_*/MEDIACONVERT_* env vars — see below)
@@ -26,6 +27,7 @@ go run . migrate            # applies pending DB migrations, then exits (no HTTP
 go build ./...
 go vet ./...
 ```
+
 There are no test files in this repo (`cms`, `discovery`, or `infrastructure`) — don't assume a test suite exists.
 
 `cms` is a JSON API only — it serves no HTML and has no static assets. The
@@ -47,12 +49,14 @@ misreports itself as v1.16.4; `go version -m $(go env GOPATH)/bin/swag` gives
 the real one.)
 
 ### infrastructure (Go, Pulumi, module `thamanyah`)
+
 ```bash
 cd infrastructure
 pulumi preview               # plan changes against stack "main"
 pulumi up                    # apply — this touches real AWS resources, confirm with the user first
 pulumi stack output          # e.g. ecsClusterArn, cmsServiceArn
 ```
+
 Deploys run in CI (`.gitea/workflows/infrastructure-deploy.yml`) on push to
 `main` touching `infrastructure/**`. Treat local `pulumi up` as something to
 confirm with the user, not a routine dev command — it mutates shared cloud
@@ -60,18 +64,47 @@ state and Pulumi state isn't safe to update concurrently with CI.
 
 ## Architecture
 
+### cms package layout
+
+Four `internal` packages, split by the kind of thing they hold — keep new code
+on the same seams:
+
+| package | holds |
+|---|---|
+| `internal/api` | the **wire contract**: request/response structs with their JSON + swaggo tags. No logic. |
+| `internal/models` | the **domain types** the rest of the code passes around: `Video`, `Category`, `VideoStatus`. No JSON tags, no SQL. |
+| `internal/handlers` | HTTP: decode `api.*`, validate, call repositories/services, encode `api.*`. |
+| `internal/db` | the `*sql.DB` connection lifecycle and migrations; `internal/db/repositories` holds all SQL. |
+| `internal/services` | **AWS only** — S3 and MediaConvert. Nothing database-related lives here. |
+
+`api` and `models` are deliberately separate types even where the fields look
+alike: `api.CompleteResponse` is the published schema, `models.Video` is the
+row. Handlers translate between them field by field, so renaming a `models`
+field doesn't move the published API and vice versa.
+
 ### cms service
+
 Plain `net/http` (Go 1.22+ pattern-based `ServeMux`), no framework. Entry
-point `cms/main.go` wires up AWS SDK v2 clients (S3, MediaConvert) and a
+point `cms/main.go` builds the dependencies and assigns them to package-level
-Postgres connection from env vars and assigns them to package-level interface
+interface vars that the handlers call through:
-vars in `internal/services` (`services.S3Client`, `services.MediaConvertClient`,
+
-`services.DB`) — handlers call through these interfaces, and
+- `services.S3Client` (`services.S3`) ← `*services.S3Concrete`
-`S3Concrete`/`MediaConvertConcrete`/`DBConcrete` are the only implementations,
+- `services.MediaConvertClient` (`services.MediaConvert`) ← `*services.MediaConvertConcrete`
-which is what makes the handlers testable even though no tests exist yet. On
+- `repositories.VideoRepo` (`repositories.VideoRepository`) ← `*repositories.ConcreteVideoRepository`
-boot, `S3Concrete.AssertSuccessfulConnection` proactively exercises
+- `repositories.CatagoriesRepo` (`repositories.CatagoriesRepository`) ← `*repositories.ConcreteCatagoriesRepository`
-head/put/get/presign against the bucket, and `DBConcrete.AssertSuccessfulConnection`
+
-pings Postgres — both panic on failure rather than letting the service come
+Each interface has exactly one implementation; the indirection is what makes
-up in a broken state.
+the handlers substitutable in tests, even though no tests exist yet. Note the
+spelling: the categories repository is `Catagories`/`CatagoriesRepo` in
+`repositories/catagories.go` (and `handlers.validateCatagoryIDS`) — the
+misspelling is load-bearing for compilation, so match it rather than "fixing"
+it piecemeal.
+
+On boot, `S3Concrete.AssertSuccessfulConnection` proactively exercises
+head-bucket/put/get/presign against the bucket (writing a throwaway
+`.s3-connectivity-check` object), and `db.AssertSuccessfulConnection` pings
+Postgres — both panic on failure rather than letting the service come up in a
+broken state.
 
 `cms/main.go` has two entry paths, dispatched on `os.Args[1]`: the default
 path (`runServer`) boots the HTTP server; `./cms migrate` (`runMigrate`)
@@ -82,6 +115,11 @@ S3/MediaConvert or start the server. This is run as its own ECS container
 before the main container starts (see infrastructure below), so `runServer`
 never runs migrations itself, only `AssertSuccessfulConnection`.
 
+The DB connection is opened once for the process lifetime by
+`db.CreateDBConnection(connString)` and closed by `db.CloseConnection` — both
+plain functions over `*sql.DB` in `internal/db/client.go`, not methods on a
+wrapper type. Repositories take that `*sql.DB` as their `SQLDB` field.
+
 Routes (`cms/main.go`): `GET /health`, `GET /api/categories`,
 `POST /api/videos/presign`, `POST /api/videos`, plus Swagger UI at
 `GET /swagger/` (`/swagger/doc.json` serves the spec). The UI assets are
@@ -90,12 +128,13 @@ nothing is fetched from a CDN at runtime.
 
 Handlers live in `cms/internal/handlers` — `handlers.go` holds `Health` and
 the shared response writers, `videos.go` the categories and upload endpoints.
-The request/response bodies they serve are *not* in that package: every wire
+The wire structs they serve live in `cms/internal/api` (`api.go` for
-struct lives in `cms/internal/api` (`api.go` for `HealthResponse` and
+`HealthResponse` and `ProblemDetails`, `videos.go` for `Category`,
-`ProblemDetails`, `videos.go` for the rest), exported and referenced by the
+`CategoriesResponse`, `PresignRequest`/`PresignResponse` and
-handlers' swaggo annotations as `api.VideoResponse` and so on — so the
+`CompleteRequest`/`CompleteResponse`), referenced by the handlers' swaggo
-generated spec's definition names track that package, and renaming a type
+annotations as `api.CompleteResponse` and so on — so the generated spec's
-there changes the published schema names.
+definition names track that package, and renaming a type there changes the
+published schema names.
 
 There is no view layer: the `internal/views` templ package, the `static/`
 directory, and the htmx frontend were all removed when the service became a
@@ -113,48 +152,55 @@ just the HTTP status phrase — meaningful titles like these are supposed to
 carry a real `type` URI. Adding per-problem type URIs is the conforming fix if
 it ever matters.
 
-### Data model (Postgres, `cms/internal/db/migrations/`)
+### Data model (Postgres, `cms/internal/db/migrations/`, two migrations: `0001`, `0002`)
+
 - `videos` — one row per uploaded video: `title`, `description`, `tags`
   (free-text, comma-separated — not normalized), `file_name`, `storage_key`
   (the S3 key, unique), `mediaconvert_job_id`, `status` (written once as
   `"processing"` on insert — see Known gaps), `size_bytes`, timestamps.
-  `status` is an enum on both sides: the `services.VideoStatus` string type
+  `id` is a `UUID` filled by the column's own `DEFAULT gen_random_uuid()`
-  in `cms/internal/services/status.go` (`processing`, `ready`, `failed`) and
+  (v4) and read back through `RETURNING id` — the application does not
-  the `videos_status_check` CHECK constraint added by migration `0003`.
+  generate it. (An earlier revision generated UUIDv7 application-side; that
-  Extending it means adding a constant there, widening the constraint in a
+  was reverted in `785154c`, so primary keys are random, not time-ordered.)
-  new migration, and extending the `enums` annotation on
+  `status` is a plain `TEXT NOT NULL DEFAULT 'processing'` column with **no
-  `api.VideoResponse.Status` before regenerating the spec.
+  CHECK constraint** — the closed set exists only in Go, as the
-  `id` is a `UUID` generated application-side as a UUIDv7
+  `models.VideoStatus` string type in `cms/internal/models/video.go`
-  (`uuid.NewV7()` in `services.DBConcrete.CreateVideo`) rather than via the
+  (`processing`, `ready`, `failed`). Extending it means adding a constant
-  column's own `DEFAULT gen_random_uuid()` (which generates v4 and is never
+  there and extending the `enums` annotation on `api.CompleteResponse.Status`
-  actually relied on) — v7 keeps primary-key inserts roughly time-ordered,
+  before regenerating the spec. (The doc comment on `VideoStatus` claims a
-  avoiding B-tree fragmentation as the table grows.
+  `videos_status_check` constraint enforces it in the database — that
+  constraint does not exist; nothing has ever created it.)
 - `categories` — a small fixed lookup table (`documentary`, `news`,
   `entertainment`, `podcast`, `other`), seeded by migration `0001`. Its
   `SMALLSERIAL` ids are part of the public API: `GET /api/categories` returns
-  `{id, name}` pairs and `POST /api/videos` takes `categoryIds`, so the seed
+  `{id, name}` pairs (ordered by name, not id) and `POST /api/videos` takes
-  order in migration `0001` is what fixes which id means which name — never
+  `categoryIds`, so the seed order in migration `0001` is what fixes which id
-  renumber it.
+  means which name — never renumber it.
 - `video_categories` — join table (`video_id`, `category_id`, composite PK,
   `ON DELETE CASCADE`) added in migration `0002`: a video can belong to
-  *multiple* categories, not just one. `CreateVideo` inserts the `videos` row
+  *multiple* categories, not just one. `ConcreteVideoRepository.CreateVideo`
-  and its `video_categories` links inside a single transaction, taking the
+  inserts the `videos` row and its `video_categories` links inside a single
-  category ids as given — `handlers.resolveCategoryIDs` is what checks them
+  transaction, taking the category ids as given — `handlers.validateCatagoryIDS`
-  against `ListCategories` (422 on an unknown id) and drops duplicates before
+  is what checks them against `CatagoriesRepo.ListCategoriesIDs` before the
-  the transcode job is queued, leaving the foreign key as a backstop.
+  transcode job is queued, leaving the foreign key and the composite PK as
+  backstops (see Known gaps for how those failures surface).
 
 ### Video upload → transcode pipeline
+
 1. Client calls `POST /api/videos/presign` → cms returns a presigned S3 `PUT`
-   URL for `raw-uploads-bucket`, key `videos/<random-hex>.<ext>`.
+   URL for `raw-uploads-bucket`, key `videos/<random-hex>.<ext>`. The
+   submitted `contentType` (only `video/mp4` or `video/quicktime`) is signed
+   into the URL, so the client's `PUT` must send the identical header.
 2. Client `PUT`s the file directly to S3 (from a browser this requires the
-   bucket's CORS rule, set up in `infrastructure/main.go` — see Known gaps,
+   bucket's CORS rule, set up in `infrastructure/main.go`,
    its allowed origin is now stale). The file never passes through cms.
 3. Client calls `POST /api/videos` with the metadata (categories given as
-   `categoryIds` from `GET /api/categories`) + key → cms calls
+   `categoryIds` from `GET /api/categories`) + key → cms validates the
-   `MediaConvertClient.QueueEncodingJob(key)`, submitting a MediaConvert job
+   category ids, calls `MediaConvertClient.QueueEncodingJob(key)`, submitting
-   `s3://raw-uploads-bucket/<key>` → `s3://encoded-bucket/<key>` (H.264/AAC →
+   a MediaConvert job `s3://raw-uploads-bucket/<key>` →
-   MP4, QVBR rate control — QVBR requires `MaxBitrate` to be set explicitly),
+   `s3://encoded-bucket/<key>` (H.264/AAC → MP4, QVBR rate control — QVBR
-   then `services.DB.CreateVideo` persists the `videos` row (status
+   requires `MaxBitrate` to be set explicitly), then
+   `repositories.VideoRepo.CreateVideo` persists the `videos` row (status
    `"processing"`) and its `video_categories` links.
 4. Finished output lands in `encoded-bucket`, served via CloudFront.
 
@@ -166,6 +212,7 @@ vars in `deployFargateService`, `infrastructure/main.go`) — `main.go` panics
 on boot if any required var is empty.
 
 ### infrastructure (`infrastructure/main.go`, single Pulumi Go program, region `us-east-1`, stack `main`)
+
 - **Postgres**: one shared RDS instance (`db.t3.micro`, single-AZ, no
   backups — intentionally minimal). Each app (`cms`, `discovery`) gets its
   own login role and same-named database via the `postgresql` provider
@@ -218,6 +265,7 @@ on boot if any required var is empty.
     Never broaden this to `ecr:*`/`ecs:*`.
 
 ### CI/CD (`.gitea/workflows/`)
+
 - **`cms-deploy.yml`**: push to `main` touching `cms/**`. Builds/pushes the
   Docker image to ECR using `gitea-ci-user`, installs the AWS CLI (not
   preinstalled on the runner image — via AWS's official install script, not
@@ -233,32 +281,3 @@ on boot if any required var is empty.
 - The runner's `ubuntu-latest` label maps to a docker image configured on
   the runner host (outside this repo) — currently minimal, lacking the AWS
   CLI, hence the manual install step in `cms-deploy.yml`.
-
-## Known gaps
-
-- `cms/internal/services/mediaconvert.go` never calls `DescribeEndpoints`
-  and configures no custom MediaConvert endpoint — relies on the SDK's
-  default regional endpoint.
-- No MediaConvert completion webhook or poller exists — a `videos.status`
-  row is written once as `VideoStatusProcessing` in `CreateVideo` and never
-  updated, even after the transcode job actually finishes or fails. The
-  `ready` and `failed` states are defined but nothing sets them yet.
-- `discovery` has infra provisioned (ECR repo, ECS service, ALB, Postgres
-  DB/role) but no application code — it doesn't touch its database at all.
-- No automated tests exist for `cms`, `discovery`, or `infrastructure`.
-- **No browser client can reach the API yet.** Two separate CORS gaps, both
-  left over from cms dropping its own UI: cms sends no CORS headers of its
-  own, and `raw-uploads-bucket`'s CORS rule in `infrastructure/main.go` still
-  allows only the `cms` ALB origin, so a frontend served from anywhere else
-  fails preflight on the direct S3 `PUT`. Both need the real frontend origin
-  before a browser client works end to end.
-- `videos.size_bytes` is hardcoded to `60` in `CompleteVideoUpload` — the
-  request body carries no size field, so every row stores 60 and the API
-  hands that back in `sizeBytes`. Fixing it means adding `sizeBytes` to the
-  `POST /api/videos` request contract.
-- The generated spec is Swagger 2.0, which has no per-response media type, so
-  error responses are documented as `application/json` even though they are
-  actually sent as `application/problem+json`. The schemas themselves are
-  right. `swag init --v3.1` would emit OpenAPI 3.1 and resolve it.
-- `cms/uploads/` holds a stray `.mov` from an older local-disk upload path.
-  It is untracked, unreferenced by any code, and safe to delete.

cms/internal/db/repositories/catagories.go

@@ -6,18 +6,13 @@ import (
 	"thamanyah/cms/v2/internal/models"
 )
 
-type CatagoriesRepository interface {
+type CatagoriesRepository struct {
-	ListCategories(ctx context.Context) ([]models.Category, error)
+	SQLDB *sql.DB
-	ListCategoriesIDs(ctx context.Context) ([]int16, error)
 }
 
 var CatagoriesRepo CatagoriesRepository
 
-type ConcreteCatagoriesRepository struct {
+func (svc CatagoriesRepository) ListCategories(ctx context.Context) ([]models.Category, error) {
-	SQLDB *sql.DB
-}
-
-func (svc ConcreteCatagoriesRepository) ListCategories(ctx context.Context) ([]models.Category, error) {
 	rows, err := svc.SQLDB.QueryContext(ctx, `SELECT id, name FROM categories ORDER BY name`)
 	if err != nil {
 		return nil, err
@@ -35,7 +30,7 @@ func (svc ConcreteCatagoriesRepository) ListCategories(ctx context.Context) ([]m
 	return categories, rows.Err()
 }
 
-func (svc ConcreteCatagoriesRepository) ListCategoriesIDs(ctx context.Context) ([]int16, error) {
+func (svc CatagoriesRepository) ListCategoriesIDs(ctx context.Context) ([]int16, error) {
 	rows, err := svc.SQLDB.QueryContext(ctx, `SELECT id FROM categories`)
 	if err != nil {
 		return nil, err

cms/internal/db/repositories/videos.go

@@ -8,17 +8,13 @@ import (
 	"thamanyah/cms/v2/internal/models"
 )
 
-type VideoRepository interface {
+type VideoRepository struct {
-	CreateVideo(ctx context.Context, v models.Video) (models.Video, error)
+	SQLDB *sql.DB
 }
 
 var VideoRepo VideoRepository
 
-type ConcreteVideoRepository struct {
+func (svc VideoRepository) CreateVideo(ctx context.Context, v models.Video) (models.Video, error) {
-	SQLDB *sql.DB
-}
-
-func (svc ConcreteVideoRepository) CreateVideo(ctx context.Context, v models.Video) (models.Video, error) {
 	tx, err := svc.SQLDB.BeginTx(ctx, nil)
 	if err != nil {
 		return models.Video{}, err

cms/internal/handlers/videos.go

@@ -10,7 +10,6 @@ import (
 	"net/http"
 	"path/filepath"
 	"slices"
-	"strconv"
 	"strings"
 	"thamanyah/cms/v2/internal/api"
 	"thamanyah/cms/v2/internal/db/repositories"
@@ -21,7 +20,7 @@ import (
 
 const (
 	videoUploadPrefix = "videos"
-	uploadURLExpiry   = 15 * time.Minute
+	uploadURLExpiry   = 60 * time.Minute
 	maxJSONBodySize   = 1 << 20 // 1 MiB
 )
 
@@ -92,7 +91,7 @@ func PresignVideoUpload(w http.ResponseWriter, r *http.Request) {
 	}
 	key := videoUploadPrefix + "/" + filename
 
-	presignedURL, err := services.S3Client.GetPresignedURL(r.Context(), key, req.ContentType, time.Hour)
+	presignedURL, err := services.S3Client.GetPresignedURL(r.Context(), key, req.ContentType, uploadURLExpiry)
 	if err != nil {
 		writeProblem(w, http.StatusInternalServerError, "Could not prepare upload.",
 			"A presigned upload URL could not be issued for the storage bucket. This is a server-side fault; no upload slot was reserved, so retry the request.")
@@ -145,6 +144,13 @@ func CompleteVideoUpload(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	exists := services.S3Client.DoesFileExist(r.Context(), key)
+	if exists {
+		writeProblem(w, http.StatusUnprocessableEntity, "Failed to find video in storage",
+			"The video file that was supposed to be uploaded via the url sent in the PresignVideoUpload endpoint was not uploaded before calling this endpoint")
+		return
+	}
+
 	err := validateCatagoryIDS(r.Context(), req.CategoryIDs)
 	if err != nil {
 		log.Printf("Something Went Wrong Loading Categories: %s", err)
@@ -210,17 +216,6 @@ func validateCatagoryIDS(ctx context.Context, submitted []int16) (err error) {
 	return nil
 }
 
-func formatCategoryIDs(ids []int16) string {
-	formatted := make([]string, 0, len(ids))
-	for _, id := range ids {
-		formatted = append(formatted, strconv.Itoa(int(id)))
-	}
-	if len(formatted) == 1 {
-		return "id " + formatted[0]
-	}
-	return "ids " + strings.Join(formatted, ", ")
-}
-
 func randomFilename(original string) (string, error) {
 	ext := filepath.Ext(filepath.Base(original))
 	buf := make([]byte, 16)

cms/internal/services/s3.go

@@ -14,7 +14,7 @@ var S3Client S3
 
 type S3 interface {
 	GetPresignedURL(ctx context.Context, key, contentType string, expiry time.Duration) (string, error)
-	DoesFileExist(ctx context.Context, key string) (bool, error)
+	DoesFileExist(ctx context.Context, key string) bool
 }
 
 type S3Concrete struct {
@@ -37,17 +37,13 @@ func (svc S3Concrete) GetPresignedURL(ctx context.Context, key, contentType stri
 	return request.URL, nil
 }
 
-func (svc S3Concrete) DoesFileExist(ctx context.Context, key string) (bool, error) {
+func (svc S3Concrete) DoesFileExist(ctx context.Context, key string) bool {
 	_, error := svc.S3Client.HeadObject(ctx, &s3.HeadObjectInput{
 		Bucket: &svc.Bucket,
 		Key:    &key,
 	})
 
-	if error != nil {
+	return error != nil
-		return false, error
-	}
-
-	return true, nil
 }
 
 // AssertSuccessfulConnection verifies that the bucket is reachable and that

cms/main.go

@@ -84,10 +84,12 @@ func runServer() {
 	concreteDBClient := db.CreateDBConnection(requireDBConnectionString())
 	defer db.CloseConnection(concreteDBClient)
 	db.AssertSuccessfulConnection(context.Background(), concreteDBClient)
-	repositories.VideoRepo = &repositories.ConcreteVideoRepository{
+
+	repositories.VideoRepo = repositories.VideoRepository{
 		SQLDB: concreteDBClient,
 	}
-	repositories.CatagoriesRepo = &repositories.ConcreteCatagoriesRepository{
+
+	repositories.CatagoriesRepo = repositories.CatagoriesRepository{
 		SQLDB: concreteDBClient,
 	}