fix(delete): run delete hooks and queries in one transaction

- resolvespec/restheadspec: single and batch delete use one transaction - add sqlmock tests for delete transaction behaviour - testmodels: serial integer ids; update tests accordingly - add compose testserver, smoke script, podman-first Makefile targets
2026-10-02 11:31:57 +00:00 · 2026-09-30 22:33:25 +02:00
parent b2b815552f
commit cd96404cdd
12 changed files with 788 additions and 226 deletions
@@ -0,0 +1,171 @@
+package restheadspec
+
+import (
+	"context"
+	"database/sql"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/DATA-DOG/go-sqlmock"
+
+	"github.com/bitechdev/ResolveSpec/pkg/common"
+	"github.com/bitechdev/ResolveSpec/pkg/common/adapters/database"
+	"github.com/bitechdev/ResolveSpec/pkg/modelregistry"
+)
+
+type delItem struct {
+	ID   int    `json:"id" bun:"id,pk"`
+	Name string `json:"name" bun:"name"`
+}
+
+func newDeleteHarness(t *testing.T) (*Handler, sqlmock.Sqlmock, *sql.DB) {
+	t.Helper()
+	db, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatal(err)
+	}
+	// One connection: any statement that bypasses the transaction while it is
+	// open cannot get a connection and fails on the request context timeout.
+	db.SetMaxOpenConns(1)
+	t.Cleanup(func() { _ = db.Close() })
+	return NewHandler(database.NewPgSQLAdapter(db), modelregistry.NewModelRegistry()), mock, db
+}
+
+func runDelete(h *Handler, id string, data interface{}) *httptest.ResponseRecorder {
+	rec := httptest.NewRecorder()
+	w, _ := common.WrapHTTPRequest(rec, httptest.NewRequest(http.MethodPost, "/", nil))
+	base, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+	ctx := WithSchema(base, "public")
+	ctx = WithEntity(ctx, "items")
+	ctx = WithTableName(ctx, "items")
+	ctx = WithModel(ctx, &delItem{})
+	h.handleDelete(ctx, w, id, data)
+	return rec
+}
+
+// recordDeleteHook registers a BeforeDelete hook that captures hookCtx.Tx.
+func recordDeleteHook(h *Handler, hookErr error) *[]common.Database {
+	var seen []common.Database
+	h.Hooks().Register(BeforeDelete, func(ctx *HookContext) error {
+		seen = append(seen, ctx.Tx)
+		return hookErr
+	})
+	return &seen
+}
+
+func TestDeleteSingleUsesOneTransaction(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+	seen := recordDeleteHook(h, nil)
+
+	mock.ExpectBegin()
+	mock.ExpectQuery(`SELECT`).WillReturnRows(sqlmock.NewRows([]string{"id", "name"}).AddRow(7, "a"))
+	mock.ExpectExec(`DELETE FROM`).WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectCommit()
+
+	rec := runDelete(h, "7", nil)
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+	if len(*seen) != 1 || (*seen)[0] == h.db {
+		t.Fatalf("BeforeDelete must run once on the transaction, got %v", *seen)
+	}
+}
+
+func TestDeleteSingleNotFoundRollsBack(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+
+	mock.ExpectBegin()
+	mock.ExpectQuery(`SELECT`).WillReturnRows(sqlmock.NewRows([]string{"id", "name"}))
+	mock.ExpectRollback()
+
+	if rec := runDelete(h, "7", nil); rec.Code != http.StatusNotFound {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestDeleteHookErrorRollsBack(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+	recordDeleteHook(h, errors.New("denied"))
+
+	mock.ExpectBegin()
+	mock.ExpectQuery(`SELECT`).WillReturnRows(sqlmock.NewRows([]string{"id", "name"}).AddRow(7, "a"))
+	mock.ExpectRollback()
+
+	if rec := runDelete(h, "7", nil); rec.Code != http.StatusBadRequest {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestDeleteSingleExecErrorRollsBack(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+
+	mock.ExpectBegin()
+	mock.ExpectQuery(`SELECT`).WillReturnRows(sqlmock.NewRows([]string{"id", "name"}).AddRow(7, "a"))
+	mock.ExpectExec(`DELETE FROM`).WillReturnError(errors.New("boom"))
+	mock.ExpectRollback()
+
+	if rec := runDelete(h, "7", nil); rec.Code != http.StatusInternalServerError {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func TestDeleteBatchUsesOneTransaction(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+	seen := recordDeleteHook(h, nil)
+
+	mock.ExpectBegin()
+	mock.ExpectExec(`DELETE FROM`).WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec(`DELETE FROM`).WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectCommit()
+
+	rec := runDelete(h, "", []interface{}{"1", "2"})
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+	// restheadspec fires the hook per item
+	if len(*seen) != 2 || (*seen)[0] == h.db || (*seen)[1] == h.db {
+		t.Fatalf("BeforeDelete must run per item on the transaction, got %v", *seen)
+	}
+	var resp map[string]float64
+	if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil || resp["deleted"] != 2 {
+		t.Fatalf("unexpected body %s (%v)", rec.Body, err)
+	}
+}
+
+func TestDeleteBatchFailureRollsBackAll(t *testing.T) {
+	h, mock, _ := newDeleteHarness(t)
+
+	mock.ExpectBegin()
+	mock.ExpectExec(`DELETE FROM`).WillReturnResult(sqlmock.NewResult(0, 1))
+	mock.ExpectExec(`DELETE FROM`).WillReturnError(errors.New("boom"))
+	mock.ExpectRollback()
+
+	if rec := runDelete(h, "", []string{"1", "2"}); rec.Code != http.StatusInternalServerError {
+		t.Fatalf("status %d body %s", rec.Code, rec.Body)
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Fatal(err)
+	}
+}
@@ -1576,7 +1576,7 @@ func (h *Handler) handleUpdate(ctx context.Context, w common.ResponseWriter, id

 		// Now read the existing record from the database
 		existingRecord := reflect.New(reflection.GetPointerElement(reflect.TypeOf(model))).Interface()
-		selectQuery := tx.NewSelect().Model(existingRecord).Where(fmt.Sprintf("%s = ?", common.QuoteIdent(pkName)), targetID)
+		selectQuery := h.db.NewSelect().Model(existingRecord).Where(fmt.Sprintf("%s = ?", common.QuoteIdent(pkName)), targetID)
 		if err := selectQuery.ScanModel(ctx); err != nil {
 			if err == sql.ErrNoRows {
 				return fmt.Errorf("record not found with ID: %v", targetID)
@@ -1934,24 +1934,62 @@ func (h *Handler) handleDelete(ctx context.Context, w common.ResponseWriter, id
 		return
 	}

-	// Get primary key name
 	pkName := reflection.GetPrimaryKeyName(model)

-	// First, fetch the record that will be deleted
 	modelType := reflect.TypeOf(model)
 	modelType = reflection.GetPointerElement(modelType)
 	recordToDelete := reflect.New(modelType).Interface()

-	selectQuery := h.db.NewSelect().Model(recordToDelete).Where(fmt.Sprintf("%s = ?", common.QuoteIdent(pkName)), id)
+	// Lookup, hooks and delete share one transaction so transaction-local
+	// state set by hooks (e.g. RLS settings) applies to every statement.
+	var failure *deleteFailure
+	txErr := h.db.RunInTransaction(ctx, func(tx common.Database) error {
+		failure = h.deleteSingleInTx(ctx, tx, w, schema, entity, tableName, model, pkName, id, recordToDelete)
+		if failure != nil {
+			return failure
+		}
+		return nil
+	})
+	if failure != nil {
+		h.sendError(w, failure.status, failure.code, failure.message, failure.err)
+		return
+	}
+	if txErr != nil {
+		logger.Error("Error in delete transaction: %v", txErr)
+		h.sendError(w, http.StatusInternalServerError, "delete_error", "Error deleting record", txErr)
+		return
+	}
+
+	// Invalidate cache for this table after commit
+	cacheTags := buildCacheTags(schema, tableName)
+	if err := invalidateCacheForTags(ctx, cacheTags); err != nil {
+		logger.Warn("Failed to invalidate cache for table %s: %v", tableName, err)
+	}
+	h.sendResponse(w, recordToDelete, nil)
+}
+
+// deleteFailure describes an error response for a delete; returning it from the
+// transaction closure rolls the transaction back.
+type deleteFailure struct {
+	status  int
+	code    string
+	message string
+	err     error
+}
+
+func (f *deleteFailure) Error() string { return f.message }
+
+// deleteSingleInTx fetches the record, runs the delete hooks and deletes it, all on tx.
+func (h *Handler) deleteSingleInTx(ctx context.Context, tx common.Database, w common.ResponseWriter, schema, entity, tableName string, model interface{}, pkName, id string, recordToDelete interface{}) *deleteFailure {
+	// First, fetch the record that will be deleted
+	selectQuery := tx.NewSelect().Model(recordToDelete).Where(fmt.Sprintf("%s = ?", common.QuoteIdent(pkName)), id)
 	if err := selectQuery.ScanModel(ctx); err != nil {
 		if err == sql.ErrNoRows {
 			logger.Warn("Record not found for delete: %s = %s", pkName, id)
-			h.sendError(w, http.StatusNotFound, "not_found", "Record not found", err)
-			return
+			return &deleteFailure{http.StatusNotFound, "not_found", "Record not found", err}
 		}
 		logger.Error("Error fetching record for delete: %v", err)
-		h.sendError(w, http.StatusInternalServerError, "fetch_error", "Error fetching record", err)
-		return
+		return &deleteFailure{http.StatusInternalServerError, "fetch_error", "Error fetching record", err}
 	}

 	// Execute BeforeDelete hooks with the record data
@@ -1965,25 +2003,23 @@ func (h *Handler) handleDelete(ctx context.Context, w common.ResponseWriter, id
 		Operation: "delete",
 		ID:        id,
 		Writer:    w,
-		Tx:        h.db,
+		Tx:        tx,
 		Data:      recordToDelete,
 	}

 	if err := h.hooks.ExecuteBeforeOp(BeforeDelete, hookCtx); err != nil {
 		logger.Error("BeforeDelete hook failed: %v", err)
-		h.sendError(w, http.StatusBadRequest, "hook_error", "Hook execution failed", err)
-		return
+		return &deleteFailure{http.StatusBadRequest, "hook_error", "Hook execution failed", err}
 	}

-	query := h.db.NewDelete().Table(tableName)
+	query := tx.NewDelete().Table(tableName)
 	query = query.Where(fmt.Sprintf("%s = ?", common.QuoteIdent(pkName)), id)

 	// Execute BeforeScan hooks - pass query chain so hooks can modify it
 	hookCtx.Query = query
 	if err := h.hooks.ExecuteBeforeOp(BeforeScan, hookCtx); err != nil {
 		logger.Error("BeforeScan hook failed: %v", err)
-		h.sendError(w, http.StatusBadRequest, "hook_error", "Hook execution failed", err)
-		return
+		return &deleteFailure{http.StatusBadRequest, "hook_error", "Hook execution failed", err}
 	}

 	// Use potentially modified query from hook context
@@ -1994,15 +2030,13 @@ func (h *Handler) handleDelete(ctx context.Context, w common.ResponseWriter, id
 	result, err := query.Exec(ctx)
 	if err != nil {
 		logger.Error("Error deleting record: %v", err)
-		h.sendError(w, http.StatusInternalServerError, "delete_error", "Error deleting record", err)
-		return
+		return &deleteFailure{http.StatusInternalServerError, "delete_error", "Error deleting record", err}
 	}

 	// Check if the record was actually deleted
 	if result.RowsAffected() == 0 {
 		logger.Warn("No rows deleted for ID: %s", id)
-		h.sendError(w, http.StatusNotFound, "not_found", "Record not found or already deleted", nil)
-		return
+		return &deleteFailure{http.StatusNotFound, "not_found", "Record not found or already deleted", nil}
 	}

 	// Execute AfterDelete hooks with the deleted record data
@@ -2011,17 +2045,9 @@ func (h *Handler) handleDelete(ctx context.Context, w common.ResponseWriter, id

 	if err := h.hooks.Execute(AfterDelete, hookCtx); err != nil {
 		logger.Error("AfterDelete hook failed: %v", err)
-		h.sendError(w, http.StatusInternalServerError, "hook_error", "Hook execution failed", err)
-		return
+		return &deleteFailure{http.StatusInternalServerError, "hook_error", "Hook execution failed", err}
 	}
-
-	// Return the deleted record data
-	// Invalidate cache for this table
-	cacheTags := buildCacheTags(schema, tableName)
-	if err := invalidateCacheForTags(ctx, cacheTags); err != nil {
-		logger.Warn("Failed to invalidate cache for table %s: %v", tableName, err)
-	}
-	h.sendResponse(w, recordToDelete, nil)
+	return nil
 }

 // mergeRecordWithRequest merges a database record with the original request data