diff --git a/server/core/internal/db/document_branch.go b/server/core/internal/db/document_branch.go index 4857d2d6..31e80eb3 100644 --- a/server/core/internal/db/document_branch.go +++ b/server/core/internal/db/document_branch.go @@ -263,10 +263,10 @@ func (a *agent) selectBranchSummary(b sq.SelectBuilder) sq.SelectBuilder { } // RecordDocumentBranchHistoryEntry records the branch as it stands, with -// the hooks its content holds, and returns the id of the entry it wrote or -// matched. It locks the branch row before reading it, so two writers cannot -// race on the newest entry. Called inside the transaction that wrote the -// branch, it records what that transaction wrote. +// the hooks its content holds. It locks the branch row before reading it, +// so two writers cannot race on the newest entry. Called inside the +// transaction that wrote the branch, it records what that transaction +// wrote. // // Ordinary entries aggregate: an edit in the same 30-minute bucket as the // branch's newest ordinary entry updates that entry in place. A boundary @@ -279,10 +279,8 @@ func (a *agent) RecordDocumentBranchHistoryEntry( organizationID string, by null.String, boundary bool, -) (xid.ID, error) { - var id xid.ID - - if err := sqlutil.WrapTx(ctx, a.sql, func(tx *sqlx.Tx) error { +) error { + return sqlutil.WrapTx(ctx, a.sql, func(tx *sqlx.Tx) error { q, args := a.selectDocumentBranch(a.builder.Select()). Where(sq.Eq{ "db.id": branchID, @@ -310,33 +308,26 @@ func (a *agent) RecordDocumentBranchHistoryEntry( boundary, ) - id, err = a.insertDocumentBranchHistoryEntry(ctx, tx, entry) - - return err - }); err != nil { - return xid.ID{}, err - } - - return id, nil + return a.insertDocumentBranchHistoryEntry(ctx, tx, entry) + }) } // insertDocumentBranchHistoryEntry writes the entry, folds it into the -// branch's newest one, or skips it when it matches the newest one. It -// returns the id of the row that holds the entry. The caller holds the -// branch row lock. -func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.Tx, entry history.Entry) (xid.ID, error) { +// branch's newest one, or skips it when it matches the newest one. The +// caller holds the branch row lock. +func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.Tx, entry history.Entry) error { checksum, err := entry.Sum() if err != nil { - return xid.ID{}, err + return err } newest, err := a.fetchNewestDocumentBranchHistoryEntry(ctx, tx, entry.BranchID) if err != nil { - return xid.ID{}, err + return err } if newest != nil && newest.Checksum == checksum && !entry.Boundary { - return newest.ID, nil + return nil } aggregates := newest != nil && @@ -365,11 +356,9 @@ func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.T Where(sq.Eq{"id": newest.ID}). MustSql() - if _, err := tx.ExecContext(ctx, q, args...); err != nil { - return xid.ID{}, err - } + _, err = tx.ExecContext(ctx, q, args...) - return newest.ID, nil + return err } q, args := a.builder.Insert("document_branch_history_entries"). @@ -390,49 +379,10 @@ func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.T MustSql() if _, err := tx.ExecContext(ctx, q, args...); err != nil { - return xid.ID{}, err - } - - if err := a.trimDocumentBranchHistoryEntries(ctx, tx, entry.BranchID); err != nil { - return xid.ID{}, err - } - - return entry.ID, nil -} - -// UpdateDocumentBranchHistoryEntryHooks replaces the hooks an entry lists -// and its checksum. A boundary entry is written before its branch's hooks -// are copied, so it gets the copied ones afterwards. -func (a *agent) UpdateDocumentBranchHistoryEntryHooks(ctx context.Context, id xid.ID, hooks history.Hooks) error { - return sqlutil.WrapTx(ctx, a.sql, func(tx *sqlx.Tx) error { - q, args := a.selectDocumentBranchHistoryEntry(a.builder.Select()). - Where(sq.Eq{"id": id}). - Suffix("FOR UPDATE"). - MustSql() - - var entry history.Entry - - if err := sqlx.GetContext(ctx, tx, &entry, q, args...); err != nil { - return err - } - - entry.Hooks = hooks - - checksum, err := entry.Sum() - if err != nil { - return err - } - - q, args = a.builder.Update("document_branch_history_entries"). - Set("hooks", hooks). - Set("checksum", checksum). - Where(sq.Eq{"id": id}). - MustSql() - - _, err = tx.ExecContext(ctx, q, args...) - return err - }) + } + + return a.trimDocumentBranchHistoryEntries(ctx, tx, entry.BranchID) } // fetchDocumentBranchHistoryHooks fetches every hook of the branch a diff --git a/server/core/internal/db/document_branch_test.go b/server/core/internal/db/document_branch_test.go index 71944781..e66685cc 100644 --- a/server/core/internal/db/document_branch_test.go +++ b/server/core/internal/db/document_branch_test.go @@ -689,9 +689,7 @@ func insertHistoryEntry(t *testing.T, db *DB, entry history.Entry) { t.Helper() require.NoError(t, sqlutil.WrapTx(context.Background(), db.sql, func(tx *sqlx.Tx) error { - _, err := db.insertDocumentBranchHistoryEntry(context.Background(), tx, entry) - - return err + return db.insertDocumentBranchHistoryEntry(context.Background(), tx, entry) })) } @@ -1017,14 +1015,8 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { db := prepTempDB(t) c := cfn(t, db) - var id xid.ID - err := sqlutil.WrapTx(context.Background(), db.sql, func(tx *sqlx.Tx) error { - var err error - - id, err = db.insertDocumentBranchHistoryEntry(context.Background(), tx, c.Entry) - - return err + return db.insertDocumentBranchHistoryEntry(context.Background(), tx, c.Entry) }) testutil.RequireEqualError(t, c.Err, err) @@ -1038,10 +1030,6 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { require.NoError(t, err) require.NotNil(t, newest) assertHistoryEntryEqual(t, c.Newest, *newest) - - // the id returned is the row that holds the entry, which is - // the newest. - assert.Equal(t, newest.ID, id) }) } } @@ -1183,7 +1171,7 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { before := timeutil.Now().Truncate(time.Microsecond) - id, err := record(context.Background(), c.BranchID, c.OrganizationID, c.By, c.Boundary) + err := record(context.Background(), c.BranchID, c.OrganizationID, c.By, c.Boundary) testutil.RequireEqualError(t, c.Err, err) if err != nil { @@ -1199,7 +1187,6 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { newest, err := db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, c.BranchID) require.NoError(t, err) require.NotNil(t, newest) - assert.Equal(t, newest.ID, id) assert.False(t, newest.CreatedAt.Before(before)) exp := c.Newest @@ -1211,30 +1198,6 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { } } -func Test_agent_UpdateDocumentBranchHistoryEntryHooks(t *testing.T) { - t.Parallel() - - db := prepTempDB(t) - doc := prepDocuments(t, db, 1, nil)[0] - entry := prepHistoryEntries(t, db, doc, 1)[0] - - entry.Hooks = history.Hooks{ - { - Type: hook.TypeURLWatcher, - BlockID: null.StringFrom("b1"), - Settings: processor.Settings(`{"url":"https://example.com"}`), - }, - } - - require.NoError(t, db.UpdateDocumentBranchHistoryEntryHooks(context.Background(), entry.ID, entry.Hooks)) - - // the stored checksum follows the hooks, which the comparison checks. - newest, err := db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, doc.BranchID) - require.NoError(t, err) - require.NotNil(t, newest) - assertHistoryEntryEqual(t, entry, *newest) -} - func Test_agent_fetchNewestDocumentBranchHistoryEntry(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/AGENTS.md b/server/core/internal/document/AGENTS.md index 0d4a18af..a2e3d2c6 100644 --- a/server/core/internal/document/AGENTS.md +++ b/server/core/internal/document/AGENTS.md @@ -33,8 +33,7 @@ reads it and its hooks, in the transaction that wrote the change. - Hook create, update and delete flush the branch, then record an entry in their transaction. Reset does not. - Create, duplicate, fork and merge write a **boundary** entry that never - aggregates and closes its bucket. It starts without hooks and gets the - ones `copyHooksToBranch` created after the commit. + aggregates, written after `CopyHooks` so it lists the copies. - Entries pin the files they reference, so the `DB_*HISTORY*` limits decide how long a removed image survives. Retention keeps each branch's newest entry. @@ -68,10 +67,11 @@ request handler: `beforeDeleteOrganization` calls `POST /api/x/organizations/{id}/teardown` while rows still exist and throws if core fails. - Create writes the row before the object; delete removes the object before - the row. `copyHooksToBranch` runs after the fork/merge/duplicate commits, - since a copy creates its watcher; a failed insert tears it down. -- **A null hook state means not set up**, as for a copy that cannot check - its target yet. The sweep sets it up. + the row. +- **A null hook state means not set up.** Copied hooks start null, never + with the source's state; the operation sets them up after commit and + the sweep retries what fails. Hook writes and the sweep share the + manager's per-hook lock. ## Search diff --git a/server/core/internal/document/hook/hook.go b/server/core/internal/document/hook/hook.go index 94a7b5b4..4beec7ec 100644 --- a/server/core/internal/document/hook/hook.go +++ b/server/core/internal/document/hook/hook.go @@ -172,19 +172,14 @@ func NewHook( return &h, nil } -// CopyTo re-creates the hook on another branch with fresh state, anchored -// to the given block. A copy within the hook's document keeps its -// cross-branch ID; a copy into another document is a hook of its own. A -// copy that cannot check its target yet is kept without state, and a -// later run sets it up. -func (h *Hook) CopyTo( - ctx context.Context, - documentID xid.ID, - branchID xid.ID, - blockID null.String, - organizationID string, - inp *Input, -) (*Hook, error) { +// NewCopy returns a copy of the hook for another branch, anchored to the +// given block. It is initializing, with a null state, until its first run +// sets it up, since the source's state can name the source's own watcher. +// The score is copied, so that run does not announce a change the source +// already had. +// A copy within the hook's document keeps its cross-branch ID; a copy into +// another document is a hook of its own. +func (h *Hook) NewCopy(documentID, branchID xid.ID, blockID null.String) Hook { id := xid.New() cp := Hook{ @@ -192,10 +187,12 @@ func (h *Hook) CopyTo( CrossBranchID: id, Type: h.Type, DocumentID: null.ValueFrom(documentID), - OrganizationID: null.StringFrom(organizationID), + OrganizationID: h.OrganizationID, BranchID: null.ValueFrom(branchID), BlockID: blockID, Settings: h.Settings, + Status: processor.StatusInitializing, + Score: h.Score, CreatedAt: timeutil.Now(), } @@ -203,11 +200,7 @@ func (h *Hook) CopyTo( cp.CrossBranchID = h.CrossBranchID } - if err := cp.Reset(ctx, inp); err != nil { - return nil, err - } - - return &cp, nil + return cp } // ApplyUpdate applies the new settings and resets the hook. It fails diff --git a/server/core/internal/document/hook/hook_test.go b/server/core/internal/document/hook/hook_test.go index 06595a20..118f1ff0 100644 --- a/server/core/internal/document/hook/hook_test.go +++ b/server/core/internal/document/hook/hook_test.go @@ -127,56 +127,34 @@ func Test_NewHook(t *testing.T) { }) } -func Test_Hook_CopyTo(t *testing.T) { +func Test_Hook_NewCopy(t *testing.T) { t.Parallel() - documentID := xid.New() - - src, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - BlockID: null.StringFrom("block-1"), - Settings: reminderSettings(t, time.Now().Add(time.Hour)), - }, documentID, xid.New(), "org-1", nil) - require.NoError(t, err) + src := Hook{ + ID: xid.New(), + CrossBranchID: xid.New(), + Type: TypeURLWatcher, + DocumentID: null.ValueFrom(xid.New()), + OrganizationID: null.StringFrom("org-1"), + BranchID: null.ValueFrom(xid.New()), + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + State: null.ValueFrom(processor.State(`{"watcherId":"w1"}`)), + Status: processor.StatusUnreachableURL, + Score: decimal.NewFromInt(10), + } cc := map[string]struct { - Hook Hook DocumentID xid.ID - Input *Input KeepsCrossBranch bool - Unset bool - Err error }{ - "Malformed settings fail": { - Hook: Hook{ - Type: TypeScheduledReminder, - Settings: processor.Settings(`{not json`), - }, - DocumentID: documentID, - Err: assert.AnError, - }, "Copy within the document keeps the cross-branch ID": { - Hook: *src, - DocumentID: documentID, + DocumentID: src.DocumentID.V, KeepsCrossBranch: true, }, "Copy into another document is a hook of its own": { - Hook: *src, DocumentID: xid.New(), }, - "Copy that cannot check its target is kept without state": { - Hook: Hook{ - ID: xid.New(), - CrossBranchID: xid.New(), - Type: TypeURLWatcher, - DocumentID: null.ValueFrom(documentID), - Settings: processor.Settings(`{"url":"https://example.com"}`), - }, - DocumentID: documentID, - Input: NewInput("org-2", nil, webchange.NewClient("", "")), - KeepsCrossBranch: true, - Unset: true, - }, } for cn, c := range cc { @@ -185,35 +163,25 @@ func Test_Hook_CopyTo(t *testing.T) { branchID := xid.New() - cp, err := c.Hook.CopyTo( - context.Background(), - c.DocumentID, - branchID, - null.StringFrom("block-2"), - "org-2", - c.Input, - ) - testutil.AssertEqualError(t, c.Err, err) + cp := src.NewCopy(c.DocumentID, branchID, null.StringFrom("b2")) - if err != nil { - return - } - - assert.NotEqual(t, c.Hook.ID, cp.ID) - assert.Equal(t, c.Hook.Type, cp.Type) - assert.Equal(t, c.Hook.Settings, cp.Settings) + assert.NotEqual(t, src.ID, cp.ID) + assert.Equal(t, src.Type, cp.Type) assert.Equal(t, null.ValueFrom(c.DocumentID), cp.DocumentID) + assert.Equal(t, src.OrganizationID, cp.OrganizationID) assert.Equal(t, null.ValueFrom(branchID), cp.BranchID) - assert.Equal(t, null.StringFrom("org-2"), cp.OrganizationID) - assert.Equal(t, null.StringFrom("block-2"), cp.BlockID) - assert.Equal(t, !c.Unset, cp.State.Valid) + assert.Equal(t, null.StringFrom("b2"), cp.BlockID) + assert.Equal(t, src.Settings, cp.Settings) + assert.Equal(t, processor.StatusInitializing, cp.Status) + assert.Equal(t, "10", cp.Score.String()) + assert.False(t, cp.CreatedAt.IsZero()) - if c.Unset { - assert.Equal(t, processor.StatusUnconfigured, cp.Status) - } + // the source's state names its own watcher, which the copy must + // never reach. + assert.False(t, cp.State.Valid) if c.KeepsCrossBranch { - assert.Equal(t, c.Hook.CrossBranchID, cp.CrossBranchID) + assert.Equal(t, src.CrossBranchID, cp.CrossBranchID) return } diff --git a/server/core/internal/document/hook/manager/0mock_copy_tx_test.go b/server/core/internal/document/hook/manager/0mock_copy_tx_test.go new file mode 100644 index 00000000..514f4e6b --- /dev/null +++ b/server/core/internal/document/hook/manager/0mock_copy_tx_test.go @@ -0,0 +1,147 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package manager + +import ( + "context" + "sync" + + "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/rs/xid" +) + +// Ensure, that CopyTxMock does implement CopyTx. +// If this is not the case, regenerate this file with moq. +var _ CopyTx = &CopyTxMock{} + +// CopyTxMock is a mock implementation of CopyTx. +// +// func TestSomethingThatUsesCopyTx(t *testing.T) { +// +// // make and configure a mocked CopyTx +// mockedCopyTx := &CopyTxMock{ +// FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { +// panic("mock out the FetchDocumentHooksByBranchID method") +// }, +// InsertDocumentHookFunc: func(ctx context.Context, hk hook.Hook) error { +// panic("mock out the InsertDocumentHook method") +// }, +// } +// +// // use mockedCopyTx in code that requires CopyTx +// // and then make assertions. +// +// } +type CopyTxMock struct { + // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. + FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) + + // InsertDocumentHookFunc mocks the InsertDocumentHook method. + InsertDocumentHookFunc func(ctx context.Context, hk hook.Hook) error + + // calls tracks calls to the methods. + calls struct { + // FetchDocumentHooksByBranchID holds details about calls to the FetchDocumentHooksByBranchID method. + FetchDocumentHooksByBranchID []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // BranchID is the branchID argument value. + BranchID xid.ID + // OrganizationID is the organizationID argument value. + OrganizationID string + } + // InsertDocumentHook holds details about calls to the InsertDocumentHook method. + InsertDocumentHook []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Hk is the hk argument value. + Hk hook.Hook + } + } + lockFetchDocumentHooksByBranchID sync.RWMutex + lockInsertDocumentHook sync.RWMutex +} + +// FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. +func (mock *CopyTxMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { + callInfo := struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + }{ + Ctx: ctx, + BranchID: branchID, + OrganizationID: organizationID, + } + mock.lockFetchDocumentHooksByBranchID.Lock() + mock.calls.FetchDocumentHooksByBranchID = append(mock.calls.FetchDocumentHooksByBranchID, callInfo) + mock.lockFetchDocumentHooksByBranchID.Unlock() + if mock.FetchDocumentHooksByBranchIDFunc == nil { + var ( + hooksOut []hook.Hook + errOut error + ) + return hooksOut, errOut + } + return mock.FetchDocumentHooksByBranchIDFunc(ctx, branchID, organizationID) +} + +// FetchDocumentHooksByBranchIDCalls gets all the calls that were made to FetchDocumentHooksByBranchID. +// Check the length with: +// +// len(mockedCopyTx.FetchDocumentHooksByBranchIDCalls()) +func (mock *CopyTxMock) FetchDocumentHooksByBranchIDCalls() []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string +} { + var calls []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + } + mock.lockFetchDocumentHooksByBranchID.RLock() + calls = mock.calls.FetchDocumentHooksByBranchID + mock.lockFetchDocumentHooksByBranchID.RUnlock() + return calls +} + +// InsertDocumentHook calls InsertDocumentHookFunc. +func (mock *CopyTxMock) InsertDocumentHook(ctx context.Context, hk hook.Hook) error { + callInfo := struct { + Ctx context.Context + Hk hook.Hook + }{ + Ctx: ctx, + Hk: hk, + } + mock.lockInsertDocumentHook.Lock() + mock.calls.InsertDocumentHook = append(mock.calls.InsertDocumentHook, callInfo) + mock.lockInsertDocumentHook.Unlock() + if mock.InsertDocumentHookFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.InsertDocumentHookFunc(ctx, hk) +} + +// InsertDocumentHookCalls gets all the calls that were made to InsertDocumentHook. +// Check the length with: +// +// len(mockedCopyTx.InsertDocumentHookCalls()) +func (mock *CopyTxMock) InsertDocumentHookCalls() []struct { + Ctx context.Context + Hk hook.Hook +} { + var calls []struct { + Ctx context.Context + Hk hook.Hook + } + mock.lockInsertDocumentHook.RLock() + calls = mock.calls.InsertDocumentHook + mock.lockInsertDocumentHook.RUnlock() + return calls +} diff --git a/server/core/internal/document/hook/manager/0mock_db_test.go b/server/core/internal/document/hook/manager/0mock_db_test.go index 210e336a..53977037 100644 --- a/server/core/internal/document/hook/manager/0mock_db_test.go +++ b/server/core/internal/document/hook/manager/0mock_db_test.go @@ -34,6 +34,9 @@ var _ DB = &DBMock{} // FetchDocumentHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) { // panic("mock out the FetchDocumentHook method") // }, +// FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { +// panic("mock out the FetchDocumentHooksByBranchID method") +// }, // FetchDocumentMaintainersFunc: func(ctx context.Context, documentID xid.ID, organizationID string) ([]string, error) { // panic("mock out the FetchDocumentMaintainers method") // }, @@ -62,6 +65,9 @@ type DBMock struct { // FetchDocumentHookFunc mocks the FetchDocumentHook method. FetchDocumentHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) + // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. + FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) + // FetchDocumentMaintainersFunc mocks the FetchDocumentMaintainers method. FetchDocumentMaintainersFunc func(ctx context.Context, documentID xid.ID, organizationID string) ([]string, error) @@ -105,6 +111,15 @@ type DBMock struct { // 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. + Ctx context.Context + // BranchID is the branchID argument value. + BranchID xid.ID + // OrganizationID is the organizationID argument value. + OrganizationID string + } // FetchDocumentMaintainers holds details about calls to the FetchDocumentMaintainers method. FetchDocumentMaintainers []struct { // Ctx is the ctx argument value. @@ -131,13 +146,14 @@ type DBMock struct { Hk hook.Hook } } - lockBeginTx sync.RWMutex - lockDeleteDocumentHook sync.RWMutex - lockFetchDocumentByBranchID sync.RWMutex - lockFetchDocumentHook sync.RWMutex - lockFetchDocumentMaintainers sync.RWMutex - lockFetchPaginatedDocumentHooks sync.RWMutex - lockUpdateDocumentHook sync.RWMutex + lockBeginTx sync.RWMutex + lockDeleteDocumentHook sync.RWMutex + lockFetchDocumentByBranchID sync.RWMutex + lockFetchDocumentHook sync.RWMutex + lockFetchDocumentHooksByBranchID sync.RWMutex + lockFetchDocumentMaintainers sync.RWMutex + lockFetchPaginatedDocumentHooks sync.RWMutex + lockUpdateDocumentHook sync.RWMutex } // BeginTx calls BeginTxFunc. @@ -306,6 +322,50 @@ func (mock *DBMock) FetchDocumentHookCalls() []struct { return calls } +// FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. +func (mock *DBMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) { + callInfo := struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + }{ + Ctx: ctx, + BranchID: branchID, + OrganizationID: organizationID, + } + mock.lockFetchDocumentHooksByBranchID.Lock() + mock.calls.FetchDocumentHooksByBranchID = append(mock.calls.FetchDocumentHooksByBranchID, callInfo) + mock.lockFetchDocumentHooksByBranchID.Unlock() + if mock.FetchDocumentHooksByBranchIDFunc == nil { + var ( + hooksOut []hook.Hook + errOut error + ) + return hooksOut, errOut + } + return mock.FetchDocumentHooksByBranchIDFunc(ctx, branchID, organizationID) +} + +// FetchDocumentHooksByBranchIDCalls gets all the calls that were made to FetchDocumentHooksByBranchID. +// Check the length with: +// +// len(mockedDB.FetchDocumentHooksByBranchIDCalls()) +func (mock *DBMock) FetchDocumentHooksByBranchIDCalls() []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string +} { + var calls []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + } + mock.lockFetchDocumentHooksByBranchID.RLock() + calls = mock.calls.FetchDocumentHooksByBranchID + mock.lockFetchDocumentHooksByBranchID.RUnlock() + return calls +} + // FetchDocumentMaintainers calls FetchDocumentMaintainersFunc. func (mock *DBMock) FetchDocumentMaintainers(ctx context.Context, documentID xid.ID, organizationID string) ([]string, error) { callInfo := struct { diff --git a/server/core/internal/document/hook/manager/0mock_tx_test.go b/server/core/internal/document/hook/manager/0mock_tx_test.go index 5b97f3ea..c5374383 100644 --- a/server/core/internal/document/hook/manager/0mock_tx_test.go +++ b/server/core/internal/document/hook/manager/0mock_tx_test.go @@ -31,7 +31,7 @@ var _ Tx = &TxMock{} // InsertDocumentHookFunc: func(ctx context.Context, hk hook.Hook) error { // panic("mock out the InsertDocumentHook method") // }, -// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { // panic("mock out the RecordDocumentBranchHistoryEntry method") // }, // RollbackFunc: func() error { @@ -57,7 +57,7 @@ type TxMock struct { InsertDocumentHookFunc func(ctx context.Context, hk hook.Hook) error // RecordDocumentBranchHistoryEntryFunc mocks the RecordDocumentBranchHistoryEntry method. - RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) + RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error // RollbackFunc mocks the Rollback method. RollbackFunc func() error @@ -225,7 +225,7 @@ func (mock *TxMock) InsertDocumentHookCalls() []struct { } // RecordDocumentBranchHistoryEntry calls RecordDocumentBranchHistoryEntryFunc. -func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -244,10 +244,9 @@ func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branch mock.lockRecordDocumentBranchHistoryEntry.Unlock() if mock.RecordDocumentBranchHistoryEntryFunc == nil { var ( - iDOut xid.ID errOut error ) - return iDOut, errOut + return errOut } return mock.RecordDocumentBranchHistoryEntryFunc(ctx, branchID, organizationID, by, boundary) } diff --git a/server/core/internal/document/hook/manager/copy.go b/server/core/internal/document/hook/manager/copy.go new file mode 100644 index 00000000..3413b586 --- /dev/null +++ b/server/core/internal/document/hook/manager/copy.go @@ -0,0 +1,44 @@ +package manager + +import ( + "context" + + "github.com/guregu/null/v5" + "github.com/rs/xid" +) + +// CopyHooks copies the branch's hooks to another in the caller's +// transaction, not set up yet. newBlockIDs maps the source's block ids to +// the copy's, for a copy whose blocks were given new ids. When set, it +// re-anchors block hooks and drops those whose block it does not name. +func CopyHooks( + ctx context.Context, + tx CopyTx, + fromBranchID, toBranchID, documentID xid.ID, + organizationID string, + newBlockIDs map[string]string, +) error { + hooks, err := tx.FetchDocumentHooksByBranchID(ctx, fromBranchID, organizationID) + if err != nil { + return err + } + + for _, hk := range hooks { + blockID := hk.BlockID + + if newBlockIDs != nil && blockID.Valid { + newID, ok := newBlockIDs[blockID.String] + if !ok { + continue + } + + blockID = null.StringFrom(newID) + } + + if err := tx.InsertDocumentHook(ctx, hk.NewCopy(documentID, toBranchID, blockID)); err != nil { + return err + } + } + + return nil +} diff --git a/server/core/internal/document/hook/manager/copy_test.go b/server/core/internal/document/hook/manager/copy_test.go new file mode 100644 index 00000000..a0c923cc --- /dev/null +++ b/server/core/internal/document/hook/manager/copy_test.go @@ -0,0 +1,92 @@ +package manager + +import ( + "context" + "testing" + "time" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/pkg/testutil" + "github.com/rs/xid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_CopyHooks(t *testing.T) { + t.Parallel() + + fromBranchID, toBranchID, documentID := xid.New(), xid.New(), xid.New() + + cc := map[string]struct { + UIDs map[string]string + FetchErr error + InsertErr error + // Blocks are the block ids of the inserted copies, in order. + Blocks []null.String + Err error + }{ + "Error returned by tx.FetchDocumentHooksByBranchID": { + FetchErr: assert.AnError, + Err: assert.AnError, + }, + "Error returned by tx.InsertDocumentHook": { + InsertErr: assert.AnError, + Blocks: []null.String{null.StringFrom("old-uid")}, + Err: assert.AnError, + }, + "Fork or merge keeps block ids": { + Blocks: []null.String{null.StringFrom("old-uid"), {}, null.StringFrom("gone-uid")}, + }, + // a duplicated branch carries fresh block uids, so a hook anchored + // to a block follows the map, a document-level hook is copied as it + // is, and a hook whose block the map does not name is dropped. + "Duplicate re-anchors through the uid map": { + UIDs: map[string]string{"old-uid": "new-uid"}, + Blocks: []null.String{null.StringFrom("new-uid"), {}}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + block := stubHook(t, fromBranchID, time.Now().Add(time.Hour), time.Now()) + block.BlockID = null.StringFrom("old-uid") + + doc := stubHook(t, fromBranchID, time.Now().Add(time.Hour), time.Now()) + + gone := stubHook(t, fromBranchID, time.Now().Add(time.Hour), time.Now()) + gone.BlockID = null.StringFrom("gone-uid") + + tx := &CopyTxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hook.Hook, error) { + return []hook.Hook{block, doc, gone}, c.FetchErr + }, + InsertDocumentHookFunc: func(context.Context, hook.Hook) error { + return c.InsertErr + }, + } + + err := CopyHooks(context.Background(), tx, fromBranchID, toBranchID, documentID, "org-1", c.UIDs) + testutil.AssertEqualError(t, c.Err, err) + + if c.FetchErr == nil { + ff := tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, ff, 1) + assert.Equal(t, fromBranchID, ff[0].BranchID) + assert.Equal(t, "org-1", ff[0].OrganizationID) + } + + ii := tx.InsertDocumentHookCalls() + require.Len(t, ii, len(c.Blocks)) + + for i, call := range ii { + assert.Equal(t, c.Blocks[i], call.Hk.BlockID) + assert.Equal(t, null.ValueFrom(toBranchID), call.Hk.BranchID) + assert.Equal(t, null.ValueFrom(documentID), call.Hk.DocumentID) + assert.False(t, call.Hk.State.Valid) + } + }) + } +} diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index 960ef886..bc1727c1 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -82,7 +82,8 @@ func (m *Manager) BindHookChange(fn func(hook.Hook)) { m.changeCallback = fn } -// Start begins the periodic processing of document hooks. +// Start begins the periodic processing of document hooks. A pass also sets +// up the hooks a branch operation could not. func (m *Manager) Start(ctx context.Context) { m.log.Info("starting") defer m.log.Info("stopped") @@ -101,6 +102,30 @@ func (m *Manager) Start(ctx context.Context) { ).Start(ctx) } +// SetUpBranch sets up the branch's hooks that are not set up yet, such as +// the copies a branch operation just stored. A hook it cannot set up waits +// for the next pass. +func (m *Manager) SetUpBranch(ctx context.Context, branchID xid.ID, organizationID string) { + hooks, err := m.db.FetchDocumentHooksByBranchID(ctx, branchID, organizationID) + if err != nil { + m.log.With("branch_id", branchID). + With("error", err). + Error("fetching branch hooks") + + return + } + + ps := &ProcessingState{ + Documents: make(map[xid.ID]*document.Document), + } + + for _, h := range hooks { + if !h.State.Valid { + m.processHook(ctx, ps, h) + } + } +} + // ProcessingState holds the state during hook processing. type ProcessingState struct { // OffsetID is the last processed hook ID. @@ -370,6 +395,9 @@ type DB interface { // FetchDocumentHook should fetch the hook of the organization. FetchDocumentHook(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) + // FetchDocumentHooksByBranchID should fetch the hooks of a branch. + FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) + // UpdateDocumentHook should update the given document hook in the database. UpdateDocumentHook(ctx context.Context, hk hook.Hook) error @@ -400,14 +428,25 @@ type Tx interface { DeleteDocumentHook(ctx context.Context, id xid.ID) error // RecordDocumentBranchHistoryEntry should record the branch as it - // stands, with its hooks, and return the id of the entry. + // stands, with its hooks. RecordDocumentBranchHistoryEntry( ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool, - ) (xid.ID, error) + ) error +} + +// CopyTx is the part of a caller's transaction CopyHooks runs in. +// +//go:generate ../../../../scripts/codegen/mock -t internal CopyTx copy_tx +type CopyTx interface { + // FetchDocumentHooksByBranchID should fetch the hooks of a branch. + FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hook.Hook, error) + + // InsertDocumentHook should insert the document hook. + InsertDocumentHook(ctx context.Context, hk hook.Hook) error } // Flusher stores what the editors of a branch hold. diff --git a/server/core/internal/document/hook/manager/manager_test.go b/server/core/internal/document/hook/manager/manager_test.go index 0b470d44..80f6f012 100644 --- a/server/core/internal/document/hook/manager/manager_test.go +++ b/server/core/internal/document/hook/manager/manager_test.go @@ -225,6 +225,66 @@ func Test_Manager_Start(t *testing.T) { assert.Len(t, db.FetchPaginatedDocumentHooksCalls(), 1) } +func Test_Manager_SetUpBranch(t *testing.T) { + t.Parallel() + + branchID := xid.New() + + cc := map[string]struct { + FetchErr error + // Updated is the hooks the set up stores, by their place in the + // branch. + Updated []int + }{ + "Error returned by db.FetchDocumentHooksByBranchID": { + FetchErr: assert.AnError, + }, + "Only the hooks not set up yet are set up": { + Updated: []int{1}, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + set := stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + + unset := stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + unset.State = null.Value[processor.State]{} + + hooks := []hook.Hook{set, unset} + + db := stubStoredHooks(&DBMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hook.Hook, error) { + return hooks, c.FetchErr + }, + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*document.Document, error) { + return stubDocument(), nil + }, + UpdateDocumentHookFunc: func(context.Context, hook.Hook) error { + return nil + }, + }, hooks) + + newTestManager(t, db, &fakePublisher{}, nil).SetUpBranch(context.Background(), branchID, "org-1") + + ff := db.FetchDocumentHooksByBranchIDCalls() + require.Len(t, ff, 1) + assert.Equal(t, branchID, ff[0].BranchID) + assert.Equal(t, "org-1", ff[0].OrganizationID) + + uu := db.UpdateDocumentHookCalls() + require.Len(t, uu, len(c.Updated)) + + for i, idx := range c.Updated { + assert.Equal(t, hooks[idx].ID, uu[i].Hk.ID) + assert.True(t, uu[i].Hk.State.Valid) + } + }) + } +} + func Test_Manager_processHooks(t *testing.T) { t.Parallel() @@ -448,6 +508,7 @@ func Test_Manager_processHooks(t *testing.T) { Hooks: func(_ *testing.T) []hook.Hook { h := urlWatcherHook(branchID) h.State = null.Value[processor.State]{} + h.Status = processor.StatusInitializing return []hook.Hook{h} }, @@ -456,6 +517,7 @@ func Test_Manager_processHooks(t *testing.T) { Checks: checks( hasError(false), wasUpdateCalled(1), + hasUpdatedStatus(processor.StatusActive), hasUpdated(func(t *testing.T, h hook.Hook) { require.True(t, h.State.Valid) assert.JSONEq(t, `{"watcherId":"w-new","lastChangedAt":null}`, string(h.State.V)) @@ -463,6 +525,27 @@ func Test_Manager_processHooks(t *testing.T) { wasPublished(), ), }, + // the copy's source may well check its target, so the copy's own + // failure is shown on it but not announced. + "Copy that cannot be set up takes the failure without a notification": { + Hooks: func(_ *testing.T) []hook.Hook { + h := urlWatcherHook(branchID) + h.State = null.Value[processor.State]{} + h.Status = processor.StatusInitializing + + return []hook.Hook{h} + }, + Doc: stubDocument(), + Checks: checks( + hasError(false), + wasUpdateCalled(1), + hasUpdatedStatus(processor.StatusUnconfigured), + hasUpdated(func(t *testing.T, h hook.Hook) { + assert.False(t, h.State.Valid) + }), + wasPublished(), + ), + }, "Failed update of a setup tears down the new watcher": { Hooks: func(_ *testing.T) []hook.Hook { h := urlWatcherHook(branchID) diff --git a/server/core/internal/document/hook/manager/write.go b/server/core/internal/document/hook/manager/write.go index b2d0eb06..d9ac14ee 100644 --- a/server/core/internal/document/hook/manager/write.go +++ b/server/core/internal/document/hook/manager/write.go @@ -80,7 +80,7 @@ func (m *Manager) CreateHook( return nil, err } - if _, err = tx.RecordDocumentBranchHistoryEntry(ctx, ci.BranchID, organizationID, null.StringFrom(updatedBy), false); err != nil { + if err = tx.RecordDocumentBranchHistoryEntry(ctx, ci.BranchID, organizationID, null.StringFrom(updatedBy), false); err != nil { return nil, err } @@ -160,7 +160,7 @@ func (m *Manager) UpdateHook( } if hk.BranchID.Valid { - if _, err = tx.RecordDocumentBranchHistoryEntry(ctx, hk.BranchID.V, organizationID, null.StringFrom(updatedBy), false); err != nil { + if err = tx.RecordDocumentBranchHistoryEntry(ctx, hk.BranchID.V, organizationID, null.StringFrom(updatedBy), false); err != nil { return nil, err } } @@ -224,7 +224,7 @@ func (m *Manager) DeleteHook(ctx context.Context, id xid.ID, organizationID, upd } if hk.BranchID.Valid { - if _, err = tx.RecordDocumentBranchHistoryEntry(ctx, hk.BranchID.V, organizationID, null.StringFrom(updatedBy), false); err != nil { + if err = tx.RecordDocumentBranchHistoryEntry(ctx, hk.BranchID.V, organizationID, null.StringFrom(updatedBy), false); err != nil { return err } } diff --git a/server/core/internal/document/hook/manager/write_test.go b/server/core/internal/document/hook/manager/write_test.go index 8f9e0d36..7c069e1a 100644 --- a/server/core/internal/document/hook/manager/write_test.go +++ b/server/core/internal/document/hook/manager/write_test.go @@ -134,8 +134,8 @@ func Test_Manager_CreateHook(t *testing.T) { Settings: processor.Settings(`{"url":"https://example.com"}`), CD: true, Tx: &TxMock{ - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.ID{}, assert.AnError + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return assert.AnError }, }, Inserts: 1, @@ -383,8 +383,8 @@ func Test_Manager_UpdateHook(t *testing.T) { UpdateDocumentHookFunc: func(context.Context, hook.Hook) error { return c.UpdateErr }, - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.New(), c.RecordErr + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return c.RecordErr }, CommitFunc: func() error { return c.CommitErr @@ -575,8 +575,8 @@ func Test_Manager_DeleteHook(t *testing.T) { DeleteDocumentHookFunc: func(context.Context, xid.ID) error { return c.DelErr }, - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.New(), c.RecordErr + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return c.RecordErr }, CommitFunc: func() error { return c.CommitErr diff --git a/server/core/internal/document/hook/processor/processor.go b/server/core/internal/document/hook/processor/processor.go index fa6b0cab..beef6f56 100644 --- a/server/core/internal/document/hook/processor/processor.go +++ b/server/core/internal/document/hook/processor/processor.go @@ -17,6 +17,10 @@ const ( // StatusActive indicates that the processor checked what it watches. StatusActive Status = "active" + // StatusInitializing indicates that the hook is a copy whose first run + // has not set it up yet. + StatusInitializing Status = "initializing" + // StatusUnconfigured indicates that the integration the processor needs // is not configured on this deployment. StatusUnconfigured Status = "unconfigured" diff --git a/server/core/internal/server/0mock_db_test.go b/server/core/internal/server/0mock_db_test.go index c8776c17..64a5837c 100644 --- a/server/core/internal/server/0mock_db_test.go +++ b/server/core/internal/server/0mock_db_test.go @@ -13,7 +13,6 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/comment" "github.com/oxynote/oxynote/server/core/internal/document/file" - "github.com/oxynote/oxynote/server/core/internal/document/history" hookCore "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" @@ -242,7 +241,7 @@ var _ DB = &DBMock{} // PromoteBranchApprovalsFunc: func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error { // panic("mock out the PromoteBranchApprovals method") // }, -// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { // panic("mock out the RecordDocumentBranchHistoryEntry method") // }, // ReplaceBranchTagsFunc: func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error { @@ -272,9 +271,6 @@ var _ DB = &DBMock{} // UpdateDocumentFunc: func(ctx context.Context, doc document.Document) error { // panic("mock out the UpdateDocument method") // }, -// UpdateDocumentBranchHistoryEntryHooksFunc: func(ctx context.Context, id xid.ID, hooks history.Hooks) error { -// panic("mock out the UpdateDocumentBranchHistoryEntryHooks method") -// }, // UpdateDocumentBranchMetadataFunc: func(ctx context.Context, doc document.Document) error { // panic("mock out the UpdateDocumentBranchMetadata method") // }, @@ -529,7 +525,7 @@ type DBMock struct { PromoteBranchApprovalsFunc func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error // RecordDocumentBranchHistoryEntryFunc mocks the RecordDocumentBranchHistoryEntry method. - RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) + RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error // ReplaceBranchTagsFunc mocks the ReplaceBranchTags method. ReplaceBranchTagsFunc func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error @@ -558,9 +554,6 @@ type DBMock struct { // UpdateDocumentFunc mocks the UpdateDocument method. UpdateDocumentFunc func(ctx context.Context, doc document.Document) error - // UpdateDocumentBranchHistoryEntryHooksFunc mocks the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooksFunc func(ctx context.Context, id xid.ID, hooks history.Hooks) error - // UpdateDocumentBranchMetadataFunc mocks the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadataFunc func(ctx context.Context, doc document.Document) error @@ -1293,15 +1286,6 @@ type DBMock struct { // Doc is the doc argument value. Doc document.Document } - // UpdateDocumentBranchHistoryEntryHooks holds details about calls to the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooks []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // ID is the id argument value. - ID xid.ID - // Hooks is the hooks argument value. - Hooks history.Hooks - } // UpdateDocumentBranchMetadata holds details about calls to the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadata []struct { // Ctx is the ctx argument value. @@ -1487,7 +1471,6 @@ type DBMock struct { lockUpdateBranchReviewer sync.RWMutex lockUpdateDataSource sync.RWMutex lockUpdateDocument sync.RWMutex - lockUpdateDocumentBranchHistoryEntryHooks sync.RWMutex lockUpdateDocumentBranchMetadata sync.RWMutex lockUpdateDocumentComment sync.RWMutex lockUpdateDocumentCommentReply sync.RWMutex @@ -4494,7 +4477,7 @@ func (mock *DBMock) PromoteBranchApprovalsCalls() []struct { } // RecordDocumentBranchHistoryEntry calls RecordDocumentBranchHistoryEntryFunc. -func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -4513,10 +4496,9 @@ func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branch mock.lockRecordDocumentBranchHistoryEntry.Unlock() if mock.RecordDocumentBranchHistoryEntryFunc == nil { var ( - iDOut xid.ID errOut error ) - return iDOut, errOut + return errOut } return mock.RecordDocumentBranchHistoryEntryFunc(ctx, branchID, organizationID, by, boundary) } @@ -4928,49 +4910,6 @@ func (mock *DBMock) UpdateDocumentCalls() []struct { return calls } -// UpdateDocumentBranchHistoryEntryHooks calls UpdateDocumentBranchHistoryEntryHooksFunc. -func (mock *DBMock) UpdateDocumentBranchHistoryEntryHooks(ctx context.Context, id xid.ID, hooks history.Hooks) error { - callInfo := struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - }{ - Ctx: ctx, - ID: id, - Hooks: hooks, - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.Lock() - mock.calls.UpdateDocumentBranchHistoryEntryHooks = append(mock.calls.UpdateDocumentBranchHistoryEntryHooks, callInfo) - mock.lockUpdateDocumentBranchHistoryEntryHooks.Unlock() - if mock.UpdateDocumentBranchHistoryEntryHooksFunc == nil { - var ( - errOut error - ) - return errOut - } - return mock.UpdateDocumentBranchHistoryEntryHooksFunc(ctx, id, hooks) -} - -// UpdateDocumentBranchHistoryEntryHooksCalls gets all the calls that were made to UpdateDocumentBranchHistoryEntryHooks. -// Check the length with: -// -// len(mockedDB.UpdateDocumentBranchHistoryEntryHooksCalls()) -func (mock *DBMock) UpdateDocumentBranchHistoryEntryHooksCalls() []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks -} { - var calls []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.RLock() - calls = mock.calls.UpdateDocumentBranchHistoryEntryHooks - mock.lockUpdateDocumentBranchHistoryEntryHooks.RUnlock() - return calls -} - // UpdateDocumentBranchMetadata calls UpdateDocumentBranchMetadataFunc. func (mock *DBMock) UpdateDocumentBranchMetadata(ctx context.Context, doc document.Document) error { callInfo := struct { diff --git a/server/core/internal/server/0mock_hook_manager_test.go b/server/core/internal/server/0mock_hook_manager_test.go index 0f8171d3..e55595b0 100644 --- a/server/core/internal/server/0mock_hook_manager_test.go +++ b/server/core/internal/server/0mock_hook_manager_test.go @@ -33,6 +33,9 @@ var _ HookManager = &HookManagerMock{} // ResetHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { // panic("mock out the ResetHook method") // }, +// SetUpBranchFunc: func(ctx context.Context, branchID xid.ID, organizationID string) { +// panic("mock out the SetUpBranch method") +// }, // UpdateHookFunc: func(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { // panic("mock out the UpdateHook method") // }, @@ -55,6 +58,9 @@ type HookManagerMock struct { // ResetHookFunc mocks the ResetHook method. ResetHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) + // SetUpBranchFunc mocks the SetUpBranch method. + SetUpBranchFunc func(ctx context.Context, branchID xid.ID, organizationID string) + // UpdateHookFunc mocks the UpdateHook method. UpdateHookFunc func(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) @@ -98,6 +104,15 @@ type HookManagerMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } + // SetUpBranch holds details about calls to the SetUpBranch method. + SetUpBranch []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // BranchID is the branchID argument value. + BranchID xid.ID + // OrganizationID is the organizationID argument value. + OrganizationID string + } // UpdateHook holds details about calls to the UpdateHook method. UpdateHook []struct { // Ctx is the ctx argument value. @@ -116,6 +131,7 @@ type HookManagerMock struct { lockCreateHook sync.RWMutex lockDeleteHook sync.RWMutex lockResetHook sync.RWMutex + lockSetUpBranch sync.RWMutex lockUpdateHook sync.RWMutex } @@ -294,6 +310,46 @@ func (mock *HookManagerMock) ResetHookCalls() []struct { return calls } +// SetUpBranch calls SetUpBranchFunc. +func (mock *HookManagerMock) SetUpBranch(ctx context.Context, branchID xid.ID, organizationID string) { + callInfo := struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + }{ + Ctx: ctx, + BranchID: branchID, + OrganizationID: organizationID, + } + mock.lockSetUpBranch.Lock() + mock.calls.SetUpBranch = append(mock.calls.SetUpBranch, callInfo) + mock.lockSetUpBranch.Unlock() + if mock.SetUpBranchFunc == nil { + return + } + mock.SetUpBranchFunc(ctx, branchID, organizationID) +} + +// SetUpBranchCalls gets all the calls that were made to SetUpBranch. +// Check the length with: +// +// len(mockedHookManager.SetUpBranchCalls()) +func (mock *HookManagerMock) SetUpBranchCalls() []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string +} { + var calls []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + } + mock.lockSetUpBranch.RLock() + calls = mock.calls.SetUpBranch + mock.lockSetUpBranch.RUnlock() + return calls +} + // UpdateHook calls UpdateHookFunc. func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, 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 f06a12e0..c78654df 100644 --- a/server/core/internal/server/internal/document/0mock_db_test.go +++ b/server/core/internal/server/internal/document/0mock_db_test.go @@ -10,7 +10,6 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" - "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -119,7 +118,7 @@ var _ DB = &DBMock{} // PromoteBranchApprovalsFunc: func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error { // panic("mock out the PromoteBranchApprovals method") // }, -// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { // panic("mock out the RecordDocumentBranchHistoryEntry method") // }, // ReplaceBranchTagsFunc: func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error { @@ -131,9 +130,6 @@ var _ DB = &DBMock{} // UpdateDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocument method") // }, -// UpdateDocumentBranchHistoryEntryHooksFunc: func(ctx context.Context, id xid.ID, hooks history.Hooks) error { -// panic("mock out the UpdateDocumentBranchHistoryEntryHooks method") -// }, // UpdateDocumentBranchMetadataFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocumentBranchMetadata method") // }, @@ -247,7 +243,7 @@ type DBMock struct { PromoteBranchApprovalsFunc func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error // RecordDocumentBranchHistoryEntryFunc mocks the RecordDocumentBranchHistoryEntry method. - RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) + RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error // ReplaceBranchTagsFunc mocks the ReplaceBranchTags method. ReplaceBranchTagsFunc func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error @@ -258,9 +254,6 @@ type DBMock struct { // UpdateDocumentFunc mocks the UpdateDocument method. UpdateDocumentFunc func(ctx context.Context, doc documentCore.Document) error - // UpdateDocumentBranchHistoryEntryHooksFunc mocks the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooksFunc func(ctx context.Context, id xid.ID, hooks history.Hooks) error - // UpdateDocumentBranchMetadataFunc mocks the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadataFunc func(ctx context.Context, doc documentCore.Document) error @@ -584,15 +577,6 @@ type DBMock struct { // Doc is the doc argument value. Doc documentCore.Document } - // UpdateDocumentBranchHistoryEntryHooks holds details about calls to the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooks []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // ID is the id argument value. - ID xid.ID - // Hooks is the hooks argument value. - Hooks history.Hooks - } // UpdateDocumentBranchMetadata holds details about calls to the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadata []struct { // Ctx is the ctx argument value. @@ -632,46 +616,45 @@ type DBMock struct { MaintainerIDs []string } } - lockBeginTx sync.RWMutex - lockCheckDocumentCycle sync.RWMutex - lockCheckDocumentExists sync.RWMutex - lockCheckOrganizationMember sync.RWMutex - lockCopyBranchTags sync.RWMutex - lockCountDocumentBranches sync.RWMutex - lockDeleteBranchReviewer sync.RWMutex - lockDeleteDocument sync.RWMutex - lockDeleteDocumentBranchByID sync.RWMutex - lockDeleteDocumentCommentsByBranchID sync.RWMutex - lockDetachDocumentHooksByBranchID sync.RWMutex - lockFetchBranchReviewer sync.RWMutex - lockFetchBranchReviewers sync.RWMutex - lockFetchDocument sync.RWMutex - lockFetchDocumentBranches sync.RWMutex - lockFetchDocumentBranchesUnsafe sync.RWMutex - lockFetchDocumentByBranchID sync.RWMutex - lockFetchDocumentFile sync.RWMutex - lockFetchDocumentHooksByBranchID sync.RWMutex - lockFetchDocumentHooksByDocumentID sync.RWMutex - lockFetchDocumentMaintainers sync.RWMutex - lockFetchDocumentTree sync.RWMutex - lockFetchDocumentTreeByDocumentParentID sync.RWMutex - lockFetchDocumentUnsafeByBranchID sync.RWMutex - lockInsertBranchReviewer sync.RWMutex - lockInsertDocument sync.RWMutex - lockInsertDocumentBranch sync.RWMutex - lockInsertDocumentFile sync.RWMutex - lockInsertDocumentHook sync.RWMutex - lockInsertSearchJob sync.RWMutex - lockPromoteBranchApprovals sync.RWMutex - lockRecordDocumentBranchHistoryEntry sync.RWMutex - lockReplaceBranchTags sync.RWMutex - lockUpdateBranchReviewer sync.RWMutex - lockUpdateDocument sync.RWMutex - lockUpdateDocumentBranchHistoryEntryHooks sync.RWMutex - lockUpdateDocumentBranchMetadata sync.RWMutex - lockUpdateDocumentParentID sync.RWMutex - lockUpdateDocumentTree sync.RWMutex - lockUpsertDocumentMaintainers sync.RWMutex + lockBeginTx sync.RWMutex + lockCheckDocumentCycle sync.RWMutex + lockCheckDocumentExists sync.RWMutex + lockCheckOrganizationMember sync.RWMutex + lockCopyBranchTags sync.RWMutex + lockCountDocumentBranches sync.RWMutex + lockDeleteBranchReviewer sync.RWMutex + lockDeleteDocument sync.RWMutex + lockDeleteDocumentBranchByID sync.RWMutex + lockDeleteDocumentCommentsByBranchID sync.RWMutex + lockDetachDocumentHooksByBranchID sync.RWMutex + lockFetchBranchReviewer sync.RWMutex + lockFetchBranchReviewers sync.RWMutex + lockFetchDocument sync.RWMutex + lockFetchDocumentBranches sync.RWMutex + lockFetchDocumentBranchesUnsafe sync.RWMutex + lockFetchDocumentByBranchID sync.RWMutex + lockFetchDocumentFile sync.RWMutex + lockFetchDocumentHooksByBranchID sync.RWMutex + lockFetchDocumentHooksByDocumentID sync.RWMutex + lockFetchDocumentMaintainers sync.RWMutex + lockFetchDocumentTree sync.RWMutex + lockFetchDocumentTreeByDocumentParentID sync.RWMutex + lockFetchDocumentUnsafeByBranchID sync.RWMutex + lockInsertBranchReviewer sync.RWMutex + lockInsertDocument sync.RWMutex + lockInsertDocumentBranch sync.RWMutex + lockInsertDocumentFile sync.RWMutex + lockInsertDocumentHook sync.RWMutex + lockInsertSearchJob sync.RWMutex + lockPromoteBranchApprovals sync.RWMutex + lockRecordDocumentBranchHistoryEntry sync.RWMutex + lockReplaceBranchTags sync.RWMutex + lockUpdateBranchReviewer sync.RWMutex + lockUpdateDocument sync.RWMutex + lockUpdateDocumentBranchMetadata sync.RWMutex + lockUpdateDocumentParentID sync.RWMutex + lockUpdateDocumentTree sync.RWMutex + lockUpsertDocumentMaintainers sync.RWMutex } // BeginTx calls BeginTxFunc. @@ -2009,7 +1992,7 @@ func (mock *DBMock) PromoteBranchApprovalsCalls() []struct { } // RecordDocumentBranchHistoryEntry calls RecordDocumentBranchHistoryEntryFunc. -func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -2028,10 +2011,9 @@ func (mock *DBMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branch mock.lockRecordDocumentBranchHistoryEntry.Unlock() if mock.RecordDocumentBranchHistoryEntryFunc == nil { var ( - iDOut xid.ID errOut error ) - return iDOut, errOut + return errOut } return mock.RecordDocumentBranchHistoryEntryFunc(ctx, branchID, organizationID, by, boundary) } @@ -2185,49 +2167,6 @@ func (mock *DBMock) UpdateDocumentCalls() []struct { return calls } -// UpdateDocumentBranchHistoryEntryHooks calls UpdateDocumentBranchHistoryEntryHooksFunc. -func (mock *DBMock) UpdateDocumentBranchHistoryEntryHooks(ctx context.Context, id xid.ID, hooks history.Hooks) error { - callInfo := struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - }{ - Ctx: ctx, - ID: id, - Hooks: hooks, - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.Lock() - mock.calls.UpdateDocumentBranchHistoryEntryHooks = append(mock.calls.UpdateDocumentBranchHistoryEntryHooks, callInfo) - mock.lockUpdateDocumentBranchHistoryEntryHooks.Unlock() - if mock.UpdateDocumentBranchHistoryEntryHooksFunc == nil { - var ( - errOut error - ) - return errOut - } - return mock.UpdateDocumentBranchHistoryEntryHooksFunc(ctx, id, hooks) -} - -// UpdateDocumentBranchHistoryEntryHooksCalls gets all the calls that were made to UpdateDocumentBranchHistoryEntryHooks. -// Check the length with: -// -// len(mockedDB.UpdateDocumentBranchHistoryEntryHooksCalls()) -func (mock *DBMock) UpdateDocumentBranchHistoryEntryHooksCalls() []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks -} { - var calls []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.RLock() - calls = mock.calls.UpdateDocumentBranchHistoryEntryHooks - mock.lockUpdateDocumentBranchHistoryEntryHooks.RUnlock() - return calls -} - // UpdateDocumentBranchMetadata calls UpdateDocumentBranchMetadataFunc. func (mock *DBMock) UpdateDocumentBranchMetadata(ctx context.Context, doc documentCore.Document) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/0mock_hook_manager_test.go b/server/core/internal/server/internal/document/0mock_hook_manager_test.go new file mode 100644 index 00000000..4149c28c --- /dev/null +++ b/server/core/internal/server/internal/document/0mock_hook_manager_test.go @@ -0,0 +1,89 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package document + +import ( + "context" + "sync" + + "github.com/rs/xid" +) + +// Ensure, that HookManagerMock does implement HookManager. +// If this is not the case, regenerate this file with moq. +var _ HookManager = &HookManagerMock{} + +// HookManagerMock is a mock implementation of HookManager. +// +// func TestSomethingThatUsesHookManager(t *testing.T) { +// +// // make and configure a mocked HookManager +// mockedHookManager := &HookManagerMock{ +// SetUpBranchFunc: func(ctx context.Context, branchID xid.ID, organizationID string) { +// panic("mock out the SetUpBranch method") +// }, +// } +// +// // use mockedHookManager in code that requires HookManager +// // and then make assertions. +// +// } +type HookManagerMock struct { + // SetUpBranchFunc mocks the SetUpBranch method. + SetUpBranchFunc func(ctx context.Context, branchID xid.ID, organizationID string) + + // calls tracks calls to the methods. + calls struct { + // SetUpBranch holds details about calls to the SetUpBranch method. + SetUpBranch []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // BranchID is the branchID argument value. + BranchID xid.ID + // OrganizationID is the organizationID argument value. + OrganizationID string + } + } + lockSetUpBranch sync.RWMutex +} + +// SetUpBranch calls SetUpBranchFunc. +func (mock *HookManagerMock) SetUpBranch(ctx context.Context, branchID xid.ID, organizationID string) { + callInfo := struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + }{ + Ctx: ctx, + BranchID: branchID, + OrganizationID: organizationID, + } + mock.lockSetUpBranch.Lock() + mock.calls.SetUpBranch = append(mock.calls.SetUpBranch, callInfo) + mock.lockSetUpBranch.Unlock() + if mock.SetUpBranchFunc == nil { + return + } + mock.SetUpBranchFunc(ctx, branchID, organizationID) +} + +// SetUpBranchCalls gets all the calls that were made to SetUpBranch. +// Check the length with: +// +// len(mockedHookManager.SetUpBranchCalls()) +func (mock *HookManagerMock) SetUpBranchCalls() []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string +} { + var calls []struct { + Ctx context.Context + BranchID xid.ID + OrganizationID string + } + mock.lockSetUpBranch.RLock() + calls = mock.calls.SetUpBranch + mock.lockSetUpBranch.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 e26c7c0e..42724131 100644 --- a/server/core/internal/server/internal/document/0mock_tx_test.go +++ b/server/core/internal/server/internal/document/0mock_tx_test.go @@ -10,7 +10,6 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" - "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/rs/xid" @@ -119,7 +118,7 @@ var _ Tx = &TxMock{} // PromoteBranchApprovalsFunc: func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error { // panic("mock out the PromoteBranchApprovals method") // }, -// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +// RecordDocumentBranchHistoryEntryFunc: func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { // panic("mock out the RecordDocumentBranchHistoryEntry method") // }, // ReplaceBranchTagsFunc: func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error { @@ -134,9 +133,6 @@ var _ Tx = &TxMock{} // UpdateDocumentFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocument method") // }, -// UpdateDocumentBranchHistoryEntryHooksFunc: func(ctx context.Context, id xid.ID, hooks history.Hooks) error { -// panic("mock out the UpdateDocumentBranchHistoryEntryHooks method") -// }, // UpdateDocumentBranchMetadataFunc: func(ctx context.Context, doc documentCore.Document) error { // panic("mock out the UpdateDocumentBranchMetadata method") // }, @@ -250,7 +246,7 @@ type TxMock struct { PromoteBranchApprovalsFunc func(ctx context.Context, fromBranchID xid.ID, toBranchID xid.ID, organizationID string) error // RecordDocumentBranchHistoryEntryFunc mocks the RecordDocumentBranchHistoryEntry method. - RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) + RecordDocumentBranchHistoryEntryFunc func(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error // ReplaceBranchTagsFunc mocks the ReplaceBranchTags method. ReplaceBranchTagsFunc func(ctx context.Context, organizationID string, fromBranchID xid.ID, toBranchID xid.ID) error @@ -264,9 +260,6 @@ type TxMock struct { // UpdateDocumentFunc mocks the UpdateDocument method. UpdateDocumentFunc func(ctx context.Context, doc documentCore.Document) error - // UpdateDocumentBranchHistoryEntryHooksFunc mocks the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooksFunc func(ctx context.Context, id xid.ID, hooks history.Hooks) error - // UpdateDocumentBranchMetadataFunc mocks the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadataFunc func(ctx context.Context, doc documentCore.Document) error @@ -589,15 +582,6 @@ type TxMock struct { // Doc is the doc argument value. Doc documentCore.Document } - // UpdateDocumentBranchHistoryEntryHooks holds details about calls to the UpdateDocumentBranchHistoryEntryHooks method. - UpdateDocumentBranchHistoryEntryHooks []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // ID is the id argument value. - ID xid.ID - // Hooks is the hooks argument value. - Hooks history.Hooks - } // UpdateDocumentBranchMetadata holds details about calls to the UpdateDocumentBranchMetadata method. UpdateDocumentBranchMetadata []struct { // Ctx is the ctx argument value. @@ -637,47 +621,46 @@ type TxMock struct { MaintainerIDs []string } } - lockCheckDocumentCycle sync.RWMutex - lockCheckDocumentExists sync.RWMutex - lockCheckOrganizationMember sync.RWMutex - lockCommit sync.RWMutex - lockCopyBranchTags sync.RWMutex - lockCountDocumentBranches sync.RWMutex - lockDeleteBranchReviewer sync.RWMutex - lockDeleteDocument sync.RWMutex - lockDeleteDocumentBranchByID sync.RWMutex - lockDeleteDocumentCommentsByBranchID sync.RWMutex - lockDetachDocumentHooksByBranchID sync.RWMutex - lockFetchBranchReviewer sync.RWMutex - lockFetchBranchReviewers sync.RWMutex - lockFetchDocument sync.RWMutex - lockFetchDocumentBranches sync.RWMutex - lockFetchDocumentBranchesUnsafe sync.RWMutex - lockFetchDocumentByBranchID sync.RWMutex - lockFetchDocumentFile sync.RWMutex - lockFetchDocumentHooksByBranchID sync.RWMutex - lockFetchDocumentHooksByDocumentID sync.RWMutex - lockFetchDocumentMaintainers sync.RWMutex - lockFetchDocumentTree sync.RWMutex - lockFetchDocumentTreeByDocumentParentID sync.RWMutex - lockFetchDocumentUnsafeByBranchID sync.RWMutex - lockInsertBranchReviewer sync.RWMutex - lockInsertDocument sync.RWMutex - lockInsertDocumentBranch sync.RWMutex - lockInsertDocumentFile sync.RWMutex - lockInsertDocumentHook sync.RWMutex - lockInsertSearchJob sync.RWMutex - lockPromoteBranchApprovals sync.RWMutex - lockRecordDocumentBranchHistoryEntry sync.RWMutex - lockReplaceBranchTags sync.RWMutex - lockRollback sync.RWMutex - lockUpdateBranchReviewer sync.RWMutex - lockUpdateDocument sync.RWMutex - lockUpdateDocumentBranchHistoryEntryHooks sync.RWMutex - lockUpdateDocumentBranchMetadata sync.RWMutex - lockUpdateDocumentParentID sync.RWMutex - lockUpdateDocumentTree sync.RWMutex - lockUpsertDocumentMaintainers sync.RWMutex + lockCheckDocumentCycle sync.RWMutex + lockCheckDocumentExists sync.RWMutex + lockCheckOrganizationMember sync.RWMutex + lockCommit sync.RWMutex + lockCopyBranchTags sync.RWMutex + lockCountDocumentBranches sync.RWMutex + lockDeleteBranchReviewer sync.RWMutex + lockDeleteDocument sync.RWMutex + lockDeleteDocumentBranchByID sync.RWMutex + lockDeleteDocumentCommentsByBranchID sync.RWMutex + lockDetachDocumentHooksByBranchID sync.RWMutex + lockFetchBranchReviewer sync.RWMutex + lockFetchBranchReviewers sync.RWMutex + lockFetchDocument sync.RWMutex + lockFetchDocumentBranches sync.RWMutex + lockFetchDocumentBranchesUnsafe sync.RWMutex + lockFetchDocumentByBranchID sync.RWMutex + lockFetchDocumentFile sync.RWMutex + lockFetchDocumentHooksByBranchID sync.RWMutex + lockFetchDocumentHooksByDocumentID sync.RWMutex + lockFetchDocumentMaintainers sync.RWMutex + lockFetchDocumentTree sync.RWMutex + lockFetchDocumentTreeByDocumentParentID sync.RWMutex + lockFetchDocumentUnsafeByBranchID sync.RWMutex + lockInsertBranchReviewer sync.RWMutex + lockInsertDocument sync.RWMutex + lockInsertDocumentBranch sync.RWMutex + lockInsertDocumentFile sync.RWMutex + lockInsertDocumentHook sync.RWMutex + lockInsertSearchJob sync.RWMutex + lockPromoteBranchApprovals sync.RWMutex + lockRecordDocumentBranchHistoryEntry sync.RWMutex + lockReplaceBranchTags sync.RWMutex + lockRollback sync.RWMutex + lockUpdateBranchReviewer sync.RWMutex + lockUpdateDocument sync.RWMutex + lockUpdateDocumentBranchMetadata sync.RWMutex + lockUpdateDocumentParentID sync.RWMutex + lockUpdateDocumentTree sync.RWMutex + lockUpsertDocumentMaintainers sync.RWMutex } // CheckDocumentCycle calls CheckDocumentCycleFunc. @@ -2006,7 +1989,7 @@ func (mock *TxMock) PromoteBranchApprovalsCalls() []struct { } // RecordDocumentBranchHistoryEntry calls RecordDocumentBranchHistoryEntryFunc. -func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) (xid.ID, error) { +func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool) error { callInfo := struct { Ctx context.Context BranchID xid.ID @@ -2025,10 +2008,9 @@ func (mock *TxMock) RecordDocumentBranchHistoryEntry(ctx context.Context, branch mock.lockRecordDocumentBranchHistoryEntry.Unlock() if mock.RecordDocumentBranchHistoryEntryFunc == nil { var ( - iDOut xid.ID errOut error ) - return iDOut, errOut + return errOut } return mock.RecordDocumentBranchHistoryEntryFunc(ctx, branchID, organizationID, by, boundary) } @@ -2212,49 +2194,6 @@ func (mock *TxMock) UpdateDocumentCalls() []struct { return calls } -// UpdateDocumentBranchHistoryEntryHooks calls UpdateDocumentBranchHistoryEntryHooksFunc. -func (mock *TxMock) UpdateDocumentBranchHistoryEntryHooks(ctx context.Context, id xid.ID, hooks history.Hooks) error { - callInfo := struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - }{ - Ctx: ctx, - ID: id, - Hooks: hooks, - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.Lock() - mock.calls.UpdateDocumentBranchHistoryEntryHooks = append(mock.calls.UpdateDocumentBranchHistoryEntryHooks, callInfo) - mock.lockUpdateDocumentBranchHistoryEntryHooks.Unlock() - if mock.UpdateDocumentBranchHistoryEntryHooksFunc == nil { - var ( - errOut error - ) - return errOut - } - return mock.UpdateDocumentBranchHistoryEntryHooksFunc(ctx, id, hooks) -} - -// UpdateDocumentBranchHistoryEntryHooksCalls gets all the calls that were made to UpdateDocumentBranchHistoryEntryHooks. -// Check the length with: -// -// len(mockedTx.UpdateDocumentBranchHistoryEntryHooksCalls()) -func (mock *TxMock) UpdateDocumentBranchHistoryEntryHooksCalls() []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks -} { - var calls []struct { - Ctx context.Context - ID xid.ID - Hooks history.Hooks - } - mock.lockUpdateDocumentBranchHistoryEntryHooks.RLock() - calls = mock.calls.UpdateDocumentBranchHistoryEntryHooks - mock.lockUpdateDocumentBranchHistoryEntryHooks.RUnlock() - return calls -} - // UpdateDocumentBranchMetadata calls UpdateDocumentBranchMetadataFunc. func (mock *TxMock) UpdateDocumentBranchMetadata(ctx context.Context, doc documentCore.Document) error { callInfo := struct { diff --git a/server/core/internal/server/internal/document/branch_http.go b/server/core/internal/server/internal/document/branch_http.go index 12130f59..a1cbe88a 100644 --- a/server/core/internal/server/internal/document/branch_http.go +++ b/server/core/internal/server/internal/document/branch_http.go @@ -1,18 +1,14 @@ package document import ( - "context" - "log/slog" "net/http" "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" - "github.com/oxynote/oxynote/server/core/internal/document/history" - "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/manager" "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/httpserver" - "github.com/oxynote/oxynote/server/core/pkg/logutil" "github.com/rs/xid" ) @@ -205,7 +201,7 @@ func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http. lastUpdatedBy = ndoc.LastUpdatedBy } - _, err = tx.RecordDocumentBranchHistoryEntry( + err = tx.RecordDocumentBranchHistoryEntry( r.Context(), doc.BranchID, doc.OrganizationID, @@ -341,9 +337,22 @@ func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { return } - // the target's hooks were just detached, so the entry starts without - // any. updateEntryHooks adds the copies after the commit. - entryID, err := tx.RecordDocumentBranchHistoryEntry( + // the copies go in before the entry is recorded, so it lists them. + err = manager.CopyHooks( + r.Context(), + tx, + fromDoc.BranchID, + toDoc.BranchID, + toDoc.ID, + session.ActiveOrganizationID, + nil, + ) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + err = tx.RecordDocumentBranchHistoryEntry( r.Context(), toDoc.BranchID, session.ActiveOrganizationID, @@ -376,17 +385,7 @@ func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { } h.searchTrigger.Trigger() - - hooks := h.copyHooksToBranch( - r.Context(), - fromDoc.BranchID, - toDoc.BranchID, - toDoc.ID, - session.ActiveOrganizationID, - nil, - ) - - h.updateEntryHooks(r.Context(), entryID, ndoc, hooks) + h.hookMan.SetUpBranch(r.Context(), toDoc.BranchID, session.ActiveOrganizationID) if h.metadata.changeCallback != nil { h.metadata.changeCallback(session.ActiveOrganizationID, ndoc) @@ -487,9 +486,22 @@ func (h *Handler) CreateDocumentBranch(w http.ResponseWriter, r *http.Request) { return } - // the fork has no hooks yet. updateEntryHooks adds the copies after - // the commit. - entryID, err := tx.RecordDocumentBranchHistoryEntry( + // the copies go in before the entry is recorded, so it lists them. + err = manager.CopyHooks( + r.Context(), + tx, + sourceDoc.BranchID, + newDoc.BranchID, + sourceDoc.ID, + session.ActiveOrganizationID, + nil, + ) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + err = tx.RecordDocumentBranchHistoryEntry( r.Context(), newDoc.BranchID, session.ActiveOrganizationID, @@ -517,17 +529,7 @@ func (h *Handler) CreateDocumentBranch(w http.ResponseWriter, r *http.Request) { } h.searchTrigger.Trigger() - - hooks := h.copyHooksToBranch( - r.Context(), - sourceDoc.BranchID, - newDoc.BranchID, - sourceDoc.ID, - session.ActiveOrganizationID, - nil, - ) - - h.updateEntryHooks(r.Context(), entryID, newDoc, hooks) + h.hookMan.SetUpBranch(r.Context(), newDoc.BranchID, session.ActiveOrganizationID) httpserver.Respond( h.log, @@ -617,106 +619,3 @@ func (h *Handler) DeleteDocumentBranch(w http.ResponseWriter, r *http.Request) { http.StatusNoContent, ) } - -// copyHooksToBranch fetches all hooks from fromBranchID and re-creates them on -// toBranchID with fresh state, once the target is committed. Creating a hook -// creates its external resource as a side effect, so the copy cannot run -// inside the caller's transaction: a rollback cannot take a changedetection.io -// watcher back, and the row that would have pointed at it is gone. A failed -// insert tears the just-created resource down again for the same reason. A -// failure stops the copy and leaves the target standing with fewer hooks than -// its source, which the critical log reports. The hooks created up to that -// point are returned either way. -// -// A branch whose content was duplicated carries fresh block uids, so the -// caller passes the old-to-new uid map and a hook anchored to a block is -// re-anchored through it; a hook whose block the map does not name has -// nothing to point at on the target and is dropped. A nil map keeps every -// block id as it is, which is right for a fork or a merge. -func (h *Handler) copyHooksToBranch( - ctx context.Context, - fromBranchID, toBranchID, documentID xid.ID, - organizationID string, - uids map[string]string, -) []hook.Hook { - hooks, err := h.db.FetchDocumentHooksByBranchID(ctx, fromBranchID, organizationID) - if err != nil { - logutil.Critical(h.log, err).Error( - "cannot fetch the hooks to copy", - slog.String("branch_id", fromBranchID.String()), - ) - - return nil - } - - inp := hook.NewInput(organizationID, h.githubMan, h.webchangeClient) - - created := make([]hook.Hook, 0, len(hooks)) - - for _, hk := range hooks { - blockID := hk.BlockID - - if uids != nil && blockID.Valid { - uid, ok := uids[blockID.String] - if !ok { - continue - } - - blockID = null.StringFrom(uid) - } - - newHk, err := hk.CopyTo( - ctx, - documentID, - toBranchID, - blockID, - organizationID, - inp, - ) - if err != nil { - logutil.Critical(h.log, err).Error( - "cannot re-create the hook on the target branch", - slog.String("hook_id", hk.ID.String()), - slog.String("branch_id", toBranchID.String()), - ) - - return created - } - - if err := h.db.InsertDocumentHook(ctx, *newHk); err != nil { - if derr := newHk.Delete(ctx, inp); derr != nil { - logutil.Critical(h.log, derr).Error( - "cannot delete the hook external resource after a failed insert", - slog.String("hook_id", newHk.ID.String()), - ) - } - - logutil.Critical(h.log, err).Error( - "cannot insert the copied hook", - slog.String("hook_id", newHk.ID.String()), - slog.String("branch_id", toBranchID.String()), - ) - - return created - } - - created = append(created, *newHk) - } - - return created -} - -// updateEntryHooks sets the hooks of a boundary entry to the ones -// copyHooksToBranch created, as the entry's document holds them. The -// entry's operation has already committed, so a failure is logged rather -// than returned. -func (h *Handler) updateEntryHooks(ctx context.Context, entryID xid.ID, doc documentCore.Document, hooks []hook.Hook) { - err := h.db.UpdateDocumentBranchHistoryEntryHooks(ctx, entryID, history.NewHooks(doc, hooks)) - if err != nil { - h.log.Error( - "cannot record the copied hooks on the history entry", - slog.String("entry_id", entryID.String()), - slog.String("error", err.Error()), - ) - } -} 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 beaabb69..2fea7349 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -1,17 +1,14 @@ package document import ( - "bytes" "context" "errors" - "log/slog" "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/history" hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/internal/search" @@ -25,14 +22,12 @@ import ( func storedHook(typ hookCore.Type) hookCore.Hook { return hookCore.Hook{ ID: xid.New(), - CrossBranchID: xid.New(), Type: typ, DocumentID: null.ValueFrom(_documentID), OrganizationID: null.StringFrom("org1"), BranchID: null.ValueFrom(_branchID), Settings: processor.Settings(`{"scale":"linear"}`), State: null.ValueFrom(processor.State(`{}`)), - Status: processor.StatusActive, } } @@ -170,8 +165,8 @@ func Test_Handler_UpdateDocumentBranchByIDUnsafe(t *testing.T) { "Error returned by Tx.RecordDocumentBranchHistoryEntry": { DB: &DBMock{FetchDocumentUnsafeByBranchIDFunc: fetchStored}, Tx: &TxMock{ - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.ID{}, errors.New("boom") + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return errors.New("boom") }, }, Body: validBody, @@ -460,14 +455,12 @@ func Test_Handler_MergeBranches(t *testing.T) { Reviewers int // History is how many history entries the merge records. History int - // EntryHooks is how many hooks the merge entry lists afterwards. - EntryHooks int }{ "History entry insert error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, Tx: &TxMock{ - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.ID{}, errors.New("boom") + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return errors.New("boom") }, }, Body: validBody, @@ -645,6 +638,16 @@ func Test_Handler_MergeBranches(t *testing.T) { RespCode: http.StatusInternalServerError, History: 1, }, + "Error returned by tx.FetchDocumentHooksByBranchID": { + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, "Commit error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, Tx: &TxMock{ @@ -658,20 +661,14 @@ func Test_Handler_MergeBranches(t *testing.T) { History: 1, }, "Successful merge": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: fetchByBranch, - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil - }, - }, - Tx: &TxMock{}, - Body: validBody, - RespCode: http.StatusOK, - Committed: 1, - Metadata: 1, - Reviewers: 1, - History: 1, - EntryHooks: 1, + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{}, + Body: validBody, + RespCode: http.StatusOK, + Committed: 1, + Metadata: 1, + Reviewers: 1, + History: 1, }, } @@ -681,6 +678,9 @@ func Test_Handler_MergeBranches(t *testing.T) { hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) + man := &HookManagerMock{} + hdl.hookMan = man + rec := httptest.NewRecorder() hdl.MergeBranches(rec, newRequest(http.MethodPut, c.Body, c.NoSession, c.OmitDoc, true)) @@ -698,25 +698,28 @@ func Test_Handler_MergeBranches(t *testing.T) { if c.RespCode == http.StatusOK { // the merge is a boundary entry of the target, attributed to - // the merging user. It takes the hooks the copy created once - // the merge commits. + // the merging user. entry := c.Tx.RecordDocumentBranchHistoryEntryCalls()[0] assert.Equal(t, _branchID, entry.BranchID) assert.Equal(t, "org1", entry.OrganizationID) assert.True(t, entry.Boundary) assert.Equal(t, null.StringFrom("u1"), entry.By) - hh := c.DB.UpdateDocumentBranchHistoryEntryHooksCalls() - require.Len(t, hh, 1) - assert.Equal(t, _entryID, hh[0].ID) - assert.Len(t, hh[0].Hooks, c.EntryHooks) - - // hooks are soft-deleted on the target inside the - // transaction and re-created from the source branch once it - // commits, since creating one creates its watcher. + // the target's hooks are detached and the source's copied + // inside the transaction; the copies are set up once it + // commits. require.Len(t, c.Tx.DetachDocumentHooksByBranchIDCalls(), 1) assert.Equal(t, _branchID, c.Tx.DetachDocumentHooksByBranchIDCalls()[0].BranchID) - assert.Empty(t, c.Tx.InsertDocumentHookCalls()) + + hh := c.Tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, hh, 1) + assert.Equal(t, _branchID2, hh[0].BranchID) + assert.Equal(t, "org1", hh[0].OrganizationID) + + pp := man.SetUpBranchCalls() + require.Len(t, pp, 1) + assert.Equal(t, _branchID, pp[0].BranchID) + assert.Equal(t, "org1", pp[0].OrganizationID) require.Len(t, c.Tx.PromoteBranchApprovalsCalls(), 1) // the target takes the source's tags the way it takes its @@ -724,15 +727,6 @@ func Test_Handler_MergeBranches(t *testing.T) { require.Len(t, c.Tx.ReplaceBranchTagsCalls(), 1) assert.Equal(t, _branchID2, c.Tx.ReplaceBranchTagsCalls()[0].FromBranchID) assert.Equal(t, _branchID, c.Tx.ReplaceBranchTagsCalls()[0].ToBranchID) - - ff := c.DB.FetchDocumentHooksByBranchIDCalls() - require.Len(t, ff, 1) - assert.Equal(t, _branchID2, ff[0].BranchID) - } - - if cn == "Successful merge" { - require.Len(t, c.DB.InsertDocumentHookCalls(), 1) - assert.Equal(t, null.ValueFrom(_branchID), c.DB.InsertDocumentHookCalls()[0].Hk.BranchID) } }) } @@ -796,16 +790,14 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { validBody := `{"branch":"feature","sourceBranchId":"` + _branchID.String() + `"}` cc := map[string]struct { - DB *DBMock - Tx *TxMock - BeginErr error - NoSession bool - OmitDoc bool - Body string - CopiedHooks []hookCore.Hook - HookFetchErr error - RespCode int - Committed int + DB *DBMock + Tx *TxMock + BeginErr error + NoSession bool + OmitDoc bool + Body string + RespCode int + Committed int // History is how many history entries the fork records. History int }{ @@ -816,8 +808,8 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { }, }, Tx: &TxMock{ - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.ID{}, errors.New("boom") + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return errors.New("boom") }, }, Body: validBody, @@ -898,20 +890,19 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, }, - "Hook copy error": { + "Error returned by tx.FetchDocumentHooksByBranchID": { DB: &DBMock{ FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { return storedDoc(), nil }, }, - Tx: &TxMock{}, - HookFetchErr: errors.New("boom"), - Body: validBody, - // the hooks are copied after the commit, so a failure there - // leaves the branch itself standing. - RespCode: http.StatusCreated, - Committed: 1, - History: 1, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, }, "Commit error": { DB: &DBMock{ @@ -950,12 +941,11 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { return storedDoc(), nil }, }, - Tx: &TxMock{}, - CopiedHooks: []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, - Body: validBody, - RespCode: http.StatusCreated, - Committed: 1, - History: 1, + Tx: &TxMock{}, + Body: validBody, + RespCode: http.StatusCreated, + Committed: 1, + History: 1, }, "Tag copy error": { DB: &DBMock{FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { @@ -976,19 +966,17 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - c.DB.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return c.CopiedHooks, c.HookFetchErr - } - hdl, _ := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) + man := &HookManagerMock{} + hdl.hookMan = man + rec := httptest.NewRecorder() hdl.CreateDocumentBranch(rec, newRequest(http.MethodPost, c.Body, c.NoSession, c.OmitDoc, true)) assert.Equal(t, c.RespCode, rec.Code) assert.Len(t, c.Tx.CommitCalls(), c.Committed) - assert.Empty(t, c.Tx.InsertDocumentHookCalls()) assert.Len(t, c.Tx.RecordDocumentBranchHistoryEntryCalls(), c.History) if c.RespCode == http.StatusCreated { @@ -1000,14 +988,23 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { assert.Nil(t, forked.RawContent) // the fork starts with a boundary entry of its own content. - // It takes the hooks the copy created once the fork commits. entry := c.Tx.RecordDocumentBranchHistoryEntryCalls()[0] assert.Equal(t, forked.BranchID, entry.BranchID) assert.Equal(t, "org1", entry.OrganizationID) assert.True(t, entry.Boundary) assert.Equal(t, null.StringFrom("u1"), entry.By) - require.Len(t, c.DB.UpdateDocumentBranchHistoryEntryHooksCalls(), 1) - assert.Equal(t, _entryID, c.DB.UpdateDocumentBranchHistoryEntryHooksCalls()[0].ID) + + // the source's hooks are copied inside the transaction and + // set up once it commits. + hh := c.Tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, hh, 1) + assert.Equal(t, _branchID, hh[0].BranchID) + assert.Equal(t, "org1", hh[0].OrganizationID) + + pp := man.SetUpBranchCalls() + require.Len(t, pp, 1) + assert.Equal(t, forked.BranchID, pp[0].BranchID) + assert.Equal(t, "org1", pp[0].OrganizationID) // the fork takes the source's tags before the commit. require.Len(t, c.Tx.CopyBranchTagsCalls(), 1) @@ -1019,21 +1016,6 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { require.Len(t, c.Tx.InsertSearchJobCalls(), 1) assert.Equal(t, search.BranchScope("org1", forked.ID, forked.BranchID), c.Tx.InsertSearchJobCalls()[0].Job) } - - if len(c.CopiedHooks) == 0 { - return - } - - // hooks are copied from the source to the new branch once the - // fork commits, since creating one creates its watcher. - forked := c.Tx.InsertDocumentBranchCalls()[0].Doc - - require.Len(t, c.DB.InsertDocumentHookCalls(), 1) - assert.Equal(t, null.ValueFrom(forked.BranchID), c.DB.InsertDocumentHookCalls()[0].Hk.BranchID) - assert.Equal(t, - history.NewHooks(forked, []hookCore.Hook{c.DB.InsertDocumentHookCalls()[0].Hk}), - c.DB.UpdateDocumentBranchHistoryEntryHooksCalls()[0].Hooks, - ) }) } } @@ -1200,126 +1182,6 @@ func Test_Handler_DeleteDocumentBranch(t *testing.T) { } } -func Test_Handler_copyHooksToBranch(t *testing.T) { - t.Parallel() - - // a url-watcher cannot get its watcher without changedetection - // configured, so the copy keeps it unset for a later run to set up. - t.Run("URL watcher copied unset without changedetection", func(t *testing.T) { - t.Parallel() - - urlHook := storedHook(hookCore.TypeURLWatcher) - urlHook.Settings = processor.Settings(`{"url":"https://example.com"}`) - - db := &DBMock{ - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{urlHook, storedHook(hookCore.TypeScheduledReminder)}, nil - }, - InsertDocumentHookFunc: func(context.Context, hookCore.Hook) error { - return nil - }, - } - - hdl, _ := newTestHandler(db, &fakePublisher{}) - - created := hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) - - ff := db.InsertDocumentHookCalls() - require.Len(t, ff, 2) - assert.Equal(t, hookCore.TypeURLWatcher, ff[0].Hk.Type) - assert.False(t, ff[0].Hk.State.Valid) - assert.Equal(t, processor.StatusUnconfigured, ff[0].Hk.Status) - assert.Equal(t, hookCore.TypeScheduledReminder, ff[1].Hk.Type) - assert.True(t, ff[1].Hk.State.Valid) - assert.Equal(t, []hookCore.Hook{ff[0].Hk, ff[1].Hk}, created) - }) - - // a fork or a merge stays within the document, so the copy is the same - // hook on another branch; a duplicate is another document, whose hooks - // are their own. - t.Run("Cross-branch ID is kept within the document only", func(t *testing.T) { - t.Parallel() - - src := storedHook(hookCore.TypeScheduledReminder) - - db := &DBMock{ - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{src}, nil - }, - } - - hdl, _ := newTestHandler(db, &fakePublisher{}) - - hdl.copyHooksToBranch(context.Background(), _branchID, _branchID2, _documentID, "org1", nil) - hdl.copyHooksToBranch(context.Background(), _branchID, _branchID2, xid.New(), "org1", nil) - - ff := db.InsertDocumentHookCalls() - require.Len(t, ff, 2) - assert.Equal(t, src.CrossBranchID, ff[0].Hk.CrossBranchID) - assert.NotEqual(t, src.ID, ff[0].Hk.ID) - assert.Equal(t, ff[1].Hk.ID, ff[1].Hk.CrossBranchID) - }) - - // a duplicated branch carries fresh block uids, so a hook anchored to a - // block follows the map to the block's new uid, a document-level hook - // is copied as it is, and a hook whose block the map does not name is - // dropped rather than left pointing at nothing. - t.Run("Block hooks are re-anchored through the uid map", func(t *testing.T) { - t.Parallel() - - blockHook := storedHook(hookCore.TypeScheduledReminder) - blockHook.BlockID = null.StringFrom("old-uid") - - goneHook := storedHook(hookCore.TypeScheduledReminder) - goneHook.BlockID = null.StringFrom("gone-uid") - - db := &DBMock{ - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{blockHook, storedHook(hookCore.TypeScheduledReminder), goneHook}, nil - }, - } - - hdl, _ := newTestHandler(db, &fakePublisher{}) - - hdl.copyHooksToBranch( - context.Background(), - _branchID2, - _branchID, - _documentID, - "org1", - map[string]string{"old-uid": "new-uid"}, - ) - - ff := db.InsertDocumentHookCalls() - require.Len(t, ff, 2) - assert.Equal(t, null.StringFrom("new-uid"), ff[0].Hk.BlockID) - assert.Equal(t, null.String{}, ff[1].Hk.BlockID) - }) - - // creating a hook creates its external resource, so a failed insert has - // to hand it back rather than leave it running with no row pointing at - // it. A url-watcher would call changedetection.io here, which the test - // cannot reach; the scheduled reminder exercises the same path with a - // teardown that needs nothing external. - db := &DBMock{ - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil - }, - InsertDocumentHookFunc: func(context.Context, hookCore.Hook) error { - return errors.New("boom") - }, - } - - hdl, _ := newTestHandler(db, &fakePublisher{}) - - created := hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) - - // the failure stops the copy rather than working through the rest of - // the branch's hooks and creating a watcher for each. - require.Len(t, db.InsertDocumentHookCalls(), 1) - assert.Empty(t, created) -} - func Test_Handler_UpdateDocumentBranch(t *testing.T) { validBody := `{"name":"Renamed","protected":true}` @@ -1483,54 +1345,3 @@ func Test_Handler_UpdateDocumentBranch(t *testing.T) { }) } } - -func Test_Handler_updateEntryHooks(t *testing.T) { - cc := map[string]struct { - Err error - Log string - }{ - // the entry's operation has already committed, so the failure - // is logged rather than returned. - "Error returned by db.UpdateDocumentBranchHistoryEntryHooks": { - Err: assert.AnError, - Log: "cannot record the copied hooks on the history entry", - }, - "Successful update": {}, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - db := &DBMock{ - UpdateDocumentBranchHistoryEntryHooksFunc: func(context.Context, xid.ID, history.Hooks) error { - return c.Err - }, - } - - var buf bytes.Buffer - - hdl, _ := newTestHandler(db, &fakePublisher{}) - hdl.log = slog.New(slog.NewTextHandler(&buf, nil)) - - entryID := xid.New() - hk := storedHook(hookCore.TypeScheduledReminder) - - doc := *storedDoc() - - hdl.updateEntryHooks(context.Background(), entryID, doc, []hookCore.Hook{hk}) - - ff := db.UpdateDocumentBranchHistoryEntryHooksCalls() - require.Len(t, ff, 1) - assert.Equal(t, entryID, ff[0].ID) - assert.Equal(t, history.NewHooks(doc, []hookCore.Hook{hk}), ff[0].Hooks) - - if c.Log == "" { - assert.Empty(t, buf.String()) - return - } - - assert.Contains(t, buf.String(), c.Log) - }) - } -} diff --git a/server/core/internal/server/internal/document/document_http.go b/server/core/internal/server/internal/document/document_http.go index a96caaed..29eb9860 100644 --- a/server/core/internal/server/internal/document/document_http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -13,8 +13,8 @@ import ( "github.com/oxynote/oxynote/server/core/internal/apps/webchange" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" - "github.com/oxynote/oxynote/server/core/internal/document/history" "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/manager" "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" @@ -39,6 +39,7 @@ type Handler struct { db DB githubMan *github.Manager webchangeClient *webchange.Client + hookMan HookManager searcher Searcher searchTrigger SearchTrigger notifPub notification.Publisher @@ -67,6 +68,7 @@ func NewHandler( db DB, githubMan *github.Manager, webchangeClient *webchange.Client, + hookMan HookManager, searcher Searcher, searchTrigger SearchTrigger, notifPub notification.Publisher, @@ -77,6 +79,7 @@ func NewHandler( db: db, githubMan: githubMan, webchangeClient: webchangeClient, + hookMan: hookMan, searcher: searcher, searchTrigger: searchTrigger, notifPub: notifPub, @@ -535,7 +538,7 @@ func (h *Handler) CreateDocument(w http.ResponseWriter, r *http.Request) { doc := documentCore.NewDocument(di, session.ActiveOrganizationID, session.UserID) - if _, err := h.insertDocumentTx(r.Context(), doc, null.Value[xid.ID]{}, session); err != nil { + if err := h.insertDocumentTx(r.Context(), doc, null.Value[xid.ID]{}, nil, session); err != nil { httpserver.RespondError(h.log, w, err) return } @@ -721,25 +724,13 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { return } - duplDoc, files, uids := doc.Duplicate(session.UserID) + duplDoc, files, newBlockIDs := doc.Duplicate(session.UserID) - entryID, err := h.insertDocumentTx(r.Context(), duplDoc, null.ValueFrom(doc.BranchID), session) - if err != nil { + if err := h.insertDocumentTx(r.Context(), duplDoc, null.ValueFrom(doc.BranchID), newBlockIDs, session); err != nil { httpserver.RespondError(h.log, w, err) return } - hooks := h.copyHooksToBranch( - r.Context(), - doc.BranchID, - duplDoc.BranchID, - duplDoc.ID, - session.ActiveOrganizationID, - uids, - ) - - h.updateEntryHooks(r.Context(), entryID, duplDoc, hooks) - h.copyDocumentFiles(r.Context(), files, duplDoc.ID, session.ActiveOrganizationID) if h.tree.changeCallback != nil { @@ -848,35 +839,44 @@ func (h *Handler) upsertBranchReviewer( // insertDocumentTx inserts a new document together with its maintainer, // its search job and the boundary history entry of its first content, and -// slots it at the top of its parent's tree, all in one transaction. It -// returns the entry's id. A document duplicated from a branch also takes -// that branch's tags. The tree-change notification is left to the caller, -// since it must not fire before the commit. +// slots it at the top of its parent's tree, all in one transaction. A +// document duplicated from a branch also takes that branch's tags and its +// hooks, re-anchored through newBlockIDs. The tree-change notification is +// left to the caller, since it must not fire before the commit. func (h *Handler) insertDocumentTx( ctx context.Context, doc documentCore.Document, sourceBranchID null.Value[xid.ID], + newBlockIDs map[string]string, session auth.Session, -) (xid.ID, error) { +) error { var tx Tx if err := h.db.BeginTx(ctx, &tx); err != nil { - return xid.ID{}, err + return err } defer tx.Rollback() //nolint:errcheck // error provides no meaningful info if doc.ParentID.Valid { if err := tx.CheckDocumentExists(ctx, doc.ParentID.V, session.ActiveOrganizationID); err != nil { - return xid.ID{}, err + return err } } if err := tx.InsertDocument(ctx, doc); err != nil { - return xid.ID{}, err + return err } - entryID, err := tx.RecordDocumentBranchHistoryEntry( + // the copies go in before the entry is recorded, so it lists them. + if sourceBranchID.Valid { + err := manager.CopyHooks(ctx, tx, sourceBranchID.V, doc.BranchID, doc.ID, session.ActiveOrganizationID, newBlockIDs) + if err != nil { + return err + } + } + + err := tx.RecordDocumentBranchHistoryEntry( ctx, doc.BranchID, session.ActiveOrganizationID, @@ -884,12 +884,12 @@ func (h *Handler) insertDocumentTx( true, ) if err != nil { - return xid.ID{}, err + return err } if sourceBranchID.Valid { if err = tx.CopyBranchTags(ctx, session.ActiveOrganizationID, sourceBranchID.V, doc.BranchID); err != nil { - return xid.ID{}, err + return err } } @@ -899,34 +899,47 @@ func (h *Handler) insertDocumentTx( session.ActiveOrganizationID, []string{session.UserID}, ); err != nil { - return xid.ID{}, err + return err } if err = tx.InsertSearchJob(ctx, search.BranchScope(session.ActiveOrganizationID, doc.ID, doc.BranchID)); err != nil { - return xid.ID{}, err + return err } tree, err := tx.FetchDocumentTreeByDocumentParentID(ctx, doc.ParentID, session.ActiveOrganizationID) if err != nil { - return xid.ID{}, err + return err } swappedTree, err := tree.Swap(doc.ID, 0) if err != nil { - return xid.ID{}, err + return err } if err = tx.UpdateDocumentTree(ctx, swappedTree, session.ActiveOrganizationID); err != nil { - return xid.ID{}, err + return err } if err = tx.Commit(); err != nil { - return xid.ID{}, err + return err } h.searchTrigger.Trigger() - return entryID, nil + if sourceBranchID.Valid { + h.hookMan.SetUpBranch(ctx, doc.BranchID, session.ActiveOrganizationID) + } + + return nil +} + +// HookManager sets up the hooks a branch operation copied. +// +//go:generate ../../../../scripts/codegen/mock -t internal HookManager hook_manager +type HookManager interface { + // SetUpBranch should set up the branch's hooks that are not set up + // yet, leaving any it cannot to a later pass. + SetUpBranch(ctx context.Context, branchID xid.ID, organizationID string) } // DB is an interface that combines sqlutil.DB and DBAgent. @@ -1010,14 +1023,14 @@ type DocumentsDBAgent interface { UpdateDocument(ctx context.Context, doc documentCore.Document) error // RecordDocumentBranchHistoryEntry should record the branch as it - // stands, with its live hooks, and return the id of the entry. + // stands, with its hooks. RecordDocumentBranchHistoryEntry( ctx context.Context, branchID xid.ID, organizationID string, by null.String, boundary bool, - ) (xid.ID, error) + ) error // DeleteDocument should delete the document and report the ids of // the document and of every cascade-deleted descendant. @@ -1055,10 +1068,6 @@ type BranchesDBAgent interface { // ReplaceBranchTags should make the target branch carry exactly the // tags the source branch carries. ReplaceBranchTags(ctx context.Context, organizationID string, fromBranchID, toBranchID xid.ID) error - - // UpdateDocumentBranchHistoryEntryHooks should replace the hooks the - // history entry lists. - UpdateDocumentBranchHistoryEntryHooks(ctx context.Context, id xid.ID, hooks history.Hooks) error } // TreeDBAgent is an interface that handles communication with the document diff --git a/server/core/internal/server/internal/document/document_http_test.go b/server/core/internal/server/internal/document/document_http_test.go index 3bc8ea23..6ef056d8 100644 --- a/server/core/internal/server/internal/document/document_http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -35,7 +35,6 @@ var ( _documentID = xid.New() _branchID = xid.New() _branchID2 = xid.New() - _entryID = xid.New() ) // fakePublisher captures published notifications. @@ -71,6 +70,7 @@ func newTestHandler(db DB, pub *fakePublisher) (*Handler, *callbackCounts) { notifPub: pub, storer: &StorerMock{}, webchangeClient: webchange.NewClient("", ""), + hookMan: &HookManagerMock{}, searchTrigger: &SearchTriggerMock{}, } @@ -82,15 +82,8 @@ func newTestHandler(db DB, pub *fakePublisher) (*Handler, *callbackCounts) { return hdl, cnt } -// withTx wires the DB mock's BeginTx to hand out the provided Tx mock. A -// history record the Tx mock does not stub returns _entryID. +// withTx wires the DB mock's BeginTx to hand out the provided Tx mock. func withTx(db *DBMock, tx *TxMock, err error) *DBMock { - if tx != nil && tx.RecordDocumentBranchHistoryEntryFunc == nil { - tx.RecordDocumentBranchHistoryEntryFunc = func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return _entryID, nil - } - } - db.BeginTxFunc = func(_ context.Context, dest any) error { if err != nil { return err @@ -164,11 +157,13 @@ func Test_NewHandler(t *testing.T) { trigger := &SearchTriggerMock{} pub := &fakePublisher{} st := &StorerMock{} + man := &HookManagerMock{} - hdl := NewHandler(slog.New(slog.DiscardHandler), db, nil, nil, searcher, trigger, pub, st) + hdl := NewHandler(slog.New(slog.DiscardHandler), db, nil, nil, man, searcher, trigger, pub, st) require.NotNil(t, hdl) assert.NotNil(t, hdl.log) assert.Same(t, db, hdl.db) + assert.Same(t, man, hdl.hookMan) assert.Same(t, searcher, hdl.searcher) assert.Same(t, trigger, hdl.searchTrigger) assert.Same(t, pub, hdl.notifPub) @@ -1308,8 +1303,8 @@ func Test_Handler_CreateDocument(t *testing.T) { }, "History entry insert error": { Tx: &TxMock{ - RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) (xid.ID, error) { - return xid.ID{}, errors.New("boom") + RecordDocumentBranchHistoryEntryFunc: func(context.Context, xid.ID, string, null.String, bool) error { + return errors.New("boom") }, }, Body: validBody, @@ -1601,17 +1596,19 @@ func Test_Handler_DuplicateDocument(t *testing.T) { } cc := map[string]struct { - DB *DBMock - Tx *TxMock - Storer *StorerMock - BeginErr error - NoSession bool - OmitDoc bool - RespCode int - Committed int - TreeCbs int - Copies int - CopiedHooks int + DB *DBMock + Tx *TxMock + Storer *StorerMock + BeginErr error + NoSession bool + OmitDoc bool + RespCode int + Committed int + TreeCbs int + Copies int + // CopiedUID is a source block uid the hook copy is told to + // re-anchor. + CopiedUID string }{ "No session in context": { DB: &DBMock{}, @@ -1705,39 +1702,27 @@ func Test_Handler_DuplicateDocument(t *testing.T) { TreeCbs: 1, Copies: 1, }, - "Hooks are copied along with the document": { - DB: func() *DBMock { - blockHook := storedHook(hookCore.TypeScheduledReminder) - blockHook.BlockID = null.StringFrom("img-1-aaaaaaaaaaaaaaa") - - db := imageDB() - db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{blockHook}, nil - } - - return db - }(), - Tx: insertAwareTx(), - Storer: &StorerMock{}, - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - Copies: 1, - CopiedHooks: 1, - }, - "Failing hook copy still yields the duplicate": { - DB: func() *DBMock { - db := &DBMock{FetchDocumentFunc: fetchStored} - db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return nil, errors.New("boom") - } - - return db - }(), + "Hooks follow the regenerated block uids": { + DB: imageDB(), Tx: insertAwareTx(), + Storer: &StorerMock{}, RespCode: http.StatusCreated, Committed: 1, TreeCbs: 1, + Copies: 1, + CopiedUID: "img-1-aaaaaaaaaaaaaaa", + }, + "Error returned by tx.FetchDocumentHooksByBranchID": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: func() *TxMock { + tx := insertAwareTx() + tx.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + } + + return tx + }(), + RespCode: http.StatusInternalServerError, }, } @@ -1751,6 +1736,17 @@ func Test_Handler_DuplicateDocument(t *testing.T) { hdl.storer = c.Storer } + man := &HookManagerMock{} + hdl.hookMan = man + + // a hook anchored to the copied block is re-anchored to its new + // uid. + if c.CopiedUID != "" { + c.Tx.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{{BlockID: null.StringFrom(c.CopiedUID)}}, nil + } + } + rec := httptest.NewRecorder() hdl.DuplicateDocument(rec, newRequest(http.MethodPost, "", c.NoSession, c.OmitDoc, true)) @@ -1777,30 +1773,31 @@ func Test_Handler_DuplicateDocument(t *testing.T) { assert.Equal(t, _branchID, c.Tx.CopyBranchTagsCalls()[0].FromBranchID) assert.Equal(t, dupl.BranchID, c.Tx.CopyBranchTagsCalls()[0].ToBranchID) - // the hooks are copied once the commit is through, since - // creating one creates its watcher. - assert.Empty(t, c.Tx.InsertDocumentHookCalls()) - require.Len(t, c.DB.InsertDocumentHookCalls(), c.CopiedHooks) + // the hooks are copied inside the transaction, re-anchored + // to the duplicate's regenerated block uids, and set up once + // it commits. + hh := c.Tx.FetchDocumentHooksByBranchIDCalls() + require.Len(t, hh, 1) + assert.Equal(t, _branchID, hh[0].BranchID) + assert.Equal(t, "org1", hh[0].OrganizationID) - // the duplicate starts with a boundary entry, which takes - // the copied hooks afterwards. + pp := man.SetUpBranchCalls() + require.Len(t, pp, 1) + assert.Equal(t, dupl.BranchID, pp[0].BranchID) + + if c.CopiedUID != "" { + ii := c.Tx.InsertDocumentHookCalls() + require.Len(t, ii, 1) + assert.Equal(t, null.ValueFrom(dupl.ID), ii[0].Hk.DocumentID) + assert.True(t, ii[0].Hk.BlockID.Valid) + assert.NotEqual(t, c.CopiedUID, ii[0].Hk.BlockID.String) + } + + // the duplicate starts with a boundary entry. require.Len(t, c.Tx.RecordDocumentBranchHistoryEntryCalls(), 1) entry := c.Tx.RecordDocumentBranchHistoryEntryCalls()[0] assert.Equal(t, dupl.BranchID, entry.BranchID) assert.True(t, entry.Boundary) - - hh := c.DB.UpdateDocumentBranchHistoryEntryHooksCalls() - require.Len(t, hh, 1) - assert.Equal(t, _entryID, hh[0].ID) - assert.Len(t, hh[0].Hooks, c.CopiedHooks) - - // a hook anchored to a block follows the block's regenerated - // uid rather than pointing at the source's. - for _, call := range c.DB.InsertDocumentHookCalls() { - assert.Equal(t, null.ValueFrom(dupl.BranchID), call.Hk.BranchID) - assert.NotEqual(t, null.StringFrom("img-1-aaaaaaaaaaaaaaa"), call.Hk.BlockID) - assert.True(t, call.Hk.BlockID.Valid) - } } }) } diff --git a/server/core/internal/server/server.go b/server/core/internal/server/server.go index e9ba19aa..177ffcec 100644 --- a/server/core/internal/server/server.go +++ b/server/core/internal/server/server.go @@ -177,7 +177,7 @@ func NewServer( searchTrigger, opts.PublicURL, ) - srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, searcher, searchTrigger, notifier, storageClient) + srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, hookMan, searcher, searchTrigger, notifier, storageClient) srv.handlers.tag = tag.NewHandler(log, db) srv.handlers.comment = comment.NewHandler(log, db, notifier) srv.handlers.files = files.NewHandler(log, db, storageClient, opts.PublicURL) @@ -365,11 +365,13 @@ type DB interface { tag.DB } -// HookManager runs the hook writes. +// HookManager runs the hook writes and sets up the hooks a branch operation +// copied. // //go:generate ../../scripts/codegen/mock -t internal HookManager hook_manager type HookManager interface { hook.Manager + document.HookManager // BindHookChange should set the function called with every hook the // manager stores, changes or deletes. diff --git a/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts b/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts index 4e0751ec..8e02bb90 100644 --- a/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts +++ b/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts @@ -163,6 +163,34 @@ describe("", { concurrent: false }, () => { expect(iconNames(hookNotice())).toEqual([expected.icon]) }) + it.for<{ + name: string + input: Partial + expected: boolean + }>([ + { + name: "says a copy is being set up", + input: { state: null, status: "initializing" }, + expected: true, + }, + { + name: "says nothing of setup once the hook is set up", + input: { state: {}, status: "active" }, + expected: false, + }, + { + name: "says nothing of setup for a copy that cannot check its target", + input: { state: null, status: "unconfigured" }, + expected: false, + }, + ])("$name", async ({ input, expected }, { expect }) => { + await mountPanel({ hook: makeHook(input) }) + + await openHookSubMenu(TITLE) + + expect(menuText().includes(t("editor.hooks.initializing"))).toBe(expected) + }) + it("shows a setup notice plainly over a triggered hook", async ({ expect, }) => { diff --git a/web/app/components/editor/hooks/HookConfigPanel.vue b/web/app/components/editor/hooks/HookConfigPanel.vue index 8cf651dc..f2da7016 100644 --- a/web/app/components/editor/hooks/HookConfigPanel.vue +++ b/web/app/components/editor/hooks/HookConfigPanel.vue @@ -34,6 +34,7 @@ const editorStore = useEditorStore() const { isEditable } = useEditorMeta() const status = computed(() => (props.hook ? hookStatus(props.hook) : null)) +const isSettingUp = computed(() => props.hook?.status === "initializing") const isRemoved = computed(() => props.diff?.status === DiffStatus.Removed) const mode = computed(() => { if (editorStore.reviewableDiffActive) { @@ -100,6 +101,13 @@ const showsDismissHint = computed( >
+

