diff --git a/pkg/appview/db/migrations/0032_drop_tags_id.yaml b/pkg/appview/db/migrations/0032_drop_tags_id.yaml new file mode 100644 index 0000000..73ab106 --- /dev/null +++ b/pkg/appview/db/migrations/0032_drop_tags_id.yaml @@ -0,0 +1,37 @@ +description: | + Drop the surrogate id from tags and key the table by (did, repository, tag). + + Nothing ever joined on tags.id. It was selected into a struct field no caller + read, and used only by DeleteTagsNotInList to issue one DELETE per row. The + natural key was already enforced by UNIQUE(did, repository, tag), so this + promotes that to the primary key and removes the column. + + An AUTOINCREMENT rowid is allocated by whichever node performs the insert, + which is fine while every write funnels through a single writer and stops being + a stable identity the moment they do not. Removing an identifier nobody used is + the cheapest way to shrink that surface. + + idx_tags_did_repo is not recreated: the new primary key indexes + (did, repository) as a prefix, so a separate index on those two columns is + redundant. It was only needed while the primary key was the surrogate id. + + Rebuilds the table, since SQLite cannot drop an INTEGER PRIMARY KEY column. + Columns are named explicitly in the INSERT ... SELECT: column order is not + guaranteed to match between a fresh install and a migrated one, so SELECT * + here could write values into the wrong columns. +query: | + CREATE TABLE IF NOT EXISTS tags_new ( + did TEXT NOT NULL, + repository TEXT NOT NULL, + tag TEXT NOT NULL, + digest TEXT NOT NULL, + created_at TIMESTAMP NOT NULL, + PRIMARY KEY(did, repository, tag), + FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE + ); + + INSERT OR IGNORE INTO tags_new (did, repository, tag, digest, created_at) + SELECT did, repository, tag, digest, created_at FROM tags; + + DROP TABLE tags; + ALTER TABLE tags_new RENAME TO tags; diff --git a/pkg/appview/db/models.go b/pkg/appview/db/models.go index ef5f88a..d01e6d8 100644 --- a/pkg/appview/db/models.go +++ b/pkg/appview/db/models.go @@ -57,7 +57,6 @@ type ManifestReference struct { // Tag represents a tag pointing to a manifest type Tag struct { - ID int64 DID string Repository string Tag string diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index b607822..9027c0a 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -356,7 +356,7 @@ func GetUserRepositories(db DBTX, did string, viewerDID string) ([]Repository, e // ordering within each repo. func bulkTagsByRepo(db DBTX, did string, accessible map[string]bool) (map[string][]Tag, error) { rows, err := db.Query(` - SELECT id, repository, tag, digest, created_at + SELECT repository, tag, digest, created_at FROM tags WHERE did = ? ORDER BY repository, created_at DESC @@ -370,7 +370,7 @@ func bulkTagsByRepo(db DBTX, did string, accessible map[string]bool) (map[string for rows.Next() { var t Tag t.DID = did - if err := rows.Scan(&t.ID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt); err != nil { + if err := rows.Scan(&t.Repository, &t.Tag, &t.Digest, &t.CreatedAt); err != nil { return nil, err } if !accessible[t.Repository] { @@ -731,6 +731,10 @@ func GetTagsForDID(db DBTX, did string) ([]struct{ Repository, Tag string }, err // DeleteTagsNotInList deletes all tags for a DID that are not in the provided list. // Atomicity is provided by the caller's transaction when used during backfill. +// +// Tags are identified by (did, repository, tag), which is their natural key and +// the table's primary key. This used to select surrogate ids, filter them in Go +// with a nested loop, and then issue one DELETE per row. func DeleteTagsNotInList(db DBTX, did string, keepTags []struct{ Repository, Tag string }) error { if len(keepTags) == 0 { // No tags to keep - delete all for this DID @@ -738,39 +742,52 @@ func DeleteTagsNotInList(db DBTX, did string, keepTags []struct{ Repository, Tag return err } - // First, get all current tags - rows, err := db.Query(`SELECT id, repository, tag FROM tags WHERE did = ?`, did) + // Work out what to remove in Go rather than with a NOT IN over the keep + // list. A single NOT IN would need two placeholders per kept tag and break + // past the driver's parameter ceiling for a user with enough tags, and it + // cannot be split into chunks: each chunk would delete the tags every other + // chunk meant to keep. + keep := make(map[struct{ Repository, Tag string }]struct{}, len(keepTags)) + for _, k := range keepTags { + keep[k] = struct{}{} + } + + rows, err := db.Query(`SELECT repository, tag FROM tags WHERE did = ?`, did) if err != nil { return err } - - var toDelete []int64 + var toDelete []struct{ Repository, Tag string } for rows.Next() { - var id int64 - var repo, tag string - if err := rows.Scan(&id, &repo, &tag); err != nil { + var t struct{ Repository, Tag string } + if err := rows.Scan(&t.Repository, &t.Tag); err != nil { rows.Close() return err } - - // Check if this tag should be kept - found := false - for _, keep := range keepTags { - if keep.Repository == repo && keep.Tag == tag { - found = true - break - } - } - - if !found { - toDelete = append(toDelete, id) + if _, ok := keep[t]; !ok { + toDelete = append(toDelete, t) } } rows.Close() + if err := rows.Err(); err != nil { + return err + } - // Delete tags not in keep list - for _, id := range toDelete { - if _, err := db.Exec(`DELETE FROM tags WHERE id = ?`, id); err != nil { + // An explicit delete list chunks safely, unlike the NOT IN above. + for i := 0; i*BatchSize < len(toDelete); i++ { + start, end := chunk(len(toDelete), i) + batch := toDelete[start:end] + + args := make([]any, 0, 1+2*len(batch)) + args = append(args, did) + pairs := make([]string, 0, len(batch)) + for _, t := range batch { + pairs = append(pairs, "(?, ?)") + args = append(args, t.Repository, t.Tag) + } + + query := `DELETE FROM tags WHERE did = ? AND (repository, tag) IN (` + + strings.Join(pairs, ", ") + `)` + if _, err := db.Exec(query, args...); err != nil { return err } } @@ -962,7 +979,7 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi query := ` WITH paged_tags AS ( - SELECT t.id, t.did, t.repository, t.tag, t.digest, t.created_at + SELECT t.did, t.repository, t.tag, t.digest, t.created_at FROM tags t JOIN manifests m ON t.did = m.did AND t.repository = m.repository AND t.digest = m.digest WHERE t.did = ? AND t.repository = ? @@ -972,7 +989,6 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi LIMIT ? OFFSET ? ) SELECT - t.id, t.did, t.repository, t.tag, @@ -1013,7 +1029,7 @@ func getTagsWithPlatformsFiltered(db DBTX, did, repository, tagName string, limi var childDigest, childHoldEndpoint string var compressedSize int64 - if err := rows.Scan(&t.ID, &t.DID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt, + if err := rows.Scan(&t.DID, &t.Repository, &t.Tag, &t.Digest, &t.CreatedAt, &mediaType, &artifactType, &holdEndpoint, &platformOS, &platformArch, &platformVariant, &platformOSVersion, &isAttestation, &childDigest, &childHoldEndpoint, &compressedSize); err != nil { @@ -1991,7 +2007,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) { // Get tags for this repo tagRows, err := db.Query(` - SELECT id, tag, digest, created_at + SELECT tag, digest, created_at FROM tags WHERE did = ? AND repository = ? ORDER BY created_at DESC @@ -2005,7 +2021,7 @@ func GetRepository(db DBTX, did, repository string) (*Repository, error) { var t Tag t.DID = did t.Repository = repository - if err := tagRows.Scan(&t.ID, &t.Tag, &t.Digest, &t.CreatedAt); err != nil { + if err := tagRows.Scan(&t.Tag, &t.Digest, &t.CreatedAt); err != nil { tagRows.Close() return nil, err } diff --git a/pkg/appview/db/schema.sql b/pkg/appview/db/schema.sql index ba2d061..78e2c54 100644 --- a/pkg/appview/db/schema.sql +++ b/pkg/appview/db/schema.sql @@ -82,17 +82,21 @@ CREATE TABLE IF NOT EXISTS manifest_references ( ); CREATE INDEX IF NOT EXISTS idx_manifest_references_digest ON manifest_references(digest); +-- Keyed by its natural key. There is no surrogate id: nothing joined on it, and +-- an AUTOINCREMENT rowid is allocated by whichever node does the insert, which +-- stops being a stable identity as soon as writes are not funnelled through a +-- single writer. CREATE TABLE IF NOT EXISTS tags ( - id INTEGER PRIMARY KEY AUTOINCREMENT, did TEXT NOT NULL, repository TEXT NOT NULL, tag TEXT NOT NULL, digest TEXT NOT NULL, created_at TIMESTAMP NOT NULL, - UNIQUE(did, repository, tag), + PRIMARY KEY(did, repository, tag), FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE ); -CREATE INDEX IF NOT EXISTS idx_tags_did_repo ON tags(did, repository); +-- No separate (did, repository) index: the primary key already indexes that as a +-- prefix. It was needed only while the primary key was the surrogate id. -- rev increments on every successful write, so a writer that read revision N can -- tell whether anyone has written since. Refresh tokens rotate on use, and the diff --git a/pkg/appview/db/tag_natural_key_test.go b/pkg/appview/db/tag_natural_key_test.go new file mode 100644 index 0000000..45a7677 --- /dev/null +++ b/pkg/appview/db/tag_natural_key_test.go @@ -0,0 +1,281 @@ +package db + +import ( + "database/sql" + "fmt" + "testing" + "time" +) + +// seedTags inserts n tags under one repository and returns the user's DID. +func seedTags(t *testing.T, database *sql.DB, repository string, n int) string { + t.Helper() + + const did = "did:plc:tagowner" + if err := UpsertUser(database, &User{ + DID: did, + Handle: "tagowner.example.com", + PDSEndpoint: "https://pds.example.com", + LastSeen: time.Now(), + }); err != nil { + t.Fatalf("UpsertUser: %v", err) + } + + for i := range n { + tag := fmt.Sprintf("v%d", i) + if err := UpsertTag(database, &Tag{ + DID: did, + Repository: repository, + Tag: tag, + Digest: fmt.Sprintf("sha256:%064d", i), + CreatedAt: time.Now(), + }); err != nil { + t.Fatalf("UpsertTag %s: %v", tag, err) + } + } + return did +} + +// TestDeleteTagsNotInListSpansChunks exercises the chunked delete. +// +// The deletions are batched at BatchSize to keep the placeholder count under the +// driver's parameter ceiling, so the interesting case is more tags than fit in +// one chunk. Getting the chunking wrong here deletes tags that should have been +// kept, which during backfill silently destroys a user's tag list. +func TestDeleteTagsNotInListSpansChunks(t *testing.T) { + database := revTestDB(t) + + const ( + repository = "myapp" + total = BatchSize*2 + 37 // spans three chunks, last one partial + ) + did := seedTags(t, database, repository, total) + + // Keep every tenth tag; delete the rest. + var keep []struct{ Repository, Tag string } + kept := map[string]bool{} + for i := 0; i < total; i += 10 { + tag := fmt.Sprintf("v%d", i) + keep = append(keep, struct{ Repository, Tag string }{repository, tag}) + kept[tag] = true + } + + if err := DeleteTagsNotInList(database, did, keep); err != nil { + t.Fatalf("DeleteTagsNotInList: %v", err) + } + + remaining, err := GetTagsForDID(database, did) + if err != nil { + t.Fatalf("GetTagsForDID: %v", err) + } + if len(remaining) != len(keep) { + t.Errorf("kept %d tags, want %d", len(remaining), len(keep)) + } + for _, r := range remaining { + if !kept[r.Tag] { + t.Errorf("tag %q survived but was not in the keep list", r.Tag) + } + } + for tag := range kept { + found := false + for _, r := range remaining { + if r.Tag == tag { + found = true + break + } + } + if !found { + t.Errorf("tag %q was in the keep list but got deleted", tag) + } + } +} + +// TestDeleteTagsNotInListEmptyKeepListDeletesAll pins the documented shortcut. +func TestDeleteTagsNotInListEmptyKeepListDeletesAll(t *testing.T) { + database := revTestDB(t) + did := seedTags(t, database, "myapp", 5) + + if err := DeleteTagsNotInList(database, did, nil); err != nil { + t.Fatalf("DeleteTagsNotInList: %v", err) + } + + remaining, err := GetTagsForDID(database, did) + if err != nil { + t.Fatalf("GetTagsForDID: %v", err) + } + if len(remaining) != 0 { + t.Errorf("expected every tag to be deleted, %d remain", len(remaining)) + } +} + +// TestDeleteTagsNotInListScopedToDID: one user's backfill must not delete +// another user's tags. The DID is part of the natural key, so this is really a +// check that the key is applied and not just the (repository, tag) pair. +func TestDeleteTagsNotInListScopedToDID(t *testing.T) { + database := revTestDB(t) + did := seedTags(t, database, "myapp", 3) + + const otherDID = "did:plc:someoneelse" + if err := UpsertUser(database, &User{ + DID: otherDID, + Handle: "other.example.com", + PDSEndpoint: "https://pds.example.com", + LastSeen: time.Now(), + }); err != nil { + t.Fatalf("UpsertUser: %v", err) + } + if err := UpsertTag(database, &Tag{ + DID: otherDID, + Repository: "myapp", + Tag: "v0", + Digest: "sha256:other", + CreatedAt: time.Now(), + }); err != nil { + t.Fatalf("UpsertTag: %v", err) + } + + // Wipe every tag belonging to the first user. + if err := DeleteTagsNotInList(database, did, nil); err != nil { + t.Fatalf("DeleteTagsNotInList: %v", err) + } + + otherTags, err := GetTagsForDID(database, otherDID) + if err != nil { + t.Fatalf("GetTagsForDID: %v", err) + } + if len(otherTags) != 1 { + t.Errorf("another user's tags were affected: %d remain, want 1", len(otherTags)) + } +} + +// TestTagNaturalKeyRejectsDuplicates: (did, repository, tag) is the primary key +// now, so a repeated upsert must update in place rather than adding a row. +func TestTagNaturalKeyRejectsDuplicates(t *testing.T) { + database := revTestDB(t) + did := seedTags(t, database, "myapp", 1) + + if err := UpsertTag(database, &Tag{ + DID: did, + Repository: "myapp", + Tag: "v0", + Digest: "sha256:updated", + CreatedAt: time.Now(), + }); err != nil { + t.Fatalf("UpsertTag: %v", err) + } + + var count int + if err := database.QueryRow( + `SELECT COUNT(*) FROM tags WHERE did = ? AND repository = ? AND tag = ?`, + did, "myapp", "v0", + ).Scan(&count); err != nil { + t.Fatalf("count: %v", err) + } + if count != 1 { + t.Errorf("expected the upsert to replace the row, found %d rows", count) + } +} + +// TestMigration0032PreservesTagRows runs the rebuild migration against a table +// in the OLD shape and checks every row survives. +// +// A rebuild migration is the one kind that can silently lose data: it copies +// rows between tables, and a mistake in the INSERT ... SELECT shows up as +// missing or misplaced values rather than an error. The drift test proves the +// resulting *shape* is right but says nothing about the contents. +func TestMigration0032PreservesTagRows(t *testing.T) { + database := revTestDB(t) + + // Rebuild the old shape: surrogate id, UNIQUE rather than PRIMARY KEY. + oldShape := []string{ + `DROP TABLE tags`, + `CREATE TABLE tags ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + did TEXT NOT NULL, + repository TEXT NOT NULL, + tag TEXT NOT NULL, + digest TEXT NOT NULL, + created_at TIMESTAMP NOT NULL, + UNIQUE(did, repository, tag), + FOREIGN KEY(did) REFERENCES users(did) ON DELETE CASCADE + )`, + `CREATE INDEX idx_tags_did_repo ON tags(did, repository)`, + } + for _, stmt := range oldShape { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("set up the old tags shape: %v", err) + } + } + + if err := UpsertUser(database, &User{ + DID: "did:plc:tagowner", + Handle: "tagowner.example.com", + PDSEndpoint: "https://pds.example.com", + LastSeen: time.Now(), + }); err != nil { + t.Fatalf("UpsertUser: %v", err) + } + + type row struct{ repo, tag, digest string } + want := []row{ + {"myapp", "latest", "sha256:aaa"}, + {"myapp", "v1.0.0", "sha256:bbb"}, + {"otherapp", "latest", "sha256:ccc"}, + } + for _, r := range want { + if _, err := database.Exec( + `INSERT INTO tags (did, repository, tag, digest, created_at) VALUES (?, ?, ?, ?, ?)`, + "did:plc:tagowner", r.repo, r.tag, r.digest, time.Now(), + ); err != nil { + t.Fatalf("seed %s/%s: %v", r.repo, r.tag, err) + } + } + + // Run the migration body itself, not a paraphrase of it. + migrations, err := loadMigrations() + if err != nil { + t.Fatalf("loadMigrations: %v", err) + } + var query string + for _, m := range migrations { + if m.Version == 32 { + query = m.Query + } + } + if query == "" { + t.Fatal("migration 0032 not found") + } + for i, stmt := range splitSQLStatements(query) { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("migration 0032 statement %d: %v\n%s", i+1, err, stmt) + } + } + + for _, r := range want { + var digest string + err := database.QueryRow( + `SELECT digest FROM tags WHERE did = ? AND repository = ? AND tag = ?`, + "did:plc:tagowner", r.repo, r.tag, + ).Scan(&digest) + if err != nil { + t.Errorf("%s/%s did not survive the rebuild: %v", r.repo, r.tag, err) + continue + } + if digest != r.digest { + t.Errorf("%s/%s digest = %q, want %q (columns are misaligned)", r.repo, r.tag, digest, r.digest) + } + } + + var count int + if err := database.QueryRow(`SELECT COUNT(*) FROM tags`).Scan(&count); err != nil { + t.Fatalf("count: %v", err) + } + if count != len(want) { + t.Errorf("tag count = %d after the rebuild, want %d", count, len(want)) + } + + // The surrogate id must be gone. + if _, err := database.Query(`SELECT id FROM tags LIMIT 1`); err == nil { + t.Error("tags.id still exists after the migration") + } +}