From 3f66b15b433e328c79dd76c5eb00533bcdb4658e Mon Sep 17 00:00:00 2001 From: david <58231312+davseby@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:13:54 +0300 Subject: [PATCH] add document files cleanup logic --- docker/env/core.example.env | 6 + server/CLAUDE.md | 11 + server/core/cmd/core/main.go | 72 +++ server/core/internal/db/db.go | 10 + server/core/internal/db/document_branch.go | 31 ++ .../core/internal/db/document_branch_test.go | 170 ++++++ .../core/internal/db/document_comment_test.go | 2 +- server/core/internal/db/document_file.go | 123 +++- server/core/internal/db/document_file_test.go | 184 +++++- server/core/internal/db/document_hook_test.go | 10 +- server/core/internal/db/document_test.go | 53 ++ .../internal/db/migrations/001_initial.sql | 8 +- server/core/internal/document/document.go | 17 +- .../core/internal/document/document_test.go | 32 +- server/core/internal/document/file.go | 58 -- server/core/internal/document/file/file.go | 96 ++++ .../core/internal/document/file/file_test.go | 77 +++ .../document/file/manager/0mock_db_test.go | 321 +++++++++++ .../file/manager/0mock_storer_test.go | 90 +++ .../internal/document/file/manager/manager.go | 249 +++++++++ .../document/file/manager/manager_test.go | 524 ++++++++++++++++++ server/core/internal/document/file_test.go | 31 -- server/core/internal/document/hook/hook.go | 9 +- .../core/internal/document/hook/hook_test.go | 2 +- .../internal/document/hook/manager/manager.go | 87 +-- .../document/hook/manager/manager_test.go | 37 +- server/core/internal/document/node.go | 106 +++- server/core/internal/document/node_test.go | 84 ++- server/core/internal/server/0mock_db_test.go | 248 +++++---- .../core/internal/server/0mock_storer_test.go | 71 +++ .../server/internal/document/0mock_db_test.go | 114 ++++ .../internal/document/0mock_storer_test.go | 102 ++++ .../server/internal/document/0mock_tx_test.go | 114 ++++ .../internal/document/branch_http_test.go | 174 +++++- .../internal/document/files/0mock_db_test.go | 77 ++- .../document/files/0mock_storer_test.go | 59 -- .../server/internal/document/files/http.go | 59 +- .../internal/document/files/http_test.go | 99 ++-- .../internal/document/hook/http_test.go | 5 +- .../internal/server/internal/document/http.go | 89 ++- .../server/internal/document/http_test.go | 5 +- server/core/internal/server/router.go | 3 - server/core/internal/server/server.go | 6 +- server/core/internal/storage/client_test.go | 49 ++ server/core/internal/storage/object.go | 26 +- server/core/internal/storage/object_test.go | 47 ++ 46 files changed, 3428 insertions(+), 419 deletions(-) delete mode 100644 server/core/internal/document/file.go create mode 100644 server/core/internal/document/file/file.go create mode 100644 server/core/internal/document/file/file_test.go create mode 100644 server/core/internal/document/file/manager/0mock_db_test.go create mode 100644 server/core/internal/document/file/manager/0mock_storer_test.go create mode 100644 server/core/internal/document/file/manager/manager.go create mode 100644 server/core/internal/document/file/manager/manager_test.go delete mode 100644 server/core/internal/document/file_test.go create mode 100644 server/core/internal/server/internal/document/0mock_storer_test.go diff --git a/docker/env/core.example.env b/docker/env/core.example.env index b451ed8f..45c63d54 100644 --- a/docker/env/core.example.env +++ b/docker/env/core.example.env @@ -44,6 +44,12 @@ OXYNOTE_CORE_SLACK_REDIRECT_URL=http://localhost:8080/slack OXYNOTE_CORE_SLACK_INSTALLATION_SIGNING_SECRET= OXYNOTE_CORE_ANTHROPIC_API_KEY= +# document changelog retention. Snapshots pin the files they reference, so +# these bounds also decide how long a removed image survives. 0 disables +# the respective bound. +OXYNOTE_CORE_DB_MAX_DOCUMENT_CHANGELOGS=100 +OXYNOTE_CORE_DB_DOCUMENT_CHANGELOG_RETENTION=2160h + # secrets OXYNOTE_CORE_DB_DATA_SOURCE_CREDENTIALS_SIGNING_SECRET=devsigningsecret diff --git a/server/CLAUDE.md b/server/CLAUDE.md index ea51ffed..c3b9e012 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -77,6 +77,17 @@ This is the model that's least obvious from a fresh read: - `onLoadDocument` / `onStoreDocument` round-trip through core's internal `/api/x/documents/{id}/branch/{branchId}` endpoints. **Do not** use `Y.applyUpdate` to seed a freshly-created doc with content from another doc — the differing `clientID`s CRDT-merge and silently duplicate content. The codebase's pattern is `replaceYdocContent` in `server/auth-realtime/src/ydocument.ts`, which deep-clones `Y.XmlElement`s (preserving non-string attrs that `XmlElement.clone()` drops). Read the comments at `server/auth-realtime/src/hocuspocus.ts:154` and `server/auth-realtime/src/routes.ts:73` before touching this area. - Branch merging: `PUT /documents/:documentId/merge` is handled by auth-realtime, which proxies to core, then **directly mutates the in-memory target-branch Y.Doc** via `replaceYdocContent` (not `applyUpdate`) and immediately persists `rawContent` back through `/api/x/.../branch/:branchId` so a server restart can't reset the clientID and duplicate content on reconnect. +## External resource lifecycle (files, hooks) + +Uploaded images live in MinIO under `organizations/{org}/documents/{doc}/files/{blockUID}` — the file id **is** the image block's `uid` — and are tracked in `document_files`. Hooks hold external resources too (changedetection.io watchers, GitHub tracking). Both are reclaimed by sweeps, not by request handlers: + +- **The row outlives its owner.** `document_files` and `document_hooks` reference documents, branches and organizations with `ON DELETE SET NULL`, so a deleted document (or an org deleted by Better Auth, which core never sees) leaves rows behind rather than destroying the only record of the external resource. `document_files.storage_key` stores the object key outright so it stays reachable once the FKs are gone. +- **`internal/document/file/manager`** ticks every 5 minutes: it trims expired changelog snapshots, then reclaims files. A file is referenced if its id appears in any branch content of its document, any retained `document_branch_changelogs` snapshot, or any comment/reply — checked in SQL (`CheckDocumentFileReferenced`), so a spurious match only ever retains. Unreferenced files are stamped `unreferenced_at` and deleted a day later; rows with NULL FKs skip the wait; files younger than a day are never touched, which covers the gap between an upload and the first content persist that mentions it. +- **`internal/document/hook/manager`** does the same for hooks: a row whose branch or document went NULL gets `Hook.Delete` (external teardown) before the row is dropped. +- **Ordering is inverted between create and delete.** Creating writes the row before the object (upload, duplication); deleting removes the object before the row. Both leave at most a row without an object, which the sweep resolves — an object without a row is invisible to every cleanup path. +- **Duplication copies, never shares.** `Document.Duplicate` regenerates every block uid, rewrites image `src` paths to the new document (swapping only the path, never the host), and returns the old→new id map the handler uses to copy each object server-side into the duplicate's own folder. Files are therefore owned by exactly one document. +- Snapshots pin the files they reference, so `DB_MAX_DOCUMENT_CHANGELOGS` (count, trimmed on insert) and `DB_DOCUMENT_CHANGELOG_RETENTION` (age, trimmed in the sweep) also decide how long a removed image survives. + The ProseMirror schema is defined in `server/auth-realtime/src/schema/` (one file per block kind, `index.ts` aggregates). The Go-side mirror is `server/core/internal/document/node.go` (`RootBlock`, `Block`, `Mark`). ## Database diff --git a/server/core/cmd/core/main.go b/server/core/cmd/core/main.go index 9f869edf..786128d3 100644 --- a/server/core/cmd/core/main.go +++ b/server/core/cmd/core/main.go @@ -23,6 +23,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/assistant/edit" "github.com/oxynote/oxynote/server/core/internal/buildinfo" "github.com/oxynote/oxynote/server/core/internal/db" + fileMan "github.com/oxynote/oxynote/server/core/internal/document/file/manager" hookMan "github.com/oxynote/oxynote/server/core/internal/document/hook/manager" "github.com/oxynote/oxynote/server/core/internal/email" "github.com/oxynote/oxynote/server/core/internal/notification" @@ -47,6 +48,14 @@ const ( // retained per user. _maxNotifications = 500 + // _defaultMaxDocumentChangelogs specifies how many changelog snapshots + // are retained per document branch when the environment says nothing. + _defaultMaxDocumentChangelogs = 100 + + // _defaultDocumentChangelogRetention specifies how long changelog + // snapshots are retained when the environment says nothing. + _defaultDocumentChangelogRetention = time.Hour * 24 * 90 + // _sentryDedupInterval specifies how long a reported error suppresses // duplicate reports. _sentryDedupInterval = 10 * time.Minute @@ -72,9 +81,34 @@ func main() { //nolint:maintidx // main performs linear wiring of all components metrics, cleanup := newMetrics(log) defer cleanup() + maxDocumentChangelogs, err := parseUintEnv("DB_MAX_DOCUMENT_CHANGELOGS", _defaultMaxDocumentChangelogs) + if err != nil { + err = ioutil.AppendCloseErr(ioutil.MultiCloser(true, closers...), err) + + log.With("error", err). + Error("cannot read the configuration") + + return + } + + documentChangelogRetention, err := parseDurationEnv( + "DB_DOCUMENT_CHANGELOG_RETENTION", + _defaultDocumentChangelogRetention, + ) + if err != nil { + err = ioutil.AppendCloseErr(ioutil.MultiCloser(true, closers...), err) + + log.With("error", err). + Error("cannot read the configuration") + + return + } + dbc, err := db.New(log, metrics, db.Options{ DSN: buildinfo.Getenv("DB_DSN"), MaxNotifications: _maxNotifications, + MaxDocumentChangelogs: maxDocumentChangelogs, + DocumentChangelogRetention: documentChangelogRetention, DataSourceCredentialsSigningSecret: buildinfo.Getenv("DB_DATA_SOURCE_CREDENTIALS_SIGNING_SECRET"), }) if err != nil { @@ -303,6 +337,9 @@ func main() { //nolint:maintidx // main performs linear wiring of all components hooksMan := hookMan.NewManager(log, dbc, githubMan, webchangeClient, notifMan) searchManager := searchMan.NewManager(log, dbc, searchClient) + filesMan := fileMan.NewManager(log, dbc, storageClient, fileMan.Options{ + ChangelogRetention: documentChangelogRetention, + }) closers = append([]io.Closer{srv}, closers...) @@ -323,6 +360,9 @@ func main() { //nolint:maintidx // main performs linear wiring of all components backgroundWg.Go(func() { searchManager.Start(termCtx) }) + backgroundWg.Go(func() { + filesMan.Start(termCtx) + }) <-termCtx.Done() @@ -406,6 +446,38 @@ func newMetrics(log *slog.Logger) (metricutil.Factory, func()) { } } +// parseUintEnv reads an unsigned integer from the environment, falling back +// to the given default when the variable is unset. +func parseUintEnv(name string, def uint64) (uint64, error) { + v := buildinfo.Getenv(name) + if v == "" { + return def, nil + } + + res, err := strconv.ParseUint(v, 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid %s: %w", name, err) + } + + return res, nil +} + +// parseDurationEnv reads a duration from the environment, falling back to +// the given default when the variable is unset. +func parseDurationEnv(name string, def time.Duration) (time.Duration, error) { + v := buildinfo.Getenv(name) + if v == "" { + return def, nil + } + + res, err := time.ParseDuration(v) + if err != nil { + return 0, fmt.Errorf("invalid %s: %w", name, err) + } + + return res, nil +} + // parseOrigins splits the configured allowed-origin list. An empty value // yields no origins at all, which is what leaves origin checking disabled: // strings.Split would instead return a one-element slice holding "", which diff --git a/server/core/internal/db/db.go b/server/core/internal/db/db.go index 7030dbee..62e98b83 100644 --- a/server/core/internal/db/db.go +++ b/server/core/internal/db/db.go @@ -86,6 +86,16 @@ type Options struct { // Zero means no limit. MaxNotifications uint64 + // MaxDocumentChangelogs specifies the maximum number of changelog + // snapshots to keep per document branch. + // Zero means no limit. + MaxDocumentChangelogs uint64 + + // DocumentChangelogRetention specifies how long changelog snapshots + // are kept before the file manager trims them. + // Zero means no age limit. + DocumentChangelogRetention time.Duration + // DataSourceCredentialsSigningSecret is the secret used to sign // data source credentials. DataSourceCredentialsSigningSecret string diff --git a/server/core/internal/db/document_branch.go b/server/core/internal/db/document_branch.go index ad822cad..82b88ea8 100644 --- a/server/core/internal/db/document_branch.go +++ b/server/core/internal/db/document_branch.go @@ -335,6 +335,37 @@ func (a *agent) insertDocumentBranchChangelog( "content = excluded.content, raw_content = excluded.raw_content, created_at = excluded.created_at"). MustSql() + if _, err := a.sql.ExecContext(ctx, q, args...); err != nil { + return err + } + + return a.trimDocumentBranchChangelogs(ctx, branchID) +} + +// trimDocumentBranchChangelogs keeps only the newest snapshots of a branch. +// Age trimming lives in the file manager instead, since it has to reach +// branches that stopped inserting altogether. +func (a *agent) trimDocumentBranchChangelogs(ctx context.Context, 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 + // snapshot of the branch. + if a.opts.MaxDocumentChangelogs == 0 { + return nil + } + + b := a.builder.Select("id"). + From("document_branch_changelogs"). + Where(sq.Eq{"fk_branch_id": branchID}). + OrderBy("created_at DESC"). + Limit(a.opts.MaxDocumentChangelogs). + Prefix("id NOT IN ("). + Suffix(")") + + q, args := a.builder.Delete("document_branch_changelogs").Where(sq.And{ + b, + sq.Eq{"fk_branch_id": branchID}, + }).MustSql() + _, err := a.sql.ExecContext(ctx, q, args...) return err diff --git a/server/core/internal/db/document_branch_test.go b/server/core/internal/db/document_branch_test.go index 591f3e75..5d6890ba 100644 --- a/server/core/internal/db/document_branch_test.go +++ b/server/core/internal/db/document_branch_test.go @@ -11,6 +11,7 @@ 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/hook" "github.com/oxynote/oxynote/server/core/pkg/sqlutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" @@ -407,6 +408,7 @@ func Test_agent_DeleteDocumentBranchByID(t *testing.T) { CancelledContext bool BranchID xid.ID OrganizationID string + HookID xid.ID Err error } @@ -421,6 +423,24 @@ func Test_agent_DeleteDocumentBranchByID(t *testing.T) { Err: assert.AnError, } }, + // the hook survives its branch: it holds an external watcher that + // only the hook manager can tear down, and the row is what points + // at it. + "Hooks outlive the branch": func(t *testing.T, db *DB) tcase { + branch := prepDocumentBranches(t, db, 1, nil)[0] + + hk := prepDocumentHooks(t, db, 1, func(_ int, hk *hook.Hook) { + hk.DocumentID = null.ValueFrom(branch.ID) + hk.OrganizationID = branch.OrganizationID + hk.BranchID = null.ValueFrom(branch.BranchID) + })[0] + + return tcase{ + BranchID: branch.BranchID, + OrganizationID: branch.OrganizationID, + HookID: hk.ID, + } + }, "Successful delete": func(t *testing.T, db *DB) tcase { branch := prepDocumentBranches(t, db, 1, nil)[0] @@ -462,6 +482,20 @@ func Test_agent_DeleteDocumentBranchByID(t *testing.T) { err = db.sql.Get(&id, q, args...) testutil.AssertEqualError(t, sql.ErrNoRows, err) + + if c.HookID.IsZero() { + return + } + + var hk hook.Hook + + q, args = db.selectDocumentHook(db.builder.Select()). + Where(sq.Eq{"document_hooks.id": c.HookID}). + MustSql() + + require.NoError(t, db.sql.Get(&hk, q, args...)) + assert.False(t, hk.BranchID.Valid) + assert.True(t, hk.DocumentID.Valid, "the document itself is still there") }) } } @@ -607,11 +641,128 @@ func Test_agent_FetchDocumentUnsafeByBranchID(t *testing.T) { testutil.AssertFilterEqual(t, branch, res) } +// prepChangelogs writes count snapshots of the document's branch, one per +// aggregation bucket so each lands as its own entry, oldest first. +func prepChangelogs(t *testing.T, db *DB, doc *document.Document, count int) []document.Changelog { + t.Helper() + + res := make([]document.Changelog, count) + base := timeutil.Now().Truncate(time.Second) + + for i := range count { + doc.UpdatedAt = base.Add(-time.Duration(count-i) * time.Hour) + clog := doc.Changelog() + + require.NoError(t, db.insertDocumentBranchChangelog( + context.Background(), + doc.ID, + doc.BranchID, + clog, + )) + + res[i] = clog + } + + return res +} + +// countChangelogs returns how many snapshots the branch currently has. +func countChangelogs(t *testing.T, db *DB, branchID xid.ID) int { + t.Helper() + + q, args := db.builder.Select("COUNT(*)"). + From("document_branch_changelogs"). + Where(sq.Eq{"fk_branch_id": branchID}). + MustSql() + + var count int + + require.NoError(t, db.sql.Get(&count, q, args...)) + + return count +} + +func Test_agent_trimDocumentBranchChangelogs(t *testing.T) { + cc := map[string]struct { + Max uint64 + Remaining int + }{ + "Zero keeps every snapshot": { + Max: 0, + Remaining: 4, + }, + "Only the newest snapshots survive": { + Max: 2, + Remaining: 2, + }, + "Limit above the count changes nothing": { + Max: 10, + Remaining: 4, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + db := prepTempDB(t) + doc := prepDocuments(t, db, 1, nil)[0] + + clogs := prepChangelogs(t, db, doc, 4) + + db.opts.MaxDocumentChangelogs = c.Max + + require.NoError(t, db.trimDocumentBranchChangelogs(context.Background(), doc.BranchID)) + assert.Equal(t, c.Remaining, countChangelogs(t, db, doc.BranchID)) + + if c.Max == 0 || c.Remaining == len(clogs) { + return + } + + // the survivors are the newest ones. + var ids []string + + q, args := db.builder.Select("id"). + From("document_branch_changelogs"). + Where(sq.Eq{"fk_branch_id": doc.BranchID}). + MustSql() + + require.NoError(t, db.sql.Select(&ids, q, args...)) + assert.ElementsMatch(t, []string{clogs[2].ID, clogs[3].ID}, ids) + }) + } +} + +func Test_agent_DeleteExpiredDocumentBranchChangelogs(t *testing.T) { + t.Parallel() + + db := prepTempDB(t) + doc := prepDocuments(t, db, 1, nil)[0] + + clogs := prepChangelogs(t, db, doc, 4) + require.Equal(t, 4, countChangelogs(t, db, doc.BranchID)) + + // nothing is old enough yet. + require.NoError(t, db.DeleteExpiredDocumentBranchChangelogs( + context.Background(), + clogs[0].CreatedAt.Add(-time.Second), + )) + assert.Equal(t, 4, countChangelogs(t, db, doc.BranchID)) + + // everything created before the newest snapshot goes. + require.NoError(t, db.DeleteExpiredDocumentBranchChangelogs( + context.Background(), + clogs[3].CreatedAt, + )) + assert.Equal(t, 1, countChangelogs(t, db, doc.BranchID)) +} + func Test_agent_insertDocumentBranchChangelog(t *testing.T) { type tcase struct { DocumentID xid.ID BranchID xid.ID Changelog document.Changelog + Trimmed int Err error } @@ -636,6 +787,21 @@ func Test_agent_insertDocumentBranchChangelog(t *testing.T) { Changelog: doc.Changelog(), } }, + "Insert trims the branch down to the retained snapshots": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + + prepChangelogs(t, db, doc, 3) + + db.opts.MaxDocumentChangelogs = 2 + doc.UpdatedAt = timeutil.Now().Truncate(time.Second) + + return tcase{ + DocumentID: doc.ID, + BranchID: doc.BranchID, + Changelog: doc.Changelog(), + Trimmed: 2, + } + }, "Successful update of an existing entry": func(t *testing.T, db *DB) tcase { doc := prepDocuments(t, db, 1, nil)[0] @@ -666,6 +832,10 @@ func Test_agent_insertDocumentBranchChangelog(t *testing.T) { err := db.insertDocumentBranchChangelog(context.Background(), c.DocumentID, c.BranchID, c.Changelog) testutil.RequireEqualError(t, c.Err, err) + if c.Trimmed != 0 { + assert.Equal(t, c.Trimmed, countChangelogs(t, db, c.BranchID)) + } + if err != nil { return } diff --git a/server/core/internal/db/document_comment_test.go b/server/core/internal/db/document_comment_test.go index a354fbd0..8ad3dfce 100644 --- a/server/core/internal/db/document_comment_test.go +++ b/server/core/internal/db/document_comment_test.go @@ -18,7 +18,7 @@ import ( "github.com/stretchr/testify/require" ) -func prepDocumentComments(t *testing.T, db *DB, count int, fn func(int, *comment.Comment)) []comment.Comment { //nolint:unparam // fixture helpers share a uniform signature +func prepDocumentComments(t *testing.T, db *DB, count int, fn func(int, *comment.Comment)) []comment.Comment { t.Helper() res := make([]comment.Comment, count) diff --git a/server/core/internal/db/document_file.go b/server/core/internal/db/document_file.go index 8f7c8d32..1114a5d4 100644 --- a/server/core/internal/db/document_file.go +++ b/server/core/internal/db/document_file.go @@ -2,22 +2,36 @@ package db import ( "context" + "time" sq "github.com/Masterminds/squirrel" + "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/file" + "github.com/rs/xid" ) // InsertDocumentFile inserts a new document file into the database. -func (a *agent) InsertDocumentFile(ctx context.Context, f document.File) error { +// 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. +func (a *agent) InsertDocumentFile(ctx context.Context, f file.File) error { q, args := a.builder.Insert("document_files"). SetMap(map[string]any{ "id": f.ID, "location": f.Location, + "storage_key": f.StorageKey, "fk_document_id": f.DocumentID, "fk_organization_id": f.OrganizationID, "created_at": f.CreatedAt, - }).MustSql() + "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, " + + "created_at = excluded.created_at, unreferenced_at = NULL"). + MustSql() _, err := a.sql.ExecContext(ctx, q, args...) @@ -25,7 +39,7 @@ func (a *agent) InsertDocumentFile(ctx context.Context, f document.File) error { } // FetchDocumentFile retrieves a document file by its block ID from the database. -func (a *agent) FetchDocumentFile(ctx context.Context, id, organizationID string) (*document.File, error) { +func (a *agent) FetchDocumentFile(ctx context.Context, id, organizationID string) (*file.File, error) { q, args := a.selectDocumentFile(a.builder.Select()). Where(sq.Eq{ "document_files.id": id, @@ -34,7 +48,7 @@ func (a *agent) FetchDocumentFile(ctx context.Context, id, organizationID string Limit(1). MustSql() - f := &document.File{} + f := &file.File{} if err := sqlx.GetContext(ctx, a.sql, f, q, args...); err != nil { return nil, err } @@ -42,13 +56,112 @@ func (a *agent) FetchDocumentFile(ctx context.Context, id, organizationID string return f, nil } +// FetchPaginatedDocumentFiles retrieves a list of document files from +// the database with pagination support. +func (a *agent) FetchPaginatedDocumentFiles(ctx context.Context, offsetID string, limit int64) ([]file.File, error) { + b := a.selectDocumentFile(a.builder.Select()). + OrderBy("document_files.id ASC"). + Limit(uint64(limit)) + + if offsetID != "" { + b = b.Where(sq.Gt{"document_files.id": offsetID}) + } + + q, args := b.MustSql() + + files := []file.File{} + if err := sqlx.SelectContext(ctx, a.sql, &files, q, args...); err != nil { + return nil, err + } + + return files, nil +} + +// UpdateDocumentFileUnreferencedAt sets or clears the timestamp marking +// when the file was first observed to be referenced by nothing. +func (a *agent) UpdateDocumentFileUnreferencedAt(ctx context.Context, id string, at null.Time) error { + q, args := a.builder.Update("document_files"). + SetMap(map[string]any{ + "unreferenced_at": at, + }). + Where(sq.Eq{"id": id}). + MustSql() + + _, err := a.sql.ExecContext(ctx, q, args...) + + return err +} + +// DeleteDocumentFile removes a document file row from the database. +// The row is not scoped to an organization: the sweep deletes rows whose +// organization is already gone. +func (a *agent) DeleteDocumentFile(ctx context.Context, id string) error { + q, args := a.builder.Delete("document_files"). + Where(sq.Eq{"id": id}). + MustSql() + + _, err := a.sql.ExecContext(ctx, q, args...) + + return err +} + +// CheckDocumentFileReferenced reports whether the file id still appears in +// the document's live branch content, in a retained changelog snapshot, or +// in a comment or comment reply. +// +// The id is matched as a substring of the serialized content rather than by +// walking the block tree, which keeps the whole check inside Postgres. The +// imprecision is deliberate and one-directional: a spurious match retains a +// file that could have been deleted, and can never delete a live one. +func (a *agent) CheckDocumentFileReferenced(ctx context.Context, id string, documentID xid.ID) (bool, error) { + q := ` + SELECT EXISTS ( + SELECT 1 FROM document_branches + WHERE fk_document_id = $1 AND position($2 in content::text) > 0 + UNION ALL + SELECT 1 FROM document_branch_changelogs + WHERE fk_document_id = $1 AND position($2 in content::text) > 0 + UNION ALL + SELECT 1 FROM document_comments + WHERE fk_document_id = $1 AND position($2 in content::text) > 0 + UNION ALL + SELECT 1 FROM document_comment_replies r + JOIN document_comments c ON c.id = r.fk_document_comment_id + WHERE c.fk_document_id = $1 AND position($2 in r.content::text) > 0 + ) + ` + + var referenced bool + if err := sqlx.GetContext(ctx, a.sql, &referenced, q, documentID, id); err != nil { + return false, err + } + + return referenced, nil +} + +// DeleteExpiredDocumentBranchChangelogs removes changelog snapshots created +// before the given time. Age trimming happens here rather than on insert +// because an idle branch never inserts again, and its snapshots would +// otherwise pin the files they reference forever. +func (a *agent) DeleteExpiredDocumentBranchChangelogs(ctx context.Context, before time.Time) error { + q, args := a.builder.Delete("document_branch_changelogs"). + Where(sq.Lt{"created_at": before}). + MustSql() + + _, err := a.sql.ExecContext(ctx, q, args...) + + return err +} + // selectDocumentFile prepares a sql select statement for fetching document files. func (a *agent) selectDocumentFile(b sq.SelectBuilder) sq.SelectBuilder { return b.Columns( `document_files.id AS "id"`, `document_files.location AS "location"`, + `document_files.storage_key AS "storage_key"`, `document_files.fk_document_id AS "fk_document_id"`, `document_files.fk_organization_id AS "fk_organization_id"`, `document_files.created_at AS "created_at"`, + `document_files.unreferenced_at AS "unreferenced_at"`, ).From("document_files") } diff --git a/server/core/internal/db/document_file_test.go b/server/core/internal/db/document_file_test.go index 5217a1d4..0a2d3991 100644 --- a/server/core/internal/db/document_file_test.go +++ b/server/core/internal/db/document_file_test.go @@ -3,10 +3,14 @@ package db import ( "context" "database/sql" + "strconv" "testing" "time" + "github.com/guregu/null/v5" "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/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" @@ -14,28 +18,29 @@ import ( "github.com/stretchr/testify/require" ) -func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *document.File)) []document.File { +func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *file.File)) []file.File { t.Helper() - res := make([]document.File, count) + res := make([]file.File, count) now := timeutil.Now().Truncate(time.Second) for i := range count { - f := document.File{ - ID: "file-" + xid.New().String(), - Location: document.LocationDocument, - CreatedAt: now.Add(-time.Duration(i) * time.Second), + f := file.File{ + ID: "file-" + xid.New().String(), + Location: file.LocationDocument, + StorageKey: "organizations/org/documents/doc/files/file", + CreatedAt: now.Add(-time.Duration(i) * time.Second), } if fn != nil { fn(i, &f) } - if f.DocumentID.IsZero() { + if !f.DocumentID.Valid { doc := prepDocuments(t, db, 1, nil)[0] - f.DocumentID = doc.ID - f.OrganizationID = doc.OrganizationID + f.DocumentID = null.ValueFrom(doc.ID) + f.OrganizationID = null.StringFrom(doc.OrganizationID) } res[i] = f @@ -44,9 +49,11 @@ func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *document.F SetMap(map[string]any{ "id": f.ID, "location": f.Location, + "storage_key": f.StorageKey, "fk_document_id": f.DocumentID, "fk_organization_id": f.OrganizationID, "created_at": f.CreatedAt, + "unreferenced_at": f.UnreferencedAt, }).MustSql() _, err := db.sql.Exec(q, args...) @@ -58,16 +65,26 @@ func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *document.F func Test_agent_InsertDocumentFile(t *testing.T) { type tcase struct { - File document.File + File file.File Err error } cc := map[string]func(*testing.T, *DB) tcase{ - "Duplicate file ID": func(t *testing.T, db *DB) tcase { - f := prepDocumentFiles(t, db, 1, nil)[0] + "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" + f.CreatedAt = timeutil.Now().Truncate(time.Second) + f.UnreferencedAt = null.Time{} + + return tcase{File: f} + }, + "Missing document": func(_ *testing.T, _ *DB) tcase { return tcase{ - File: f, + File: file.NewFile("file-1", file.LocationDocument, "key", xid.New(), "org-1"), Err: assert.AnError, } }, @@ -75,11 +92,12 @@ func Test_agent_InsertDocumentFile(t *testing.T) { doc := prepDocuments(t, db, 1, nil)[0] return tcase{ - File: document.File{ + File: file.File{ ID: "file-1", - Location: document.LocationComment, - DocumentID: doc.ID, - OrganizationID: doc.OrganizationID, + Location: file.LocationComment, + StorageKey: "organizations/org/documents/doc/files/file-1", + DocumentID: null.ValueFrom(doc.ID), + OrganizationID: null.StringFrom(doc.OrganizationID), CreatedAt: timeutil.Now().Truncate(time.Second), }, } @@ -100,7 +118,7 @@ func Test_agent_InsertDocumentFile(t *testing.T) { return } - res, err := db.FetchDocumentFile(context.Background(), c.File.ID, c.File.OrganizationID) + res, err := db.FetchDocumentFile(context.Background(), c.File.ID, c.File.OrganizationID.String) require.NoError(t, err) testutil.AssertFilterEqual(t, &c.File, res) }) @@ -118,7 +136,135 @@ func Test_agent_FetchDocumentFile(t *testing.T) { // success f := prepDocumentFiles(t, db, 1, nil)[0] - res, err = db.FetchDocumentFile(context.Background(), f.ID, f.OrganizationID) + res, err = db.FetchDocumentFile(context.Background(), f.ID, f.OrganizationID.String) assert.NoError(t, err) testutil.AssertFilterEqual(t, &f, res) } + +func Test_agent_FetchPaginatedDocumentFiles(t *testing.T) { + db := prepTempDB(t) + + // empty + res, err := db.FetchPaginatedDocumentFiles(context.Background(), "", 10) + require.NoError(t, err) + assert.Empty(t, res) + + ff := prepDocumentFiles(t, db, 3, func(i int, f *file.File) { + f.ID = "file-" + strconv.Itoa(i) + }) + + // first page + res, err = db.FetchPaginatedDocumentFiles(context.Background(), "", 2) + require.NoError(t, err) + require.Len(t, res, 2) + assert.Equal(t, ff[0].ID, res[0].ID) + assert.Equal(t, ff[1].ID, res[1].ID) + + // second page, offset excludes everything up to the given id + res, err = db.FetchPaginatedDocumentFiles(context.Background(), res[1].ID, 2) + require.NoError(t, err) + require.Len(t, res, 1) + assert.Equal(t, ff[2].ID, res[0].ID) +} + +func Test_agent_UpdateDocumentFileUnreferencedAt(t *testing.T) { + db := prepTempDB(t) + + f := prepDocumentFiles(t, db, 1, nil)[0] + at := timeutil.Now().Truncate(time.Second) + + // set + require.NoError(t, db.UpdateDocumentFileUnreferencedAt(context.Background(), f.ID, null.TimeFrom(at))) + + res, err := db.FetchDocumentFile(context.Background(), f.ID, f.OrganizationID.String) + require.NoError(t, err) + assert.Equal(t, at.UTC(), res.UnreferencedAt.Time.UTC()) + + // clear + require.NoError(t, db.UpdateDocumentFileUnreferencedAt(context.Background(), f.ID, null.Time{})) + + res, err = db.FetchDocumentFile(context.Background(), f.ID, f.OrganizationID.String) + require.NoError(t, err) + assert.False(t, res.UnreferencedAt.Valid) +} + +func Test_agent_DeleteDocumentFile(t *testing.T) { + db := prepTempDB(t) + + f := prepDocumentFiles(t, db, 1, nil)[0] + + require.NoError(t, db.DeleteDocumentFile(context.Background(), f.ID)) + + _, err := db.FetchDocumentFile(context.Background(), f.ID, f.OrganizationID.String) + testutil.AssertEqualError(t, sql.ErrNoRows, err) +} + +func Test_agent_CheckDocumentFileReferenced(t *testing.T) { + cc := map[string]func(*testing.T, *DB) (string, xid.ID, bool){ + "Referenced by branch content": func(t *testing.T, db *DB) (string, xid.ID, bool) { + doc := prepDocuments(t, db, 1, func(_ int, doc *document.Document) { + doc.Content.Content[0].Attrs = document.Attributes{"uid": "file-ref"} + })[0] + + return "file-ref", doc.ID, true + }, + "Referenced by a changelog snapshot": func(t *testing.T, db *DB) (string, xid.ID, bool) { + doc := prepDocuments(t, db, 1, nil)[0] + doc.Content.Content[0].Attrs = document.Attributes{"uid": "file-ref"} + + require.NoError(t, db.insertDocumentBranchChangelog( + context.Background(), + doc.ID, + doc.BranchID, + doc.Changelog(), + )) + + return "file-ref", doc.ID, true + }, + "Referenced by a comment": func(t *testing.T, db *DB) (string, xid.ID, bool) { + c := prepDocumentComments(t, db, 1, func(_ int, c *comment.Comment) { + c.Content = comment.Content{"uid": "file-ref"} + })[0] + + return "file-ref", c.DocumentID, true + }, + "Referenced by a comment reply": func(t *testing.T, db *DB) (string, xid.ID, bool) { + c := prepDocumentComments(t, db, 1, nil)[0] + + prepDocumentCommentReplies(t, db, 1, func(_ int, r *comment.Reply) { + r.CommentID = c.ID + r.OrganizationID = c.OrganizationID + r.Content = comment.Content{"uid": "file-ref"} + }) + + return "file-ref", c.DocumentID, true + }, + "Referenced by another document": func(t *testing.T, db *DB) (string, xid.ID, bool) { + prepDocuments(t, db, 1, func(_ int, doc *document.Document) { + doc.Content.Content[0].Attrs = document.Attributes{"uid": "file-ref"} + }) + + other := prepDocuments(t, db, 1, nil)[0] + + return "file-ref", other.ID, false + }, + "Not referenced": func(t *testing.T, db *DB) (string, xid.ID, bool) { + doc := prepDocuments(t, db, 1, nil)[0] + + return "file-ref", doc.ID, false + }, + } + + for cn, cfn := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + db := prepTempDB(t) + id, documentID, exp := cfn(t, db) + + res, err := db.CheckDocumentFileReferenced(context.Background(), id, documentID) + require.NoError(t, err) + assert.Equal(t, exp, res) + }) + } +} diff --git a/server/core/internal/db/document_hook_test.go b/server/core/internal/db/document_hook_test.go index 376c0780..024c0a44 100644 --- a/server/core/internal/db/document_hook_test.go +++ b/server/core/internal/db/document_hook_test.go @@ -47,12 +47,12 @@ func prepDocumentHooks(t *testing.T, db *DB, count int, fn func(int, *hook.Hook) fn(i, &hk) } - if hk.DocumentID.IsZero() { + if !hk.DocumentID.Valid { if branch == nil { branch = prepDocumentBranches(t, db, 1, nil)[0] } - hk.DocumentID = branch.ID + hk.DocumentID = null.ValueFrom(branch.ID) hk.OrganizationID = branch.OrganizationID hk.BranchID = null.ValueFrom(branch.BranchID) } @@ -63,7 +63,7 @@ func prepDocumentHooks(t *testing.T, db *DB, count int, fn func(int, *hook.Hook) SetMap(map[string]any{ "id": hk.ID, "type": hk.Type, - "fk_document_id": hk.DocumentID, + "fk_document_id": hk.DocumentID.V, "fk_organization_id": hk.OrganizationID, "fk_branch_id": hk.BranchID, "block_id": hk.BlockID, @@ -104,7 +104,7 @@ func Test_agent_InsertDocumentHook(t *testing.T) { Hook: hook.Hook{ ID: xid.New(), Type: hook.TypeScheduledReminder, - DocumentID: branch.ID, + DocumentID: null.ValueFrom(branch.ID), OrganizationID: branch.OrganizationID, BranchID: null.ValueFrom(branch.BranchID), Settings: processor.Settings(`{"schedule": "0 0 * * * *"}`), @@ -173,7 +173,7 @@ func Test_agent_FetchDocumentHooksByDocumentID(t *testing.T) { // success hooks := prepDocumentHooks(t, db, 2, nil) - res, err = db.FetchDocumentHooksByDocumentID(context.Background(), hooks[0].DocumentID, hooks[0].OrganizationID) + res, err = db.FetchDocumentHooksByDocumentID(context.Background(), hooks[0].DocumentID.V, hooks[0].OrganizationID) assert.NoError(t, err) testutil.AssertFilterEqual(t, hooks, res) } diff --git a/server/core/internal/db/document_test.go b/server/core/internal/db/document_test.go index 86baf494..3dcc78a5 100644 --- a/server/core/internal/db/document_test.go +++ b/server/core/internal/db/document_test.go @@ -11,6 +11,8 @@ import ( sq "github.com/Masterminds/squirrel" "github.com/guregu/null/v5" "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/hook" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" @@ -851,6 +853,8 @@ func Test_agent_DeleteDocument(t *testing.T) { type tcase struct { ID xid.ID OrganizationID string + FileID string + HookID xid.ID Err error } @@ -863,6 +867,30 @@ func Test_agent_DeleteDocument(t *testing.T) { OrganizationID: doc.OrganizationID, } }, + // files and hooks outlive their document on purpose: the row is + // the only record of the object or the external watcher, so the + // managers need it to reclaim them. + "External resources outlive the document": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + + f := prepDocumentFiles(t, db, 1, func(_ int, f *file.File) { + f.DocumentID = null.ValueFrom(doc.ID) + f.OrganizationID = null.StringFrom(doc.OrganizationID) + })[0] + + hk := prepDocumentHooks(t, db, 1, func(_ int, hk *hook.Hook) { + hk.DocumentID = null.ValueFrom(doc.ID) + hk.OrganizationID = doc.OrganizationID + hk.BranchID = null.ValueFrom(doc.BranchID) + })[0] + + return tcase{ + ID: doc.ID, + OrganizationID: doc.OrganizationID, + FileID: f.ID, + HookID: hk.ID, + } + }, } for cn, cfn := range cc { @@ -889,6 +917,31 @@ func Test_agent_DeleteDocument(t *testing.T) { err = db.sql.Get(&doc, q, args...) require.Equal(t, sql.ErrNoRows, err) + + if c.FileID == "" { + return + } + + var f file.File + + q, args = db.selectDocumentFile(db.builder.Select()). + Where(sq.Eq{"document_files.id": c.FileID}). + MustSql() + + require.NoError(t, db.sql.Get(&f, q, args...)) + assert.False(t, f.DocumentID.Valid) + assert.True(t, f.Orphaned()) + assert.NotEmpty(t, f.StorageKey, "the object stays reachable without its document") + + var hk hook.Hook + + q, args = db.selectDocumentHook(db.builder.Select()). + Where(sq.Eq{"document_hooks.id": c.HookID}). + MustSql() + + require.NoError(t, db.sql.Get(&hk, q, args...)) + assert.False(t, hk.DocumentID.Valid) + assert.False(t, hk.BranchID.Valid) }) } } diff --git a/server/core/internal/db/migrations/001_initial.sql b/server/core/internal/db/migrations/001_initial.sql index 24b962e6..21e2c04c 100644 --- a/server/core/internal/db/migrations/001_initial.sql +++ b/server/core/internal/db/migrations/001_initial.sql @@ -139,9 +139,9 @@ CREATE INDEX document_branch_changelogs_fk_branch_id_idx ON document_branch_chan CREATE TABLE document_hooks ( id TEXT PRIMARY KEY, type TEXT NOT NULL, - fk_document_id TEXT NOT NULL REFERENCES documents ON DELETE CASCADE, + fk_document_id TEXT REFERENCES documents ON DELETE SET NULL, fk_organization_id TEXT NOT NULL REFERENCES organizations ON DELETE CASCADE, - fk_branch_id TEXT NOT NULL REFERENCES document_branches(id) ON DELETE CASCADE, + fk_branch_id TEXT REFERENCES document_branches(id) ON DELETE SET NULL, block_id TEXT, settings JSONB NOT NULL, state TEXT NOT NULL, @@ -201,9 +201,11 @@ CREATE INDEX branch_reviewers_fk_organization_id_idx ON branch_reviewers (fk_org CREATE TABLE document_files ( id TEXT PRIMARY KEY, location TEXT NOT NULL, + storage_key TEXT NOT NULL, fk_document_id TEXT REFERENCES documents ON DELETE SET NULL, fk_organization_id TEXT REFERENCES organizations ON DELETE SET NULL, - created_at TIMESTAMP NOT NULL + created_at TIMESTAMP NOT NULL, + unreferenced_at TIMESTAMP ); CREATE INDEX document_files_fk_document_id_idx ON document_files (fk_document_id); CREATE INDEX document_files_fk_organization_id_idx ON document_files (fk_organization_id); diff --git a/server/core/internal/document/document.go b/server/core/internal/document/document.go index c47e0833..5e5f8391 100644 --- a/server/core/internal/document/document.go +++ b/server/core/internal/document/document.go @@ -369,7 +369,7 @@ func InitialDocumentContent(dataSourceID xid.ID) (RootBlock, error) { return RootBlock{}, fmt.Errorf("failed to unmarshal available metrics content: %w", err) } - return applyMetrics(drb, mrb, dataSourceID).Duplicate(), nil + return applyMetrics(drb, mrb, dataSourceID).RegenerateUIDs(), nil } // applyMetrics replaces every metricBlock in rb with a metricBlock from @@ -462,12 +462,17 @@ func NewDocumentContent() RootBlock { // Duplicate creates a copy of the document with a new ID, duplicated content // (with comment marks removed, nodeCommentId attributes removed, and uid // attributes regenerated), and cleared raw content. The new document name -// includes a timestamp suffix. -func (d Document) Duplicate(duplicatedBy string) Document { +// includes a timestamp suffix. The returned map pairs the source document's +// file ids with the ids the copy refers to them by, so the caller can copy +// the stored objects. +func (d Document) Duplicate(duplicatedBy string) (Document, map[string]string) { tstamp := timeutil.Now() + id := xid.New() + + content, files := d.Content.Duplicate(d.ID, id) return Document{ - ID: xid.New(), + ID: id, ParentID: d.ParentID, OrganizationID: d.OrganizationID, Branch: Branch{ @@ -475,7 +480,7 @@ func (d Document) Duplicate(duplicatedBy string) Document { BranchName: DefaultBranch, DocumentName: fmt.Sprintf("%s (%s)", d.DocumentName, tstamp.Format("2006 Jan. 02 15:04")), Icon: d.Icon, - Content: d.Content.Duplicate(), + Content: content, RawContent: nil, Protected: false, CreatedAt: tstamp, @@ -483,5 +488,5 @@ func (d Document) Duplicate(duplicatedBy string) Document { UpdatedAt: tstamp, LastUpdatedBy: null.StringFrom(duplicatedBy), }, - } + }, files } diff --git a/server/core/internal/document/document_test.go b/server/core/internal/document/document_test.go index 2d93269f..2c3de665 100644 --- a/server/core/internal/document/document_test.go +++ b/server/core/internal/document/document_test.go @@ -1,6 +1,7 @@ package document import ( + "fmt" "testing" "time" @@ -289,7 +290,9 @@ func Test_Document_Duplicate(t *testing.T) { doc := stubDocument() doc.Protected = true - dup := doc.Duplicate("user-2") + dup, files := doc.Duplicate("user-2") + + assert.Empty(t, files) assert.NotEqual(t, doc.ID, dup.ID) assert.NotEqual(t, doc.BranchID, dup.BranchID) @@ -310,6 +313,33 @@ func Test_Document_Duplicate(t *testing.T) { assert.NotEqual(t, origUID, dupUID) } +func Test_Document_Duplicate_files(t *testing.T) { + t.Parallel() + + doc := stubDocument() + doc.Content.Content[0] = Block{ + Type: BlockNodeImageBlock, + Attrs: Attributes{ + "uid": "img1", + "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, doc.ID, "img1"), + }, + } + + dup, files := doc.Duplicate("user-2") + + require.Len(t, files, 1) + + newID, ok := files["img1"] + require.True(t, ok) + + // the duplicate refers to its own copy under its own document. + assert.Equal( + t, + "https://app.test/core"+fmt.Sprintf(FilePathFormat, dup.ID, newID), + dup.Content.Content[0].Attrs["src"], + ) +} + func Test_NewDocumentContent(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/file.go b/server/core/internal/document/file.go deleted file mode 100644 index d6b4bb7f..00000000 --- a/server/core/internal/document/file.go +++ /dev/null @@ -1,58 +0,0 @@ -package document - -import ( - "time" - - "github.com/oxynote/oxynote/server/core/pkg/timeutil" - "github.com/rs/xid" -) - -// Location represents the location of the file. -type Location string - -const ( - // LocationDocument indicates that the file is stored in a document. - LocationDocument Location = "document" - - // LocationComment indicates that the file is stored in a comment. - LocationComment Location = "comment" -) - -// Valid checks if the location is valid. -func (l Location) Valid() bool { - switch l { - case LocationDocument, LocationComment: - return true - default: - return false - } -} - -// File represents a file attached to a document. -type File struct { - // ID is the identifier of this file. - ID string `json:"id" db:"id"` - - // Location indicates where the file is stored. - Location Location `json:"location" db:"location"` - - // DocumentID is the identifier for the document this file belongs to. - DocumentID xid.ID `json:"documentId" db:"fk_document_id"` - - // OrganizationID is the identifier for the organization this file belongs to. - OrganizationID string `json:"organizationId" db:"fk_organization_id"` - - // CreatedAt is the timestamp when the file was created. - CreatedAt time.Time `json:"createdAt" db:"created_at"` -} - -// NewFile creates a new file instance. -func NewFile(id string, loc Location, documentID xid.ID, organizationID string) File { - return File{ - ID: id, - Location: loc, - DocumentID: documentID, - OrganizationID: organizationID, - CreatedAt: timeutil.Now(), - } -} diff --git a/server/core/internal/document/file/file.go b/server/core/internal/document/file/file.go new file mode 100644 index 00000000..eb9f9b94 --- /dev/null +++ b/server/core/internal/document/file/file.go @@ -0,0 +1,96 @@ +// Package file defines files attached to documents and their storage records. +package file + +import ( + "fmt" + "path" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/pkg/timeutil" + "github.com/rs/xid" +) + +// _folderFormat is the storage folder format for a document's files. +const _folderFormat = "organizations/%s/documents/%s/files" + +// Folder returns the storage folder holding the given document's files. +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. +func Key(organizationID string, documentID xid.ID, id string) string { + return path.Join(Folder(organizationID, documentID), id) +} + +// Location represents the location of the file. +type Location string + +const ( + // LocationDocument indicates that the file is stored in a document. + LocationDocument Location = "document" + + // LocationComment indicates that the file is stored in a comment. + LocationComment Location = "comment" +) + +// Valid checks if the location is valid. +func (l Location) Valid() bool { + switch l { + case LocationDocument, LocationComment: + return true + default: + return false + } +} + +// File represents a file attached to a document. +type File struct { + // ID is the identifier of this file. + ID string `json:"id" db:"id"` + + // Location indicates where the file is stored. + Location Location `json:"location" db:"location"` + + // StorageKey is the full object key of the file in the object storage. + // It is stored rather than derived so that the object stays reachable + // after the owning document or organization is gone. + StorageKey string `json:"storageKey" db:"storage_key"` + + // DocumentID is the identifier for the document this file belongs to. + // Null when the document has been deleted; the manager will clean up + // such files. + DocumentID null.Value[xid.ID] `json:"documentId" db:"fk_document_id"` + + // OrganizationID is the identifier for the organization this file + // belongs to. Null when the organization has been deleted. + OrganizationID null.String `json:"organizationId" db:"fk_organization_id"` + + // CreatedAt is the timestamp when the file was created. + CreatedAt time.Time `json:"createdAt" db:"created_at"` + + // UnreferencedAt is the timestamp when the file was first observed + // to be referenced by no document content, changelog snapshot or + // comment. Null while the file is still referenced. + UnreferencedAt null.Time `json:"unreferencedAt" db:"unreferenced_at"` +} + +// NewFile creates a new file instance. +func NewFile(id string, loc Location, storageKey string, documentID xid.ID, organizationID string) File { + return File{ + ID: id, + Location: loc, + StorageKey: storageKey, + DocumentID: null.ValueFrom(documentID), + OrganizationID: null.StringFrom(organizationID), + CreatedAt: timeutil.Now(), + } +} + +// Orphaned reports whether the file lost its owner, which happens when the +// document or the organization is deleted by a path that cascades around +// the file itself. +func (f File) Orphaned() bool { + return !f.DocumentID.Valid || !f.OrganizationID.Valid +} diff --git a/server/core/internal/document/file/file_test.go b/server/core/internal/document/file/file_test.go new file mode 100644 index 00000000..90ba1970 --- /dev/null +++ b/server/core/internal/document/file/file_test.go @@ -0,0 +1,77 @@ +package file + +import ( + "testing" + + "github.com/guregu/null/v5" + "github.com/rs/xid" + "github.com/stretchr/testify/assert" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + +func Test_Location_Valid(t *testing.T) { + t.Parallel() + + assert.True(t, LocationDocument.Valid()) + assert.True(t, LocationComment.Valid()) + assert.False(t, Location("attachment").Valid()) + assert.False(t, Location("").Valid()) +} + +func Test_NewFile(t *testing.T) { + t.Parallel() + + documentID := xid.New() + + f := NewFile("file-1", LocationComment, "organizations/org-1/documents/doc-1/files/file-1", documentID, "org-1") + + assert.Equal(t, "file-1", f.ID) + assert.Equal(t, LocationComment, f.Location) + assert.Equal(t, "organizations/org-1/documents/doc-1/files/file-1", f.StorageKey) + assert.Equal(t, documentID, f.DocumentID.V) + assert.Equal(t, "org-1", f.OrganizationID.String) + assert.False(t, f.Orphaned()) + assert.False(t, f.CreatedAt.IsZero()) +} + +func Test_Folder(t *testing.T) { + t.Parallel() + + documentID := xid.New() + + assert.Equal( + t, + "organizations/org-1/documents/"+documentID.String()+"/files", + Folder("org-1", documentID), + ) +} + +func Test_Key(t *testing.T) { + t.Parallel() + + documentID := xid.New() + + assert.Equal( + t, + "organizations/org-1/documents/"+documentID.String()+"/files/file-1", + Key("org-1", documentID, "file-1"), + ) +} + +func Test_File_Orphaned(t *testing.T) { + t.Parallel() + + f := NewFile("file-1", LocationDocument, "key", xid.New(), "org-1") + assert.False(t, f.Orphaned()) + + f.DocumentID = null.Value[xid.ID]{} + assert.True(t, f.Orphaned()) + + f = NewFile("file-1", LocationDocument, "key", xid.New(), "org-1") + f.OrganizationID = null.String{} + assert.True(t, f.Orphaned()) +} diff --git a/server/core/internal/document/file/manager/0mock_db_test.go b/server/core/internal/document/file/manager/0mock_db_test.go new file mode 100644 index 00000000..5deb7737 --- /dev/null +++ b/server/core/internal/document/file/manager/0mock_db_test.go @@ -0,0 +1,321 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package manager + +import ( + "context" + "sync" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/rs/xid" +) + +// Ensure, that DBMock does implement DB. +// If this is not the case, regenerate this file with moq. +var _ DB = &DBMock{} + +// DBMock is a mock implementation of DB. +// +// func TestSomethingThatUsesDB(t *testing.T) { +// +// // make and configure a mocked DB +// mockedDB := &DBMock{ +// CheckDocumentFileReferencedFunc: func(ctx context.Context, id string, documentID xid.ID) (bool, error) { +// panic("mock out the CheckDocumentFileReferenced method") +// }, +// DeleteDocumentFileFunc: func(ctx context.Context, id string) error { +// panic("mock out the DeleteDocumentFile method") +// }, +// DeleteExpiredDocumentBranchChangelogsFunc: func(ctx context.Context, before time.Time) error { +// panic("mock out the DeleteExpiredDocumentBranchChangelogs method") +// }, +// FetchPaginatedDocumentFilesFunc: func(ctx context.Context, offsetID string, limit int64) ([]file.File, error) { +// panic("mock out the FetchPaginatedDocumentFiles method") +// }, +// UpdateDocumentFileUnreferencedAtFunc: func(ctx context.Context, id string, at null.Time) error { +// panic("mock out the UpdateDocumentFileUnreferencedAt method") +// }, +// } +// +// // use mockedDB in code that requires DB +// // and then make assertions. +// +// } +type DBMock struct { + // CheckDocumentFileReferencedFunc mocks the CheckDocumentFileReferenced method. + CheckDocumentFileReferencedFunc func(ctx context.Context, id string, documentID xid.ID) (bool, error) + + // DeleteDocumentFileFunc mocks the DeleteDocumentFile method. + DeleteDocumentFileFunc func(ctx context.Context, id string) error + + // DeleteExpiredDocumentBranchChangelogsFunc mocks the DeleteExpiredDocumentBranchChangelogs method. + DeleteExpiredDocumentBranchChangelogsFunc func(ctx context.Context, before time.Time) error + + // FetchPaginatedDocumentFilesFunc mocks the FetchPaginatedDocumentFiles method. + FetchPaginatedDocumentFilesFunc func(ctx context.Context, offsetID string, limit int64) ([]file.File, error) + + // UpdateDocumentFileUnreferencedAtFunc mocks the UpdateDocumentFileUnreferencedAt method. + UpdateDocumentFileUnreferencedAtFunc func(ctx context.Context, id string, at null.Time) error + + // calls tracks calls to the methods. + calls struct { + // CheckDocumentFileReferenced holds details about calls to the CheckDocumentFileReferenced method. + CheckDocumentFileReferenced []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + // DocumentID is the documentID argument value. + DocumentID xid.ID + } + // DeleteDocumentFile holds details about calls to the DeleteDocumentFile method. + DeleteDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + } + // DeleteExpiredDocumentBranchChangelogs holds details about calls to the DeleteExpiredDocumentBranchChangelogs method. + DeleteExpiredDocumentBranchChangelogs []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Before is the before argument value. + Before time.Time + } + // FetchPaginatedDocumentFiles holds details about calls to the FetchPaginatedDocumentFiles method. + FetchPaginatedDocumentFiles []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // OffsetID is the offsetID argument value. + OffsetID string + // Limit is the limit argument value. + Limit int64 + } + // UpdateDocumentFileUnreferencedAt holds details about calls to the UpdateDocumentFileUnreferencedAt method. + UpdateDocumentFileUnreferencedAt []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + // At is the at argument value. + At null.Time + } + } + lockCheckDocumentFileReferenced sync.RWMutex + lockDeleteDocumentFile sync.RWMutex + lockDeleteExpiredDocumentBranchChangelogs sync.RWMutex + lockFetchPaginatedDocumentFiles sync.RWMutex + lockUpdateDocumentFileUnreferencedAt sync.RWMutex +} + +// CheckDocumentFileReferenced calls CheckDocumentFileReferencedFunc. +func (mock *DBMock) CheckDocumentFileReferenced(ctx context.Context, id string, documentID xid.ID) (bool, error) { + callInfo := struct { + Ctx context.Context + ID string + DocumentID xid.ID + }{ + Ctx: ctx, + ID: id, + DocumentID: documentID, + } + mock.lockCheckDocumentFileReferenced.Lock() + mock.calls.CheckDocumentFileReferenced = append(mock.calls.CheckDocumentFileReferenced, callInfo) + mock.lockCheckDocumentFileReferenced.Unlock() + if mock.CheckDocumentFileReferencedFunc == nil { + var ( + bOut bool + errOut error + ) + return bOut, errOut + } + return mock.CheckDocumentFileReferencedFunc(ctx, id, documentID) +} + +// CheckDocumentFileReferencedCalls gets all the calls that were made to CheckDocumentFileReferenced. +// Check the length with: +// +// len(mockedDB.CheckDocumentFileReferencedCalls()) +func (mock *DBMock) CheckDocumentFileReferencedCalls() []struct { + Ctx context.Context + ID string + DocumentID xid.ID +} { + var calls []struct { + Ctx context.Context + ID string + DocumentID xid.ID + } + mock.lockCheckDocumentFileReferenced.RLock() + calls = mock.calls.CheckDocumentFileReferenced + mock.lockCheckDocumentFileReferenced.RUnlock() + return calls +} + +// DeleteDocumentFile calls DeleteDocumentFileFunc. +func (mock *DBMock) DeleteDocumentFile(ctx context.Context, id string) error { + callInfo := struct { + Ctx context.Context + ID string + }{ + Ctx: ctx, + ID: id, + } + mock.lockDeleteDocumentFile.Lock() + mock.calls.DeleteDocumentFile = append(mock.calls.DeleteDocumentFile, callInfo) + mock.lockDeleteDocumentFile.Unlock() + if mock.DeleteDocumentFileFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.DeleteDocumentFileFunc(ctx, id) +} + +// DeleteDocumentFileCalls gets all the calls that were made to DeleteDocumentFile. +// Check the length with: +// +// len(mockedDB.DeleteDocumentFileCalls()) +func (mock *DBMock) DeleteDocumentFileCalls() []struct { + Ctx context.Context + ID string +} { + var calls []struct { + Ctx context.Context + ID string + } + mock.lockDeleteDocumentFile.RLock() + calls = mock.calls.DeleteDocumentFile + mock.lockDeleteDocumentFile.RUnlock() + return calls +} + +// DeleteExpiredDocumentBranchChangelogs calls DeleteExpiredDocumentBranchChangelogsFunc. +func (mock *DBMock) DeleteExpiredDocumentBranchChangelogs(ctx context.Context, before time.Time) error { + callInfo := struct { + Ctx context.Context + Before time.Time + }{ + Ctx: ctx, + Before: before, + } + mock.lockDeleteExpiredDocumentBranchChangelogs.Lock() + mock.calls.DeleteExpiredDocumentBranchChangelogs = append(mock.calls.DeleteExpiredDocumentBranchChangelogs, callInfo) + mock.lockDeleteExpiredDocumentBranchChangelogs.Unlock() + if mock.DeleteExpiredDocumentBranchChangelogsFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.DeleteExpiredDocumentBranchChangelogsFunc(ctx, before) +} + +// DeleteExpiredDocumentBranchChangelogsCalls gets all the calls that were made to DeleteExpiredDocumentBranchChangelogs. +// Check the length with: +// +// len(mockedDB.DeleteExpiredDocumentBranchChangelogsCalls()) +func (mock *DBMock) DeleteExpiredDocumentBranchChangelogsCalls() []struct { + Ctx context.Context + Before time.Time +} { + var calls []struct { + Ctx context.Context + Before time.Time + } + mock.lockDeleteExpiredDocumentBranchChangelogs.RLock() + calls = mock.calls.DeleteExpiredDocumentBranchChangelogs + mock.lockDeleteExpiredDocumentBranchChangelogs.RUnlock() + return calls +} + +// FetchPaginatedDocumentFiles calls FetchPaginatedDocumentFilesFunc. +func (mock *DBMock) FetchPaginatedDocumentFiles(ctx context.Context, offsetID string, limit int64) ([]file.File, error) { + callInfo := struct { + Ctx context.Context + OffsetID string + Limit int64 + }{ + Ctx: ctx, + OffsetID: offsetID, + Limit: limit, + } + mock.lockFetchPaginatedDocumentFiles.Lock() + mock.calls.FetchPaginatedDocumentFiles = append(mock.calls.FetchPaginatedDocumentFiles, callInfo) + mock.lockFetchPaginatedDocumentFiles.Unlock() + if mock.FetchPaginatedDocumentFilesFunc == nil { + var ( + filesOut []file.File + errOut error + ) + return filesOut, errOut + } + return mock.FetchPaginatedDocumentFilesFunc(ctx, offsetID, limit) +} + +// FetchPaginatedDocumentFilesCalls gets all the calls that were made to FetchPaginatedDocumentFiles. +// Check the length with: +// +// len(mockedDB.FetchPaginatedDocumentFilesCalls()) +func (mock *DBMock) FetchPaginatedDocumentFilesCalls() []struct { + Ctx context.Context + OffsetID string + Limit int64 +} { + var calls []struct { + Ctx context.Context + OffsetID string + Limit int64 + } + mock.lockFetchPaginatedDocumentFiles.RLock() + calls = mock.calls.FetchPaginatedDocumentFiles + mock.lockFetchPaginatedDocumentFiles.RUnlock() + return calls +} + +// UpdateDocumentFileUnreferencedAt calls UpdateDocumentFileUnreferencedAtFunc. +func (mock *DBMock) UpdateDocumentFileUnreferencedAt(ctx context.Context, id string, at null.Time) error { + callInfo := struct { + Ctx context.Context + ID string + At null.Time + }{ + Ctx: ctx, + ID: id, + At: at, + } + mock.lockUpdateDocumentFileUnreferencedAt.Lock() + mock.calls.UpdateDocumentFileUnreferencedAt = append(mock.calls.UpdateDocumentFileUnreferencedAt, callInfo) + mock.lockUpdateDocumentFileUnreferencedAt.Unlock() + if mock.UpdateDocumentFileUnreferencedAtFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.UpdateDocumentFileUnreferencedAtFunc(ctx, id, at) +} + +// UpdateDocumentFileUnreferencedAtCalls gets all the calls that were made to UpdateDocumentFileUnreferencedAt. +// Check the length with: +// +// len(mockedDB.UpdateDocumentFileUnreferencedAtCalls()) +func (mock *DBMock) UpdateDocumentFileUnreferencedAtCalls() []struct { + Ctx context.Context + ID string + At null.Time +} { + var calls []struct { + Ctx context.Context + ID string + At null.Time + } + mock.lockUpdateDocumentFileUnreferencedAt.RLock() + calls = mock.calls.UpdateDocumentFileUnreferencedAt + mock.lockUpdateDocumentFileUnreferencedAt.RUnlock() + return calls +} diff --git a/server/core/internal/document/file/manager/0mock_storer_test.go b/server/core/internal/document/file/manager/0mock_storer_test.go new file mode 100644 index 00000000..ba50dcbc --- /dev/null +++ b/server/core/internal/document/file/manager/0mock_storer_test.go @@ -0,0 +1,90 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package manager + +import ( + "context" + "sync" +) + +// Ensure, that StorerMock does implement Storer. +// If this is not the case, regenerate this file with moq. +var _ Storer = &StorerMock{} + +// StorerMock is a mock implementation of Storer. +// +// func TestSomethingThatUsesStorer(t *testing.T) { +// +// // make and configure a mocked Storer +// mockedStorer := &StorerMock{ +// DeleteFunc: func(ctx context.Context, folder string, id string) error { +// panic("mock out the Delete method") +// }, +// } +// +// // use mockedStorer in code that requires Storer +// // and then make assertions. +// +// } +type StorerMock struct { + // DeleteFunc mocks the Delete method. + DeleteFunc func(ctx context.Context, folder string, id string) error + + // calls tracks calls to the methods. + calls struct { + // Delete holds details about calls to the Delete method. + Delete []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Folder is the folder argument value. + Folder string + // ID is the id argument value. + ID string + } + } + lockDelete sync.RWMutex +} + +// Delete calls DeleteFunc. +func (mock *StorerMock) Delete(ctx context.Context, folder string, id string) error { + callInfo := struct { + Ctx context.Context + Folder string + ID string + }{ + Ctx: ctx, + Folder: folder, + ID: id, + } + mock.lockDelete.Lock() + mock.calls.Delete = append(mock.calls.Delete, callInfo) + mock.lockDelete.Unlock() + if mock.DeleteFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.DeleteFunc(ctx, folder, id) +} + +// DeleteCalls gets all the calls that were made to Delete. +// Check the length with: +// +// len(mockedStorer.DeleteCalls()) +func (mock *StorerMock) DeleteCalls() []struct { + Ctx context.Context + Folder string + ID string +} { + var calls []struct { + Ctx context.Context + Folder string + ID string + } + mock.lockDelete.RLock() + calls = mock.calls.Delete + mock.lockDelete.RUnlock() + return calls +} diff --git a/server/core/internal/document/file/manager/manager.go b/server/core/internal/document/file/manager/manager.go new file mode 100644 index 00000000..b54868a9 --- /dev/null +++ b/server/core/internal/document/file/manager/manager.go @@ -0,0 +1,249 @@ +// Package manager periodically reclaims document files that nothing +// references any more. +package manager + +import ( + "context" + "fmt" + "log/slog" + "path" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/pkg/timeutil" + "github.com/rs/xid" +) + +const ( + // _processingBatch defines how many files to process in one batch. + _processingBatch = 100 + + // _processingInterval defines how often to reclaim files. + _processingInterval = time.Minute * 5 + + // _fileRetentionDuration defines how long a file is kept after it was + // first observed to be referenced by nothing. It covers the window + // between an upload and the first content persist that mentions it, + // during which a brand new file legitimately looks unreferenced. + _fileRetentionDuration = time.Hour * 24 +) + +// Manager reclaims the objects of files no document refers to. +type Manager struct { + log *slog.Logger + db DB + storer Storer + opts Options +} + +// Options holds all file manager options. +type Options struct { + // ChangelogRetention specifies how long document changelog snapshots + // are kept. Snapshots pin the files they reference, so trimming them + // is what releases a file removed from live content. + // Zero disables age-based trimming. + ChangelogRetention time.Duration +} + +// NewManager creates a new Manager with the given database interface. +func NewManager( + log *slog.Logger, + db DB, + storer Storer, + opts Options, +) *Manager { + return &Manager{ + log: log.With("component", "document-files-manager"), + db: db, + storer: storer, + opts: opts, + } +} + +// Start begins the periodic reclamation of document files. +func (m *Manager) Start(ctx context.Context) { + m.log.Info("starting") + defer m.log.Info("stopped") + + tm := time.NewTimer(0) + defer tm.Stop() + + for { + select { + case <-tm.C: + err := m.processFiles(ctx) + if err != nil { + m.log.With("error", err). + Error("processing document files") + } + + tm.Reset(_processingInterval) + case <-ctx.Done(): + return + } + } +} + +// processFiles trims expired changelog snapshots and then reclaims every +// file that no longer has anything referring to it. +// +// The snapshots go first: a file released by an expiring snapshot is then +// reclaimed in the same pass instead of waiting for the next one. +func (m *Manager) processFiles(ctx context.Context) error { + if err := m.trimChangelogs(ctx); err != nil { + return err + } + + var offsetID string + + for { + files, err := m.db.FetchPaginatedDocumentFiles(ctx, offsetID, _processingBatch) + if err != nil { + return fmt.Errorf("fetching paginated document files: %w", err) + } + + for _, f := range files { + offsetID = f.ID + + m.processFile(ctx, f) + } + + if len(files) < _processingBatch { + break + } + } + + return nil +} + +// processFile reclaims a single file, marks it as unreferenced, or leaves +// it alone. +func (m *Manager) processFile(ctx context.Context, f file.File) { + // the owner is gone, so no content can reference the file any more and + // there is no upload in flight to protect: the retention window would + // only delay the inevitable. + if f.Orphaned() { + m.deleteFile(ctx, f) + return + } + + if timeutil.Now().Sub(f.CreatedAt) < _fileRetentionDuration { + return + } + + referenced, err := m.db.CheckDocumentFileReferenced(ctx, f.ID, f.DocumentID.V) + if err != nil { + m.log.With("file_id", f.ID). + With("error", err). + Error("checking whether document file is referenced") + + return + } + + if referenced { + if f.UnreferencedAt.Valid { + m.markFile(ctx, f, null.Time{}) + } + + return + } + + if !f.UnreferencedAt.Valid { + m.markFile(ctx, f, null.TimeFrom(timeutil.Now())) + return + } + + if f.UnreferencedAt.Time.After(timeutil.Now().Add(-_fileRetentionDuration)) { + return + } + + m.deleteFile(ctx, f) +} + +// markFile records or clears the moment the file was observed to be +// referenced by nothing. +func (m *Manager) markFile(ctx context.Context, f file.File, at null.Time) { + err := m.db.UpdateDocumentFileUnreferencedAt(ctx, f.ID, at) + if err != nil { + m.log.With("file_id", f.ID). + With("error", err). + Error("updating document file unreferenced timestamp") + } +} + +// deleteFile removes the object and then the row that describes it. The +// object goes first so that a failure in between leaves a row pointing at +// an object that is already gone, which the next pass resolves; the other +// order would leave an object nothing knows about. +func (m *Manager) deleteFile(ctx context.Context, f file.File) { + folder, id := path.Split(f.StorageKey) + + err := m.storer.Delete(ctx, folder, id) + if err != nil { + m.log.With("file_id", f.ID). + With("error", err). + Error("deleting document file object") + + return + } + + err = m.db.DeleteDocumentFile(ctx, f.ID) + if err != nil { + m.log.With("file_id", f.ID). + With("error", err). + Error("deleting document file") + } +} + +// trimChangelogs removes changelog snapshots that outlived their retention. +// Snapshot count is trimmed on insert instead; age has to be handled here, +// because a branch that stopped being edited never inserts again and its +// snapshots would pin their files forever. +func (m *Manager) trimChangelogs(ctx context.Context) error { + if m.opts.ChangelogRetention == 0 { + return nil + } + + err := m.db.DeleteExpiredDocumentBranchChangelogs( + ctx, + timeutil.Now().Add(-m.opts.ChangelogRetention), + ) + if err != nil { + return fmt.Errorf("deleting expired document branch changelogs: %w", err) + } + + return nil +} + +// DB defines the database operations required by the Manager. +// +//go:generate ../../../../scripts/codegen/mock -t internal DB db +type DB interface { + // FetchPaginatedDocumentFiles should retrieve a batch of document files + // following the given offset. + FetchPaginatedDocumentFiles(ctx context.Context, offsetID string, limit int64) ([]file.File, error) + + // CheckDocumentFileReferenced should report whether the file is still + // referenced by the document's content, changelogs or comments. + CheckDocumentFileReferenced(ctx context.Context, id string, documentID xid.ID) (bool, error) + + // UpdateDocumentFileUnreferencedAt should set or clear the moment the + // file was observed to be referenced by nothing. + UpdateDocumentFileUnreferencedAt(ctx context.Context, id string, at null.Time) error + + // DeleteDocumentFile should remove the document file. + DeleteDocumentFile(ctx context.Context, id string) error + + // DeleteExpiredDocumentBranchChangelogs should remove changelog + // snapshots created before the given time. + DeleteExpiredDocumentBranchChangelogs(ctx context.Context, before time.Time) error +} + +// Storer defines the object storage operations required by the Manager. +// +//go:generate ../../../../scripts/codegen/mock -t internal Storer +type Storer interface { + // Delete should delete an object by its ID. + Delete(ctx context.Context, folder, id string) error +} diff --git a/server/core/internal/document/file/manager/manager_test.go b/server/core/internal/document/file/manager/manager_test.go new file mode 100644 index 00000000..0a10ecba --- /dev/null +++ b/server/core/internal/document/file/manager/manager_test.go @@ -0,0 +1,524 @@ +package manager + +import ( + "context" + "log/slog" + "testing" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/pkg/testutil" + "github.com/oxynote/oxynote/server/core/pkg/timeutil" + "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) +} + +// stubFile builds a file created long enough ago to be past the grace +// period, which is what the interesting cases need. +func stubFile() file.File { + f := file.NewFile("file-1", file.LocationDocument, "folder/file-1", xid.New(), "org-1") + f.CreatedAt = timeutil.Now().Add(-_fileRetentionDuration * 2) + + return f +} + +func Test_NewManager(t *testing.T) { + t.Parallel() + + db := &DBMock{} + storer := &StorerMock{} + opts := Options{ChangelogRetention: time.Hour} + + m := NewManager(slog.New(slog.DiscardHandler), db, storer, opts) + require.NotNil(t, m) + assert.NotNil(t, m.log) + assert.Same(t, db, m.db) + assert.Same(t, storer, m.storer) + assert.Equal(t, opts, m.opts) +} + +func Test_Manager_Start(t *testing.T) { + t.Parallel() + + fetched := make(chan struct{}) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + db := &DBMock{ + FetchPaginatedDocumentFilesFunc: func(context.Context, string, int64) ([]file.File, error) { + close(fetched) + cancel() + + return nil, nil + }, + } + + stopped := make(chan struct{}) + + go func() { + defer close(stopped) + + NewManager(slog.New(slog.DiscardHandler), db, &StorerMock{}, Options{}).Start(ctx) + }() + + <-fetched + <-stopped + + assert.Len(t, db.FetchPaginatedDocumentFilesCalls(), 1) +} + +func Test_Manager_processFiles(t *testing.T) { + type check func(*testing.T, *DBMock, *StorerMock, error) + + checks := func(cc ...check) []check { return cc } + + hasError := func(exp error) check { + return func(t *testing.T, _ *DBMock, _ *StorerMock, err error) { + testutil.AssertEqualError(t, exp, err) + } + } + + wasFetchCalled := func(count int) check { + return func(t *testing.T, db *DBMock, _ *StorerMock, _ error) { + require.Len(t, db.FetchPaginatedDocumentFilesCalls(), count) + } + } + + wasDeleteCalled := func(count int) check { + return func(t *testing.T, db *DBMock, _ *StorerMock, _ error) { + assert.Len(t, db.DeleteDocumentFileCalls(), count) + } + } + + wasTrimCalled := func(count int) check { + return func(t *testing.T, db *DBMock, _ *StorerMock, _ error) { + assert.Len(t, db.DeleteExpiredDocumentBranchChangelogsCalls(), count) + } + } + + // a full batch followed by a partial one, which is what makes the + // loop page a second time. + pagedFiles := func() []file.File { + ff := make([]file.File, _processingBatch) + + for i := range ff { + f := stubFile() + f.DocumentID = null.Value[xid.ID]{} + ff[i] = f + } + + return ff + } + + cc := map[string]struct { + DB *DBMock + Storer *StorerMock + Opts Options + Checks []check + }{ + "Error returned by db.DeleteExpiredDocumentBranchChangelogs": { + DB: &DBMock{ + DeleteExpiredDocumentBranchChangelogsFunc: func(context.Context, time.Time) error { + return assert.AnError + }, + }, + Storer: &StorerMock{}, + Opts: Options{ChangelogRetention: time.Hour}, + Checks: checks( + hasError(assert.AnError), + wasTrimCalled(1), + wasFetchCalled(0), + ), + }, + "Error returned by db.FetchPaginatedDocumentFiles": { + DB: &DBMock{ + FetchPaginatedDocumentFilesFunc: func(context.Context, string, int64) ([]file.File, error) { + return nil, assert.AnError + }, + }, + Storer: &StorerMock{}, + Checks: checks( + hasError(assert.AnError), + wasFetchCalled(1), + ), + }, + "Full batch pages once more": { + DB: func() *DBMock { + db := &DBMock{} + + calls := 0 + db.FetchPaginatedDocumentFilesFunc = func(context.Context, string, int64) ([]file.File, error) { + calls++ + + if calls == 1 { + return pagedFiles(), nil + } + + return nil, nil + } + + return db + }(), + Storer: &StorerMock{}, + Checks: checks( + hasError(nil), + wasFetchCalled(2), + wasDeleteCalled(_processingBatch), + ), + }, + "Nothing to process": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Checks: checks( + hasError(nil), + wasFetchCalled(1), + wasTrimCalled(0), + wasDeleteCalled(0), + ), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + m := NewManager(slog.New(slog.DiscardHandler), c.DB, c.Storer, c.Opts) + + err := m.processFiles(context.Background()) + + for _, ch := range c.Checks { + ch(t, c.DB, c.Storer, err) + } + }) + } +} + +func Test_Manager_processFile(t *testing.T) { + type check func(*testing.T, *DBMock, *StorerMock) + + checks := func(cc ...check) []check { return cc } + + wasCheckCalled := func(count int) check { + return func(t *testing.T, db *DBMock, _ *StorerMock) { + assert.Len(t, db.CheckDocumentFileReferencedCalls(), count) + } + } + + wasMarkCalled := func(at null.Time) check { + return func(t *testing.T, db *DBMock, _ *StorerMock) { + ff := db.UpdateDocumentFileUnreferencedAtCalls() + require.Len(t, ff, 1) + assert.Equal(t, "file-1", ff[0].ID) + assert.Equal(t, at.Valid, ff[0].At.Valid) + } + } + + wasNotMarked := func() check { + return func(t *testing.T, db *DBMock, _ *StorerMock) { + assert.Empty(t, db.UpdateDocumentFileUnreferencedAtCalls()) + } + } + + wasObjectDeleted := func(count int) check { + return func(t *testing.T, _ *DBMock, storer *StorerMock) { + ff := storer.DeleteCalls() + require.Len(t, ff, count) + + if count == 0 { + return + } + + assert.Equal(t, "folder/", ff[0].Folder) + assert.Equal(t, "file-1", ff[0].ID) + } + } + + wasRowDeleted := func(count int) check { + return func(t *testing.T, db *DBMock, _ *StorerMock) { + assert.Len(t, db.DeleteDocumentFileCalls(), count) + } + } + + referencedDB := func(referenced bool) *DBMock { + return &DBMock{ + CheckDocumentFileReferencedFunc: func(context.Context, string, xid.ID) (bool, error) { + return referenced, nil + }, + } + } + + cc := map[string]struct { + DB *DBMock + Storer *StorerMock + File func() file.File + Checks []check + }{ + "Orphaned file is reclaimed without waiting": { + DB: &DBMock{}, + Storer: &StorerMock{}, + File: func() file.File { + f := stubFile() + f.CreatedAt = timeutil.Now() + f.DocumentID = null.Value[xid.ID]{} + + return f + }, + Checks: checks( + wasCheckCalled(0), + wasObjectDeleted(1), + wasRowDeleted(1), + ), + }, + "File within the grace period is left alone": { + DB: &DBMock{}, + Storer: &StorerMock{}, + File: func() file.File { + f := stubFile() + f.CreatedAt = timeutil.Now() + + return f + }, + Checks: checks( + wasCheckCalled(0), + wasNotMarked(), + wasObjectDeleted(0), + wasRowDeleted(0), + ), + }, + "Error returned by db.CheckDocumentFileReferenced": { + DB: &DBMock{ + CheckDocumentFileReferencedFunc: func(context.Context, string, xid.ID) (bool, error) { + return false, assert.AnError + }, + }, + Storer: &StorerMock{}, + File: stubFile, + Checks: checks( + wasCheckCalled(1), + wasNotMarked(), + wasObjectDeleted(0), + ), + }, + "Referenced file keeps its clean state": { + DB: referencedDB(true), + Storer: &StorerMock{}, + File: stubFile, + Checks: checks( + wasCheckCalled(1), + wasNotMarked(), + wasObjectDeleted(0), + ), + }, + "Referenced file loses its unreferenced mark": { + DB: referencedDB(true), + Storer: &StorerMock{}, + File: func() file.File { + f := stubFile() + f.UnreferencedAt = null.TimeFrom(timeutil.Now()) + + return f + }, + Checks: checks( + wasMarkCalled(null.Time{}), + wasObjectDeleted(0), + ), + }, + "Unreferenced file is marked": { + DB: referencedDB(false), + Storer: &StorerMock{}, + File: stubFile, + Checks: checks( + wasMarkCalled(null.TimeFrom(timeutil.Now())), + wasObjectDeleted(0), + wasRowDeleted(0), + ), + }, + "Marked file within the retention window survives": { + DB: referencedDB(false), + Storer: &StorerMock{}, + File: func() file.File { + f := stubFile() + f.UnreferencedAt = null.TimeFrom(timeutil.Now()) + + return f + }, + Checks: checks( + wasNotMarked(), + wasObjectDeleted(0), + wasRowDeleted(0), + ), + }, + "Marked file past the retention window is reclaimed": { + DB: referencedDB(false), + Storer: &StorerMock{}, + File: func() file.File { + f := stubFile() + f.UnreferencedAt = null.TimeFrom(timeutil.Now().Add(-_fileRetentionDuration * 2)) + + return f + }, + Checks: checks( + wasNotMarked(), + wasObjectDeleted(1), + wasRowDeleted(1), + ), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + m := NewManager(slog.New(slog.DiscardHandler), c.DB, c.Storer, Options{}) + + m.processFile(context.Background(), c.File()) + + for _, ch := range c.Checks { + ch(t, c.DB, c.Storer) + } + }) + } +} + +func Test_Manager_markFile(t *testing.T) { + cc := map[string]struct { + DB *DBMock + At null.Time + }{ + "Error returned by db.UpdateDocumentFileUnreferencedAt": { + DB: &DBMock{ + UpdateDocumentFileUnreferencedAtFunc: func(context.Context, string, null.Time) error { + return assert.AnError + }, + }, + At: null.TimeFrom(timeutil.Now()), + }, + "Successful update": { + DB: &DBMock{}, + At: null.Time{}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + m := NewManager(slog.New(slog.DiscardHandler), c.DB, &StorerMock{}, Options{}) + + m.markFile(context.Background(), stubFile(), c.At) + + ff := c.DB.UpdateDocumentFileUnreferencedAtCalls() + require.Len(t, ff, 1) + assert.Equal(t, "file-1", ff[0].ID) + assert.Equal(t, c.At, ff[0].At) + }) + } +} + +func Test_Manager_deleteFile(t *testing.T) { + cc := map[string]struct { + DB *DBMock + Storer *StorerMock + RowCount int + }{ + "Error returned by storer.Delete keeps the row": { + DB: &DBMock{}, + Storer: &StorerMock{ + DeleteFunc: func(context.Context, string, string) error { + return assert.AnError + }, + }, + RowCount: 0, + }, + "Error returned by db.DeleteDocumentFile": { + DB: &DBMock{ + DeleteDocumentFileFunc: func(context.Context, string) error { + return assert.AnError + }, + }, + Storer: &StorerMock{}, + RowCount: 1, + }, + "Successful deletion": { + DB: &DBMock{}, + Storer: &StorerMock{}, + RowCount: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + m := NewManager(slog.New(slog.DiscardHandler), c.DB, c.Storer, Options{}) + + m.deleteFile(context.Background(), stubFile()) + + ff := c.Storer.DeleteCalls() + require.Len(t, ff, 1) + assert.Equal(t, "folder/", ff[0].Folder) + assert.Equal(t, "file-1", ff[0].ID) + + assert.Len(t, c.DB.DeleteDocumentFileCalls(), c.RowCount) + }) + } +} + +func Test_Manager_trimChangelogs(t *testing.T) { + cc := map[string]struct { + DB *DBMock + Opts Options + Calls int + Err error + }{ + "Zero retention disables trimming": { + DB: &DBMock{}, + Opts: Options{}, + Calls: 0, + }, + "Error returned by db.DeleteExpiredDocumentBranchChangelogs": { + DB: &DBMock{ + DeleteExpiredDocumentBranchChangelogsFunc: func(context.Context, time.Time) error { + return assert.AnError + }, + }, + Opts: Options{ChangelogRetention: time.Hour}, + Calls: 1, + Err: assert.AnError, + }, + "Successful trim": { + DB: &DBMock{}, + Opts: Options{ChangelogRetention: time.Hour}, + Calls: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + m := NewManager(slog.New(slog.DiscardHandler), c.DB, &StorerMock{}, c.Opts) + + err := m.trimChangelogs(context.Background()) + + testutil.AssertEqualError(t, c.Err, err) + + ff := c.DB.DeleteExpiredDocumentBranchChangelogsCalls() + require.Len(t, ff, c.Calls) + + if c.Calls == 0 { + return + } + + assert.WithinDuration(t, timeutil.Now().Add(-time.Hour), ff[0].Before, time.Minute) + }) + } +} diff --git a/server/core/internal/document/file_test.go b/server/core/internal/document/file_test.go deleted file mode 100644 index 6c5bb871..00000000 --- a/server/core/internal/document/file_test.go +++ /dev/null @@ -1,31 +0,0 @@ -package document - -import ( - "testing" - - "github.com/rs/xid" - "github.com/stretchr/testify/assert" -) - -func Test_Location_Valid(t *testing.T) { - t.Parallel() - - assert.True(t, LocationDocument.Valid()) - assert.True(t, LocationComment.Valid()) - assert.False(t, Location("attachment").Valid()) - assert.False(t, Location("").Valid()) -} - -func Test_NewFile(t *testing.T) { - t.Parallel() - - documentID := xid.New() - - f := NewFile("file-1", LocationComment, documentID, "org-1") - - assert.Equal(t, "file-1", f.ID) - assert.Equal(t, LocationComment, f.Location) - assert.Equal(t, documentID, f.DocumentID) - assert.Equal(t, "org-1", f.OrganizationID) - assert.False(t, f.CreatedAt.IsZero()) -} diff --git a/server/core/internal/document/hook/hook.go b/server/core/internal/document/hook/hook.go index df89630e..fbed9ac9 100644 --- a/server/core/internal/document/hook/hook.go +++ b/server/core/internal/document/hook/hook.go @@ -75,13 +75,16 @@ type Hook struct { Type Type `json:"type" db:"type"` // DocumentID is the ID of the document that this hook is associated with. - DocumentID xid.ID `json:"documentId" db:"fk_document_id"` + // Null when the document has been deleted; the manager will clean up + // such hooks. + DocumentID null.Value[xid.ID] `json:"documentId" db:"fk_document_id"` // OrganizationID is the ID of the organization that owns the hook. OrganizationID string `json:"organizationId" db:"fk_organization_id"` // BranchID is the ID of the document branch this hook is associated with. - // Null when the branch has been deleted; the manager will clean up such hooks. + // Null when the branch has been deleted; the manager will clean up such + // hooks, tearing down the external resource they hold first. BranchID null.Value[xid.ID] `json:"branchId" db:"fk_branch_id"` // BlockID is the ID of the block that this hook is associated with. @@ -123,7 +126,7 @@ func NewHook( h := Hook{ ID: xid.New(), Type: ci.Type, - DocumentID: documentID, + DocumentID: null.ValueFrom(documentID), OrganizationID: organizationID, BranchID: null.ValueFrom(branchID), BlockID: ci.BlockID, diff --git a/server/core/internal/document/hook/hook_test.go b/server/core/internal/document/hook/hook_test.go index d6d3433d..c6b91af2 100644 --- a/server/core/internal/document/hook/hook_test.go +++ b/server/core/internal/document/hook/hook_test.go @@ -70,7 +70,7 @@ func Test_NewHook(t *testing.T) { assert.False(t, h.ID.IsZero()) assert.Equal(t, TypeScheduledReminder, h.Type) - assert.Equal(t, documentID, h.DocumentID) + assert.Equal(t, null.ValueFrom(documentID), h.DocumentID) assert.Equal(t, null.ValueFrom(branchID), h.BranchID) assert.Equal(t, "org-1", h.OrganizationID) assert.Equal(t, null.StringFrom("block-1"), h.BlockID) diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index 88f04c9f..f7291d61 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -97,7 +97,7 @@ type ProcessingState struct { } // processHooks processes document hooks in a paginated manner. -func (m *Manager) processHooks(ctx context.Context) error { //nolint:gocognit // this method is complex, however, it's well-structured +func (m *Manager) processHooks(ctx context.Context) error { ps := &ProcessingState{ Documents: make(map[xid.ID]*document.Document), } @@ -111,13 +111,11 @@ func (m *Manager) processHooks(ctx context.Context) error { //nolint:gocognit // for _, h := range hooks { ps.OffsetID = h.ID - // Branch was deleted; clean up the orphaned hook immediately. - if !h.BranchID.Valid { - if err = m.db.DeleteDocumentHook(ctx, h.ID, h.OrganizationID); err != nil { - m.log.With("hook_id", h.ID). - With("error", err). - Error("deleting orphaned hook for deleted branch") - } + // The branch or the document was deleted, which is the only + // trace left of the hook: tear down the external resource it + // holds before the row goes away with the last reference to it. + if !h.BranchID.Valid || !h.DocumentID.Valid { + m.deleteHook(ctx, &h) continue } @@ -149,18 +147,19 @@ func (m *Manager) processHooks(ctx context.Context) error { //nolint:gocognit // With("error", err). Error("processing document hook") + // the soft-deletion mark set by ensureHook is persisted even + // here: it starts the retention clock, and a hook that keeps + // failing to process is exactly the one that would otherwise + // never reach it. + m.updateHook(ctx, h) + continue } - err = m.db.UpdateDocumentHook(ctx, h) - if err != nil { - m.log.With("hook_id", h.ID). - With("error", err). - Error("updating document hook") - } + m.updateHook(ctx, h) if previousScore.Equal(_fullScore) && h.Score.Equal(decimal.Zero) { - maintainers, err := m.db.FetchDocumentMaintainers(ctx, h.DocumentID, h.OrganizationID) + maintainers, err := m.db.FetchDocumentMaintainers(ctx, h.DocumentID.V, h.OrganizationID) if err != nil { m.log.With("hook_id", h.ID). With("error", err). @@ -172,7 +171,7 @@ func (m *Manager) processHooks(ctx context.Context) error { //nolint:gocognit // m.notifPub.PublishNotifications( h.OrganizationID, notification.NewDocumentHookTriggeredNotification( - h.DocumentID, + h.DocumentID.V, h.Type, h.BlockID, h.BranchID.V, @@ -221,25 +220,7 @@ func (m *Manager) ensureHook( } if doc == nil || (h.SoftDeletedAt.Valid && h.SoftDeletedAt.Time.Before(timeutil.Now().Add(-_hookRetentionDuration))) { - err := h.Delete(ctx, hook.NewInput( - h.OrganizationID, - m.githubMan, - m.webchangeClient, - )) - if err != nil { - m.log.With("hook_id", h.ID). - With("error", err). - Error("deleting hook for missing document") - - return false - } - - err = m.db.DeleteDocumentHook(ctx, h.ID, h.OrganizationID) - if err != nil { - m.log.With("hook_id", h.ID). - With("error", err). - Error("deleting hook for missing document from db") - } + m.deleteHook(ctx, h) return false } @@ -257,6 +238,42 @@ func (m *Manager) ensureHook( return true } +// deleteHook tears down the hook's external resource and then removes the +// row describing it. The external teardown goes first: the row is the only +// record of the resource, so dropping it first would strand the watcher +// with nothing left to find it by. +func (m *Manager) deleteHook(ctx context.Context, h *hook.Hook) { + err := h.Delete(ctx, hook.NewInput( + h.OrganizationID, + m.githubMan, + m.webchangeClient, + )) + if err != nil { + m.log.With("hook_id", h.ID). + With("error", err). + Error("deleting hook external resource") + + return + } + + err = m.db.DeleteDocumentHook(ctx, h.ID, h.OrganizationID) + if err != nil { + m.log.With("hook_id", h.ID). + With("error", err). + Error("deleting hook from db") + } +} + +// updateHook persists the hook's current state. +func (m *Manager) updateHook(ctx context.Context, h hook.Hook) { + err := m.db.UpdateDocumentHook(ctx, h) + if err != nil { + m.log.With("hook_id", h.ID). + With("error", err). + Error("updating document hook") + } +} + // DB defines the database operations required by the Manager. // //go:generate ../../../../scripts/codegen/mock -t internal DB db diff --git a/server/core/internal/document/hook/manager/manager_test.go b/server/core/internal/document/hook/manager/manager_test.go index b776f89c..2edb2f9c 100644 --- a/server/core/internal/document/hook/manager/manager_test.go +++ b/server/core/internal/document/hook/manager/manager_test.go @@ -48,7 +48,7 @@ func stubHook(t *testing.T, branchID xid.ID, schedule, startedAt time.Time) hook return hook.Hook{ ID: xid.New(), Type: hook.TypeScheduledReminder, - DocumentID: xid.New(), + DocumentID: null.ValueFrom(xid.New()), OrganizationID: "org-1", BranchID: null.ValueFrom(branchID), Settings: processor.Settings(settings), @@ -170,6 +170,41 @@ func Test_Manager_processHooks(t *testing.T) { wasUpdateCalled(0), ), }, + "Orphaned hook without a document is deleted": { + Hooks: func(t *testing.T) []hook.Hook { + h := stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + h.DocumentID = null.Value[xid.ID]{} + + return []hook.Hook{h} + }, + Checks: checks( + hasError(false), + wasDeleteCalled(1), + wasFetchDocumentCalled(0), + wasUpdateCalled(0), + ), + }, + "Processing failure still persists the soft deletion": { + Hooks: func(t *testing.T) []hook.Hook { + h := stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + h.BlockID = null.StringFrom("missing-block") + // unparsable settings make Process fail, which used to + // skip the update and leave the retention clock unstarted. + h.Settings = processor.Settings(`{"scale": "nonsense"}`) + + return []hook.Hook{h} + }, + Doc: stubDocument(), + Checks: checks( + hasError(false), + wasUpdateCalled(1), + func(t *testing.T, db *DBMock, _ *fakePublisher, _ error) { + ff := db.UpdateDocumentHookCalls() + require.NotEmpty(t, ff) + assert.True(t, ff[0].Hk.SoftDeletedAt.Valid) + }, + ), + }, "Github tracking hook is skipped when unconfigured": { Hooks: func(t *testing.T) []hook.Hook { h := stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) diff --git a/server/core/internal/document/node.go b/server/core/internal/document/node.go index 266dad32..8729ce58 100644 --- a/server/core/internal/document/node.go +++ b/server/core/internal/document/node.go @@ -4,7 +4,10 @@ import ( "database/sql/driver" "encoding/json" "errors" + "fmt" "maps" + "net/url" + "strings" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/pkg/strutil" @@ -14,9 +17,15 @@ import ( const ( _nodeCommentIDAttr = "nodeCommentId" _nodeUIDAttr = "uid" + _nodeSrcAttr = "src" _nodeCommentMarkType = "comment" ) +// FilePathFormat is the URL path format under which document files are +// served. Duplication matches stored src attributes against it, so the +// router mounts the file routes on this very format. +const FilePathFormat = "/api/documents/%s/files/%s" + // RootBlock represents a block in a document. type RootBlock struct { // Type is the type of the root block (always BlockNodeText's @@ -215,23 +224,53 @@ type Mark struct { // and nodeCommentId attributes removed. Unlike Duplicate, uid attributes are // preserved as-is. func (rb RootBlock) StripCommentMarks() RootBlock { - return rb.copyStripped(false) + return rb.copyStripped(nil) } // Duplicate creates a copy of the RootBlock with all comment marks // removed, nodeCommentId attributes removed, and uid attributes regenerated. -func (rb RootBlock) Duplicate() RootBlock { - return rb.copyStripped(true) +// Image blocks pointing at the source document's own files have their src +// rewritten to the duplicate's, and the returned map pairs every such file's +// old id with its new one so the caller can copy the objects themselves. +func (rb RootBlock) Duplicate(oldDocumentID, newDocumentID xid.ID) (RootBlock, map[string]string) { + dc := &duplication{ + oldDocumentID: oldDocumentID, + newDocumentID: newDocumentID, + files: make(map[string]string), + } + + return rb.copyStripped(dc), dc.files +} + +// RegenerateUIDs returns a copy of the RootBlock with comment marks removed +// and every uid attribute regenerated. Unlike Duplicate it rewrites no file +// references, which suits content templates: they carry none. +func (rb RootBlock) RegenerateUIDs() RootBlock { + return rb.copyStripped(&duplication{files: make(map[string]string)}) +} + +// duplication carries the state a block tree needs while it is being +// duplicated: the documents involved and the file ids collected on the way. +type duplication struct { + // oldDocumentID is the id of the document being duplicated. + oldDocumentID xid.ID + + // newDocumentID is the id of the document being created. + newDocumentID xid.ID + + // files maps the source document's file ids to the ids the duplicate + // refers to them by. + files map[string]string } // copyStripped returns a copy of the RootBlock with comment marks and -// nodeCommentId attributes removed, regenerating uid attributes when -// regenUIDs is set. -func (rb RootBlock) copyStripped(regenUIDs bool) RootBlock { +// nodeCommentId attributes removed, regenerating uid attributes and +// rewriting file references when a duplication is in progress. +func (rb RootBlock) copyStripped(dc *duplication) RootBlock { newContent := make([]Block, len(rb.Content)) for i, b := range rb.Content { - newContent[i] = b.copyStripped(regenUIDs) + newContent[i] = b.copyStripped(dc) } return RootBlock{ @@ -241,9 +280,9 @@ func (rb RootBlock) copyStripped(regenUIDs bool) RootBlock { } // copyStripped returns a copy of the Block with comment marks and -// nodeCommentId attributes removed, regenerating uid attributes when -// regenUIDs is set. -func (b Block) copyStripped(regenUIDs bool) Block { +// nodeCommentId attributes removed, regenerating uid attributes and +// rewriting file references when a duplication is in progress. +func (b Block) copyStripped(dc *duplication) Block { newBlock := Block{ Type: b.Type, Text: b.Text, @@ -253,7 +292,7 @@ func (b Block) copyStripped(regenUIDs bool) Block { newBlock.Content = make([]Block, len(b.Content)) for i, cb := range b.Content { - newBlock.Content[i] = cb.copyStripped(regenUIDs) + newBlock.Content[i] = cb.copyStripped(dc) } } @@ -279,7 +318,7 @@ func (b Block) copyStripped(regenUIDs bool) Block { continue } - if regenUIDs && k == _nodeUIDAttr { + if dc != nil && k == _nodeUIDAttr { newAttrs[k] = strutil.NanoID() continue } @@ -287,6 +326,10 @@ func (b Block) copyStripped(regenUIDs bool) Block { newAttrs[k] = v } + if dc != nil && b.Type == BlockNodeImageBlock { + dc.rewriteFileRef(b.Attrs, newAttrs) + } + if len(newAttrs) > 0 { newBlock.Attrs = newAttrs } @@ -294,3 +337,42 @@ func (b Block) copyStripped(regenUIDs bool) Block { return newBlock } + +// rewriteFileRef points a duplicated image block at the duplicate's own copy +// of the file and records the pair. Images served from anywhere but the +// source document's file route are left as they are: an externally hosted +// image has no object to copy. +func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { + oldID, ok := attrs[_nodeUIDAttr].(string) + if !ok { + return + } + + newID, ok := newAttrs[_nodeUIDAttr].(string) + if !ok { + return + } + + src, ok := attrs[_nodeSrcAttr].(string) + if !ok { + return + } + + u, err := url.Parse(src) + if err != nil { + return + } + + oldPath := fmt.Sprintf(FilePathFormat, d.oldDocumentID, oldID) + if !strings.HasSuffix(u.Path, oldPath) { + return + } + + // 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 = strings.TrimSuffix(u.Path, oldPath) + + fmt.Sprintf(FilePathFormat, d.newDocumentID, newID) + + newAttrs[_nodeSrcAttr] = 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 c89bca78..f24fb548 100644 --- a/server/core/internal/document/node_test.go +++ b/server/core/internal/document/node_test.go @@ -1,6 +1,7 @@ package document import ( + "fmt" "testing" "github.com/oxynote/oxynote/server/core/internal/search" @@ -280,7 +281,9 @@ func Test_RootBlock_Duplicate(t *testing.T) { t.Parallel() orig := stubMarkedTree() - dup := orig.Duplicate() + dup, files := orig.Duplicate(xid.New(), xid.New()) + + assert.Empty(t, files) require.Len(t, dup.Content, 1) @@ -301,6 +304,85 @@ func Test_RootBlock_Duplicate(t *testing.T) { assert.Equal(t, "left", orig.Content[0].Attrs["align"]) } +func Test_RootBlock_Duplicate_files(t *testing.T) { + t.Parallel() + + oldDocumentID := xid.New() + newDocumentID := xid.New() + + orig := RootBlock{ + Type: BlockNodeDoc, + Content: []Block{ + { + Type: BlockNodeImageBlock, + Attrs: Attributes{ + "uid": "img1", + "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, oldDocumentID, "img1"), + }, + }, + { + Type: BlockNodeImageBlock, + Attrs: Attributes{ + "uid": "img2", + "src": "https://cdn.test/photo.png", + }, + }, + { + Type: BlockNodeImageBlock, + Attrs: Attributes{ + "uid": "img3", + "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, xid.New(), "img3"), + }, + }, + { + Type: BlockNodeImageBlock, + Attrs: Attributes{"uid": "img4"}, + }, + }, + } + + dup, files := orig.Duplicate(oldDocumentID, newDocumentID) + + // only the image served by the source document is remapped. + require.Len(t, files, 1) + + newID, ok := files["img1"] + require.True(t, ok) + + uid, ok := dup.Content[0].UID() + require.True(t, ok) + assert.Equal(t, newID, uid) + assert.Equal( + t, + "https://app.test/core"+fmt.Sprintf(FilePathFormat, newDocumentID, newID), + dup.Content[0].Attrs["src"], + "the host and path prefix must survive the rewrite", + ) + + // an externally hosted image, an image of another document and an + // image without a src are all left as they are. + assert.Equal(t, "https://cdn.test/photo.png", dup.Content[1].Attrs["src"]) + assert.Equal(t, orig.Content[2].Attrs["src"], dup.Content[2].Attrs["src"]) + assert.NotContains(t, dup.Content[3].Attrs, "src") + + // the original is untouched. + assert.Equal(t, "img1", orig.Content[0].Attrs["uid"]) +} + +func Test_RootBlock_RegenerateUIDs(t *testing.T) { + t.Parallel() + + orig := stubMarkedTree() + res := orig.RegenerateUIDs() + + require.Len(t, res.Content, 1) + assert.NotContains(t, res.Content[0].Attrs, "nodeCommentId") + + uid, ok := res.Content[0].UID() + require.True(t, ok) + assert.NotEqual(t, "p1", uid) +} + func Test_RootBlock_Value_Scan(t *testing.T) { t.Parallel() diff --git a/server/core/internal/server/0mock_db_test.go b/server/core/internal/server/0mock_db_test.go index f9ae31b4..4ee72b61 100644 --- a/server/core/internal/server/0mock_db_test.go +++ b/server/core/internal/server/0mock_db_test.go @@ -10,8 +10,9 @@ import ( "github.com/guregu/null/v5" slackCore "github.com/oxynote/oxynote/server/core/internal/apps/slack" "github.com/oxynote/oxynote/server/core/internal/datasource" - "github.com/oxynote/oxynote/server/core/internal/document" + documentCore "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/hook" notificationCore "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/search" @@ -65,6 +66,9 @@ var _ DB = &DBMock{} // DeleteDocumentCommentsByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) error { // panic("mock out the DeleteDocumentCommentsByBranchID method") // }, +// DeleteDocumentFileFunc: func(ctx context.Context, id string) error { +// panic("mock out the DeleteDocumentFile method") +// }, // DeleteDocumentHookFunc: func(ctx context.Context, id xid.ID, organizationID string) error { // panic("mock out the DeleteDocumentHook method") // }, @@ -77,10 +81,10 @@ var _ DB = &DBMock{} // DeleteSlackUserLinkFunc: func(ctx context.Context, slackUserID string, teamID string) error { // panic("mock out the DeleteSlackUserLink method") // }, -// FetchBranchReviewerFunc: func(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*document.BranchReviewer, error) { +// FetchBranchReviewerFunc: func(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*documentCore.BranchReviewer, error) { // panic("mock out the FetchBranchReviewer method") // }, -// FetchBranchReviewersFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]document.BranchReviewer, error) { +// FetchBranchReviewersFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]documentCore.BranchReviewer, error) { // panic("mock out the FetchBranchReviewers method") // }, // FetchDataSourceFunc: func(ctx context.Context, id xid.ID, organizationID string) (*datasource.DataSource, error) { @@ -89,16 +93,16 @@ var _ DB = &DBMock{} // FetchDataSourcesFunc: func(ctx context.Context, organizationID string) ([]datasource.DataSource, error) { // panic("mock out the FetchDataSources method") // }, -// FetchDocumentFunc: func(ctx context.Context, id xid.ID, organizationID string, branchName string) (*document.Document, error) { +// FetchDocumentFunc: func(ctx context.Context, id xid.ID, organizationID string, branchName string) (*documentCore.Document, error) { // panic("mock out the FetchDocument method") // }, -// FetchDocumentBranchesFunc: func(ctx context.Context, docID xid.ID, organizationID string) ([]document.BranchSummary, error) { +// FetchDocumentBranchesFunc: func(ctx context.Context, docID xid.ID, organizationID string) ([]documentCore.BranchSummary, error) { // panic("mock out the FetchDocumentBranches method") // }, -// FetchDocumentBranchesUnsafeFunc: func(ctx context.Context, docID xid.ID) ([]document.BranchSummary, error) { +// FetchDocumentBranchesUnsafeFunc: func(ctx context.Context, docID xid.ID) ([]documentCore.BranchSummary, error) { // panic("mock out the FetchDocumentBranchesUnsafe method") // }, -// FetchDocumentByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) { +// FetchDocumentByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) { // panic("mock out the FetchDocumentByBranchID method") // }, // FetchDocumentCommentFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*comment.Comment, error) { @@ -110,7 +114,7 @@ var _ DB = &DBMock{} // FetchDocumentCommentsByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]comment.Comment, error) { // panic("mock out the FetchDocumentCommentsByBranchID method") // }, -// FetchDocumentFileFunc: func(ctx context.Context, blockID string, organizationID string) (*document.File, error) { +// FetchDocumentFileFunc: func(ctx context.Context, id string, organizationID string) (*file.File, error) { // panic("mock out the FetchDocumentFile method") // }, // FetchDocumentHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) { @@ -125,13 +129,13 @@ var _ DB = &DBMock{} // FetchDocumentMaintainersFunc: func(ctx context.Context, documentID xid.ID, organizationID string) ([]string, error) { // panic("mock out the FetchDocumentMaintainers method") // }, -// FetchDocumentTreeFunc: func(ctx context.Context, organizationID string) (document.Summaries, error) { +// FetchDocumentTreeFunc: func(ctx context.Context, organizationID string) (documentCore.Summaries, error) { // panic("mock out the FetchDocumentTree method") // }, -// FetchDocumentTreeByDocumentParentIDFunc: func(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (document.Summaries, error) { +// FetchDocumentTreeByDocumentParentIDFunc: func(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (documentCore.Summaries, error) { // panic("mock out the FetchDocumentTreeByDocumentParentID method") // }, -// FetchDocumentUnsafeByBranchIDFunc: func(ctx context.Context, branchID xid.ID) (*document.Document, error) { +// FetchDocumentUnsafeByBranchIDFunc: func(ctx context.Context, branchID xid.ID) (*documentCore.Document, error) { // panic("mock out the FetchDocumentUnsafeByBranchID method") // }, // FetchGithubInstallationByOrganizationIDFunc: func(ctx context.Context, organizationID string) (int64, error) { @@ -167,13 +171,13 @@ var _ DB = &DBMock{} // ForkDocumentBranchFunc: func(ctx context.Context, docID xid.ID, orgID string, sourceBranch string, targetBranch string, createdBy string) error { // panic("mock out the ForkDocumentBranch method") // }, -// InsertBranchReviewerFunc: func(ctx context.Context, reviewer document.BranchReviewer) error { +// InsertBranchReviewerFunc: func(ctx context.Context, reviewer documentCore.BranchReviewer) error { // panic("mock out the InsertBranchReviewer method") // }, // InsertDataSourceFunc: func(ctx context.Context, ds *datasource.DataSource) error { // panic("mock out the InsertDataSource method") // }, -// InsertDocumentFunc: func(ctx context.Context, doc document.Document) error { +// InsertDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the InsertDocument method") // }, // InsertDocumentCommentFunc: func(ctx context.Context, c comment.Comment) error { @@ -182,7 +186,7 @@ var _ DB = &DBMock{} // InsertDocumentCommentReplyFunc: func(ctx context.Context, r comment.Reply) error { // panic("mock out the InsertDocumentCommentReply method") // }, -// InsertDocumentFileFunc: func(ctx context.Context, f document.File) error { +// InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { // panic("mock out the InsertDocumentFile method") // }, // InsertDocumentHookFunc: func(ctx context.Context, hk hook.Hook) error { @@ -221,16 +225,16 @@ var _ DB = &DBMock{} // UnassignSlackAppOrganizationFunc: func(ctx context.Context, organizationID string) error { // panic("mock out the UnassignSlackAppOrganization method") // }, -// UpdateBranchReviewerFunc: func(ctx context.Context, reviewer document.BranchReviewer) error { +// UpdateBranchReviewerFunc: func(ctx context.Context, reviewer documentCore.BranchReviewer) error { // panic("mock out the UpdateBranchReviewer method") // }, // UpdateDataSourceFunc: func(ctx context.Context, ds *datasource.DataSource) error { // panic("mock out the UpdateDataSource method") // }, -// UpdateDocumentFunc: func(ctx context.Context, doc document.Document) error { +// UpdateDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocument method") // }, -// UpdateDocumentBranchMetadataFunc: func(ctx context.Context, doc document.Document) error { +// UpdateDocumentBranchMetadataFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocumentBranchMetadata method") // }, // UpdateDocumentCommentFunc: func(ctx context.Context, c comment.Comment) error { @@ -245,7 +249,7 @@ var _ DB = &DBMock{} // UpdateDocumentParentIDFunc: func(ctx context.Context, id xid.ID, parentID null.Value[xid.ID], organizationID string) error { // panic("mock out the UpdateDocumentParentID method") // }, -// UpdateDocumentTreeFunc: func(ctx context.Context, ss document.Summaries, organizationID string) error { +// UpdateDocumentTreeFunc: func(ctx context.Context, ss documentCore.Summaries, organizationID string) error { // panic("mock out the UpdateDocumentTree method") // }, // UpdateGithubInstallationOrganizationIDFunc: func(ctx context.Context, installationID int64, organizationID string) error { @@ -309,6 +313,9 @@ type DBMock struct { // DeleteDocumentCommentsByBranchIDFunc mocks the DeleteDocumentCommentsByBranchID method. DeleteDocumentCommentsByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) error + // DeleteDocumentFileFunc mocks the DeleteDocumentFile method. + DeleteDocumentFileFunc func(ctx context.Context, id string) error + // DeleteDocumentHookFunc mocks the DeleteDocumentHook method. DeleteDocumentHookFunc func(ctx context.Context, id xid.ID, organizationID string) error @@ -322,10 +329,10 @@ type DBMock struct { DeleteSlackUserLinkFunc func(ctx context.Context, slackUserID string, teamID string) error // FetchBranchReviewerFunc mocks the FetchBranchReviewer method. - FetchBranchReviewerFunc func(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*document.BranchReviewer, error) + FetchBranchReviewerFunc func(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*documentCore.BranchReviewer, error) // FetchBranchReviewersFunc mocks the FetchBranchReviewers method. - FetchBranchReviewersFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]document.BranchReviewer, error) + FetchBranchReviewersFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]documentCore.BranchReviewer, error) // FetchDataSourceFunc mocks the FetchDataSource method. FetchDataSourceFunc func(ctx context.Context, id xid.ID, organizationID string) (*datasource.DataSource, error) @@ -334,16 +341,16 @@ type DBMock struct { FetchDataSourcesFunc func(ctx context.Context, organizationID string) ([]datasource.DataSource, error) // FetchDocumentFunc mocks the FetchDocument method. - FetchDocumentFunc func(ctx context.Context, id xid.ID, organizationID string, branchName string) (*document.Document, error) + FetchDocumentFunc func(ctx context.Context, id xid.ID, organizationID string, branchName string) (*documentCore.Document, error) // FetchDocumentBranchesFunc mocks the FetchDocumentBranches method. - FetchDocumentBranchesFunc func(ctx context.Context, docID xid.ID, organizationID string) ([]document.BranchSummary, error) + FetchDocumentBranchesFunc func(ctx context.Context, docID xid.ID, organizationID string) ([]documentCore.BranchSummary, error) // FetchDocumentBranchesUnsafeFunc mocks the FetchDocumentBranchesUnsafe method. - FetchDocumentBranchesUnsafeFunc func(ctx context.Context, docID xid.ID) ([]document.BranchSummary, error) + FetchDocumentBranchesUnsafeFunc func(ctx context.Context, docID xid.ID) ([]documentCore.BranchSummary, error) // FetchDocumentByBranchIDFunc mocks the FetchDocumentByBranchID method. - FetchDocumentByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) + FetchDocumentByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) // FetchDocumentCommentFunc mocks the FetchDocumentComment method. FetchDocumentCommentFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*comment.Comment, error) @@ -355,7 +362,7 @@ type DBMock struct { FetchDocumentCommentsByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]comment.Comment, error) // FetchDocumentFileFunc mocks the FetchDocumentFile method. - FetchDocumentFileFunc func(ctx context.Context, blockID string, organizationID string) (*document.File, error) + FetchDocumentFileFunc func(ctx context.Context, id string, organizationID string) (*file.File, error) // FetchDocumentHookFunc mocks the FetchDocumentHook method. FetchDocumentHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) @@ -370,13 +377,13 @@ type DBMock struct { FetchDocumentMaintainersFunc func(ctx context.Context, documentID xid.ID, organizationID string) ([]string, error) // FetchDocumentTreeFunc mocks the FetchDocumentTree method. - FetchDocumentTreeFunc func(ctx context.Context, organizationID string) (document.Summaries, error) + FetchDocumentTreeFunc func(ctx context.Context, organizationID string) (documentCore.Summaries, error) // FetchDocumentTreeByDocumentParentIDFunc mocks the FetchDocumentTreeByDocumentParentID method. - FetchDocumentTreeByDocumentParentIDFunc func(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (document.Summaries, error) + FetchDocumentTreeByDocumentParentIDFunc func(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (documentCore.Summaries, error) // FetchDocumentUnsafeByBranchIDFunc mocks the FetchDocumentUnsafeByBranchID method. - FetchDocumentUnsafeByBranchIDFunc func(ctx context.Context, branchID xid.ID) (*document.Document, error) + FetchDocumentUnsafeByBranchIDFunc func(ctx context.Context, branchID xid.ID) (*documentCore.Document, error) // FetchGithubInstallationByOrganizationIDFunc mocks the FetchGithubInstallationByOrganizationID method. FetchGithubInstallationByOrganizationIDFunc func(ctx context.Context, organizationID string) (int64, error) @@ -412,13 +419,13 @@ type DBMock struct { ForkDocumentBranchFunc func(ctx context.Context, docID xid.ID, orgID string, sourceBranch string, targetBranch string, createdBy string) error // InsertBranchReviewerFunc mocks the InsertBranchReviewer method. - InsertBranchReviewerFunc func(ctx context.Context, reviewer document.BranchReviewer) error + InsertBranchReviewerFunc func(ctx context.Context, reviewer documentCore.BranchReviewer) error // InsertDataSourceFunc mocks the InsertDataSource method. InsertDataSourceFunc func(ctx context.Context, ds *datasource.DataSource) error // InsertDocumentFunc mocks the InsertDocument method. - InsertDocumentFunc func(ctx context.Context, doc document.Document) error + InsertDocumentFunc func(ctx context.Context, doc documentCore.Document) error // InsertDocumentCommentFunc mocks the InsertDocumentComment method. InsertDocumentCommentFunc func(ctx context.Context, c comment.Comment) error @@ -427,7 +434,7 @@ type DBMock struct { InsertDocumentCommentReplyFunc func(ctx context.Context, r comment.Reply) error // InsertDocumentFileFunc mocks the InsertDocumentFile method. - InsertDocumentFileFunc func(ctx context.Context, f document.File) error + InsertDocumentFileFunc func(ctx context.Context, f file.File) error // InsertDocumentHookFunc mocks the InsertDocumentHook method. InsertDocumentHookFunc func(ctx context.Context, hk hook.Hook) error @@ -466,16 +473,16 @@ type DBMock struct { UnassignSlackAppOrganizationFunc func(ctx context.Context, organizationID string) error // UpdateBranchReviewerFunc mocks the UpdateBranchReviewer method. - UpdateBranchReviewerFunc func(ctx context.Context, reviewer document.BranchReviewer) error + UpdateBranchReviewerFunc func(ctx context.Context, reviewer documentCore.BranchReviewer) error // UpdateDataSourceFunc mocks the UpdateDataSource method. UpdateDataSourceFunc func(ctx context.Context, ds *datasource.DataSource) error // UpdateDocumentFunc mocks the UpdateDocument method. - UpdateDocumentFunc func(ctx context.Context, doc document.Document) error + UpdateDocumentFunc func(ctx context.Context, doc documentCore.Document) error // UpdateDocumentBranchMetadataFunc mocks the UpdateDocumentBranchMetadata method. - UpdateDocumentBranchMetadataFunc func(ctx context.Context, doc document.Document) error + UpdateDocumentBranchMetadataFunc func(ctx context.Context, doc documentCore.Document) error // UpdateDocumentCommentFunc mocks the UpdateDocumentComment method. UpdateDocumentCommentFunc func(ctx context.Context, c comment.Comment) error @@ -490,7 +497,7 @@ type DBMock struct { UpdateDocumentParentIDFunc func(ctx context.Context, id xid.ID, parentID null.Value[xid.ID], organizationID string) error // UpdateDocumentTreeFunc mocks the UpdateDocumentTree method. - UpdateDocumentTreeFunc func(ctx context.Context, ss document.Summaries, organizationID string) error + UpdateDocumentTreeFunc func(ctx context.Context, ss documentCore.Summaries, organizationID string) error // UpdateGithubInstallationOrganizationIDFunc mocks the UpdateGithubInstallationOrganizationID method. UpdateGithubInstallationOrganizationIDFunc func(ctx context.Context, installationID int64, organizationID string) error @@ -626,6 +633,13 @@ type DBMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } + // DeleteDocumentFile holds details about calls to the DeleteDocumentFile method. + DeleteDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + } // DeleteDocumentHook holds details about calls to the DeleteDocumentHook method. DeleteDocumentHook []struct { // Ctx is the ctx argument value. @@ -765,8 +779,8 @@ type DBMock struct { FetchDocumentFile []struct { // Ctx is the ctx argument value. Ctx context.Context - // BlockID is the blockID argument value. - BlockID string + // ID is the id argument value. + ID string // OrganizationID is the organizationID argument value. OrganizationID string } @@ -931,7 +945,7 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // Reviewer is the reviewer argument value. - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } // InsertDataSource holds details about calls to the InsertDataSource method. InsertDataSource []struct { @@ -945,7 +959,7 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // Doc is the doc argument value. - Doc document.Document + Doc documentCore.Document } // InsertDocumentComment holds details about calls to the InsertDocumentComment method. InsertDocumentComment []struct { @@ -966,7 +980,7 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // F is the f argument value. - F document.File + F file.File } // InsertDocumentHook holds details about calls to the InsertDocumentHook method. InsertDocumentHook []struct { @@ -1067,7 +1081,7 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // Reviewer is the reviewer argument value. - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } // UpdateDataSource holds details about calls to the UpdateDataSource method. UpdateDataSource []struct { @@ -1081,14 +1095,14 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // Doc is the doc argument value. - Doc document.Document + Doc documentCore.Document } // UpdateDocumentBranchMetadata holds details about calls to the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadata []struct { // Ctx is the ctx argument value. Ctx context.Context // Doc is the doc argument value. - Doc document.Document + Doc documentCore.Document } // UpdateDocumentComment holds details about calls to the UpdateDocumentComment method. UpdateDocumentComment []struct { @@ -1127,7 +1141,7 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // Ss is the ss argument value. - Ss document.Summaries + Ss documentCore.Summaries // OrganizationID is the organizationID argument value. OrganizationID string } @@ -1198,6 +1212,7 @@ type DBMock struct { lockDeleteDocumentComment sync.RWMutex lockDeleteDocumentCommentReply sync.RWMutex lockDeleteDocumentCommentsByBranchID sync.RWMutex + lockDeleteDocumentFile sync.RWMutex lockDeleteDocumentHook sync.RWMutex lockDeleteGithubInstallation sync.RWMutex lockDeleteSlackApp sync.RWMutex @@ -1798,6 +1813,45 @@ func (mock *DBMock) DeleteDocumentCommentsByBranchIDCalls() []struct { return calls } +// DeleteDocumentFile calls DeleteDocumentFileFunc. +func (mock *DBMock) DeleteDocumentFile(ctx context.Context, id string) error { + callInfo := struct { + Ctx context.Context + ID string + }{ + Ctx: ctx, + ID: id, + } + mock.lockDeleteDocumentFile.Lock() + mock.calls.DeleteDocumentFile = append(mock.calls.DeleteDocumentFile, callInfo) + mock.lockDeleteDocumentFile.Unlock() + if mock.DeleteDocumentFileFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.DeleteDocumentFileFunc(ctx, id) +} + +// DeleteDocumentFileCalls gets all the calls that were made to DeleteDocumentFile. +// Check the length with: +// +// len(mockedDB.DeleteDocumentFileCalls()) +func (mock *DBMock) DeleteDocumentFileCalls() []struct { + Ctx context.Context + ID string +} { + var calls []struct { + Ctx context.Context + ID string + } + mock.lockDeleteDocumentFile.RLock() + calls = mock.calls.DeleteDocumentFile + mock.lockDeleteDocumentFile.RUnlock() + return calls +} + // DeleteDocumentHook calls DeleteDocumentHookFunc. func (mock *DBMock) DeleteDocumentHook(ctx context.Context, id xid.ID, organizationID string) error { callInfo := struct { @@ -1963,7 +2017,7 @@ func (mock *DBMock) DeleteSlackUserLinkCalls() []struct { } // FetchBranchReviewer calls FetchBranchReviewerFunc. -func (mock *DBMock) FetchBranchReviewer(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*document.BranchReviewer, error) { +func (mock *DBMock) FetchBranchReviewer(ctx context.Context, branchID xid.ID, userID string, organizationID string) (*documentCore.BranchReviewer, error) { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -1980,7 +2034,7 @@ func (mock *DBMock) FetchBranchReviewer(ctx context.Context, branchID xid.ID, us mock.lockFetchBranchReviewer.Unlock() if mock.FetchBranchReviewerFunc == nil { var ( - branchReviewerOut *document.BranchReviewer + branchReviewerOut *documentCore.BranchReviewer errOut error ) return branchReviewerOut, errOut @@ -2011,7 +2065,7 @@ func (mock *DBMock) FetchBranchReviewerCalls() []struct { } // FetchBranchReviewers calls FetchBranchReviewersFunc. -func (mock *DBMock) FetchBranchReviewers(ctx context.Context, branchID xid.ID, organizationID string) ([]document.BranchReviewer, error) { +func (mock *DBMock) FetchBranchReviewers(ctx context.Context, branchID xid.ID, organizationID string) ([]documentCore.BranchReviewer, error) { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -2026,7 +2080,7 @@ func (mock *DBMock) FetchBranchReviewers(ctx context.Context, branchID xid.ID, o mock.lockFetchBranchReviewers.Unlock() if mock.FetchBranchReviewersFunc == nil { var ( - branchReviewersOut []document.BranchReviewer + branchReviewersOut []documentCore.BranchReviewer errOut error ) return branchReviewersOut, errOut @@ -2139,7 +2193,7 @@ func (mock *DBMock) FetchDataSourcesCalls() []struct { } // FetchDocument calls FetchDocumentFunc. -func (mock *DBMock) FetchDocument(ctx context.Context, id xid.ID, organizationID string, branchName string) (*document.Document, error) { +func (mock *DBMock) FetchDocument(ctx context.Context, id xid.ID, organizationID string, branchName string) (*documentCore.Document, error) { callInfo := struct { Ctx context.Context ID xid.ID @@ -2156,7 +2210,7 @@ func (mock *DBMock) FetchDocument(ctx context.Context, id xid.ID, organizationID mock.lockFetchDocument.Unlock() if mock.FetchDocumentFunc == nil { var ( - documentOut *document.Document + documentOut *documentCore.Document errOut error ) return documentOut, errOut @@ -2187,7 +2241,7 @@ func (mock *DBMock) FetchDocumentCalls() []struct { } // FetchDocumentBranches calls FetchDocumentBranchesFunc. -func (mock *DBMock) FetchDocumentBranches(ctx context.Context, docID xid.ID, organizationID string) ([]document.BranchSummary, error) { +func (mock *DBMock) FetchDocumentBranches(ctx context.Context, docID xid.ID, organizationID string) ([]documentCore.BranchSummary, error) { callInfo := struct { Ctx context.Context DocID xid.ID @@ -2202,7 +2256,7 @@ func (mock *DBMock) FetchDocumentBranches(ctx context.Context, docID xid.ID, org mock.lockFetchDocumentBranches.Unlock() if mock.FetchDocumentBranchesFunc == nil { var ( - branchSummarysOut []document.BranchSummary + branchSummarysOut []documentCore.BranchSummary errOut error ) return branchSummarysOut, errOut @@ -2231,7 +2285,7 @@ func (mock *DBMock) FetchDocumentBranchesCalls() []struct { } // FetchDocumentBranchesUnsafe calls FetchDocumentBranchesUnsafeFunc. -func (mock *DBMock) FetchDocumentBranchesUnsafe(ctx context.Context, docID xid.ID) ([]document.BranchSummary, error) { +func (mock *DBMock) FetchDocumentBranchesUnsafe(ctx context.Context, docID xid.ID) ([]documentCore.BranchSummary, error) { callInfo := struct { Ctx context.Context DocID xid.ID @@ -2244,7 +2298,7 @@ func (mock *DBMock) FetchDocumentBranchesUnsafe(ctx context.Context, docID xid.I mock.lockFetchDocumentBranchesUnsafe.Unlock() if mock.FetchDocumentBranchesUnsafeFunc == nil { var ( - branchSummarysOut []document.BranchSummary + branchSummarysOut []documentCore.BranchSummary errOut error ) return branchSummarysOut, errOut @@ -2271,7 +2325,7 @@ func (mock *DBMock) FetchDocumentBranchesUnsafeCalls() []struct { } // FetchDocumentByBranchID calls FetchDocumentByBranchIDFunc. -func (mock *DBMock) FetchDocumentByBranchID(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) { +func (mock *DBMock) FetchDocumentByBranchID(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -2286,7 +2340,7 @@ func (mock *DBMock) FetchDocumentByBranchID(ctx context.Context, branchID xid.ID mock.lockFetchDocumentByBranchID.Unlock() if mock.FetchDocumentByBranchIDFunc == nil { var ( - documentOut *document.Document + documentOut *documentCore.Document errOut error ) return documentOut, errOut @@ -2455,14 +2509,14 @@ func (mock *DBMock) FetchDocumentCommentsByBranchIDCalls() []struct { } // FetchDocumentFile calls FetchDocumentFileFunc. -func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organizationID string) (*document.File, error) { +func (mock *DBMock) FetchDocumentFile(ctx context.Context, id string, organizationID string) (*file.File, error) { callInfo := struct { Ctx context.Context - BlockID string + ID string OrganizationID string }{ Ctx: ctx, - BlockID: blockID, + ID: id, OrganizationID: organizationID, } mock.lockFetchDocumentFile.Lock() @@ -2470,12 +2524,12 @@ func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organ mock.lockFetchDocumentFile.Unlock() if mock.FetchDocumentFileFunc == nil { var ( - fileOut *document.File + fileOut *file.File errOut error ) return fileOut, errOut } - return mock.FetchDocumentFileFunc(ctx, blockID, organizationID) + return mock.FetchDocumentFileFunc(ctx, id, organizationID) } // FetchDocumentFileCalls gets all the calls that were made to FetchDocumentFile. @@ -2484,12 +2538,12 @@ func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organ // len(mockedDB.FetchDocumentFileCalls()) func (mock *DBMock) FetchDocumentFileCalls() []struct { Ctx context.Context - BlockID string + ID string OrganizationID string } { var calls []struct { Ctx context.Context - BlockID string + ID string OrganizationID string } mock.lockFetchDocumentFile.RLock() @@ -2675,7 +2729,7 @@ func (mock *DBMock) FetchDocumentMaintainersCalls() []struct { } // FetchDocumentTree calls FetchDocumentTreeFunc. -func (mock *DBMock) FetchDocumentTree(ctx context.Context, organizationID string) (document.Summaries, error) { +func (mock *DBMock) FetchDocumentTree(ctx context.Context, organizationID string) (documentCore.Summaries, error) { callInfo := struct { Ctx context.Context OrganizationID string @@ -2688,7 +2742,7 @@ func (mock *DBMock) FetchDocumentTree(ctx context.Context, organizationID string mock.lockFetchDocumentTree.Unlock() if mock.FetchDocumentTreeFunc == nil { var ( - summariesOut document.Summaries + summariesOut documentCore.Summaries errOut error ) return summariesOut, errOut @@ -2715,7 +2769,7 @@ func (mock *DBMock) FetchDocumentTreeCalls() []struct { } // FetchDocumentTreeByDocumentParentID calls FetchDocumentTreeByDocumentParentIDFunc. -func (mock *DBMock) FetchDocumentTreeByDocumentParentID(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (document.Summaries, error) { +func (mock *DBMock) FetchDocumentTreeByDocumentParentID(ctx context.Context, parentID null.Value[xid.ID], organizationID string) (documentCore.Summaries, error) { callInfo := struct { Ctx context.Context ParentID null.Value[xid.ID] @@ -2730,7 +2784,7 @@ func (mock *DBMock) FetchDocumentTreeByDocumentParentID(ctx context.Context, par mock.lockFetchDocumentTreeByDocumentParentID.Unlock() if mock.FetchDocumentTreeByDocumentParentIDFunc == nil { var ( - summariesOut document.Summaries + summariesOut documentCore.Summaries errOut error ) return summariesOut, errOut @@ -2759,7 +2813,7 @@ func (mock *DBMock) FetchDocumentTreeByDocumentParentIDCalls() []struct { } // FetchDocumentUnsafeByBranchID calls FetchDocumentUnsafeByBranchIDFunc. -func (mock *DBMock) FetchDocumentUnsafeByBranchID(ctx context.Context, branchID xid.ID) (*document.Document, error) { +func (mock *DBMock) FetchDocumentUnsafeByBranchID(ctx context.Context, branchID xid.ID) (*documentCore.Document, error) { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -2772,7 +2826,7 @@ func (mock *DBMock) FetchDocumentUnsafeByBranchID(ctx context.Context, branchID mock.lockFetchDocumentUnsafeByBranchID.Unlock() if mock.FetchDocumentUnsafeByBranchIDFunc == nil { var ( - documentOut *document.Document + documentOut *documentCore.Document errOut error ) return documentOut, errOut @@ -3279,10 +3333,10 @@ func (mock *DBMock) ForkDocumentBranchCalls() []struct { } // InsertBranchReviewer calls InsertBranchReviewerFunc. -func (mock *DBMock) InsertBranchReviewer(ctx context.Context, reviewer document.BranchReviewer) error { +func (mock *DBMock) InsertBranchReviewer(ctx context.Context, reviewer documentCore.BranchReviewer) error { callInfo := struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer }{ Ctx: ctx, Reviewer: reviewer, @@ -3305,11 +3359,11 @@ func (mock *DBMock) InsertBranchReviewer(ctx context.Context, reviewer document. // len(mockedDB.InsertBranchReviewerCalls()) func (mock *DBMock) InsertBranchReviewerCalls() []struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } { var calls []struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } mock.lockInsertBranchReviewer.RLock() calls = mock.calls.InsertBranchReviewer @@ -3357,10 +3411,10 @@ func (mock *DBMock) InsertDataSourceCalls() []struct { } // InsertDocument calls InsertDocumentFunc. -func (mock *DBMock) InsertDocument(ctx context.Context, doc document.Document) error { +func (mock *DBMock) InsertDocument(ctx context.Context, doc documentCore.Document) error { callInfo := struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document }{ Ctx: ctx, Doc: doc, @@ -3383,11 +3437,11 @@ func (mock *DBMock) InsertDocument(ctx context.Context, doc document.Document) e // len(mockedDB.InsertDocumentCalls()) func (mock *DBMock) InsertDocumentCalls() []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } { var calls []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } mock.lockInsertDocument.RLock() calls = mock.calls.InsertDocument @@ -3474,10 +3528,10 @@ func (mock *DBMock) InsertDocumentCommentReplyCalls() []struct { } // InsertDocumentFile calls InsertDocumentFileFunc. -func (mock *DBMock) InsertDocumentFile(ctx context.Context, f document.File) error { +func (mock *DBMock) InsertDocumentFile(ctx context.Context, f file.File) error { callInfo := struct { Ctx context.Context - F document.File + F file.File }{ Ctx: ctx, F: f, @@ -3500,11 +3554,11 @@ func (mock *DBMock) InsertDocumentFile(ctx context.Context, f document.File) err // len(mockedDB.InsertDocumentFileCalls()) func (mock *DBMock) InsertDocumentFileCalls() []struct { Ctx context.Context - F document.File + F file.File } { var calls []struct { Ctx context.Context - F document.File + F file.File } mock.lockInsertDocumentFile.RLock() calls = mock.calls.InsertDocumentFile @@ -4001,10 +4055,10 @@ func (mock *DBMock) UnassignSlackAppOrganizationCalls() []struct { } // UpdateBranchReviewer calls UpdateBranchReviewerFunc. -func (mock *DBMock) UpdateBranchReviewer(ctx context.Context, reviewer document.BranchReviewer) error { +func (mock *DBMock) UpdateBranchReviewer(ctx context.Context, reviewer documentCore.BranchReviewer) error { callInfo := struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer }{ Ctx: ctx, Reviewer: reviewer, @@ -4027,11 +4081,11 @@ func (mock *DBMock) UpdateBranchReviewer(ctx context.Context, reviewer document. // len(mockedDB.UpdateBranchReviewerCalls()) func (mock *DBMock) UpdateBranchReviewerCalls() []struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } { var calls []struct { Ctx context.Context - Reviewer document.BranchReviewer + Reviewer documentCore.BranchReviewer } mock.lockUpdateBranchReviewer.RLock() calls = mock.calls.UpdateBranchReviewer @@ -4079,10 +4133,10 @@ func (mock *DBMock) UpdateDataSourceCalls() []struct { } // UpdateDocument calls UpdateDocumentFunc. -func (mock *DBMock) UpdateDocument(ctx context.Context, doc document.Document) error { +func (mock *DBMock) UpdateDocument(ctx context.Context, doc documentCore.Document) error { callInfo := struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document }{ Ctx: ctx, Doc: doc, @@ -4105,11 +4159,11 @@ func (mock *DBMock) UpdateDocument(ctx context.Context, doc document.Document) e // len(mockedDB.UpdateDocumentCalls()) func (mock *DBMock) UpdateDocumentCalls() []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } { var calls []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } mock.lockUpdateDocument.RLock() calls = mock.calls.UpdateDocument @@ -4118,10 +4172,10 @@ func (mock *DBMock) UpdateDocumentCalls() []struct { } // UpdateDocumentBranchMetadata calls UpdateDocumentBranchMetadataFunc. -func (mock *DBMock) UpdateDocumentBranchMetadata(ctx context.Context, doc document.Document) error { +func (mock *DBMock) UpdateDocumentBranchMetadata(ctx context.Context, doc documentCore.Document) error { callInfo := struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document }{ Ctx: ctx, Doc: doc, @@ -4144,11 +4198,11 @@ func (mock *DBMock) UpdateDocumentBranchMetadata(ctx context.Context, doc docume // len(mockedDB.UpdateDocumentBranchMetadataCalls()) func (mock *DBMock) UpdateDocumentBranchMetadataCalls() []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } { var calls []struct { Ctx context.Context - Doc document.Document + Doc documentCore.Document } mock.lockUpdateDocumentBranchMetadata.RLock() calls = mock.calls.UpdateDocumentBranchMetadata @@ -4321,10 +4375,10 @@ func (mock *DBMock) UpdateDocumentParentIDCalls() []struct { } // UpdateDocumentTree calls UpdateDocumentTreeFunc. -func (mock *DBMock) UpdateDocumentTree(ctx context.Context, ss document.Summaries, organizationID string) error { +func (mock *DBMock) UpdateDocumentTree(ctx context.Context, ss documentCore.Summaries, organizationID string) error { callInfo := struct { Ctx context.Context - Ss document.Summaries + Ss documentCore.Summaries OrganizationID string }{ Ctx: ctx, @@ -4349,12 +4403,12 @@ func (mock *DBMock) UpdateDocumentTree(ctx context.Context, ss document.Summarie // len(mockedDB.UpdateDocumentTreeCalls()) func (mock *DBMock) UpdateDocumentTreeCalls() []struct { Ctx context.Context - Ss document.Summaries + Ss documentCore.Summaries OrganizationID string } { var calls []struct { Ctx context.Context - Ss document.Summaries + Ss documentCore.Summaries OrganizationID string } mock.lockUpdateDocumentTree.RLock() diff --git a/server/core/internal/server/0mock_storer_test.go b/server/core/internal/server/0mock_storer_test.go index ffcc96e1..51447d1f 100644 --- a/server/core/internal/server/0mock_storer_test.go +++ b/server/core/internal/server/0mock_storer_test.go @@ -21,6 +21,9 @@ var _ Storer = &StorerMock{} // // // make and configure a mocked Storer // mockedStorer := &StorerMock{ +// CopyFunc: func(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error { +// panic("mock out the Copy method") +// }, // DeleteFunc: func(ctx context.Context, folder string, id string) error { // panic("mock out the Delete method") // }, @@ -37,6 +40,9 @@ var _ Storer = &StorerMock{} // // } type StorerMock struct { + // CopyFunc mocks the Copy method. + CopyFunc func(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error + // DeleteFunc mocks the Delete method. DeleteFunc func(ctx context.Context, folder string, id string) error @@ -48,6 +54,19 @@ type StorerMock struct { // calls tracks calls to the methods. calls struct { + // Copy holds details about calls to the Copy method. + Copy []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // SrcFolder is the srcFolder argument value. + SrcFolder string + // SrcID is the srcID argument value. + SrcID string + // DstFolder is the dstFolder argument value. + DstFolder string + // DstID is the dstID argument value. + DstID string + } // Delete holds details about calls to the Delete method. Delete []struct { // Ctx is the ctx argument value. @@ -78,11 +97,63 @@ type StorerMock struct { R io.Reader } } + lockCopy sync.RWMutex lockDelete sync.RWMutex lockRetrieve sync.RWMutex lockUpload sync.RWMutex } +// Copy calls CopyFunc. +func (mock *StorerMock) Copy(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error { + callInfo := struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string + }{ + Ctx: ctx, + SrcFolder: srcFolder, + SrcID: srcID, + DstFolder: dstFolder, + DstID: dstID, + } + mock.lockCopy.Lock() + mock.calls.Copy = append(mock.calls.Copy, callInfo) + mock.lockCopy.Unlock() + if mock.CopyFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.CopyFunc(ctx, srcFolder, srcID, dstFolder, dstID) +} + +// CopyCalls gets all the calls that were made to Copy. +// Check the length with: +// +// len(mockedStorer.CopyCalls()) +func (mock *StorerMock) CopyCalls() []struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string +} { + var calls []struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string + } + mock.lockCopy.RLock() + calls = mock.calls.Copy + mock.lockCopy.RUnlock() + return calls +} + // Delete calls DeleteFunc. func (mock *StorerMock) Delete(ctx context.Context, folder string, id string) 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 e89e580d..f06d808c 100644 --- a/server/core/internal/server/internal/document/0mock_db_test.go +++ b/server/core/internal/server/internal/document/0mock_db_test.go @@ -9,6 +9,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/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -69,6 +70,9 @@ var _ DB = &DBMock{} // FetchDocumentByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) { // panic("mock out the FetchDocumentByBranchID method") // }, +// FetchDocumentFileFunc: func(ctx context.Context, id string, organizationID string) (*file.File, error) { +// panic("mock out the FetchDocumentFile method") +// }, // FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { // panic("mock out the FetchDocumentHooksByBranchID method") // }, @@ -96,6 +100,9 @@ var _ DB = &DBMock{} // InsertDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the InsertDocument method") // }, +// InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { +// panic("mock out the InsertDocumentFile method") +// }, // InsertDocumentHookFunc: func(ctx context.Context, hk hook.Hook) error { // panic("mock out the InsertDocumentHook method") // }, @@ -178,6 +185,9 @@ type DBMock struct { // FetchDocumentByBranchIDFunc mocks the FetchDocumentByBranchID method. FetchDocumentByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) + // FetchDocumentFileFunc mocks the FetchDocumentFile method. + FetchDocumentFileFunc func(ctx context.Context, id string, organizationID string) (*file.File, error) + // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) @@ -205,6 +215,9 @@ type DBMock struct { // InsertDocumentFunc mocks the InsertDocument method. InsertDocumentFunc func(ctx context.Context, doc documentCore.Document) error + // InsertDocumentFileFunc mocks the InsertDocumentFile method. + InsertDocumentFileFunc func(ctx context.Context, f file.File) error + // InsertDocumentHookFunc mocks the InsertDocumentHook method. InsertDocumentHookFunc func(ctx context.Context, hk hook.Hook) error @@ -376,6 +389,15 @@ type DBMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } + // FetchDocumentFile holds details about calls to the FetchDocumentFile method. + FetchDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + // OrganizationID is the organizationID argument value. + OrganizationID string + } // FetchDocumentHooksByBranchID holds details about calls to the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchID []struct { // Ctx is the ctx argument value. @@ -455,6 +477,13 @@ type DBMock struct { // Doc is the doc argument value. Doc documentCore.Document } + // InsertDocumentFile holds details about calls to the InsertDocumentFile method. + InsertDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // F is the f argument value. + F file.File + } // InsertDocumentHook holds details about calls to the InsertDocumentHook method. InsertDocumentHook []struct { // Ctx is the ctx argument value. @@ -557,6 +586,7 @@ type DBMock struct { lockFetchDocumentBranches sync.RWMutex lockFetchDocumentBranchesUnsafe sync.RWMutex lockFetchDocumentByBranchID sync.RWMutex + lockFetchDocumentFile sync.RWMutex lockFetchDocumentHooksByBranchID sync.RWMutex lockFetchDocumentHooksByDocumentID sync.RWMutex lockFetchDocumentMaintainers sync.RWMutex @@ -566,6 +596,7 @@ type DBMock struct { lockForkDocumentBranch sync.RWMutex lockInsertBranchReviewer sync.RWMutex lockInsertDocument sync.RWMutex + lockInsertDocumentFile sync.RWMutex lockInsertDocumentHook sync.RWMutex lockInsertDocumentSearchJob sync.RWMutex lockPromoteBranchApprovals sync.RWMutex @@ -1240,6 +1271,50 @@ func (mock *DBMock) FetchDocumentByBranchIDCalls() []struct { return calls } +// FetchDocumentFile calls FetchDocumentFileFunc. +func (mock *DBMock) FetchDocumentFile(ctx context.Context, id string, organizationID string) (*file.File, error) { + callInfo := struct { + Ctx context.Context + ID string + OrganizationID string + }{ + Ctx: ctx, + ID: id, + OrganizationID: organizationID, + } + mock.lockFetchDocumentFile.Lock() + mock.calls.FetchDocumentFile = append(mock.calls.FetchDocumentFile, callInfo) + mock.lockFetchDocumentFile.Unlock() + if mock.FetchDocumentFileFunc == nil { + var ( + fileOut *file.File + errOut error + ) + return fileOut, errOut + } + return mock.FetchDocumentFileFunc(ctx, id, organizationID) +} + +// FetchDocumentFileCalls gets all the calls that were made to FetchDocumentFile. +// Check the length with: +// +// len(mockedDB.FetchDocumentFileCalls()) +func (mock *DBMock) FetchDocumentFileCalls() []struct { + Ctx context.Context + ID string + OrganizationID string +} { + var calls []struct { + Ctx context.Context + ID string + OrganizationID string + } + mock.lockFetchDocumentFile.RLock() + calls = mock.calls.FetchDocumentFile + mock.lockFetchDocumentFile.RUnlock() + return calls +} + // FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. func (mock *DBMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { callInfo := struct { @@ -1629,6 +1704,45 @@ func (mock *DBMock) InsertDocumentCalls() []struct { return calls } +// InsertDocumentFile calls InsertDocumentFileFunc. +func (mock *DBMock) InsertDocumentFile(ctx context.Context, f file.File) error { + callInfo := struct { + Ctx context.Context + F file.File + }{ + Ctx: ctx, + F: f, + } + mock.lockInsertDocumentFile.Lock() + mock.calls.InsertDocumentFile = append(mock.calls.InsertDocumentFile, callInfo) + mock.lockInsertDocumentFile.Unlock() + if mock.InsertDocumentFileFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentFileFunc(ctx, f) +} + +// InsertDocumentFileCalls gets all the calls that were made to InsertDocumentFile. +// Check the length with: +// +// len(mockedDB.InsertDocumentFileCalls()) +func (mock *DBMock) InsertDocumentFileCalls() []struct { + Ctx context.Context + F file.File +} { + var calls []struct { + Ctx context.Context + F file.File + } + mock.lockInsertDocumentFile.RLock() + calls = mock.calls.InsertDocumentFile + mock.lockInsertDocumentFile.RUnlock() + return calls +} + // InsertDocumentHook calls InsertDocumentHookFunc. func (mock *DBMock) InsertDocumentHook(ctx context.Context, hk hook.Hook) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/0mock_storer_test.go b/server/core/internal/server/internal/document/0mock_storer_test.go new file mode 100644 index 00000000..b52c0a6c --- /dev/null +++ b/server/core/internal/server/internal/document/0mock_storer_test.go @@ -0,0 +1,102 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package document + +import ( + "context" + "sync" +) + +// Ensure, that StorerMock does implement Storer. +// If this is not the case, regenerate this file with moq. +var _ Storer = &StorerMock{} + +// StorerMock is a mock implementation of Storer. +// +// func TestSomethingThatUsesStorer(t *testing.T) { +// +// // make and configure a mocked Storer +// mockedStorer := &StorerMock{ +// CopyFunc: func(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error { +// panic("mock out the Copy method") +// }, +// } +// +// // use mockedStorer in code that requires Storer +// // and then make assertions. +// +// } +type StorerMock struct { + // CopyFunc mocks the Copy method. + CopyFunc func(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error + + // calls tracks calls to the methods. + calls struct { + // Copy holds details about calls to the Copy method. + Copy []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // SrcFolder is the srcFolder argument value. + SrcFolder string + // SrcID is the srcID argument value. + SrcID string + // DstFolder is the dstFolder argument value. + DstFolder string + // DstID is the dstID argument value. + DstID string + } + } + lockCopy sync.RWMutex +} + +// Copy calls CopyFunc. +func (mock *StorerMock) Copy(ctx context.Context, srcFolder string, srcID string, dstFolder string, dstID string) error { + callInfo := struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string + }{ + Ctx: ctx, + SrcFolder: srcFolder, + SrcID: srcID, + DstFolder: dstFolder, + DstID: dstID, + } + mock.lockCopy.Lock() + mock.calls.Copy = append(mock.calls.Copy, callInfo) + mock.lockCopy.Unlock() + if mock.CopyFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.CopyFunc(ctx, srcFolder, srcID, dstFolder, dstID) +} + +// CopyCalls gets all the calls that were made to Copy. +// Check the length with: +// +// len(mockedStorer.CopyCalls()) +func (mock *StorerMock) CopyCalls() []struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string +} { + var calls []struct { + Ctx context.Context + SrcFolder string + SrcID string + DstFolder string + DstID string + } + mock.lockCopy.RLock() + calls = mock.calls.Copy + mock.lockCopy.RUnlock() + return calls +} 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 5c4ce000..e18b81ab 100644 --- a/server/core/internal/server/internal/document/0mock_tx_test.go +++ b/server/core/internal/server/internal/document/0mock_tx_test.go @@ -9,6 +9,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/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -69,6 +70,9 @@ var _ Tx = &TxMock{} // FetchDocumentByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) { // panic("mock out the FetchDocumentByBranchID method") // }, +// FetchDocumentFileFunc: func(ctx context.Context, id string, organizationID string) (*file.File, error) { +// panic("mock out the FetchDocumentFile method") +// }, // FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { // panic("mock out the FetchDocumentHooksByBranchID method") // }, @@ -96,6 +100,9 @@ var _ Tx = &TxMock{} // InsertDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the InsertDocument method") // }, +// InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { +// panic("mock out the InsertDocumentFile method") +// }, // InsertDocumentHookFunc: func(ctx context.Context, hk hook.Hook) error { // panic("mock out the InsertDocumentHook method") // }, @@ -181,6 +188,9 @@ type TxMock struct { // FetchDocumentByBranchIDFunc mocks the FetchDocumentByBranchID method. FetchDocumentByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) (*documentCore.Document, error) + // FetchDocumentFileFunc mocks the FetchDocumentFile method. + FetchDocumentFileFunc func(ctx context.Context, id string, organizationID string) (*file.File, error) + // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) @@ -208,6 +218,9 @@ type TxMock struct { // InsertDocumentFunc mocks the InsertDocument method. InsertDocumentFunc func(ctx context.Context, doc documentCore.Document) error + // InsertDocumentFileFunc mocks the InsertDocumentFile method. + InsertDocumentFileFunc func(ctx context.Context, f file.File) error + // InsertDocumentHookFunc mocks the InsertDocumentHook method. InsertDocumentHookFunc func(ctx context.Context, hk hook.Hook) error @@ -378,6 +391,15 @@ type TxMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } + // FetchDocumentFile holds details about calls to the FetchDocumentFile method. + FetchDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + // OrganizationID is the organizationID argument value. + OrganizationID string + } // FetchDocumentHooksByBranchID holds details about calls to the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchID []struct { // Ctx is the ctx argument value. @@ -457,6 +479,13 @@ type TxMock struct { // Doc is the doc argument value. Doc documentCore.Document } + // InsertDocumentFile holds details about calls to the InsertDocumentFile method. + InsertDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // F is the f argument value. + F file.File + } // InsertDocumentHook holds details about calls to the InsertDocumentHook method. InsertDocumentHook []struct { // Ctx is the ctx argument value. @@ -562,6 +591,7 @@ type TxMock struct { lockFetchDocumentBranches sync.RWMutex lockFetchDocumentBranchesUnsafe sync.RWMutex lockFetchDocumentByBranchID sync.RWMutex + lockFetchDocumentFile sync.RWMutex lockFetchDocumentHooksByBranchID sync.RWMutex lockFetchDocumentHooksByDocumentID sync.RWMutex lockFetchDocumentMaintainers sync.RWMutex @@ -571,6 +601,7 @@ type TxMock struct { lockForkDocumentBranch sync.RWMutex lockInsertBranchReviewer sync.RWMutex lockInsertDocument sync.RWMutex + lockInsertDocumentFile sync.RWMutex lockInsertDocumentHook sync.RWMutex lockInsertDocumentSearchJob sync.RWMutex lockPromoteBranchApprovals sync.RWMutex @@ -1237,6 +1268,50 @@ func (mock *TxMock) FetchDocumentByBranchIDCalls() []struct { return calls } +// FetchDocumentFile calls FetchDocumentFileFunc. +func (mock *TxMock) FetchDocumentFile(ctx context.Context, id string, organizationID string) (*file.File, error) { + callInfo := struct { + Ctx context.Context + ID string + OrganizationID string + }{ + Ctx: ctx, + ID: id, + OrganizationID: organizationID, + } + mock.lockFetchDocumentFile.Lock() + mock.calls.FetchDocumentFile = append(mock.calls.FetchDocumentFile, callInfo) + mock.lockFetchDocumentFile.Unlock() + if mock.FetchDocumentFileFunc == nil { + var ( + fileOut *file.File + errOut error + ) + return fileOut, errOut + } + return mock.FetchDocumentFileFunc(ctx, id, organizationID) +} + +// FetchDocumentFileCalls gets all the calls that were made to FetchDocumentFile. +// Check the length with: +// +// len(mockedTx.FetchDocumentFileCalls()) +func (mock *TxMock) FetchDocumentFileCalls() []struct { + Ctx context.Context + ID string + OrganizationID string +} { + var calls []struct { + Ctx context.Context + ID string + OrganizationID string + } + mock.lockFetchDocumentFile.RLock() + calls = mock.calls.FetchDocumentFile + mock.lockFetchDocumentFile.RUnlock() + return calls +} + // FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. func (mock *TxMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { callInfo := struct { @@ -1626,6 +1701,45 @@ func (mock *TxMock) InsertDocumentCalls() []struct { return calls } +// InsertDocumentFile calls InsertDocumentFileFunc. +func (mock *TxMock) InsertDocumentFile(ctx context.Context, f file.File) error { + callInfo := struct { + Ctx context.Context + F file.File + }{ + Ctx: ctx, + F: f, + } + mock.lockInsertDocumentFile.Lock() + mock.calls.InsertDocumentFile = append(mock.calls.InsertDocumentFile, callInfo) + mock.lockInsertDocumentFile.Unlock() + if mock.InsertDocumentFileFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentFileFunc(ctx, f) +} + +// InsertDocumentFileCalls gets all the calls that were made to InsertDocumentFile. +// Check the length with: +// +// len(mockedTx.InsertDocumentFileCalls()) +func (mock *TxMock) InsertDocumentFileCalls() []struct { + Ctx context.Context + F file.File +} { + var calls []struct { + Ctx context.Context + F file.File + } + mock.lockInsertDocumentFile.RLock() + calls = mock.calls.InsertDocumentFile + mock.lockInsertDocumentFile.RUnlock() + return calls +} + // InsertDocumentHook calls InsertDocumentHookFunc. func (mock *TxMock) InsertDocumentHook(ctx context.Context, hk hook.Hook) error { callInfo := struct { 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 2de98a8d..349cb65b 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -3,12 +3,14 @@ package document import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" "testing" "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/file" 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" @@ -23,7 +25,7 @@ func storedHook(typ hookCore.Type) hookCore.Hook { return hookCore.Hook{ ID: xid.New(), Type: typ, - DocumentID: _documentID, + DocumentID: null.ValueFrom(_documentID), OrganizationID: "org1", BranchID: null.ValueFrom(_branchID), Settings: processor.Settings(`{"scale":"linear"}`), @@ -655,15 +657,50 @@ func Test_Handler_DuplicateDocument(t *testing.T) { return tx } + // fetchStoredWithImage serves a document whose content holds an image + // served by that same document, which is what duplication has to copy. + fetchStoredWithImage := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { + doc := storedDoc() + doc.Content = documentCore.RootBlock{ + Type: documentCore.BlockNodeDoc, + Content: []documentCore.Block{ + { + Type: documentCore.BlockNodeImageBlock, + Attrs: documentCore.Attributes{ + "uid": "img1", + "src": "https://app.test/core" + fmt.Sprintf(documentCore.FilePathFormat, _documentID, "img1"), + }, + }, + }, + } + + return doc, nil + } + + // imageDB serves the document with the image plus the file row that + // image refers to, which is what the copy is made from. + imageDB := func() *DBMock { + return &DBMock{ + FetchDocumentFunc: fetchStoredWithImage, + FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { + f := file.NewFile(id, file.LocationDocument, "key", _documentID, organizationID) + + return &f, nil + }, + } + } + cc := map[string]struct { DB *DBMock Tx *TxMock + Storer *StorerMock BeginErr error NoSession bool OmitDoc bool RespCode int Committed int TreeCbs int + Copies int }{ "No session in context": { DB: &DBMock{}, @@ -726,6 +763,28 @@ func Test_Handler_DuplicateDocument(t *testing.T) { Committed: 1, TreeCbs: 1, }, + "Files are copied along with the document": { + DB: imageDB(), + Tx: insertAwareTx(), + Storer: &StorerMock{}, + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + Copies: 1, + }, + "Failing file copy still yields the duplicate": { + DB: imageDB(), + Tx: insertAwareTx(), + Storer: &StorerMock{ + CopyFunc: func(context.Context, string, string, string, string) error { + return errors.New("boom") + }, + }, + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + Copies: 1, + }, } for cn, c := range cc { @@ -734,6 +793,10 @@ func Test_Handler_DuplicateDocument(t *testing.T) { hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) + if c.Storer != nil { + hdl.storer = c.Storer + } + rec := httptest.NewRecorder() hdl.DuplicateDocument(rec, newRequest(http.MethodPost, "", c.NoSession, c.OmitDoc, true)) @@ -742,6 +805,11 @@ func Test_Handler_DuplicateDocument(t *testing.T) { assert.Len(t, c.Tx.CommitCalls(), c.Committed) assert.Equal(t, c.TreeCbs, cnt.tree) + if c.Storer != nil { + assert.Len(t, c.Storer.CopyCalls(), c.Copies) + assert.Len(t, c.DB.InsertDocumentFileCalls(), c.Copies) + } + if c.RespCode == http.StatusCreated { // the duplicate is a new document owned by the caller. require.Len(t, c.Tx.InsertDocumentCalls(), 1) @@ -752,6 +820,110 @@ func Test_Handler_DuplicateDocument(t *testing.T) { } } +func Test_Handler_copyDocumentFiles(t *testing.T) { + toDocumentID := xid.New() + + fileDB := func() *DBMock { + return &DBMock{ + FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { + f := file.NewFile(id, file.LocationComment, "key", _documentID, organizationID) + + return &f, nil + }, + } + } + + cc := map[string]struct { + DB *DBMock + Storer *StorerMock + Files map[string]string + Inserts int + Copies int + }{ + "Nothing to copy": { + DB: fileDB(), + Storer: &StorerMock{}, + Files: map[string]string{}, + }, + "File row is missing": { + DB: &DBMock{ + FetchDocumentFileFunc: func(context.Context, string, string) (*file.File, error) { + return nil, errors.New("boom") + }, + }, + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + }, + "Error returned by db.InsertDocumentFile": { + DB: func() *DBMock { + db := fileDB() + db.InsertDocumentFileFunc = func(context.Context, file.File) error { + return errors.New("boom") + } + + return db + }(), + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + }, + "Error returned by storer.Copy": { + DB: fileDB(), + Storer: &StorerMock{ + CopyFunc: func(context.Context, string, string, string, string) error { + return errors.New("boom") + }, + }, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + Copies: 1, + }, + "Successful copy": { + DB: fileDB(), + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + Copies: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + hdl, _ := newTestHandler(c.DB, &fakePublisher{}) + hdl.storer = c.Storer + + hdl.copyDocumentFiles(context.Background(), c.Files, _documentID, toDocumentID, "org1") + + ff := c.DB.InsertDocumentFileCalls() + require.Len(t, ff, c.Inserts) + + cf := c.Storer.CopyCalls() + require.Len(t, cf, c.Copies) + + if c.Inserts == 0 { + return + } + + // the copy owns its row, keyed under the new document. + assert.Equal(t, "new-1", ff[0].F.ID) + assert.Equal(t, file.LocationComment, ff[0].F.Location) + assert.Equal(t, file.Key("org1", toDocumentID, "new-1"), ff[0].F.StorageKey) + assert.Equal(t, toDocumentID, ff[0].F.DocumentID.V) + + if c.Copies == 0 { + return + } + + assert.Equal(t, file.Folder("org1", _documentID), cf[0].SrcFolder) + assert.Equal(t, "old-1", cf[0].SrcID) + assert.Equal(t, file.Folder("org1", toDocumentID), cf[0].DstFolder) + assert.Equal(t, "new-1", cf[0].DstID) + }) + } +} + func Test_Handler_FetchDocumentBranches(t *testing.T) { cc := map[string]struct { DB *DBMock diff --git a/server/core/internal/server/internal/document/files/0mock_db_test.go b/server/core/internal/server/internal/document/files/0mock_db_test.go index 0cdbd94e..05957e58 100644 --- a/server/core/internal/server/internal/document/files/0mock_db_test.go +++ b/server/core/internal/server/internal/document/files/0mock_db_test.go @@ -7,7 +7,7 @@ import ( "context" "sync" - "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/file" "github.com/rs/xid" ) @@ -24,10 +24,13 @@ var _ DB = &DBMock{} // CheckDocumentExistsFunc: func(ctx context.Context, id xid.ID, organizationID string) error { // panic("mock out the CheckDocumentExists method") // }, -// FetchDocumentFileFunc: func(ctx context.Context, blockID string, organizationID string) (*document.File, error) { +// DeleteDocumentFileFunc: func(ctx context.Context, id string) error { +// panic("mock out the DeleteDocumentFile method") +// }, +// FetchDocumentFileFunc: func(ctx context.Context, blockID string, organizationID string) (*file.File, error) { // panic("mock out the FetchDocumentFile method") // }, -// InsertDocumentFileFunc: func(ctx context.Context, f document.File) error { +// InsertDocumentFileFunc: func(ctx context.Context, f file.File) error { // panic("mock out the InsertDocumentFile method") // }, // } @@ -40,11 +43,14 @@ type DBMock struct { // CheckDocumentExistsFunc mocks the CheckDocumentExists method. CheckDocumentExistsFunc func(ctx context.Context, id xid.ID, organizationID string) error + // DeleteDocumentFileFunc mocks the DeleteDocumentFile method. + DeleteDocumentFileFunc func(ctx context.Context, id string) error + // FetchDocumentFileFunc mocks the FetchDocumentFile method. - FetchDocumentFileFunc func(ctx context.Context, blockID string, organizationID string) (*document.File, error) + FetchDocumentFileFunc func(ctx context.Context, blockID string, organizationID string) (*file.File, error) // InsertDocumentFileFunc mocks the InsertDocumentFile method. - InsertDocumentFileFunc func(ctx context.Context, f document.File) error + InsertDocumentFileFunc func(ctx context.Context, f file.File) error // calls tracks calls to the methods. calls struct { @@ -57,6 +63,13 @@ type DBMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } + // DeleteDocumentFile holds details about calls to the DeleteDocumentFile method. + DeleteDocumentFile []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + } // FetchDocumentFile holds details about calls to the FetchDocumentFile method. FetchDocumentFile []struct { // Ctx is the ctx argument value. @@ -71,10 +84,11 @@ type DBMock struct { // Ctx is the ctx argument value. Ctx context.Context // F is the f argument value. - F document.File + F file.File } } lockCheckDocumentExists sync.RWMutex + lockDeleteDocumentFile sync.RWMutex lockFetchDocumentFile sync.RWMutex lockInsertDocumentFile sync.RWMutex } @@ -122,8 +136,47 @@ func (mock *DBMock) CheckDocumentExistsCalls() []struct { return calls } +// DeleteDocumentFile calls DeleteDocumentFileFunc. +func (mock *DBMock) DeleteDocumentFile(ctx context.Context, id string) error { + callInfo := struct { + Ctx context.Context + ID string + }{ + Ctx: ctx, + ID: id, + } + mock.lockDeleteDocumentFile.Lock() + mock.calls.DeleteDocumentFile = append(mock.calls.DeleteDocumentFile, callInfo) + mock.lockDeleteDocumentFile.Unlock() + if mock.DeleteDocumentFileFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.DeleteDocumentFileFunc(ctx, id) +} + +// DeleteDocumentFileCalls gets all the calls that were made to DeleteDocumentFile. +// Check the length with: +// +// len(mockedDB.DeleteDocumentFileCalls()) +func (mock *DBMock) DeleteDocumentFileCalls() []struct { + Ctx context.Context + ID string +} { + var calls []struct { + Ctx context.Context + ID string + } + mock.lockDeleteDocumentFile.RLock() + calls = mock.calls.DeleteDocumentFile + mock.lockDeleteDocumentFile.RUnlock() + return calls +} + // FetchDocumentFile calls FetchDocumentFileFunc. -func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organizationID string) (*document.File, error) { +func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organizationID string) (*file.File, error) { callInfo := struct { Ctx context.Context BlockID string @@ -138,7 +191,7 @@ func (mock *DBMock) FetchDocumentFile(ctx context.Context, blockID string, organ mock.lockFetchDocumentFile.Unlock() if mock.FetchDocumentFileFunc == nil { var ( - fileOut *document.File + fileOut *file.File errOut error ) return fileOut, errOut @@ -167,10 +220,10 @@ func (mock *DBMock) FetchDocumentFileCalls() []struct { } // InsertDocumentFile calls InsertDocumentFileFunc. -func (mock *DBMock) InsertDocumentFile(ctx context.Context, f document.File) error { +func (mock *DBMock) InsertDocumentFile(ctx context.Context, f file.File) error { callInfo := struct { Ctx context.Context - F document.File + F file.File }{ Ctx: ctx, F: f, @@ -193,11 +246,11 @@ func (mock *DBMock) InsertDocumentFile(ctx context.Context, f document.File) err // len(mockedDB.InsertDocumentFileCalls()) func (mock *DBMock) InsertDocumentFileCalls() []struct { Ctx context.Context - F document.File + F file.File } { var calls []struct { Ctx context.Context - F document.File + F file.File } mock.lockInsertDocumentFile.RLock() calls = mock.calls.InsertDocumentFile diff --git a/server/core/internal/server/internal/document/files/0mock_storer_test.go b/server/core/internal/server/internal/document/files/0mock_storer_test.go index 8be331dd..d1379b50 100644 --- a/server/core/internal/server/internal/document/files/0mock_storer_test.go +++ b/server/core/internal/server/internal/document/files/0mock_storer_test.go @@ -21,9 +21,6 @@ var _ Storer = &StorerMock{} // // // make and configure a mocked Storer // mockedStorer := &StorerMock{ -// DeleteFunc: func(ctx context.Context, folder string, id string) error { -// panic("mock out the Delete method") -// }, // RetrieveFunc: func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { // panic("mock out the Retrieve method") // }, @@ -37,9 +34,6 @@ var _ Storer = &StorerMock{} // // } type StorerMock struct { - // DeleteFunc mocks the Delete method. - DeleteFunc func(ctx context.Context, folder string, id string) error - // RetrieveFunc mocks the Retrieve method. RetrieveFunc func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) @@ -48,15 +42,6 @@ type StorerMock struct { // calls tracks calls to the methods. calls struct { - // Delete holds details about calls to the Delete method. - Delete []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // Folder is the folder argument value. - Folder string - // ID is the id argument value. - ID string - } // Retrieve holds details about calls to the Retrieve method. Retrieve []struct { // Ctx is the ctx argument value. @@ -78,54 +63,10 @@ type StorerMock struct { R io.Reader } } - lockDelete sync.RWMutex lockRetrieve sync.RWMutex lockUpload sync.RWMutex } -// Delete calls DeleteFunc. -func (mock *StorerMock) Delete(ctx context.Context, folder string, id string) error { - callInfo := struct { - Ctx context.Context - Folder string - ID string - }{ - Ctx: ctx, - Folder: folder, - ID: id, - } - mock.lockDelete.Lock() - mock.calls.Delete = append(mock.calls.Delete, callInfo) - mock.lockDelete.Unlock() - if mock.DeleteFunc == nil { - var ( - errOut error - ) - return errOut - } - return mock.DeleteFunc(ctx, folder, id) -} - -// DeleteCalls gets all the calls that were made to Delete. -// Check the length with: -// -// len(mockedStorer.DeleteCalls()) -func (mock *StorerMock) DeleteCalls() []struct { - Ctx context.Context - Folder string - ID string -} { - var calls []struct { - Ctx context.Context - Folder string - ID string - } - mock.lockDelete.RLock() - calls = mock.calls.Delete - mock.lockDelete.RUnlock() - return calls -} - // Retrieve calls RetrieveFunc. func (mock *StorerMock) Retrieve(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { callInfo := struct { diff --git a/server/core/internal/server/internal/document/files/http.go b/server/core/internal/server/internal/document/files/http.go index 848c3b30..588dab44 100644 --- a/server/core/internal/server/internal/document/files/http.go +++ b/server/core/internal/server/internal/document/files/http.go @@ -8,16 +8,14 @@ import ( "log/slog" "net/http" - "github.com/oxynote/oxynote/server/core/internal/document" + "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/httpserver" "github.com/rs/xid" ) -// _documentFilesFolderFormat is the folder format for document files. -const _documentFilesFolderFormat = "organizations/%s/documents/%s/files" - // Handler holds dependencies required for document file operations. type Handler struct { log *slog.Logger @@ -68,40 +66,43 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { return } - location := document.Location(r.URL.Query().Get("location")) + location := file.Location(r.URL.Query().Get("location")) if !location.Valid() { httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) return } - file, _, err := r.FormFile("file") + body, _, err := r.FormFile("file") if err != nil { httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) return } - defer file.Close() //nolint:errcheck // error provides no meaningful info + defer body.Close() //nolint:errcheck // error provides no meaningful info - folder := fmt.Sprintf(_documentFilesFolderFormat, session.ActiveOrganizationID, documentID) - - err = h.storer.Upload(r.Context(), folder, fileID, file) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } + folder := file.Folder(session.ActiveOrganizationID, documentID) + // the row is written before the object so that a crash can only ever + // leave a row without an object, which the file manager reaps. An + // object without a row would be invisible to every cleanup path. err = h.db.InsertDocumentFile( r.Context(), - document.NewFile( + file.NewFile( fileID, location, + file.Key(session.ActiveOrganizationID, documentID, fileID), documentID, session.ActiveOrganizationID, ), ) if err != nil { - // Clean up the uploaded file if DB insert fails. - if derr := h.storer.Delete(r.Context(), folder, fileID); derr != nil { - h.log.Error("deleting file after DB failure", "error", derr.Error()) + httpserver.RespondError(h.log, w, err) + return + } + + err = h.storer.Upload(r.Context(), folder, fileID, body) + if err != nil { + if derr := h.db.DeleteDocumentFile(r.Context(), fileID); derr != nil { + h.log.Error("deleting file row after upload failure", "error", derr.Error()) } httpserver.RespondError(h.log, w, err) @@ -141,13 +142,21 @@ func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { } // Verify file exists and belongs to the organization. - _, err = h.db.FetchDocumentFile(r.Context(), id, session.ActiveOrganizationID) + f, err := h.db.FetchDocumentFile(r.Context(), id, session.ActiveOrganizationID) if err != nil { httpserver.RespondError(h.log, w, err) return } - folder := fmt.Sprintf(_documentFilesFolderFormat, session.ActiveOrganizationID, documentID) + // the object key is built from the document in the url, so a file + // belonging to another document would merely miss the key; reject it + // as the authorization failure it is instead. + if f.DocumentID.V != documentID { + httpserver.RespondError(h.log, w, errutil.ErrNotFound) + return + } + + folder := file.Folder(session.ActiveOrganizationID, documentID) obj, found, err := h.storer.Retrieve(r.Context(), folder, id) if err != nil { @@ -200,10 +209,13 @@ type DB interface { // DBAgent is an interface that handles communication with the document files database. type DBAgent interface { // InsertDocumentFile should insert the document file. - InsertDocumentFile(ctx context.Context, f document.File) error + InsertDocumentFile(ctx context.Context, f file.File) error // FetchDocumentFile should fetch the document file for the given block id. - FetchDocumentFile(ctx context.Context, blockID, organizationID string) (*document.File, error) + FetchDocumentFile(ctx context.Context, blockID, organizationID string) (*file.File, error) + + // DeleteDocumentFile should remove the document file row. + DeleteDocumentFile(ctx context.Context, id string) error } // Storer is an interface that defines methods for uploading and retrieving objects. @@ -215,7 +227,4 @@ type Storer interface { // Retrieve retrieves an object by its ID. Retrieve(ctx context.Context, folder, id string) (*storage.ObjectInfo, bool, error) - - // Delete deletes an object by its ID. - Delete(ctx context.Context, folder, id string) error } 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 31e08ab0..ec6ddf66 100644 --- a/server/core/internal/server/internal/document/files/http_test.go +++ b/server/core/internal/server/internal/document/files/http_test.go @@ -12,7 +12,7 @@ import ( "strings" "testing" - "github.com/oxynote/oxynote/server/core/internal/document" + "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/testutil" @@ -111,15 +111,23 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { } assert.Equal(t, "f1", ff[0].F.ID) - assert.Equal(t, document.LocationDocument, ff[0].F.Location) - assert.Equal(t, _documentID, ff[0].F.DocumentID) - assert.Equal(t, "org1", ff[0].F.OrganizationID) + assert.Equal(t, file.LocationDocument, ff[0].F.Location) + assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files/f1", ff[0].F.StorageKey) + assert.Equal(t, _documentID, ff[0].F.DocumentID.V) + assert.Equal(t, "org1", ff[0].F.OrganizationID.String) } } wasDeleteCalled := func(count int) check { - return func(t *testing.T, _ *DBMock, storer *StorerMock, _ *httptest.ResponseRecorder) { - assert.Len(t, storer.DeleteCalls(), count) + return func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) { + ff := db.DeleteDocumentFileCalls() + require.Len(t, ff, count) + + if count == 0 { + return + } + + assert.Equal(t, "f1", ff[0].ID) } } @@ -193,7 +201,22 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { wasUploadCalled(0), ), }, - "Storer upload error": { + "DB insert error": { + DB: &DBMock{ + InsertDocumentFileFunc: func(context.Context, file.File) error { + return errors.New("boom") + }, + }, + Storer: &StorerMock{}, + Query: "?id=f1&location=document", + Checks: checks( + hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), + wasInsertCalled(1), + wasUploadCalled(0), + wasDeleteCalled(0), + ), + }, + "Storer upload error rolls the row back": { DB: &DBMock{}, Storer: &StorerMock{ UploadFunc: func(context.Context, string, string, io.Reader) error { @@ -203,34 +226,20 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { Query: "?id=f1&location=document", Checks: checks( hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), - wasUploadCalled(1), - wasInsertCalled(0), - ), - }, - "DB insert error triggers cleanup": { - DB: &DBMock{ - InsertDocumentFileFunc: func(context.Context, document.File) error { - return errors.New("boom") - }, - }, - Storer: &StorerMock{}, - Query: "?id=f1&location=document", - Checks: checks( - hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), - wasUploadCalled(1), wasInsertCalled(1), + wasUploadCalled(1), wasDeleteCalled(1), ), }, - "DB insert error with failing cleanup": { + "Storer upload error with failing rollback": { DB: &DBMock{ - InsertDocumentFileFunc: func(context.Context, document.File) error { - return errors.New("boom") + DeleteDocumentFileFunc: func(context.Context, string) error { + return errors.New("cleanup boom") }, }, Storer: &StorerMock{ - DeleteFunc: func(context.Context, string, string) error { - return errors.New("cleanup boom") + UploadFunc: func(context.Context, string, string, io.Reader) error { + return errors.New("boom") }, }, Query: "?id=f1&location=document", @@ -253,7 +262,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) { ff := db.InsertDocumentFileCalls() require.Len(t, ff, 1) - assert.Equal(t, document.LocationComment, ff[0].F.Location) + assert.Equal(t, file.LocationComment, ff[0].F.Location) }, wasDeleteCalled(0), ), @@ -307,6 +316,18 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { } } +// stubFileDB returns a db mock whose file record belongs to the given +// document, which is what the ownership check compares the url against. +func stubFileDB(documentID xid.ID) *DBMock { + return &DBMock{ + FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { + f := file.NewFile(id, file.LocationDocument, "key", documentID, organizationID) + + return &f, nil + }, + } +} + func Test_Handler_RetrieveDocumentFile(t *testing.T) { type check func(*testing.T, *DBMock, *StorerMock, *httptest.ResponseRecorder) @@ -387,7 +408,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { }, "File record fetch error": { DB: &DBMock{ - FetchDocumentFileFunc: func(context.Context, string, string) (*document.File, error) { + FetchDocumentFileFunc: func(context.Context, string, string) (*file.File, error) { return nil, errors.New("boom") }, }, @@ -400,8 +421,20 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { wasRetrieveCalled(0), ), }, + "File belongs to another document": { + DB: stubFileDB(xid.New()), + Storer: &StorerMock{}, + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusNotFound, rec.Code) + assert.JSONEq(t, `{"code":"general","message":"not found"}`, rec.Body.String()) + }, + wasFetchCalled(1), + wasRetrieveCalled(0), + ), + }, "Storer retrieval error": { - DB: &DBMock{}, + DB: stubFileDB(_documentID), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return nil, false, errors.New("boom") @@ -415,7 +448,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "File object not found": { - DB: &DBMock{}, + DB: stubFileDB(_documentID), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return nil, false, nil @@ -429,7 +462,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "Matching If-None-Match": { - DB: &DBMock{}, + DB: stubFileDB(_documentID), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return &storage.ObjectInfo{ @@ -448,7 +481,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "Successful retrieval": { - DB: &DBMock{}, + DB: stubFileDB(_documentID), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return &storage.ObjectInfo{ diff --git a/server/core/internal/server/internal/document/hook/http_test.go b/server/core/internal/server/internal/document/hook/http_test.go index f1f434c0..c650598e 100644 --- a/server/core/internal/server/internal/document/hook/http_test.go +++ b/server/core/internal/server/internal/document/hook/http_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/document" hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" @@ -45,7 +46,7 @@ func scheduledHook(typ hookCore.Type) *hookCore.Hook { return &hookCore.Hook{ ID: _hookID, Type: typ, - DocumentID: _documentID, + DocumentID: null.ValueFrom(_documentID), OrganizationID: "org1", Settings: processor.Settings(`{"scale":"linear"}`), } @@ -202,7 +203,7 @@ func Test_Handler_CreateDocumentHook(t *testing.T) { } assert.Equal(t, hookCore.TypeScheduledReminder, ff[0].Hk.Type) - assert.Equal(t, _documentID, ff[0].Hk.DocumentID) + assert.Equal(t, null.ValueFrom(_documentID), ff[0].Hk.DocumentID) assert.Equal(t, "org1", ff[0].Hk.OrganizationID) assert.NotEmpty(t, ff[0].Hk.State) } diff --git a/server/core/internal/server/internal/document/http.go b/server/core/internal/server/internal/document/http.go index 55a173a6..45bd0a8e 100644 --- a/server/core/internal/server/internal/document/http.go +++ b/server/core/internal/server/internal/document/http.go @@ -11,12 +11,14 @@ import ( "github.com/oxynote/oxynote/server/core/internal/apps/github" "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/hook" "github.com/oxynote/oxynote/server/core/internal/notification" "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/sqlutil" "github.com/rs/xid" ) @@ -40,6 +42,7 @@ type Handler struct { webchangeClient *webchange.Client searchGateway SearchGateway notifPub notification.Publisher + storer Storer tree struct { changeCallback func(organizationID string, parentId null.Value[xid.ID]) @@ -66,6 +69,7 @@ func NewHandler( webchangeClient *webchange.Client, searchGateway SearchGateway, notifPub notification.Publisher, + storer Storer, ) *Handler { return &Handler{ log: log, @@ -74,6 +78,7 @@ func NewHandler( webchangeClient: webchangeClient, searchGateway: searchGateway, notifPub: notifPub, + storer: storer, } } @@ -1014,7 +1019,7 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { return } - duplDoc := doc.Duplicate(session.UserID) + duplDoc, files := doc.Duplicate(session.UserID) var tx Tx @@ -1069,6 +1074,8 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { return } + h.copyDocumentFiles(r.Context(), files, id, duplDoc.ID, session.ActiveOrganizationID) + if h.tree.changeCallback != nil { h.tree.changeCallback(session.ActiveOrganizationID, duplDoc.ParentID) } @@ -1081,6 +1088,65 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { ) } +// copyDocumentFiles gives the duplicated document its own copies of the +// source document's files. +// +// It runs after the document is committed, and each copy writes its row +// before the object: a crash can then only leave a row without an object, +// which the file manager reaps, whereas an object without a row would be +// invisible to every cleanup path. A failure here leaves the duplicate with +// broken images rather than failing the duplication outright — the document +// exists, and deleting it reclaims whatever was copied. +func (h *Handler) copyDocumentFiles( + ctx context.Context, + files map[string]string, + fromDocumentID, toDocumentID xid.ID, + organizationID string, +) { + for oldID, newID := range files { + f, err := h.db.FetchDocumentFile(ctx, oldID, organizationID) + if err != nil { + // the content referenced a file that has no row, so there is + // nothing to copy; the duplicate keeps the dangling reference + // its source already had. + h.log.Warn( + "cannot fetch document file for duplication", + slog.String("error", err.Error()), + slog.String("file_id", oldID), + ) + + continue + } + + fromFolder := file.Folder(organizationID, fromDocumentID) + toFolder := file.Folder(organizationID, toDocumentID) + + err = h.db.InsertDocumentFile(ctx, file.NewFile( + newID, + f.Location, + file.Key(organizationID, toDocumentID, newID), + toDocumentID, + organizationID, + )) + if err != nil { + logutil.Critical(h.log, err).Error( + "cannot insert duplicated document file", + slog.String("file_id", newID), + ) + + continue + } + + err = h.storer.Copy(ctx, fromFolder, oldID, toFolder, newID) + if err != nil { + logutil.Critical(h.log, err).Error( + "cannot copy duplicated document file", + slog.String("file_id", newID), + ) + } + } +} + // FetchDocumentBranches handles the retrieval of all branches for a document. func (h *Handler) FetchDocumentBranches(w http.ResponseWriter, r *http.Request) { session, err := auth.ExtractSessionFromContext(r.Context()) @@ -1298,6 +1364,7 @@ type DBAgent interface { BranchesDBAgent TreeDBAgent MaintainersDBAgent + FilesDBAgent // DeleteDocumentCommentsByBranchID should delete all comments for a branch. DeleteDocumentCommentsByBranchID(ctx context.Context, branchID xid.ID, organizationID string) error @@ -1432,6 +1499,26 @@ type ReviewersDBAgent interface { PromoteBranchApprovals(ctx context.Context, fromBranchID, toBranchID xid.ID, organizationID string) error } +// FilesDBAgent is an interface that handles communication with the document +// files database, covering the file operations needed when a document is +// duplicated. +type FilesDBAgent interface { + // FetchDocumentFile should fetch the document file for the given block id. + FetchDocumentFile(ctx context.Context, id, organizationID string) (*file.File, error) + + // InsertDocumentFile should insert the document file. + InsertDocumentFile(ctx context.Context, f file.File) error +} + +// Storer is an interface that defines the object storage operations needed +// when a document is duplicated. +// +//go:generate ../../../../scripts/codegen/mock -t internal Storer +type Storer interface { + // Copy should copy an object within the storage. + Copy(ctx context.Context, srcFolder, srcID, dstFolder, dstID string) error +} + // SearchGateway is an interface that handles communication with the search engine. // //go:generate ../../../../scripts/codegen/mock -t internal SearchGateway diff --git a/server/core/internal/server/internal/document/http_test.go b/server/core/internal/server/internal/document/http_test.go index c112858a..02bdc29c 100644 --- a/server/core/internal/server/internal/document/http_test.go +++ b/server/core/internal/server/internal/document/http_test.go @@ -65,6 +65,7 @@ func newTestHandler(db DB, pub *fakePublisher) (*Handler, *callbackCounts) { log: slog.New(slog.DiscardHandler), db: db, notifPub: pub, + storer: &StorerMock{}, } hdl.tree.changeCallback = func(string, null.Value[xid.ID]) { cnt.tree++ } @@ -152,13 +153,15 @@ func Test_NewHandler(t *testing.T) { db := &DBMock{} gw := &SearchGatewayMock{} pub := &fakePublisher{} + st := &StorerMock{} - hdl := NewHandler(slog.New(slog.DiscardHandler), db, nil, nil, gw, pub) + hdl := NewHandler(slog.New(slog.DiscardHandler), db, nil, nil, gw, pub, st) require.NotNil(t, hdl) assert.NotNil(t, hdl.log) assert.Same(t, db, hdl.db) assert.Same(t, gw, hdl.searchGateway) assert.Same(t, pub, hdl.notifPub) + assert.Same(t, st, hdl.storer) assert.Nil(t, hdl.githubMan) assert.Nil(t, hdl.webchangeClient) } diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index db760554..6254f4dd 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -21,9 +21,6 @@ const ( // _organizationLogoLocation is the URL path for organization logos. _organizationLogoLocation = "/api/organizations/logo" - // _documentFileLocationFormat is the URL format for document files. - _documentFileLocationFormat = "/api/documents/%s/files/%s" - // _requestTimeout bounds the processing of a single HTTP request. _requestTimeout = 30 * time.Second diff --git a/server/core/internal/server/server.go b/server/core/internal/server/server.go index 0e8b612e..21475411 100644 --- a/server/core/internal/server/server.go +++ b/server/core/internal/server/server.go @@ -18,6 +18,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/apps/webchange" assistantCore "github.com/oxynote/oxynote/server/core/internal/assistant" "github.com/oxynote/oxynote/server/core/internal/buildinfo" + documentCore "github.com/oxynote/oxynote/server/core/internal/document" notificationCore "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/server/internal/assistant" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" @@ -134,9 +135,9 @@ func NewServer( srv.handlers.user = user.NewHandler(log, db, storageClient, opts.PublicURL+_userImageLocationFormat) srv.handlers.organization = org.NewHandler(log, db, storageClient, opts.PublicURL+_organizationLogoLocation, opts.DemoPrometheusURL) - srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, searchGateway, notifier) + srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, searchGateway, notifier, storageClient) srv.handlers.comment = comment.NewHandler(log, db, notifier) - srv.handlers.files = files.NewHandler(log, db, storageClient, opts.PublicURL+_documentFileLocationFormat) + srv.handlers.files = files.NewHandler(log, db, storageClient, opts.PublicURL+documentCore.FilePathFormat) srv.handlers.hook = hook.NewHandler(log, db, githubMan, webchangeClient) // The assistant's CRUD tools mutate the document tree directly @@ -312,6 +313,7 @@ type Storer interface { org.Storer user.Storer files.Storer + document.Storer } // Notifier is an interface that combines notificationCore.Notifier and diff --git a/server/core/internal/storage/client_test.go b/server/core/internal/storage/client_test.go index 9b60947e..d5a8c3dc 100644 --- a/server/core/internal/storage/client_test.go +++ b/server/core/internal/storage/client_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "strconv" "strings" "sync" @@ -192,6 +193,9 @@ type fakeS3 struct { // failDelete forces object removal to fail. failDelete bool + + // failCopy forces server-side object copies to fail. + failCopy bool } // ServeHTTP dispatches the S3 REST requests issued by the minio client. @@ -199,9 +203,17 @@ func (f *fakeS3) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.mu.Lock() defer f.mu.Unlock() + // the maps are initialized independently: a fixture that seeds only + // objects still needs the other two. if f.objects == nil { f.objects = make(map[string][]byte) + } + + if f.contentTypes == nil { f.contentTypes = make(map[string]string) + } + + if f.parts == nil { f.parts = make(map[string][]byte) } @@ -224,6 +236,8 @@ func (f *fakeS3) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.handleMakeBucket(w) case r.Method == http.MethodPost && query.Has("uploads"): f.handleInitiateUpload(w, r, key) + case r.Method == http.MethodPut && r.Header.Get("X-Amz-Copy-Source") != "": + f.handleCopy(w, r, key) case r.Method == http.MethodPut && query.Get("uploadId") != "": f.handleUploadPart(w, r, key) case r.Method == http.MethodPost && query.Get("uploadId") != "": @@ -366,6 +380,41 @@ func (f *fakeS3) handleDelete(w http.ResponseWriter, key string) { w.WriteHeader(http.StatusNoContent) } +// handleCopy serves server-side object copies, which arrive as a PUT +// naming their source in the x-amz-copy-source header. +func (f *fakeS3) handleCopy(w http.ResponseWriter, r *http.Request, key string) { + if f.failCopy { + writeS3Error(w, http.StatusForbidden, "AccessDenied") + + return + } + + src, err := url.PathUnescape(strings.TrimPrefix(r.Header.Get("X-Amz-Copy-Source"), "/")) + if err != nil { + writeS3Error(w, http.StatusBadRequest, "InvalidArgument") + + return + } + + // the header names the source as "/". + _, srcKey, _ := strings.Cut(src, "/") + + data, ok := f.objects[srcKey] + if !ok { + writeS3Error(w, http.StatusNotFound, "NoSuchKey") + + return + } + + f.objects[key] = data + f.contentTypes[key] = f.contentTypes[srcKey] + + fmt.Fprint( //nolint:errcheck // test server response errors are irrelevant + w, + `"test-etag"2006-01-02T15:04:05.000Z`, + ) +} + // writeS3ObjectHeaders writes the object metadata headers the minio // client requires to build stat results. func writeS3ObjectHeaders(w http.ResponseWriter, data []byte, contentType string) { diff --git a/server/core/internal/storage/object.go b/server/core/internal/storage/object.go index 4d619e4e..3790f581 100644 --- a/server/core/internal/storage/object.go +++ b/server/core/internal/storage/object.go @@ -121,7 +121,31 @@ func (c *Client) Retrieve(ctx context.Context, folder, id string) (*ObjectInfo, }, true, nil } -// Delete deletes an object by its ID. +// Copy copies an object within the bucket, server-side. The object is never +// streamed through this process: uploads are capped well below the multipart +// threshold, so a copy stays a metadata-only operation. +func (c *Client) Copy(ctx context.Context, srcFolder, srcID, dstFolder, dstID string) error { + _, err := c.client.CopyObject( + ctx, + minio.CopyDestOptions{ + Bucket: c.bucket, + Object: path.Join(dstFolder, dstID), + }, + minio.CopySrcOptions{ + Bucket: c.bucket, + Object: path.Join(srcFolder, srcID), + }, + ) + if err != nil { + return fmt.Errorf("copying object: %w", err) + } + + return nil +} + +// Delete deletes an object by its ID. Deleting an object that is not there +// is not an error, which is what lets a crashed upload heal: the row that +// outlived it is removed on the next cleanup pass either way. func (c *Client) Delete(ctx context.Context, folder, id string) error { err := c.client.RemoveObject( ctx, diff --git a/server/core/internal/storage/object_test.go b/server/core/internal/storage/object_test.go index a4078829..2a8ab8a9 100644 --- a/server/core/internal/storage/object_test.go +++ b/server/core/internal/storage/object_test.go @@ -166,6 +166,48 @@ func Test_Client_Retrieve(t *testing.T) { } } +func Test_Client_Copy(t *testing.T) { + cc := map[string]struct { + Fake *fakeS3 + Err error + }{ + "Error returned by CopyObject": { + Fake: &fakeS3{ + objects: map[string][]byte{"folder/object-id": _testPNG}, + failCopy: true, + }, + Err: assert.AnError, + }, + "Missing source object": { + Fake: &fakeS3{objects: map[string][]byte{}}, + Err: assert.AnError, + }, + "Successful copy": { + Fake: &fakeS3{ + objects: map[string][]byte{"folder/object-id": _testPNG}, + }, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + client := prepClient(t, c.Fake) + + err := client.Copy(context.Background(), "folder", "object-id", "other-folder", "copy-id") + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, _testPNG, c.Fake.objects["other-folder/copy-id"]) + assert.Equal(t, _testPNG, c.Fake.objects["folder/object-id"]) + }) + } +} + func Test_Client_Delete(t *testing.T) { cc := map[string]struct { Fake *fakeS3 @@ -178,6 +220,11 @@ func Test_Client_Delete(t *testing.T) { }, Err: assert.AnError, }, + // a file row can outlive its object, so the sweep must be able to + // drop the row without the delete failing first. + "Object is already gone": { + Fake: &fakeS3{objects: map[string][]byte{}}, + }, "Successful deletion": { Fake: &fakeS3{ objects: map[string][]byte{"folder/object-id": _testPNG},