mirror of
https://tangled.org/evan.jarrett.net/at-container-registry
synced 2026-09-02 00:06:58 +00:00
db: cover the recency change on all four surfaces, not one
The MAX(id) replacement touched four queries that each carry their own copy of the same CTE: SearchRepositories, GetRepoCards, GetUserRepoCards and GetStarredRepoCards. Only the third had a test. Fixing one and missing another would leave the UI disagreeing with itself about which manifest is current, depending on which page you were looking at. All four now assert that recency follows created_at rather than insert order, and that a tie between manifests pushed in the same second resolves the same way on every surface. Verified by regressing the CTEs back to rowid ordering: each of the four fails independently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
454a6bad3d
commit
e9d43aa767
@@ -225,10 +225,10 @@ func reshapeToPre0034WithChildren(t *testing.T, database *sql.DB) {
|
||||
}
|
||||
|
||||
// TestLatestManifestUsesCreatedAtNotInsertOrder pins the recency change that
|
||||
// dropping id forced.
|
||||
// dropping id forced, across every query that makes it.
|
||||
//
|
||||
// Repo cards, search results and starred repos all needed "the newest manifest
|
||||
// in this repo", and all five queries answered it with MAX(id) — the rowid as a
|
||||
// Repo cards, search, user pages and starred repos all needed "the newest
|
||||
// manifest in this repo", and all four answered it with MAX(id) — the rowid as a
|
||||
// proxy for insert order. A derived key has no ordering, so recency now comes
|
||||
// from created_at, with manifest_key as a deterministic tiebreak.
|
||||
//
|
||||
@@ -236,6 +236,10 @@ func reshapeToPre0034WithChildren(t *testing.T, database *sql.DB) {
|
||||
// backfill does routinely: it walks a PDS and inserts whatever it finds, so the
|
||||
// last row inserted is not the most recently pushed. created_at is the push
|
||||
// time, and is the answer these queries always wanted.
|
||||
//
|
||||
// All four are covered because they carry four separate copies of the same CTE;
|
||||
// fixing one and missing another would leave the UI disagreeing with itself
|
||||
// about which manifest is current.
|
||||
func TestLatestManifestUsesCreatedAtNotInsertOrder(t *testing.T) {
|
||||
database := revTestDB(t)
|
||||
did := manifestKeyTestUser(t, database)
|
||||
@@ -255,21 +259,50 @@ func TestLatestManifestUsesCreatedAtNotInsertOrder(t *testing.T) {
|
||||
t.Fatalf("insert older: %v", err)
|
||||
}
|
||||
|
||||
cards, err := GetUserRepoCards(database, did, "")
|
||||
if err != nil {
|
||||
t.Fatalf("GetUserRepoCards: %v", err)
|
||||
// Star it so the starred-repos query has something to return.
|
||||
if err := UpsertStar(database, did, did, "myapp", now); err != nil {
|
||||
t.Fatalf("UpsertStar: %v", err)
|
||||
}
|
||||
if len(cards) != 1 {
|
||||
t.Fatalf("expected 1 repo card, got %d", len(cards))
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
run func() ([]RepoCardData, error)
|
||||
}{
|
||||
{"GetUserRepoCards", func() ([]RepoCardData, error) {
|
||||
return GetUserRepoCards(database, did, "")
|
||||
}},
|
||||
{"GetRepoCards", func() ([]RepoCardData, error) {
|
||||
return GetRepoCards(database, 10, "", SortByLastUpdate)
|
||||
}},
|
||||
{"GetStarredRepoCards", func() ([]RepoCardData, error) {
|
||||
return GetStarredRepoCards(database, did, "")
|
||||
}},
|
||||
{"SearchRepositories", func() ([]RepoCardData, error) {
|
||||
cards, _, err := SearchRepositories(database, "myapp", 10, 0, "")
|
||||
return cards, err
|
||||
}},
|
||||
}
|
||||
if cards[0].Digest != "sha256:newer" {
|
||||
t.Errorf("repo card shows digest %q, want the most recently created manifest sha256:newer; "+
|
||||
"recency is following insert order rather than created_at", cards[0].Digest)
|
||||
|
||||
for _, c := range cases {
|
||||
t.Run(c.name, func(t *testing.T) {
|
||||
cards, err := c.run()
|
||||
if err != nil {
|
||||
t.Fatalf("%s: %v", c.name, err)
|
||||
}
|
||||
if len(cards) != 1 {
|
||||
t.Fatalf("%s returned %d cards, want 1", c.name, len(cards))
|
||||
}
|
||||
if cards[0].Digest != "sha256:newer" {
|
||||
t.Errorf("%s shows digest %q, want the most recently created manifest sha256:newer; "+
|
||||
"recency is following insert order rather than created_at", c.name, cards[0].Digest)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestLatestManifestTieBreakIsDeterministic: two manifests pushed in the same
|
||||
// second must not make the card flip between them from query to query.
|
||||
// second must not make the card flip between them from query to query, or from
|
||||
// one surface to another.
|
||||
func TestLatestManifestTieBreakIsDeterministic(t *testing.T) {
|
||||
database := revTestDB(t)
|
||||
did := manifestKeyTestUser(t, database)
|
||||
@@ -283,8 +316,12 @@ func TestLatestManifestTieBreakIsDeterministic(t *testing.T) {
|
||||
t.Fatalf("insert %s: %v", digest, err)
|
||||
}
|
||||
}
|
||||
if err := UpsertStar(database, did, did, "myapp", same); err != nil {
|
||||
t.Fatalf("UpsertStar: %v", err)
|
||||
}
|
||||
|
||||
var first string
|
||||
// Repeated runs must agree...
|
||||
var want string
|
||||
for i := range 5 {
|
||||
cards, err := GetUserRepoCards(database, did, "")
|
||||
if err != nil {
|
||||
@@ -294,11 +331,35 @@ func TestLatestManifestTieBreakIsDeterministic(t *testing.T) {
|
||||
t.Fatalf("expected 1 repo card, got %d", len(cards))
|
||||
}
|
||||
if i == 0 {
|
||||
first = cards[0].Digest
|
||||
want = cards[0].Digest
|
||||
continue
|
||||
}
|
||||
if cards[0].Digest != first {
|
||||
t.Fatalf("tie broken differently between runs: %q then %q", first, cards[0].Digest)
|
||||
if cards[0].Digest != want {
|
||||
t.Fatalf("tie broken differently between runs: %q then %q", want, cards[0].Digest)
|
||||
}
|
||||
}
|
||||
|
||||
// ...and so must the other three surfaces, or the same repo shows a
|
||||
// different "current" manifest depending on which page you are looking at.
|
||||
others := map[string]func() ([]RepoCardData, error){
|
||||
"GetRepoCards": func() ([]RepoCardData, error) { return GetRepoCards(database, 10, "", SortByLastUpdate) },
|
||||
"GetStarredRepoCards": func() ([]RepoCardData, error) { return GetStarredRepoCards(database, did, "") },
|
||||
"SearchRepositories": func() ([]RepoCardData, error) {
|
||||
cards, _, err := SearchRepositories(database, "myapp", 10, 0, "")
|
||||
return cards, err
|
||||
},
|
||||
}
|
||||
for name, run := range others {
|
||||
cards, err := run()
|
||||
if err != nil {
|
||||
t.Fatalf("%s: %v", name, err)
|
||||
}
|
||||
if len(cards) != 1 {
|
||||
t.Fatalf("%s returned %d cards, want 1", name, len(cards))
|
||||
}
|
||||
if cards[0].Digest != want {
|
||||
t.Errorf("%s picked %q but GetUserRepoCards picked %q; the tiebreak differs between surfaces",
|
||||
name, cards[0].Digest, want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user