fix(hub): deterministic order for public share links (BEA-30) (#75)

ShareDB.List ranged over a map, so the project Settings → Public links
table and the org-wide share audit came back in a different order on
every load — with a Revoke button on each row. Sort in List (Created
desc, then Path, then Token) so both surfaces inherit one total order.

Created.Equal rather than !=: a time.Time carries a monotonic reading
and a location, so two logically-equal instants can compare unequal,
which would make the comparator non-transitive.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Snow W. Lee (Sungwon)
2026-07-29 17:07:23 +09:00
committed by GitHub
co-authored by Claude Opus 5
parent acb99e85e3
commit 6da3c7957e
2 changed files with 105 additions and 1 deletions
+18 -1
View File
@@ -7,6 +7,7 @@ import (
"io"
"net/http"
"path"
"sort"
"strings"
"sync"
"time"
@@ -140,7 +141,10 @@ func (db *ShareDB) SetExpiry(token string, ttl time.Duration) (Share, bool, erro
return s, true, nil
}
// List returns a project's live shares.
// List returns a project's live shares, newest first. The order has to be a
// total one: byToken is a map, so without a sort every call reshuffles the
// rows — and these rows carry a Revoke button, so "the second one" must mean
// the same link on every load.
func (db *ShareDB) List(project string) []Share {
db.mu.Lock()
defer db.mu.Unlock()
@@ -150,6 +154,19 @@ func (db *ShareDB) List(project string) []Share {
out = append(out, s)
}
}
sort.Slice(out, func(i, j int) bool {
// Equal, not !=: a time.Time carries a monotonic reading and a
// location, so two logically-equal instants can compare unequal —
// which would make this comparator non-transitive and leave the
// order worse than the map iteration it replaces.
if !out[i].Created.Equal(out[j].Created) {
return out[i].Created.After(out[j].Created)
}
if out[i].Path != out[j].Path {
return out[i].Path < out[j].Path
}
return out[i].Token < out[j].Token
})
return out
}
+87
View File
@@ -6,6 +6,7 @@ import (
"net/http"
"net/http/httptest"
"path/filepath"
"reflect"
"strings"
"testing"
"time"
@@ -276,6 +277,92 @@ func TestShareMarkdownRendersAndExpires(t *testing.T) {
}
}
// A Revoke button sits on every row of the public-links table, so the row
// order must not move between loads. byToken is a map, so List has to sort.
func TestShareListIsDeterministic(t *testing.T) {
db, err := OpenShareDB(filepath.Join(t.TempDir(), "shares.json"))
if err != nil {
t.Fatal(err)
}
// Hand-set Created — Create stamps time.Now(), which never collides, so
// going through it would leave the tie-break path untested.
tick := time.Date(2026, 7, 28, 12, 0, 0, 0, time.UTC)
for _, s := range []Share{
{Token: "t3", Project: "p", Path: "z.md", Created: tick},
{Token: "t2", Project: "p", Path: "a.md", Created: tick}, // same instant as t3
{Token: "t1", Project: "p", Path: "newest.md", Created: tick.Add(time.Hour)},
{Token: "t0", Project: "p", Path: "oldest.md", Created: tick.Add(-time.Hour)},
{Token: "x", Project: "other", Path: "a.md", Created: tick},
{Token: "dead", Project: "p", Path: "gone.md", Created: tick, Expires: tick},
} {
db.byToken[s.Token] = s
if err := db.repo.Put(s); err != nil {
t.Fatal(err)
}
}
first := db.List("p")
want := []string{"t1", "t2", "t3", "t0"} // created desc, then path asc (a.md < z.md)
got := make([]string, len(first))
for i, s := range first {
got[i] = s.Token
}
if !reflect.DeepEqual(got, want) {
t.Fatalf("order = %v, want %v (newest first, path tie-break)", got, want)
}
for i := 0; i < 10; i++ {
if !reflect.DeepEqual(db.List("p"), first) {
t.Fatalf("call %d reordered: %v vs %v", i, db.List("p"), first)
}
}
// A project with no shares still returns nil, not an empty slice.
if db.List("nobody") != nil {
t.Fatal("empty project should list as nil")
}
}
// Both share-listing APIs — the project settings table and the org-wide audit
// — must hand back the same bytes on consecutive reads.
func TestShareListAPIsAreStable(t *testing.T) {
h, srv, c, p := permHub(t)
// Mint directly: the HTTP route requires a synced file, and this test is
// about ordering, not about the mint path.
for _, path := range []string{"b.md", "a.md", "c.md"} {
if _, err := srv.Shares.Create(p.ID, path, "alice@x.io", 0); err != nil {
t.Fatal(err)
}
}
for _, url := range []string{"/api/p/" + p.ID + "/shares", "/api/orgs/" + p.Org + "/shares"} {
first := doAs(t, h, "GET", url, nil, c["alice"])
if first.Code != 200 {
t.Fatalf("GET %s: %d %s", url, first.Code, first.Body)
}
if !strings.Contains(first.Body.String(), "a.md") {
t.Fatalf("GET %s listed no shares: %s", url, first.Body)
}
for i := 0; i < 5; i++ {
again := doAs(t, h, "GET", url, nil, c["alice"])
if again.Body.String() != first.Body.String() {
t.Fatalf("GET %s moved between loads:\n%s\n%s", url, first.Body, again.Body)
}
}
}
// The newest link is the first row — what you just minted is what you are
// most likely to want to undo.
rec := doAs(t, h, "GET", "/api/p/"+p.ID+"/shares", nil, c["alice"])
var out struct {
Shares []struct {
Path string `json:"path"`
} `json:"shares"`
}
if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
t.Fatal(err)
}
if len(out.Shares) != 3 || out.Shares[0].Path != "c.md" {
t.Fatalf("newest share is not first: %s", rec.Body)
}
}
// helpers
func jsonReq(t *testing.T, method, url string, body any) *http.Request {