Fix follower cleanup semantics

CHANGES.md

@@ -6,6 +6,29 @@ Version 0.4.4
 
 To be released.
 
+### @fedify/botkit
+
+ -  Fixed `MemoryRepository`, `KvRepository`, and `MemoryCachedRepository` so
+    removing one of multiple active follow requests for the same actor no
+    longer deletes the follower too early or fires a premature unfollow event,
+    and reassigning a follow request no longer leaves stale followers behind.
+    [[#25], [#26]]
+
+[#25]: https://github.com/fedify-dev/botkit/issues/25
+[#26]: https://github.com/fedify-dev/botkit/pull/26
+
+### @fedify/botkit-sqlite
+
+ -  Fixed `SqliteRepository` so removing one of multiple active follow requests
+    for the same actor no longer fails with a foreign key error, and reassigning
+    a follow request no longer leaves stale followers behind.  [[#25]]
+
+### @fedify/botkit-postgres
+
+ -  Fixed `PostgresRepository` so removing one of multiple active follow
+    requests for the same actor no longer reports a follower removal until the
+    last active follow request is removed.  [[#25]]
+
 
 Version 0.4.3
 -------------

packages/botkit-postgres/src/mod.test.ts

@@ -24,7 +24,7 @@
 import postgres from "postgres";
 
 if (!("Temporal" in globalThis)) {
   globalThis.Temporal = (await import("@js-temporal" + "/polyfill")).Temporal;
 }
 
 function getPostgresUrl(): string | undefined {
@@ -872,8 +872,8 @@
         assert.deepStrictEqual(await repo.countFollowers(), 2);
         assert.ok(await repo.hasFollower(followerA.id!));
         assert.deepStrictEqual(
-          await (await repo.removeFollower(followA, followerA.id!))?.toJsonLd(),
-          await followerA.toJsonLd(),
+          await repo.removeFollower(followA, followerA.id!),
+          undefined,
         );
         assert.ok(await repo.hasFollower(followerA.id!));
         assert.deepStrictEqual(await repo.countFollowers(), 2);

packages/botkit-postgres/src/mod.ts

@@ -539,8 +539,8 @@ export class PostgresRepository implements Repository, AsyncDisposable {
           WHERE follow_request_id = $1`,
         [followId.href],
       );
-      await this.cleanupFollower(sql, followerId.href);
-      return await parseActor(row.actor_json);
+      const removed = await this.cleanupFollower(sql, followerId.href);
+      return removed ? await parseActor(row.actor_json) : undefined;
     });
   }
 
@@ -758,19 +758,21 @@ export class PostgresRepository implements Repository, AsyncDisposable {
   private async cleanupFollower(
     sql: Queryable,
     followerId: string,
-  ): Promise<void> {
+  ): Promise<boolean> {
     await this.lockFollower(sql, followerId);
-    await this.query(
+    const rows = await this.query<{ readonly follower_id: string }>(
       sql,
       `DELETE FROM ${this.table("followers")}
         WHERE follower_id = $1
           AND NOT EXISTS (
             SELECT 1
               FROM ${this.table("follow_requests")}
              WHERE follower_id = $1
-          )`,
+          )
+        RETURNING follower_id`,
       [followerId],
     );
+    return rows.length > 0;
   }
 
   private async ensureReady(): Promise<void> {

packages/botkit-sqlite/src/mod.test.ts

@@ -26,7 +26,7 @@
 import { describe, test } from "node:test";
 
 if (!("Temporal" in globalThis)) {
   globalThis.Temporal = (await import("@js-temporal" + "/polyfill")).Temporal;
 }
 
 function createSqliteRepository(
@@ -167,6 +167,95 @@
     }
   });
 
+  test("followers with multiple follow requests", async () => {
+    const repo = createSqliteRepository();
+    try {
+      const follower = new Person({
+        id: new URL("https://example.com/ap/actor/alice"),
+        preferredUsername: "alice",
+      });
+      const followA = new URL(
+        "https://example.com/ap/follow/f2fb7255-d3ad-4fef-8f9a-1d0f2c2ec0b4",
+      );
+      const followB = new URL(
+        "https://example.com/ap/follow/a3d4cc4f-af93-4a9f-a7b3-0b7c0fe4901d",
+      );
+
+      await repo.addFollower(followA, follower);
+      await repo.addFollower(followB, follower);
+      assert.deepStrictEqual(await repo.countFollowers(), 1);
+      assert.ok(await repo.hasFollower(follower.id!));
+
+      assert.deepStrictEqual(
+        await repo.removeFollower(followA, follower.id!),
+        undefined,
+      );
+      assert.deepStrictEqual(await repo.countFollowers(), 1);
+      assert.ok(await repo.hasFollower(follower.id!));
+
+      assert.deepStrictEqual(
+        await (await repo.removeFollower(followB, follower.id!))?.toJsonLd(),
+        await follower.toJsonLd(),
+      );
+      assert.deepStrictEqual(await repo.countFollowers(), 0);
+      assert.deepStrictEqual(await repo.hasFollower(follower.id!), false);
+    } finally {
+      repo.close();
+    }
+  });
+
+  test("followers with reassigned follow requests", async () => {
+    const repo = createSqliteRepository();
+    try {
+      const oldFollower = new Person({
+        id: new URL("https://example.com/ap/actor/alice"),
+        preferredUsername: "alice",
+      });
+      const newFollower = new Person({
+        id: new URL("https://example.com/ap/actor/bob"),
+        preferredUsername: "bob",
+      });
+      const followA = new URL(
+        "https://example.com/ap/follow/f2fb7255-d3ad-4fef-8f9a-1d0f2c2ec0b4",
+      );
+      const followB = new URL(
+        "https://example.com/ap/follow/a3d4cc4f-af93-4a9f-a7b3-0b7c0fe4901d",
+      );
+
+      await repo.addFollower(followA, oldFollower);
+      await repo.addFollower(followB, oldFollower);
+      await repo.addFollower(followA, newFollower);
+
+      assert.deepStrictEqual(await repo.countFollowers(), 2);
+      assert.ok(await repo.hasFollower(oldFollower.id!));
+      assert.ok(await repo.hasFollower(newFollower.id!));
+
+      assert.deepStrictEqual(
+        await (await repo.removeFollower(followB, oldFollower.id!))
+          ?.toJsonLd(),
+        await oldFollower.toJsonLd(),
+      );
+      assert.deepStrictEqual(await repo.countFollowers(), 1);
+      assert.deepStrictEqual(await repo.hasFollower(oldFollower.id!), false);
+      assert.ok(await repo.hasFollower(newFollower.id!));
+
+      await repo.addFollower(followA, oldFollower);
+      assert.deepStrictEqual(await repo.countFollowers(), 1);
+      assert.ok(await repo.hasFollower(oldFollower.id!));
+      assert.deepStrictEqual(await repo.hasFollower(newFollower.id!), false);
+      assert.deepStrictEqual(
+        await Promise.all(
+          (await Array.fromAsync(repo.getFollowers())).map((follower) =>
+            follower.toJsonLd()
+          ),
+        ),
+        [await oldFollower.toJsonLd()],
+      );
+    } finally {
+      repo.close();
+    }
+  });
+
   test("poll voting", async () => {
     const repo = createSqliteRepository();
     try {

packages/botkit-sqlite/src/mod.ts

@@ -130,6 +130,11 @@ export class SqliteRepository implements Repository, Disposable {
       )
     `);
 
+    this.db.exec(`
+      CREATE INDEX IF NOT EXISTS idx_follow_requests_follower
+      ON follow_requests(follower_id)
+    `);
+
     // Sent follows table
     this.db.exec(`
       CREATE TABLE IF NOT EXISTS sent_follows (
@@ -374,19 +379,35 @@ export class SqliteRepository implements Repository, Disposable {
     );
 
     const insertFollowerStmt = this.db.prepare(`
-      INSERT OR REPLACE INTO followers (follower_id, actor_json) 
+      INSERT INTO followers (follower_id, actor_json)
       VALUES (?, ?)
+      ON CONFLICT(follower_id)
+      DO UPDATE SET actor_json = excluded.actor_json
     `);
 
     const insertRequestStmt = this.db.prepare(`
-      INSERT OR REPLACE INTO follow_requests (follow_request_id, follower_id) 
+      INSERT INTO follow_requests (follow_request_id, follower_id)
       VALUES (?, ?)
+      ON CONFLICT(follow_request_id)
+      DO UPDATE SET follower_id = excluded.follower_id
     `);
 
-    this.db.exec("BEGIN TRANSACTION");
+    const previousFollowerStmt = this.db.prepare(`
+      SELECT follower_id FROM follow_requests WHERE follow_request_id = ?
+    `);
+
+    this.db.exec("BEGIN IMMEDIATE TRANSACTION");
     try {
+      const previousRow = previousFollowerStmt.get(followRequestId.href) as
+        | { follower_id: string }
+        | undefined;
       insertFollowerStmt.run(follower.id.href, followerJson);
       insertRequestStmt.run(followRequestId.href, follower.id.href);
+      if (
+        previousRow != null && previousRow.follower_id !== follower.id.href
+      ) {
+        this.cleanupFollower(previousRow.follower_id);
+      }
       this.db.exec("COMMIT");
     } catch (error) {
       this.db.exec("ROLLBACK");
@@ -398,40 +419,46 @@ export class SqliteRepository implements Repository, Disposable {
     followRequestId: URL,
     actorId: URL,
   ): Promise<Actor | undefined> {
-    // Check if the follow request exists and matches the actor
     const checkStmt = this.db.prepare(`
-      SELECT fr.follower_id, f.actor_json 
-      FROM follow_requests fr 
-      JOIN followers f ON fr.follower_id = f.follower_id 
+      SELECT fr.follower_id, f.actor_json
+      FROM follow_requests fr
+      JOIN followers f ON fr.follower_id = f.follower_id
       WHERE fr.follow_request_id = ? AND fr.follower_id = ?
     `);
 
-    const row = checkStmt.get(followRequestId.href, actorId.href) as {
-      follower_id: string;
-      actor_json: string;
-    } | undefined;
-
-    if (!row) return undefined;
-
-    // Remove the follower and follow request
     const deleteRequestStmt = this.db.prepare(`
-      DELETE FROM follow_requests WHERE follow_request_id = ?
-    `);
-
-    const deleteFollowerStmt = this.db.prepare(`
-      DELETE FROM followers WHERE follower_id = ?
+      DELETE FROM follow_requests
+      WHERE follow_request_id = ? AND follower_id = ?
     `);
 
-    this.db.exec("BEGIN TRANSACTION");
+    let row: { follower_id: string; actor_json: string } | undefined;
+    let removed = false;
+    this.db.exec("BEGIN IMMEDIATE TRANSACTION");
     try {
-      deleteRequestStmt.run(followRequestId.href);
-      deleteFollowerStmt.run(actorId.href);
+      row = checkStmt.get(followRequestId.href, actorId.href) as
+        | { follower_id: string; actor_json: string }
+        | undefined;
+      if (row == null) {
+        this.db.exec("COMMIT");
+        return undefined;
+      }
+      const deleteResult = deleteRequestStmt.run(
+        followRequestId.href,
+        actorId.href,
+      ) as { readonly changes: number };
+      if (deleteResult.changes < 1) {
+        this.db.exec("COMMIT");
+        return undefined;
+      }
+      removed = this.cleanupFollower(actorId.href);
       this.db.exec("COMMIT");
     } catch (error) {
       this.db.exec("ROLLBACK");
       throw error;
     }
 
+    if (!removed || row == null) return undefined;
+
     try {
       const actorData = JSON.parse(row.actor_json);
       const actor = await Object.fromJsonLd(actorData);
@@ -446,6 +473,21 @@ export class SqliteRepository implements Repository, Disposable {
     return undefined;
   }
 
+  private cleanupFollower(followerId: string): boolean {
+    const deleteFollowerStmt = this.db.prepare(`
+      DELETE FROM followers
+      WHERE follower_id = ?
+        AND NOT EXISTS (
+          SELECT 1 FROM follow_requests WHERE follower_id = ?
+        )
+    `);
+
+    const result = deleteFollowerStmt.run(followerId, followerId) as {
+      readonly changes: number;
+    };
+    return result.changes > 0;
+  }
+
   hasFollower(followerId: URL): Promise<boolean> {
     const stmt = this.db.prepare(`
       SELECT 1 FROM followers WHERE follower_id = ?

packages/botkit/package.json

@@ -76,7 +76,7 @@
     },
     "./package.json": "./package.json"
   },
-  "sideEffects": false,
+  "sideEffects": true,
   "files": [
     "dist",
     "LICENSE",