diff --git a/server/auth-realtime/src/core.test.ts b/server/auth-realtime/src/core.test.ts index b3202915..b340dfa3 100644 --- a/server/auth-realtime/src/core.test.ts +++ b/server/auth-realtime/src/core.test.ts @@ -363,6 +363,30 @@ describe("createCoreClient", () => { { headers }, ) }) + + it.for([ + { name: "the current", input: "." }, + { name: "the parent", input: ".." }, + ])( + "refuses an id naming $name path", + async ({ input }, { expect }) => { + const http = stubHttp({ status: 200, data: {} }) + + await expect( + createCoreClient( + BASE_URL, + http, + ).updateBranch( + "doc-1", + input, + { body: {} }, + { headers: new AxiosHeaders() }, + ), + ).rejects.toThrow(`invalid path id "${input}"`) + + expect(http.put).not.toHaveBeenCalled() + }, + ) }) describe("deleteBranch", () => { diff --git a/server/auth-realtime/src/core.ts b/server/auth-realtime/src/core.ts index 8a3f9321..64e9e32f 100644 --- a/server/auth-realtime/src/core.ts +++ b/server/auth-realtime/src/core.ts @@ -238,10 +238,24 @@ export function createCoreClient( } } -// urlOn returns a template tag that appends a path to base, encoding each -// interpolated id so one holding "/" or ".." stays in its own segment. +// urlOn returns a template tag that appends a path to base. Each +// interpolated id is encoded, so one holding "/" stays in its own segment. +// Encoding keeps dots, so an id of "." or ".." would name another path and +// is refused. function urlOn(base: string) { - return (strings: TemplateStringsArray, ...ids: string[]): string => - base + - String.raw({ raw: strings }, ...ids.map(encodeURIComponent)) + return (strings: TemplateStringsArray, ...ids: string[]): string => { + for (const id of ids) { + if (id === "." || id === "..") { + throw new Error(`invalid path id "${id}"`) + } + } + + return ( + base + + String.raw( + { raw: strings }, + ...ids.map(encodeURIComponent), + ) + ) + } } diff --git a/server/auth-realtime/src/routes.ts b/server/auth-realtime/src/routes.ts index 5e9caa16..03433e5a 100644 --- a/server/auth-realtime/src/routes.ts +++ b/server/auth-realtime/src/routes.ts @@ -663,14 +663,14 @@ export function createRoutes({ app.post( "/internal/documents/:documentId/branches/:branchId/flush", async (c) => { - const branch = await flushed(c, () => + const flush = await flushed(c, () => flushBranch( c.req.param("documentId"), c.req.param("branchId"), ), ) - if (branch instanceof Response) { - return branch + if (flush instanceof Response) { + return flush } return c.body(null, 204) diff --git a/server/core/internal/apps/registry/client.go b/server/core/internal/apps/registry/client.go index 86aaa2d3..def84a41 100644 --- a/server/core/internal/apps/registry/client.go +++ b/server/core/internal/apps/registry/client.go @@ -21,9 +21,6 @@ var ( // ErrNotFound is returned when the registry has no such image or tag. ErrNotFound = errutil.New(http.StatusNotFound, "registry.not_found", "container image not found") - - // ErrInvalidReference is returned when an image reference cannot be parsed. - ErrInvalidReference = errutil.New(http.StatusBadRequest, "registry.invalid_reference", "invalid container image reference") ) // digestOptions holds options for configuring the Digest function. @@ -130,7 +127,7 @@ func parseReference(image string) (name.Reference, error) { name.WithDefaultTag("latest"), ) if err != nil { - return nil, ErrInvalidReference + return nil, fmt.Errorf("parsing image reference %q: %w", image, err) } return ref, nil diff --git a/server/core/internal/apps/registry/client_test.go b/server/core/internal/apps/registry/client_test.go index ffcba654..527f5b3d 100644 --- a/server/core/internal/apps/registry/client_test.go +++ b/server/core/internal/apps/registry/client_test.go @@ -13,6 +13,7 @@ import ( "github.com/google/go-containerregistry/pkg/registry" "github.com/google/go-containerregistry/pkg/v1/random" "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -82,11 +83,12 @@ func Test_Digest(t *testing.T) { Path string Opts []DigestOption ExpectedErr string + ErrContains string }{ "Anonymous fetch returns the seeded digest": {}, "Invalid image reference": { Reference: "UPPERCASE not allowed", - ExpectedErr: ErrInvalidReference.Error(), + ErrContains: "parsing image reference", }, "Missing tag maps to ErrNotFound": { Path: "/test/img:v2", @@ -138,6 +140,13 @@ func Test_Digest(t *testing.T) { digest, err := Digest(context.Background(), reference, tc.Opts...) + if tc.ErrContains != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.ErrContains) + + return + } + if tc.ExpectedErr != "" { require.Error(t, err) assert.EqualError(t, err, tc.ExpectedErr) @@ -154,7 +163,27 @@ func Test_Digest(t *testing.T) { func Test_ValidateReference(t *testing.T) { t.Parallel() - assert.NoError(t, ValidateReference("nginx")) - assert.NoError(t, ValidateReference("ghcr.io/owner/image:v1")) - assert.ErrorIs(t, ValidateReference("UPPERCASE not allowed"), ErrInvalidReference) + cc := map[string]struct { + Image string + Err error + }{ + "Invalid reference": { + Image: "UPPERCASE not allowed", + Err: assert.AnError, + }, + "Docker Hub image": { + Image: "nginx", + }, + "Image of another registry": { + Image: "ghcr.io/owner/image:v1", + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, ValidateReference(c.Image)) + }) + } } diff --git a/server/core/internal/assistant/AGENTS.md b/server/core/internal/assistant/AGENTS.md index 5e91f8f1..cd1ad47f 100644 --- a/server/core/internal/assistant/AGENTS.md +++ b/server/core/internal/assistant/AGENTS.md @@ -36,11 +36,9 @@ for free. tag-only writes record none). - **Tag and hook writes notify**: tag tools end with `Input.NotifyTagTreeChange` (`assign_tag`/`unassign_tag` also - `NotifyBranchTagsChange`), hook writes call `NotifyHooksChange`, wired in - `server.NewServer` beside the tree notifier. Hook writes drive the same - `hook.NewHook`/`ApplyUpdate`/`Reset`/`Delete` lifecycle as the HTTP - handler; `decodeHookSettings` switches on type into the processor's own - struct. + `NotifyBranchTagsChange`). Hook writes go through `HookManager`, which + records history and announces the change, as for the HTTP handler; + `decodeHookSettings` switches on type into the processor's own struct. - **A block write reports from the operation, never a re-read** (the realtime service persists on a debounce): `sanitizeBlock` once, ship the canonical uids, return `blockRows` of the expanded tree. diff --git a/server/core/internal/assistant/tools/0mock_hook_manager_test.go b/server/core/internal/assistant/tools/0mock_hook_manager_test.go index a9b7cef0..fede6db6 100644 --- a/server/core/internal/assistant/tools/0mock_hook_manager_test.go +++ b/server/core/internal/assistant/tools/0mock_hook_manager_test.go @@ -24,13 +24,13 @@ var _ HookManager = &HookManagerMock{} // CreateHookFunc: func(ctx context.Context, ci hook.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hook.Hook, error) { // panic("mock out the CreateHook method") // }, -// DeleteHookFunc: func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +// DeleteHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { // panic("mock out the DeleteHook method") // }, -// ResetHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) { +// ResetHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hook.Hook, error) { // panic("mock out the ResetHook method") // }, -// UpdateHookFunc: func(ctx context.Context, id xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) { +// UpdateHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) { // panic("mock out the UpdateHook method") // }, // } @@ -44,13 +44,13 @@ type HookManagerMock struct { CreateHookFunc func(ctx context.Context, ci hook.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hook.Hook, error) // DeleteHookFunc mocks the DeleteHook method. - DeleteHookFunc func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error + DeleteHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error // ResetHookFunc mocks the ResetHook method. - ResetHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) + ResetHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hook.Hook, error) // UpdateHookFunc mocks the UpdateHook method. - UpdateHookFunc func(ctx context.Context, id xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) + UpdateHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) // calls tracks calls to the methods. calls struct { @@ -73,6 +73,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UpdatedBy is the updatedBy argument value. @@ -84,6 +86,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string } @@ -93,6 +97,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UI is the ui argument value. @@ -160,15 +166,17 @@ func (mock *HookManagerMock) CreateHookCalls() []struct { } // DeleteHook calls DeleteHookFunc. -func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UpdatedBy: updatedBy, } @@ -181,7 +189,7 @@ func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organiza ) return errOut } - return mock.DeleteHookFunc(ctx, id, organizationID, updatedBy) + return mock.DeleteHookFunc(ctx, id, documentID, organizationID, updatedBy) } // DeleteHookCalls gets all the calls that were made to DeleteHook. @@ -191,12 +199,14 @@ func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organiza func (mock *HookManagerMock) DeleteHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } @@ -207,14 +217,16 @@ func (mock *HookManagerMock) DeleteHookCalls() []struct { } // ResetHook calls ResetHookFunc. -func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) { +func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hook.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, } mock.lockResetHook.Lock() @@ -227,7 +239,7 @@ func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizat ) return hookOut, errOut } - return mock.ResetHookFunc(ctx, id, organizationID) + return mock.ResetHookFunc(ctx, id, documentID, organizationID) } // ResetHookCalls gets all the calls that were made to ResetHook. @@ -237,11 +249,13 @@ func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizat func (mock *HookManagerMock) ResetHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } mock.lockResetHook.RLock() @@ -251,16 +265,18 @@ func (mock *HookManagerMock) ResetHookCalls() []struct { } // UpdateHook calls UpdateHookFunc. -func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) { +func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hook.UpdateInput UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UI: ui, UpdatedBy: updatedBy, @@ -275,7 +291,7 @@ func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organiza ) return hookOut, errOut } - return mock.UpdateHookFunc(ctx, id, organizationID, ui, updatedBy) + return mock.UpdateHookFunc(ctx, id, documentID, organizationID, ui, updatedBy) } // UpdateHookCalls gets all the calls that were made to UpdateHook. @@ -285,6 +301,7 @@ func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organiza func (mock *HookManagerMock) UpdateHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hook.UpdateInput UpdatedBy string @@ -292,6 +309,7 @@ func (mock *HookManagerMock) UpdateHookCalls() []struct { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hook.UpdateInput UpdatedBy string diff --git a/server/core/internal/assistant/tools/hook_test.go b/server/core/internal/assistant/tools/hook_test.go index fd4160d2..a9456f3b 100644 --- a/server/core/internal/assistant/tools/hook_test.go +++ b/server/core/internal/assistant/tools/hook_test.go @@ -58,7 +58,7 @@ func stubHookManager(stored ...*hook.Hook) *HookManagerMock { return hook.NewHook(ctx, ci, documentID, ci.BranchID, organizationID, inp(organizationID)) }, - UpdateHookFunc: func(ctx context.Context, _ xid.ID, organizationID string, ui hook.UpdateInput, _ string) (*hook.Hook, error) { + UpdateHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID string, ui hook.UpdateInput, _ string) (*hook.Hook, error) { hk := lookup() if err := hk.ApplyUpdate(ctx, ui, inp(organizationID)); err != nil { return nil, err @@ -66,10 +66,10 @@ func stubHookManager(stored ...*hook.Hook) *HookManagerMock { return hk, nil }, - DeleteHookFunc: func(ctx context.Context, _ xid.ID, organizationID, _ string) error { + DeleteHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID, _ string) error { return lookup().Delete(ctx, inp(organizationID)) }, - ResetHookFunc: func(ctx context.Context, _ xid.ID, organizationID string) (*hook.Hook, error) { + ResetHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID string) (*hook.Hook, error) { hk := lookup() if err := hk.Reset(ctx, inp(organizationID)); err != nil { return nil, err @@ -703,7 +703,7 @@ func Test_updateHook_Execute(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.UpdateHookFunc = func(context.Context, xid.ID, string, hook.UpdateInput, string) (*hook.Hook, error) { + failing.UpdateHookFunc = func(context.Context, xid.ID, xid.ID, string, hook.UpdateInput, string) (*hook.Hook, error) { return nil, assert.AnError } @@ -863,7 +863,7 @@ func Test_resetHook_Execute(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.ResetHookFunc = func(context.Context, xid.ID, string) (*hook.Hook, error) { + failing.ResetHookFunc = func(context.Context, xid.ID, xid.ID, string) (*hook.Hook, error) { return nil, assert.AnError } @@ -976,7 +976,7 @@ func Test_deleteHook_Execute(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.DeleteHookFunc = func(context.Context, xid.ID, string, string) error { + failing.DeleteHookFunc = func(context.Context, xid.ID, xid.ID, string, string) error { return assert.AnError } diff --git a/server/core/internal/assistant/tools/input.go b/server/core/internal/assistant/tools/input.go index 97a437e1..f20eb5e5 100644 --- a/server/core/internal/assistant/tools/input.go +++ b/server/core/internal/assistant/tools/input.go @@ -1033,11 +1033,9 @@ func (i *input) unknownHook(documentID, hookID xid.ID) error { } // CreateHook creates a hook on the branch branchID names, anchored to the -// block blockUID names when one is given. The branch and block are -// checked here rather than by the caller, so a hook cannot land on a -// branch of another document or on a block the branch does not hold. A -// type whose integration the deployment lacks is refused before anything -// is created. +// block blockUID names when one is given. The branch is checked here, so +// a hook cannot land on a branch of another document. A type whose +// integration the deployment lacks is refused before anything is created. func (i *input) CreateHook(documentID, branchID xid.ID, blockUID string, tp hook.Type, settings processor.Settings) (*hook.Hook, error) { doc, err := i.FetchBranch(documentID, branchID) if err != nil { @@ -1081,7 +1079,7 @@ func (i *input) CreateHook(documentID, branchID xid.ID, blockUID string, tp hook // UpdateHook replaces the hook's settings and resets its score and state, // as a fresh hook with those settings would have them. func (i *input) UpdateHook(hk *hook.Hook, settings processor.Settings) error { - updated, err := i.hookMan.UpdateHook(i.ctx, hk.ID, i.orgID, hook.UpdateInput{Settings: settings}, i.userID) + updated, err := i.hookMan.UpdateHook(i.ctx, hk.ID, hk.DocumentID.V, i.orgID, hook.UpdateInput{Settings: settings}, i.userID) if err != nil { return err } @@ -1095,7 +1093,7 @@ func (i *input) UpdateHook(hk *hook.Hook, settings processor.Settings) error { // ResetHook restores the hook's score and state, keeping its settings. func (i *input) ResetHook(hk *hook.Hook) error { - reset, err := i.hookMan.ResetHook(i.ctx, hk.ID, i.orgID) + reset, err := i.hookMan.ResetHook(i.ctx, hk.ID, hk.DocumentID.V, i.orgID) if err != nil { return err } @@ -1111,7 +1109,7 @@ func (i *input) ResetHook(hk *hook.Hook) error { // removes its row. The branch stays, and the editor showing it has to // redraw, so the delete records the branch as touched. func (i *input) DeleteHook(hk *hook.Hook) error { - if err := i.hookMan.DeleteHook(i.ctx, hk.ID, i.orgID, i.userID); err != nil { + if err := i.hookMan.DeleteHook(i.ctx, hk.ID, hk.DocumentID.V, i.orgID, i.userID); err != nil { return err } @@ -1289,17 +1287,17 @@ type HookManager interface { // credited to updatedBy. CreateHook(ctx context.Context, ci hook.CreateInput, documentID xid.ID, organizationID, updatedBy string) (*hook.Hook, error) - // UpdateHook should replace the hook's settings, credited to - // updatedBy, and return the stored hook. - UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) + // UpdateHook should replace the settings of the document's hook, + // credited to updatedBy, and return the stored hook. + UpdateHook(ctx context.Context, id, documentID xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string) (*hook.Hook, error) - // DeleteHook should tear the hook down and remove it, credited to - // updatedBy. - DeleteHook(ctx context.Context, id xid.ID, organizationID, updatedBy string) error + // DeleteHook should tear the document's hook down and remove it, + // credited to updatedBy. + DeleteHook(ctx context.Context, id, documentID xid.ID, organizationID, updatedBy string) error - // ResetHook should restore the hook's score and state and return the - // stored hook. - ResetHook(ctx context.Context, id xid.ID, organizationID string) (*hook.Hook, error) + // ResetHook should reset the document's hook and return the stored + // hook. + ResetHook(ctx context.Context, id, documentID xid.ID, organizationID string) (*hook.Hook, error) } // SearchTrigger runs the search-job worker once a job has committed. diff --git a/server/core/internal/assistant/tools/input_test.go b/server/core/internal/assistant/tools/input_test.go index 72374745..ee3562d1 100644 --- a/server/core/internal/assistant/tools/input_test.go +++ b/server/core/internal/assistant/tools/input_test.go @@ -2530,7 +2530,7 @@ func Test_input_UpdateHook(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.UpdateHookFunc = func(context.Context, xid.ID, string, hook.UpdateInput, string) (*hook.Hook, error) { + failing.UpdateHookFunc = func(context.Context, xid.ID, xid.ID, string, hook.UpdateInput, string) (*hook.Hook, error) { return nil, assert.AnError } @@ -2602,7 +2602,7 @@ func Test_input_ResetHook(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.ResetHookFunc = func(context.Context, xid.ID, string) (*hook.Hook, error) { + failing.ResetHookFunc = func(context.Context, xid.ID, xid.ID, string) (*hook.Hook, error) { return nil, assert.AnError } @@ -2669,7 +2669,7 @@ func Test_input_DeleteHook(t *testing.T) { t.Parallel() failing := stubHookManager() - failing.DeleteHookFunc = func(context.Context, xid.ID, string, string) error { + failing.DeleteHookFunc = func(context.Context, xid.ID, xid.ID, string, string) error { return assert.AnError } diff --git a/server/core/internal/db/migrations/002_history_entry_snapshot.sql b/server/core/internal/db/migrations/002_history_entry_snapshot.sql index cd998535..83cf176b 100644 --- a/server/core/internal/db/migrations/002_history_entry_snapshot.sql +++ b/server/core/internal/db/migrations/002_history_entry_snapshot.sql @@ -1,5 +1,9 @@ -- +migrate Up +-- History entries become restorable snapshots: they carry the branch +-- name, icon, author and hook definitions and drop the Yjs binary. The +-- existing rows predate that shape and nothing reads them, so they are +-- dropped rather than backfilled. TRUNCATE document_branch_history_entries; ALTER TABLE document_branch_history_entries @@ -11,6 +15,8 @@ ALTER TABLE document_branch_history_entries ADD COLUMN boundary BOOLEAN NOT NULL; CREATE INDEX document_branch_history_entries_fk_last_updated_by_idx ON document_branch_history_entries (fk_last_updated_by); +-- Entries are read per branch, newest first, and expired by age; the +-- composite index also covers the plain branch lookup it replaces. DROP INDEX document_branch_history_entries_fk_branch_id_idx; CREATE INDEX document_branch_history_entries_fk_branch_id_created_at_idx ON document_branch_history_entries (fk_branch_id, created_at DESC, id DESC); CREATE INDEX document_branch_history_entries_created_at_idx ON document_branch_history_entries (created_at); diff --git a/server/core/internal/document/hook/hook.go b/server/core/internal/document/hook/hook.go index 4beec7ec..ec8c0137 100644 --- a/server/core/internal/document/hook/hook.go +++ b/server/core/internal/document/hook/hook.go @@ -31,6 +31,10 @@ var ErrUpstreamUnavailable = errutil.New(http.StatusFailedDependency, "document_ // another document. var ErrBranchMismatch = errutil.New(http.StatusNotFound, "document.branch_mismatch", "branch does not belong to the document") +// ErrHookMismatch is returned when a hook is addressed under a document it +// does not belong to. +var ErrHookMismatch = errutil.New(http.StatusNotFound, "document.hook_mismatch", "hook does not belong to the document") + // ErrBlockNotFound is returned when a hook is to be anchored to a block the // branch's content does not hold. var ErrBlockNotFound = errutil.New(http.StatusNotFound, "document.hook_block_not_found", "block not found in the branch") @@ -317,12 +321,7 @@ func (h *Hook) setUp(ctx context.Context, inp *Input) error { return nil } - return errutil.New( - http.StatusUnprocessableEntity, - "document_hook."+string(h.Status), - "the hook cannot check its target: %s", - h.Status, - ) + return newStatusError(h.Status) } // ensurePrepared prepares the hook for processing. @@ -403,6 +402,31 @@ type UpdateInput struct { Settings processor.Settings `json:"settings"` } +// newStatusError returns the error a hook is refused with when its check +// failed with the given status. +func newStatusError(status processor.Status) error { + switch status { + case processor.StatusUnconfigured: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.unconfigured", "the integration the hook needs is not configured") + case processor.StatusMissingInstallation: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_installation", "the organization has no github app installation") + case processor.StatusMissingRepository: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_repository", "the github repository was not found") + case processor.StatusMissingBranch: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_branch", "the github branch was not found") + case processor.StatusTreeTruncated: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.tree_truncated", "the github repository is too large to compare") + case processor.StatusUnreachableURL: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.unreachable_url", "the url cannot be reached") + case processor.StatusUnauthorized: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.unauthorized", "the container registry refused access") + case processor.StatusImageNotFound: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.image_not_found", "the container image was not found") + default: + return errutil.New(http.StatusUnprocessableEntity, "document_hook.cannot_check", "the hook cannot check its target") + } +} + // runner is an interface that defines the methods for applying a freshness hook. type runner interface { // Validate checks the settings without reaching any service. diff --git a/server/core/internal/document/hook/hook_test.go b/server/core/internal/document/hook/hook_test.go index 118f1ff0..ffa42136 100644 --- a/server/core/internal/document/hook/hook_test.go +++ b/server/core/internal/document/hook/hook_test.go @@ -123,7 +123,7 @@ func Test_NewHook(t *testing.T) { }, documentID, branchID, "org-1", NewInput("org-1", nil, webchange.NewClient("", ""))) require.Error(t, err) assert.Equal(t, http.StatusUnprocessableEntity, errutil.StatusCode(err, false)) - assert.EqualError(t, err, "the hook cannot check its target: unconfigured") + assert.EqualError(t, err, "the integration the hook needs is not configured") }) } @@ -417,6 +417,60 @@ func Test_Hook_ensurePrepared(t *testing.T) { } } +func Test_newStatusError(t *testing.T) { + t.Parallel() + + cc := map[string]struct { + Status processor.Status + Err error + }{ + "Unconfigured": { + Status: processor.StatusUnconfigured, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unconfigured", "the integration the hook needs is not configured"), + }, + "Missing installation": { + Status: processor.StatusMissingInstallation, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_installation", "the organization has no github app installation"), + }, + "Missing repository": { + Status: processor.StatusMissingRepository, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_repository", "the github repository was not found"), + }, + "Missing branch": { + Status: processor.StatusMissingBranch, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.missing_branch", "the github branch was not found"), + }, + "Tree truncated": { + Status: processor.StatusTreeTruncated, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.tree_truncated", "the github repository is too large to compare"), + }, + "Unreachable url": { + Status: processor.StatusUnreachableURL, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unreachable_url", "the url cannot be reached"), + }, + "Unauthorized": { + Status: processor.StatusUnauthorized, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unauthorized", "the container registry refused access"), + }, + "Image not found": { + Status: processor.StatusImageNotFound, + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.image_not_found", "the container image was not found"), + }, + "Unknown status": { + Status: processor.Status("bogus"), + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.cannot_check", "the hook cannot check its target"), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, newStatusError(c.Status)) + }) + } +} + func Test_Type_Validate(t *testing.T) { t.Parallel() diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index bc1727c1..213f13b1 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -251,7 +251,7 @@ func (m *Manager) runHook(ctx context.Context, doc *document.Document, h *hook.H // The mark is still stored, since it starts the retention clock. if !h.SoftDeletedAt.Valid { if err := h.Process(ctx, m.input(h.OrganizationID.String)); err != nil { - // a transient failure keeps the stored state. + // a failed run keeps the stored score and state. m.log.With("hook_id", h.ID). With("error", err). Error("processing document hook") diff --git a/server/core/internal/document/hook/manager/write.go b/server/core/internal/document/hook/manager/write.go index d9ac14ee..fd723d39 100644 --- a/server/core/internal/document/hook/manager/write.go +++ b/server/core/internal/document/hook/manager/write.go @@ -2,11 +2,14 @@ package manager import ( "context" + "errors" "fmt" + "log/slog" "net/http" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/rs/xid" ) @@ -50,16 +53,7 @@ func (m *Manager) CreateHook( hk, err := hook.NewHook(ctx, ci, documentID, ci.BranchID, organizationID, m.input(organizationID)) if err != nil { - // a refusal the hook reports passes through. A service it could - // not reach is logged here, since the caller only sees the - // sentinel. - if errutil.StatusCode(err, false) >= http.StatusInternalServerError { - m.log.With("error", err).Warn("cannot reach the service a hook checks") - - return nil, hook.ErrUpstreamUnavailable - } - - return nil, err + return nil, m.hookErr(err) } defer func() { @@ -95,12 +89,13 @@ func (m *Manager) CreateHook( // UpdateHook applies the new settings, which resets the hook's state, then // stores the hook and records its branch in history, credited to -// updatedBy. It fails unless the hook can check its target with the new -// settings. The outside calls run before the transaction opens, so no -// connection waits on them. +// updatedBy. It fails unless the hook belongs to the document and can +// check its target with the new settings. The outside calls run before the +// transaction opens, so no connection waits on them. func (m *Manager) UpdateHook( ctx context.Context, id xid.ID, + documentID xid.ID, organizationID string, ui hook.UpdateInput, updatedBy string, @@ -120,7 +115,13 @@ func (m *Manager) UpdateHook( return nil, err } - if hk.DocumentID.Valid && hk.BranchID.Valid { + // a row whose document was deleted carries no document id and is not + // addressable through any document. + if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { + return nil, hook.ErrHookMismatch + } + + if hk.BranchID.Valid { if err = m.flusher.Flush(ctx, hk.DocumentID.V, hk.BranchID.V); err != nil { return nil, fmt.Errorf("storing the branch's pending edits: %w", err) } @@ -135,16 +136,7 @@ func (m *Manager) UpdateHook( }() if err = hk.ApplyUpdate(ctx, ui, m.input(organizationID)); err != nil { - // a refusal the hook reports passes through. A service it could - // not reach is logged here, since the caller only sees the - // sentinel. - if errutil.StatusCode(err, false) >= http.StatusInternalServerError { - m.log.With("error", err).Warn("cannot reach the service a hook checks") - - return nil, hook.ErrUpstreamUnavailable - } - - return nil, err + return nil, m.hookErr(err) } var tx Tx @@ -175,8 +167,9 @@ func (m *Manager) UpdateHook( } // DeleteHook tears down the hook's external resource, then removes the row -// and records its branch in history, credited to updatedBy. -func (m *Manager) DeleteHook(ctx context.Context, id xid.ID, organizationID, updatedBy string) error { +// and records its branch in history, credited to updatedBy. It fails unless +// the hook belongs to the document. +func (m *Manager) DeleteHook(ctx context.Context, id, documentID xid.ID, organizationID, updatedBy string) error { ctx, cancel := context.WithTimeout(ctx, _hookTimeout) defer cancel() @@ -192,23 +185,20 @@ func (m *Manager) DeleteHook(ctx context.Context, id xid.ID, organizationID, upd return err } - if hk.DocumentID.Valid && hk.BranchID.Valid { + // a row whose document was deleted carries no document id and is not + // addressable through any document. + if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { + return hook.ErrHookMismatch + } + + if hk.BranchID.Valid { if err = m.flusher.Flush(ctx, hk.DocumentID.V, hk.BranchID.V); err != nil { return fmt.Errorf("storing the branch's pending edits: %w", err) } } if err = hk.Delete(ctx, m.input(organizationID)); err != nil { - // a refusal the hook reports passes through. A service it could - // not reach is logged here, since the caller only sees the - // sentinel. - if errutil.StatusCode(err, false) >= http.StatusInternalServerError { - m.log.With("error", err).Warn("cannot reach the service a hook checks") - - return hook.ErrUpstreamUnavailable - } - - return err + return m.hookErr(err) } var tx Tx @@ -238,10 +228,11 @@ func (m *Manager) DeleteHook(ctx context.Context, id xid.ID, organizationID, upd return nil } -// ResetHook restores the hook's score and state and stores them. History -// holds no watcher state, so nothing is recorded. A hook that cannot check -// its target is stored with that status, and its maintainers are told. -func (m *Manager) ResetHook(ctx context.Context, id xid.ID, organizationID string) (_ *hook.Hook, err error) { +// ResetHook takes the target as it is now as the hook's new baseline and +// stores it. History holds no watcher state, so nothing is recorded. A hook +// that cannot check its target is stored with that status, and its +// maintainers are told. It fails unless the hook belongs to the document. +func (m *Manager) ResetHook(ctx context.Context, id, documentID xid.ID, organizationID string) (_ *hook.Hook, err error) { ctx, cancel := context.WithTimeout(ctx, _hookTimeout) defer cancel() @@ -257,6 +248,12 @@ func (m *Manager) ResetHook(ctx context.Context, id xid.ID, organizationID strin return nil, err } + // a row whose document was deleted carries no document id and is not + // addressable through any document. + if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { + return nil, hook.ErrHookMismatch + } + prev := *hk defer func() { @@ -266,16 +263,7 @@ func (m *Manager) ResetHook(ctx context.Context, id xid.ID, organizationID strin }() if err = hk.Reset(ctx, m.input(organizationID)); err != nil { - // a refusal the hook reports passes through. A service it could - // not reach is logged here, since the caller only sees the - // sentinel. - if errutil.StatusCode(err, false) >= http.StatusInternalServerError { - m.log.With("error", err).Warn("cannot reach the service a hook checks") - - return nil, hook.ErrUpstreamUnavailable - } - - return nil, err + return nil, m.hookErr(err) } if err = m.db.UpdateDocumentHook(ctx, *hk); err != nil { @@ -287,3 +275,17 @@ func (m *Manager) ResetHook(ctx context.Context, id xid.ID, organizationID strin return hk, nil } + +// hookErr returns a hook's error as a write's caller sees it. A service +// the hook could not reach becomes ErrUpstreamUnavailable, logged here +// since the caller sees only the sentinel. A refusal with its own status +// and any other error pass through. +func (m *Manager) hookErr(err error) error { + if !errors.Is(err, processor.ErrUnreachable) || errutil.StatusCode(err, false) < http.StatusInternalServerError { + return err + } + + m.log.Warn("cannot reach the service a hook checks", slog.String("error", err.Error())) + + return hook.ErrUpstreamUnavailable +} diff --git a/server/core/internal/document/hook/manager/write_test.go b/server/core/internal/document/hook/manager/write_test.go index 7c069e1a..820a741f 100644 --- a/server/core/internal/document/hook/manager/write_test.go +++ b/server/core/internal/document/hook/manager/write_test.go @@ -111,7 +111,7 @@ func Test_Manager_CreateHook(t *testing.T) { Type: hook.TypeURLWatcher, Settings: processor.Settings(`{"url":"https://example.com"}`), Tx: &TxMock{}, - Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unconfigured", "the hook cannot check its target: %s", processor.StatusUnconfigured), + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unconfigured", "the integration the hook needs is not configured"), }, "Service the hook checks is unreachable": { Type: hook.TypeURLWatcher, @@ -156,8 +156,7 @@ func Test_Manager_CreateHook(t *testing.T) { Watchers: []string{"w-new"}, Err: assert.AnError, }, - // the insert is rolled back, so the watcher the hook created is torn - // down again. + // the hook was not stored, so the watcher it created is torn down. "Error returned by tx.InsertDocumentHook": { Type: hook.TypeURLWatcher, Settings: processor.Settings(`{"url":"https://example.com"}`), @@ -290,20 +289,21 @@ func Test_Manager_UpdateHook(t *testing.T) { cc := map[string]struct { // Held has another write hold the hook's lock until the update // gives up. - Held bool - Branchless bool - Unreachable bool - Settings processor.Settings - FetchErr error - FlushErr error - BeginErr error - UpdateErr error - RecordErr error - CommitErr error - Updates int - Records int - Watchers []string - Err error + Held bool + Branchless bool + OtherDocument bool + Unreachable bool + Settings processor.Settings + FetchErr error + FlushErr error + BeginErr error + UpdateErr error + RecordErr error + CommitErr error + Updates int + Records int + Watchers []string + Err error }{ "Hook another write holds": { Held: true, @@ -315,6 +315,11 @@ func Test_Manager_UpdateHook(t *testing.T) { FetchErr: sql.ErrNoRows, Err: sql.ErrNoRows, }, + "Hook of another document": { + Settings: settings, + OtherDocument: true, + Err: hook.ErrHookMismatch, + }, "Error returned by flusher.Flush": { Settings: settings, FlushErr: assert.AnError, @@ -424,7 +429,12 @@ func Test_Manager_UpdateHook(t *testing.T) { defer cancel() } - hk, err := man.UpdateHook(ctx, stored.ID, "org-1", hook.UpdateInput{Settings: c.Settings}, "u1") + documentID := stored.DocumentID.V + if c.OtherDocument { + documentID = xid.New() + } + + hk, err := man.UpdateHook(ctx, stored.ID, documentID, "org-1", hook.UpdateInput{Settings: c.Settings}, "u1") testutil.AssertEqualError(t, c.Err, err) assert.Len(t, tx.UpdateDocumentHookCalls(), c.Updates) @@ -468,7 +478,10 @@ func Test_Manager_DeleteHook(t *testing.T) { branchID := xid.New() cc := map[string]struct { - Hook func(*testing.T) hook.Hook + Hook func(*testing.T) hook.Hook + OtherDocument bool + // CDURL points changedetection at this address. + CDURL string FetchErr error FlushErr error BeginErr error @@ -486,6 +499,13 @@ func Test_Manager_DeleteHook(t *testing.T) { FetchErr: sql.ErrNoRows, Err: sql.ErrNoRows, }, + "Hook of another document": { + Hook: func(t *testing.T) hook.Hook { + return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + }, + OtherDocument: true, + Err: hook.ErrHookMismatch, + }, "Error returned by flusher.Flush": { Hook: func(t *testing.T) hook.Hook { return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) @@ -495,14 +515,22 @@ func Test_Manager_DeleteHook(t *testing.T) { }, // the row goes only once the resource it describes is gone, so a // failed teardown keeps it. - "External teardown fails": { + "Unreachable changedetection keeps the hook": { + Hook: func(*testing.T) hook.Hook { + return urlWatcherHook(branchID) + }, + CDURL: "http://127.0.0.1:1", + Err: hook.ErrUpstreamUnavailable, + }, + // a stored state that does not decode is no service's failure. + "Malformed state keeps the hook": { Hook: func(*testing.T) hook.Hook { hk := urlWatcherHook(branchID) hk.State = null.ValueFrom(processor.State(`{`)) return hk }, - Err: hook.ErrUpstreamUnavailable, + Err: assert.AnError, }, "Error returned by db.BeginTx": { Hook: func(t *testing.T) hook.Hook { @@ -583,7 +611,13 @@ func Test_Manager_DeleteHook(t *testing.T) { }, } - man := newTestManager(t, stubStoredHook(stubDB(&DBMock{}, tx, c.BeginErr), hk, c.FetchErr), &fakePublisher{}, nil) + var wc *webchange.Client + + if c.CDURL != "" { + wc = webchange.NewClient(c.CDURL, "key") + } + + man := newTestManager(t, stubStoredHook(stubDB(&DBMock{}, tx, c.BeginErr), hk, c.FetchErr), &fakePublisher{}, wc) if c.FlushErr != nil { man.flusher = &FlusherMock{ @@ -596,9 +630,19 @@ func Test_Manager_DeleteHook(t *testing.T) { changes := &changeRecorder{} man.BindHookChange(changes.record) - err := man.DeleteHook(context.Background(), hk.ID, "org-1", "u1") + documentID := hk.DocumentID.V + if c.OtherDocument { + documentID = xid.New() + } + + err := man.DeleteHook(context.Background(), hk.ID, documentID, "org-1", "u1") testutil.AssertEqualError(t, c.Err, err) + // only a service the hook could not reach is reported as one. + if c.Err != hook.ErrUpstreamUnavailable { //nolint:err113,errorlint // the sentinel is returned as is + assert.NotEqual(t, hook.ErrUpstreamUnavailable, err) + } + ff := tx.DeleteDocumentHookCalls() require.Len(t, ff, c.Deletes) assert.Len(t, tx.RecordDocumentBranchHistoryEntryCalls(), c.Records) @@ -627,16 +671,17 @@ func Test_Manager_ResetHook(t *testing.T) { branchID := xid.New() cc := map[string]struct { - Hook func(*testing.T) hook.Hook - CD bool - Unreachable bool - FetchErr error - UpdateErr error - Updates int - Status processor.Status - Published []notification.Code - Watchers []string - Err error + Hook func(*testing.T) hook.Hook + OtherDocument bool + CD bool + Unreachable bool + FetchErr error + UpdateErr error + Updates int + Status processor.Status + Published []notification.Code + Watchers []string + Err error }{ "Error returned by db.FetchDocumentHook": { Hook: func(t *testing.T) hook.Hook { @@ -645,6 +690,13 @@ func Test_Manager_ResetHook(t *testing.T) { FetchErr: sql.ErrNoRows, Err: sql.ErrNoRows, }, + "Hook of another document": { + Hook: func(t *testing.T) hook.Hook { + return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + }, + OtherDocument: true, + Err: hook.ErrHookMismatch, + }, "Service the hook checks is unreachable": { Hook: func(*testing.T) hook.Hook { hk := urlWatcherHook(branchID) @@ -722,7 +774,12 @@ func Test_Manager_ResetHook(t *testing.T) { changes := &changeRecorder{} man.BindHookChange(changes.record) - hk, err := man.ResetHook(context.Background(), stored.ID, "org-1") + documentID := stored.DocumentID.V + if c.OtherDocument { + documentID = xid.New() + } + + hk, err := man.ResetHook(context.Background(), stored.ID, documentID, "org-1") testutil.AssertEqualError(t, c.Err, err) ff := db.UpdateDocumentHookCalls() diff --git a/server/core/internal/document/hook/processor/container_image_watcher.go b/server/core/internal/document/hook/processor/container_image_watcher.go index 7653f29c..884ef7c7 100644 --- a/server/core/internal/document/hook/processor/container_image_watcher.go +++ b/server/core/internal/document/hook/processor/container_image_watcher.go @@ -87,7 +87,7 @@ func (ciw *ContainerImageWatcher) digest(ctx context.Context) (string, Status, e case errors.Is(err, registry.ErrNotFound): return "", StatusImageNotFound, nil default: - return "", "", fmt.Errorf("fetching container image digest: %w", err) + return "", "", unreachable(fmt.Errorf("fetching container image digest: %w", err)) } } diff --git a/server/core/internal/document/hook/processor/github_tracking.go b/server/core/internal/document/hook/processor/github_tracking.go index 9fb4e346..f0900e65 100644 --- a/server/core/internal/document/hook/processor/github_tracking.go +++ b/server/core/internal/document/hook/processor/github_tracking.go @@ -131,7 +131,7 @@ func (gt *GithubTracking) fetchTree(ctx context.Context, inp Input) (github.Tree case errors.Is(err, github.ErrInstallationNotFound): return nil, StatusMissingInstallation, nil default: - return nil, "", fmt.Errorf("getting github client: %w", err) + return nil, "", unreachable(fmt.Errorf("getting github client: %w", err)) } tree, err := client.FetchRepositoryTree(ctx, gt.Repository, gt.Branch) @@ -146,7 +146,7 @@ func (gt *GithubTracking) fetchTree(ctx context.Context, inp Input) (github.Tree case errors.Is(err, github.ErrTreeTruncated): return nil, StatusTreeTruncated, nil default: - return nil, "", fmt.Errorf("fetching repository tree: %w", err) + return nil, "", unreachable(fmt.Errorf("fetching repository tree: %w", err)) } } diff --git a/server/core/internal/document/hook/processor/processor.go b/server/core/internal/document/hook/processor/processor.go index beef6f56..8086fc69 100644 --- a/server/core/internal/document/hook/processor/processor.go +++ b/server/core/internal/document/hook/processor/processor.go @@ -12,6 +12,10 @@ import ( "github.com/shopspring/decimal" ) +// ErrUnreachable is returned when a processor cannot reach the service it +// checks. +var ErrUnreachable = errors.New("cannot reach the service the hook checks") + // All available Status constants. const ( // StatusActive indicates that the processor checked what it watches. @@ -211,3 +215,8 @@ type ChangeDetection interface { // DeleteWatcher deletes the watcher with the given watch ID. DeleteWatcher(ctx context.Context, watchID string) error } + +// unreachable marks err, a failed call to the service a processor checks. +func unreachable(err error) error { + return fmt.Errorf("%w: %w", ErrUnreachable, err) +} diff --git a/server/core/internal/document/hook/processor/url_watcher.go b/server/core/internal/document/hook/processor/url_watcher.go index bd429923..ada66705 100644 --- a/server/core/internal/document/hook/processor/url_watcher.go +++ b/server/core/internal/document/hook/processor/url_watcher.go @@ -56,7 +56,7 @@ func (uw *URLWatcher) Process(ctx context.Context, inp Input) (Result, error) { case errors.Is(err, webchange.ErrNotConfigured): return inactive(StatusUnconfigured), nil default: - return Result{}, fmt.Errorf("fetching url watcher: %w", err) + return Result{}, unreachable(fmt.Errorf("fetching url watcher: %w", err)) } // a write that failed after moving the watcher leaves it on a URL the @@ -117,18 +117,18 @@ func (uw *URLWatcher) syncWatcher(ctx context.Context, cd ChangeDetection, watch }, nil case err == nil: if err = cd.UpdateWatcher(ctx, watcherID, uw.URL); err != nil { - return URLWatcherState{}, fmt.Errorf("updating url watcher: %w", err) + return URLWatcherState{}, unreachable(fmt.Errorf("updating url watcher: %w", err)) } return URLWatcherState{WatcherID: watcherID}, nil case !errors.Is(err, webchange.ErrWatcherNotFound): - return URLWatcherState{}, fmt.Errorf("fetching url watcher: %w", err) + return URLWatcherState{}, unreachable(fmt.Errorf("fetching url watcher: %w", err)) } } watcherID, err := cd.CreateWatcher(ctx, uw.URL) if err != nil { - return URLWatcherState{}, fmt.Errorf("creating url watcher: %w", err) + return URLWatcherState{}, unreachable(fmt.Errorf("creating url watcher: %w", err)) } return URLWatcherState{WatcherID: watcherID}, nil @@ -152,7 +152,7 @@ func (uw *URLWatcher) Delete(ctx context.Context, inp Input) error { err := inp.ChangeDetection().DeleteWatcher(ctx, uws.WatcherID) if err != nil { - return fmt.Errorf("deleting url watcher: %w", err) + return unreachable(fmt.Errorf("deleting url watcher: %w", err)) } return nil diff --git a/server/core/internal/server/0mock_db_test.go b/server/core/internal/server/0mock_db_test.go index 64a5837c..512d07ad 100644 --- a/server/core/internal/server/0mock_db_test.go +++ b/server/core/internal/server/0mock_db_test.go @@ -136,9 +136,6 @@ var _ DB = &DBMock{} // FetchDocumentFileFunc: func(ctx context.Context, id string, organizationID string) (*file.File, error) { // panic("mock out the FetchDocumentFile method") // }, -// FetchDocumentHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { -// panic("mock out the FetchDocumentHook method") -// }, // FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) { // panic("mock out the FetchDocumentHooksByBranchID method") // }, @@ -419,9 +416,6 @@ type DBMock struct { // FetchDocumentFileFunc mocks the FetchDocumentFile method. FetchDocumentFileFunc func(ctx context.Context, id string, organizationID string) (*file.File, error) - // FetchDocumentHookFunc mocks the FetchDocumentHook method. - FetchDocumentHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) - // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) @@ -917,15 +911,6 @@ type DBMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } - // FetchDocumentHook holds details about calls to the FetchDocumentHook method. - FetchDocumentHook []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // ID is the id argument value. - ID xid.ID - // 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. @@ -1426,7 +1411,6 @@ type DBMock struct { lockFetchDocumentCommentReply sync.RWMutex lockFetchDocumentCommentsByBranchID sync.RWMutex lockFetchDocumentFile sync.RWMutex - lockFetchDocumentHook sync.RWMutex lockFetchDocumentHooksByBranchID sync.RWMutex lockFetchDocumentHooksByDocumentID sync.RWMutex lockFetchDocumentHooksByOrganizationID sync.RWMutex @@ -3027,50 +3011,6 @@ func (mock *DBMock) FetchDocumentFileCalls() []struct { return calls } -// FetchDocumentHook calls FetchDocumentHookFunc. -func (mock *DBMock) FetchDocumentHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { - callInfo := struct { - Ctx context.Context - ID xid.ID - OrganizationID string - }{ - Ctx: ctx, - ID: id, - OrganizationID: organizationID, - } - mock.lockFetchDocumentHook.Lock() - mock.calls.FetchDocumentHook = append(mock.calls.FetchDocumentHook, callInfo) - mock.lockFetchDocumentHook.Unlock() - if mock.FetchDocumentHookFunc == nil { - var ( - hookOut *hookCore.Hook - errOut error - ) - return hookOut, errOut - } - return mock.FetchDocumentHookFunc(ctx, id, organizationID) -} - -// FetchDocumentHookCalls gets all the calls that were made to FetchDocumentHook. -// Check the length with: -// -// len(mockedDB.FetchDocumentHookCalls()) -func (mock *DBMock) FetchDocumentHookCalls() []struct { - Ctx context.Context - ID xid.ID - OrganizationID string -} { - var calls []struct { - Ctx context.Context - ID xid.ID - OrganizationID string - } - mock.lockFetchDocumentHook.RLock() - calls = mock.calls.FetchDocumentHook - mock.lockFetchDocumentHook.RUnlock() - return calls -} - // FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. func (mock *DBMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, 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 e55595b0..243a3b20 100644 --- a/server/core/internal/server/0mock_hook_manager_test.go +++ b/server/core/internal/server/0mock_hook_manager_test.go @@ -27,16 +27,16 @@ var _ HookManager = &HookManagerMock{} // CreateHookFunc: func(ctx context.Context, ci hookCore.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hookCore.Hook, error) { // panic("mock out the CreateHook method") // }, -// DeleteHookFunc: func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +// DeleteHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { // panic("mock out the DeleteHook method") // }, -// ResetHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { +// ResetHookFunc: func(ctx context.Context, id xid.ID, documentID 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) { +// UpdateHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { // panic("mock out the UpdateHook method") // }, // } @@ -53,16 +53,16 @@ type HookManagerMock struct { CreateHookFunc func(ctx context.Context, ci hookCore.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hookCore.Hook, error) // DeleteHookFunc mocks the DeleteHook method. - DeleteHookFunc func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error + DeleteHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error // ResetHookFunc mocks the ResetHook method. - ResetHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) + ResetHookFunc func(ctx context.Context, id xid.ID, documentID 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) + UpdateHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) // calls tracks calls to the methods. calls struct { @@ -90,6 +90,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UpdatedBy is the updatedBy argument value. @@ -101,6 +103,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string } @@ -119,6 +123,8 @@ type HookManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UI is the ui argument value. @@ -220,15 +226,17 @@ func (mock *HookManagerMock) CreateHookCalls() []struct { } // DeleteHook calls DeleteHookFunc. -func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UpdatedBy: updatedBy, } @@ -241,7 +249,7 @@ func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organiza ) return errOut } - return mock.DeleteHookFunc(ctx, id, organizationID, updatedBy) + return mock.DeleteHookFunc(ctx, id, documentID, organizationID, updatedBy) } // DeleteHookCalls gets all the calls that were made to DeleteHook. @@ -251,12 +259,14 @@ func (mock *HookManagerMock) DeleteHook(ctx context.Context, id xid.ID, organiza func (mock *HookManagerMock) DeleteHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } @@ -267,14 +277,16 @@ func (mock *HookManagerMock) DeleteHookCalls() []struct { } // ResetHook calls ResetHookFunc. -func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { +func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hookCore.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, } mock.lockResetHook.Lock() @@ -287,7 +299,7 @@ func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizat ) return hookOut, errOut } - return mock.ResetHookFunc(ctx, id, organizationID) + return mock.ResetHookFunc(ctx, id, documentID, organizationID) } // ResetHookCalls gets all the calls that were made to ResetHook. @@ -297,11 +309,13 @@ func (mock *HookManagerMock) ResetHook(ctx context.Context, id xid.ID, organizat func (mock *HookManagerMock) ResetHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } mock.lockResetHook.RLock() @@ -351,16 +365,18 @@ func (mock *HookManagerMock) SetUpBranchCalls() []struct { } // UpdateHook calls UpdateHookFunc. -func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { +func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UI: ui, UpdatedBy: updatedBy, @@ -375,7 +391,7 @@ func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organiza ) return hookOut, errOut } - return mock.UpdateHookFunc(ctx, id, organizationID, ui, updatedBy) + return mock.UpdateHookFunc(ctx, id, documentID, organizationID, ui, updatedBy) } // UpdateHookCalls gets all the calls that were made to UpdateHook. @@ -385,6 +401,7 @@ func (mock *HookManagerMock) UpdateHook(ctx context.Context, id xid.ID, organiza func (mock *HookManagerMock) UpdateHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string @@ -392,6 +409,7 @@ func (mock *HookManagerMock) UpdateHookCalls() []struct { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string 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 2fea7349..ed463fd7 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -556,56 +556,6 @@ func Test_Handler_MergeBranches(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, }, - // the hooks are copied after the commit, so a failure there leaves - // the merge itself standing. - "Hook copy fetch error": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: fetchByBranch, - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return nil, errors.New("boom") - }, - }, - Tx: &TxMock{}, - Body: validBody, - RespCode: http.StatusOK, - Committed: 1, - Metadata: 1, - Reviewers: 1, - History: 1, - }, - "Hook re-creation error": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: fetchByBranch, - FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{storedHook("bogus")}, nil - }, - }, - Tx: &TxMock{}, - Body: validBody, - RespCode: http.StatusOK, - Committed: 1, - Metadata: 1, - Reviewers: 1, - History: 1, - }, - "Hook insertion error": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: fetchByBranch, - 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") - }, - }, - Tx: &TxMock{}, - Body: validBody, - RespCode: http.StatusOK, - Committed: 1, - Metadata: 1, - Reviewers: 1, - History: 1, - }, "Document update error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, Tx: &TxMock{ diff --git a/server/core/internal/server/internal/document/hook/0mock_db_test.go b/server/core/internal/server/internal/document/hook/0mock_db_test.go index ca314e24..adafb098 100644 --- a/server/core/internal/server/internal/document/hook/0mock_db_test.go +++ b/server/core/internal/server/internal/document/hook/0mock_db_test.go @@ -25,9 +25,6 @@ var _ DB = &DBMock{} // FetchDocumentByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) { // panic("mock out the FetchDocumentByBranchID method") // }, -// FetchDocumentHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { -// panic("mock out the FetchDocumentHook method") -// }, // FetchDocumentHooksByBranchIDFunc: func(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) { // panic("mock out the FetchDocumentHooksByBranchID method") // }, @@ -41,9 +38,6 @@ type DBMock struct { // FetchDocumentByBranchIDFunc mocks the FetchDocumentByBranchID method. FetchDocumentByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) - // FetchDocumentHookFunc mocks the FetchDocumentHook method. - FetchDocumentHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) - // FetchDocumentHooksByBranchIDFunc mocks the FetchDocumentHooksByBranchID method. FetchDocumentHooksByBranchIDFunc func(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) @@ -58,15 +52,6 @@ type DBMock struct { // OrganizationID is the organizationID argument value. OrganizationID string } - // FetchDocumentHook holds details about calls to the FetchDocumentHook method. - FetchDocumentHook []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // ID is the id argument value. - ID xid.ID - // 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. @@ -78,7 +63,6 @@ type DBMock struct { } } lockFetchDocumentByBranchID sync.RWMutex - lockFetchDocumentHook sync.RWMutex lockFetchDocumentHooksByBranchID sync.RWMutex } @@ -126,50 +110,6 @@ func (mock *DBMock) FetchDocumentByBranchIDCalls() []struct { return calls } -// FetchDocumentHook calls FetchDocumentHookFunc. -func (mock *DBMock) FetchDocumentHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { - callInfo := struct { - Ctx context.Context - ID xid.ID - OrganizationID string - }{ - Ctx: ctx, - ID: id, - OrganizationID: organizationID, - } - mock.lockFetchDocumentHook.Lock() - mock.calls.FetchDocumentHook = append(mock.calls.FetchDocumentHook, callInfo) - mock.lockFetchDocumentHook.Unlock() - if mock.FetchDocumentHookFunc == nil { - var ( - hookOut *hookCore.Hook - errOut error - ) - return hookOut, errOut - } - return mock.FetchDocumentHookFunc(ctx, id, organizationID) -} - -// FetchDocumentHookCalls gets all the calls that were made to FetchDocumentHook. -// Check the length with: -// -// len(mockedDB.FetchDocumentHookCalls()) -func (mock *DBMock) FetchDocumentHookCalls() []struct { - Ctx context.Context - ID xid.ID - OrganizationID string -} { - var calls []struct { - Ctx context.Context - ID xid.ID - OrganizationID string - } - mock.lockFetchDocumentHook.RLock() - calls = mock.calls.FetchDocumentHook - mock.lockFetchDocumentHook.RUnlock() - return calls -} - // FetchDocumentHooksByBranchID calls FetchDocumentHooksByBranchIDFunc. func (mock *DBMock) FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) { callInfo := struct { diff --git a/server/core/internal/server/internal/document/hook/0mock_manager_test.go b/server/core/internal/server/internal/document/hook/0mock_manager_test.go index 2fa7d7ef..160702a6 100644 --- a/server/core/internal/server/internal/document/hook/0mock_manager_test.go +++ b/server/core/internal/server/internal/document/hook/0mock_manager_test.go @@ -24,13 +24,13 @@ var _ Manager = &ManagerMock{} // CreateHookFunc: func(ctx context.Context, ci hookCore.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hookCore.Hook, error) { // panic("mock out the CreateHook method") // }, -// DeleteHookFunc: func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +// DeleteHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { // panic("mock out the DeleteHook method") // }, -// ResetHookFunc: func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { +// ResetHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hookCore.Hook, error) { // panic("mock out the ResetHook method") // }, -// UpdateHookFunc: func(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { +// UpdateHookFunc: func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { // panic("mock out the UpdateHook method") // }, // } @@ -44,13 +44,13 @@ type ManagerMock struct { CreateHookFunc func(ctx context.Context, ci hookCore.CreateInput, documentID xid.ID, organizationID string, updatedBy string) (*hookCore.Hook, error) // DeleteHookFunc mocks the DeleteHook method. - DeleteHookFunc func(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error + DeleteHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error // ResetHookFunc mocks the ResetHook method. - ResetHookFunc func(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) + ResetHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hookCore.Hook, error) // UpdateHookFunc mocks the UpdateHook method. - UpdateHookFunc func(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) + UpdateHookFunc func(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) // calls tracks calls to the methods. calls struct { @@ -73,6 +73,8 @@ type ManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UpdatedBy is the updatedBy argument value. @@ -84,6 +86,8 @@ type ManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string } @@ -93,6 +97,8 @@ type ManagerMock struct { Ctx context.Context // ID is the id argument value. ID xid.ID + // DocumentID is the documentID argument value. + DocumentID xid.ID // OrganizationID is the organizationID argument value. OrganizationID string // UI is the ui argument value. @@ -160,15 +166,17 @@ func (mock *ManagerMock) CreateHookCalls() []struct { } // DeleteHook calls DeleteHookFunc. -func (mock *ManagerMock) DeleteHook(ctx context.Context, id xid.ID, organizationID string, updatedBy string) error { +func (mock *ManagerMock) DeleteHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, updatedBy string) error { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UpdatedBy: updatedBy, } @@ -181,7 +189,7 @@ func (mock *ManagerMock) DeleteHook(ctx context.Context, id xid.ID, organization ) return errOut } - return mock.DeleteHookFunc(ctx, id, organizationID, updatedBy) + return mock.DeleteHookFunc(ctx, id, documentID, organizationID, updatedBy) } // DeleteHookCalls gets all the calls that were made to DeleteHook. @@ -191,12 +199,14 @@ func (mock *ManagerMock) DeleteHook(ctx context.Context, id xid.ID, organization func (mock *ManagerMock) DeleteHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UpdatedBy string } @@ -207,14 +217,16 @@ func (mock *ManagerMock) DeleteHookCalls() []struct { } // ResetHook calls ResetHookFunc. -func (mock *ManagerMock) ResetHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) { +func (mock *ManagerMock) ResetHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string) (*hookCore.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, } mock.lockResetHook.Lock() @@ -227,7 +239,7 @@ func (mock *ManagerMock) ResetHook(ctx context.Context, id xid.ID, organizationI ) return hookOut, errOut } - return mock.ResetHookFunc(ctx, id, organizationID) + return mock.ResetHookFunc(ctx, id, documentID, organizationID) } // ResetHookCalls gets all the calls that were made to ResetHook. @@ -237,11 +249,13 @@ func (mock *ManagerMock) ResetHook(ctx context.Context, id xid.ID, organizationI func (mock *ManagerMock) ResetHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string } mock.lockResetHook.RLock() @@ -251,16 +265,18 @@ func (mock *ManagerMock) ResetHookCalls() []struct { } // UpdateHook calls UpdateHookFunc. -func (mock *ManagerMock) UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { +func (mock *ManagerMock) UpdateHook(ctx context.Context, id xid.ID, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) { callInfo := struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string }{ Ctx: ctx, ID: id, + DocumentID: documentID, OrganizationID: organizationID, UI: ui, UpdatedBy: updatedBy, @@ -275,7 +291,7 @@ func (mock *ManagerMock) UpdateHook(ctx context.Context, id xid.ID, organization ) return hookOut, errOut } - return mock.UpdateHookFunc(ctx, id, organizationID, ui, updatedBy) + return mock.UpdateHookFunc(ctx, id, documentID, organizationID, ui, updatedBy) } // UpdateHookCalls gets all the calls that were made to UpdateHook. @@ -285,6 +301,7 @@ func (mock *ManagerMock) UpdateHook(ctx context.Context, id xid.ID, organization func (mock *ManagerMock) UpdateHookCalls() []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string @@ -292,6 +309,7 @@ func (mock *ManagerMock) UpdateHookCalls() []struct { var calls []struct { Ctx context.Context ID xid.ID + DocumentID xid.ID OrganizationID string UI hookCore.UpdateInput UpdatedBy string diff --git a/server/core/internal/server/internal/document/hook/http.go b/server/core/internal/server/internal/document/hook/http.go index d672fed5..c7567e74 100644 --- a/server/core/internal/server/internal/document/hook/http.go +++ b/server/core/internal/server/internal/document/hook/http.go @@ -9,19 +9,10 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" - "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/httpserver" "github.com/rs/xid" ) -// ErrBranchMismatch is returned when the requested branch does not belong to -// the document identified by the request path. -var ErrBranchMismatch = errutil.New(http.StatusNotFound, "document.branch_mismatch", "branch does not belong to the document") - -// ErrHookMismatch is returned when the requested hook does not belong to the -// document identified by the request path. -var ErrHookMismatch = errutil.New(http.StatusNotFound, "document.hook_mismatch", "hook does not belong to the document") - // Handler holds dependencies required for document hook operations. type Handler struct { log *slog.Logger @@ -70,7 +61,7 @@ func (h *Handler) FetchDocumentHooks(w http.ResponseWriter, r *http.Request) { } if branchDoc.ID != documentID { - httpserver.RespondError(h.log, w, ErrBranchMismatch) + httpserver.RespondError(h.log, w, hookCore.ErrBranchMismatch) return } @@ -146,19 +137,6 @@ func (h *Handler) UpdateDocumentHook(w http.ResponseWriter, r *http.Request) { return } - hk, err := h.db.FetchDocumentHook(r.Context(), id, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // a row whose document was deleted carries no document id at all and is - // not addressable through any document path. - if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { - httpserver.RespondError(h.log, w, ErrHookMismatch) - return - } - var ui hookCore.UpdateInput if err = httpserver.DecodeJSON(r, &ui); err != nil { @@ -166,7 +144,7 @@ func (h *Handler) UpdateDocumentHook(w http.ResponseWriter, r *http.Request) { return } - hk, err = h.hookMan.UpdateHook(r.Context(), hk.ID, session.ActiveOrganizationID, ui, session.UserID) + hk, err := h.hookMan.UpdateHook(r.Context(), id, documentID, session.ActiveOrganizationID, ui, session.UserID) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -199,20 +177,7 @@ func (h *Handler) ResetDocumentHook(w http.ResponseWriter, r *http.Request) { return } - hk, err := h.db.FetchDocumentHook(r.Context(), id, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // a row whose document was deleted carries no document id at all and is - // not addressable through any document path. - if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { - httpserver.RespondError(h.log, w, ErrHookMismatch) - return - } - - hk, err = h.hookMan.ResetHook(r.Context(), hk.ID, session.ActiveOrganizationID) + hk, err := h.hookMan.ResetHook(r.Context(), id, documentID, session.ActiveOrganizationID) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -245,20 +210,7 @@ func (h *Handler) DeleteDocumentHook(w http.ResponseWriter, r *http.Request) { return } - hk, err := h.db.FetchDocumentHook(r.Context(), id, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // a row whose document was deleted carries no document id at all and is - // not addressable through any document path. - if !hk.DocumentID.Valid || hk.DocumentID.V != documentID { - httpserver.RespondError(h.log, w, ErrHookMismatch) - return - } - - if err = h.hookMan.DeleteHook(r.Context(), hk.ID, session.ActiveOrganizationID, session.UserID); err != nil { + if err = h.hookMan.DeleteHook(r.Context(), id, documentID, session.ActiveOrganizationID, session.UserID); err != nil { httpserver.RespondError(h.log, w, err) return } @@ -279,9 +231,6 @@ type DB interface { // FetchDocumentByBranchID should fetch the document joined against the branch identified by branchID. FetchDocumentByBranchID(ctx context.Context, branchID xid.ID, organizationID string) (*document.Document, error) - // FetchDocumentHook should fetch the document hook for the given id. - FetchDocumentHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) - // FetchDocumentHooksByBranchID should fetch all hooks for a specific branch. FetchDocumentHooksByBranchID(ctx context.Context, branchID xid.ID, organizationID string) ([]hookCore.Hook, error) } @@ -294,15 +243,15 @@ type Manager interface { // credited to updatedBy. CreateHook(ctx context.Context, ci hookCore.CreateInput, documentID xid.ID, organizationID, updatedBy string) (*hookCore.Hook, error) - // UpdateHook should replace the hook's settings, credited to - // updatedBy, and return the stored hook. - UpdateHook(ctx context.Context, id xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) + // UpdateHook should replace the settings of the document's hook, + // credited to updatedBy, and return the stored hook. + UpdateHook(ctx context.Context, id, documentID xid.ID, organizationID string, ui hookCore.UpdateInput, updatedBy string) (*hookCore.Hook, error) - // DeleteHook should tear the hook down and remove it, credited to - // updatedBy. - DeleteHook(ctx context.Context, id xid.ID, organizationID, updatedBy string) error + // DeleteHook should tear the document's hook down and remove it, + // credited to updatedBy. + DeleteHook(ctx context.Context, id, documentID xid.ID, organizationID, updatedBy string) error - // ResetHook should restore the hook's score and state and return the - // stored hook. - ResetHook(ctx context.Context, id xid.ID, organizationID string) (*hookCore.Hook, error) + // ResetHook should reset the document's hook and return the stored + // hook. + ResetHook(ctx context.Context, id, documentID xid.ID, organizationID string) (*hookCore.Hook, error) } diff --git a/server/core/internal/server/internal/document/hook/http_test.go b/server/core/internal/server/internal/document/hook/http_test.go index 4ee654df..825df9d0 100644 --- a/server/core/internal/server/internal/document/hook/http_test.go +++ b/server/core/internal/server/internal/document/hook/http_test.go @@ -56,10 +56,10 @@ func scheduledHook() *hookCore.Hook { // storedHookManager answers an update or reset with the stored hook. func storedHookManager() *ManagerMock { return &ManagerMock{ - UpdateHookFunc: func(context.Context, xid.ID, string, hookCore.UpdateInput, string) (*hookCore.Hook, error) { + UpdateHookFunc: func(context.Context, xid.ID, xid.ID, string, hookCore.UpdateInput, string) (*hookCore.Hook, error) { return scheduledHook(), nil }, - ResetHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { + ResetHookFunc: func(context.Context, xid.ID, xid.ID, string) (*hookCore.Hook, error) { return scheduledHook(), nil }, } @@ -288,15 +288,6 @@ func hookRequest(method, body string, noSession, omitDoc, omitHook bool) *http.R return req.WithContext(ctx) } -// storedHookDB answers the hook lookup with the given hook. -func storedHookDB(hk *hookCore.Hook) *DBMock { - return &DBMock{ - FetchDocumentHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { - return hk, nil - }, - } -} - func Test_Handler_CreateDocumentHook(t *testing.T) { validBody := `{"type":"scheduled-reminder","branchId":"` + _branchID.String() + `","settings":{"scale":"linear"}}` @@ -397,11 +388,7 @@ func Test_Handler_CreateDocumentHook(t *testing.T) { func Test_Handler_UpdateDocumentHook(t *testing.T) { validBody := `{"settings":{"scale":"linear","duration":"48h"}}` - otherDocument := scheduledHook() - otherDocument.DocumentID = null.ValueFrom(xid.New()) - cc := map[string]struct { - DB *DBMock Man *ManagerMock NoSession bool OmitDoc bool @@ -412,53 +399,42 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { Updates int }{ "No session in context": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, NoSession: true, Body: validBody, RespCode: http.StatusUnauthorized, }, "Missing document ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitDoc: true, Body: validBody, RespCode: http.StatusNotFound, }, "Missing hook ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitHook: true, Body: validBody, RespCode: http.StatusNotFound, }, - "Hook fetch error": { - DB: &DBMock{ - FetchDocumentHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { - return nil, errors.New("boom") + "Hook of another document": { + Man: &ManagerMock{ + UpdateHookFunc: func(context.Context, xid.ID, xid.ID, string, hookCore.UpdateInput, string) (*hookCore.Hook, error) { + return nil, hookCore.ErrHookMismatch }, }, - Man: &ManagerMock{}, - Body: validBody, - RespCode: http.StatusInternalServerError, - }, - "Hook of another document": { - DB: storedHookDB(otherDocument), - Man: &ManagerMock{}, Body: validBody, RespCode: http.StatusNotFound, RespBody: `{"code":"document.hook_mismatch","message":"hook does not belong to the document"}`, + Updates: 1, }, "Invalid JSON body": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, Body: "{", RespCode: http.StatusBadRequest, }, "Error returned by man.UpdateHook": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{ - UpdateHookFunc: func(context.Context, xid.ID, string, hookCore.UpdateInput, string) (*hookCore.Hook, error) { + UpdateHookFunc: func(context.Context, xid.ID, xid.ID, string, hookCore.UpdateInput, string) (*hookCore.Hook, error) { return nil, hookCore.ErrUpstreamUnavailable }, }, @@ -468,7 +444,6 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { Updates: 1, }, "Successful update": { - DB: storedHookDB(scheduledHook()), Man: storedHookManager(), Body: validBody, RespCode: http.StatusOK, @@ -480,7 +455,7 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - hdl := NewHandler(slog.New(slog.DiscardHandler), c.DB, c.Man) + hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) got := recordNotifications(hdl) rec := httptest.NewRecorder() @@ -499,6 +474,7 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { if c.Updates > 0 { assert.Equal(t, _hookID, ff[0].ID) + assert.Equal(t, _documentID, ff[0].DocumentID) assert.Equal(t, "org1", ff[0].OrganizationID) assert.JSONEq(t, `{"scale":"linear","duration":"48h"}`, string(ff[0].UI.Settings)) assert.Equal(t, "u1", ff[0].UpdatedBy) @@ -508,11 +484,7 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { } func Test_Handler_ResetDocumentHook(t *testing.T) { - otherDocument := scheduledHook() - otherDocument.DocumentID = null.ValueFrom(xid.New()) - cc := map[string]struct { - DB *DBMock Man *ManagerMock NoSession bool OmitDoc bool @@ -521,41 +493,32 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { Resets int }{ "No session in context": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, NoSession: true, RespCode: http.StatusUnauthorized, }, "Missing document ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitDoc: true, RespCode: http.StatusNotFound, }, "Missing hook ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitHook: true, RespCode: http.StatusNotFound, }, - "Hook fetch error": { - DB: &DBMock{ - FetchDocumentHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { - return nil, errors.New("boom") + "Hook of another document": { + Man: &ManagerMock{ + ResetHookFunc: func(context.Context, xid.ID, xid.ID, string) (*hookCore.Hook, error) { + return nil, hookCore.ErrHookMismatch }, }, - Man: &ManagerMock{}, - RespCode: http.StatusInternalServerError, - }, - "Hook of another document": { - DB: storedHookDB(otherDocument), - Man: &ManagerMock{}, RespCode: http.StatusNotFound, + Resets: 1, }, "Error returned by man.ResetHook": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{ - ResetHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { + ResetHookFunc: func(context.Context, xid.ID, xid.ID, string) (*hookCore.Hook, error) { return nil, errors.New("boom") }, }, @@ -563,7 +526,6 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { Resets: 1, }, "Successful reset": { - DB: storedHookDB(scheduledHook()), Man: storedHookManager(), RespCode: http.StatusOK, Resets: 1, @@ -574,7 +536,7 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - hdl := NewHandler(slog.New(slog.DiscardHandler), c.DB, c.Man) + hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) got := recordNotifications(hdl) rec := httptest.NewRecorder() @@ -589,6 +551,7 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { if c.Resets > 0 { assert.Equal(t, _hookID, ff[0].ID) + assert.Equal(t, _documentID, ff[0].DocumentID) assert.Equal(t, "org1", ff[0].OrganizationID) } }) @@ -596,11 +559,7 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { } func Test_Handler_DeleteDocumentHook(t *testing.T) { - otherDocument := scheduledHook() - otherDocument.DocumentID = null.ValueFrom(xid.New()) - cc := map[string]struct { - DB *DBMock Man *ManagerMock NoSession bool OmitDoc bool @@ -609,41 +568,32 @@ func Test_Handler_DeleteDocumentHook(t *testing.T) { Deletes int }{ "No session in context": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, NoSession: true, RespCode: http.StatusUnauthorized, }, "Missing document ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitDoc: true, RespCode: http.StatusNotFound, }, "Missing hook ID parameter": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, OmitHook: true, RespCode: http.StatusNotFound, }, - "Hook fetch error": { - DB: &DBMock{ - FetchDocumentHookFunc: func(context.Context, xid.ID, string) (*hookCore.Hook, error) { - return nil, errors.New("boom") + "Hook of another document": { + Man: &ManagerMock{ + DeleteHookFunc: func(context.Context, xid.ID, xid.ID, string, string) error { + return hookCore.ErrHookMismatch }, }, - Man: &ManagerMock{}, - RespCode: http.StatusInternalServerError, - }, - "Hook of another document": { - DB: storedHookDB(otherDocument), - Man: &ManagerMock{}, RespCode: http.StatusNotFound, + Deletes: 1, }, "Error returned by man.DeleteHook": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{ - DeleteHookFunc: func(context.Context, xid.ID, string, string) error { + DeleteHookFunc: func(context.Context, xid.ID, xid.ID, string, string) error { return errors.New("boom") }, }, @@ -651,7 +601,6 @@ func Test_Handler_DeleteDocumentHook(t *testing.T) { Deletes: 1, }, "Successful deletion": { - DB: storedHookDB(scheduledHook()), Man: &ManagerMock{}, RespCode: http.StatusNoContent, Deletes: 1, @@ -662,7 +611,7 @@ func Test_Handler_DeleteDocumentHook(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - hdl := NewHandler(slog.New(slog.DiscardHandler), c.DB, c.Man) + hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) got := recordNotifications(hdl) rec := httptest.NewRecorder() @@ -677,6 +626,7 @@ func Test_Handler_DeleteDocumentHook(t *testing.T) { if c.Deletes > 0 { assert.Equal(t, _hookID, ff[0].ID) + assert.Equal(t, _documentID, ff[0].DocumentID) assert.Equal(t, "org1", ff[0].OrganizationID) assert.Equal(t, "u1", ff[0].UpdatedBy) } diff --git a/server/core/pkg/syncutil/keyed_mutex.go b/server/core/pkg/syncutil/keyed_mutex.go index 49cc9e9b..25281a47 100644 --- a/server/core/pkg/syncutil/keyed_mutex.go +++ b/server/core/pkg/syncutil/keyed_mutex.go @@ -27,7 +27,10 @@ func (km *KeyedMutex[K]) Lock(ctx context.Context, key K) (func(), error) { select { case l.ch <- struct{}{}: - return km.unlocker(key, l), nil + return func() { + <-l.ch + km.unref(key, l) + }, nil case <-ctx.Done(): km.unref(key, l) @@ -64,14 +67,6 @@ func (km *KeyedMutex[K]) unref(key K, l *keyedLock) { } } -// unlocker returns the function that unlocks a held lock. -func (km *KeyedMutex[K]) unlocker(key K, l *keyedLock) func() { - return func() { - <-l.ch - km.unref(key, l) - } -} - // keyedLock is one key's lock: holding it is a value in ch. type keyedLock struct { ch chan struct{} diff --git a/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts b/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts index b87eb5f1..06a1b04e 100644 --- a/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts +++ b/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts @@ -62,10 +62,6 @@ function githubHook(overrides: Partial = {}) { }) } -function failingGithubHook(status: DocumentHookStatusGitHubTracking) { - return githubHook({ status: status }) -} - function mockGitHub( options: { connected?: boolean; repositories?: string[] } = {}, ) { @@ -183,7 +179,7 @@ describe("", { concurrent: false }, () => { async ({ status, problem }, { expect }) => { mockGitHub() - await mountMenu({ hook: failingGithubHook(status) }) + await mountMenu({ hook: githubHook({ status: status }) }) expect(menuText()).toContain( t("editor.hooks.subtext-detail", { @@ -222,7 +218,7 @@ describe("", { concurrent: false }, () => { it("warns that a hook lost its github connection", async ({ expect }) => { mockGitHub() - await mountMenu({ hook: failingGithubHook("missing_installation") }) + await mountMenu({ hook: githubHook({ status: "missing_installation" }) }) await openHookSubMenu(t(TITLE)) @@ -269,7 +265,7 @@ describe("", { concurrent: false }, () => { "warns about $name it can no longer reach", async ({ status, key }, { expect }) => { mockGitHub() - await mountMenu({ hook: failingGithubHook(status) }) + await mountMenu({ hook: githubHook({ status: status }) }) await openHookSubMenu(t(TITLE)) @@ -284,7 +280,7 @@ describe("", { concurrent: false }, () => { // notice does not send the reader there it("warns that github is not set up on this server", async ({ expect }) => { mockGitHub({ connected: false }) - await mountMenu({ hook: failingGithubHook("unconfigured") }) + await mountMenu({ hook: githubHook({ status: "unconfigured" }) }) await openHookSubMenu(t(TITLE)) diff --git a/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue b/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue index ca6164d8..d8a5c763 100644 --- a/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue +++ b/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue @@ -50,6 +50,24 @@ const targetBranchSettings = computed( const checkStatus = computed( () => props.hook?.status as DocumentHookStatusGitHubTracking | undefined, ) +// how many of the repository, branch and files fields an editor can change. +// A failed check leaves those up to the one at fault. +const editableFields = computed(() => { + switch (checkStatus.value) { + case "unconfigured": + case "missing_installation": + return 0 + case "missing_repository": + return 1 + case "missing_branch": + case "tree_truncated": + return 2 + case "active": + case "initializing": + case undefined: + return 3 + } +}) const status = computed(() => (props.hook ? hookStatus(props.hook) : null)) const selectedRepository = ref( activeBranchSettings.value?.repository, @@ -354,8 +372,7 @@ function close() { cn( 'flex flex-col gap-1 opacity-100 transition-opacity duration-200', (!fetchGitHubConnectionStatus.data.value?.connected || - checkStatus === 'missing_installation' || - checkStatus === 'unconfigured') && + editableFields === 0) && 'pointer-events-none opacity-60', ) " @@ -366,12 +383,7 @@ function close() { !fetchGitHubConnectionStatus.data.value?.connected || fetchGitHubRepositories.isLoading.value || fetchGitHubRepositories.state.value.data?.length === 0 || - (!!checkStatus && - checkStatus !== 'active' && - checkStatus !== 'initializing' && - checkStatus !== 'missing_repository' && - checkStatus !== 'missing_branch' && - checkStatus !== 'tree_truncated') + editableFields < 1 " > @@ -416,11 +428,7 @@ function close() { :disabled=" !selectedRepository || fetchGitHubBranches.isLoading.value || - (!!checkStatus && - checkStatus !== 'active' && - checkStatus !== 'initializing' && - checkStatus !== 'missing_branch' && - checkStatus !== 'tree_truncated') + editableFields < 2 " > @@ -468,9 +476,7 @@ function close() { !selectedRepository || !selectedBranch || fetchGitHubPaths.isLoading.value || - (!!checkStatus && - checkStatus !== 'active' && - checkStatus !== 'initializing') + editableFields < 3 " :options="fetchGitHubPaths.state.value.data || []" :placeholder="