From fea6aa0fc7f13c53d6a5ef99c7c865ce53cc9bd0 Mon Sep 17 00:00:00 2001 From: Lewis Date: Mon, 11 May 2026 18:05:37 +0000 Subject: [PATCH] appview: fix issue/pr notifications being skipped Lewis: May this revision serve well! --- appview/db/collaborators.go | 5 ++++- appview/db/collaborators_null_rkey_test.go | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ appview/db/db.go | 31 +++++++++++++++++++++++++++++++ appview/db/repos.go | 8 ++++++-- appview/models/collaborator.go | 3 ++- appview/notify/db/db_test.go | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ appview/repo/repo.go | 15 ++++++++++++++- 7 file(s) changed, 324 insertion(s)(+), 5 deletion(s)(-) diff --git a/appview/db/collaborators.go b/appview/db/collaborators.go --- a/appview/db/collaborators.go +++ b/appview/db/collaborators.go @@ -11,7 +11,10 @@ ) func AddCollaborator(e Execer, c models.Collaborator) error { _, err := e.Exec( - `insert into collaborators (did, rkey, subject_did, repo_did) values (?, ?, ?, ?);`, + `insert into collaborators (did, rkey, subject_did, repo_did) values (?, ?, ?, ?) + on conflict(repo_did, subject_did) do update set + did = excluded.did, + rkey = excluded.rkey`, c.Did, c.Rkey, c.SubjectDid, string(c.RepoDid), ) return err diff --git a/appview/db/collaborators_null_rkey_test.go b/appview/db/collaborators_null_rkey_test.go new file mode 100644 --- /dev/null +++ b/appview/db/collaborators_null_rkey_test.go @@ -0,0 +1,95 @@ +package db + +import ( + "database/sql" + "testing" + + "github.com/bluesky-social/indigo/atproto/syntax" + "tangled.org/core/appview/models" + "tangled.org/core/orm" +) + +func TestGetCollaborators_NullRkey(t *testing.T) { + d := newTestDB(t) + + seedRepo(t, d, "did:plc:boltless", "knot.example", "anemone", "anemone", "did:plc:anemone") + + _, err := d.Exec( + `insert into collaborators (did, rkey, subject_did, repo_did) values (?, NULL, ?, ?)`, + "did:plc:boltless", "did:plc:akshay", "did:plc:anemone", + ) + if err != nil { + t.Fatalf("insert: %v", err) + } + + collabs, err := GetCollaborators(d, orm.FilterEq("repo_did", "did:plc:anemone")) + if err != nil { + t.Fatalf("GetCollaborators: %v", err) + } + if len(collabs) != 1 { + t.Fatalf("want 1 collab, got %d", len(collabs)) + } + if collabs[0].Rkey.Valid { + t.Fatalf("expected NULL rkey to scan as !Valid, got %+v", collabs[0].Rkey) + } +} + +func TestAddCollaborator_DuplicateUpdatesRkey(t *testing.T) { + d := newTestDB(t) + + seedRepo(t, d, "did:plc:boltless", "knot.example", "anemone", "anemone", "did:plc:anemone") + + collab := func(rkey string) models.Collaborator { + return models.Collaborator{ + Did: syntax.DID("did:plc:boltless"), + Rkey: sql.NullString{String: rkey, Valid: true}, + SubjectDid: syntax.DID("did:plc:akshay"), + RepoDid: syntax.DID("did:plc:anemone"), + } + } + + if err := AddCollaborator(d, collab("rkey-first")); err != nil { + t.Fatalf("first AddCollaborator: %v", err) + } + if err := AddCollaborator(d, collab("rkey-second")); err != nil { + t.Fatalf("second AddCollaborator: %v", err) + } + + collabs, err := GetCollaborators(d, orm.FilterEq("repo_did", "did:plc:anemone")) + if err != nil { + t.Fatalf("GetCollaborators: %v", err) + } + if len(collabs) != 1 { + t.Fatalf("want 1 collab after dup add, got %d", len(collabs)) + } + if collabs[0].Rkey.String != "rkey-second" { + t.Fatalf("want rkey-second, got %q", collabs[0].Rkey.String) + } +} + +func TestEnqueuePdsRewritesForRepo_SkipsNullRkeyCollab(t *testing.T) { + d := newTestDB(t) + + seedRepo(t, d, "did:plc:boltless", "knot.example", "anemone", "anemone", "did:plc:anemone") + + if _, err := d.Exec( + `insert into collaborators (did, rkey, subject_did, repo_did) values (?, NULL, ?, ?)`, + "did:plc:boltless", "did:plc:akshay", "did:plc:anemone", + ); err != nil { + t.Fatalf("insert: %v", err) + } + + tx, err := d.Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + defer tx.Rollback() + + if err := EnqueuePdsRewritesForRepo(tx, "did:plc:anemone", "at://did:plc:boltless/sh.tangled.repo/anemone"); err != nil { + t.Fatalf("EnqueuePdsRewritesForRepo: %v", err) + } + + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } +} diff --git a/appview/db/db.go b/appview/db/db.go --- a/appview/db/db.go +++ b/appview/db/db.go @@ -2127,6 +2127,37 @@ return err }) conn.ExecContext(ctx, "pragma foreign_keys = on;") + orm.RunMigration(conn, logger, "collaborators-unique-on-repo-subject", func(tx *sql.Tx) error { + _, err := tx.Exec(` + CREATE TABLE collaborators_new ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + did TEXT NOT NULL, + rkey TEXT, + subject_did TEXT NOT NULL, + repo_did TEXT NOT NULL, + created TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')), + UNIQUE(repo_did, subject_did), + FOREIGN KEY (repo_did) REFERENCES repos(repo_did) ON DELETE CASCADE + ); + INSERT INTO collaborators_new (id, did, rkey, subject_did, repo_did, created) + SELECT id, did, rkey, subject_did, repo_did, created + FROM ( + SELECT + id, did, rkey, subject_did, repo_did, created, + ROW_NUMBER() OVER ( + PARTITION BY repo_did, subject_did + ORDER BY created DESC, id DESC + ) AS rn + FROM collaborators + ) + WHERE rn = 1; + DROP TABLE collaborators; + ALTER TABLE collaborators_new RENAME TO collaborators; + CREATE INDEX idx_collaborators_repo_did ON collaborators(repo_did); + CREATE INDEX idx_collaborators_subject_did ON collaborators(subject_did); + `) + return err + }) return &DB{ db, logger, diff --git a/appview/db/repos.go b/appview/db/repos.go --- a/appview/db/repos.go +++ b/appview/db/repos.go @@ -689,12 +689,16 @@ } var pairs []struct{ did, rkey string } for rows.Next() { - var d, r string + var d string + var r sql.NullString if scanErr := rows.Scan(&d, &r); scanErr != nil { rows.Close() return fmt.Errorf("scan %s for pds rewrites: %w", src.table, scanErr) } - pairs = append(pairs, struct{ did, rkey string }{d, r}) + if !r.Valid { + continue + } + pairs = append(pairs, struct{ did, rkey string }{d, r.String}) } rows.Close() if rowsErr := rows.Err(); rowsErr != nil { diff --git a/appview/models/collaborator.go b/appview/models/collaborator.go --- a/appview/models/collaborator.go +++ b/appview/models/collaborator.go @@ -1,6 +1,7 @@ package models import ( + "database/sql" "time" "github.com/bluesky-social/indigo/atproto/syntax" @@ -10,7 +11,7 @@ type Collaborator struct { // identifiers for the record Id int64 Did syntax.DID - Rkey string + Rkey sql.NullString // content SubjectDid syntax.DID diff --git a/appview/notify/db/db_test.go b/appview/notify/db/db_test.go new file mode 100644 --- /dev/null +++ b/appview/notify/db/db_test.go @@ -0,0 +1,172 @@ +package db_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/bluesky-social/indigo/atproto/syntax" + appviewdb "tangled.org/core/appview/db" + "tangled.org/core/appview/models" + notifydb "tangled.org/core/appview/notify/db" +) + +func TestNewIssue_DeliversWithNullRkeyCollaborator(t *testing.T) { + d := setupNotifyTestDB(t) + + ownerDid := "did:plc:boltless" + repoDid := "did:plc:anemone" + collabDid := "did:plc:limpet" + authorDid := "did:plc:akshay" + + repo := seedNotifyRepo(t, d, ownerDid, repoDid) + insertNullRkeyCollaborator(t, d, ownerDid, collabDid, repoDid) + + issue := seedIssue(t, d, authorDid, repoDid, repo) + + notifier := notifydb.NewDatabaseNotifier(d, nil) + notifier.NewIssue(context.Background(), issue, nil) + + if got := notificationCount(t, d, ownerDid); got != 1 { + t.Errorf("repo owner %s: want 1 notification, got %d", ownerDid, got) + } + if got := notificationCount(t, d, collabDid); got != 1 { + t.Errorf("null-rkey collaborator %s: want 1 notification, got %d", collabDid, got) + } + if got := notificationCount(t, d, authorDid); got != 0 { + t.Errorf("issue author %s: want 0 notifications, got %d", authorDid, got) + } +} + +func TestNewPull_DeliversWithNullRkeyCollaborator(t *testing.T) { + d := setupNotifyTestDB(t) + + ownerDid := "did:plc:boltless" + repoDid := "did:plc:anemone" + collabDid := "did:plc:limpet" + authorDid := "did:plc:akshay" + + seedNotifyRepo(t, d, ownerDid, repoDid) + insertNullRkeyCollaborator(t, d, ownerDid, collabDid, repoDid) + + pull := seedPull(t, d, authorDid, repoDid) + + notifier := notifydb.NewDatabaseNotifier(d, nil) + notifier.NewPull(context.Background(), pull) + + if got := notificationCount(t, d, ownerDid); got != 1 { + t.Errorf("repo owner %s: want 1 notification, got %d", ownerDid, got) + } + if got := notificationCount(t, d, collabDid); got != 1 { + t.Errorf("null-rkey collaborator %s: want 1 notification, got %d", collabDid, got) + } + if got := notificationCount(t, d, authorDid); got != 0 { + t.Errorf("pull author %s: want 0 notifications, got %d", authorDid, got) + } +} + +func setupNotifyTestDB(t *testing.T) *appviewdb.DB { + t.Helper() + path := filepath.Join(t.TempDir(), "test.db") + d, err := appviewdb.Make(context.Background(), path) + if err != nil { + t.Fatalf("Make: %v", err) + } + t.Cleanup(func() { d.Close() }) + return d +} + +func seedNotifyRepo(t *testing.T, d *appviewdb.DB, ownerDid, repoDid string) *models.Repo { + t.Helper() + tx, err := d.Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + repo := &models.Repo{ + Did: ownerDid, + Name: "shell", + Knot: "knot.example", + Rkey: "shellrkey", + RepoDid: repoDid, + } + if err := appviewdb.AddRepo(tx, repo); err != nil { + t.Fatalf("AddRepo: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + return repo +} + +func seedIssue(t *testing.T, d *appviewdb.DB, authorDid, repoDid string, repo *models.Repo) *models.Issue { + t.Helper() + issue := &models.Issue{ + Did: authorDid, + Rkey: "issuerkey", + RepoDid: syntax.DID(repoDid), + IssueId: 1, + Title: "test", + Body: "body", + Open: true, + Repo: repo, + } + result, err := d.Exec( + `insert into issues (did, rkey, repo_did, issue_id, title, body, open) values (?, ?, ?, ?, ?, ?, 1)`, + issue.Did, issue.Rkey, string(issue.RepoDid), issue.IssueId, issue.Title, issue.Body, + ) + if err != nil { + t.Fatalf("insert issue: %v", err) + } + id, err := result.LastInsertId() + if err != nil { + t.Fatalf("LastInsertId: %v", err) + } + issue.Id = id + return issue +} + +func seedPull(t *testing.T, d *appviewdb.DB, authorDid, repoDid string) *models.Pull { + t.Helper() + pull := &models.Pull{ + RepoDid: syntax.DID(repoDid), + OwnerDid: authorDid, + Rkey: "pullrkey", + Title: "test", + Body: "body", + TargetBranch: "main", + State: models.PullOpen, + } + tx, err := d.Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + if err := appviewdb.PutPull(tx, pull); err != nil { + t.Fatalf("PutPull: %v", err) + } + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + return pull +} + +func insertNullRkeyCollaborator(t *testing.T, d *appviewdb.DB, issuerDid, subjectDid, repoDid string) { + t.Helper() + if _, err := d.Exec( + `insert into collaborators (did, rkey, subject_did, repo_did) values (?, NULL, ?, ?)`, + issuerDid, subjectDid, repoDid, + ); err != nil { + t.Fatalf("insert null-rkey collaborator: %v", err) + } +} + +func notificationCount(t *testing.T, d *appviewdb.DB, recipientDid string) int { + t.Helper() + var count int + if err := d.QueryRow( + `select count(*) from notifications where recipient_did = ?`, + recipientDid, + ).Scan(&count); err != nil { + t.Fatalf("count notifications for %s: %v", recipientDid, err) + } + return count +} diff --git a/appview/repo/repo.go b/appview/repo/repo.go --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -745,6 +745,19 @@ } l = l.With("collaborator", collaboratorIdent.Handle) l = l.With("knot", f.Knot) + existing, err := db.GetCollaborators(rp.db, + orm.FilterEq("repo_did", f.RepoDid), + orm.FilterEq("subject_did", collaboratorIdent.DID.String()), + ) + if err != nil { + fail("Failed to check existing collaborators.", err) + return + } + if len(existing) > 0 { + fail(fmt.Sprintf("%s is already a collaborator.", collaboratorIdent.Handle), nil) + return + } + // announce this relation into the firehose, store into owners' pds client, err := rp.oauth.AuthorizedClient(r) if err != nil { @@ -803,7 +816,7 @@ } err = db.AddCollaborator(tx, models.Collaborator{ Did: syntax.DID(currentUser.Did), - Rkey: rkey, + Rkey: sql.NullString{String: rkey, Valid: true}, SubjectDid: collaboratorIdent.DID, RepoDid: syntax.DID(f.RepoDid), Created: createdAt, -- tangled.sh