From 7476941e1f5343607bffd24c3d394937e5ae20eb Mon Sep 17 00:00:00 2001 From: david <58231312+davseby@users.noreply.github.com> Date: Sat, 5 Sep 2026 20:22:27 +0300 Subject: [PATCH] disconnect clients on document deletion --- CLAUDE.md | 2 +- e2e/tests/prod-trust-boundary.test.ts | 5 + server/CLAUDE.md | 2 +- server/auth-realtime/src/core.test.ts | 23 + server/auth-realtime/src/core.ts | 13 + server/auth-realtime/src/routes.test.ts | 64 ++ server/auth-realtime/src/routes.ts | 34 + server/auth-realtime/src/test-helpers.ts | 4 + .../server/internal/document/branch_http.go | 667 +++++++++++++++++ .../internal/document/branch_http_test.go | 668 +++++------------- .../document/{http.go => document_http.go} | 663 +---------------- .../{http_test.go => document_http_test.go} | 522 +++++++++++--- server/core/internal/server/router.go | 12 +- server/core/internal/server/router_test.go | 13 +- .../DocumentContainer.client.nuxt.test.ts | 18 +- .../editor/DocumentContainer.client.vue | 7 +- web/app/composables/api/useDocumentAPI.ts | 12 +- web/i18n/locales/en/editor.json | 4 + 18 files changed, 1458 insertions(+), 1275 deletions(-) create mode 100644 server/core/internal/server/internal/document/branch_http.go rename server/core/internal/server/internal/document/{http.go => document_http.go} (65%) rename server/core/internal/server/internal/document/{http_test.go => document_http_test.go} (75%) diff --git a/CLAUDE.md b/CLAUDE.md index 69208dac..d2bb127b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -157,7 +157,7 @@ Component build/test/qa commands are listed in the nested CLAUDE.md files. ## Cross-component contracts - **Front door**: Caddy on host `:8080`. `/core/*` is path-stripped and proxied to core (`:8080`); `/auth-realtime/*` is path-stripped and proxied to auth-realtime (`:8081`, so `/auth-realtime/api/...` and `/auth-realtime/hocuspocus` publicly); everything else goes to the web SSR container. The frontend reaches the core API via `NUXT_PUBLIC_CORE_API_BASE_HTTP_URL` (`…:8080/core`) and auth/realtime via `NUXT_PUBLIC_AUTH_REALTIME_API_BASE_*_URL` (`…:8080/auth-realtime`). -- **Trust boundary**: core has four surfaces. `/api/...` requires a session. `/api/x/...` (core) and `/api/internal/...` (auth-realtime) have no auth and must never be exposed by the reverse proxy (the Caddyfile blocks both at the front door) — they exist for service-to-service calls (auth-realtime ↔ core). Three of core's `/api/x` routes are internal *and* session-authed: fork (`POST .../branches`), branch update (`PUT .../branches/{branchId}`) and merge (`PUT .../merge`) act for a person but are reachable only through auth-realtime, which first flushes the branch's pending Hocuspocus store so core reads what the editors show, forwards the caller's session for core to authorize, and resets the branch's connections after a protection change. `/api/apps/...` is public and sessionless: GitHub and Slack deliver webhooks and OAuth callbacks there, and every request proves itself with the provider's signature or an encrypted state parameter. `/api/mcp` is the MCP surface: OAuth 2.1 bearer tokens issued by auth-realtime (which is the authorization server via `@better-auth/mcp`), validated per request against auth-realtime's internal MCP session endpoint; the Caddyfile also routes the front-door `/.well-known/oauth-*` discovery documents and `/api/auth/*` OAuth endpoints to auth-realtime. +- **Trust boundary**: core has four surfaces. `/api/...` requires a session. `/api/x/...` (core) and `/api/internal/...` (auth-realtime) have no auth and must never be exposed by the reverse proxy (the Caddyfile blocks both at the front door) — they exist for service-to-service calls (auth-realtime ↔ core). Four of core's `/api/x` routes are internal *and* session-authed: fork (`POST .../branches`), branch update (`PUT .../branches/{branchId}`), merge (`PUT .../merge`) and branch delete (`DELETE .../branches/{branchId}`) act for a person but are reachable only through auth-realtime, which first flushes the branch's pending Hocuspocus store so core reads what the editors show, forwards the caller's session for core to authorize, and resets the branch's connections after a protection change or a delete. `/api/apps/...` is public and sessionless: GitHub and Slack deliver webhooks and OAuth callbacks there, and every request proves itself with the provider's signature or an encrypted state parameter. `/api/mcp` is the MCP surface: OAuth 2.1 bearer tokens issued by auth-realtime (which is the authorization server via `@better-auth/mcp`), validated per request against auth-realtime's internal MCP session endpoint; the Caddyfile also routes the front-door `/.well-known/oauth-*` discovery documents and `/api/auth/*` OAuth endpoints to auth-realtime. - **Session validation**: core's auth middleware validates sessions by calling auth-realtime's `/api/auth/get-session`; auth-realtime owns the Better Auth schema. - **Yjs invariant**: the Hocuspocus `documentName` is `"-"`, split on the first `-`. Never seed or merge one Y.Doc from another with `Y.applyUpdate` — use `replaceYdocContent` (`server/auth-realtime/src/ydocument.ts`). Read the document-storage section of [server/CLAUDE.md](server/CLAUDE.md) before touching branch content anywhere. - **System writes**: a protected branch refuses every persist but core's own. Core marks the batches it originates with `edit.Client.Apply`'s `system` argument, which only a caller acting for core itself may set, auth-realtime opens the direct connection under that origin, and `onStoreDocument` marks the persist system only while every change in the window came from core. Protection itself is enforced where a write enters the document — `onAuthenticate` makes a connection to a protected branch read-only — because the persist carries whole-document state and could otherwise smuggle an edit it never authorised. diff --git a/e2e/tests/prod-trust-boundary.test.ts b/e2e/tests/prod-trust-boundary.test.ts index aa96af47..6b03e455 100644 --- a/e2e/tests/prod-trust-boundary.test.ts +++ b/e2e/tests/prod-trust-boundary.test.ts @@ -86,6 +86,11 @@ const CORE_INTERNAL_ENDPOINTS = [ method: "PUT", path: "/core/api/x/documents/doc-e2e/branches/branch-e2e", }, + { + name: "a branch deletion", + method: "DELETE", + path: "/core/api/x/documents/doc-e2e/branches/branch-e2e", + }, ] as const const AUTH_REALTIME_INTERNAL_ENDPOINTS = [ diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 40169501..230f6ed1 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -89,7 +89,7 @@ Every organization is seeded at initialization with a Prometheus data source nam - `onLoadDocument` / `onStoreDocument` round-trip through core's internal `/api/x/documents/{id}/branch/{branchId}` endpoints. **Do not** use `Y.applyUpdate` to seed a freshly-created doc with content from another doc — the differing `clientID`s CRDT-merge and silently duplicate content. The codebase's pattern is `replaceYdocContent` in `server/auth-realtime/src/ydocument.ts`, which deep-clones `Y.XmlElement`s (preserving non-string attrs that `XmlElement.clone()` drops). Read the comments on `onLoadDocument` in `server/auth-realtime/src/hocuspocus.ts` and on `applyMergeToOpenDocument` in `server/auth-realtime/src/routes.ts` before touching this area. - **The main branch is identified by its `"default"` flag, not by its name.** The tree join and `FetchMainBranchContent` key on the flag; `UpdateDocumentBranch` refuses to rename the default branch so the two never disagree. Anything that creates a document's first branch — `NewDocument`, `Duplicate` — must set `Default: true`, and forks must not. - **Maintainers accumulate; nothing removes them.** The `maintainers` field on a branch update is *who edited in that persist*, not the document's maintainer set — auth-realtime's store hook sends the current session's editors and system writes send none. `UpsertDocumentMaintainers` only ever adds, so a diff against the stored set would drop everyone not editing at that moment. Removing a maintainer needs a path of its own; there is none today. -- **Fork, branch update and merge go through auth-realtime.** The web client calls `POST /documents/:documentId/branches`, `PUT /documents/:documentId/branches/:branchId` and `PUT /documents/:documentId/merge` on auth-realtime, which flushes the affected branch's pending Hocuspocus store (both sides for a merge; the `-default` alias too for the default branch) and refuses the operation if that store fails, then proxies to core's session-authed group under `/api/x` with the caller's headers. Core reads the branch's stored row, so without the flush a fork copies stale content, a merge reads it, and a protect toggle makes the pending user store be refused. After core confirms a protection change, auth-realtime drops the socket of every client on the branch's document (`resetConnections` in `hocuspocus.ts`): read-only is decided per connection in `onAuthenticate`, so each client reconnects with the permission the branch now has. Hocuspocus's own `closeConnections` is not that: it sends a close *message* on the still-open multiplexed socket, and the provider leaves the document closed until the page reloads. A merge then **directly mutates the in-memory target-branch Y.Doc** via `replaceYdocContent` (not `applyUpdate`) and immediately persists `rawContent` back through `/api/x/.../branch/:branchId` so a server restart can't reset the clientID and duplicate content on reconnect. +- **Fork, branch update, merge and branch delete go through auth-realtime.** The web client calls `POST /documents/:documentId/branches`, `PUT /documents/:documentId/branches/:branchId`, `PUT /documents/:documentId/merge` and `DELETE /documents/:documentId/branches/:branchId` on auth-realtime, which flushes the affected branch's pending Hocuspocus store (both sides for a merge; the `-default` alias too for the default branch) and refuses the operation if that store fails, then proxies to core's session-authed group under `/api/x` with the caller's headers. Core reads the branch's stored row, so without the flush a fork copies stale content, a merge reads it, and a protect toggle makes the pending user store be refused. After core confirms a protection change, auth-realtime drops the socket of every client on the branch's document (`resetConnections` in `hocuspocus.ts`): read-only is decided per connection in `onAuthenticate`, so each client reconnects with the permission the branch now has. Hocuspocus's own `closeConnections` is not that: it sends a close *message* on the still-open multiplexed socket, and the provider leaves the document closed until the page reloads. A delete flushes nothing — there is no row left to store into — but resets the branch's connections the same way once core confirms it, since a client still on the branch would otherwise keep editing a live document of a branch that is gone, with every later store refused. A merge then **directly mutates the in-memory target-branch Y.Doc** via `replaceYdocContent` (not `applyUpdate`) and immediately persists `rawContent` back through `/api/x/.../branch/:branchId` so a server restart can't reset the clientID and duplicate content on reconnect. ## External resource lifecycle (files, hooks) diff --git a/server/auth-realtime/src/core.test.ts b/server/auth-realtime/src/core.test.ts index fc7f432a..20c41d23 100644 --- a/server/auth-realtime/src/core.test.ts +++ b/server/auth-realtime/src/core.test.ts @@ -9,6 +9,7 @@ function stubHttp(response: HttpResponse = { status: 200, data: null }) { get: vi.fn().mockResolvedValue(response), post: vi.fn().mockResolvedValue(response), put: vi.fn().mockResolvedValue(response), + delete: vi.fn().mockResolvedValue(response), } } @@ -343,4 +344,26 @@ describe("createCoreClient", () => { expect(result.status).toBe(200) }) }) + + describe("deleteBranch", () => { + it("deletes core's internal branch route with the caller's headers", async ({ + expect, + }) => { + const http = stubHttp({ status: 204, data: null }) + const headers = new AxiosHeaders() + headers.set("cookie", "auth.session=abc") + + const result = await createCoreClient( + BASE_URL, + http, + ).deleteBranch("doc-1", "branch-1", { headers }) + + expect(http.delete).toHaveBeenCalledTimes(1) + expect(http.delete).toHaveBeenCalledWith( + "http://core:8080/api/x/documents/doc-1/branches/branch-1", + { headers }, + ) + expect(result.status).toBe(204) + }) + }) }) diff --git a/server/auth-realtime/src/core.ts b/server/auth-realtime/src/core.ts index c6c78ae8..5e341ad9 100644 --- a/server/auth-realtime/src/core.ts +++ b/server/auth-realtime/src/core.ts @@ -77,6 +77,7 @@ export interface HttpClient { data?: unknown, config?: RequestOptions, ): Promise + delete(url: string, config?: RequestOptions): Promise } // every call this service makes into core. Transport only: a failed @@ -122,6 +123,11 @@ export interface CoreClient { request: BranchRequest, options: RequestOptions, ): Promise + deleteBranch( + documentId: string, + branchId: string, + options: RequestOptions, + ): Promise } export function createCoreClient( @@ -220,5 +226,12 @@ export function createCoreClient( options, ) }, + + async deleteBranch(documentId, branchId, options) { + return http.delete( + `${internal}/documents/${documentId}/branches/${branchId}`, + options, + ) + }, } } diff --git a/server/auth-realtime/src/routes.test.ts b/server/auth-realtime/src/routes.test.ts index 73575009..c7a7d661 100644 --- a/server/auth-realtime/src/routes.test.ts +++ b/server/auth-realtime/src/routes.test.ts @@ -564,6 +564,70 @@ describe("createRoutes", () => { }) }) + describe("DELETE /documents/:documentId/branches/:branchId", () => { + it("forwards the deletion to core with the caller's headers, without a flush", async ({ + expect, + }) => { + const { app, core, flushDocument } = build() + + const res = await app.request( + "/documents/doc1/branches/b1", + { + method: "DELETE", + headers: { Cookie: "auth.session=abc" }, + }, + ) + + expect(res.status).toBe(204) + expect(flushDocument).toHaveBeenCalledTimes(0) + const [documentId, branchId, options] = + core.deleteBranch.mock.calls[0] ?? [] + expect(documentId).toBe("doc1") + expect(branchId).toBe("b1") + expect(options?.headers?.get("cookie")).toBe( + "auth.session=abc", + ) + }) + + it("drops the connections still on the deleted branch", async ({ + expect, + }) => { + const { app, resetConnections } = build() + + await app.request("/documents/doc1/branches/b1", { + method: "DELETE", + }) + + expect( + resetConnections.mock.calls.map( + (call) => call[0], + ), + ).toEqual(["doc1-b1"]) + }) + + it("keeps the connections when core refuses the deletion", async ({ + expect, + }) => { + const core = stubCore() + core.deleteBranch.mockResolvedValue({ + status: 409, + data: { code: "document.last_branch" }, + }) + const { app, resetConnections } = build({ core }) + + const res = await app.request( + "/documents/doc1/branches/b1", + { method: "DELETE" }, + ) + + expect(res.status).toBe(409) + expect(await res.json()).toEqual({ + code: "document.last_branch", + }) + expect(resetConnections).toHaveBeenCalledTimes(0) + }) + }) + describe("PUT /documents/:documentId/merge", () => { it("flushes the source and then the target before core merges", async ({ expect, diff --git a/server/auth-realtime/src/routes.ts b/server/auth-realtime/src/routes.ts index bcdb2569..5518f5a2 100644 --- a/server/auth-realtime/src/routes.ts +++ b/server/auth-realtime/src/routes.ts @@ -357,6 +357,40 @@ export function createRoutes({ ) }) + // a delete stores nothing — there is no row left to store into — but + // the clients still on the branch hold a live document of it and would + // keep editing one that is gone, every later store refused. Once core + // confirms, each is dropped and finds no branch to reconnect to. + app.delete("/documents/:documentId/branches/:branchId", async (c) => { + const documentId = c.req.param("documentId") + const branchId = c.req.param("branchId") + + const response = await calledCore( + c, + "failed to delete branch", + () => + core.deleteBranch(documentId, branchId, { + headers: toAxiosHeaders( + c.req.raw.headers, + ), + }), + ) + if (response instanceof Response) { + return response + } + + if (response.status === 204) { + resetConnections(`${documentId}-${branchId}`) + + return c.body(null, 204) + } + + return c.json( + response.data, + response.status as ContentfulStatusCode, + ) + }) + // a merge reads the source branch's row and rewrites the target's; // both are flushed so neither side is behind its editors. The // in-memory Y.Doc mutation below only runs when core responds 200, diff --git a/server/auth-realtime/src/test-helpers.ts b/server/auth-realtime/src/test-helpers.ts index 9ff96493..b8a63d10 100644 --- a/server/auth-realtime/src/test-helpers.ts +++ b/server/auth-realtime/src/test-helpers.ts @@ -64,6 +64,10 @@ export function stubCore(): StubCore { status: 200, data: { branchId: "branch-1" }, }), + deleteBranch: vi.fn().mockResolvedValue({ + status: 204, + data: null, + }), } } diff --git a/server/core/internal/server/internal/document/branch_http.go b/server/core/internal/server/internal/document/branch_http.go new file mode 100644 index 00000000..a142dc96 --- /dev/null +++ b/server/core/internal/server/internal/document/branch_http.go @@ -0,0 +1,667 @@ +package document + +import ( + "context" + "errors" + "log/slog" + "net/http" + "slices" + + "github.com/guregu/null/v5" + "github.com/oxynote/oxynote/server/core/internal/apps/webchange" + documentCore "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/hook" + "github.com/oxynote/oxynote/server/core/internal/search" + "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" + "github.com/oxynote/oxynote/server/core/pkg/errutil" + "github.com/oxynote/oxynote/server/core/pkg/httpserver" + "github.com/oxynote/oxynote/server/core/pkg/logutil" + "github.com/rs/xid" +) + +// UpdateDocumentBranch handles updating the name and protection status of a document branch. +func (h *Handler) UpdateDocumentBranch(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + id, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + branchID, err := httpserver.ExtractNamedID(r, "branchId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + doc, err := h.db.FetchDocumentByBranchID(r.Context(), branchID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if doc.ID != id { + httpserver.RespondError(h.log, w, ErrBranchMismatch) + return + } + + var ui struct { + Name null.String `json:"name"` + Protected null.Bool `json:"protected"` + } + + if err := httpserver.DecodeJSON(r, &ui); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + // the main branch is found by name in the tree and content queries, and + // a rename would make the document look nameless and contentless. + if doc.Default && ui.Name.Valid && ui.Name.String != doc.BranchName { + httpserver.RespondError(h.log, w, ErrDefaultBranchRename) + return + } + + ndoc := doc.ApplyBranchUpdate(ui.Name, ui.Protected, session.UserID) + + var tx Tx + + if err := h.db.BeginTx(r.Context(), &tx); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + defer tx.Rollback() //nolint:errcheck // error provides no meaningful info + + if err := tx.UpdateDocumentBranchMetadata(r.Context(), ndoc); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + // the index carries the branch name on every entry of the branch. + if ndoc.BranchName != doc.BranchName { + if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(doc.Search(), ndoc.Search())); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + } + + if err := tx.Commit(); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if h.metadata.changeCallback != nil { + h.metadata.changeCallback(session.ActiveOrganizationID, ndoc) + } + + httpserver.Respond( + h.log, + w, + ndoc, + http.StatusOK, + ) +} + +// FetchDocumentBranchesUnsafe handles the retrieval of all branches for a document +// without organization ownership checks. Intended for internal system use only. +func (h *Handler) FetchDocumentBranchesUnsafe(w http.ResponseWriter, r *http.Request) { + id, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + branches, err := h.db.FetchDocumentBranchesUnsafe(r.Context(), id) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + httpserver.Respond( + h.log, + w, + branches, + http.StatusOK, + ) +} + +// FetchDocumentBranchByIDUnsafe handles the retrieval of a document branch by its ID +// without organization ownership checks. Intended for internal system use only. +func (h *Handler) FetchDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http.Request) { + branchID, err := httpserver.ExtractNamedID(r, "branchId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + doc, err := h.db.FetchDocumentUnsafeByBranchID(r.Context(), branchID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + httpserver.Respond( + h.log, + w, + doc, + http.StatusOK, + ) +} + +// UpdateDocumentBranchByIDUnsafe handles the update of a document branch content by branch ID +// without organization ownership checks. Intended for internal system use only. +func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http.Request) { + branchID, err := httpserver.ExtractNamedID(r, "branchId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + doc, err := h.db.FetchDocumentUnsafeByBranchID(r.Context(), branchID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var ui documentCore.UpdateInput + + if err = httpserver.DecodeJSON(r, &ui); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + ui.Branch = doc.BranchName + + ndoc, err := doc.ApplyUpdate(ui) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var tx Tx + + err = h.db.BeginTx(r.Context(), &tx) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + defer tx.Rollback() //nolint:errcheck // error provides no meaningful info + + if err = tx.UpdateDocument(r.Context(), ndoc); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + maintainers, err := tx.FetchDocumentMaintainers(r.Context(), doc.ID, doc.OrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + // the update carries the editors of one persist, so the set is only + // ever added to. Diffing it against the stored maintainers would drop + // everyone who happens not to be editing right now. + var maintainersAdded bool + + for _, maintainer := range ui.Maintainers { + if !slices.Contains(maintainers, maintainer) { + maintainersAdded = true + break + } + } + + if maintainersAdded { + if err = tx.UpsertDocumentMaintainers( + r.Context(), + doc.ID, + doc.OrganizationID, + ui.Maintainers, + ); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + } + + if err = h.searchJobs.Enqueue( + r.Context(), + tx, + search.BlocksDiff(doc.Search(), ndoc.Search()), + ); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + err = tx.Commit() + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if h.metadata.changeCallback != nil { + h.metadata.changeCallback(doc.OrganizationID, ndoc) + } + + if maintainersAdded && h.maintainers.changeCallback != nil { + h.maintainers.changeCallback(doc.OrganizationID, doc.ID) + } + + httpserver.Respond( + h.log, + w, + ndoc, + http.StatusOK, + ) +} + +// MergeBranches merges the content of a source branch into a target branch. +// The target branch's hooks are detached for the hook manager to tear down +// and replaced with copies from the source. +// Target branch comments are cleared. Source branch reviewer approvals are promoted. +func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + id, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var inp struct { + FromBranchID xid.ID `json:"fromBranchId"` + ToBranchID xid.ID `json:"toBranchId"` + } + + if err = httpserver.DecodeJSON(r, &inp); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + // a self-merge would soft-delete the branch's hooks and comments and + // then copy the hooks back from the same, now hook-less, branch — + // permanently destroying both. + if inp.FromBranchID == inp.ToBranchID { + httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "document.branch_self_merge", "cannot merge a branch into itself")) + return + } + + fromDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.FromBranchID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + toDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.ToBranchID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if fromDoc.ID != id || toDoc.ID != id { + httpserver.RespondError(h.log, w, ErrBranchMismatch) + return + } + + ndoc := toDoc.MergeBranch(fromDoc.Branch, session.UserID) + + var tx Tx + + if err := h.db.BeginTx(r.Context(), &tx); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + defer tx.Rollback() //nolint:errcheck // error provides no meaningful info + + if err := tx.DetachDocumentHooksByBranchID(r.Context(), toDoc.BranchID, session.ActiveOrganizationID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.DeleteDocumentCommentsByBranchID(r.Context(), toDoc.BranchID, session.ActiveOrganizationID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.UpdateDocument(r.Context(), ndoc); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.ReplaceBranchTags(r.Context(), session.ActiveOrganizationID, fromDoc.BranchID, toDoc.BranchID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(toDoc.Search(), ndoc.Search())); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.PromoteBranchApprovals(r.Context(), fromDoc.BranchID, toDoc.BranchID, session.ActiveOrganizationID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.Commit(); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + h.copyHooksToBranch( + r.Context(), + fromDoc.BranchID, + toDoc.BranchID, + toDoc.ID, + session.ActiveOrganizationID, + nil, + ) + + if h.metadata.changeCallback != nil { + h.metadata.changeCallback(session.ActiveOrganizationID, ndoc) + } + + if h.reviewers.changeCallback != nil { + h.reviewers.changeCallback(session.ActiveOrganizationID, toDoc.ID) + } + + httpserver.Respond( + h.log, + w, + ndoc, + http.StatusOK, + ) +} + +// FetchDocumentBranches handles the retrieval of all branches for a document. +func (h *Handler) FetchDocumentBranches(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + docID, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + branches, err := h.db.FetchDocumentBranches(r.Context(), docID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + httpserver.Respond( + h.log, + w, + branches, + http.StatusOK, + ) +} + +// CreateDocumentBranch handles the creation of a new branch forked from an existing source branch. +// Hooks from the source branch are copied to the new branch. +func (h *Handler) CreateDocumentBranch(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + id, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var inp struct { + Branch string `json:"branch"` + SourceBranchID xid.ID `json:"sourceBranchId"` + } + + if err = httpserver.DecodeJSON(r, &inp); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if inp.Branch == "" { + httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) + return + } + + sourceDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.SourceBranchID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if sourceDoc.ID != id { + httpserver.RespondError(h.log, w, ErrBranchMismatch) + return + } + + newDoc := sourceDoc.Fork(inp.Branch, session.UserID) + + var tx Tx + + if err = h.db.BeginTx(r.Context(), &tx); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + defer tx.Rollback() //nolint:errcheck // error provides no meaningful info + + if err = tx.InsertDocumentBranch(r.Context(), newDoc); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.CopyBranchTags(r.Context(), session.ActiveOrganizationID, sourceDoc.BranchID, newDoc.BranchID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(nil, newDoc.Search())); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.Commit(); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + h.copyHooksToBranch( + r.Context(), + sourceDoc.BranchID, + newDoc.BranchID, + sourceDoc.ID, + session.ActiveOrganizationID, + nil, + ) + + httpserver.Respond( + h.log, + w, + newDoc, + http.StatusCreated, + ) +} + +// DeleteDocumentBranch handles the deletion of a specific document branch by ID. +// Returns 409 if it is the last remaining branch for the document. +func (h *Handler) DeleteDocumentBranch(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + id, err := httpserver.ExtractNamedID(r, "documentId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + branchID, err := httpserver.ExtractNamedID(r, "branchId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + branchDoc, err := h.db.FetchDocumentByBranchID(r.Context(), branchID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if branchDoc.ID != id { + httpserver.RespondError(h.log, w, ErrBranchMismatch) + return + } + + if branchDoc.Default { + httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.default_branch", "cannot delete the default branch")) + return + } + + count, err := h.db.CountDocumentBranches(r.Context(), branchDoc.ID, session.ActiveOrganizationID) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if count <= 1 { + httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.last_branch", "cannot delete the last branch")) + return + } + + var tx Tx + + if err := h.db.BeginTx(r.Context(), &tx); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + defer tx.Rollback() //nolint:errcheck // error provides no meaningful info + + if err := tx.DeleteDocumentBranchByID(r.Context(), branchID, session.ActiveOrganizationID); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDifference{ + RemovedBranches: []search.BranchRemoval{{DocumentID: branchDoc.ID, BranchID: branchID}}, + }); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := tx.Commit(); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + httpserver.Respond( + h.log, + w, + nil, + http.StatusNoContent, + ) +} + +// copyHooksToBranch fetches all hooks from fromBranchID and re-creates them on +// toBranchID with fresh state, once the target is committed. Creating a hook +// creates its external resource as a side effect, so the copy cannot run +// inside the caller's transaction: a rollback cannot take a changedetection.io +// watcher back, and the row that would have pointed at it is gone. A failed +// insert tears the just-created resource down again for the same reason. A +// failure stops the copy and leaves the target standing with fewer hooks than +// its source, which the critical log reports. +// +// A branch whose content was duplicated carries fresh block uids, so the +// caller passes the old-to-new uid map and a hook anchored to a block is +// re-anchored through it; a hook whose block the map does not name has +// nothing to point at on the target and is dropped. A nil map keeps every +// block id as it is, which is right for a fork or a merge. +func (h *Handler) copyHooksToBranch( + ctx context.Context, + fromBranchID, toBranchID, documentID xid.ID, + organizationID string, + uids map[string]string, +) { + hooks, err := h.db.FetchDocumentHooksByBranchID(ctx, fromBranchID, organizationID) + if err != nil { + logutil.Critical(h.log, err).Error( + "cannot fetch the hooks to copy", + slog.String("branch_id", fromBranchID.String()), + ) + + return + } + + inp := hook.NewInput(organizationID, h.githubMan, h.webchangeClient) + + for _, hk := range hooks { + blockID := hk.BlockID + + if uids != nil && blockID.Valid { + uid, ok := uids[blockID.String] + if !ok { + continue + } + + blockID = null.StringFrom(uid) + } + + newHk, err := hook.NewHook(ctx, hook.CreateInput{ + Type: hk.Type, + BranchID: toBranchID, + BlockID: blockID, + Settings: hk.Settings, + }, documentID, toBranchID, organizationID, inp) + if err != nil { + // a hook whose integration is not configured cannot get its + // external resource; dropping it from the copy beats failing + // the whole fork or merge over it. + if errors.Is(err, webchange.ErrNotConfigured) { + h.log.With("hook_id", hk.ID). + Warn("skipping hook copy: its integration is not configured") + + continue + } + + logutil.Critical(h.log, err).Error( + "cannot re-create the hook on the target branch", + slog.String("hook_id", hk.ID.String()), + slog.String("branch_id", toBranchID.String()), + ) + + return + } + + if err := h.db.InsertDocumentHook(ctx, *newHk); err != nil { + if derr := newHk.Delete(ctx, inp); derr != nil { + logutil.Critical(h.log, derr).Error( + "cannot delete the hook external resource after a failed insert", + slog.String("hook_id", newHk.ID.String()), + ) + } + + logutil.Critical(h.log, err).Error( + "cannot insert the copied hook", + slog.String("hook_id", newHk.ID.String()), + slog.String("branch_id", toBranchID.String()), + ) + + return + } + } +} 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 31f09603..cdc6e021 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -3,7 +3,6 @@ package document import ( "context" "errors" - "fmt" "net/http" "net/http/httptest" "strings" @@ -11,7 +10,6 @@ import ( "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" - "github.com/oxynote/oxynote/server/core/internal/document/file" hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/document/hook/processor" "github.com/oxynote/oxynote/server/core/internal/search" @@ -416,8 +414,8 @@ func Test_Handler_MergeBranches(t *testing.T) { }, Tx: &TxMock{}, Body: validBody, - RespCode: http.StatusBadRequest, - RespJSON: `{"code":"document.branch_mismatch","message":"branches must belong to the same document"}`, + RespCode: http.StatusNotFound, + RespJSON: `{"code":"document.branch_mismatch","message":"branch does not belong to the document"}`, }, "Branches of another document": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchOtherDocByBranch}, @@ -605,492 +603,6 @@ func Test_Handler_MergeBranches(t *testing.T) { } } -func Test_Handler_DeleteDocument(t *testing.T) { - fetchStored := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { - return storedDoc(), nil - } - - cc := map[string]struct { - DB *DBMock - Tx *TxMock - BeginErr error - NoSession bool - OmitDoc bool - RespCode int - Committed int - TreeCbs int - }{ - "No session in context": { - DB: &DBMock{}, - Tx: &TxMock{}, - NoSession: true, - RespCode: http.StatusUnauthorized, - }, - "Missing document ID parameter": { - DB: &DBMock{}, - Tx: &TxMock{}, - OmitDoc: true, - RespCode: http.StatusNotFound, - }, - "Document fetch error": { - DB: &DBMock{ - FetchDocumentFunc: func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { - return nil, errors.New("boom") - }, - }, - Tx: &TxMock{}, - RespCode: http.StatusInternalServerError, - }, - "Hook fetch error": { - DB: &DBMock{ - FetchDocumentFunc: fetchStored, - FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return nil, errors.New("boom") - }, - }, - Tx: &TxMock{}, - RespCode: http.StatusInternalServerError, - }, - "Hook cleanup error": { - DB: &DBMock{ - FetchDocumentFunc: fetchStored, - FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{storedHook("bogus")}, nil - }, - }, - Tx: &TxMock{}, - RespCode: http.StatusInternalServerError, - }, - "Transaction start error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{}, - BeginErr: errors.New("boom"), - RespCode: http.StatusInternalServerError, - }, - "Document deletion error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{ - DeleteDocumentFunc: func(context.Context, xid.ID, string) ([]xid.ID, error) { - return nil, errors.New("boom") - }, - }, - RespCode: http.StatusInternalServerError, - }, - "Search removal enqueue error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{ - DeleteDocumentFunc: func(context.Context, xid.ID, string) ([]xid.ID, error) { - return []xid.ID{_documentID}, nil - }, - InsertDocumentSearchJobFunc: func(context.Context, search.BlocksDifference) error { - return errors.New("boom") - }, - }, - RespCode: http.StatusInternalServerError, - }, - "Commit error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{ - CommitFunc: func() error { - return errors.New("boom") - }, - }, - RespCode: http.StatusInternalServerError, - Committed: 1, - }, - "Successful deletion with hook cleanup": { - DB: &DBMock{ - FetchDocumentFunc: fetchStored, - FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil - }, - }, - Tx: &TxMock{ - DeleteDocumentFunc: func(_ context.Context, id xid.ID, _ string) ([]xid.ID, error) { - return []xid.ID{id}, nil - }, - }, - RespCode: http.StatusOK, - Committed: 1, - TreeCbs: 1, - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) - - rec := httptest.NewRecorder() - - hdl.DeleteDocument(rec, newRequest(http.MethodDelete, "", c.NoSession, c.OmitDoc, true)) - - assert.Equal(t, c.RespCode, rec.Code) - assert.Len(t, c.Tx.CommitCalls(), c.Committed) - assert.Equal(t, c.TreeCbs, cnt.tree) - - if c.RespCode == http.StatusOK { - require.Len(t, c.Tx.DeleteDocumentCalls(), 1) - assert.Equal(t, _documentID, c.Tx.DeleteDocumentCalls()[0].ID) - - // the removal of the returned subtree ids rides the same - // transaction as the delete. - ff := c.Tx.InsertDocumentSearchJobCalls() - require.Len(t, ff, 1) - assert.Equal(t, []xid.ID{_documentID}, ff[0].Diff.RemovedDocuments) - } - }) - } -} - -func Test_Handler_DuplicateDocument(t *testing.T) { - fetchStored := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { - return storedDoc(), nil - } - - // insertAwareTx returns the freshly inserted duplicate in the parent - // subtree so the sort-index swap can find it. - insertAwareTx := func() *TxMock { - tx := &TxMock{} - tx.FetchDocumentTreeByDocumentParentIDFunc = func(context.Context, null.Value[xid.ID], string) (documentCore.Summaries, error) { - return documentCore.Summaries{{ID: tx.InsertDocumentCalls()[0].Doc.ID}}, nil - } - - return tx - } - - // fetchStoredWithImage serves a document whose content holds an image - // served by that same document, which is what duplication has to copy. - fetchStoredWithImage := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { - doc := storedDoc() - doc.Content = documentCore.RootBlock{ - Type: documentCore.BlockNodeDoc, - Content: []documentCore.Block{ - { - Type: documentCore.BlockNodeImageBlock, - Attrs: documentCore.Attributes{ - "uid": "img1", - "src": "https://app.test/core" + fmt.Sprintf(documentCore.FilePathFormat, _documentID, "img1"), - }, - }, - }, - } - - return doc, nil - } - - // imageDB serves the document with the image plus the file row that - // image refers to, which is what the copy is made from. - imageDB := func() *DBMock { - return &DBMock{ - FetchDocumentFunc: fetchStoredWithImage, - FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { - f := file.NewFile(id, file.LocationDocument, "key", _documentID, organizationID) - - return &f, nil - }, - } - } - - cc := map[string]struct { - DB *DBMock - Tx *TxMock - Storer *StorerMock - BeginErr error - NoSession bool - OmitDoc bool - RespCode int - Committed int - TreeCbs int - Copies int - CopiedHooks int - }{ - "No session in context": { - DB: &DBMock{}, - Tx: &TxMock{}, - NoSession: true, - RespCode: http.StatusUnauthorized, - }, - "Missing document ID parameter": { - DB: &DBMock{}, - Tx: &TxMock{}, - OmitDoc: true, - RespCode: http.StatusNotFound, - }, - "Document fetch error": { - DB: &DBMock{ - FetchDocumentFunc: func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { - return nil, errors.New("boom") - }, - }, - Tx: &TxMock{}, - RespCode: http.StatusInternalServerError, - }, - "Transaction start error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{}, - BeginErr: errors.New("boom"), - RespCode: http.StatusInternalServerError, - }, - "Duplicate insertion error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{ - InsertDocumentFunc: func(context.Context, documentCore.Document) error { - return errors.New("boom") - }, - }, - RespCode: http.StatusInternalServerError, - }, - "Tag copy error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{ - CopyBranchTagsFunc: func(context.Context, string, xid.ID, xid.ID) error { - return errors.New("boom") - }, - }, - RespCode: http.StatusInternalServerError, - }, - "Sort index swap error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: &TxMock{}, - RespCode: http.StatusBadRequest, - }, - "Commit error": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: func() *TxMock { - tx := insertAwareTx() - tx.CommitFunc = func() error { - return errors.New("boom") - } - - return tx - }(), - RespCode: http.StatusInternalServerError, - Committed: 1, - }, - "Successful duplication": { - DB: &DBMock{FetchDocumentFunc: fetchStored}, - Tx: insertAwareTx(), - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - }, - "Files are copied along with the document": { - DB: imageDB(), - Tx: insertAwareTx(), - Storer: &StorerMock{}, - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - Copies: 1, - }, - "Failing file copy still yields the duplicate": { - DB: imageDB(), - Tx: insertAwareTx(), - Storer: &StorerMock{ - CopyFunc: func(context.Context, string, string, string, string) error { - return errors.New("boom") - }, - }, - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - Copies: 1, - }, - "Hooks are copied along with the document": { - DB: func() *DBMock { - blockHook := storedHook(hookCore.TypeScheduledReminder) - blockHook.BlockID = null.StringFrom("img1") - - db := imageDB() - db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{blockHook}, nil - } - - return db - }(), - Tx: insertAwareTx(), - Storer: &StorerMock{}, - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - Copies: 1, - CopiedHooks: 1, - }, - "Failing hook copy still yields the duplicate": { - DB: func() *DBMock { - db := &DBMock{FetchDocumentFunc: fetchStored} - db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return nil, errors.New("boom") - } - - return db - }(), - Tx: insertAwareTx(), - RespCode: http.StatusCreated, - Committed: 1, - TreeCbs: 1, - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) - - if c.Storer != nil { - hdl.storer = c.Storer - } - - rec := httptest.NewRecorder() - - hdl.DuplicateDocument(rec, newRequest(http.MethodPost, "", c.NoSession, c.OmitDoc, true)) - - assert.Equal(t, c.RespCode, rec.Code) - assert.Len(t, c.Tx.CommitCalls(), c.Committed) - assert.Equal(t, c.TreeCbs, cnt.tree) - - if c.Storer != nil { - assert.Len(t, c.Storer.CopyCalls(), c.Copies) - assert.Len(t, c.DB.InsertDocumentFileCalls(), c.Copies) - } - - if c.RespCode == http.StatusCreated { - // the duplicate is a new document owned by the caller. - require.Len(t, c.Tx.InsertDocumentCalls(), 1) - dupl := c.Tx.InsertDocumentCalls()[0].Doc - assert.NotEqual(t, _documentID, dupl.ID) - assert.Equal(t, []string{"u1"}, c.Tx.UpsertDocumentMaintainersCalls()[0].MaintainerIDs) - - // the copy's default branch takes the source's tags before - // the commit. - require.Len(t, c.Tx.CopyBranchTagsCalls(), 1) - assert.Equal(t, _branchID, c.Tx.CopyBranchTagsCalls()[0].FromBranchID) - assert.Equal(t, dupl.BranchID, c.Tx.CopyBranchTagsCalls()[0].ToBranchID) - - // the hooks are copied once the commit is through, since - // creating one creates its watcher. - assert.Empty(t, c.Tx.InsertDocumentHookCalls()) - require.Len(t, c.DB.InsertDocumentHookCalls(), c.CopiedHooks) - - // a hook anchored to a block follows the block's regenerated - // uid rather than pointing at the source's. - for _, call := range c.DB.InsertDocumentHookCalls() { - assert.Equal(t, null.ValueFrom(dupl.BranchID), call.Hk.BranchID) - assert.NotEqual(t, null.StringFrom("img1"), call.Hk.BlockID) - assert.True(t, call.Hk.BlockID.Valid) - } - } - }) - } -} - -func Test_Handler_copyDocumentFiles(t *testing.T) { - toDocumentID := xid.New() - - fileDB := func() *DBMock { - return &DBMock{ - FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { - f := file.NewFile(id, file.LocationComment, "key", _documentID, organizationID) - - return &f, nil - }, - } - } - - cc := map[string]struct { - DB *DBMock - Storer *StorerMock - Files map[string]string - Inserts int - Copies int - }{ - "Nothing to copy": { - DB: fileDB(), - Storer: &StorerMock{}, - Files: map[string]string{}, - }, - "File row is missing": { - DB: &DBMock{ - FetchDocumentFileFunc: func(context.Context, string, string) (*file.File, error) { - return nil, errors.New("boom") - }, - }, - Storer: &StorerMock{}, - Files: map[string]string{"old-1": "new-1"}, - }, - "Error returned by db.InsertDocumentFile": { - DB: func() *DBMock { - db := fileDB() - db.InsertDocumentFileFunc = func(context.Context, file.File) error { - return errors.New("boom") - } - - return db - }(), - Storer: &StorerMock{}, - Files: map[string]string{"old-1": "new-1"}, - Inserts: 1, - }, - "Error returned by storer.Copy": { - DB: fileDB(), - Storer: &StorerMock{ - CopyFunc: func(context.Context, string, string, string, string) error { - return errors.New("boom") - }, - }, - Files: map[string]string{"old-1": "new-1"}, - Inserts: 1, - Copies: 1, - }, - "Successful copy": { - DB: fileDB(), - Storer: &StorerMock{}, - Files: map[string]string{"old-1": "new-1"}, - Inserts: 1, - Copies: 1, - }, - } - - for cn, c := range cc { - t.Run(cn, func(t *testing.T) { - t.Parallel() - - hdl, _ := newTestHandler(c.DB, &fakePublisher{}) - hdl.storer = c.Storer - - hdl.copyDocumentFiles(context.Background(), c.Files, _documentID, toDocumentID, "org1") - - ff := c.DB.InsertDocumentFileCalls() - require.Len(t, ff, c.Inserts) - - cf := c.Storer.CopyCalls() - require.Len(t, cf, c.Copies) - - if c.Inserts == 0 { - return - } - - // the copy owns its row, keyed under the new document. - assert.Equal(t, "new-1", ff[0].F.ID) - assert.Equal(t, file.LocationComment, ff[0].F.Location) - assert.Equal(t, file.Key("org1", toDocumentID, "new-1"), ff[0].F.StorageKey) - assert.Equal(t, toDocumentID, ff[0].F.DocumentID.V) - - if c.Copies == 0 { - return - } - - assert.Equal(t, file.Folder("org1", _documentID), cf[0].SrcFolder) - assert.Equal(t, "old-1", cf[0].SrcID) - assert.Equal(t, file.Folder("org1", toDocumentID), cf[0].DstFolder) - assert.Equal(t, "new-1", cf[0].DstID) - }) - } -} - func Test_Handler_FetchDocumentBranches(t *testing.T) { cc := map[string]struct { DB *DBMock @@ -1546,8 +1058,7 @@ func Test_Handler_copyHooksToBranch(t *testing.T) { hdl, _ := newTestHandler(db, &fakePublisher{}) - err := hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) - require.NoError(t, err) + hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) ff := db.InsertDocumentHookCalls() require.Len(t, ff, 1) @@ -1575,7 +1086,7 @@ func Test_Handler_copyHooksToBranch(t *testing.T) { hdl, _ := newTestHandler(db, &fakePublisher{}) - err := hdl.copyHooksToBranch( + hdl.copyHooksToBranch( context.Background(), _branchID2, _branchID, @@ -1583,7 +1094,6 @@ func Test_Handler_copyHooksToBranch(t *testing.T) { "org1", map[string]string{"old-uid": "new-uid"}, ) - require.NoError(t, err) ff := db.InsertDocumentHookCalls() require.Len(t, ff, 2) @@ -1607,10 +1117,176 @@ func Test_Handler_copyHooksToBranch(t *testing.T) { hdl, _ := newTestHandler(db, &fakePublisher{}) - err := hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) - require.Error(t, err) + hdl.copyHooksToBranch(context.Background(), _branchID2, _branchID, _documentID, "org1", nil) // the failure stops the copy rather than working through the rest of // the branch's hooks and creating a watcher for each. require.Len(t, db.InsertDocumentHookCalls(), 1) } + +func Test_Handler_UpdateDocumentBranch(t *testing.T) { + validBody := `{"name":"Renamed","protected":true}` + + cc := map[string]struct { + DB *DBMock + Tx *TxMock + NoSession bool + OmitDoc bool + Body string + WantName string + RespCode int + Updated int + Metadata int + Jobs int + }{ + "No session in context": { + DB: &DBMock{}, + NoSession: true, + Body: validBody, + RespCode: http.StatusUnauthorized, + }, + "Missing document ID parameter": { + DB: &DBMock{}, + OmitDoc: true, + Body: validBody, + RespCode: http.StatusNotFound, + }, + "Branch document fetch error": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return nil, errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, + "Branch of another document": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + doc := branchDoc(_branchID) + doc.ID = xid.New() + + return doc, nil + }, + }, + Body: validBody, + RespCode: http.StatusNotFound, + }, + "Invalid JSON body": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Body: "{", + RespCode: http.StatusBadRequest, + }, + "Metadata update error": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return branchDoc(_branchID), nil + }, + }, + Tx: &TxMock{ + UpdateDocumentBranchMetadataFunc: func(context.Context, documentCore.Document) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + Updated: 1, + }, + "Search job insert error": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return branchDoc(_branchID), nil + }, + }, + Tx: &TxMock{ + InsertDocumentSearchJobFunc: func(context.Context, search.BlocksDifference) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + Updated: 1, + Jobs: 1, + }, + // the main branch is found by its flag in the tree and content + // queries, but its name is what the user sees; renaming it would + // leave the document looking nameless. + "Renaming the default branch is rejected": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Body: validBody, + RespCode: http.StatusBadRequest, + }, + "Protecting the default branch is allowed": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Body: `{"protected":true}`, + WantName: documentCore.DefaultBranch, + RespCode: http.StatusOK, + Updated: 1, + Metadata: 1, + }, + "Successful update": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return branchDoc(_branchID), nil + }, + }, + Body: validBody, + WantName: "Renamed", + RespCode: http.StatusOK, + Updated: 1, + Metadata: 1, + Jobs: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + tx := c.Tx + if tx == nil { + tx = &TxMock{} + } + + hdl, cnt := newTestHandler(withTx(c.DB, tx, nil), &fakePublisher{}) + + rec := httptest.NewRecorder() + + hdl.UpdateDocumentBranch(rec, newRequest(http.MethodPut, c.Body, c.NoSession, c.OmitDoc, false)) + + assert.Equal(t, c.RespCode, rec.Code) + assert.Len(t, tx.UpdateDocumentBranchMetadataCalls(), c.Updated) + assert.Len(t, tx.InsertDocumentSearchJobCalls(), c.Jobs) + assert.Equal(t, c.Metadata, cnt.metadata) + + if c.RespCode == http.StatusOK { + ff := tx.UpdateDocumentBranchMetadataCalls() + require.NotEmpty(t, ff) + assert.Equal(t, c.WantName, ff[0].Doc.BranchName) + assert.True(t, ff[0].Doc.Protected) + assert.Len(t, tx.CommitCalls(), 1) + } + + // a rename rewrites every entry of the branch, since each one + // carries the branch name. + if c.Jobs == 1 && c.RespCode == http.StatusOK { + diff := tx.InsertDocumentSearchJobCalls()[0].Diff + require.Len(t, diff.Updated, 1) + assert.Equal(t, _branchID.String()+"-docname", diff.Updated[0].ID) + assert.Equal(t, "Renamed", diff.Updated[0].BranchName) + } + }) + } +} diff --git a/server/core/internal/server/internal/document/http.go b/server/core/internal/server/internal/document/document_http.go similarity index 65% rename from server/core/internal/server/internal/document/http.go rename to server/core/internal/server/internal/document/document_http.go index e689fca8..ecb53e9f 100644 --- a/server/core/internal/server/internal/document/http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -6,7 +6,6 @@ import ( "log/slog" "net/http" "net/url" - "slices" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/github" @@ -568,246 +567,6 @@ func (h *Handler) CreateDocument(w http.ResponseWriter, r *http.Request) { ) } -// UpdateDocumentBranch handles updating the name and protection status of a document branch. -func (h *Handler) UpdateDocumentBranch(w http.ResponseWriter, r *http.Request) { - session, ok := auth.RequireSession(h.log, w, r) - if !ok { - return - } - - id, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - branchID, err := httpserver.ExtractNamedID(r, "branchId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - doc, err := h.db.FetchDocumentByBranchID(r.Context(), branchID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if doc.ID != id { - httpserver.RespondError(h.log, w, ErrBranchMismatch) - return - } - - var ui struct { - Name null.String `json:"name"` - Protected null.Bool `json:"protected"` - } - - if err := httpserver.DecodeJSON(r, &ui); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // the main branch is found by name in the tree and content queries, and - // a rename would make the document look nameless and contentless. - if doc.Default && ui.Name.Valid && ui.Name.String != doc.BranchName { - httpserver.RespondError(h.log, w, ErrDefaultBranchRename) - return - } - - ndoc := doc.ApplyBranchUpdate(ui.Name, ui.Protected, session.UserID) - - var tx Tx - - if err := h.db.BeginTx(r.Context(), &tx); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - - if err := tx.UpdateDocumentBranchMetadata(r.Context(), ndoc); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // the index carries the branch name on every entry of the branch. - if ndoc.BranchName != doc.BranchName { - if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(doc.Search(), ndoc.Search())); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - } - - if err := tx.Commit(); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if h.metadata.changeCallback != nil { - h.metadata.changeCallback(session.ActiveOrganizationID, ndoc) - } - - httpserver.Respond( - h.log, - w, - ndoc, - http.StatusOK, - ) -} - -// FetchDocumentBranchesUnsafe handles the retrieval of all branches for a document -// without organization ownership checks. Intended for internal system use only. -func (h *Handler) FetchDocumentBranchesUnsafe(w http.ResponseWriter, r *http.Request) { - id, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - branches, err := h.db.FetchDocumentBranchesUnsafe(r.Context(), id) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - httpserver.Respond( - h.log, - w, - branches, - http.StatusOK, - ) -} - -// FetchDocumentBranchByIDUnsafe handles the retrieval of a document branch by its ID -// without organization ownership checks. Intended for internal system use only. -func (h *Handler) FetchDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http.Request) { - branchID, err := httpserver.ExtractNamedID(r, "branchId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - doc, err := h.db.FetchDocumentUnsafeByBranchID(r.Context(), branchID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - httpserver.Respond( - h.log, - w, - doc, - http.StatusOK, - ) -} - -// UpdateDocumentBranchByIDUnsafe handles the update of a document branch content by branch ID -// without organization ownership checks. Intended for internal system use only. -func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http.Request) { - branchID, err := httpserver.ExtractNamedID(r, "branchId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - doc, err := h.db.FetchDocumentUnsafeByBranchID(r.Context(), branchID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - var ui documentCore.UpdateInput - - if err = httpserver.DecodeJSON(r, &ui); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - ui.Branch = doc.BranchName - - ndoc, err := doc.ApplyUpdate(ui) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - var tx Tx - - err = h.db.BeginTx(r.Context(), &tx) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - - if err = tx.UpdateDocument(r.Context(), ndoc); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - maintainers, err := tx.FetchDocumentMaintainers(r.Context(), doc.ID, doc.OrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // the update carries the editors of one persist, so the set is only - // ever added to. Diffing it against the stored maintainers would drop - // everyone who happens not to be editing right now. - var maintainersAdded bool - - for _, maintainer := range ui.Maintainers { - if !slices.Contains(maintainers, maintainer) { - maintainersAdded = true - break - } - } - - if maintainersAdded { - if err = tx.UpsertDocumentMaintainers( - r.Context(), - doc.ID, - doc.OrganizationID, - ui.Maintainers, - ); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - } - - if err = h.searchJobs.Enqueue( - r.Context(), - tx, - search.BlocksDiff(doc.Search(), ndoc.Search()), - ); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - err = tx.Commit() - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if h.metadata.changeCallback != nil { - h.metadata.changeCallback(doc.OrganizationID, ndoc) - } - - if maintainersAdded && h.maintainers.changeCallback != nil { - h.maintainers.changeCallback(doc.OrganizationID, doc.ID) - } - - httpserver.Respond( - h.log, - w, - ndoc, - http.StatusOK, - ) -} - // UpdateBranchReviewApproval handles setting the current user's approval state on a branch. func (h *Handler) UpdateBranchReviewApproval(w http.ResponseWriter, r *http.Request) { session, ok := auth.RequireSession(h.log, w, r) @@ -874,140 +633,6 @@ func (h *Handler) UpdateBranchReviewApproval(w http.ResponseWriter, r *http.Requ ) } -// MergeBranches merges the content of a source branch into a target branch. -// The target branch's hooks are detached for the hook manager to tear down -// and replaced with copies from the source. -// Target branch comments are cleared. Source branch reviewer approvals are promoted. -func (h *Handler) MergeBranches(w http.ResponseWriter, r *http.Request) { - session, ok := auth.RequireSession(h.log, w, r) - if !ok { - return - } - - id, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - var inp struct { - FromBranchID xid.ID `json:"fromBranchId"` - ToBranchID xid.ID `json:"toBranchId"` - } - - if err = httpserver.DecodeJSON(r, &inp); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // a self-merge would soft-delete the branch's hooks and comments and - // then copy the hooks back from the same, now hook-less, branch — - // permanently destroying both. - if inp.FromBranchID == inp.ToBranchID { - httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "document.branch_self_merge", "cannot merge a branch into itself")) - return - } - - fromDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.FromBranchID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - toDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.ToBranchID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if fromDoc.ID != toDoc.ID { - httpserver.RespondError(h.log, w, errutil.New(http.StatusBadRequest, "document.branch_mismatch", "branches must belong to the same document")) - return - } - - if toDoc.ID != id { - httpserver.RespondError(h.log, w, ErrBranchMismatch) - return - } - - ndoc := toDoc.MergeBranch(fromDoc.Branch, session.UserID) - - var tx Tx - - if err := h.db.BeginTx(r.Context(), &tx); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - - if err := tx.DetachDocumentHooksByBranchID(r.Context(), toDoc.BranchID, session.ActiveOrganizationID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.DeleteDocumentCommentsByBranchID(r.Context(), toDoc.BranchID, session.ActiveOrganizationID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.UpdateDocument(r.Context(), ndoc); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.ReplaceBranchTags(r.Context(), session.ActiveOrganizationID, fromDoc.BranchID, toDoc.BranchID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(toDoc.Search(), ndoc.Search())); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.PromoteBranchApprovals(r.Context(), fromDoc.BranchID, toDoc.BranchID, session.ActiveOrganizationID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.Commit(); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // the hooks are copied after the commit: creating one creates its - // external watcher too, and a rollback cannot take that back. - if err := h.copyHooksToBranch( - r.Context(), - fromDoc.BranchID, - toDoc.BranchID, - toDoc.ID, - session.ActiveOrganizationID, - nil, - ); err != nil { - logutil.Critical(h.log, err).Error( - "cannot copy hooks to the merged branch", - slog.String("branch_id", toDoc.BranchID.String()), - ) - } - - if h.metadata.changeCallback != nil { - h.metadata.changeCallback(session.ActiveOrganizationID, ndoc) - } - - if h.reviewers.changeCallback != nil { - h.reviewers.changeCallback(session.ActiveOrganizationID, toDoc.ID) - } - - httpserver.Respond( - h.log, - w, - ndoc, - http.StatusOK, - ) -} - // DeleteDocument handles the deletion of a document by its ID. func (h *Handler) DeleteDocument(w http.ResponseWriter, r *http.Request) { session, ok := auth.RequireSession(h.log, w, r) @@ -1118,21 +743,14 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { return } - // the hooks are copied after the commit: creating one creates its - // external watcher too, and a rollback cannot take that back. - if err := h.copyHooksToBranch( + h.copyHooksToBranch( r.Context(), doc.BranchID, duplDoc.BranchID, duplDoc.ID, session.ActiveOrganizationID, uids, - ); err != nil { - logutil.Critical(h.log, err).Error( - "cannot copy hooks to the duplicated document", - slog.String("branch_id", duplDoc.BranchID.String()), - ) - } + ) h.copyDocumentFiles(r.Context(), files, id, duplDoc.ID, session.ActiveOrganizationID) @@ -1207,283 +825,6 @@ func (h *Handler) copyDocumentFiles( } } -// FetchDocumentBranches handles the retrieval of all branches for a document. -func (h *Handler) FetchDocumentBranches(w http.ResponseWriter, r *http.Request) { - session, ok := auth.RequireSession(h.log, w, r) - if !ok { - return - } - - docID, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - branches, err := h.db.FetchDocumentBranches(r.Context(), docID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - httpserver.Respond( - h.log, - w, - branches, - http.StatusOK, - ) -} - -// CreateDocumentBranch handles the creation of a new branch forked from an existing source branch. -// Hooks from the source branch are copied to the new branch. -func (h *Handler) CreateDocumentBranch(w http.ResponseWriter, r *http.Request) { - session, ok := auth.RequireSession(h.log, w, r) - if !ok { - return - } - - id, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - var inp struct { - Branch string `json:"branch"` - SourceBranchID xid.ID `json:"sourceBranchId"` - } - - if err = httpserver.DecodeJSON(r, &inp); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if inp.Branch == "" { - httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) - return - } - - sourceDoc, err := h.db.FetchDocumentByBranchID(r.Context(), inp.SourceBranchID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if sourceDoc.ID != id { - httpserver.RespondError(h.log, w, ErrBranchMismatch) - return - } - - newDoc := sourceDoc.Fork(inp.Branch, session.UserID) - - var tx Tx - - if err = h.db.BeginTx(r.Context(), &tx); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - - if err = tx.InsertDocumentBranch(r.Context(), newDoc); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.CopyBranchTags(r.Context(), session.ActiveOrganizationID, sourceDoc.BranchID, newDoc.BranchID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDiff(nil, newDoc.Search())); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.Commit(); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - // the hooks are copied after the commit: creating one creates its - // external watcher too, and a rollback cannot take that back. - if err := h.copyHooksToBranch( - r.Context(), - sourceDoc.BranchID, - newDoc.BranchID, - sourceDoc.ID, - session.ActiveOrganizationID, - nil, - ); err != nil { - logutil.Critical(h.log, err).Error( - "cannot copy hooks to the forked branch", - slog.String("branch_id", newDoc.BranchID.String()), - ) - } - - httpserver.Respond( - h.log, - w, - newDoc, - http.StatusCreated, - ) -} - -// DeleteDocumentBranch handles the deletion of a specific document branch by ID. -// Returns 409 if it is the last remaining branch for the document. -func (h *Handler) DeleteDocumentBranch(w http.ResponseWriter, r *http.Request) { - session, ok := auth.RequireSession(h.log, w, r) - if !ok { - return - } - - id, err := httpserver.ExtractNamedID(r, "documentId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - branchID, err := httpserver.ExtractNamedID(r, "branchId") - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - branchDoc, err := h.db.FetchDocumentByBranchID(r.Context(), branchID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if branchDoc.ID != id { - httpserver.RespondError(h.log, w, ErrBranchMismatch) - return - } - - if branchDoc.Default { - httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.default_branch", "cannot delete the default branch")) - return - } - - count, err := h.db.CountDocumentBranches(r.Context(), branchDoc.ID, session.ActiveOrganizationID) - if err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if count <= 1 { - httpserver.RespondError(h.log, w, errutil.New(http.StatusConflict, "document.last_branch", "cannot delete the last branch")) - return - } - - var tx Tx - - if err := h.db.BeginTx(r.Context(), &tx); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - defer tx.Rollback() //nolint:errcheck // error provides no meaningful info - - if err := tx.DeleteDocumentBranchByID(r.Context(), branchID, session.ActiveOrganizationID); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := h.searchJobs.Enqueue(r.Context(), tx, search.BlocksDifference{ - RemovedBranches: []search.BranchRemoval{{DocumentID: branchDoc.ID, BranchID: branchID}}, - }); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - if err := tx.Commit(); err != nil { - httpserver.RespondError(h.log, w, err) - return - } - - httpserver.Respond( - h.log, - w, - nil, - http.StatusNoContent, - ) -} - -// copyHooksToBranch fetches all hooks from fromBranchID and re-creates them on -// toBranchID with fresh state. This is handler-level business logic; the DB -// layer is not involved in the re-creation decision. -// -// Creating a hook creates its external resource as a side effect, so this runs -// outside the caller's transaction: a rollback cannot take a changedetection.io -// watcher back, and the row that would have pointed at it is gone. A failed -// insert tears the watcher down again for the same reason. -// -// A branch whose content was duplicated carries fresh block uids, so the -// caller passes the old-to-new uid map and a hook anchored to a block is -// re-anchored through it; a hook whose block the map does not name has -// nothing to point at on the target and is dropped. A nil map keeps every -// block id as it is, which is right for a fork or a merge. -func (h *Handler) copyHooksToBranch( - ctx context.Context, - fromBranchID, toBranchID, documentID xid.ID, - organizationID string, - uids map[string]string, -) error { - hooks, err := h.db.FetchDocumentHooksByBranchID(ctx, fromBranchID, organizationID) - if err != nil { - return err - } - - inp := hook.NewInput(organizationID, h.githubMan, h.webchangeClient) - - for _, hk := range hooks { - // a url-watcher cannot get its changedetection.io watcher without - // the integration configured; dropping it from the copy beats - // failing the whole fork or merge over it. - if hk.Type == hook.TypeURLWatcher && !h.webchangeClient.Configured() { - h.log.With("hook_id", hk.ID). - Warn("skipping url-watcher hook copy: changedetection is not configured") - - continue - } - - blockID := hk.BlockID - - if uids != nil && blockID.Valid { - uid, ok := uids[blockID.String] - if !ok { - continue - } - - blockID = null.StringFrom(uid) - } - - newHk, err := hook.NewHook(ctx, hook.CreateInput{ - Type: hk.Type, - BranchID: toBranchID, - BlockID: blockID, - Settings: hk.Settings, - }, documentID, toBranchID, organizationID, inp) - if err != nil { - return err - } - - if err := h.db.InsertDocumentHook(ctx, *newHk); err != nil { - if derr := newHk.Delete(ctx, inp); derr != nil { - logutil.Critical(h.log, derr).Error( - "cannot delete the hook external resource after a failed insert", - slog.String("hook_id", newHk.ID.String()), - ) - } - - return err - } - } - - return nil -} - // upsertBranchReviewer records the reviewer's approval state, inserting the // row when the user is not yet a reviewer of the branch. PreviouslyApproved // is never written here: UpdateBranchReviewer persists currently_approved diff --git a/server/core/internal/server/internal/document/http_test.go b/server/core/internal/server/internal/document/document_http_test.go similarity index 75% rename from server/core/internal/server/internal/document/http_test.go rename to server/core/internal/server/internal/document/document_http_test.go index 052da9bb..bf980d6e 100644 --- a/server/core/internal/server/internal/document/http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "log/slog" "net/http" "net/http/httptest" @@ -13,6 +14,8 @@ import ( "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/webchange" documentCore "github.com/oxynote/oxynote/server/core/internal/document" + "github.com/oxynote/oxynote/server/core/internal/document/file" + hookCore "github.com/oxynote/oxynote/server/core/internal/document/hook" "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" @@ -1390,130 +1393,114 @@ func Test_Handler_CreateDocument(t *testing.T) { } } -func Test_Handler_UpdateDocumentBranch(t *testing.T) { - validBody := `{"name":"Renamed","protected":true}` +func Test_Handler_DeleteDocument(t *testing.T) { + fetchStored := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { + return storedDoc(), nil + } cc := map[string]struct { DB *DBMock Tx *TxMock + BeginErr error NoSession bool OmitDoc bool - Body string - WantName string RespCode int - Updated int - Metadata int - Jobs int + Committed int + TreeCbs int }{ "No session in context": { DB: &DBMock{}, + Tx: &TxMock{}, NoSession: true, - Body: validBody, RespCode: http.StatusUnauthorized, }, "Missing document ID parameter": { DB: &DBMock{}, + Tx: &TxMock{}, OmitDoc: true, - Body: validBody, RespCode: http.StatusNotFound, }, - "Branch document fetch error": { + "Document fetch error": { DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + FetchDocumentFunc: func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { return nil, errors.New("boom") }, }, - Body: validBody, + Tx: &TxMock{}, RespCode: http.StatusInternalServerError, }, - "Branch of another document": { + "Hook fetch error": { DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - doc := branchDoc(_branchID) - doc.ID = xid.New() - - return doc, nil + FetchDocumentFunc: fetchStored, + FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") }, }, - Body: validBody, - RespCode: http.StatusNotFound, - }, - "Invalid JSON body": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return storedDoc(), nil - }, - }, - Body: "{", - RespCode: http.StatusBadRequest, - }, - "Metadata update error": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return branchDoc(_branchID), nil - }, - }, - Tx: &TxMock{ - UpdateDocumentBranchMetadataFunc: func(context.Context, documentCore.Document) error { - return errors.New("boom") - }, - }, - Body: validBody, + Tx: &TxMock{}, RespCode: http.StatusInternalServerError, - Updated: 1, }, - "Search job insert error": { + "Hook cleanup error": { DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return branchDoc(_branchID), nil + FetchDocumentFunc: fetchStored, + FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook("bogus")}, nil }, }, + Tx: &TxMock{}, + RespCode: http.StatusInternalServerError, + }, + "Transaction start error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{}, + BeginErr: errors.New("boom"), + RespCode: http.StatusInternalServerError, + }, + "Document deletion error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, Tx: &TxMock{ + DeleteDocumentFunc: func(context.Context, xid.ID, string) ([]xid.ID, error) { + return nil, errors.New("boom") + }, + }, + RespCode: http.StatusInternalServerError, + }, + "Search removal enqueue error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{ + DeleteDocumentFunc: func(context.Context, xid.ID, string) ([]xid.ID, error) { + return []xid.ID{_documentID}, nil + }, InsertDocumentSearchJobFunc: func(context.Context, search.BlocksDifference) error { return errors.New("boom") }, }, - Body: validBody, RespCode: http.StatusInternalServerError, - Updated: 1, - Jobs: 1, }, - // the main branch is found by its flag in the tree and content - // queries, but its name is what the user sees; renaming it would - // leave the document looking nameless. - "Renaming the default branch is rejected": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return storedDoc(), nil + "Commit error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{ + CommitFunc: func() error { + return errors.New("boom") }, }, - Body: validBody, - RespCode: http.StatusBadRequest, + RespCode: http.StatusInternalServerError, + Committed: 1, }, - "Protecting the default branch is allowed": { + "Successful deletion with hook cleanup": { DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return storedDoc(), nil + FetchDocumentFunc: fetchStored, + FetchDocumentHooksByDocumentIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil }, }, - Body: `{"protected":true}`, - WantName: documentCore.DefaultBranch, - RespCode: http.StatusOK, - Updated: 1, - Metadata: 1, - }, - "Successful update": { - DB: &DBMock{ - FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { - return branchDoc(_branchID), nil + Tx: &TxMock{ + DeleteDocumentFunc: func(_ context.Context, id xid.ID, _ string) ([]xid.ID, error) { + return []xid.ID{id}, nil }, }, - Body: validBody, - WantName: "Renamed", - RespCode: http.StatusOK, - Updated: 1, - Metadata: 1, - Jobs: 1, + RespCode: http.StatusOK, + Committed: 1, + TreeCbs: 1, }, } @@ -1521,38 +1508,373 @@ func Test_Handler_UpdateDocumentBranch(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - tx := c.Tx - if tx == nil { - tx = &TxMock{} - } - - hdl, cnt := newTestHandler(withTx(c.DB, tx, nil), &fakePublisher{}) + hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) rec := httptest.NewRecorder() - hdl.UpdateDocumentBranch(rec, newRequest(http.MethodPut, c.Body, c.NoSession, c.OmitDoc, false)) + hdl.DeleteDocument(rec, newRequest(http.MethodDelete, "", c.NoSession, c.OmitDoc, true)) assert.Equal(t, c.RespCode, rec.Code) - assert.Len(t, tx.UpdateDocumentBranchMetadataCalls(), c.Updated) - assert.Len(t, tx.InsertDocumentSearchJobCalls(), c.Jobs) - assert.Equal(t, c.Metadata, cnt.metadata) + assert.Len(t, c.Tx.CommitCalls(), c.Committed) + assert.Equal(t, c.TreeCbs, cnt.tree) if c.RespCode == http.StatusOK { - ff := tx.UpdateDocumentBranchMetadataCalls() - require.NotEmpty(t, ff) - assert.Equal(t, c.WantName, ff[0].Doc.BranchName) - assert.True(t, ff[0].Doc.Protected) - assert.Len(t, tx.CommitCalls(), 1) - } + require.Len(t, c.Tx.DeleteDocumentCalls(), 1) + assert.Equal(t, _documentID, c.Tx.DeleteDocumentCalls()[0].ID) - // a rename rewrites every entry of the branch, since each one - // carries the branch name. - if c.Jobs == 1 && c.RespCode == http.StatusOK { - diff := tx.InsertDocumentSearchJobCalls()[0].Diff - require.Len(t, diff.Updated, 1) - assert.Equal(t, _branchID.String()+"-docname", diff.Updated[0].ID) - assert.Equal(t, "Renamed", diff.Updated[0].BranchName) + // the removal of the returned subtree ids rides the same + // transaction as the delete. + ff := c.Tx.InsertDocumentSearchJobCalls() + require.Len(t, ff, 1) + assert.Equal(t, []xid.ID{_documentID}, ff[0].Diff.RemovedDocuments) } }) } } + +func Test_Handler_DuplicateDocument(t *testing.T) { + fetchStored := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { + return storedDoc(), nil + } + + // insertAwareTx returns the freshly inserted duplicate in the parent + // subtree so the sort-index swap can find it. + insertAwareTx := func() *TxMock { + tx := &TxMock{} + tx.FetchDocumentTreeByDocumentParentIDFunc = func(context.Context, null.Value[xid.ID], string) (documentCore.Summaries, error) { + return documentCore.Summaries{{ID: tx.InsertDocumentCalls()[0].Doc.ID}}, nil + } + + return tx + } + + // fetchStoredWithImage serves a document whose content holds an image + // served by that same document, which is what duplication has to copy. + fetchStoredWithImage := func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { + doc := storedDoc() + doc.Content = documentCore.RootBlock{ + Type: documentCore.BlockNodeDoc, + Content: []documentCore.Block{ + { + Type: documentCore.BlockNodeImageBlock, + Attrs: documentCore.Attributes{ + "uid": "img1", + "src": "https://app.test/core" + fmt.Sprintf(documentCore.FilePathFormat, _documentID, "img1"), + }, + }, + }, + } + + return doc, nil + } + + // imageDB serves the document with the image plus the file row that + // image refers to, which is what the copy is made from. + imageDB := func() *DBMock { + return &DBMock{ + FetchDocumentFunc: fetchStoredWithImage, + FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { + f := file.NewFile(id, file.LocationDocument, "key", _documentID, organizationID) + + return &f, nil + }, + } + } + + cc := map[string]struct { + DB *DBMock + Tx *TxMock + Storer *StorerMock + BeginErr error + NoSession bool + OmitDoc bool + RespCode int + Committed int + TreeCbs int + Copies int + CopiedHooks int + }{ + "No session in context": { + DB: &DBMock{}, + Tx: &TxMock{}, + NoSession: true, + RespCode: http.StatusUnauthorized, + }, + "Missing document ID parameter": { + DB: &DBMock{}, + Tx: &TxMock{}, + OmitDoc: true, + RespCode: http.StatusNotFound, + }, + "Document fetch error": { + DB: &DBMock{ + FetchDocumentFunc: func(context.Context, xid.ID, string, string) (*documentCore.Document, error) { + return nil, errors.New("boom") + }, + }, + Tx: &TxMock{}, + RespCode: http.StatusInternalServerError, + }, + "Transaction start error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{}, + BeginErr: errors.New("boom"), + RespCode: http.StatusInternalServerError, + }, + "Duplicate insertion error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{ + InsertDocumentFunc: func(context.Context, documentCore.Document) error { + return errors.New("boom") + }, + }, + RespCode: http.StatusInternalServerError, + }, + "Tag copy error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{ + CopyBranchTagsFunc: func(context.Context, string, xid.ID, xid.ID) error { + return errors.New("boom") + }, + }, + RespCode: http.StatusInternalServerError, + }, + "Sort index swap error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: &TxMock{}, + RespCode: http.StatusBadRequest, + }, + "Commit error": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: func() *TxMock { + tx := insertAwareTx() + tx.CommitFunc = func() error { + return errors.New("boom") + } + + return tx + }(), + RespCode: http.StatusInternalServerError, + Committed: 1, + }, + "Successful duplication": { + DB: &DBMock{FetchDocumentFunc: fetchStored}, + Tx: insertAwareTx(), + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + }, + "Files are copied along with the document": { + DB: imageDB(), + Tx: insertAwareTx(), + Storer: &StorerMock{}, + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + Copies: 1, + }, + "Failing file copy still yields the duplicate": { + DB: imageDB(), + Tx: insertAwareTx(), + Storer: &StorerMock{ + CopyFunc: func(context.Context, string, string, string, string) error { + return errors.New("boom") + }, + }, + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + Copies: 1, + }, + "Hooks are copied along with the document": { + DB: func() *DBMock { + blockHook := storedHook(hookCore.TypeScheduledReminder) + blockHook.BlockID = null.StringFrom("img1") + + db := imageDB() + db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{blockHook}, nil + } + + return db + }(), + Tx: insertAwareTx(), + Storer: &StorerMock{}, + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + Copies: 1, + CopiedHooks: 1, + }, + "Failing hook copy still yields the duplicate": { + DB: func() *DBMock { + db := &DBMock{FetchDocumentFunc: fetchStored} + db.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return nil, errors.New("boom") + } + + return db + }(), + Tx: insertAwareTx(), + RespCode: http.StatusCreated, + Committed: 1, + TreeCbs: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + hdl, cnt := newTestHandler(withTx(c.DB, c.Tx, c.BeginErr), &fakePublisher{}) + + if c.Storer != nil { + hdl.storer = c.Storer + } + + rec := httptest.NewRecorder() + + hdl.DuplicateDocument(rec, newRequest(http.MethodPost, "", c.NoSession, c.OmitDoc, true)) + + assert.Equal(t, c.RespCode, rec.Code) + assert.Len(t, c.Tx.CommitCalls(), c.Committed) + assert.Equal(t, c.TreeCbs, cnt.tree) + + if c.Storer != nil { + assert.Len(t, c.Storer.CopyCalls(), c.Copies) + assert.Len(t, c.DB.InsertDocumentFileCalls(), c.Copies) + } + + if c.RespCode == http.StatusCreated { + // the duplicate is a new document owned by the caller. + require.Len(t, c.Tx.InsertDocumentCalls(), 1) + dupl := c.Tx.InsertDocumentCalls()[0].Doc + assert.NotEqual(t, _documentID, dupl.ID) + assert.Equal(t, []string{"u1"}, c.Tx.UpsertDocumentMaintainersCalls()[0].MaintainerIDs) + + // the copy's default branch takes the source's tags before + // the commit. + require.Len(t, c.Tx.CopyBranchTagsCalls(), 1) + assert.Equal(t, _branchID, c.Tx.CopyBranchTagsCalls()[0].FromBranchID) + assert.Equal(t, dupl.BranchID, c.Tx.CopyBranchTagsCalls()[0].ToBranchID) + + // the hooks are copied once the commit is through, since + // creating one creates its watcher. + assert.Empty(t, c.Tx.InsertDocumentHookCalls()) + require.Len(t, c.DB.InsertDocumentHookCalls(), c.CopiedHooks) + + // a hook anchored to a block follows the block's regenerated + // uid rather than pointing at the source's. + for _, call := range c.DB.InsertDocumentHookCalls() { + assert.Equal(t, null.ValueFrom(dupl.BranchID), call.Hk.BranchID) + assert.NotEqual(t, null.StringFrom("img1"), call.Hk.BlockID) + assert.True(t, call.Hk.BlockID.Valid) + } + } + }) + } +} + +func Test_Handler_copyDocumentFiles(t *testing.T) { + toDocumentID := xid.New() + + fileDB := func() *DBMock { + return &DBMock{ + FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { + f := file.NewFile(id, file.LocationComment, "key", _documentID, organizationID) + + return &f, nil + }, + } + } + + cc := map[string]struct { + DB *DBMock + Storer *StorerMock + Files map[string]string + Inserts int + Copies int + }{ + "Nothing to copy": { + DB: fileDB(), + Storer: &StorerMock{}, + Files: map[string]string{}, + }, + "File row is missing": { + DB: &DBMock{ + FetchDocumentFileFunc: func(context.Context, string, string) (*file.File, error) { + return nil, errors.New("boom") + }, + }, + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + }, + "Error returned by db.InsertDocumentFile": { + DB: func() *DBMock { + db := fileDB() + db.InsertDocumentFileFunc = func(context.Context, file.File) error { + return errors.New("boom") + } + + return db + }(), + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + }, + "Error returned by storer.Copy": { + DB: fileDB(), + Storer: &StorerMock{ + CopyFunc: func(context.Context, string, string, string, string) error { + return errors.New("boom") + }, + }, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + Copies: 1, + }, + "Successful copy": { + DB: fileDB(), + Storer: &StorerMock{}, + Files: map[string]string{"old-1": "new-1"}, + Inserts: 1, + Copies: 1, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + hdl, _ := newTestHandler(c.DB, &fakePublisher{}) + hdl.storer = c.Storer + + hdl.copyDocumentFiles(context.Background(), c.Files, _documentID, toDocumentID, "org1") + + ff := c.DB.InsertDocumentFileCalls() + require.Len(t, ff, c.Inserts) + + cf := c.Storer.CopyCalls() + require.Len(t, cf, c.Copies) + + if c.Inserts == 0 { + return + } + + // the copy owns its row, keyed under the new document. + assert.Equal(t, "new-1", ff[0].F.ID) + assert.Equal(t, file.LocationComment, ff[0].F.Location) + assert.Equal(t, file.Key("org1", toDocumentID, "new-1"), ff[0].F.StorageKey) + assert.Equal(t, toDocumentID, ff[0].F.DocumentID.V) + + if c.Copies == 0 { + return + } + + assert.Equal(t, file.Folder("org1", _documentID), cf[0].SrcFolder) + assert.Equal(t, "old-1", cf[0].SrcID) + assert.Equal(t, file.Folder("org1", toDocumentID), cf[0].DstFolder) + assert.Equal(t, "new-1", cf[0].DstID) + }) + } +} diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index 96b664f4..727a17eb 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -146,16 +146,19 @@ func (s *Server) internalRouter() chi.Router { }) // the branch operations that read or rewrite a branch's stored - // content. They are internal so that only auth-realtime, which - // first stores what the editors still hold, can start one — yet - // they act for a person, so the session it forwards is required - // and authorizes the change exactly as on the public surface. + // content, or remove a branch editors may still be connected to. + // They are internal so that only auth-realtime, which first stores + // what the editors still hold and afterwards drops the connections + // a change invalidated, can start one — yet they act for a person, + // so the session it forwards is required and authorizes the change + // exactly as on the public surface. sr.Group(func(ssr chi.Router) { ssr.Use(auth.Middleware(s.log, s.opts.Auth, s.client)) ssr.Use(s.handlers.document.RequireDocumentAccess) ssr.Put("/merge", s.handlers.document.MergeBranches) ssr.Post("/branches", s.handlers.document.CreateDocumentBranch) ssr.Put("/branches/{branchId}", s.handlers.document.UpdateDocumentBranch) + ssr.Delete("/branches/{branchId}", s.handlers.document.DeleteDocumentBranch) }) }) @@ -299,7 +302,6 @@ func (s *Server) router() chi.Router { ssr.Route("/branches", func(sssr chi.Router) { sssr.Get("/", s.handlers.document.FetchDocumentBranches) sssr.Route("/{branchId}", func(ssssr chi.Router) { - ssssr.Delete("/", s.handlers.document.DeleteDocumentBranch) ssssr.Put("/review-approve", s.handlers.document.UpdateBranchReviewApproval) ssssr.Post("/blocks/{blockUid}/run", s.handlers.block.RunBlock) ssssr.Route("/reviewers", func(sssssr chi.Router) { diff --git a/server/core/internal/server/router_test.go b/server/core/internal/server/router_test.go index 90a28160..39ec2e24 100644 --- a/server/core/internal/server/router_test.go +++ b/server/core/internal/server/router_test.go @@ -85,11 +85,13 @@ func Test_Server_httpRouter(t *testing.T) { // the branch operations are reachable only through auth-realtime, // which stores the editors' pending changes before core reads the - // branch: a public mount would let a client skip that. + // branch and drops the connections a delete leaves behind: a public + // mount would let a client skip that. internalOnly := map[string]string{ - "PUT /api/x/documents/{documentId}/merge": "PUT /api/*/documents/{documentId}/merge", - "POST /api/x/documents/{documentId}/branches": "POST /api/*/documents/{documentId}/branches/", - "PUT /api/x/documents/{documentId}/branches/{branchId}": "PUT /api/*/documents/{documentId}/branches/{branchId}/", + "PUT /api/x/documents/{documentId}/merge": "PUT /api/*/documents/{documentId}/merge", + "POST /api/x/documents/{documentId}/branches": "POST /api/*/documents/{documentId}/branches/", + "PUT /api/x/documents/{documentId}/branches/{branchId}": "PUT /api/*/documents/{documentId}/branches/{branchId}/", + "DELETE /api/x/documents/{documentId}/branches/{branchId}": "DELETE /api/*/documents/{documentId}/branches/{branchId}/", } for internal, public := range internalOnly { @@ -97,7 +99,6 @@ func Test_Server_httpRouter(t *testing.T) { assert.False(t, routes[public], "%s must not be publicly routable", public) } - // the branch reads and the delete stay public alongside them. + // the branch reads stay public alongside them. assert.True(t, routes["GET /api/*/documents/{documentId}/branches/"]) - assert.True(t, routes["DELETE /api/*/documents/{documentId}/branches/{branchId}/"]) } diff --git a/web/app/components/editor/DocumentContainer.client.nuxt.test.ts b/web/app/components/editor/DocumentContainer.client.nuxt.test.ts index 963a6f77..bfc08e9f 100644 --- a/web/app/components/editor/DocumentContainer.client.nuxt.test.ts +++ b/web/app/components/editor/DocumentContainer.client.nuxt.test.ts @@ -35,6 +35,14 @@ const providers = vi.hoisted(() => { } }) +const { showToastMessageMock } = vi.hoisted(() => { + return { showToastMessageMock: vi.fn() } +}) + +vi.mock("~/components/toast", () => { + return { showToastMessage: showToastMessageMock } +}) + vi.mock("@hocuspocus/provider", () => { function FakeProvider(options: ProviderOptions): ProviderEntry { const entry: ProviderEntry = { @@ -243,10 +251,13 @@ describe("", { concurrent: false }, () => { expect(redirectToLogin).toHaveBeenCalledTimes(1) }) - it("reports an error the realtime service sends", async ({ expect }) => { + it("tells the reader about an error the realtime service sends", async ({ + expect, + }) => { const consoleError = vi.spyOn(console, "error").mockImplementation(() => { return undefined }) + showToastMessageMock.mockClear() await mountContainer() providerFor(BRANCH_ID).options.onStateless({ @@ -254,6 +265,8 @@ describe("", { concurrent: false }, () => { }) expect(consoleError).toHaveBeenCalledTimes(1) + expect(showToastMessageMock).toHaveBeenCalledTimes(1) + expect(showToastMessageMock.mock.calls[0]?.[0]).toBe("error") }) it("stays quiet for any other realtime message", async ({ expect }) => { @@ -262,11 +275,14 @@ describe("", { concurrent: false }, () => { }) await mountContainer() + showToastMessageMock.mockClear() + providerFor(BRANCH_ID).options.onStateless({ payload: JSON.stringify({ type: "info" }), }) expect(consoleError).toHaveBeenCalledTimes(0) + expect(showToastMessageMock).toHaveBeenCalledTimes(0) }) it("keeps a protected branch read-only", async ({ expect }) => { diff --git a/web/app/components/editor/DocumentContainer.client.vue b/web/app/components/editor/DocumentContainer.client.vue index 59d215c6..e05f54c7 100644 --- a/web/app/components/editor/DocumentContainer.client.vue +++ b/web/app/components/editor/DocumentContainer.client.vue @@ -6,6 +6,7 @@ import ContentEditor from "./ContentEditor.vue" import NameEditor from "./NameEditor.vue" import DiffEditor from "./diff/DiffEditor.vue" import { redirectToLogin } from "~/plugins/03.api-fetch" +import { showToastMessage } from "~/components/toast" import { refreshGapDecorationsInBackground } from "./drag-handle/gap-decorations" import { editorCaretColors } from "~/assets/css" @@ -223,8 +224,12 @@ function createBranchProvider( onStateless({ payload }) { const data = JSON.parse(payload) as { type?: string } if (data.type === "error") { - // TODO show toast console.error(`Error in provider ${streamName}:`, data) + showToastMessage( + "error", + t("editor.realtime-error.title"), + t("editor.realtime-error.description"), + ) } }, onConnect: () => { diff --git a/web/app/composables/api/useDocumentAPI.ts b/web/app/composables/api/useDocumentAPI.ts index 0d0107fd..3f3ee69f 100644 --- a/web/app/composables/api/useDocumentAPI.ts +++ b/web/app/composables/api/useDocumentAPI.ts @@ -775,9 +775,15 @@ export default function () { return } - await $coreAPIClient(`/api/documents/${docId}/branches/${branchId}`, { - method: "DELETE", - }) + // auth-realtime drops the connections still on the branch once + // core has removed it, so nobody keeps editing a branch that is + // gone. + await $authRealtimeAPIClient( + `/api/documents/${docId}/branches/${branchId}`, + { + method: "DELETE", + }, + ) }, async onSuccess(_data, { docId, branchId }) { if (!isXid(docId) || !isXid(branchId)) { diff --git a/web/i18n/locales/en/editor.json b/web/i18n/locales/en/editor.json index ea7e805e..0fc972cd 100644 --- a/web/i18n/locales/en/editor.json +++ b/web/i18n/locales/en/editor.json @@ -1,6 +1,10 @@ { "editor": { "unknown-user-caret": "Unknown", + "realtime-error": { + "title": "Your latest changes could not be saved", + "description": "Reload the page to keep editing" + }, "placeholders": { "name-default": "New Page", "content": {