+ + {{ $t("editor.hooks.initializing") }} +

{ input: TRIGGERED, expected: "triggered", }, + { + name: "shows a copy being set up by its source's score", + input: { score: "0", status: "initializing" }, + expected: "triggered", + }, { name: "shows an unreachable website as needing attention", input: FAILED, diff --git a/web/app/components/editor/hooks/hook-status.ts b/web/app/components/editor/hooks/hook-status.ts index 38a182c3..32f2499a 100644 --- a/web/app/components/editor/hooks/hook-status.ts +++ b/web/app/components/editor/hooks/hook-status.ts @@ -39,9 +39,10 @@ export const HOOK_BAR_STATUS_CLASS: Record< } // hookStatus puts a failed check first. A hook that cannot check what it -// watches needs fixing before its trigger means anything. +// watches needs fixing before its trigger means anything. A copy still +// being set up keeps the score its source had. export function hookStatus(hook: DocumentHook): HookStatus { - if (hook.status !== "active") { + if (hook.status !== "active" && hook.status !== "initializing") { return "needs-attention" } diff --git a/web/app/components/editor/hooks/url-watcher/ConfigMenu.vue b/web/app/components/editor/hooks/url-watcher/ConfigMenu.vue index a78e9a57..7984a540 100644 --- a/web/app/components/editor/hooks/url-watcher/ConfigMenu.vue +++ b/web/app/components/editor/hooks/url-watcher/ConfigMenu.vue @@ -115,6 +115,7 @@ function checkFailure( ): { detail: string; notice: string } | null { switch (checkStatus) { case "active": + case "initializing": return null case "unconfigured": return { diff --git a/web/app/utils/api/document.ts b/web/app/utils/api/document.ts index 731eb267..508ccef7 100644 --- a/web/app/utils/api/document.ts +++ b/web/app/utils/api/document.ts @@ -120,19 +120,21 @@ export type DocumentHookStatus = | DocumentHookStatusURLWatcher | DocumentHookStatusContainerImageWatcher -// a status other than active names why the last check failed -export type DocumentHookStatusScheduledReminder = "active" +// initializing marks a copy its first run has not set up yet. Any other +// status than active names why the last check failed. +export type DocumentHookStatusScheduledReminder = "active" | "initializing" export type DocumentHookStatusGitHubTracking = | "active" + | "initializing" | "unconfigured" | "missing_installation" | "missing_repository" | "missing_branch" | "tree_truncated" export type DocumentHookStatusURLWatcher = - "active" | "unconfigured" | "unreachable_url" + "active" | "initializing" | "unconfigured" | "unreachable_url" export type DocumentHookStatusContainerImageWatcher = - "active" | "unauthorized" | "image_not_found" + "active" | "initializing" | "unauthorized" | "image_not_found" export interface DocumentHookSettingsScheduledReminder { scale: "linear" diff --git a/web/i18n/locales/en/editor.json b/web/i18n/locales/en/editor.json index dfca52e8..45c0aa02 100644 --- a/web/i18n/locales/en/editor.json +++ b/web/i18n/locales/en/editor.json @@ -398,6 +398,7 @@ "reset-failed": "Failed to reset the hook. Please try again.", "renew-failed": "Failed to renew the hook. Please try again." }, + "initializing": "Initializing this hook. It will be ready in a moment.", "scheduled-reminder": { "title": "Scheduled Reminder", "description": "Nudge maintainers on a date",