From 74389d81f613b44eb35947ccfbea57aea6bdb8e6 Mon Sep 17 00:00:00 2001 From: david <58231312+davseby@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:38:57 +0300 Subject: [PATCH] prepare document history table for reinstantiating documents --- server/AGENTS.md | 9 +- server/core/internal/db/db.go | 6 + server/core/internal/db/db_test.go | 146 +++++----- server/core/internal/db/document.go | 6 +- server/core/internal/db/document_branch.go | 131 +++++++-- .../core/internal/db/document_branch_test.go | 232 +++++++++++----- server/core/internal/db/document_file.go | 34 +-- server/core/internal/db/document_file_test.go | 28 +- server/core/internal/db/document_test.go | 39 --- .../migrations/002_history_entry_snapshot.sql | 39 +++ server/core/internal/document/AGENTS.md | 35 ++- server/core/internal/document/document.go | 64 ++--- .../core/internal/document/document_test.go | 69 +++-- server/core/internal/document/file/file.go | 5 +- .../core/internal/document/history/history.go | 134 +++++++++ .../internal/document/history/history_test.go | 193 +++++++++++++ server/core/internal/document/node.go | 46 ++-- server/core/internal/document/node_test.go | 31 ++- server/core/internal/server/0mock_db_test.go | 54 ++++ .../server/internal/document/0mock_db_test.go | 54 ++++ .../server/internal/document/0mock_tx_test.go | 54 ++++ .../server/internal/document/branch_http.go | 52 ++++ .../internal/document/branch_http_test.go | 256 ++++++++++++++++++ .../server/internal/document/document_http.go | 4 + .../internal/document/document_http_test.go | 8 +- .../server/internal/document/files/http.go | 11 +- .../internal/document/files/http_test.go | 16 ++ .../editor/blocks/file/FileBlock.nuxt.test.ts | 33 +-- .../editor/blocks/file/FileBlock.vue | 7 +- .../blocks/image/ImageBlock.nuxt.test.ts | 33 +-- .../editor/blocks/image/ImageBlock.vue | 8 +- .../editor/blocks/upload-handler.test.ts | 30 +- .../editor/blocks/upload-handler.ts | 5 +- 33 files changed, 1416 insertions(+), 456 deletions(-) create mode 100644 server/core/internal/db/migrations/002_history_entry_snapshot.sql create mode 100644 server/core/internal/document/history/history.go create mode 100644 server/core/internal/document/history/history_test.go diff --git a/server/AGENTS.md b/server/AGENTS.md index 000c4446..7a43c3f5 100644 --- a/server/AGENTS.md +++ b/server/AGENTS.md @@ -96,11 +96,10 @@ The README lists the public routes; update it when changing handlers. ## Database Postgres 18. Migrations are embedded from `core/internal/db/migrations/` and -applied by `db.New` (`rubenv/sql-migrate`). **Until the first release there -is only `001_initial.sql` and schema changes go into it in place**; -afterwards, add `NNN_.sql`. An existing dev volume keeps the old -schema, so reset it with `docker compose -p oxynote -f -docker/docker-compose.dev.yaml down -v` after editing. +applied by `db.New` (`rubenv/sql-migrate`). **The system is live: every +schema change is a new `NNN_.sql`; an applied migration is never +edited.** A dev volume behind on migrations can be reset with `docker +compose -p oxynote -f docker/docker-compose.dev.yaml down -v`. **Core migrations own the Better Auth tables** (snake_case columns matching the `fields` mappings in `auth-realtime/src/auth.ts`). diff --git a/server/core/internal/db/db.go b/server/core/internal/db/db.go index a4a13e84..7c7d9f01 100644 --- a/server/core/internal/db/db.go +++ b/server/core/internal/db/db.go @@ -236,6 +236,12 @@ func DetectError(err error) error { ) case "tags_fk_organization_id_tag_name_key": return tag.ErrDuplicateTagName + case "document_files_pkey": + return errutil.New( + http.StatusConflict, + "document_file.exists", + "file id is already in use", + ) } } diff --git a/server/core/internal/db/db_test.go b/server/core/internal/db/db_test.go index 8d5e87c4..42f8ed2e 100644 --- a/server/core/internal/db/db_test.go +++ b/server/core/internal/db/db_test.go @@ -124,80 +124,104 @@ func Test_DB_Close(t *testing.T) { } func Test_DetectError(t *testing.T) { - cc := map[string]struct { + type tcase struct { Err error Result error - }{ - "Not found": { - Err: sql.ErrNoRows, - Result: errutil.ErrNotFound, + } + + cc := map[string]func(*testing.T) tcase{ + "Not found": func(*testing.T) tcase { + return tcase{ + Err: sql.ErrNoRows, + Result: errutil.ErrNotFound, + } }, - "Error pass through": { - Err: sql.ErrConnDone, - Result: sql.ErrConnDone, + "Error pass through": func(*testing.T) tcase { + return tcase{ + Err: sql.ErrConnDone, + Result: sql.ErrConnDone, + } }, - "No error": { - Err: nil, - Result: nil, + "No error": func(*testing.T) tcase { + return tcase{} }, - "Unmatched pgx constraint": { - Err: &pgconn.PgError{ - Code: "123", - }, - Result: &pgconn.PgError{ - Code: "123", - }, + "Unmatched pgx constraint": func(*testing.T) tcase { + return tcase{ + Err: &pgconn.PgError{ + Code: "123", + }, + Result: &pgconn.PgError{ + Code: "123", + }, + } }, - "Unmatched constraint": { - Err: errors.New("b"), - Result: errors.New("b"), + "Unmatched constraint": func(*testing.T) tcase { + return tcase{ + Err: errors.New("b"), + Result: errors.New("b"), + } + }, + "Duplicate branch name": func(t *testing.T) tcase { + db := prepTempDB(t) + + // renaming a branch onto a sibling's name trips the unique key + // that DetectError has to recognise. + branches := prepDocumentBranches(t, db, 2, nil) + + second := branches[1] + second.BranchName = branches[0].BranchName + + err := db.UpdateDocumentBranchMetadata(context.Background(), *second) + require.Error(t, err) + + return tcase{ + Err: err, + Result: errutil.New(http.StatusBadRequest, "document_branch.duplicate_name", "branch name is already in use"), + } + }, + "Duplicate file id": func(t *testing.T) tcase { + db := prepTempDB(t) + + f := prepDocumentFiles(t, db, 1, nil)[0] + + err := db.InsertDocumentFile(context.Background(), f) + require.Error(t, err) + + return tcase{ + Err: err, + Result: errutil.New(http.StatusConflict, "document_file.exists", "file id is already in use"), + } + }, + "Duplicate tag name": func(t *testing.T) tcase { + db := prepTempDB(t) + + existing := prepTags(t, db, 1, nil)[0] + + dup := tag.NewTag( + tag.CreateInput{TagName: existing.TagName, Color: "#000000"}, + existing.OrganizationID, + prepUsers(t, db, 1)[0], + ) + + err := db.InsertTag(context.Background(), dup) + require.Error(t, err) + + return tcase{ + Err: err, + Result: tag.ErrDuplicateTagName, + } }, } - for cn, c := range cc { + for cn, cfn := range cc { t.Run(cn, func(t *testing.T) { + t.Parallel() + + c := cfn(t) + assert.Equal(t, c.Result, DetectError(c.Err)) }) } - - t.Run("Duplicate branch name", func(t *testing.T) { - t.Parallel() - - db := prepTempDB(t) - - // renaming a branch onto a sibling's name trips the unique key that - // DetectError has to recognise. - branches := prepDocumentBranches(t, db, 2, nil) - - second := branches[1] - second.BranchName = branches[0].BranchName - - err := db.UpdateDocumentBranchMetadata(context.Background(), *second) - require.Error(t, err) - - mapped := DetectError(err) - assert.Equal(t, http.StatusBadRequest, errutil.StatusCode(mapped, false)) - assert.Contains(t, mapped.Error(), "branch name is already in use") - }) - - t.Run("Duplicate tag name", func(t *testing.T) { - t.Parallel() - - db := prepTempDB(t) - - existing := prepTags(t, db, 1, nil)[0] - - dup := tag.NewTag( - tag.CreateInput{TagName: existing.TagName, Color: "#000000"}, - existing.OrganizationID, - prepUsers(t, db, 1)[0], - ) - - err := db.InsertTag(context.Background(), dup) - require.Error(t, err) - - assert.Equal(t, tag.ErrDuplicateTagName, DetectError(err)) - }) } func Test_DB_BeginTx(t *testing.T) { diff --git a/server/core/internal/db/document.go b/server/core/internal/db/document.go index 82e80ca8..2c8a6884 100644 --- a/server/core/internal/db/document.go +++ b/server/core/internal/db/document.go @@ -252,11 +252,7 @@ func (a *agent) UpdateDocument(ctx context.Context, doc document.Document) error return err } - if err := a.upsertDocumentBranch(ctx, tx, doc); err != nil { - return err - } - - return a.insertDocumentBranchHistoryEntry(ctx, tx, doc.ID, doc.BranchID, doc.HistoryEntry()) + return a.upsertDocumentBranch(ctx, tx, doc) }) } diff --git a/server/core/internal/db/document_branch.go b/server/core/internal/db/document_branch.go index 01166897..370a0e4c 100644 --- a/server/core/internal/db/document_branch.go +++ b/server/core/internal/db/document_branch.go @@ -2,13 +2,22 @@ package db import ( "context" + "database/sql" + "errors" + "time" sq "github.com/Masterminds/squirrel" "github.com/jmoiron/sqlx" "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/history" + "github.com/oxynote/oxynote/server/core/pkg/sqlutil" "github.com/rs/xid" ) +// _historyAggregationDuration is the length of the time bucket that +// ordinary history entries of one branch collapse into. +var _historyAggregationDuration = 30 * time.Minute + // insertDocumentBranch inserts a branch row for a newly created document. // Called inside the same transaction as InsertDocument. func (a *agent) insertDocumentBranch(ctx context.Context, tx *sqlx.Tx, doc document.Document) error { @@ -250,38 +259,91 @@ func (a *agent) selectBranchSummary(b sq.SelectBuilder) sq.SelectBuilder { ).From("document_branches") } -// insertDocumentBranchHistoryEntry inserts a history entry for a branch update -// through the given executor. The entry is keyed by branch id rather than -// branch name so that renaming a branch cannot break the reference. -func (a *agent) insertDocumentBranchHistoryEntry( - ctx context.Context, - ex sqlx.ExecerContext, - docID, branchID xid.ID, - entry document.HistoryEntry, -) error { - q, args := a.builder.Insert("document_branch_history_entries"). - SetMap(map[string]any{ - "id": entry.ID, - "fk_document_id": docID, - "fk_branch_id": branchID, - "content": entry.Content, - "raw_content": entry.RawContent, - "created_at": entry.CreatedAt, - }). - Suffix("ON CONFLICT (id) DO UPDATE SET " + - "content = excluded.content, raw_content = excluded.raw_content, created_at = excluded.created_at"). +// InsertDocumentBranchHistoryEntry records a snapshot of a branch. Ordinary +// entries aggregate: when the branch's newest entry is an ordinary one from +// the same time bucket, that entry is updated in place instead of a row +// being added. A boundary entry (merge, fork) is always its own row and +// closes the bucket, so the state right before it survives and the next +// edit starts a new entry. The count trim runs afterwards; age trimming +// lives in the file manager, since it has to reach branches that stopped +// inserting altogether. +func (a *agent) InsertDocumentBranchHistoryEntry(ctx context.Context, entry history.Entry) error { + return sqlutil.WrapTx(ctx, a.sql, func(tx *sqlx.Tx) error { + newest, err := a.fetchNewestDocumentBranchHistoryEntry(ctx, tx, entry.BranchID) + if err != nil { + return err + } + + aggregates := newest != nil && + !newest.Boundary && + !entry.Boundary && + newest.CreatedAt.Truncate(_historyAggregationDuration).Equal(entry.CreatedAt.Truncate(_historyAggregationDuration)) + + if aggregates { + q, args := a.builder.Update("document_branch_history_entries"). + SetMap(map[string]any{ + "document_name": entry.DocumentName, + "icon": entry.Icon, + "content": entry.Content, + "hooks": entry.Hooks, + "fk_last_updated_by": entry.LastUpdatedBy, + "created_at": entry.CreatedAt, + }). + Where(sq.Eq{"id": newest.ID}). + MustSql() + + _, err := tx.ExecContext(ctx, q, args...) + + return err + } + + q, args := a.builder.Insert("document_branch_history_entries"). + SetMap(map[string]any{ + "id": entry.ID, + "fk_document_id": entry.DocumentID, + "fk_branch_id": entry.BranchID, + "document_name": entry.DocumentName, + "icon": entry.Icon, + "content": entry.Content, + "hooks": entry.Hooks, + "fk_last_updated_by": entry.LastUpdatedBy, + "boundary": entry.Boundary, + "created_at": entry.CreatedAt, + }). + MustSql() + + if _, err := tx.ExecContext(ctx, q, args...); err != nil { + return err + } + + return a.trimDocumentBranchHistoryEntries(ctx, tx, entry.BranchID) + }) +} + +// fetchNewestDocumentBranchHistoryEntry retrieves the branch's newest +// history entry, or nil when the branch has none. +func (a *agent) fetchNewestDocumentBranchHistoryEntry(ctx context.Context, q sqlx.QueryerContext, branchID xid.ID) (*history.Entry, error) { + sqlq, args := a.selectDocumentBranchHistoryEntry(a.builder.Select()). + Where(sq.Eq{"fk_branch_id": branchID}). + OrderBy("created_at DESC", "id DESC"). + Limit(1). MustSql() - if _, err := ex.ExecContext(ctx, q, args...); err != nil { - return err + var entry history.Entry + + err := sqlx.GetContext(ctx, q, &entry, sqlq, args...) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, nil //nolint:nilnil // no entry is a regular outcome, not an error + } + + return nil, err } - return a.trimDocumentBranchHistoryEntries(ctx, ex, branchID) + return &entry, nil } // trimDocumentBranchHistoryEntries keeps only the newest entries of a branch. -// Age trimming lives in the file manager instead, since it has to reach -// branches that stopped inserting altogether. func (a *agent) trimDocumentBranchHistoryEntries(ctx context.Context, ex sqlx.ExecerContext, branchID xid.ID) error { // a zero limit means unlimited retention; without this guard the // subquery below would emit LIMIT 0 and the delete would drop every @@ -293,7 +355,7 @@ func (a *agent) trimDocumentBranchHistoryEntries(ctx context.Context, ex sqlx.Ex b := a.builder.Select("id"). From("document_branch_history_entries"). Where(sq.Eq{"fk_branch_id": branchID}). - OrderBy("created_at DESC"). + OrderBy("created_at DESC", "id DESC"). Limit(a.opts.MaxDocumentHistoryEntries). Prefix("id NOT IN ("). Suffix(")") @@ -308,6 +370,23 @@ func (a *agent) trimDocumentBranchHistoryEntries(ctx context.Context, ex sqlx.Ex return err } +// selectDocumentBranchHistoryEntry prepares a sql select statement for +// fetching history entries. +func (a *agent) selectDocumentBranchHistoryEntry(b sq.SelectBuilder) sq.SelectBuilder { + return b.Columns( + `id AS "id"`, + `fk_document_id AS "fk_document_id"`, + `fk_branch_id AS "fk_branch_id"`, + `document_name AS "document_name"`, + `icon AS "icon"`, + `content AS "content"`, + `hooks AS "hooks"`, + `fk_last_updated_by AS "fk_last_updated_by"`, + `boundary AS "boundary"`, + `created_at AS "created_at"`, + ).From("document_branch_history_entries") +} + // documentBranchRow maps a document's branch onto the document_branches // columns. func documentBranchRow(doc document.Document) map[string]any { diff --git a/server/core/internal/db/document_branch_test.go b/server/core/internal/db/document_branch_test.go index 025146da..3607be0c 100644 --- a/server/core/internal/db/document_branch_test.go +++ b/server/core/internal/db/document_branch_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "net/http" + "slices" "strconv" "testing" "time" @@ -12,7 +13,9 @@ import ( "github.com/guregu/null/v5" "github.com/jmoiron/sqlx" "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/sqlutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" @@ -653,25 +656,23 @@ func Test_agent_FetchDocumentBranchesAfter(t *testing.T) { testutil.AssertFilterEqual(t, all[2:], res, null.Value[xid.ID]{}) } -// prepHistoryEntries writes count entries of the document's branch, one per -// aggregation bucket so each lands as its own entry, oldest first. -func prepHistoryEntries(t *testing.T, db *DB, doc *document.Document, count int) []document.HistoryEntry { +// prepHistoryEntries writes count ordinary entries of the document's branch, +// one per aggregation bucket so each lands as its own row, oldest first. +func prepHistoryEntries(t *testing.T, db *DB, doc *document.Document, count int) []history.Entry { t.Helper() - res := make([]document.HistoryEntry, count) + res := make([]history.Entry, count) base := timeutil.Now().Truncate(time.Second) for i := range count { - doc.UpdatedAt = base.Add(-time.Duration(count-i) * time.Hour) - entry := doc.HistoryEntry() + entry := history.NewEntry(*doc, + base.Add(-time.Duration(count-i)*time.Hour), + null.String{}, + history.Hooks{}, + false, + ) - require.NoError(t, db.insertDocumentBranchHistoryEntry( - context.Background(), - db.sql, - doc.ID, - doc.BranchID, - entry, - )) + require.NoError(t, db.InsertDocumentBranchHistoryEntry(context.Background(), entry)) res[i] = entry } @@ -679,6 +680,28 @@ func prepHistoryEntries(t *testing.T, db *DB, doc *document.Document, count int) return res } +// assertHistoryEntryEqual compares two entries, matching hook settings as +// JSON since jsonb normalises their spacing. +func assertHistoryEntryEqual(t *testing.T, exp, got history.Entry) { + t.Helper() + + require.Len(t, got.Hooks, len(exp.Hooks)) + + // the hook slices are cloned: cases share their definitions and the + // subtests run in parallel. + exp.Hooks = slices.Clone(exp.Hooks) + got.Hooks = slices.Clone(got.Hooks) + + for i := range exp.Hooks { + assert.JSONEq(t, string(exp.Hooks[i].Settings), string(got.Hooks[i].Settings)) + + exp.Hooks[i].Settings = nil + got.Hooks[i].Settings = nil + } + + assert.Equal(t, exp, got) +} + // countHistoryEntries returns how many entries the branch currently has. func countHistoryEntries(t *testing.T, db *DB, branchID xid.ID) int { t.Helper() @@ -733,7 +756,7 @@ func Test_agent_trimDocumentBranchHistoryEntries(t *testing.T) { } // the survivors are the newest ones. - var ids []string + var ids []xid.ID q, args := db.builder.Select("id"). From("document_branch_history_entries"). @@ -741,7 +764,7 @@ func Test_agent_trimDocumentBranchHistoryEntries(t *testing.T) { MustSql() require.NoError(t, db.sql.Select(&ids, q, args...)) - assert.ElementsMatch(t, []string{entries[2].ID, entries[3].ID}, ids) + assert.ElementsMatch(t, []xid.ID{entries[2].ID, entries[3].ID}, ids) }) } } @@ -770,34 +793,110 @@ func Test_agent_DeleteExpiredDocumentBranchHistoryEntries(t *testing.T) { assert.Equal(t, 1, countHistoryEntries(t, db, doc.BranchID)) } -func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { +func Test_agent_InsertDocumentBranchHistoryEntry(t *testing.T) { type tcase struct { - DocumentID xid.ID - BranchID xid.ID - HistoryEntry document.HistoryEntry - Trimmed int - Err error + Entry history.Entry + // Rows is how many entries the branch holds afterwards. + Rows int + // Newest is the entry the branch's newest row is expected to + // equal afterwards. + Newest history.Entry + Err error + } + + base := time.Date(2026, 1, 1, 10, 0, 0, 0, time.UTC) + hooks := history.Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, } cc := map[string]func(*testing.T, *DB) tcase{ "Non-existent branch": func(t *testing.T, db *DB) tcase { doc := prepDocuments(t, db, 1, nil)[0] - entry := doc.HistoryEntry() + doc.BranchID = xid.New() return tcase{ - DocumentID: doc.ID, - BranchID: xid.New(), - HistoryEntry: entry, - Err: assert.AnError, + Entry: history.NewEntry(*doc, base, null.String{}, hooks, false), + Err: assert.AnError, } }, - "Successful insert": func(t *testing.T, db *DB) tcase { + "First entry of a branch": func(t *testing.T, db *DB) tcase { doc := prepDocuments(t, db, 1, nil)[0] + user := prepUsers(t, db, 1)[0] + + entry := history.NewEntry(*doc, base.Add(5*time.Minute), null.StringFrom(user), hooks, false) return tcase{ - DocumentID: doc.ID, - BranchID: doc.BranchID, - HistoryEntry: doc.HistoryEntry(), + Entry: entry, + Rows: 1, + Newest: entry, + } + }, + "Edit in the same bucket updates the entry in place": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + user := prepUsers(t, db, 1)[0] + + first := history.NewEntry(*doc, base.Add(5*time.Minute), null.String{}, history.Hooks{}, false) + require.NoError(t, db.InsertDocumentBranchHistoryEntry(context.Background(), first)) + + doc.DocumentName = "Renamed" + doc.Icon = "📕" + doc.Content.Content[0].Text = "Edited." + + entry := history.NewEntry(*doc, base.Add(29*time.Minute), null.StringFrom(user), hooks, false) + + newest := entry + newest.ID = first.ID + + return tcase{ + Entry: entry, + Rows: 1, + Newest: newest, + } + }, + "Edit in the next bucket inserts": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + + first := history.NewEntry(*doc, base.Add(5*time.Minute), null.String{}, history.Hooks{}, false) + require.NoError(t, db.InsertDocumentBranchHistoryEntry(context.Background(), first)) + + entry := history.NewEntry(*doc, base.Add(31*time.Minute), null.String{}, hooks, false) + + return tcase{ + Entry: entry, + Rows: 2, + Newest: entry, + } + }, + "Boundary entry in an occupied bucket inserts": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + + first := history.NewEntry(*doc, base.Add(5*time.Minute), null.String{}, history.Hooks{}, false) + require.NoError(t, db.InsertDocumentBranchHistoryEntry(context.Background(), first)) + + entry := history.NewEntry(*doc, base.Add(6*time.Minute), null.String{}, hooks, true) + + return tcase{ + Entry: entry, + Rows: 2, + Newest: entry, + } + }, + "Edit after a boundary in the same bucket inserts": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + + boundary := history.NewEntry(*doc, base.Add(5*time.Minute), null.String{}, history.Hooks{}, true) + require.NoError(t, db.InsertDocumentBranchHistoryEntry(context.Background(), boundary)) + + entry := history.NewEntry(*doc, base.Add(6*time.Minute), null.String{}, hooks, false) + + return tcase{ + Entry: entry, + Rows: 2, + Newest: entry, } }, "Insert trims the branch down to the retained entries": func(t *testing.T, db *DB) tcase { @@ -806,32 +905,13 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { prepHistoryEntries(t, db, doc, 3) db.opts.MaxDocumentHistoryEntries = 2 - doc.UpdatedAt = timeutil.Now().Truncate(time.Second) + + entry := history.NewEntry(*doc, timeutil.Now().Truncate(time.Second), null.String{}, hooks, false) return tcase{ - DocumentID: doc.ID, - BranchID: doc.BranchID, - HistoryEntry: doc.HistoryEntry(), - Trimmed: 2, - } - }, - "Successful update of an existing entry": func(t *testing.T, db *DB) tcase { - doc := prepDocuments(t, db, 1, nil)[0] - - require.NoError(t, db.insertDocumentBranchHistoryEntry( - context.Background(), - db.sql, - doc.ID, - doc.BranchID, - doc.HistoryEntry(), - )) - - doc.RawContent = []byte("updated raw content") - - return tcase{ - DocumentID: doc.ID, - BranchID: doc.BranchID, - HistoryEntry: doc.HistoryEntry(), + Entry: entry, + Rows: 2, + Newest: entry, } }, } @@ -843,33 +923,39 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { db := prepTempDB(t) c := cfn(t, db) - err := db.insertDocumentBranchHistoryEntry(context.Background(), db.sql, c.DocumentID, c.BranchID, c.HistoryEntry) + err := db.InsertDocumentBranchHistoryEntry(context.Background(), c.Entry) testutil.RequireEqualError(t, c.Err, err) - if c.Trimmed != 0 { - assert.Equal(t, c.Trimmed, countHistoryEntries(t, db, c.BranchID)) - } - if err != nil { return } - var entry document.HistoryEntry + assert.Equal(t, c.Rows, countHistoryEntries(t, db, c.Entry.BranchID)) - q, args := db.builder.Select( - `id AS "id"`, - `fk_document_id AS "fk_document_id"`, - `content AS "content"`, - `raw_content AS "raw_content"`, - `created_at AS "created_at"`, - ).From("document_branch_history_entries"). - Where(sq.Eq{ - "id": c.HistoryEntry.ID, - }).MustSql() - - err = db.sql.Get(&entry, q, args...) + newest, err := db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, c.Entry.BranchID) require.NoError(t, err) - testutil.AssertFilterEqual(t, c.HistoryEntry, entry) + require.NotNil(t, newest) + assertHistoryEntryEqual(t, c.Newest, *newest) }) } } + +func Test_agent_fetchNewestDocumentBranchHistoryEntry(t *testing.T) { + t.Parallel() + + db := prepTempDB(t) + doc := prepDocuments(t, db, 1, nil)[0] + + // no entries + res, err := db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, doc.BranchID) + require.NoError(t, err) + assert.Nil(t, res) + + // the newest of several + entries := prepHistoryEntries(t, db, doc, 3) + + res, err = db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, doc.BranchID) + require.NoError(t, err) + require.NotNil(t, res) + assertHistoryEntryEqual(t, entries[2], *res) +} diff --git a/server/core/internal/db/document_file.go b/server/core/internal/db/document_file.go index f7209961..ad689881 100644 --- a/server/core/internal/db/document_file.go +++ b/server/core/internal/db/document_file.go @@ -8,17 +8,13 @@ import ( "github.com/guregu/null/v5" "github.com/jmoiron/sqlx" "github.com/oxynote/oxynote/server/core/internal/document/file" - "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/rs/xid" ) -// InsertDocumentFile inserts a new document file into the database. -// Re-uploading into the same block reuses the file id, so the insert -// refreshes the existing row instead of failing: created_at is bumped -// to re-arm the retention grace period for what is a new object, and -// the unreferenced mark is cleared. A row owned by another organization -// is left alone and reported as errutil.ErrNotFound, since the id is -// client-chosen and must not reassign a file across organizations. +// InsertDocumentFile inserts a new document file into the database. The id +// is client-chosen and unique across organizations: an id that already has +// a row is refused, so an object a history entry points at is never +// overwritten in place. func (a *agent) InsertDocumentFile(ctx context.Context, f file.File) error { q, args := a.builder.Insert("document_files"). SetMap(map[string]any{ @@ -33,29 +29,11 @@ func (a *agent) InsertDocumentFile(ctx context.Context, f file.File) error { "created_at": f.CreatedAt, "unreferenced_at": f.UnreferencedAt, }). - Suffix("ON CONFLICT (id) DO UPDATE SET " + - "location = excluded.location, storage_key = excluded.storage_key, " + - "fk_document_id = excluded.fk_document_id, fk_organization_id = excluded.fk_organization_id, " + - "name = excluded.name, size = excluded.size, content_type = excluded.content_type, " + - "created_at = excluded.created_at, unreferenced_at = NULL " + - "WHERE document_files.fk_organization_id = excluded.fk_organization_id"). MustSql() - res, err := a.sql.ExecContext(ctx, q, args...) - if err != nil { - return err - } + _, err := a.sql.ExecContext(ctx, q, args...) - n, err := res.RowsAffected() - if err != nil { - return err - } - - if n == 0 { - return errutil.ErrNotFound - } - - return nil + return err } // FetchDocumentFile retrieves a document file by its block ID from the database. diff --git a/server/core/internal/db/document_file_test.go b/server/core/internal/db/document_file_test.go index a6ce0947..b203af0e 100644 --- a/server/core/internal/db/document_file_test.go +++ b/server/core/internal/db/document_file_test.go @@ -3,6 +3,7 @@ package db import ( "context" "database/sql" + "net/http" "strconv" "testing" "time" @@ -11,6 +12,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/comment" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" @@ -77,29 +79,20 @@ func Test_agent_InsertDocumentFile(t *testing.T) { } cc := map[string]func(*testing.T, *DB) tcase{ - "Re-upload refreshes the existing row": func(t *testing.T, db *DB) tcase { - f := prepDocumentFiles(t, db, 1, func(_ int, f *file.File) { - f.UnreferencedAt = null.TimeFrom(timeutil.Now().Truncate(time.Second)) - })[0] - - f.Location = file.LocationComment - f.StorageKey = "organizations/org/documents/doc/files/replaced" + "Existing id": func(t *testing.T, db *DB) tcase { + f := prepDocumentFiles(t, db, 1, nil)[0] f.Name = "notes.zip" - f.Size = 2048 - f.ContentType = "application/zip" - f.CreatedAt = timeutil.Now().Truncate(time.Second) - f.UnreferencedAt = null.Time{} - return tcase{File: f} + return tcase{File: f, Err: errutil.New(http.StatusConflict, "document_file.exists", "file id is already in use")} }, - "Re-upload from another organization": func(t *testing.T, db *DB) tcase { + "Existing id from another organization": func(t *testing.T, db *DB) tcase { f := prepDocumentFiles(t, db, 1, nil)[0] doc := prepDocuments(t, db, 1, nil)[0] f.DocumentID = null.ValueFrom(doc.ID) f.OrganizationID = null.StringFrom(doc.OrganizationID) - return tcase{File: f, Err: errutil.ErrNotFound} + return tcase{File: f, Err: errutil.New(http.StatusConflict, "document_file.exists", "file id is already in use")} }, "Missing document": func(_ *testing.T, _ *DB) tcase { return tcase{ @@ -250,12 +243,9 @@ func Test_agent_CheckDocumentFileReferenced(t *testing.T) { doc := prepDocuments(t, db, 1, nil)[0] doc.Content.Content[0].Attrs = document.Attributes{"uid": "file-ref"} - require.NoError(t, db.insertDocumentBranchHistoryEntry( + require.NoError(t, db.InsertDocumentBranchHistoryEntry( context.Background(), - db.sql, - doc.ID, - doc.BranchID, - doc.HistoryEntry(), + history.NewEntry(*doc, timeutil.Now(), null.String{}, nil, false), )) return "file-ref", doc.ID, true diff --git a/server/core/internal/db/document_test.go b/server/core/internal/db/document_test.go index 07f79b96..59300d10 100644 --- a/server/core/internal/db/document_test.go +++ b/server/core/internal/db/document_test.go @@ -711,33 +711,6 @@ func Test_agent_UpdateDocument(t *testing.T) { assert.Equal(t, assert.AnError, err) }) - t.Run("HistoryEntries are kept per branch", func(t *testing.T) { - t.Parallel() - - db := prepTempDB(t) - - // two branches of one document, updated inside the same - // aggregation window. - branches := prepDocumentBranches(t, db, 2, nil) - now := timeutil.Now().Truncate(time.Second) - - for _, b := range branches { - b.UpdatedAt = now - - require.NoError(t, db.UpdateDocument(context.Background(), *b)) - } - - var logs uint64 - - q, args := db.builder.Select("COUNT(*)").From("document_branch_history_entries"). - Where(sq.Eq{ - "fk_document_id": branches[0].ID, - }).MustSql() - - require.NoError(t, db.sql.Get(&logs, q, args...)) - assert.Equal(t, uint64(2), logs) - }) - type tcase struct { Document document.Document Err error @@ -783,18 +756,6 @@ func Test_agent_UpdateDocument(t *testing.T) { err := db.sql.Get(&doc, q, args...) require.NoError(t, err) assert.Equal(t, c.Document, doc) - - var logs uint64 - - q, args = db.builder.Select("COUNT(*)").From("document_branch_history_entries"). - Where(sq.Eq{ - "fk_document_id": c.Document.ID, - }).MustSql() - - err = db.sql.Get(&logs, q, args...) - require.NoError(t, err) - assert.Equal(t, c.Document, doc) - assert.Equal(t, uint64(1), logs) }) } } diff --git a/server/core/internal/db/migrations/002_history_entry_snapshot.sql b/server/core/internal/db/migrations/002_history_entry_snapshot.sql new file mode 100644 index 00000000..83cf176b --- /dev/null +++ b/server/core/internal/db/migrations/002_history_entry_snapshot.sql @@ -0,0 +1,39 @@ +-- +migrate Up + +-- History entries become restorable snapshots: they carry the branch +-- name, icon, author and hook definitions and drop the Yjs binary. The +-- existing rows predate that shape and nothing reads them, so they are +-- dropped rather than backfilled. +TRUNCATE document_branch_history_entries; + +ALTER TABLE document_branch_history_entries + DROP COLUMN raw_content, + ADD COLUMN document_name TEXT NULL, + ADD COLUMN icon TEXT NULL, + ADD COLUMN fk_last_updated_by TEXT REFERENCES users ON DELETE SET NULL, + ADD COLUMN hooks JSONB NOT NULL, + ADD COLUMN boundary BOOLEAN NOT NULL; +CREATE INDEX document_branch_history_entries_fk_last_updated_by_idx ON document_branch_history_entries (fk_last_updated_by); + +-- Entries are read per branch, newest first, and expired by age; the +-- composite index also covers the plain branch lookup it replaces. +DROP INDEX document_branch_history_entries_fk_branch_id_idx; +CREATE INDEX document_branch_history_entries_fk_branch_id_created_at_idx ON document_branch_history_entries (fk_branch_id, created_at DESC, id DESC); +CREATE INDEX document_branch_history_entries_created_at_idx ON document_branch_history_entries (created_at); + +-- +migrate Down + +DROP INDEX document_branch_history_entries_created_at_idx; +DROP INDEX document_branch_history_entries_fk_branch_id_created_at_idx; +CREATE INDEX document_branch_history_entries_fk_branch_id_idx ON document_branch_history_entries (fk_branch_id); +DROP INDEX document_branch_history_entries_fk_last_updated_by_idx; + +TRUNCATE document_branch_history_entries; + +ALTER TABLE document_branch_history_entries + DROP COLUMN boundary, + DROP COLUMN hooks, + DROP COLUMN fk_last_updated_by, + DROP COLUMN icon, + DROP COLUMN document_name, + ADD COLUMN raw_content BYTEA NULL; diff --git a/server/core/internal/document/AGENTS.md b/server/core/internal/document/AGENTS.md index 29712bc3..5fbb4f66 100644 --- a/server/core/internal/document/AGENTS.md +++ b/server/core/internal/document/AGENTS.md @@ -15,18 +15,35 @@ Cross-service storage rules (Hocuspocus, Yjs) live in who edited in that persist; `UpsertDocumentMaintainers` only adds. There is no removal path. - **Duplication copies, never shares**: `Document.Duplicate` regenerates - every block uid, rewrites image/file `src` paths to the new document, and - returns old→new maps for files (copied server-side) and block uids (hooks - re-anchored; unmapped ones dropped). -- History entries pin the files they reference, so - `DB_MAX_DOCUMENT_HISTORY_ENTRIES` and `DB_DOCUMENT_HISTORY_RETENTION` also - decide how long a removed image survives. + every block uid, rewrites image/file `src` paths to the new document (the + file id is read off the src), and returns old→new maps for files (copied + server-side) and block uids (hooks re-anchored; unmapped ones dropped). + +## History entries + +`document_branch_history_entries` are restorable snapshots: name, icon, +content, the branch's live hook definitions (type, block, settings; never +watcher state) and the author, written in the same transaction as the +branch update through `InsertDocumentBranchHistoryEntry`. Rules: + +- Ordinary edits within one 30-minute bucket update the branch's newest + entry in place. A persist that changes nothing writes no entry; a system + write is unattributed (null author) and still takes the time of the + write. +- Merge and fork write a **boundary** entry that never aggregates and closes + its bucket: the next edit starts a new entry. The merge entry lists the + source's hooks. +- Entries pin the files they reference, so `DB_MAX_DOCUMENT_HISTORY_ENTRIES` + and `DB_DOCUMENT_HISTORY_RETENTION` also decide how long a removed image + survives. ## Files and hooks -Files live at `organizations/{org}/documents/{doc}/files/{blockUID}` (the -file id is the block uid, so a re-upload overwrites in place), tracked in -`document_files` with name, size and sniffed content type. `GET +Files live at `organizations/{org}/documents/{doc}/files/{fileId}`. The id +is a nanoid the editor mints per upload, not the block uid, and an id that +already has a row is refused with 409, so a history entry keeps its object +when a block's file is replaced. Rows are `document_files` with name, size +and sniffed content type. `GET /api/documents/{id}/files/{fileId}-{fileName}` cuts the 21-char nanoid off the front and ignores the name; `Content-Disposition` is `inline` only for `file.Viewable`. The key is an S3 object key or a path under the storage diff --git a/server/core/internal/document/document.go b/server/core/internal/document/document.go index bdd98bcc..d216d128 100644 --- a/server/core/internal/document/document.go +++ b/server/core/internal/document/document.go @@ -2,6 +2,7 @@ package document import ( + "bytes" _ "embed" "encoding/json" "fmt" @@ -27,16 +28,6 @@ var ( // DefaultBranch is the name of the default branch for a document. const DefaultBranch = "main" -const ( - - // _aggregationDuration is the duration for which document changes are aggregated. - _aggregationDuration = 30 * time.Minute - - // _historyEntryTimeLayout is the layout used for timestamps in history - // entry IDs. - _historyEntryTimeLayout = "2006-01-02T15:04:05" -) - // Branch holds branch-specific content and metadata for a document. type Branch struct { // BranchID is the unique identifier for this branch row. @@ -276,46 +267,27 @@ func (d Document) Title() string { return d.DocumentName + " on branch " + d.BranchName } -// HistoryEntry returns a history entry for the current state of the document. -// Entries aggregate per branch and per time bucket: the branch is part of the -// ID so that two branches of one document edited within the same bucket keep -// separate entries instead of overwriting each other. -func (d Document) HistoryEntry() HistoryEntry { - id := fmt.Sprintf( - "%s-%s-%s", - d.ID, - d.BranchID, - d.UpdatedAt.Truncate(_aggregationDuration). - Format(_historyEntryTimeLayout), - ) - - return HistoryEntry{ - ID: id, - DocumentID: d.ID, - Content: d.Content, - RawContent: d.RawContent, - CreatedAt: d.UpdatedAt, +// SnapshotEqual reports whether the other document would produce the same +// snapshot: the same name, icon and content. Content is compared as +// JSON, which encoding/json emits with sorted keys. +func (d Document) SnapshotEqual(o Document) bool { + if d.DocumentName != o.DocumentName || d.Icon != o.Icon { + return false } -} -// HistoryEntry represents one entry in a document's history. -type HistoryEntry struct { - // ID is the unique identifier for the entry. - ID string `json:"id" db:"id"` + dc, err := json.Marshal(d.Content) + if err != nil { + // NOCOV: a block tree of plain values always marshals. + return false + } - // DocumentID is the identifier for the document associated - // with this entry. - DocumentID xid.ID `json:"documentId" db:"fk_document_id"` + oc, err := json.Marshal(o.Content) + if err != nil { + // NOCOV: a block tree of plain values always marshals. + return false + } - // Content is the content of the document at the time of the entry. - Content RootBlock `json:"content" db:"content"` - - // RawContent is the raw content of the document, typically in - // a text format. - RawContent []byte `json:"rawContent" db:"raw_content"` - - // CreatedAt is the timestamp when the entry was created. - CreatedAt time.Time `json:"createdAt" db:"created_at"` + return bytes.Equal(dc, oc) } // CreateInput is the input structure for document operations. diff --git a/server/core/internal/document/document_test.go b/server/core/internal/document/document_test.go index f0b75245..86a47b3a 100644 --- a/server/core/internal/document/document_test.go +++ b/server/core/internal/document/document_test.go @@ -262,27 +262,58 @@ func Test_Document_ApplyBranchUpdate(t *testing.T) { } } -func Test_Document_HistoryEntry(t *testing.T) { +func Test_Document_SnapshotEqual(t *testing.T) { t.Parallel() - doc := stubDocument() - doc.UpdatedAt = time.Date(2026, 1, 1, 10, 44, 30, 0, time.UTC) + cc := map[string]struct { + Mutate func(d *Document) + Equal bool + }{ + "Identical": { + Mutate: func(_ *Document) {}, + Equal: true, + }, + "Different name": { + Mutate: func(d *Document) { + d.DocumentName = "Other" + }, + }, + "Different icon": { + Mutate: func(d *Document) { + d.Icon = "📕" + }, + }, + "Different block": { + Mutate: func(d *Document) { + d.Content.Content = d.Content.Content[1:] + }, + }, + "Different attribute": { + Mutate: func(d *Document) { + d.Content.Content[0].Attrs = map[string]any{"uid": "changed"} + }, + }, + "Different raw content only": { + Mutate: func(d *Document) { + d.RawContent = []byte("other") + }, + Equal: true, + }, + } - entry := doc.HistoryEntry() + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - // the timestamp is truncated to the 30-minute aggregation window. - assert.Equal(t, doc.ID.String()+"-"+doc.BranchID.String()+"-2026-01-01T10:30:00", entry.ID) - assert.Equal(t, doc.ID, entry.DocumentID) - assert.Equal(t, doc.Content, entry.Content) - assert.Equal(t, doc.RawContent, entry.RawContent) - assert.Equal(t, doc.UpdatedAt, entry.CreatedAt) + doc := stubDocument() + other := stubDocument() + other.ID = doc.ID + other.BranchID = doc.BranchID + c.Mutate(&other) - // a second branch of the same document updated within the same window - // keeps an entry of its own. - other := doc - other.BranchID = xid.New() - - assert.NotEqual(t, entry.ID, other.HistoryEntry().ID) + assert.Equal(t, c.Equal, doc.SnapshotEqual(other)) + }) + } } func Test_Document_Duplicate(t *testing.T) { @@ -327,8 +358,8 @@ func Test_Document_Duplicate(t *testing.T) { doc.Content.Content[0] = Block{ Type: BlockNodeImageBlock, Attrs: Attributes{ - "uid": "img1", - "src": "https://app.test/core" + FilePath(doc.ID, "img1", "shot.png"), + "uid": "img-1-aaaaaaaaaaaaaaa", + "src": "https://app.test/core" + FilePath(doc.ID, "img-1-aaaaaaaaaaaaaaa", "shot.png"), }, } @@ -337,7 +368,7 @@ func Test_Document_Duplicate(t *testing.T) { Check: func(t *testing.T, _, dup Document, files map[string]string) { require.Len(t, files, 1) - newID, ok := files["img1"] + newID, ok := files["img-1-aaaaaaaaaaaaaaa"] require.True(t, ok) // the duplicate refers to its own copy under its own document. diff --git a/server/core/internal/document/file/file.go b/server/core/internal/document/file/file.go index dc197edd..8fce4dea 100644 --- a/server/core/internal/document/file/file.go +++ b/server/core/internal/document/file/file.go @@ -21,9 +21,8 @@ func Folder(organizationID string, documentID xid.ID) string { return fmt.Sprintf(_folderFormat, organizationID, documentID) } -// Key returns the storage key of the given document's file. The key -// is the file id alone: a re-upload into the same block then overwrites -// its object in place, so no object is ever left without a row naming it. +// Key returns the storage key of the given document's file: the file id +// under the document's folder. func Key(organizationID string, documentID xid.ID, id string) string { return path.Join(Folder(organizationID, documentID), id) } diff --git a/server/core/internal/document/history/history.go b/server/core/internal/document/history/history.go new file mode 100644 index 00000000..a37c95e0 --- /dev/null +++ b/server/core/internal/document/history/history.go @@ -0,0 +1,134 @@ +// Package history defines the restorable history entries of a document branch. +package history + +import ( + "database/sql/driver" + "encoding/json" + "errors" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" + "github.com/rs/xid" +) + +// Entry is a snapshot of a branch that a restore can put back. +type Entry struct { + // ID is the unique identifier for the entry. + ID xid.ID `json:"id" db:"id"` + + // DocumentID is the identifier of the document the entry belongs to. + DocumentID xid.ID `json:"documentId" db:"fk_document_id"` + + // BranchID is the identifier of the branch the entry belongs to. + BranchID xid.ID `json:"branchId" db:"fk_branch_id"` + + // DocumentName is the name of the document at the time of the entry. + DocumentName string `json:"documentName" db:"document_name"` + + // Icon is the icon of the document at the time of the entry. + Icon string `json:"icon" db:"icon"` + + // Content is the content of the branch at the time of the entry. + Content document.RootBlock `json:"content" db:"content"` + + // Hooks are the hooks the branch carried at the time of the entry. + Hooks Hooks `json:"hooks" db:"hooks"` + + // LastUpdatedBy is the user whose change the entry records. Null + // for a system write, which no user made. + LastUpdatedBy null.String `json:"lastUpdatedBy" db:"fk_last_updated_by"` + + // Boundary indicates whether the entry was written by an operation + // that replaced the whole content and therefore never aggregates. + Boundary bool `json:"boundary" db:"boundary"` + + // CreatedAt is the timestamp when the entry was taken. + CreatedAt time.Time `json:"createdAt" db:"created_at"` +} + +// NewEntry takes a snapshot of the branch as it is now, at the given time. +// Aggregation into time buckets is decided where the entry is stored, so +// every call mints a fresh id. A boundary entry is one written by an +// operation that replaced the whole content (merge, fork); it never joins +// a bucket and closes the one it lands in. +func NewEntry(doc document.Document, at time.Time, by null.String, hooks Hooks, boundary bool) Entry { + return Entry{ + ID: xid.New(), + DocumentID: doc.ID, + BranchID: doc.BranchID, + DocumentName: doc.DocumentName, + Icon: doc.Icon, + Content: doc.Content, + Hooks: hooks, + LastUpdatedBy: by, + Boundary: boundary, + CreatedAt: at, + } +} + +// Hook is what it takes to re-create a hook: its type, the block it is +// anchored to and its settings. Watcher state is not part of it. +type Hook struct { + // Type is the type of the hook. + Type hook.Type `json:"type"` + + // BlockID is the block the hook is anchored to; null for a document + // level hook. + BlockID null.String `json:"blockId"` + + // Settings are the hook's settings in JSON format. + Settings processor.Settings `json:"settings"` +} + +// Hooks is the list of hooks stored on a history entry. +type Hooks []Hook + +// NewHooks reduces hooks to what a history entry records of them. +func NewHooks(hooks []hook.Hook) Hooks { + res := make(Hooks, 0, len(hooks)) + + for _, hk := range hooks { + res = append(res, Hook{ + Type: hk.Type, + BlockID: hk.BlockID, + Settings: hk.Settings, + }) + } + + return res +} + +// Value transforms the hooks into a database entry. +func (hh Hooks) Value() (driver.Value, error) { + if hh == nil { + return json.Marshal(Hooks{}) + } + + return json.Marshal(hh) +} + +// Scan transforms a database entry into hooks. +func (hh *Hooks) Scan(src any) error { + var pv []byte + + switch v := src.(type) { + case []byte: + pv = v + case string: + pv = []byte(v) + default: + return errors.New("invalid hooks type") + } + + data := Hooks{} + if err := json.Unmarshal(pv, &data); err != nil { + return err + } + + *hh = data + + return nil +} diff --git a/server/core/internal/document/history/history_test.go b/server/core/internal/document/history/history_test.go new file mode 100644 index 00000000..dfd5fce9 --- /dev/null +++ b/server/core/internal/document/history/history_test.go @@ -0,0 +1,193 @@ +package history + +import ( + "testing" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" + "github.com/oxynote/oxynote/server/core/pkg/testutil" + "github.com/rs/xid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + +func Test_NewEntry(t *testing.T) { + t.Parallel() + + doc := document.Document{ + ID: xid.New(), + OrganizationID: "org-1", + BranchID: xid.New(), + BranchName: document.DefaultBranch, + DocumentName: "Runbook", + Icon: "📘", + Content: document.RootBlock{ + Type: "doc", + Content: []document.Block{ + {Type: document.BlockNodeParagraph, Text: "Hello"}, + }, + }, + } + at := time.Date(2026, 1, 1, 10, 44, 30, 0, time.UTC) + hooks := Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + } + + entry := NewEntry(doc, at, null.StringFrom("user-1"), hooks, true) + + assert.False(t, entry.ID.IsNil()) + assert.Equal(t, doc.ID, entry.DocumentID) + assert.Equal(t, doc.BranchID, entry.BranchID) + assert.Equal(t, doc.DocumentName, entry.DocumentName) + assert.Equal(t, doc.Icon, entry.Icon) + assert.Equal(t, doc.Content, entry.Content) + assert.Equal(t, hooks, entry.Hooks) + assert.Equal(t, null.StringFrom("user-1"), entry.LastUpdatedBy) + assert.True(t, entry.Boundary) + assert.Equal(t, at, entry.CreatedAt) + + // every call mints its own id. + assert.NotEqual(t, entry.ID, NewEntry(doc, at, null.String{}, nil, false).ID) +} + +func Test_NewHooks(t *testing.T) { + t.Parallel() + + assert.Equal(t, Hooks{}, NewHooks(nil)) + assert.Equal(t, Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + { + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + }, + }, NewHooks([]hook.Hook{ + { + ID: xid.New(), + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + State: processor.State(`{"watcher":"w1"}`), + }, + { + ID: xid.New(), + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + }, + })) +} + +func Test_Hooks_Value(t *testing.T) { + t.Parallel() + + cc := map[string]struct { + Hooks Hooks + JSON string + }{ + "Nil hooks": { + JSON: `[]`, + }, + "Hooks": { + Hooks: Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + { + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + }, + }, + JSON: `[{"type":"url-watcher","blockId":"b1","settings":{"url":"https://example.com"}},` + + `{"type":"scheduled-reminder","blockId":null,"settings":{"cron":"0 9 * * 1"}}]`, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + v, err := c.Hooks.Value() + require.NoError(t, err) + + data, ok := v.([]byte) + require.True(t, ok) + assert.JSONEq(t, c.JSON, string(data)) + }) + } +} + +func Test_Hooks_Scan(t *testing.T) { + t.Parallel() + + cc := map[string]struct { + Src any + Result Hooks + Err error + }{ + "Invalid type": { + Src: 42, + Err: assert.AnError, + }, + "Invalid JSON": { + Src: []byte(`{`), + Err: assert.AnError, + }, + "Bytes": { + Src: []byte(`[{"type":"url-watcher","blockId":"b1","settings":{"url":"https://example.com"}}]`), + Result: Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + }, + }, + "String": { + Src: `[{"type":"scheduled-reminder","blockId":null,"settings":{}}]`, + Result: Hooks{ + { + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{}`), + }, + }, + }, + "Empty list": { + Src: []byte(`[]`), + Result: Hooks{}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + var hh Hooks + + err := hh.Scan(c.Src) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, c.Result, hh) + }) + } +} diff --git a/server/core/internal/document/node.go b/server/core/internal/document/node.go index 14101156..628bf91e 100644 --- a/server/core/internal/document/node.go +++ b/server/core/internal/document/node.go @@ -17,9 +17,14 @@ import ( // router mounts the file routes on this very format. const FilePathFormat = "/api/documents/%s/files/%s" +// FileIDLength is the exact length of a file id: a 21-character nanoid. +// The id's alphabet includes the dash, so a "-" segment is +// split by length, never at the first dash. +const FileIDLength = 21 + // FilePath returns the URL path a document's file is served under: the // file route with the id followed by a dash and the file name, so the -// address reads as the file it is. Every uid is a 21-character nanoid, +// address reads as the file it is. Every file id is a 21-character nanoid, // which is what lets the server cut the id off the front again. func FilePath(documentID xid.ID, id, name string) string { return fmt.Sprintf(FilePathFormat, documentID, id+"-"+url.PathEscape(name)) @@ -351,21 +356,11 @@ func (d *duplication) regenerateUID(old any) string { return uid } -// rewriteFileRef points a duplicated image or file block at the -// duplicate's own copy of the file and records the pair. Files served -// from anywhere but the source document's file route are left as they -// are: an externally hosted image has no object to copy. +// rewriteFileRef points a duplicated block's src at the duplicate's own +// copy of the file and records the pair. The file id is the fixed-length +// segment after the source document's file route; a src served from +// anywhere else has no object to copy and is left as it is. func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { - oldID, ok := attrs[AttrUID].(string) - if !ok { - return - } - - newID, ok := newAttrs[AttrUID].(string) - if !ok { - return - } - src, ok := attrs[AttrSrc].(string) if !ok { return @@ -376,24 +371,27 @@ func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { return } - // the id is followed by the dash and the file name. - oldPath := fmt.Sprintf(FilePathFormat, d.oldDocumentID, oldID) - - idx := strings.Index(u.Path, oldPath) - if idx < 0 { + prefix, rest, ok := strings.Cut(u.Path, fmt.Sprintf(FilePathFormat, d.oldDocumentID, "")) + if !ok { return } - rest := u.Path[idx+len(oldPath):] - if !strings.HasPrefix(rest, "-") { + if len(rest) <= FileIDLength || rest[FileIDLength] != '-' { return } + oldID, name := rest[:FileIDLength], rest[FileIDLength+1:] + + newID, ok := d.files[oldID] + if !ok { + newID = strutil.NanoID() + d.files[oldID] = newID + } + // only the path is swapped: the src was built by the frontend from its // own api base url, which need not match this server's public url. - u.Path = u.Path[:idx] + fmt.Sprintf(FilePathFormat, d.newDocumentID, newID) + rest + u.Path = prefix + fmt.Sprintf(FilePathFormat, d.newDocumentID, newID) + "-" + name u.RawPath = "" newAttrs[AttrSrc] = u.String() - d.files[oldID] = newID } diff --git a/server/core/internal/document/node_test.go b/server/core/internal/document/node_test.go index 744eba23..c597e08e 100644 --- a/server/core/internal/document/node_test.go +++ b/server/core/internal/document/node_test.go @@ -342,8 +342,8 @@ func Test_RootBlock_Duplicate(t *testing.T) { { Type: BlockNodeImageBlock, Attrs: Attributes{ - "uid": "img1", - "src": "https://app.test/core" + FilePath(oldDocumentID, "img1", "shot.png"), + "uid": "img-1-aaaaaaaaaaaaaaa", + "src": "https://app.test/core" + FilePath(oldDocumentID, "img-1-aaaaaaaaaaaaaaa", "shot.png"), }, }, { @@ -356,8 +356,8 @@ func Test_RootBlock_Duplicate(t *testing.T) { { Type: BlockNodeImageBlock, Attrs: Attributes{ - "uid": "img3", - "src": "https://app.test/core" + FilePath(xid.New(), "img3", "shot.png"), + "uid": "img-3-aaaaaaaaaaaaaaa", + "src": "https://app.test/core" + FilePath(xid.New(), "img-3-aaaaaaaaaaaaaaa", "shot.png"), }, }, { @@ -367,8 +367,8 @@ func Test_RootBlock_Duplicate(t *testing.T) { { Type: BlockNodeFileBlock, Attrs: Attributes{ - "uid": "file1", - "src": "https://app.test/core" + FilePath(oldDocumentID, "file1", "my notes.zip"), + "uid": "file-1-aaaaaaaaaaaaaa", + "src": "https://app.test/core" + FilePath(oldDocumentID, "file-1-aaaaaaaaaaaaaa", "my notes.zip"), "name": "notes.zip", "size": 2048, "contentType": "application/zip", @@ -381,12 +381,16 @@ func Test_RootBlock_Duplicate(t *testing.T) { // document are remapped. require.Len(t, files, 2) - newID, ok := files["img1"] + newID, ok := files["img-1-aaaaaaaaaaaaaaa"] require.True(t, ok) + assert.Len(t, newID, 21) + // the file gets an id of its own; the block's uid is + // regenerated separately. uid, ok := dup.Content[0].UID() require.True(t, ok) - assert.Equal(t, newID, uid) + assert.NotEqual(t, newID, uid) + assert.NotEqual(t, "img-1-aaaaaaaaaaaaaaa", uid) assert.Equal( t, "https://app.test/core"+FilePath(newDocumentID, newID, "shot.png"), @@ -403,12 +407,9 @@ func Test_RootBlock_Duplicate(t *testing.T) { // the file block is remapped the way the image is, keeps // the file name segment of its address, and keeps what // it shows. - newFileID, ok := files["file1"] + newFileID, ok := files["file-1-aaaaaaaaaaaaaa"] require.True(t, ok) - - fileUID, ok := dup.Content[4].UID() - require.True(t, ok) - assert.Equal(t, newFileID, fileUID) + assert.Len(t, newFileID, 21) assert.Equal( t, "https://app.test/core"+FilePath(newDocumentID, newFileID, "my notes.zip"), @@ -419,8 +420,8 @@ func Test_RootBlock_Duplicate(t *testing.T) { assert.Equal(t, "application/zip", dup.Content[4].Attrs["contentType"]) // the original is untouched. - assert.Equal(t, "img1", orig.Content[0].Attrs["uid"]) - assert.Equal(t, "file1", orig.Content[4].Attrs["uid"]) + assert.Equal(t, "img-1-aaaaaaaaaaaaaaa", orig.Content[0].Attrs["uid"]) + assert.Equal(t, "file-1-aaaaaaaaaaaaaa", orig.Content[4].Attrs["uid"]) }, } }(), diff --git a/server/core/internal/server/0mock_db_test.go b/server/core/internal/server/0mock_db_test.go index 7d763ca7..651cda21 100644 --- a/server/core/internal/server/0mock_db_test.go +++ b/server/core/internal/server/0mock_db_test.go @@ -13,6 +13,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/comment" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" notificationCore "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/search" @@ -208,6 +209,9 @@ var _ DB = &DBMock{} // InsertDocumentBranchFunc: func(ctx context.Context, doc document.Document) error { // panic("mock out the InsertDocumentBranch method") // }, +// InsertDocumentBranchHistoryEntryFunc: func(ctx context.Context, entry history.Entry) error { +// panic("mock out the InsertDocumentBranchHistoryEntry method") +// }, // InsertDocumentCommentFunc: func(ctx context.Context, c comment.Comment) error { // panic("mock out the InsertDocumentComment method") // }, @@ -494,6 +498,9 @@ type DBMock struct { // InsertDocumentBranchFunc mocks the InsertDocumentBranch method. InsertDocumentBranchFunc func(ctx context.Context, doc document.Document) error + // InsertDocumentBranchHistoryEntryFunc mocks the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntryFunc func(ctx context.Context, entry history.Entry) error + // InsertDocumentCommentFunc mocks the InsertDocumentComment method. InsertDocumentCommentFunc func(ctx context.Context, c comment.Comment) error @@ -1115,6 +1122,13 @@ type DBMock struct { // Doc is the doc argument value. Doc document.Document } + // InsertDocumentBranchHistoryEntry holds details about calls to the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntry []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Entry is the entry argument value. + Entry history.Entry + } // InsertDocumentComment holds details about calls to the InsertDocumentComment method. InsertDocumentComment []struct { // Ctx is the ctx argument value. @@ -1457,6 +1471,7 @@ type DBMock struct { lockInsertDataSource sync.RWMutex lockInsertDocument sync.RWMutex lockInsertDocumentBranch sync.RWMutex + lockInsertDocumentBranchHistoryEntry sync.RWMutex lockInsertDocumentComment sync.RWMutex lockInsertDocumentCommentReply sync.RWMutex lockInsertDocumentFile sync.RWMutex @@ -4039,6 +4054,45 @@ func (mock *DBMock) InsertDocumentBranchCalls() []struct { return calls } +// InsertDocumentBranchHistoryEntry calls InsertDocumentBranchHistoryEntryFunc. +func (mock *DBMock) InsertDocumentBranchHistoryEntry(ctx context.Context, entry history.Entry) error { + callInfo := struct { + Ctx context.Context + Entry history.Entry + }{ + Ctx: ctx, + Entry: entry, + } + mock.lockInsertDocumentBranchHistoryEntry.Lock() + mock.calls.InsertDocumentBranchHistoryEntry = append(mock.calls.InsertDocumentBranchHistoryEntry, callInfo) + mock.lockInsertDocumentBranchHistoryEntry.Unlock() + if mock.InsertDocumentBranchHistoryEntryFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentBranchHistoryEntryFunc(ctx, entry) +} + +// InsertDocumentBranchHistoryEntryCalls gets all the calls that were made to InsertDocumentBranchHistoryEntry. +// Check the length with: +// +// len(mockedDB.InsertDocumentBranchHistoryEntryCalls()) +func (mock *DBMock) InsertDocumentBranchHistoryEntryCalls() []struct { + Ctx context.Context + Entry history.Entry +} { + var calls []struct { + Ctx context.Context + Entry history.Entry + } + mock.lockInsertDocumentBranchHistoryEntry.RLock() + calls = mock.calls.InsertDocumentBranchHistoryEntry + mock.lockInsertDocumentBranchHistoryEntry.RUnlock() + return calls +} + // InsertDocumentComment calls InsertDocumentCommentFunc. func (mock *DBMock) InsertDocumentComment(ctx context.Context, c comment.Comment) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/0mock_db_test.go b/server/core/internal/server/internal/document/0mock_db_test.go index d44a004d..db21760e 100644 --- a/server/core/internal/server/internal/document/0mock_db_test.go +++ b/server/core/internal/server/internal/document/0mock_db_test.go @@ -10,6 +10,7 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -106,6 +107,9 @@ var _ DB = &DBMock{} // InsertDocumentBranchFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the InsertDocumentBranch method") // }, +// InsertDocumentBranchHistoryEntryFunc: func(ctx context.Context, entry history.Entry) error { +// panic("mock out the InsertDocumentBranchHistoryEntry method") +// }, // InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { // panic("mock out the InsertDocumentFile method") // }, @@ -227,6 +231,9 @@ type DBMock struct { // InsertDocumentBranchFunc mocks the InsertDocumentBranch method. InsertDocumentBranchFunc func(ctx context.Context, doc documentCore.Document) error + // InsertDocumentBranchHistoryEntryFunc mocks the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntryFunc func(ctx context.Context, entry history.Entry) error + // InsertDocumentFileFunc mocks the InsertDocumentFile method. InsertDocumentFileFunc func(ctx context.Context, f file.File) error @@ -501,6 +508,13 @@ type DBMock struct { // Doc is the doc argument value. Doc documentCore.Document } + // InsertDocumentBranchHistoryEntry holds details about calls to the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntry []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Entry is the entry argument value. + Entry history.Entry + } // InsertDocumentFile holds details about calls to the InsertDocumentFile method. InsertDocumentFile []struct { // Ctx is the ctx argument value. @@ -624,6 +638,7 @@ type DBMock struct { lockInsertBranchReviewer sync.RWMutex lockInsertDocument sync.RWMutex lockInsertDocumentBranch sync.RWMutex + lockInsertDocumentBranchHistoryEntry sync.RWMutex lockInsertDocumentFile sync.RWMutex lockInsertDocumentHook sync.RWMutex lockInsertSearchJob sync.RWMutex @@ -1807,6 +1822,45 @@ func (mock *DBMock) InsertDocumentBranchCalls() []struct { return calls } +// InsertDocumentBranchHistoryEntry calls InsertDocumentBranchHistoryEntryFunc. +func (mock *DBMock) InsertDocumentBranchHistoryEntry(ctx context.Context, entry history.Entry) error { + callInfo := struct { + Ctx context.Context + Entry history.Entry + }{ + Ctx: ctx, + Entry: entry, + } + mock.lockInsertDocumentBranchHistoryEntry.Lock() + mock.calls.InsertDocumentBranchHistoryEntry = append(mock.calls.InsertDocumentBranchHistoryEntry, callInfo) + mock.lockInsertDocumentBranchHistoryEntry.Unlock() + if mock.InsertDocumentBranchHistoryEntryFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentBranchHistoryEntryFunc(ctx, entry) +} + +// InsertDocumentBranchHistoryEntryCalls gets all the calls that were made to InsertDocumentBranchHistoryEntry. +// Check the length with: +// +// len(mockedDB.InsertDocumentBranchHistoryEntryCalls()) +func (mock *DBMock) InsertDocumentBranchHistoryEntryCalls() []struct { + Ctx context.Context + Entry history.Entry +} { + var calls []struct { + Ctx context.Context + Entry history.Entry + } + mock.lockInsertDocumentBranchHistoryEntry.RLock() + calls = mock.calls.InsertDocumentBranchHistoryEntry + mock.lockInsertDocumentBranchHistoryEntry.RUnlock() + return calls +} + // InsertDocumentFile calls InsertDocumentFileFunc. func (mock *DBMock) InsertDocumentFile(ctx context.Context, f file.File) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/0mock_tx_test.go b/server/core/internal/server/internal/document/0mock_tx_test.go index c14a8b79..54d3c573 100644 --- a/server/core/internal/server/internal/document/0mock_tx_test.go +++ b/server/core/internal/server/internal/document/0mock_tx_test.go @@ -10,6 +10,7 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -106,6 +107,9 @@ var _ Tx = &TxMock{} // InsertDocumentBranchFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the InsertDocumentBranch method") // }, +// InsertDocumentBranchHistoryEntryFunc: func(ctx context.Context, entry history.Entry) error { +// panic("mock out the InsertDocumentBranchHistoryEntry method") +// }, // InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { // panic("mock out the InsertDocumentFile method") // }, @@ -230,6 +234,9 @@ type TxMock struct { // InsertDocumentBranchFunc mocks the InsertDocumentBranch method. InsertDocumentBranchFunc func(ctx context.Context, doc documentCore.Document) error + // InsertDocumentBranchHistoryEntryFunc mocks the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntryFunc func(ctx context.Context, entry history.Entry) error + // InsertDocumentFileFunc mocks the InsertDocumentFile method. InsertDocumentFileFunc func(ctx context.Context, f file.File) error @@ -503,6 +510,13 @@ type TxMock struct { // Doc is the doc argument value. Doc documentCore.Document } + // InsertDocumentBranchHistoryEntry holds details about calls to the InsertDocumentBranchHistoryEntry method. + InsertDocumentBranchHistoryEntry []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Entry is the entry argument value. + Entry history.Entry + } // InsertDocumentFile holds details about calls to the InsertDocumentFile method. InsertDocumentFile []struct { // Ctx is the ctx argument value. @@ -629,6 +643,7 @@ type TxMock struct { lockInsertBranchReviewer sync.RWMutex lockInsertDocument sync.RWMutex lockInsertDocumentBranch sync.RWMutex + lockInsertDocumentBranchHistoryEntry sync.RWMutex lockInsertDocumentFile sync.RWMutex lockInsertDocumentHook sync.RWMutex lockInsertSearchJob sync.RWMutex @@ -1804,6 +1819,45 @@ func (mock *TxMock) InsertDocumentBranchCalls() []struct { return calls } +// InsertDocumentBranchHistoryEntry calls InsertDocumentBranchHistoryEntryFunc. +func (mock *TxMock) InsertDocumentBranchHistoryEntry(ctx context.Context, entry history.Entry) error { + callInfo := struct { + Ctx context.Context + Entry history.Entry + }{ + Ctx: ctx, + Entry: entry, + } + mock.lockInsertDocumentBranchHistoryEntry.Lock() + mock.calls.InsertDocumentBranchHistoryEntry = append(mock.calls.InsertDocumentBranchHistoryEntry, callInfo) + mock.lockInsertDocumentBranchHistoryEntry.Unlock() + if mock.InsertDocumentBranchHistoryEntryFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentBranchHistoryEntryFunc(ctx, entry) +} + +// InsertDocumentBranchHistoryEntryCalls gets all the calls that were made to InsertDocumentBranchHistoryEntry. +// Check the length with: +// +// len(mockedTx.InsertDocumentBranchHistoryEntryCalls()) +func (mock *TxMock) InsertDocumentBranchHistoryEntryCalls() []struct { + Ctx context.Context + Entry history.Entry +} { + var calls []struct { + Ctx context.Context + Entry history.Entry + } + mock.lockInsertDocumentBranchHistoryEntry.RLock() + calls = mock.calls.InsertDocumentBranchHistoryEntry + mock.lockInsertDocumentBranchHistoryEntry.RUnlock() + return calls +} + // InsertDocumentFile calls InsertDocumentFileFunc. func (mock *TxMock) InsertDocumentFile(ctx context.Context, f file.File) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/branch_http.go b/server/core/internal/server/internal/document/branch_http.go index d2786c5f..7654c43c 100644 --- a/server/core/internal/server/internal/document/branch_http.go +++ b/server/core/internal/server/internal/document/branch_http.go @@ -10,12 +10,14 @@ import ( "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/webchange" documentCore "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/oxynote/oxynote/server/core/pkg/logutil" + "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" ) @@ -200,6 +202,31 @@ func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http. return } + // a persist that changes nothing, such as the Yjs seed Hocuspocus + // sends on first load, leaves history alone. A system write is not + // attributed: no user made that change. + if !doc.SnapshotEqual(ndoc) { + var hooks []hook.Hook + + hooks, err = tx.FetchDocumentHooksByBranchID(r.Context(), doc.BranchID, doc.OrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var lastUpdatedBy null.String + if !ui.System { + lastUpdatedBy = ndoc.LastUpdatedBy + } + + entry := history.NewEntry(ndoc, timeutil.Now(), lastUpdatedBy, history.NewHooks(hooks), false) + + if err = tx.InsertDocumentBranchHistoryEntry(r.Context(), entry); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + } + maintainers, err := tx.FetchDocumentMaintainers(r.Context(), doc.ID, doc.OrganizationID) if err != nil { httpserver.RespondError(h.log, w, err) @@ -336,6 +363,11 @@ func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { return } + if err := h.insertBoundaryEntry(r.Context(), tx, ndoc, fromDoc.BranchID, session); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + if err := tx.ReplaceBranchTags(r.Context(), session.ActiveOrganizationID, fromDoc.BranchID, toDoc.BranchID); err != nil { httpserver.RespondError(h.log, w, err) return @@ -466,6 +498,11 @@ func (h *Handler) CreateDocumentBranch(w http.ResponseWriter, r *http.Request) { return } + if err := h.insertBoundaryEntry(r.Context(), tx, newDoc, sourceDoc.BranchID, session); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + if err := tx.CopyBranchTags(r.Context(), session.ActiveOrganizationID, sourceDoc.BranchID, newDoc.BranchID); err != nil { httpserver.RespondError(h.log, w, err) return @@ -669,3 +706,18 @@ func (h *Handler) copyHooksToBranch( } } } + +// insertBoundaryEntry records a boundary history entry of the document's +// branch, attributed to the session user. The entry lists the hooks of the +// branch the content came from, which are what the branch carries once +// copyHooksToBranch has run after the commit. +func (h *Handler) insertBoundaryEntry(ctx context.Context, tx Tx, doc documentCore.Document, sourceBranchID xid.ID, session auth.Session) error { + hooks, err := tx.FetchDocumentHooksByBranchID(ctx, sourceBranchID, session.ActiveOrganizationID) + if err != nil { + return err + } + + entry := history.NewEntry(doc, timeutil.Now(), null.StringFrom(session.UserID), history.NewHooks(hooks), true) + + return tx.InsertDocumentBranchHistoryEntry(ctx, entry) +} diff --git a/server/core/internal/server/internal/document/branch_http_test.go b/server/core/internal/server/internal/document/branch_http_test.go index ccfe419f..b57633f7 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -9,9 +9,12 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/history" hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/internal/search" + "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/rs/xid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -146,7 +149,78 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { Maintainers int SearchJobs int BranchID xid.ID + // History is how many history entries the persist records. + History int + // HistoryCheck inspects the recorded entry. + HistoryCheck func(t *testing.T, entry history.Entry) }{ + "Unchanged persist writes no history entry": { + DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, + Tx: &TxMock{}, + Body: `{"name":"Doc","maintainers":["u1","u9"]}`, + RespCode: http.StatusOK, + Committed: 1, + Metadata: 1, + Maintainers: 1, + SearchJobs: 1, + }, + "History hook fetch error": { + DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, + "History entry insert error": { + DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, + Tx: &TxMock{ + InsertDocumentBranchHistoryEntryFunc: func(context.Context, history.Entry) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + History: 1, + }, + "System write records an unattributed entry": { + DB: &DBMock{ + FetchDocumentUnsafeByBranchIDFunc: func(context.Context, xid.ID) (*documentCore.Document, error) { + doc := storedDoc() + doc.LastUpdatedBy = null.StringFrom("u5") + + return doc, nil + }, + }, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil + }, + FetchDocumentMaintainersFunc: func(context.Context, xid.ID, string) ([]string, error) { + return []string{"u1", "u9"}, nil + }, + }, + Body: `{"name":"Renamed","system":true}`, + RespCode: http.StatusOK, + Committed: 1, + Metadata: 1, + SearchJobs: 1, + History: 1, + HistoryCheck: func(t *testing.T, entry history.Entry) { + assert.Equal(t, _branchID, entry.BranchID) + assert.Equal(t, "Renamed", entry.DocumentName) + assert.False(t, entry.LastUpdatedBy.Valid) + assert.False(t, entry.Boundary) + assert.Equal(t, history.Hooks{ + { + Type: hookCore.TypeScheduledReminder, + Settings: processor.Settings(`{"scale":"linear"}`), + }, + }, entry.Hooks) + }, + }, "Missing branch ID parameter": { DB: &DBMock{}, Tx: &TxMock{}, @@ -209,6 +283,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, "Maintainer upsert error": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, @@ -219,6 +294,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, "Search job insertion error on default branch": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, @@ -233,6 +309,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, SearchJobs: 1, + History: 1, }, "Commit error": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, @@ -248,6 +325,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { RespCode: http.StatusInternalServerError, Committed: 1, SearchJobs: 1, + History: 1, }, "Successful update with new maintainers": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, @@ -258,6 +336,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { Metadata: 1, Maintainers: 1, SearchJobs: 1, + History: 1, }, "Successful update without new maintainers": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, @@ -271,6 +350,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { Committed: 1, Metadata: 1, SearchJobs: 1, + History: 1, }, "Non-default branch queues its own search job": { DB: &DBMock{ @@ -289,6 +369,7 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { Metadata: 1, SearchJobs: 1, BranchID: _branchID2, + History: 1, }, } @@ -307,6 +388,17 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { assert.Equal(t, c.Metadata, cnt.metadata) assert.Equal(t, c.Maintainers, cnt.maintainers) assert.Len(t, c.Tx.InsertSearchJobCalls(), c.SearchJobs) + assert.Len(t, c.Tx.InsertDocumentBranchHistoryEntryCalls(), c.History) + + if c.HistoryCheck != nil { + c.HistoryCheck(t, c.Tx.InsertDocumentBranchHistoryEntryCalls()[0].Entry) + } + + if cn == "Successful update with new maintainers" { + entry := c.Tx.InsertDocumentBranchHistoryEntryCalls()[0].Entry + assert.Equal(t, null.StringFrom("u9"), entry.LastUpdatedBy) + assert.Equal(t, history.Hooks{}, entry.Hooks) + } // the job is scoped to the persisted branch, whichever branch // it is. @@ -356,7 +448,30 @@ func Test_Handler_MergeBranches(t *testing.T) { Committed int Metadata int Reviewers int + // History is how many history entries the merge records. + History int }{ + "History hook fetch error": { + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, + "History entry insert error": { + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{ + InsertDocumentBranchHistoryEntryFunc: func(context.Context, history.Entry) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + History: 1, + }, "No session in context": { DB: &DBMock{}, Tx: &TxMock{}, @@ -461,6 +576,7 @@ func Test_Handler_MergeBranches(t *testing.T) { Committed: 1, Metadata: 1, Reviewers: 1, + History: 1, }, "Hook re-creation error": { DB: &DBMock{ @@ -475,6 +591,7 @@ func Test_Handler_MergeBranches(t *testing.T) { Committed: 1, Metadata: 1, Reviewers: 1, + History: 1, }, "Hook insertion error": { DB: &DBMock{ @@ -492,6 +609,7 @@ func Test_Handler_MergeBranches(t *testing.T) { Committed: 1, Metadata: 1, Reviewers: 1, + History: 1, }, "Document update error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, @@ -512,6 +630,7 @@ func Test_Handler_MergeBranches(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, "Tag replacement error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, @@ -522,6 +641,7 @@ func Test_Handler_MergeBranches(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, "Commit error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, @@ -533,6 +653,7 @@ func Test_Handler_MergeBranches(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, Committed: 1, + History: 1, }, "Successful merge": { DB: &DBMock{ @@ -547,6 +668,7 @@ func Test_Handler_MergeBranches(t *testing.T) { Committed: 1, Metadata: 1, Reviewers: 1, + History: 1, }, } @@ -569,8 +691,20 @@ func Test_Handler_MergeBranches(t *testing.T) { assert.Len(t, c.Tx.CommitCalls(), c.Committed) assert.Equal(t, c.Metadata, cnt.metadata) assert.Equal(t, c.Reviewers, cnt.reviewers) + assert.Len(t, c.Tx.InsertDocumentBranchHistoryEntryCalls(), c.History) if c.RespCode == http.StatusOK { + // the merge is a boundary entry of the target, listing the + // source's hooks and attributed to the merging user. + entry := c.Tx.InsertDocumentBranchHistoryEntryCalls()[0].Entry + assert.Equal(t, _branchID, entry.BranchID) + assert.True(t, entry.Boundary) + assert.Equal(t, null.StringFrom("u1"), entry.LastUpdatedBy) + + hh := c.Tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, hh, 1) + assert.Equal(t, _branchID2, hh[0].BranchID) + // hooks are soft-deleted on the target inside the // transaction and re-created from the source branch once it // commits, since creating one creates its watcher. @@ -666,7 +800,38 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { HookFetchErr error RespCode int Committed int + // History is how many history entries the fork records. + History int }{ + "History hook fetch error": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, + "History entry insert error": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Tx: &TxMock{ + InsertDocumentBranchHistoryEntryFunc: func(context.Context, history.Entry) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + History: 1, + }, "No session in context": { DB: &DBMock{}, Tx: &TxMock{}, @@ -754,6 +919,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { // leaves the branch itself standing. RespCode: http.StatusCreated, Committed: 1, + History: 1, }, "Commit error": { DB: &DBMock{ @@ -769,6 +935,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, Committed: 1, + History: 1, }, "Search job insert error": { DB: &DBMock{ @@ -783,6 +950,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, "Successful branch creation": { DB: &DBMock{ @@ -795,6 +963,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { Body: validBody, RespCode: http.StatusCreated, Committed: 1, + History: 1, }, "Tag copy error": { DB: &DBMock{FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { @@ -807,6 +976,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { }, Body: validBody, RespCode: http.StatusInternalServerError, + History: 1, }, } @@ -827,6 +997,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { assert.Equal(t, c.RespCode, rec.Code) assert.Len(t, c.Tx.CommitCalls(), c.Committed) assert.Empty(t, c.Tx.InsertDocumentHookCalls()) + assert.Len(t, c.Tx.InsertDocumentBranchHistoryEntryCalls(), c.History) if c.RespCode == http.StatusCreated { require.Len(t, c.Tx.InsertDocumentBranchCalls(), 1) @@ -836,6 +1007,15 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { assert.False(t, forked.Protected) assert.Nil(t, forked.RawContent) + // the fork starts with a boundary entry of its own content, + // listing the source's hooks. + entry := c.Tx.InsertDocumentBranchHistoryEntryCalls()[0].Entry + assert.Equal(t, forked.BranchID, entry.BranchID) + assert.True(t, entry.Boundary) + assert.Equal(t, null.StringFrom("u1"), entry.LastUpdatedBy) + require.Len(t, c.Tx.FetchDocumentHooksByBranchIDCalls(), 1) + assert.Equal(t, _branchID, c.Tx.FetchDocumentHooksByBranchIDCalls()[0].BranchID) + // the fork takes the source's tags before the commit. require.Len(t, c.Tx.CopyBranchTagsCalls(), 1) assert.Equal(t, _branchID, c.Tx.CopyBranchTagsCalls()[0].FromBranchID) @@ -1274,3 +1454,79 @@ func Test_Handler_UpdateDocumentBranch(t *testing.T) { }) } } + +func Test_Handler_insertBoundaryEntry(t *testing.T) { + cc := map[string]struct { + Tx *TxMock + Err error + Hooks history.Hooks + }{ + "Error returned by Tx.FetchDocumentHooksByBranchID": { + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, assert.AnError + }, + }, + Err: assert.AnError, + }, + "Error returned by Tx.InsertDocumentBranchHistoryEntry": { + Tx: &TxMock{ + InsertDocumentBranchHistoryEntryFunc: func(context.Context, history.Entry) error { + return assert.AnError + }, + }, + Err: assert.AnError, + }, + "Successful insert": { + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil + }, + }, + Hooks: history.Hooks{ + { + Type: hookCore.TypeScheduledReminder, + Settings: processor.Settings(`{"scale":"linear"}`), + }, + }, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + hdl, _ := newTestHandler(&DBMock{}, &fakePublisher{}) + doc := branchDoc(_branchID2) + + err := hdl.insertBoundaryEntry( + context.Background(), + c.Tx, + *doc, + _branchID, + auth.Session{UserID: "u1", ActiveOrganizationID: "org1"}, + ) + testutil.AssertEqualError(t, c.Err, err) + + ff := c.Tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, ff, 1) + assert.Equal(t, _branchID, ff[0].BranchID) + assert.Equal(t, "org1", ff[0].OrganizationID) + + if err != nil && len(c.Tx.InsertDocumentBranchHistoryEntryCalls()) == 0 { + return + } + + require.Len(t, c.Tx.InsertDocumentBranchHistoryEntryCalls(), 1) + entry := c.Tx.InsertDocumentBranchHistoryEntryCalls()[0].Entry + assert.Equal(t, _branchID2, entry.BranchID) + assert.Equal(t, doc.DocumentName, entry.DocumentName) + assert.True(t, entry.Boundary) + assert.Equal(t, null.StringFrom("u1"), entry.LastUpdatedBy) + + if c.Hooks != nil { + assert.Equal(t, c.Hooks, entry.Hooks) + } + }) + } +} diff --git a/server/core/internal/server/internal/document/document_http.go b/server/core/internal/server/internal/document/document_http.go index 452d048f..0009ad1f 100644 --- a/server/core/internal/server/internal/document/document_http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -14,6 +14,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/apps/webchange" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/search" @@ -1016,6 +1017,9 @@ type DocumentsDBAgent interface { // UpdateDocument should update the document. UpdateDocument(ctx context.Context, doc documentCore.Document) error + // InsertDocumentBranchHistoryEntry should record a snapshot of a branch. + InsertDocumentBranchHistoryEntry(ctx context.Context, entry history.Entry) error + // DeleteDocument should delete the document and report the ids of // the document and of every cascade-deleted descendant. DeleteDocument(ctx context.Context, id xid.ID, organizationID string) ([]xid.ID, error) diff --git a/server/core/internal/server/internal/document/document_http_test.go b/server/core/internal/server/internal/document/document_http_test.go index c06c99b8..885322f8 100644 --- a/server/core/internal/server/internal/document/document_http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -1552,8 +1552,8 @@ func Test_Handler_DuplicateDocument(t *testing.T) { { Type: documentCore.BlockNodeImageBlock, Attrs: documentCore.Attributes{ - "uid": "img1", - "src": "https://app.test/core" + documentCore.FilePath(_documentID, "img1", "shot.png"), + "uid": "img-1-aaaaaaaaaaaaaaa", + "src": "https://app.test/core" + documentCore.FilePath(_documentID, "img-1-aaaaaaaaaaaaaaa", "shot.png"), }, }, }, @@ -1683,7 +1683,7 @@ func Test_Handler_DuplicateDocument(t *testing.T) { "Hooks are copied along with the document": { DB: func() *DBMock { blockHook := storedHook(hookCore.TypeScheduledReminder) - blockHook.BlockID = null.StringFrom("img1") + blockHook.BlockID = null.StringFrom("img-1-aaaaaaaaaaaaaaa") db := imageDB() db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { @@ -1761,7 +1761,7 @@ func Test_Handler_DuplicateDocument(t *testing.T) { // uid rather than pointing at the source's. for _, call := range c.DB.InsertDocumentHookCalls() { assert.Equal(t, null.ValueFrom(dupl.BranchID), call.Hk.BranchID) - assert.NotEqual(t, null.StringFrom("img1"), call.Hk.BlockID) + assert.NotEqual(t, null.StringFrom("img-1-aaaaaaaaaaaaaaa"), call.Hk.BlockID) assert.True(t, call.Hk.BlockID.Valid) } } diff --git a/server/core/internal/server/internal/document/files/http.go b/server/core/internal/server/internal/document/files/http.go index 620d8e91..576f3e99 100644 --- a/server/core/internal/server/internal/document/files/http.go +++ b/server/core/internal/server/internal/document/files/http.go @@ -19,12 +19,7 @@ import ( "github.com/rs/xid" ) -// _fileIDLength is the length of a block uid: every editor mints -// 21-character nanoids, and the retrieval route relies on it to cut the -// id off the front of "-". -const _fileIDLength = 21 - -// _fileIDPattern matches a block uid. The charset shuts out the path +// _fileIDPattern matches a file id. The charset shuts out the path // separators and dot segments that would otherwise let an id escape its // storage folder once joined into an object key. var _fileIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{21}$`) @@ -172,11 +167,11 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { // fileID cuts the id off a "-" route segment, reporting // whether the segment has that shape. func fileID(ref string) (string, bool) { - if len(ref) <= _fileIDLength || ref[_fileIDLength] != '-' { + if len(ref) <= document.FileIDLength || ref[document.FileIDLength] != '-' { return "", false } - id := ref[:_fileIDLength] + id := ref[:document.FileIDLength] if !_fileIDPattern.MatchString(id) { return "", false } diff --git a/server/core/internal/server/internal/document/files/http_test.go b/server/core/internal/server/internal/document/files/http_test.go index 6c3adb68..a72fab05 100644 --- a/server/core/internal/server/internal/document/files/http_test.go +++ b/server/core/internal/server/internal/document/files/http_test.go @@ -16,6 +16,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document/file" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/internal/storage" + "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/rs/xid" "github.com/stretchr/testify/assert" @@ -322,6 +323,21 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { wasUploadCalled(0), ), }, + "Existing file id": { + DB: &DBMock{ + InsertDocumentFileFunc: func(context.Context, file.File) error { + return errutil.New(http.StatusConflict, "document_file.exists", "file id is already in use") + }, + }, + Storer: &StorerMock{}, + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", + Checks: checks( + hasResp(http.StatusConflict, `{"code":"document_file.exists","message":"file id is already in use"}`), + wasInsertCalled(1), + wasUploadCalled(0), + wasDeleteCalled(0), + ), + }, "DB insert error": { DB: &DBMock{ InsertDocumentFileFunc: func(context.Context, file.File) error { diff --git a/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts b/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts index b147bb89..14b88585 100644 --- a/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts +++ b/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts @@ -457,8 +457,12 @@ describe("", { concurrent: false }, () => { await vi.waitFor(() => { expect(calls).toHaveLength(1) }, WAIT_FOR_OPTIONS) + // the upload carries an id of its own, never the block's uid, so a + // replacement leaves the previous object in place + const fileId = String(calls[0]?.query.id) + expect(fileId).toMatch(/^[A-Za-z0-9_-]{21}$/) expect(calls[0]?.query).toEqual({ - id: "file-1", + id: fileId, location: "document", kind: "file", }) @@ -470,9 +474,8 @@ describe("", { concurrent: false }, () => { }) expect(updateAttributes).toHaveBeenNthCalledWith(2, { src: matchingString( - new RegExp(`/api/documents/${DOCUMENT_ID}/files/file-1-notes.zip$`), + new RegExp(`/api/documents/${DOCUMENT_ID}/files/${fileId}-notes.zip$`), ), - uid: "file-1", name: "notes.zip", size: 6, contentType: "application/zip", @@ -481,30 +484,6 @@ describe("", { concurrent: false }, () => { expect(toast.custom).toHaveBeenCalledTimes(0) }) - it("uploads under a generated id when the node has none", async ({ - expect, - }) => { - const calls = mockEndpoint( - "POST", - `/api/documents/${DOCUMENT_ID}/files`, - (_call, event) => { - setResponseHeader(event, "location", "https://cdn.test/stored.zip") - - return uploadedZip() - }, - ) - const updateAttributes = vi.fn() - const wrapper = await mountFile({ uid: null }, updateAttributes) - - await pickFile(wrapper, zipFile()) - - await vi.waitFor(() => { - expect(calls).toHaveLength(1) - }, WAIT_FOR_OPTIONS) - expect(calls[0]?.query.id).toEqual(expect.any(String)) - expect(calls[0]?.query.id).not.toBe("") - }) - it("does nothing when the file dialog is dismissed", async ({ expect }) => { const calls = mockEndpoint( "POST", diff --git a/web/app/components/editor/blocks/file/FileBlock.vue b/web/app/components/editor/blocks/file/FileBlock.vue index d2432a9d..9f014810 100644 --- a/web/app/components/editor/blocks/file/FileBlock.vue +++ b/web/app/components/editor/blocks/file/FileBlock.vue @@ -99,21 +99,20 @@ async function handleFileChange(event: Event) { return } - const blockId = (props.node.attrs.uid as string) || nanoid() + const fileId = nanoid() props.updateAttributes({ uploading: true, name: file.name, size: file.size }) try { const uploaded = await uploadDocumentFile.mutateAsync({ documentId: documentId.value, - id: blockId, + id: fileId, loc: DocumentFileLocation.Document, kind: DocumentFileKind.File, file, }) props.updateAttributes({ - src: buildDocumentFileSrc(documentId.value, blockId, uploaded.name), - uid: blockId, + src: buildDocumentFileSrc(documentId.value, fileId, uploaded.name), name: uploaded.name, size: uploaded.size, contentType: uploaded.contentType, diff --git a/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts b/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts index b61ab960..9edf3ac8 100644 --- a/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts +++ b/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts @@ -425,8 +425,12 @@ describe("", { concurrent: false }, () => { await vi.waitFor(() => { expect(calls).toHaveLength(1) }, WAIT_FOR_OPTIONS) + // the upload carries an id of its own, never the block's uid, so a + // replacement leaves the previous object in place + const fileId = String(calls[0]?.query.id) + expect(fileId).toMatch(/^[A-Za-z0-9_-]{21}$/) expect(calls[0]?.query).toEqual({ - id: "image-1", + id: fileId, location: "document", kind: "image", }) @@ -434,38 +438,13 @@ describe("", { concurrent: false }, () => { expect(updateAttributes).toHaveBeenNthCalledWith(1, { uploading: true }) expect(updateAttributes).toHaveBeenNthCalledWith(2, { src: matchingString( - new RegExp(`/api/documents/${DOCUMENT_ID}/files/image-1-shot.png$`), + new RegExp(`/api/documents/${DOCUMENT_ID}/files/${fileId}-shot.png$`), ), - uid: "image-1", uploading: false, }) expect(toast.custom).toHaveBeenCalledTimes(0) }) - it("uploads under a generated id when the node has none", async ({ - expect, - }) => { - const calls = mockEndpoint( - "POST", - `/api/documents/${DOCUMENT_ID}/files`, - (_call, event) => { - setResponseHeader(event, "location", "https://cdn.test/stored.png") - - return uploadedImage() - }, - ) - const updateAttributes = vi.fn() - const wrapper = await mountImage({ uid: null }, updateAttributes) - - await pickFile(wrapper, pngFile()) - - await vi.waitFor(() => { - expect(calls).toHaveLength(1) - }, WAIT_FOR_OPTIONS) - expect(calls[0]?.query.id).toEqual(expect.any(String)) - expect(calls[0]?.query.id).not.toBe("") - }) - it("does nothing when the file dialog is dismissed", async ({ expect }) => { const calls = mockEndpoint( "POST", diff --git a/web/app/components/editor/blocks/image/ImageBlock.vue b/web/app/components/editor/blocks/image/ImageBlock.vue index 472dcd50..577f4259 100644 --- a/web/app/components/editor/blocks/image/ImageBlock.vue +++ b/web/app/components/editor/blocks/image/ImageBlock.vue @@ -176,24 +176,24 @@ async function handleFileChange(event: Event) { return } - const blockId = (props.node.attrs.uid as string) || nanoid() + const fileId = nanoid() props.updateAttributes({ uploading: true }) try { const uploaded = await uploadDocumentFile.mutateAsync({ documentId: documentId.value, - id: blockId, + id: fileId, loc: DocumentFileLocation.Document, kind: DocumentFileKind.Image, file, }) const nextSrc = buildDocumentFileSrc( documentId.value, - blockId, + fileId, uploaded.name, ) - props.updateAttributes({ src: nextSrc, uid: blockId, uploading: false }) + props.updateAttributes({ src: nextSrc, uploading: false }) } catch (error) { props.updateAttributes({ uploading: false }) if (!handleStorageError(error, t)) { diff --git a/web/app/components/editor/blocks/upload-handler.test.ts b/web/app/components/editor/blocks/upload-handler.test.ts index 63a03df0..2cb28482 100644 --- a/web/app/components/editor/blocks/upload-handler.test.ts +++ b/web/app/components/editor/blocks/upload-handler.test.ts @@ -27,8 +27,8 @@ vi.mock("~/components/toast", () => ({ // buildDocumentFileSrc is pulled in through a nuxt auto-import, so the // module itself is the only seam: the real one reaches for the nuxt app vi.mock("~/composables/api/useDocumentFileAPI", () => ({ - buildDocumentFileSrc: (documentId: string, blockId: string, name: string) => - `/files/${documentId}/${blockId}-${name}`, + buildDocumentFileSrc: (documentId: string, fileId: string, name: string) => + `/files/${documentId}/${fileId}-${name}`, })) const extensions = [ @@ -135,6 +135,13 @@ function blockUid(doc: PMNode, typeName: string): string { return blockAttrs(doc, typeName).uid as string } +// the id every upload is sent under: a 21-character nanoid +const FILE_ID_PATTERN = /^[A-Za-z0-9_-]{21}$/ + +function uploadedFileId(): string { + return (uploadDocumentFile.mock.calls[0]?.[0] as { id: string }).id +} + // the upload chain is pure microtasks, so one macrotask boundary is // enough to settle it — no timers involved function flushUpload(): Promise { @@ -177,11 +184,15 @@ describe("createUploadFileHandler", { concurrent: false }, () => { expect(uploadDocumentFile).toHaveBeenCalledTimes(1) expect(uploadDocumentFile).toHaveBeenCalledWith({ documentId: "d1", - id: blockUid(editor.state.doc, IMAGE_BLOCK_NAME), + id: expect.stringMatching(FILE_ID_PATTERN) as string, loc: DocumentFileLocation.Document, kind: DocumentFileKind.Image, file, }) + // the file id is the upload's own, not the block's uid + expect(uploadedFileId()).not.toBe( + blockUid(editor.state.doc, IMAGE_BLOCK_NAME), + ) expect(showToastMessage).toHaveBeenCalledTimes(0) }) @@ -211,11 +222,14 @@ describe("createUploadFileHandler", { concurrent: false }, () => { expect(uploadDocumentFile).toHaveBeenCalledTimes(1) expect(uploadDocumentFile).toHaveBeenCalledWith({ documentId: "d1", - id: blockUid(editor.state.doc, FILE_BLOCK_NAME), + id: expect.stringMatching(FILE_ID_PATTERN) as string, loc: DocumentFileLocation.Document, kind: DocumentFileKind.File, file, }) + expect(uploadedFileId()).not.toBe( + blockUid(editor.state.doc, FILE_BLOCK_NAME), + ) expect(showToastMessage).toHaveBeenCalledTimes(0) }) @@ -250,13 +264,13 @@ describe("createUploadFileHandler", { concurrent: false }, () => { const editor = makeEditor([paragraph("one")]) editor.commands.setTextSelection(2) fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) - const uid = blockUid(editor.state.doc, IMAGE_BLOCK_NAME) + const fileId = uploadedFileId() await flushUpload() expect(shape(editor.state.doc)).toEqual([ "paragraph", - `imageBlock:/files/d1/${uid}-a.zip:false`, + `imageBlock:/files/d1/${fileId}-a.zip:false`, ]) expect(showToastMessage).toHaveBeenCalledTimes(0) }) @@ -274,13 +288,13 @@ describe("createUploadFileHandler", { concurrent: false }, () => { fileCallbacks({ documentId: "d1" }).onPaste(editor, [ zipFile("notes.zip"), ]) - const uid = blockUid(editor.state.doc, FILE_BLOCK_NAME) + const fileId = uploadedFileId() await flushUpload() expect(shape(editor.state.doc)).toEqual([ "paragraph", - `fileBlock:/files/d1/${uid}-renamed.zip:false`, + `fileBlock:/files/d1/${fileId}-renamed.zip:false`, ]) expect(blockAttrs(editor.state.doc, FILE_BLOCK_NAME)).toMatchObject({ name: "renamed.zip", diff --git a/web/app/components/editor/blocks/upload-handler.ts b/web/app/components/editor/blocks/upload-handler.ts index d4a94368..945de702 100644 --- a/web/app/components/editor/blocks/upload-handler.ts +++ b/web/app/components/editor/blocks/upload-handler.ts @@ -78,6 +78,7 @@ function insertBlockWithUpload( const isImage = IMAGE_MIME_TYPES.has(file.type) const nodeName = isImage ? IMAGE_BLOCK_NAME : FILE_BLOCK_NAME const blockId = nanoid() + const fileId = nanoid() const { state, view } = editor const { schema } = state @@ -109,13 +110,13 @@ function insertBlockWithUpload( uploadDocumentFile .mutateAsync({ documentId, - id: blockId, + id: fileId, loc: DocumentFileLocation.Document, kind: isImage ? DocumentFileKind.Image : DocumentFileKind.File, file, }) .then((uploaded) => { - const src = buildDocumentFileSrc(documentId, blockId, uploaded.name) + const src = buildDocumentFileSrc(documentId, fileId, uploaded.name) updateAttrsByUid( editor,