From 37142229eab90c2c7d1a422227bc0b9bdccf905c Mon Sep 17 00:00:00 2001
From: Tim Van Baak <tim.vanbaak@gmail.com>
Date: Fri, 31 Jan 2025 09:55:07 -0800
Subject: [PATCH] Make item updates transactional

---
 cmd/actionExecute.go | 16 +++++++++------
 core/source.go       | 12 +++++++++++
 core/source_test.go  | 47 ++++++++++++++++++++++++++++++++++++++++++++
 web/item.go          | 16 +++++++++------
 4 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/cmd/actionExecute.go b/cmd/actionExecute.go
index c76ef16..64930c5 100644
--- a/cmd/actionExecute.go
+++ b/cmd/actionExecute.go
@@ -143,11 +143,15 @@ func actionExecute(
 		return
 	}
 
-	if err = core.UpdateItems(db, []core.Item{newItem}); err != nil {
-		log.Fatalf("error: failed to update item: %v", err)
-	}
-
-	if err = core.SetState(db, source, newState); err != nil {
-		log.Fatalf("error: failed to set state for %s: %v", source, err)
+	if err = db.Transact(func(tx core.DB) error {
+		if _err := core.UpdateItems(tx, []core.Item{newItem}); err != nil {
+			return fmt.Errorf("failed to update item: %v", _err)
+		}
+		if _err := core.SetState(tx, source, newState); err != nil {
+			return fmt.Errorf("failed to set state for %s: %v", source, _err)
+		}
+		return nil
+	}); err != nil {
+		log.Fatalf("error: %v", err)
 	}
 }
diff --git a/core/source.go b/core/source.go
index d7a4051..942ba56 100644
--- a/core/source.go
+++ b/core/source.go
@@ -254,6 +254,18 @@ func SetState(db DB, source string, state []byte) error {
 //
 // Returns the number of new and deleted items on success.
 func UpdateWithFetchedItems(db DB, source string, state []byte, items []Item) (int, int, error) {
+	var new int
+	var del int
+	var err error
+	err = db.Transact(func(tx DB) error {
+		new, del, err = updateWithFetchedItemsTx(tx, source, state, items)
+		return err
+	})
+	return new, del, err
+}
+
+// Implementation logic for [UpdateWithFetchedItems], which executes this inside a transaction.
+func updateWithFetchedItemsTx(db DB, source string, state []byte, items []Item) (int, int, error) {
 	// Get the existing items
 	existingItems, err := GetAllItemsForSource(db, source)
 	if err != nil {
diff --git a/core/source_test.go b/core/source_test.go
index 053f1d3..4dfb2ac 100644
--- a/core/source_test.go
+++ b/core/source_test.go
@@ -1,8 +1,10 @@
 package core
 
 import (
+	"errors"
 	"fmt"
 	"slices"
+	"strings"
 	"testing"
 	"time"
 
@@ -163,6 +165,51 @@ func TestUpdateSourceAddAndDelete(t *testing.T) {
 	}
 }
 
+func TestUpdateSourceTransaction(t *testing.T) {
+	db := EphemeralDb(t)
+	if err := AddSource(db, "s"); err != nil {
+		t.Fatal(err)
+	}
+
+	a := Item{Source: "s", Id: "a"}
+	b := Item{Source: "s", Id: "b"}
+
+	// Add and deactivate a so it will be deleted on next fetch without it
+	if add, _, err := UpdateWithFetchedItems(db, "s", nil, []Item{a}); add != 1 || err != nil {
+		t.Fatalf("expected 1 add, got %d and err %v", add, err)
+	}
+	if _, err := DeactivateItem(db, "s", "a"); err != nil {
+		t.Fatal(err)
+	}
+
+	// Add b and cause a to be deleted, but the delete throws an error
+	fdb := &FailureDb{
+		db: db,
+		execError: func(q string, a ...any) error {
+			if strings.Contains(q, "delete from") {
+				return errors.New("no deletes!")
+			}
+			return nil
+		},
+	}
+	add, del, err := UpdateWithFetchedItems(fdb, "s", nil, []Item{b})
+	if add != 0 || del != 0 || err == nil {
+		t.Fatalf("expected failure, got %d %d %v", add, del, err)
+	}
+
+	// Failure should not add b
+	items, err := GetAllItemsForSource(db, "s")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(items) != 1 {
+		t.Fatalf("should only have one item, got %d", len((items)))
+	}
+	if items[0].Id != "a" {
+		t.Fatalf("expected only item to still be a, got %s", items[0].Id)
+	}
+}
+
 func TestOnCreateAction(t *testing.T) {
 	db := EphemeralDb(t)
 	if err := AddSource(db, "test"); err != nil {
diff --git a/web/item.go b/web/item.go
index c58a9e2..6c415de 100644
--- a/web/item.go
+++ b/web/item.go
@@ -1,6 +1,7 @@
 package web
 
 import (
+	"fmt"
 	"log"
 	"net/http"
 	"strings"
@@ -77,13 +78,16 @@ func (env *Env) doAction(writer http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	if err = core.UpdateItems(env.db, []core.Item{newItem}); err != nil {
+	if err = env.db.Transact(func(tx core.DB) error {
+		if _err := core.UpdateItems(tx, []core.Item{newItem}); err != nil {
+			return fmt.Errorf("failed to update item: %v", _err)
+		}
+		if _err := core.SetState(tx, source, newState); err != nil {
+			return fmt.Errorf("failed to set state for %s: %v", source, _err)
+		}
+		return nil
+	}); err != nil {
 		http.Error(writer, err.Error(), 500)
-		return
-	}
-
-	if err = core.SetState(env.db, source, newState); err != nil {
-		log.Fatalf("error: failed to set state for %s: %v", source, err)
 	}
 
 	html.Item(writer, html.ItemData{Item: newItem})