diff --git a/CHANGELOG.md b/CHANGELOG.md index 96325c7..d980c79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 now goes through the same validation as a `POST`. - A `DELETE` of a missing software answered `404` but still recorded a delete event and notified the webhooks. +- Two concurrent `DELETE` of the same software or publisher both + answered `204` and recorded two delete events. - The event recorded by an analysis `PATCH` had no actor. - `?from` and `?to` refused RFC 3339 values with a zone offset. - `?from` and `?to` were ignored on the per entity log lists and on the diff --git a/internal/handlers/bundles.go b/internal/handlers/bundles.go index da58550..7a5fb38 100644 --- a/internal/handlers/bundles.go +++ b/internal/handlers/bundles.go @@ -130,19 +130,13 @@ func (b *Bundle) PatchBundle(ctx *fiber.Ctx) error { func (b *Bundle) DeleteBundle(ctx *fiber.Ctx) error { const errMsg = "can't delete Bundle" - bundle, err := findOne[models.Bundle](b.db, ctx.Params("id"), findOptions{ - title: errMsg, - name: bundleEntityName, - }) - if err != nil { - return err - } + bundle := models.Bundle{ID: ctx.Params("id")} // Select("Software") also removes the bundles_software join rows. if err := models.Transaction(writeDB(ctx, b.db), func(tran *gorm.DB) error { - return tran.Select(softwareAssociation).Delete(bundle).Error + return deleteResult(tran.Select(softwareAssociation).Delete(&bundle)) }); err != nil { - return common.InternalServerError(errMsg) + return deleteError(err, errMsg, bundleEntityName) } return ctx.SendStatus(fiber.StatusNoContent) diff --git a/internal/handlers/catalogs.go b/internal/handlers/catalogs.go index 8f1b7de..796e43a 100644 --- a/internal/handlers/catalogs.go +++ b/internal/handlers/catalogs.go @@ -262,9 +262,9 @@ func (c *Catalog) DeleteCatalog(ctx *fiber.Ctx) error { return err } - return tran.Delete(&catalog).Error + return deleteResult(tran.Delete(&catalog)) }); err != nil { - return common.InternalServerError(errMsg) + return deleteError(err, errMsg, "Catalog") } if conflictErr != nil { diff --git a/internal/handlers/crud.go b/internal/handlers/crud.go index 6b23ab5..cecb70c 100644 --- a/internal/handlers/crud.go +++ b/internal/handlers/crud.go @@ -280,6 +280,32 @@ func deleteEntityLogs(tran *gorm.DB, entity models.Model) error { Delete(&models.Log{}).Error } +// deleteResult reports gorm.ErrRecordNotFound when the delete matched no +// row. gorm runs the AfterDelete hook either way, so a concurrent DELETE +// of an id already gone would otherwise answer 204 and record a second +// delete event. +func deleteResult(result *gorm.DB) error { + if result.Error != nil { + return result.Error + } + + if result.RowsAffected == 0 { + return gorm.ErrRecordNotFound + } + + return nil +} + +// deleteError maps the error of a delete to Problem JSON: 404 when the row +// was already gone, 500 otherwise. +func deleteError(err error, title, name string) error { + if errors.Is(err, gorm.ErrRecordNotFound) { + return common.Error(fiber.StatusNotFound, title, name+" was not found") + } + + return common.InternalServerError(title) +} + // updateColumns writes the listed columns of entity and only those. A plain // struct update skips a nil pointer, so a merge patch nulling an optional // field would answer 200 and leave the old value in the row. Naming the diff --git a/internal/handlers/publishers.go b/internal/handlers/publishers.go index 5fb91ad..950acec 100644 --- a/internal/handlers/publishers.go +++ b/internal/handlers/publishers.go @@ -68,10 +68,14 @@ func (p *Publisher) PatchPublisher(ctx *fiber.Ctx) error { return updatePublisher(ctx, writeDB(ctx, p.db), *found) } -// DeletePublisher deletes the publisher with the given ID. +// DeletePublisher deletes the publisher with the given ID. The lookup +// stays because the delete matches the UUID alone, while the id in the +// path can be an alternative id. func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error { + const errMsg = "can't delete Publisher" + found, err := findOne[models.Publisher](p.db, ctx.Params("id"), findOptions{ - title: "can't delete Publisher", + title: errMsg, name: publisherEntityName, byAlternativeID: true, }) @@ -86,9 +90,9 @@ func (p *Publisher) DeletePublisher(ctx *fiber.Ctx) error { return err } - return tran.Select(codeHostingAssociation).Delete(&publisher).Error + return deleteResult(tran.Select(codeHostingAssociation).Delete(&publisher)) }); err != nil { - return common.InternalServerError("can't delete Publisher") + return deleteError(err, errMsg, publisherEntityName) } return ctx.SendStatus(fiber.StatusNoContent) diff --git a/internal/handlers/software.go b/internal/handlers/software.go index 6d6382d..aa27e1d 100644 --- a/internal/handlers/software.go +++ b/internal/handlers/software.go @@ -86,28 +86,20 @@ func (p *Software) PatchSoftware(ctx *fiber.Ctx) error { return updateSoftware(ctx, writeDB(ctx, p.db), software) } -// DeleteSoftware deletes the software with the given ID. The row is -// looked up first: gorm runs the AfterDelete hook even when the delete -// matched nothing, and the hook would record an event for a software -// that never existed. +// DeleteSoftware deletes the software with the given ID. func (p *Software) DeleteSoftware(ctx *fiber.Ctx) error { const errMsg = "can't delete Software" - found, err := findOne[models.Software](p.db, ctx.Params("id"), findOptions{title: errMsg, name: softwareEntityName}) - if err != nil { - return err - } - - software := *found + software := models.Software{ID: ctx.Params("id")} if err := models.Transaction(writeDB(ctx, p.db), func(tran *gorm.DB) error { if err := deleteEntityLogs(tran, software); err != nil { return err } - return tran.Select("Aliases", "Bundles").Delete(&software).Error + return deleteResult(tran.Select("Aliases", "Bundles").Delete(&software)) }); err != nil { - return common.InternalServerError(errMsg) + return deleteError(err, errMsg, softwareEntityName) } return ctx.SendStatus(fiber.StatusNoContent) diff --git a/internal/handlers/webhooks.go b/internal/handlers/webhooks.go index 049e380..d6fa7e0 100644 --- a/internal/handlers/webhooks.go +++ b/internal/handlers/webhooks.go @@ -138,21 +138,16 @@ func (p *Webhook[T]) PostSingleResourceWebhook(ctx *fiber.Ctx) error { return ctx.JSON(&webhook) } -// DeleteWebhook deletes the webhook with the given ID. The row is looked -// up first, so that the AfterDelete hook records no event for a missing -// id. +// DeleteWebhook deletes the webhook with the given ID. func (p *Webhook[T]) DeleteWebhook(ctx *fiber.Ctx) error { const errMsg = "can't delete Webhook" - webhook, err := findOne[models.Webhook](p.db, ctx.Params("id"), findOptions{title: errMsg, name: "Webhook"}) - if err != nil { - return err - } + webhook := models.Webhook{ID: ctx.Params("id")} if err := models.Transaction(writeDB(ctx, p.db), func(tran *gorm.DB) error { - return tran.Delete(webhook).Error + return deleteResult(tran.Delete(&webhook)) }); err != nil { - return common.InternalServerError(errMsg) + return deleteError(err, errMsg, "Webhook") } return ctx.SendStatus(fiber.StatusNoContent) diff --git a/main_test.go b/main_test.go index 25f3dc3..ba5c2c4 100644 --- a/main_test.go +++ b/main_test.go @@ -322,6 +322,33 @@ func dbNull(t *testing.T, table, column, whereCol, whereVal string) bool { return val == nil } +// addEntityLogs attaches count logs to the entity. Soft deleting them +// makes the delete of the entity itself slow enough that a concurrent +// DELETE of the same id still finds the row. +func addEntityLogs(t *testing.T, entityType, entityID string, count int) { + t.Helper() + + query := fmt.Sprintf( + "INSERT INTO logs (id, message, entity_id, entity_type, created_at, updated_at) "+ + "VALUES (%s, %s, %s, %s, %s, %s)", + placeholder(1), + placeholder(2), + placeholder(3), + placeholder(4), + placeholder(5), + placeholder(6), + ) + + now := time.Now().UTC() + + for i := range count { + id := fmt.Sprintf("33333333-3333-3333-3333-%012d", i) + + _, err := db.Exec(query, id, "concurrent delete padding", entityID, entityType, now, now) + require.NoError(t, err) + } +} + // decodeJSON unmarshals a response body into a map, so a test can assert on // the keys it carries. func decodeJSON(t *testing.T, body []byte) map[string]any { diff --git a/publishers_test.go b/publishers_test.go index 18df97e..bdec83b 100644 --- a/publishers_test.go +++ b/publishers_test.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "slices" "strings" "testing" "time" @@ -1220,6 +1221,54 @@ func TestPublishersDeleteDBChecks(t *testing.T) { assert.False(t, dbNull(t, "logs", "deleted_at", "id", logID)) assert.Equal(t, 1, dbCount(t, "events", "entity_id", italiaPublisherID)) }) + + t.Run("concurrent DELETE of one publisher answers 404 once", func(t *testing.T) { + loadFixtures(t) + + const ( + publisherID = "0947ebca-ae8f-46e1-9cfe-2caccc87108b" + paddingLogs = 300 + ) + + addEntityLogs(t, "publishers", publisherID, paddingLogs) + + start := make(chan struct{}) + statuses := make(chan int, 2) + + for range 2 { + go func() { + req, err := newTestRequest("DELETE", "/v1/publishers/"+publisherID, nil) + if err != nil { + statuses <- 0 + + return + } + + req.Header = map[string][]string{"Authorization": {goodToken}} + + <-start + + res, err := app.Test(req, -1) + if err != nil { + statuses <- 0 + + return + } + + _ = res.Body.Close() + + statuses <- res.StatusCode + }() + } + + close(start) + + statusCodes := []int{<-statuses, <-statuses} + slices.Sort(statusCodes) + + assert.Equal(t, []int{204, 404}, statusCodes) + assert.Equal(t, 1, dbCount(t, "events", "entity_id", publisherID)) + }) } func addPublishersForPaginationCapTest(t *testing.T) { diff --git a/software_test.go b/software_test.go index 167fc5d..de79da2 100644 --- a/software_test.go +++ b/software_test.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "fmt" + "slices" "strings" "sync" "testing" @@ -2159,6 +2160,54 @@ func TestSoftwareDeleteDBChecks(t *testing.T) { assert.Equal(t, 0, dbCount(t, "events", "entity_id", softwareID)) }) + + t.Run("concurrent DELETE of one software answers 404 once", func(t *testing.T) { + loadFixtures(t) + + const ( + softwareID = "11e101c4-f989-4cc4-a665-63f9f34e83f6" + paddingLogs = 300 + ) + + addEntityLogs(t, "software", softwareID, paddingLogs) + + start := make(chan struct{}) + statuses := make(chan int, 2) + + for range 2 { + go func() { + req, err := newTestRequest("DELETE", "/v1/software/"+softwareID, nil) + if err != nil { + statuses <- 0 + + return + } + + req.Header = map[string][]string{"Authorization": {goodToken}} + + <-start + + res, err := app.Test(req, -1) + if err != nil { + statuses <- 0 + + return + } + + _ = res.Body.Close() + + statuses <- res.StatusCode + }() + } + + close(start) + + statusCodes := []int{<-statuses, <-statuses} + slices.Sort(statusCodes) + + assert.Equal(t, []int{204, 404}, statusCodes) + assert.Equal(t, 1, dbCount(t, "events", "entity_id", softwareID)) + }) } func addSoftwareForPaginationCapTest(t *testing.T) {