diff --git a/CLAUDE.md b/CLAUDE.md index 5818b32..b9d0952 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,8 +63,8 @@ Black-box BDD covering the video upload feature: its own Go module importing nothing from `cms/`, talking to a running service over HTTP only, so the same scenarios run against compose and against a deployed environment. Skips (does not fail) when nothing is serving. Uploading is a plain `PUT` to the presigned -URL, no AWS SDK. Scenarios tagged `@known-gap` deliberately pin current -behaviour that differs from the documented contract — see `tests/README.md`. +URL, no AWS SDK. Scenarios can be tagged `@known-gap` to pin current behaviour +that differs from the documented contract — see `tests/README.md`. Note compose starts `cms` in server mode only: the migrate container exists in the ECS task definition, not in `docker-compose.yml`, so a freshly created diff --git a/cms/docs/docs.go b/cms/docs/docs.go index 2512bb4..4ed94c0 100644 --- a/cms/docs/docs.go +++ b/cms/docs/docs.go @@ -84,6 +84,12 @@ const docTemplate = `{ "$ref": "#/definitions/api.ProblemDetails" } }, + "409": { + "description": "key has already been registered by an earlier request", + "schema": { + "$ref": "#/definitions/api.ProblemDetails" + } + }, "422": { "description": "title was empty, key was missing or not a key issued by this API, or categoryIds was empty", "schema": { @@ -91,7 +97,7 @@ const docTemplate = `{ } }, "500": { - "description": "Categories could not be read, the transcoding job could not be queued, or the video record could not be saved", + "description": "Categories could not be read, the key could not be checked, the transcoding job could not be queued, or the video record could not be saved", "schema": { "$ref": "#/definitions/api.ProblemDetails" } diff --git a/cms/docs/swagger.json b/cms/docs/swagger.json index bf5dd1e..c00317e 100644 --- a/cms/docs/swagger.json +++ b/cms/docs/swagger.json @@ -77,6 +77,12 @@ "$ref": "#/definitions/api.ProblemDetails" } }, + "409": { + "description": "key has already been registered by an earlier request", + "schema": { + "$ref": "#/definitions/api.ProblemDetails" + } + }, "422": { "description": "title was empty, key was missing or not a key issued by this API, or categoryIds was empty", "schema": { @@ -84,7 +90,7 @@ } }, "500": { - "description": "Categories could not be read, the transcoding job could not be queued, or the video record could not be saved", + "description": "Categories could not be read, the key could not be checked, the transcoding job could not be queued, or the video record could not be saved", "schema": { "$ref": "#/definitions/api.ProblemDetails" } diff --git a/cms/docs/swagger.yaml b/cms/docs/swagger.yaml index 7dd67fd..36ebfd4 100644 --- a/cms/docs/swagger.yaml +++ b/cms/docs/swagger.yaml @@ -190,14 +190,19 @@ paths: description: categoryIds referenced a category that does not exist schema: $ref: '#/definitions/api.ProblemDetails' + "409": + description: key has already been registered by an earlier request + schema: + $ref: '#/definitions/api.ProblemDetails' "422": description: title was empty, key was missing or not a key issued by this API, or categoryIds was empty schema: $ref: '#/definitions/api.ProblemDetails' "500": - description: Categories could not be read, the transcoding job could not - be queued, or the video record could not be saved + description: Categories could not be read, the key could not be checked, + the transcoding job could not be queued, or the video record could not + be saved schema: $ref: '#/definitions/api.ProblemDetails' summary: Register an uploaded video diff --git a/cms/internal/db/repositories/videos.go b/cms/internal/db/repositories/videos.go index 4f4101f..3ed9e4d 100644 --- a/cms/internal/db/repositories/videos.go +++ b/cms/internal/db/repositories/videos.go @@ -14,6 +14,19 @@ type VideoRepository struct { var VideoRepo VideoRepository +// VideoExistsWithStorageKey reports whether a video has already been +// registered under a storage key. It is served by videos_storage_key_key, the +// index Postgres builds for the column's UNIQUE constraint. +func (svc VideoRepository) VideoExistsWithStorageKey(ctx context.Context, storageKey string) (bool, error) { + var exists bool + err := svc.SQLDB.QueryRowContext(ctx, + `SELECT EXISTS (SELECT 1 FROM videos WHERE storage_key = $1)`, storageKey).Scan(&exists) + if err != nil { + return false, err + } + return exists, nil +} + func (svc VideoRepository) CreateVideo(ctx context.Context, v models.Video) (models.Video, error) { tx, err := svc.SQLDB.BeginTx(ctx, nil) if err != nil { diff --git a/cms/internal/handlers/videos.go b/cms/internal/handlers/videos.go index 1c16b7f..de9b433 100644 --- a/cms/internal/handlers/videos.go +++ b/cms/internal/handlers/videos.go @@ -113,8 +113,9 @@ func PresignVideoUpload(w http.ResponseWriter, r *http.Request) { // @Success 201 {object} api.CompleteResponse // @Failure 400 {object} api.ProblemDetails "Request body was not valid JSON" // @Failure 404 {object} api.ProblemDetails "categoryIds referenced a category that does not exist" +// @Failure 409 {object} api.ProblemDetails "key has already been registered by an earlier request" // @Failure 422 {object} api.ProblemDetails "title was empty, key was missing or not a key issued by this API, or categoryIds was empty" -// @Failure 500 {object} api.ProblemDetails "Categories could not be read, the transcoding job could not be queued, or the video record could not be saved" +// @Failure 500 {object} api.ProblemDetails "Categories could not be read, the key could not be checked, the transcoding job could not be queued, or the video record could not be saved" // @Router /api/videos [post] func CompleteVideoUpload(w http.ResponseWriter, r *http.Request) { r.Body = http.MaxBytesReader(w, r.Body, maxJSONBodySize) @@ -166,6 +167,24 @@ func CompleteVideoUpload(w http.ResponseWriter, r *http.Request) { return } + // Ahead of queueing, because a key that is already registered would + // otherwise get a second transcoding job before the insert failed on the + // storage_key UNIQUE constraint. That constraint is still the backstop: + // this check and the insert are not one atomic step, so two concurrent + // requests carrying the same key can both get past here. + alreadyRegistered, err := repositories.VideoRepo.VideoExistsWithStorageKey(r.Context(), key) + if err != nil { + log.Printf("Something Went Wrong Checking Whether The Video Was Already Registered: %s", err) + writeProblem(w, http.StatusInternalServerError, "Something Went Wrong Checking Whether The Video Was Already Registered", + "The submitted key could not be checked against the existing video records, so no video record was created and no transcoding job was queued. This is a server-side fault; the uploaded file is still in storage, so retry this request with the same 'key'.") + return + } + if alreadyRegistered { + writeProblem(w, http.StatusConflict, "Video Already Registered", + fmt.Sprintf("The 'key' %q has already been registered, so a video record and a transcoding job exist for that file. Registering it a second time would queue a duplicate job. If you meant to add a different video, request a fresh key from POST /api/videos/presign and upload the file to it.", key)) + return + } + jobID, err := services.MediaConvertClient.QueueEncodingJob(r.Context(), key) if err != nil { log.Printf("Something Went Wrong On Creation Of Transcoding Job: %s", err) diff --git a/tests/README.md b/tests/README.md index e7c229b..374f59e 100644 --- a/tests/README.md +++ b/tests/README.md @@ -66,20 +66,8 @@ Both steps of the upload flow, end to end: category, and a storage key this API never issued - answering 404 for a category id no category has, kept distinct from the 500 the category lookup itself failing produces - -## `@known-gap` - -One scenario is tagged `@known-gap`. It passes — it pins down what the service -does **today**, where that differs from what it is documented to do: - -- **registering the same upload twice answers 500.** `videos.storage_key` is - `UNIQUE`, so the retry the API's own error message invites ("retry this - request with the same 'key'") fails the insert — after a second MediaConvert - job has already been queued for the same file. - -Fixing it means changing the scenario alongside the handler. It is tagged so it -is easy to find, and so it can be excluded with -`gobdd.WithIgnoredTags("@known-gap")` if that is ever wanted. +- answering 409 for a key that was already registered, rather than queueing a + second transcoding job for the same file ## Adding a scenario diff --git a/tests/features/video_upload.feature b/tests/features/video_upload.feature index 50cda6c..5a28d96 100644 --- a/tests/features/video_upload.feature +++ b/tests/features/video_upload.feature @@ -97,8 +97,6 @@ Feature: Video upload | ../videos/escape.mp4 | | videos-of-someone-else/clip.mp4 | - # An id no category has is the client's mistake, and is answered separately - # from the category lookup failing, which is the server's and stays a 500. Scenario: Registering a video under a category id that does not exist Given I have requested an upload slot for "mystery.mp4" of type "video/mp4" And I have uploaded the file to the upload URL @@ -106,14 +104,12 @@ Feature: Video upload Then the request is rejected with status 404 And the problem title is "Catagory Does Not Exist" - # Registration is not idempotent: videos.storage_key is UNIQUE, so a retry - # with a key that was already registered fails the insert — after a second - # MediaConvert job has already been queued for the same file. - @known-gap + # A key is registered once. The second attempt is turned away before the + # transcoding job is queued, so it cannot leave a duplicate job behind. Scenario: Registering the same upload twice Given I have requested an upload slot for "twice.mp4" of type "video/mp4" And I have uploaded the file to the upload URL And I have registered the uploaded video titled "Filed Once" under categories "other" When I register the uploaded video titled "Filed Twice" under categories "other" - Then the request is rejected with status 500 - And the problem title is "Something Went Wrong Saving The Video Record" + Then the request is rejected with status 409 + And the problem title is "Video Already Registered"