diff --git a/docker/Caddyfile b/docker/Caddyfile index 5b6d560b..04f943cd 100644 --- a/docker/Caddyfile +++ b/docker/Caddyfile @@ -13,7 +13,7 @@ (common) { request_body { - max_size 10MB # must be higher than file upload limit + max_size 30MB # must be higher than file upload limit } header -Server diff --git a/docker/prod/Caddyfile b/docker/prod/Caddyfile index dab7f232..a553e165 100644 --- a/docker/prod/Caddyfile +++ b/docker/prod/Caddyfile @@ -14,7 +14,7 @@ (common) { request_body { - max_size 10MB # must be higher than file upload limit + max_size 30MB # must be higher than file upload limit } header -Server diff --git a/e2e/tests/files.test.ts b/e2e/tests/files.test.ts new file mode 100644 index 00000000..7c6269e8 --- /dev/null +++ b/e2e/tests/files.test.ts @@ -0,0 +1,125 @@ +import { readFile } from "node:fs/promises" +import { expect, test, type Locator, type Page } from "@playwright/test" +import { + contentEditor, + createDocument, + documentPersisted, + openSlashMenu, + waitForEditor, +} from "../helpers/editor" +import { t } from "../helpers/i18n" +import { visit } from "../helpers/page" +import { signUpWithWorkspace } from "../helpers/workspace" + +// a zip's magic bytes followed by padding to exactly 2 KB, so the server +// detects an archive and the card shows a round size +const ZIP_BYTES = Buffer.concat([ + Buffer.from([0x50, 0x4b, 0x03, 0x04]), + Buffer.alloc(2044, 0x41), +]) + +const TEXT = "hello from the attachment" + +// insertFileBlock adds a file block from the slash menu and answers the +// picker it opens with the given file +async function insertFileBlock( + page: Page, + file: { name: string; mimeType: string; buffer: Buffer }, +): Promise { + await contentEditor(page).click() + const menu = await openSlashMenu(page) + await page.keyboard.type("File") + + // typing narrows the menu down to the one matching command + await expect(menu.getByRole("button")).toHaveCount(1) + await expect(menu.getByRole("button")).toContainText("File") + await page.keyboard.press("Enter") + + const chooser = page.waitForEvent("filechooser") + await contentEditor(page).getByText(t("editor.file.description")).click() + await (await chooser).setFiles(file) +} + +// fileCard is the uploaded attachment's card, which is what the reader +// clicks +function fileCard(page: Page): Locator { + return contentEditor(page).locator('[data-type="fileBlock"] a') +} + +test.describe("file attachments", () => { + test("uploads a file, keeps it across a reload and downloads it under its name", async ({ + page, + request, + }) => { + await signUpWithWorkspace(page, request) + await createDocument(page) + const url = page.url() + + await insertFileBlock(page, { + name: "notes.zip", + mimeType: "application/zip", + buffer: ZIP_BYTES, + }) + + await expect(fileCard(page)).toContainText("notes.zip") + await expect(fileCard(page)).toContainText("2.0 KB") + await documentPersisted(page) + + await visit(page, url) + + await waitForEditor(page) + await expect(fileCard(page)).toContainText("notes.zip") + await expect(fileCard(page)).toContainText("2.0 KB") + + // an archive is not something a browser shows, so the click is a + // download under the original name carrying the original bytes + const download = page.waitForEvent("download") + await fileCard(page).click() + const downloaded = await download + + expect(downloaded.suggestedFilename()).toBe("notes.zip") + const path = await downloaded.path() + expect(await readFile(path)).toEqual(ZIP_BYTES) + + // the server decides the download by its disposition, which + // carries the recorded name, and forbids sniffing the type away + const href = await fileCard(page).getAttribute("href") + const response = await page.request.get(href ?? "") + expect(response.headers()["content-type"]).toBe("application/zip") + expect(response.headers()["content-disposition"]).toBe( + "attachment; filename=notes.zip", + ) + expect(response.headers()["x-content-type-options"]).toBe("nosniff") + }) + + test("opens a browser-viewable file in a new tab", async ({ + page, + request, + }) => { + await signUpWithWorkspace(page, request) + await createDocument(page) + + await insertFileBlock(page, { + name: "readme.txt", + mimeType: "text/plain", + buffer: Buffer.from(TEXT), + }) + await expect(fileCard(page)).toContainText("readme.txt") + + const popup = page.waitForEvent("popup") + await fileCard(page).click() + const opened = await popup + + await expect(opened).toHaveURL(/\/api\/documents\/[a-z0-9]{20}\/files\//) + await expect(opened.locator("body")).toContainText(TEXT) + + // plain text is on the server's inline allowlist, so the tab + // renders it rather than downloading it + const response = await page.request.get(opened.url()) + expect(response.headers()["content-type"]).toBe("text/plain; charset=utf-8") + expect(response.headers()["content-disposition"]).toBe( + "inline; filename=readme.txt", + ) + expect(response.headers()["x-content-type-options"]).toBe("nosniff") + }) +}) diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 230f6ed1..61634fac 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -93,14 +93,14 @@ Every organization is seeded at initialization with a Prometheus data source nam ## External resource lifecycle (files, hooks) -Uploaded images live under `organizations/{org}/documents/{doc}/files/{blockUID}` — the file id **is** the image block's `uid` — and are tracked in `document_files`. That key is an object key in the S3-compatible store, or a path under the storage directory on a deployment running without one; `internal/storage` holds what the two backends share and `internal/storage/{s3,fs}` implement them. Hooks hold external resources too (changedetection.io watchers, GitHub tracking). Both are reclaimed by sweeps, not by request handlers: +Uploaded images and file attachments live under `organizations/{org}/documents/{doc}/files/{blockUID}` — the file id **is** the image or file block's `uid`, so a re-upload into the same block overwrites its object in place — and are tracked in `document_files`, which also records the original name, size and the content type detected from the bytes; retrieval (`GET /api/documents/{id}/files/{fileId}-{fileName}`: every uid is a 21-character nanoid, so the id is cut off the front and the name, which the frontend appends so the address reads as the file, is ignored) resolves the object by the row's key and serves the type from the row with a `Content-Disposition` that is `inline` only for a fixed browser-viewable allowlist (`file.Viewable`). That key is an object key in the S3-compatible store, or a path under the storage directory on a deployment running without one; `internal/storage` holds what the two backends share and `internal/storage/{s3,fs}` implement them. Hooks hold external resources too (changedetection.io watchers, GitHub tracking). Both are reclaimed by sweeps, not by request handlers: - **The row outlives its owner.** `document_files` and `document_hooks` reference documents, branches and organizations with `ON DELETE SET NULL`, so a deleted document — or an org deleted by Better Auth — leaves rows behind rather than destroying the only record of the external resource. `document_files.storage_key` stores the object key outright so it stays reachable once the FKs are gone. - **`internal/document/file/manager`** ticks every 5 minutes: it trims expired history entries, then reclaims files. A file is referenced if its id appears in any branch content of its document, any retained `document_branch_history_entries` row, or any comment/reply — checked in SQL (`CheckDocumentFileReferenced`), so a spurious match only ever retains. Unreferenced files are stamped `unreferenced_at` and deleted a day later; rows with NULL FKs skip the wait; files younger than a day are never touched, which covers the gap between an upload and the first content persist that mentions it. - **`internal/document/hook/manager`** does the same for hooks: a row whose branch, document or organization went NULL gets `Hook.Delete` (external teardown) before the row is dropped. A merge replaces the target's hooks by nulling their branch and document (`DetachDocumentHooksByBranchID`) rather than soft-deleting them: the soft-delete mark means "block gone" and the sweep lifts it while the block is still in the branch, which after a merge it is. - **Organization deletion is announced, not discovered.** Better Auth owns the organizations table, so core cannot notice a deletion on its own. `beforeDeleteOrganization` (`auth-realtime/src/auth.ts`) calls `POST /api/x/organizations/{id}/teardown` while every row still exists: core tears down the org's hooks, queues the search removal, drops the `slack_apps` and `github_installations` rows, and deletes the logo object. The hook throws if core fails, so the organization survives instead of leaving orphans nothing can address. Document files still need no action — the cascade nulls their keys and the file manager reclaims them. - **Ordering is inverted between create and delete.** Creating writes the row before the object (upload, duplication); deleting removes the object before the row. Hooks follow the same rule from outside the transaction: `copyHooksToBranch` runs after the fork, merge or duplicate commits, because `hook.NewHook` creates the watcher as a side effect and a rollback cannot take that back; a failed insert tears the just-created watcher down again. Both leave at most a row without an object, which the sweep resolves — an object without a row is invisible to every cleanup path. -- **Duplication copies, never shares.** `Document.Duplicate` regenerates every block uid, rewrites image `src` paths to the new document (swapping only the path, never the host), and returns two old→new maps: the file ids the handler uses to copy each object server-side into the duplicate's own folder, and the block uids through which the source branch's hooks are re-anchored on the copy (a hook whose block the map does not name is dropped). Files are therefore owned by exactly one document. +- **Duplication copies, never shares.** `Document.Duplicate` regenerates every block uid, rewrites image and file block `src` paths to the new document (swapping only the path, never the host), and returns two old→new maps: the file ids the handler uses to copy each object server-side into the duplicate's own folder, and the block uids through which the source branch's hooks are re-anchored on the copy (a hook whose block the map does not name is dropped). Files are therefore owned by exactly one document. - **Search jobs apply in order, per document.** A diff is a delta against the state its predecessors left, so the manager holds back every later diff of a document whose earlier one failed, rather than replaying it on top of newer work; an organization removal is global and holds everything after it. Without this a failed add replayed after a delete re-inserts the blocks of a document that no longer exists, and no later job would ever remove them. - **Search index entries are branch-scoped.** Every branch of every document is indexed; an entry's id is `-` (`-docname` for the name entry), since a fork copies its source's uids, and it carries `documentId`, `branchId`, `branchName` and `branchDefault`. Every path that changes a branch queues its diff through `search.Jobs` in the same transaction: a persist on any branch, a fork (a full add of the new branch), a rename (every entry of the branch changes, since each carries the name), a merge (the target's diff). A branch deletion queues a `RemovedBranches` removal the manager applies by `branchId` filter, and a document deletion clears every branch at once: `agent.DeleteDocument` collects the subtree ids before the row goes away and returns them, so the manager clears the index by `documentId` filter — the descendants the cascade destroys have no other trace to clean up by. Index settings are declared in one object in `search.Client` and sent only when they differ from what the index reports, since a searchable or filterable attribute change re-indexes everything; every write task is awaited, so a batch Meilisearch rejects fails its job and is retried in order. - History entries pin the files they reference, so `DB_MAX_DOCUMENT_HISTORY_ENTRIES` (count, trimmed on insert) and `DB_DOCUMENT_HISTORY_RETENTION` (age, trimmed in the sweep) also decide how long a removed image survives. diff --git a/server/auth-realtime/src/schema/file.ts b/server/auth-realtime/src/schema/file.ts new file mode 100644 index 00000000..3a69de5d --- /dev/null +++ b/server/auth-realtime/src/schema/file.ts @@ -0,0 +1,78 @@ +import { mergeAttributes, Node } from "@tiptap/core" + +export const FileBlock = Node.create({ + name: "fileBlock", + group: "block", + atom: true, + draggable: false, + selectable: false, + addAttributes() { + return { + src: { + default: null, + parseHTML: (element) => + element.getAttribute("href"), + renderHTML: (attrs) => { + if (!attrs.src) return {} + return { href: attrs.src as string } + }, + }, + name: { + default: null, + parseHTML: (element) => + element.getAttribute("data-name"), + renderHTML: (attrs) => { + if (!attrs.name) return {} + return { + "data-name": + attrs.name as string, + } + }, + }, + size: { + default: null, + parseHTML: (element) => { + const size = + element.getAttribute( + "data-size", + ) + return size + ? Number.parseInt(size, 10) + : null + }, + renderHTML: (attrs) => { + if (!attrs.size) return {} + return { + "data-size": + attrs.size as number, + } + }, + }, + contentType: { + default: null, + parseHTML: (element) => + element.getAttribute( + "data-content-type", + ), + renderHTML: (attrs) => { + if (!attrs.contentType) return {} + return { + "data-content-type": + attrs.contentType as string, + } + }, + }, + } + }, + parseHTML() { + return [{ tag: `a[data-type="file-block"]` }] + }, + renderHTML({ HTMLAttributes }) { + return [ + "a", + mergeAttributes(HTMLAttributes, { + "data-type": "file-block", + }), + ] + }, +}) diff --git a/server/auth-realtime/src/schema/index.test.ts b/server/auth-realtime/src/schema/index.test.ts index c8294a87..d3112569 100644 --- a/server/auth-realtime/src/schema/index.test.ts +++ b/server/auth-realtime/src/schema/index.test.ts @@ -20,6 +20,7 @@ import { } from "./index.js" import { FigmaBlock } from "./figma.js" import { ImageBlock } from "./image.js" +import { FileBlock } from "./file.js" import { MermaidBlock } from "./mermaid.js" import { MetricBlock, MetricGrid } from "./metric.js" import { transformer } from "../ydocument.js" @@ -131,6 +132,7 @@ describe("getEditorExtensions", () => { { name: "metric", input: "metricBlock" }, { name: "metric grid", input: "metricGrid" }, { name: "image", input: "imageBlock" }, + { name: "file", input: "fileBlock" }, { name: "figma", input: "figmaBlock" }, { name: "mermaid", input: "mermaidBlock" }, { name: "titled code", input: "titledCodeBlock" }, @@ -183,6 +185,19 @@ describe("editor schema", () => { }, }, }, + { + name: "a file attachment", + input: { + type: "fileBlock", + attrs: { + uid: "f1", + src: "https://example/x.zip", + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }, + }, + }, { name: "a figma embed", input: { @@ -656,6 +671,7 @@ describe("schema html contract", () => { it.for([ { name: "image", input: ImageBlock, expected: "img" }, + { name: "file", input: FileBlock, expected: "a" }, { name: "mermaid", input: MermaidBlock, expected: "pre" }, { name: "code", input: CodeBlock, expected: "pre" }, { name: "callout", input: CalloutBlock, expected: "div" }, @@ -697,6 +713,103 @@ describe("schema html contract", () => { }) }) + describe("file attributes", () => { + it("tags the block as file-block and parses it back", ({ + expect, + }) => { + expect(render(FileBlock)[1]["data-type"]).toBe( + "file-block", + ) + expect(parseSelector(FileBlock)).toBe( + `a[data-type="file-block"]`, + ) + }) + + // the size arrives as a string on the element and is stored as a + // number, so a document round-trips as a number + it("parses the size into a number", ({ expect }) => { + const attr = attributesOf(FileBlock).size + + expect( + attr?.parseHTML?.({ + getAttribute: () => "2048", + }), + ).toBe(2048) + }) + + it.for([ + { name: "src", input: "src" }, + { name: "name", input: "name" }, + { name: "size", input: "size" }, + { name: "content type", input: "contentType" }, + ])( + "parses a missing $name as null", + ({ input }, { expect }) => { + const attr = attributesOf(FileBlock)[input] + + expect( + attr?.parseHTML?.({ + getAttribute: () => null, + }), + ).toBeNull() + }, + ) + + it.for([ + { + name: "src", + input: "src", + value: "https://example/x.zip", + expected: "href", + }, + { + name: "name", + input: "name", + value: "notes.zip", + expected: "data-name", + }, + { + name: "size", + input: "size", + value: 2048, + expected: "data-size", + }, + { + name: "content type", + input: "contentType", + value: "application/zip", + expected: "data-content-type", + }, + ])( + "writes $name back to $expected", + ({ input, value, expected }, { expect }) => { + const attr = attributesOf(FileBlock)[input] + + expect( + attr?.renderHTML?.({ [input]: value }), + ).toEqual({ + [expected]: value, + }) + }, + ) + + it.for([ + { name: "src", input: "src" }, + { name: "name", input: "name" }, + { name: "size", input: "size" }, + { name: "content type", input: "contentType" }, + ])( + "writes no attribute for an unset $name", + ({ input }, { expect }) => { + const attr = attributesOf(FileBlock)[input] + + expect( + attr?.renderHTML?.({ [input]: null }), + ).toEqual({}) + }, + ) + }) + describe("figma attributes", () => { // the width and height arrive as strings on the element and are // stored as numbers, so a document round-trips as numbers diff --git a/server/auth-realtime/src/schema/index.ts b/server/auth-realtime/src/schema/index.ts index e4286dc8..4c8dc4c0 100644 --- a/server/auth-realtime/src/schema/index.ts +++ b/server/auth-realtime/src/schema/index.ts @@ -27,6 +27,7 @@ import { CalloutBlock } from "./callout.js" import { MermaidBlock } from "./mermaid.js" import { MetricBlock, MetricGrid } from "./metric.js" import { ImageBlock } from "./image.js" +import { FileBlock } from "./file.js" import { FigmaBlock } from "./figma.js" import { CodeBlock, CodeBlockTitle, TitledCodeBlock } from "./code-block.js" import { @@ -67,6 +68,7 @@ const contentExtensionsWithIDs: Extensions = [ MetricBlock, MetricGrid, ImageBlock, + FileBlock, FigmaBlock, CodeBlock, MermaidBlock, diff --git a/server/core/internal/assistant/block/block.go b/server/core/internal/assistant/block/block.go index 2ec79d1d..23708313 100644 --- a/server/core/internal/assistant/block/block.go +++ b/server/core/internal/assistant/block/block.go @@ -79,6 +79,12 @@ const ( // "alt", "title", "width". BlockImage Type = "image" + // BlockFile is a file attachment. Attrs "src", "name", + // "contentType" and "size" describe the uploaded file. It is only + // ever created by uploading in the editor: the AI reads and moves + // one but never writes it. + BlockFile Type = "file" + // BlockFigma is an embedded Figma frame. Required attr "src"; // optional "width", "height". BlockFigma Type = "figma" diff --git a/server/core/internal/assistant/block/compact.go b/server/core/internal/assistant/block/compact.go index 2471377a..7c2dec93 100644 --- a/server/core/internal/assistant/block/compact.go +++ b/server/core/internal/assistant/block/compact.go @@ -45,6 +45,8 @@ func Compact(b document.Block) (Block, error) { }, nil case document.BlockNodeImageBlock: return compactImage(b, uid), nil + case document.BlockNodeFileBlock: + return compactFile(b, uid), nil case document.BlockNodeFigmaBlock: return compactFigma(b, uid), nil case document.BlockNodeMetricBlock: @@ -335,6 +337,21 @@ func compactImage(b document.Block, uid string) Block { } } +// compactFile compacts a fileBlock atom node, surfacing its +// src/name/contentType/size attributes. +func compactFile(b document.Block, uid string) Block { + attrs := document.Attributes{} + + copyStringAttrs(attrs, b.Attrs, document.AttrSrc, document.AttrName, document.AttrContentType) + copyPositiveIntAttrs(attrs, b.Attrs, document.AttrSize) + + return Block{ + Type: BlockFile, + UID: uid, + Attrs: attrsOrNil(attrs), + } +} + // compactFigma compacts a figmaBlock atom node, surfacing its // src/width/height attributes. func compactFigma(b document.Block, uid string) Block { diff --git a/server/core/internal/assistant/block/compact_test.go b/server/core/internal/assistant/block/compact_test.go index b85410df..dba1e339 100644 --- a/server/core/internal/assistant/block/compact_test.go +++ b/server/core/internal/assistant/block/compact_test.go @@ -360,6 +360,29 @@ func Test_Compact(t *testing.T) { Attrs: map[string]any{"src": "http://x", "title": "shot", "width": 200}, }, }, + "File carries src, name, content type and size attrs": { + Input: document.Block{ + Type: document.BlockNodeFileBlock, + Attrs: map[string]any{ + "uid": "f1", "src": "http://x", "name": "notes.zip", "contentType": "application/zip", "size": 2048, "uploading": false, + }, + }, + Expected: Block{ + Type: BlockFile, + UID: "f1", + Attrs: map[string]any{"src": "http://x", "name": "notes.zip", "contentType": "application/zip", "size": 2048}, + }, + }, + "File without attrs": { + Input: document.Block{ + Type: document.BlockNodeFileBlock, + Attrs: map[string]any{"uid": "f1"}, + }, + Expected: Block{ + Type: BlockFile, + UID: "f1", + }, + }, "Metric passes attrs through without uid": { Input: document.Block{ Type: document.BlockNodeMetricBlock, diff --git a/server/core/internal/assistant/block/expand.go b/server/core/internal/assistant/block/expand.go index 7e313704..8a4f5245 100644 --- a/server/core/internal/assistant/block/expand.go +++ b/server/core/internal/assistant/block/expand.go @@ -44,6 +44,8 @@ func Expand(b Block) (document.Block, error) { return expandHorizontalRule(b), nil case BlockImage: return expandImage(b), nil + case BlockFile: + return document.Block{}, errFileNotAuthored case BlockFigma: return expandFigma(b), nil case BlockMetric: diff --git a/server/core/internal/assistant/block/expand_test.go b/server/core/internal/assistant/block/expand_test.go index 9e27e61e..792492af 100644 --- a/server/core/internal/assistant/block/expand_test.go +++ b/server/core/internal/assistant/block/expand_test.go @@ -641,6 +641,10 @@ func Test_Expand(t *testing.T) { Input: Block{Type: "not_a_type"}, Err: assert.AnError, }, + "File cannot be authored": { + Input: Block{Type: BlockFile, Attrs: map[string]any{"src": "http://x", "name": "notes.zip"}}, + Err: errFileNotAuthored, + }, } for cn, c := range cc { diff --git a/server/core/internal/assistant/block/types.go b/server/core/internal/assistant/block/types.go index f1be8ec2..34790969 100644 --- a/server/core/internal/assistant/block/types.go +++ b/server/core/internal/assistant/block/types.go @@ -20,6 +20,7 @@ var _canonicalTypes = map[document.BlockNodeType]Type{ document.BlockNodeMermaidBlock: BlockMermaid, document.BlockNodeHorizontalRule: BlockHorizontalRule, document.BlockNodeImageBlock: BlockImage, + document.BlockNodeFileBlock: BlockFile, document.BlockNodeFigmaBlock: BlockFigma, document.BlockNodeMetricBlock: BlockMetric, document.BlockNodeMetricGrid: BlockMetricGrid, @@ -44,6 +45,7 @@ var _typeOrder = []Type{ BlockMermaid, BlockHorizontalRule, BlockImage, + BlockFile, BlockFigma, BlockMetric, BlockMetricGrid, diff --git a/server/core/internal/assistant/block/validate.go b/server/core/internal/assistant/block/validate.go index 97c52f57..29a4379e 100644 --- a/server/core/internal/assistant/block/validate.go +++ b/server/core/internal/assistant/block/validate.go @@ -1,6 +1,7 @@ package block import ( + "errors" "fmt" "slices" "strings" @@ -8,6 +9,14 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" ) +// errFileNotAuthored is returned by every write path handed a file +// block: one exists only by uploading in the editor, so the AI reads and +// moves it but never writes it. +var errFileNotAuthored = errors.New( + "file blocks are created by uploading in the editor and cannot be written by the assistant; " + + "move_block moves an existing one", +) + // Allowed child-type sets, mirroring the editor's schema rules. var ( // _allowedBlockquoteItems mirrors TipTap's default Blockquote @@ -102,6 +111,7 @@ var ( BlockMermaid: true, BlockHorizontalRule: true, BlockImage: true, + BlockFile: true, BlockFigma: true, BlockMetricGrid: true, BlockSplitDoc: true, @@ -176,6 +186,8 @@ func ValidateAttrs(t Type, attrs document.Attributes) error { return validateTitledCodeAttrs(attrs, "") case BlockImage, BlockFigma: return validateSrcAttrs(t, attrs, "") + case BlockFile: + return errFileNotAuthored case BlockMetric: return validateMetricAttrs(attrs, "") case BlockParagraph, @@ -287,6 +299,8 @@ func validateBlock(b Block, path string) error { return validateMetric(b, path) case BlockImage, BlockFigma: return validateAtomWithSrc(b, path) + case BlockFile: + return verr(path, errFileNotAuthored.Error()) case BlockMetricGrid: return validateMetricGrid(b, path) case BlockSplitDoc: diff --git a/server/core/internal/assistant/block/validate_test.go b/server/core/internal/assistant/block/validate_test.go index e8ac1341..ded84e4a 100644 --- a/server/core/internal/assistant/block/validate_test.go +++ b/server/core/internal/assistant/block/validate_test.go @@ -441,6 +441,10 @@ func Test_Validate(t *testing.T) { "Image with src passes": { Input: Block{Type: BlockImage, Attrs: map[string]any{"src": "http://x"}}, }, + "File is rejected as authoring": { + Input: Block{Type: BlockFile, Attrs: map[string]any{"src": "http://x", "name": "notes.zip"}}, + Err: assert.AnError, + }, "Figma with task_items is rejected": { Input: Block{Type: BlockFigma, TaskItems: []TaskItem{{Block: Block{Type: BlockParagraph}}}}, Err: assert.AnError, @@ -662,6 +666,10 @@ func Test_ValidateAsRoot(t *testing.T) { Input: Block{Type: BlockHeading, Text: "x"}, Err: assert.AnError, }, + "File at root is rejected as authoring": { + Input: Block{Type: BlockFile, Attrs: map[string]any{"src": "http://x"}}, + Err: assert.AnError, + }, } for cn, c := range cc { @@ -683,6 +691,109 @@ func Test_ValidateAsRoot(t *testing.T) { } } +func Test_ValidateAttrs(t *testing.T) { + cc := map[string]struct { + Type Type + Attrs document.Attributes + Err error + }{ + "Heading with a level passes": { + Type: BlockHeading, + Attrs: document.Attributes{"level": 2}, + }, + "Heading without a level fails": { + Type: BlockHeading, + Err: assert.AnError, + }, + "Titled code with a title passes": { + Type: BlockTitledCode, + Attrs: document.Attributes{"title": "ex.go"}, + }, + "Image with src passes": { + Type: BlockImage, + Attrs: document.Attributes{"src": "http://x"}, + }, + "Figma without src fails": { + Type: BlockFigma, + Err: assert.AnError, + }, + "File is rejected as authoring": { + Type: BlockFile, + Attrs: document.Attributes{"name": "renamed.zip"}, + Err: errFileNotAuthored, + }, + "Metric with a valid width passes": { + Type: BlockMetric, + Attrs: document.Attributes{"width": "compact"}, + }, + "Paragraph carries no attribute rules": { + Type: BlockParagraph, + Attrs: document.Attributes{"anything": "goes"}, + }, + "Unknown type carries no attribute rules": { + Type: Type("wibble"), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + err := ValidateAttrs(c.Type, c.Attrs) + + testutil.AssertEqualError(t, c.Err, err) + }) + } +} + +func Test_AllowedInContainer(t *testing.T) { + cc := map[string]struct { + Container document.BlockNodeType + Type Type + Err error + }{ + "Paragraph at the root": { + Container: document.BlockNodeDoc, + Type: BlockParagraph, + }, + // a file block is never written, but an existing one can be moved + // among the root blocks the way an image can. + "File at the root": { + Container: document.BlockNodeDoc, + Type: BlockFile, + }, + "Metric at the root": { + Container: document.BlockNodeDoc, + Type: BlockMetric, + Err: assert.AnError, + }, + "Metric in a metric grid": { + Container: document.BlockNodeMetricGrid, + Type: BlockMetric, + }, + "File in a metric grid": { + Container: document.BlockNodeMetricGrid, + Type: BlockFile, + Err: assert.AnError, + }, + "Unknown container": { + Container: document.BlockNodeType("wibble"), + Type: BlockParagraph, + Err: assert.AnError, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + err := AllowedInContainer(c.Container, c.Type) + + testutil.AssertEqualError(t, c.Err, err) + }) + } +} + func Test_ValidateInContainer(t *testing.T) { titledCode := Block{ Type: BlockTitledCode, diff --git a/server/core/internal/assistant/prompt.go b/server/core/internal/assistant/prompt.go index 9a04abb3..5af5c5d6 100644 --- a/server/core/internal/assistant/prompt.go +++ b/server/core/internal/assistant/prompt.go @@ -68,13 +68,14 @@ Inline text is a minimal markdown subset: **bold**, *italic*, _underline_, ~~str | mermaid | text (raw mermaid source) | - | | horizontal_rule | - | - | | image | - | src (required), alt, title, width | +| file | - | name, contentType, size, src; read-only | | figma | - | src (required), width, height | | metric | - | chart configuration, see "Metric blocks" | | metric_grid | items: [metric] | - | | split_doc | left: [Block], right: [Block] | inversed (optional) | | split_doc_param_list | header (plain text), params: [{name, type, description}] | - | -Three types live only inside a container: titled_code and metric go in split_doc's right side (metric also in a metric_grid), and split_doc_param_list goes in split_doc's left side. Every other type is fine at the document root, and a write that puts a block where its type is not allowed is rejected. +Three types live only inside a container: titled_code and metric go in split_doc's right side (metric also in a metric_grid), and split_doc_param_list goes in split_doc's left side. Every other type is fine at the document root, and a write that puts a block where its type is not allowed is rejected. A file block is a file someone uploaded in the editor: you can read it and move it, but a write that creates or edits one is rejected. ### Metric blocks diff --git a/server/core/internal/assistant/tools/docwalk.go b/server/core/internal/assistant/tools/docwalk.go index 153e574c..c9027ee0 100644 --- a/server/core/internal/assistant/tools/docwalk.go +++ b/server/core/internal/assistant/tools/docwalk.go @@ -122,6 +122,7 @@ func (w *docWalker) walkLevel(blocks []document.Block, depth int, parentUID stri document.BlockNodeMermaidBlock, document.BlockNodeHorizontalRule, document.BlockNodeImageBlock, + document.BlockNodeFileBlock, document.BlockNodeFigmaBlock, document.BlockNodeMetricBlock, document.BlockNodeListItem, @@ -216,7 +217,7 @@ func canonicalKindForPM(pm document.BlockNodeType) string { // (already on the entry); opaque metric configurations are dropped // because they would balloon the payload and the AI never edits // them by hand anyway. -func summaryAttrs(b document.Block) map[string]any { //nolint:cyclop // this function is complex, however, it's well-structured +func summaryAttrs(b document.Block) map[string]any { switch b.Type { case document.BlockNodeHeading: if v, ok := b.Attrs[document.AttrLevel]; ok { @@ -235,29 +236,11 @@ func summaryAttrs(b document.Block) map[string]any { //nolint:cyclop // this fun return map[string]any{document.AttrChecked: v} } case document.BlockNodeImageBlock: - out := map[string]any{} - - for _, k := range []string{document.AttrSrc, document.AttrAlt, document.AttrTitle, document.AttrWidth} { - if v, ok := b.Attrs[k]; ok && v != nil && v != "" { - out[k] = v - } - } - - if len(out) > 0 { - return out - } + return presentAttrs(b, document.AttrSrc, document.AttrAlt, document.AttrTitle, document.AttrWidth) + case document.BlockNodeFileBlock: + return presentAttrs(b, document.AttrName, document.AttrContentType, document.AttrSize, document.AttrSrc) case document.BlockNodeFigmaBlock: - out := map[string]any{} - - for _, k := range []string{document.AttrSrc, document.AttrWidth, document.AttrHeight} { - if v, ok := b.Attrs[k]; ok && v != nil && v != "" { - out[k] = v - } - } - - if len(out) > 0 { - return out - } + return presentAttrs(b, document.AttrSrc, document.AttrWidth, document.AttrHeight) case document.BlockNodeSplitDoc: if a, ok := b.Attrs.Get(document.AttrInversed); ok && a.Bool() { return map[string]any{document.AttrInversed: true} @@ -268,3 +251,21 @@ func summaryAttrs(b document.Block) map[string]any { //nolint:cyclop // this fun return nil } + +// presentAttrs picks the named attrs the block carries with a value, +// or nil when it carries none of them. +func presentAttrs(b document.Block, keys ...string) map[string]any { + out := map[string]any{} + + for _, k := range keys { + if v, ok := b.Attrs[k]; ok && v != nil && v != "" { + out[k] = v + } + } + + if len(out) == 0 { + return nil + } + + return out +} diff --git a/server/core/internal/assistant/tools/docwalk_test.go b/server/core/internal/assistant/tools/docwalk_test.go index d1b0bb53..b19417de 100644 --- a/server/core/internal/assistant/tools/docwalk_test.go +++ b/server/core/internal/assistant/tools/docwalk_test.go @@ -181,6 +181,7 @@ func Test_canonicalKindForPM(t *testing.T) { "Mermaid": {PM: document.BlockNodeMermaidBlock, Result: "mermaid"}, "Horizontal rule": {PM: document.BlockNodeHorizontalRule, Result: "horizontal_rule"}, "Image": {PM: document.BlockNodeImageBlock, Result: "image"}, + "File": {PM: document.BlockNodeFileBlock, Result: "file"}, "Figma": {PM: document.BlockNodeFigmaBlock, Result: "figma"}, "Metric": {PM: document.BlockNodeMetricBlock, Result: "metric"}, "Metric grid": {PM: document.BlockNodeMetricGrid, Result: "metric_grid"}, @@ -198,6 +199,38 @@ func Test_canonicalKindForPM(t *testing.T) { } } +func Test_presentAttrs(t *testing.T) { + cc := map[string]struct { + Input document.Block + Keys []string + Result map[string]any + }{ + "Named attrs with values are kept": { + Input: pmBlock(document.BlockNodeFileBlock, "f", map[string]any{"name": "a.zip", "size": 1, "other": "x"}), + Keys: []string{"name", "size"}, + Result: map[string]any{"name": "a.zip", "size": 1}, + }, + "Empty and nil values are dropped": { + Input: pmBlock(document.BlockNodeFileBlock, "f", map[string]any{"name": "", "size": nil, "src": "http://f"}), + Keys: []string{"name", "size", "src"}, + Result: map[string]any{"src": "http://f"}, + }, + "Nothing present": { + Input: pmBlock(document.BlockNodeFileBlock, "f", nil), + Keys: []string{"name"}, + Result: nil, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, presentAttrs(c.Input, c.Keys...)) + }) + } +} + func Test_summaryAttrs(t *testing.T) { cc := map[string]struct { Input document.Block @@ -237,6 +270,16 @@ func Test_summaryAttrs(t *testing.T) { Input: pmBlock(document.BlockNodeImageBlock, "i", nil), Result: nil, }, + "File attrs filtered": { + Input: pmBlock(document.BlockNodeFileBlock, "f", map[string]any{ + "src": "http://f", "name": "notes.zip", "contentType": "application/zip", "size": 2048, "uploading": false, + }), + Result: map[string]any{"src": "http://f", "name": "notes.zip", "contentType": "application/zip", "size": 2048}, + }, + "File without usable attrs": { + Input: pmBlock(document.BlockNodeFileBlock, "f", nil), + Result: nil, + }, "Figma attrs filtered": { Input: pmBlock(document.BlockNodeFigmaBlock, "f", map[string]any{ "src": "http://f", "height": 200, diff --git a/server/core/internal/assistant/tools/testdata/tool_schemas.golden b/server/core/internal/assistant/tools/testdata/tool_schemas.golden index 25c6c8e8..a8979729 100644 --- a/server/core/internal/assistant/tools/testdata/tool_schemas.golden +++ b/server/core/internal/assistant/tools/testdata/tool_schemas.golden @@ -1 +1 @@ -{"create_document":{"desc":"Create a new document and return {document_id, branch_id}, the branch_id being its default branch. The document starts with a single empty paragraph, so follow up with insert_block calls (position end, that branch_id) in the same turn to fill it. Omit parent_id to create it at the organisation root.","has_params_one_of":true,"json_schema":{"properties":{"icon":{"description":"Iconify identifier, as \"collection:name\". The product's own icons are MingCute fills (e.g. \"mingcute:file-code-fill\"); prefer one so the document matches the rest of the sidebar. Defaults to \"mingcute:document-2-fill\" when empty.","type":"string"},"name":{"description":"Display name for the new document.","type":"string"},"parent_id":{"description":"Optional parent document id. Omit to create at the org root.","type":"string"}},"required":["name"],"type":"object"},"name":"create_document"},"delete_block":{"desc":"Delete one block by uid, including anything nested inside it. Its comments, hooks and files go with it and cannot be restored, so use move_block when the aim is to reorder and update_block_text when the aim is new wording. Returns {deleted: uid}.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block to delete.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid"],"type":"object"},"name":"delete_block"},"delete_document":{"desc":"Delete a document and every document nested under it. The whole subtree goes and cannot be restored, so check the tree with list_documents first, and use update_document when the aim is to relocate rather than remove. Returns {document_id, deleted}.","has_params_one_of":true,"json_schema":{"properties":{"document_id":{"description":"The document id to delete.","type":"string"}},"required":["document_id"],"type":"object"},"name":"delete_document"},"get_document":{"desc":"Read one document on one branch: its name, icon, parent_id, protected flag and updated_at, the branch read and every branch the document has (each with the id a branch_id argument takes), followed by one row per block with the block's uid, kind, flattened text, depth, parent_uid and the few attrs that matter for reading (heading level, callout icon, code language, task checked). Use it as the way to read a document before editing it; branch_id is the default_branch_id a listing or search hit carries, or any id from branches. A protected branch can be read but refuses every write, so pick an unprotected branch id from branches to write to. Rows marked has_children hold nested blocks the rows do not list, and read_block returns those when you need them.","has_params_one_of":true,"json_schema":{"properties":{"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The document id.","type":"string"}},"required":["document_id","branch_id"],"type":"object"},"name":"get_document"},"get_prometheus_metadata":{"desc":"List the metrics a Prometheus data source exposes, with each metric's type, help text and unit. Use it to find the metric names to write a PromQL query against instead of guessing them.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"get_prometheus_metadata"},"get_sql_metadata":{"desc":"List the tables and their columns in a PostgreSQL, MariaDB or MySQL data source, plus the default schema. Read it before writing a query so the table and column names are the real ones.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"get_sql_metadata"},"get_sql_query_labels":{"desc":"Run a SQL query limited to one row and return its string columns with an example value each, which are the columns a chart would treat as series labels. Use it to check what a query returns before charting it; it is cheaper than query_sql.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The SQL query to probe. $__ macros are expanded as they are for a metric block.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"get_sql_query_labels"},"insert_block":{"desc":"Insert one canonical block into a document. position start or end puts it at the document's start or end; before or after puts it beside the block reference_block_uid names, which stays in place. The block has to be legal where it lands: the document root takes every type except titled_code, metric and split_doc_param_list, and a reference inside a split_doc or metric_grid takes only what that container holds. Returns the summary rows of the new block and everything nested in it, uids included, with depth counted from the block itself.","has_params_one_of":true,"json_schema":{"properties":{"block":{"description":"A canonical block. Which fields apply depends on type; supply only that type's own.","properties":{"attrs":{"description":"Per-type attributes. Required where the type says so: heading level, titled_code title, image and figma src. Also code language, callout icon, image alt and width, and a metric's chart configuration, whose width is compact, standard or wide.","properties":{"level":{"description":"heading: the level. Required on a heading.","enum":[1,2,3],"type":"integer"},"refreshInterval":{"description":"metric: how often the chart re-queries.","enum":["5s","10s","30s","1m","5m","15m","30m","1h","2h","1d"],"type":"string"},"simulationPreset":{"description":"metric: draws this generated series in place of the query's own result, for a block documenting a metric that has no real data yet. Omit it to chart the query.","enum":["cpu_usage","memory_usage","disk_usage","http_requests","http_latency","error_rate"],"type":"string"},"timeRange":{"description":"metric: the window queried.","enum":["last_5_minutes","last_15_minutes","last_30_minutes","last_1_hour","last_3_hours","last_6_hours","last_12_hours","last_24_hours","last_2_days","last_7_days","last_30_days","last_90_days","last_6_months","last_1_year","last_2_years","last_5_years","today","yesterday","today_so_far","this_week","this_week_so_far","this_month","this_month_so_far","this_year","this_year_so_far","previous_week","previous_month","previous_year"],"type":"string"},"unitType":{"description":"metric: the unit values are read in. With custom, put the label in unitCustom.","enum":["custom","nanoseconds","microseconds","milliseconds","seconds","minutes","hours","days","bytes","kilobytes","megabytes","gigabytes","terabytes","bits","kilobits","megabits","gigabits","terabits","percent0to100","percent0to1"],"type":"string"},"visualizationType":{"description":"metric: the chart kind.","enum":["line_chart","bar_chart","gauge_chart"],"type":"string"}},"type":"object"},"children":{"description":"Blocks nested under a single list entry. A list entry is a paragraph; anything else nested under it goes here, never in the entry itself and never in a second entry.","type":"array"},"header":{"description":"split_doc_param_list heading, as plain text.","type":"string"},"items":{"description":"Entries of bullet_list and ordered_list, which are paragraphs; items of metric_grid, and of blockquote or callout when not using text.","type":"array"},"left":{"description":"split_doc concept side. Must start with a level-1 heading; split_doc_param_list is legal only here.","type":"array"},"params":{"description":"split_doc_param_list rows, each {name, type, description}.","type":"array"},"right":{"description":"split_doc example side. Holds titled_code or metric only, and is the one place titled_code is legal.","type":"array"},"task_items":{"description":"Rows of task_list, each {checked, block}.","type":"array"},"text":{"description":"Inline text for paragraph, heading, blockquote, code, titled_code, mermaid and callout. Raw inside code, titled_code and mermaid; minimal markdown elsewhere. One block is one paragraph; a newline inside text does not start a new one.","type":"string"},"type":{"description":"The block's canonical type.","enum":["paragraph","heading","blockquote","bullet_list","ordered_list","task_list","callout","code","titled_code","mermaid","horizontal_rule","image","figma","metric","metric_grid","split_doc","split_doc_param_list"],"type":"string"},"uid":{"description":"Leave unset. Uids are generated server-side and are the handle later edits use.","type":"string"}},"required":["type"],"type":"object"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"position":{"description":"Where the block lands: before or after the reference block, or at the start or end of the document.","enum":["before","after","start","end"],"type":"string"},"reference_block_uid":{"description":"The uid of the block to insert beside. Required with before and after; leave it out with start and end.","type":"string"}},"required":["document_id","branch_id","position","block"],"type":"object"},"name":"insert_block"},"list_data_sources":{"desc":"List the organisation's data sources. Returns id, name, type (prometheus, postgresql, mariadb, mysql) and connection status. Start here: every other data-source tool takes an id from this list, and the type decides which of them serves it.","has_params_one_of":true,"json_schema":{"properties":{},"type":"object"},"name":"list_data_sources"},"list_documents":{"desc":"List the organisation's documents as a tree of {id, name, default_branch_id, children}; default_branch_id is what get_document and the block tools take as branch_id for the document's default branch. Use it to find a document by name or to see what sits under a parent; use search_documents when you are looking for content rather than a title. Pass parent_id to get only that document's direct children, or omit it for the whole organisation.","has_params_one_of":true,"json_schema":{"properties":{"parent_id":{"description":"Optional. Return only the direct children of this document. Omit for the full tree.","type":"string"}},"type":"object"},"name":"list_documents"},"list_prometheus_label_names":{"desc":"List the label names present in a Prometheus data source, optionally only those on the series the matchers select. Pair it with list_prometheus_label_values to build a filtered PromQL query.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"matchers":{"description":"Optional. PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"list_prometheus_label_names"},"list_prometheus_label_values":{"desc":"List the values a label takes in a Prometheus data source, optionally only on the series the matchers select. Use it to discover the concrete label values a query should filter on.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"label":{"description":"The label whose values to list, e.g. \"job\".","type":"string"},"matchers":{"description":"Optional. PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","label"],"type":"object"},"name":"list_prometheus_label_values"},"list_prometheus_series":{"desc":"List the series matching a set of selectors in a Prometheus data source, each as its full label set. Use it to see which label combinations a metric actually has before querying it.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"matchers":{"description":"PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","matchers"],"type":"object"},"name":"list_prometheus_series"},"move_block":{"desc":"Move an existing block before or after another block in the same document. The block keeps its uid, attrs and nested content, so comments, hooks and files attached to it stay attached; deleting it and inserting a copy would lose them. The landing spot has to accept the block's type, by the same rule insert_block applies. Returns the summary rows of the moved block and everything nested in it.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block to move.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"position":{"description":"Landing side relative to the reference block.","enum":["before","after"],"type":"string"},"reference_block_uid":{"description":"The uid of the block to move relative to.","type":"string"}},"required":["document_id","branch_id","block_uid","position","reference_block_uid"],"type":"object"},"name":"move_block"},"query_prometheus":{"desc":"Run a PromQL range query against a Prometheus data source over the given window. Returns the raw result by default; with chart_type set it instead describes what a metric block would render, which is how to check a query before writing it into a block.","has_params_one_of":true,"json_schema":{"properties":{"chart_type":{"description":"Optional. One of line_chart, bar_chart, gauge_chart. When set, the result describes what the metric block would draw (render status, series count, and each series' labels, point count and endpoints) instead of the raw data. Use it to check a query before putting it in a metric block; omit it when you need the values themselves.","type":"string"},"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The PromQL expression to run.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"query_prometheus"},"query_sql":{"desc":"Run a read-only query against a PostgreSQL, MariaDB or MySQL data source. Returns columns and rows by default; with chart_type set it instead describes what a metric block would render, which is how to check a query before writing it into a block. $__ macros ($__timeFilter, $__timeGroupAlias and the rest) are expanded against the window.","has_params_one_of":true,"json_schema":{"properties":{"chart_type":{"description":"Optional. One of line_chart, bar_chart, gauge_chart. When set, the result describes what the metric block would draw (render status, series count, and each series' labels, point count and endpoints) instead of the raw data. Use it to check a query before putting it in a metric block; omit it when you need the values themselves.","type":"string"},"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The SQL query to run. For a chart, select a time column aliased \"time\" plus one or more numeric columns.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"query_sql"},"read_block":{"desc":"Return the full canonical content of one block by uid, including any nested children. Use it only when get_document's rows are not enough: to edit a split_doc, a nested list or a split_doc_param_list, whose inner structure has to be written back in full. Fails when the uid is not in the document.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The block uid to fetch.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The document id.","type":"string"}},"required":["document_id","branch_id","block_uid"],"type":"object"},"name":"read_block"},"read_tool_output":{"desc":"Retrieve the full output of an earlier tool call that was too large to keep in the conversation. Pass the path shown in the truncation notice.","has_params_one_of":true,"json_schema":{"properties":{"file_path":{"description":"The stored output path from the truncation notice.","type":"string"}},"required":["file_path"],"type":"object"},"name":"read_tool_output"},"replace_block":{"desc":"Replace a block by uid with a new block in the same position. The old block's uid, content and children are all gone unless the new block carries them, so use it to change a block's type or its whole structure. For a wording change use update_block_text, and for an attribute change update_block_attrs; both keep the uid, which comments, hooks and files hang off. Returns the summary rows of the new block and everything nested in it, uids included, with depth counted from the block itself.","has_params_one_of":true,"json_schema":{"properties":{"block":{"description":"A canonical block. Which fields apply depends on type; supply only that type's own.","properties":{"attrs":{"description":"Per-type attributes. Required where the type says so: heading level, titled_code title, image and figma src. Also code language, callout icon, image alt and width, and a metric's chart configuration, whose width is compact, standard or wide.","properties":{"level":{"description":"heading: the level. Required on a heading.","enum":[1,2,3],"type":"integer"},"refreshInterval":{"description":"metric: how often the chart re-queries.","enum":["5s","10s","30s","1m","5m","15m","30m","1h","2h","1d"],"type":"string"},"simulationPreset":{"description":"metric: draws this generated series in place of the query's own result, for a block documenting a metric that has no real data yet. Omit it to chart the query.","enum":["cpu_usage","memory_usage","disk_usage","http_requests","http_latency","error_rate"],"type":"string"},"timeRange":{"description":"metric: the window queried.","enum":["last_5_minutes","last_15_minutes","last_30_minutes","last_1_hour","last_3_hours","last_6_hours","last_12_hours","last_24_hours","last_2_days","last_7_days","last_30_days","last_90_days","last_6_months","last_1_year","last_2_years","last_5_years","today","yesterday","today_so_far","this_week","this_week_so_far","this_month","this_month_so_far","this_year","this_year_so_far","previous_week","previous_month","previous_year"],"type":"string"},"unitType":{"description":"metric: the unit values are read in. With custom, put the label in unitCustom.","enum":["custom","nanoseconds","microseconds","milliseconds","seconds","minutes","hours","days","bytes","kilobytes","megabytes","gigabytes","terabytes","bits","kilobits","megabits","gigabits","terabits","percent0to100","percent0to1"],"type":"string"},"visualizationType":{"description":"metric: the chart kind.","enum":["line_chart","bar_chart","gauge_chart"],"type":"string"}},"type":"object"},"children":{"description":"Blocks nested under a single list entry. A list entry is a paragraph; anything else nested under it goes here, never in the entry itself and never in a second entry.","type":"array"},"header":{"description":"split_doc_param_list heading, as plain text.","type":"string"},"items":{"description":"Entries of bullet_list and ordered_list, which are paragraphs; items of metric_grid, and of blockquote or callout when not using text.","type":"array"},"left":{"description":"split_doc concept side. Must start with a level-1 heading; split_doc_param_list is legal only here.","type":"array"},"params":{"description":"split_doc_param_list rows, each {name, type, description}.","type":"array"},"right":{"description":"split_doc example side. Holds titled_code or metric only, and is the one place titled_code is legal.","type":"array"},"task_items":{"description":"Rows of task_list, each {checked, block}.","type":"array"},"text":{"description":"Inline text for paragraph, heading, blockquote, code, titled_code, mermaid and callout. Raw inside code, titled_code and mermaid; minimal markdown elsewhere. One block is one paragraph; a newline inside text does not start a new one.","type":"string"},"type":{"description":"The block's canonical type.","enum":["paragraph","heading","blockquote","bullet_list","ordered_list","task_list","callout","code","titled_code","mermaid","horizontal_rule","image","figma","metric","metric_grid","split_doc","split_doc_param_list"],"type":"string"},"uid":{"description":"Leave unset. Uids are generated server-side and are the handle later edits use.","type":"string"}},"required":["type"],"type":"object"},"block_uid":{"description":"The uid of the block being replaced.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid","block"],"type":"object"},"name":"replace_block"},"search_documents":{"desc":"Full-text search across every branch of every document in the organisation for blocks whose text matches the query. Returns hits with document_id, document_name, branch_id, branch_name, default (whether that branch is the document's default), block_uid and text; branch_id is what a follow-up get_document or read_block takes. A block a fork copied unchanged is found on every branch that holds it. Use it to find where a topic is discussed or which documents the user might mean; use list_documents when you know the title. A hit names the innermost block holding the text, which may sit below anything get_document lists; read_block resolves it.","has_params_one_of":true,"json_schema":{"properties":{"limit":{"description":"Maximum number of hits to return. Defaults to 20; cap is 50.","type":"integer"},"query":{"description":"The full-text search query (typo-tolerant, matches block text).","type":"string"}},"required":["query"],"type":"object"},"name":"search_documents"},"update_block_attrs":{"desc":"Set or override named attributes on an existing block, such as a heading's level or a callout's icon. Attributes not mentioned are kept and uid cannot change. Values are validated for the block's type, so a level outside 1 to 3 or a metric width other than compact, standard or wide is rejected. Use replace_block when the type itself has to change. Returns the summary rows of the block and everything nested in it, attrs as they now stand.","has_params_one_of":true,"json_schema":{"properties":{"attrs":{"description":"Attribute keys and values to set (e.g. {\"level\": 2}, {\"icon\": \"lucide:warning\"}).","type":"object"},"block_uid":{"description":"The uid of the block whose attrs should be updated.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid","attrs"],"type":"object"},"name":"update_block_attrs"},"update_block_text":{"desc":"Replace the inline text of one text-bearing block: paragraph, heading, blockquote, code, titled_code, mermaid, or a callout written with text. Type, attrs and uid are kept, so this is the tool for wording changes. Text follows the canonical markdown subset (**bold**, *italic*, _underline_, ~~strike~~, backtick code, [label](url)), and is raw in code, titled_code and mermaid. One block is one paragraph; to add a paragraph, insert a block instead. Returns the block's summary row with the new text.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block whose text should be replaced.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"text":{"description":"New inline text in canonical markdown.","type":"string"}},"required":["document_id","branch_id","block_uid","text"],"type":"object"},"name":"update_block_text"},"update_document":{"desc":"Change a document's name, icon or place in the tree, any combination in one call; content is untouched. Give only the fields to change: name is the new display name, icon an Iconify identifier as \"collection:name\" (the product's own icons are MingCute fills, e.g. \"mingcute:rocket-fill\", so prefer one to match the sidebar), and parent_id the new parent, or an empty string for the organisation root. A call with none of the three is refused, and a parent that is the document itself or one of its descendants fails. Returns {document_id} with the fields that changed.","has_params_one_of":true,"json_schema":{"properties":{"document_id":{"description":"The document id.","type":"string"},"icon":{"description":"Optional. The new icon identifier; omit to keep the current one.","type":"string"},"name":{"description":"Optional. The new display name; omit to keep the current one.","type":"string"},"parent_id":{"description":"Optional. The new parent document id, or an empty string to move the document to the organisation root; omit to leave it where it is.","type":"string"}},"required":["document_id"],"type":"object"},"name":"update_document"}} \ No newline at end of file +{"create_document":{"desc":"Create a new document and return {document_id, branch_id}, the branch_id being its default branch. The document starts with a single empty paragraph, so follow up with insert_block calls (position end, that branch_id) in the same turn to fill it. Omit parent_id to create it at the organisation root.","has_params_one_of":true,"json_schema":{"properties":{"icon":{"description":"Iconify identifier, as \"collection:name\". The product's own icons are MingCute fills (e.g. \"mingcute:file-code-fill\"); prefer one so the document matches the rest of the sidebar. Defaults to \"mingcute:document-2-fill\" when empty.","type":"string"},"name":{"description":"Display name for the new document.","type":"string"},"parent_id":{"description":"Optional parent document id. Omit to create at the org root.","type":"string"}},"required":["name"],"type":"object"},"name":"create_document"},"delete_block":{"desc":"Delete one block by uid, including anything nested inside it. Its comments, hooks and files go with it and cannot be restored, so use move_block when the aim is to reorder and update_block_text when the aim is new wording. Returns {deleted: uid}.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block to delete.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid"],"type":"object"},"name":"delete_block"},"delete_document":{"desc":"Delete a document and every document nested under it. The whole subtree goes and cannot be restored, so check the tree with list_documents first, and use update_document when the aim is to relocate rather than remove. Returns {document_id, deleted}.","has_params_one_of":true,"json_schema":{"properties":{"document_id":{"description":"The document id to delete.","type":"string"}},"required":["document_id"],"type":"object"},"name":"delete_document"},"get_document":{"desc":"Read one document on one branch: its name, icon, parent_id, protected flag and updated_at, the branch read and every branch the document has (each with the id a branch_id argument takes), followed by one row per block with the block's uid, kind, flattened text, depth, parent_uid and the few attrs that matter for reading (heading level, callout icon, code language, task checked). Use it as the way to read a document before editing it; branch_id is the default_branch_id a listing or search hit carries, or any id from branches. A protected branch can be read but refuses every write, so pick an unprotected branch id from branches to write to. Rows marked has_children hold nested blocks the rows do not list, and read_block returns those when you need them.","has_params_one_of":true,"json_schema":{"properties":{"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The document id.","type":"string"}},"required":["document_id","branch_id"],"type":"object"},"name":"get_document"},"get_prometheus_metadata":{"desc":"List the metrics a Prometheus data source exposes, with each metric's type, help text and unit. Use it to find the metric names to write a PromQL query against instead of guessing them.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"get_prometheus_metadata"},"get_sql_metadata":{"desc":"List the tables and their columns in a PostgreSQL, MariaDB or MySQL data source, plus the default schema. Read it before writing a query so the table and column names are the real ones.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"get_sql_metadata"},"get_sql_query_labels":{"desc":"Run a SQL query limited to one row and return its string columns with an example value each, which are the columns a chart would treat as series labels. Use it to check what a query returns before charting it; it is cheaper than query_sql.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The SQL query to probe. $__ macros are expanded as they are for a metric block.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"get_sql_query_labels"},"insert_block":{"desc":"Insert one canonical block into a document. position start or end puts it at the document's start or end; before or after puts it beside the block reference_block_uid names, which stays in place. The block has to be legal where it lands: the document root takes every type except titled_code, metric and split_doc_param_list, and a reference inside a split_doc or metric_grid takes only what that container holds. Returns the summary rows of the new block and everything nested in it, uids included, with depth counted from the block itself.","has_params_one_of":true,"json_schema":{"properties":{"block":{"description":"A canonical block. Which fields apply depends on type; supply only that type's own.","properties":{"attrs":{"description":"Per-type attributes. Required where the type says so: heading level, titled_code title, image and figma src. Also code language, callout icon, image alt and width, and a metric's chart configuration, whose width is compact, standard or wide.","properties":{"level":{"description":"heading: the level. Required on a heading.","enum":[1,2,3],"type":"integer"},"refreshInterval":{"description":"metric: how often the chart re-queries.","enum":["5s","10s","30s","1m","5m","15m","30m","1h","2h","1d"],"type":"string"},"simulationPreset":{"description":"metric: draws this generated series in place of the query's own result, for a block documenting a metric that has no real data yet. Omit it to chart the query.","enum":["cpu_usage","memory_usage","disk_usage","http_requests","http_latency","error_rate"],"type":"string"},"timeRange":{"description":"metric: the window queried.","enum":["last_5_minutes","last_15_minutes","last_30_minutes","last_1_hour","last_3_hours","last_6_hours","last_12_hours","last_24_hours","last_2_days","last_7_days","last_30_days","last_90_days","last_6_months","last_1_year","last_2_years","last_5_years","today","yesterday","today_so_far","this_week","this_week_so_far","this_month","this_month_so_far","this_year","this_year_so_far","previous_week","previous_month","previous_year"],"type":"string"},"unitType":{"description":"metric: the unit values are read in. With custom, put the label in unitCustom.","enum":["custom","nanoseconds","microseconds","milliseconds","seconds","minutes","hours","days","bytes","kilobytes","megabytes","gigabytes","terabytes","bits","kilobits","megabits","gigabits","terabits","percent0to100","percent0to1"],"type":"string"},"visualizationType":{"description":"metric: the chart kind.","enum":["line_chart","bar_chart","gauge_chart"],"type":"string"}},"type":"object"},"children":{"description":"Blocks nested under a single list entry. A list entry is a paragraph; anything else nested under it goes here, never in the entry itself and never in a second entry.","type":"array"},"header":{"description":"split_doc_param_list heading, as plain text.","type":"string"},"items":{"description":"Entries of bullet_list and ordered_list, which are paragraphs; items of metric_grid, and of blockquote or callout when not using text.","type":"array"},"left":{"description":"split_doc concept side. Must start with a level-1 heading; split_doc_param_list is legal only here.","type":"array"},"params":{"description":"split_doc_param_list rows, each {name, type, description}.","type":"array"},"right":{"description":"split_doc example side. Holds titled_code or metric only, and is the one place titled_code is legal.","type":"array"},"task_items":{"description":"Rows of task_list, each {checked, block}.","type":"array"},"text":{"description":"Inline text for paragraph, heading, blockquote, code, titled_code, mermaid and callout. Raw inside code, titled_code and mermaid; minimal markdown elsewhere. One block is one paragraph; a newline inside text does not start a new one.","type":"string"},"type":{"description":"The block's canonical type.","enum":["paragraph","heading","blockquote","bullet_list","ordered_list","task_list","callout","code","titled_code","mermaid","horizontal_rule","image","file","figma","metric","metric_grid","split_doc","split_doc_param_list"],"type":"string"},"uid":{"description":"Leave unset. Uids are generated server-side and are the handle later edits use.","type":"string"}},"required":["type"],"type":"object"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"position":{"description":"Where the block lands: before or after the reference block, or at the start or end of the document.","enum":["before","after","start","end"],"type":"string"},"reference_block_uid":{"description":"The uid of the block to insert beside. Required with before and after; leave it out with start and end.","type":"string"}},"required":["document_id","branch_id","position","block"],"type":"object"},"name":"insert_block"},"list_data_sources":{"desc":"List the organisation's data sources. Returns id, name, type (prometheus, postgresql, mariadb, mysql) and connection status. Start here: every other data-source tool takes an id from this list, and the type decides which of them serves it.","has_params_one_of":true,"json_schema":{"properties":{},"type":"object"},"name":"list_data_sources"},"list_documents":{"desc":"List the organisation's documents as a tree of {id, name, default_branch_id, children}; default_branch_id is what get_document and the block tools take as branch_id for the document's default branch. Use it to find a document by name or to see what sits under a parent; use search_documents when you are looking for content rather than a title. Pass parent_id to get only that document's direct children, or omit it for the whole organisation.","has_params_one_of":true,"json_schema":{"properties":{"parent_id":{"description":"Optional. Return only the direct children of this document. Omit for the full tree.","type":"string"}},"type":"object"},"name":"list_documents"},"list_prometheus_label_names":{"desc":"List the label names present in a Prometheus data source, optionally only those on the series the matchers select. Pair it with list_prometheus_label_values to build a filtered PromQL query.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"matchers":{"description":"Optional. PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id"],"type":"object"},"name":"list_prometheus_label_names"},"list_prometheus_label_values":{"desc":"List the values a label takes in a Prometheus data source, optionally only on the series the matchers select. Use it to discover the concrete label values a query should filter on.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"label":{"description":"The label whose values to list, e.g. \"job\".","type":"string"},"matchers":{"description":"Optional. PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","label"],"type":"object"},"name":"list_prometheus_label_values"},"list_prometheus_series":{"desc":"List the series matching a set of selectors in a Prometheus data source, each as its full label set. Use it to see which label combinations a metric actually has before querying it.","has_params_one_of":true,"json_schema":{"properties":{"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"matchers":{"description":"PromQL series selectors, e.g. [\"up\", \"{job=\\\"api\\\"}\"].","items":{"type":"string"},"type":"array"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","matchers"],"type":"object"},"name":"list_prometheus_series"},"move_block":{"desc":"Move an existing block before or after another block in the same document. The block keeps its uid, attrs and nested content, so comments, hooks and files attached to it stay attached; deleting it and inserting a copy would lose them. The landing spot has to accept the block's type, by the same rule insert_block applies. Returns the summary rows of the moved block and everything nested in it.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block to move.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"position":{"description":"Landing side relative to the reference block.","enum":["before","after"],"type":"string"},"reference_block_uid":{"description":"The uid of the block to move relative to.","type":"string"}},"required":["document_id","branch_id","block_uid","position","reference_block_uid"],"type":"object"},"name":"move_block"},"query_prometheus":{"desc":"Run a PromQL range query against a Prometheus data source over the given window. Returns the raw result by default; with chart_type set it instead describes what a metric block would render, which is how to check a query before writing it into a block.","has_params_one_of":true,"json_schema":{"properties":{"chart_type":{"description":"Optional. One of line_chart, bar_chart, gauge_chart. When set, the result describes what the metric block would draw (render status, series count, and each series' labels, point count and endpoints) instead of the raw data. Use it to check a query before putting it in a metric block; omit it when you need the values themselves.","type":"string"},"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The PromQL expression to run.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"query_prometheus"},"query_sql":{"desc":"Run a read-only query against a PostgreSQL, MariaDB or MySQL data source. Returns columns and rows by default; with chart_type set it instead describes what a metric block would render, which is how to check a query before writing it into a block. $__ macros ($__timeFilter, $__timeGroupAlias and the rest) are expanded against the window.","has_params_one_of":true,"json_schema":{"properties":{"chart_type":{"description":"Optional. One of line_chart, bar_chart, gauge_chart. When set, the result describes what the metric block would draw (render status, series count, and each series' labels, point count and endpoints) instead of the raw data. Use it to check a query before putting it in a metric block; omit it when you need the values themselves.","type":"string"},"data_source_id":{"description":"The data source id, from list_data_sources.","type":"string"},"from":{"description":"Optional. Range start as an RFC3339 timestamp. Defaults to an hour before 'to'.","type":"string"},"query":{"description":"The SQL query to run. For a chart, select a time column aliased \"time\" plus one or more numeric columns.","type":"string"},"to":{"description":"Optional. Range end as an RFC3339 timestamp. Defaults to now.","type":"string"}},"required":["data_source_id","query"],"type":"object"},"name":"query_sql"},"read_block":{"desc":"Return the full canonical content of one block by uid, including any nested children. Use it only when get_document's rows are not enough: to edit a split_doc, a nested list or a split_doc_param_list, whose inner structure has to be written back in full. Fails when the uid is not in the document.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The block uid to fetch.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The document id.","type":"string"}},"required":["document_id","branch_id","block_uid"],"type":"object"},"name":"read_block"},"read_tool_output":{"desc":"Retrieve the full output of an earlier tool call that was too large to keep in the conversation. Pass the path shown in the truncation notice.","has_params_one_of":true,"json_schema":{"properties":{"file_path":{"description":"The stored output path from the truncation notice.","type":"string"}},"required":["file_path"],"type":"object"},"name":"read_tool_output"},"replace_block":{"desc":"Replace a block by uid with a new block in the same position. The old block's uid, content and children are all gone unless the new block carries them, so use it to change a block's type or its whole structure. For a wording change use update_block_text, and for an attribute change update_block_attrs; both keep the uid, which comments, hooks and files hang off. Returns the summary rows of the new block and everything nested in it, uids included, with depth counted from the block itself.","has_params_one_of":true,"json_schema":{"properties":{"block":{"description":"A canonical block. Which fields apply depends on type; supply only that type's own.","properties":{"attrs":{"description":"Per-type attributes. Required where the type says so: heading level, titled_code title, image and figma src. Also code language, callout icon, image alt and width, and a metric's chart configuration, whose width is compact, standard or wide.","properties":{"level":{"description":"heading: the level. Required on a heading.","enum":[1,2,3],"type":"integer"},"refreshInterval":{"description":"metric: how often the chart re-queries.","enum":["5s","10s","30s","1m","5m","15m","30m","1h","2h","1d"],"type":"string"},"simulationPreset":{"description":"metric: draws this generated series in place of the query's own result, for a block documenting a metric that has no real data yet. Omit it to chart the query.","enum":["cpu_usage","memory_usage","disk_usage","http_requests","http_latency","error_rate"],"type":"string"},"timeRange":{"description":"metric: the window queried.","enum":["last_5_minutes","last_15_minutes","last_30_minutes","last_1_hour","last_3_hours","last_6_hours","last_12_hours","last_24_hours","last_2_days","last_7_days","last_30_days","last_90_days","last_6_months","last_1_year","last_2_years","last_5_years","today","yesterday","today_so_far","this_week","this_week_so_far","this_month","this_month_so_far","this_year","this_year_so_far","previous_week","previous_month","previous_year"],"type":"string"},"unitType":{"description":"metric: the unit values are read in. With custom, put the label in unitCustom.","enum":["custom","nanoseconds","microseconds","milliseconds","seconds","minutes","hours","days","bytes","kilobytes","megabytes","gigabytes","terabytes","bits","kilobits","megabits","gigabits","terabits","percent0to100","percent0to1"],"type":"string"},"visualizationType":{"description":"metric: the chart kind.","enum":["line_chart","bar_chart","gauge_chart"],"type":"string"}},"type":"object"},"children":{"description":"Blocks nested under a single list entry. A list entry is a paragraph; anything else nested under it goes here, never in the entry itself and never in a second entry.","type":"array"},"header":{"description":"split_doc_param_list heading, as plain text.","type":"string"},"items":{"description":"Entries of bullet_list and ordered_list, which are paragraphs; items of metric_grid, and of blockquote or callout when not using text.","type":"array"},"left":{"description":"split_doc concept side. Must start with a level-1 heading; split_doc_param_list is legal only here.","type":"array"},"params":{"description":"split_doc_param_list rows, each {name, type, description}.","type":"array"},"right":{"description":"split_doc example side. Holds titled_code or metric only, and is the one place titled_code is legal.","type":"array"},"task_items":{"description":"Rows of task_list, each {checked, block}.","type":"array"},"text":{"description":"Inline text for paragraph, heading, blockquote, code, titled_code, mermaid and callout. Raw inside code, titled_code and mermaid; minimal markdown elsewhere. One block is one paragraph; a newline inside text does not start a new one.","type":"string"},"type":{"description":"The block's canonical type.","enum":["paragraph","heading","blockquote","bullet_list","ordered_list","task_list","callout","code","titled_code","mermaid","horizontal_rule","image","file","figma","metric","metric_grid","split_doc","split_doc_param_list"],"type":"string"},"uid":{"description":"Leave unset. Uids are generated server-side and are the handle later edits use.","type":"string"}},"required":["type"],"type":"object"},"block_uid":{"description":"The uid of the block being replaced.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid","block"],"type":"object"},"name":"replace_block"},"search_documents":{"desc":"Full-text search across every branch of every document in the organisation for blocks whose text matches the query. Returns hits with document_id, document_name, branch_id, branch_name, default (whether that branch is the document's default), block_uid and text; branch_id is what a follow-up get_document or read_block takes. A block a fork copied unchanged is found on every branch that holds it. Use it to find where a topic is discussed or which documents the user might mean; use list_documents when you know the title. A hit names the innermost block holding the text, which may sit below anything get_document lists; read_block resolves it.","has_params_one_of":true,"json_schema":{"properties":{"limit":{"description":"Maximum number of hits to return. Defaults to 20; cap is 50.","type":"integer"},"query":{"description":"The full-text search query (typo-tolerant, matches block text).","type":"string"}},"required":["query"],"type":"object"},"name":"search_documents"},"update_block_attrs":{"desc":"Set or override named attributes on an existing block, such as a heading's level or a callout's icon. Attributes not mentioned are kept and uid cannot change. Values are validated for the block's type, so a level outside 1 to 3 or a metric width other than compact, standard or wide is rejected. Use replace_block when the type itself has to change. Returns the summary rows of the block and everything nested in it, attrs as they now stand.","has_params_one_of":true,"json_schema":{"properties":{"attrs":{"description":"Attribute keys and values to set (e.g. {\"level\": 2}, {\"icon\": \"lucide:warning\"}).","type":"object"},"block_uid":{"description":"The uid of the block whose attrs should be updated.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"}},"required":["document_id","branch_id","block_uid","attrs"],"type":"object"},"name":"update_block_attrs"},"update_block_text":{"desc":"Replace the inline text of one text-bearing block: paragraph, heading, blockquote, code, titled_code, mermaid, or a callout written with text. Type, attrs and uid are kept, so this is the tool for wording changes. Text follows the canonical markdown subset (**bold**, *italic*, _underline_, ~~strike~~, backtick code, [label](url)), and is raw in code, titled_code and mermaid. One block is one paragraph; to add a paragraph, insert a block instead. Returns the block's summary row with the new text.","has_params_one_of":true,"json_schema":{"properties":{"block_uid":{"description":"The uid of the block whose text should be replaced.","type":"string"},"branch_id":{"description":"The id of the branch to read or write: a document's default_branch_id from list_documents, a search hit's branch_id, or any id from the branches get_document lists. A protected branch can be read but refuses every write.","type":"string"},"document_id":{"description":"The target document id.","type":"string"},"text":{"description":"New inline text in canonical markdown.","type":"string"}},"required":["document_id","branch_id","block_uid","text"],"type":"object"},"name":"update_block_text"},"update_document":{"desc":"Change a document's name, icon or place in the tree, any combination in one call; content is untouched. Give only the fields to change: name is the new display name, icon an Iconify identifier as \"collection:name\" (the product's own icons are MingCute fills, e.g. \"mingcute:rocket-fill\", so prefer one to match the sidebar), and parent_id the new parent, or an empty string for the organisation root. A call with none of the three is refused, and a parent that is the document itself or one of its descendants fails. Returns {document_id} with the fields that changed.","has_params_one_of":true,"json_schema":{"properties":{"document_id":{"description":"The document id.","type":"string"},"icon":{"description":"Optional. The new icon identifier; omit to keep the current one.","type":"string"},"name":{"description":"Optional. The new display name; omit to keep the current one.","type":"string"},"parent_id":{"description":"Optional. The new parent document id, or an empty string to move the document to the organisation root; omit to leave it where it is.","type":"string"}},"required":["document_id"],"type":"object"},"name":"update_document"}} \ No newline at end of file diff --git a/server/core/internal/assistant/tools/util.go b/server/core/internal/assistant/tools/util.go index e0219d60..3352e479 100644 --- a/server/core/internal/assistant/tools/util.go +++ b/server/core/internal/assistant/tools/util.go @@ -91,6 +91,8 @@ func blockKindLabel(kind block.Type) string { return "a divider" case block.BlockImage: return "an image" + case block.BlockFile: + return "a file attachment" case block.BlockFigma: return "a figma embed" case block.BlockMetric: diff --git a/server/core/internal/assistant/tools/util_test.go b/server/core/internal/assistant/tools/util_test.go index 0c17bc55..2dfaddd4 100644 --- a/server/core/internal/assistant/tools/util_test.go +++ b/server/core/internal/assistant/tools/util_test.go @@ -45,6 +45,7 @@ func Test_blockKindLabel(t *testing.T) { "Mermaid": {Kind: "mermaid", Result: "a mermaid diagram"}, "Horizontal rule": {Kind: "horizontal_rule", Result: "a divider"}, "Image": {Kind: "image", Result: "an image"}, + "File": {Kind: "file", Result: "a file attachment"}, "Figma": {Kind: "figma", Result: "a figma embed"}, "Metric": {Kind: "metric", Result: "a metric"}, "Metric grid": {Kind: "metric_grid", Result: "a metric grid"}, diff --git a/server/core/internal/db/document_file.go b/server/core/internal/db/document_file.go index 01202cdd..f7209961 100644 --- a/server/core/internal/db/document_file.go +++ b/server/core/internal/db/document_file.go @@ -8,6 +8,7 @@ import ( "github.com/guregu/null/v5" "github.com/jmoiron/sqlx" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/rs/xid" ) @@ -15,7 +16,9 @@ import ( // Re-uploading into the same block reuses the file id, so the insert // refreshes the existing row instead of failing: created_at is bumped // to re-arm the retention grace period for what is a new object, and -// the unreferenced mark is cleared. +// the unreferenced mark is cleared. A row owned by another organization +// is left alone and reported as errutil.ErrNotFound, since the id is +// client-chosen and must not reassign a file across organizations. func (a *agent) InsertDocumentFile(ctx context.Context, f file.File) error { q, args := a.builder.Insert("document_files"). SetMap(map[string]any{ @@ -24,18 +27,35 @@ func (a *agent) InsertDocumentFile(ctx context.Context, f file.File) error { "storage_key": f.StorageKey, "fk_document_id": f.DocumentID, "fk_organization_id": f.OrganizationID, + "name": f.Name, + "size": f.Size, + "content_type": f.ContentType, "created_at": f.CreatedAt, "unreferenced_at": f.UnreferencedAt, }). Suffix("ON CONFLICT (id) DO UPDATE SET " + "location = excluded.location, storage_key = excluded.storage_key, " + "fk_document_id = excluded.fk_document_id, fk_organization_id = excluded.fk_organization_id, " + - "created_at = excluded.created_at, unreferenced_at = NULL"). + "name = excluded.name, size = excluded.size, content_type = excluded.content_type, " + + "created_at = excluded.created_at, unreferenced_at = NULL " + + "WHERE document_files.fk_organization_id = excluded.fk_organization_id"). MustSql() - _, err := a.sql.ExecContext(ctx, q, args...) + res, err := a.sql.ExecContext(ctx, q, args...) + if err != nil { + return err + } - return err + n, err := res.RowsAffected() + if err != nil { + return err + } + + if n == 0 { + return errutil.ErrNotFound + } + + return nil } // FetchDocumentFile retrieves a document file by its block ID from the database. @@ -161,6 +181,9 @@ func (a *agent) selectDocumentFile(b sq.SelectBuilder) sq.SelectBuilder { `document_files.storage_key AS "storage_key"`, `document_files.fk_document_id AS "fk_document_id"`, `document_files.fk_organization_id AS "fk_organization_id"`, + `document_files.name AS "name"`, + `document_files.size AS "size"`, + `document_files.content_type AS "content_type"`, `document_files.created_at AS "created_at"`, `document_files.unreferenced_at AS "unreferenced_at"`, ).From("document_files") diff --git a/server/core/internal/db/document_file_test.go b/server/core/internal/db/document_file_test.go index a09fd613..a6ce0947 100644 --- a/server/core/internal/db/document_file_test.go +++ b/server/core/internal/db/document_file_test.go @@ -11,6 +11,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/comment" "github.com/oxynote/oxynote/server/core/internal/document/file" + "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/oxynote/oxynote/server/core/pkg/timeutil" "github.com/rs/xid" @@ -27,10 +28,13 @@ func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *file.File) for i := range count { f := file.File{ - ID: "file-" + xid.New().String(), - Location: file.LocationDocument, - StorageKey: "organizations/org/documents/doc/files/file", - CreatedAt: now.Add(-time.Duration(i) * time.Second), + ID: "file-" + xid.New().String(), + Location: file.LocationDocument, + StorageKey: "organizations/org/documents/doc/files/file", + Name: "shot.png", + Size: 1024, + ContentType: "image/png", + CreatedAt: now.Add(-time.Duration(i) * time.Second), } if fn != nil { @@ -52,6 +56,9 @@ func prepDocumentFiles(t *testing.T, db *DB, count int, fn func(int, *file.File) "storage_key": f.StorageKey, "fk_document_id": f.DocumentID, "fk_organization_id": f.OrganizationID, + "name": f.Name, + "size": f.Size, + "content_type": f.ContentType, "created_at": f.CreatedAt, "unreferenced_at": f.UnreferencedAt, }).MustSql() @@ -77,14 +84,26 @@ func Test_agent_InsertDocumentFile(t *testing.T) { f.Location = file.LocationComment f.StorageKey = "organizations/org/documents/doc/files/replaced" + f.Name = "notes.zip" + f.Size = 2048 + f.ContentType = "application/zip" f.CreatedAt = timeutil.Now().Truncate(time.Second) f.UnreferencedAt = null.Time{} return tcase{File: f} }, + "Re-upload from another organization": func(t *testing.T, db *DB) tcase { + f := prepDocumentFiles(t, db, 1, nil)[0] + doc := prepDocuments(t, db, 1, nil)[0] + + f.DocumentID = null.ValueFrom(doc.ID) + f.OrganizationID = null.StringFrom(doc.OrganizationID) + + return tcase{File: f, Err: errutil.ErrNotFound} + }, "Missing document": func(_ *testing.T, _ *DB) tcase { return tcase{ - File: file.NewFile("file-1", file.LocationDocument, "key", xid.New(), "org-1"), + File: file.NewFile("file-1", file.LocationDocument, "key", xid.New(), "org-1", "shot.png", 1024, "image/png"), Err: assert.AnError, } }, @@ -98,6 +117,9 @@ func Test_agent_InsertDocumentFile(t *testing.T) { StorageKey: "organizations/org/documents/doc/files/file-1", DocumentID: null.ValueFrom(doc.ID), OrganizationID: null.StringFrom(doc.OrganizationID), + Name: "notes.pdf", + Size: 4096, + ContentType: "application/pdf", CreatedAt: timeutil.Now().Truncate(time.Second), }, } @@ -208,6 +230,22 @@ func Test_agent_CheckDocumentFileReferenced(t *testing.T) { return "file-ref", doc.ID, true }, + "Referenced by a file block in branch content": func(t *testing.T, db *DB) (string, xid.ID, bool) { + doc := prepDocuments(t, db, 1, func(_ int, doc *document.Document) { + doc.Content.Content = append(doc.Content.Content, document.Block{ + Type: document.BlockNodeFileBlock, + Attrs: document.Attributes{ + "uid": "file-ref", + "src": "/api/documents/" + doc.ID.String() + "/files/file-ref", + "name": "notes.zip", + "size": 2048, + "contentType": "application/zip", + }, + }) + })[0] + + return "file-ref", doc.ID, true + }, "Referenced by a history entry": func(t *testing.T, db *DB) (string, xid.ID, bool) { doc := prepDocuments(t, db, 1, nil)[0] doc.Content.Content[0].Attrs = document.Attributes{"uid": "file-ref"} diff --git a/server/core/internal/db/migrations/001_initial.sql b/server/core/internal/db/migrations/001_initial.sql index 2466e710..e9039d24 100644 --- a/server/core/internal/db/migrations/001_initial.sql +++ b/server/core/internal/db/migrations/001_initial.sql @@ -413,6 +413,9 @@ CREATE TABLE document_files ( storage_key TEXT NOT NULL, fk_document_id TEXT REFERENCES documents ON DELETE SET NULL, fk_organization_id TEXT REFERENCES organizations ON DELETE SET NULL, + name TEXT NOT NULL, + size BIGINT NOT NULL, + content_type TEXT NOT NULL, created_at TIMESTAMP NOT NULL, unreferenced_at TIMESTAMP ); diff --git a/server/core/internal/document/attribute_keys.go b/server/core/internal/document/attribute_keys.go index 27806ffb..8b83584d 100644 --- a/server/core/internal/document/attribute_keys.go +++ b/server/core/internal/document/attribute_keys.go @@ -34,6 +34,15 @@ const ( // AttrHeight is the rendered height of an embed. AttrHeight = "height" + // AttrName is the file name a file block shows. + AttrName = "name" + + // AttrSize is the size in bytes of a file block's file. + AttrSize = "size" + + // AttrContentType is the media type of a file block's file. + AttrContentType = "contentType" + // AttrInversed indicates that a split documentation macro renders its // sides the other way round. AttrInversed = "inversed" diff --git a/server/core/internal/document/document_test.go b/server/core/internal/document/document_test.go index 7f5d3237..7f525af6 100644 --- a/server/core/internal/document/document_test.go +++ b/server/core/internal/document/document_test.go @@ -1,7 +1,6 @@ package document import ( - "fmt" "testing" "time" @@ -360,7 +359,7 @@ func Test_Document_Duplicate(t *testing.T) { Type: BlockNodeImageBlock, Attrs: Attributes{ "uid": "img1", - "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, doc.ID, "img1"), + "src": "https://app.test/core" + FilePath(doc.ID, "img1", "shot.png"), }, } @@ -375,7 +374,7 @@ func Test_Document_Duplicate(t *testing.T) { // the duplicate refers to its own copy under its own document. assert.Equal( t, - "https://app.test/core"+fmt.Sprintf(FilePathFormat, dup.ID, newID), + "https://app.test/core"+FilePath(dup.ID, newID, "shot.png"), dup.Content.Content[0].Attrs["src"], ) }, diff --git a/server/core/internal/document/file/file.go b/server/core/internal/document/file/file.go index e0cad476..dc197edd 100644 --- a/server/core/internal/document/file/file.go +++ b/server/core/internal/document/file/file.go @@ -3,7 +3,9 @@ package file import ( "fmt" + "mime" "path" + "strings" "time" "github.com/guregu/null/v5" @@ -19,7 +21,9 @@ func Folder(organizationID string, documentID xid.ID) string { return fmt.Sprintf(_folderFormat, organizationID, documentID) } -// Key returns the storage key of the given document's file. +// Key returns the storage key of the given document's file. The key +// is the file id alone: a re-upload into the same block then overwrites +// its object in place, so no object is ever left without a row naming it. func Key(organizationID string, documentID xid.ID, id string) string { return path.Join(Folder(organizationID, documentID), id) } @@ -67,6 +71,18 @@ type File struct { // belongs to. Null when the organization has been deleted. OrganizationID null.String `json:"organizationId" db:"fk_organization_id"` + // Name is the file name the upload carried, which the file is served + // back under. + Name string `json:"name" db:"name"` + + // Size is the size of the stored object in bytes. + Size int64 `json:"size" db:"size"` + + // ContentType is the media type detected from the object's bytes at + // upload time. It is what the file is served as: a client's own claim + // about the type never reaches the row. + ContentType string `json:"contentType" db:"content_type"` + // CreatedAt is the timestamp when the file was created. CreatedAt time.Time `json:"createdAt" db:"created_at"` @@ -77,17 +93,63 @@ type File struct { } // NewFile creates a new file instance. -func NewFile(id string, loc Location, storageKey string, documentID xid.ID, organizationID string) File { +func NewFile( + id string, + loc Location, + storageKey string, + documentID xid.ID, + organizationID string, + name string, + size int64, + contentType string, +) File { return File{ ID: id, Location: loc, StorageKey: storageKey, DocumentID: null.ValueFrom(documentID), OrganizationID: null.StringFrom(organizationID), + Name: name, + Size: size, + ContentType: contentType, CreatedAt: timeutil.Now(), } } +// Viewable reports whether a browser may render the file inline. The +// set is a fixed allowlist of types a browser shows rather than runs: +// HTML, SVG, XML and scripts are deliberately absent, since served inline +// they would execute under the API's origin. +func Viewable(contentType string) bool { + mt, _, err := mime.ParseMediaType(contentType) + if err != nil { + return false + } + + switch mt { + case "application/pdf", "image/png", "image/jpeg", "image/gif", "image/webp", "text/plain": + return true + } + + return strings.HasPrefix(mt, "video/") || strings.HasPrefix(mt, "audio/") +} + +// Disposition returns the Content-Disposition header value the file is +// served with: inline for a viewable type, attachment for everything +// else, carrying the file's name either way. The name is formatted as a +// media type parameter, which quotes it and percent-encodes non-ASCII +// and control characters into an RFC 2231 filename*, so no name can +// break out of the header. +func (f File) Disposition() string { + disposition := "attachment" + + if Viewable(f.ContentType) { + disposition = "inline" + } + + return mime.FormatMediaType(disposition, map[string]string{"filename": f.Name}) +} + // Orphaned reports whether the file lost its owner, which happens when the // document or the organization is deleted by a path that cascades around // the file itself. diff --git a/server/core/internal/document/file/file_test.go b/server/core/internal/document/file/file_test.go index 90ba1970..ba76c4c4 100644 --- a/server/core/internal/document/file/file_test.go +++ b/server/core/internal/document/file/file_test.go @@ -27,17 +27,110 @@ func Test_NewFile(t *testing.T) { documentID := xid.New() - f := NewFile("file-1", LocationComment, "organizations/org-1/documents/doc-1/files/file-1", documentID, "org-1") + f := NewFile( + "file-1", + LocationComment, + "organizations/org-1/documents/doc-1/files/file-1", + documentID, + "org-1", + "notes.pdf", + 4096, + "application/pdf", + ) assert.Equal(t, "file-1", f.ID) assert.Equal(t, LocationComment, f.Location) assert.Equal(t, "organizations/org-1/documents/doc-1/files/file-1", f.StorageKey) assert.Equal(t, documentID, f.DocumentID.V) assert.Equal(t, "org-1", f.OrganizationID.String) + assert.Equal(t, "notes.pdf", f.Name) + assert.EqualValues(t, 4096, f.Size) + assert.Equal(t, "application/pdf", f.ContentType) assert.False(t, f.Orphaned()) assert.False(t, f.CreatedAt.IsZero()) } +func Test_Viewable(t *testing.T) { + cc := map[string]struct { + ContentType string + Result bool + }{ + "PDF": {ContentType: "application/pdf", Result: true}, + "PNG": {ContentType: "image/png", Result: true}, + "JPEG": {ContentType: "image/jpeg", Result: true}, + "GIF": {ContentType: "image/gif", Result: true}, + "WebP": {ContentType: "image/webp", Result: true}, + "Video": {ContentType: "video/mp4", Result: true}, + "Audio": {ContentType: "audio/mpeg", Result: true}, + "Plain text with a charset": {ContentType: "text/plain; charset=utf-8", Result: true}, + "HTML": {ContentType: "text/html; charset=utf-8"}, + "SVG": {ContentType: "image/svg+xml"}, + "XML": {ContentType: "text/xml; charset=utf-8"}, + "Script": {ContentType: "text/javascript"}, + "Zip": {ContentType: "application/zip"}, + "Octet stream": {ContentType: "application/octet-stream"}, + "Malformed": {ContentType: "not a media type"}, + "Empty": {ContentType: ""}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, Viewable(c.ContentType)) + }) + } +} + +func Test_File_Disposition(t *testing.T) { + cc := map[string]struct { + Name string + ContentType string + Result string + }{ + "Viewable type is served inline": { + Name: "report.pdf", + ContentType: "application/pdf", + Result: `inline; filename=report.pdf`, + }, + "Other type is served as an attachment": { + Name: "notes.zip", + ContentType: "application/zip", + Result: `attachment; filename=notes.zip`, + }, + "HTML is served as an attachment": { + Name: "evil.html", + ContentType: "text/html; charset=utf-8", + Result: `attachment; filename=evil.html`, + }, + "Quotes and separators are quoted": { + Name: `a "quoted"; name.txt`, + ContentType: "text/plain", + Result: `inline; filename="a \"quoted\"; name.txt"`, + }, + "Non-ASCII name is encoded": { + Name: "résumé.pdf", + ContentType: "application/pdf", + Result: `inline; filename*=utf-8''r%C3%A9sum%C3%A9.pdf`, + }, + "Control characters are encoded": { + Name: "a\r\nX-Injected: yes.txt", + ContentType: "text/plain", + Result: `inline; filename*=utf-8''a%0D%0AX-Injected%3A%20yes.txt`, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + f := File{Name: c.Name, ContentType: c.ContentType} + + assert.Equal(t, c.Result, f.Disposition()) + }) + } +} + func Test_Folder(t *testing.T) { t.Parallel() @@ -51,8 +144,6 @@ func Test_Folder(t *testing.T) { } func Test_Key(t *testing.T) { - t.Parallel() - documentID := xid.New() assert.Equal( @@ -65,13 +156,13 @@ func Test_Key(t *testing.T) { func Test_File_Orphaned(t *testing.T) { t.Parallel() - f := NewFile("file-1", LocationDocument, "key", xid.New(), "org-1") + f := NewFile("file-1", LocationDocument, "key", xid.New(), "org-1", "shot.png", 1024, "image/png") assert.False(t, f.Orphaned()) f.DocumentID = null.Value[xid.ID]{} assert.True(t, f.Orphaned()) - f = NewFile("file-1", LocationDocument, "key", xid.New(), "org-1") + f = NewFile("file-1", LocationDocument, "key", xid.New(), "org-1", "shot.png", 1024, "image/png") f.OrganizationID = null.String{} assert.True(t, f.Orphaned()) } diff --git a/server/core/internal/document/file/manager/0mock_db_test.go b/server/core/internal/document/file/manager/0mock_db_test.go index 30285e3b..de7b033f 100644 --- a/server/core/internal/document/file/manager/0mock_db_test.go +++ b/server/core/internal/document/file/manager/0mock_db_test.go @@ -104,11 +104,11 @@ type DBMock struct { At null.Time } } - lockCheckDocumentFileReferenced sync.RWMutex - lockDeleteDocumentFile sync.RWMutex + lockCheckDocumentFileReferenced sync.RWMutex + lockDeleteDocumentFile sync.RWMutex lockDeleteExpiredDocumentBranchHistoryEntries sync.RWMutex - lockFetchPaginatedDocumentFiles sync.RWMutex - lockUpdateDocumentFileUnreferencedAt sync.RWMutex + lockFetchPaginatedDocumentFiles sync.RWMutex + lockUpdateDocumentFileUnreferencedAt sync.RWMutex } // CheckDocumentFileReferenced calls CheckDocumentFileReferencedFunc. diff --git a/server/core/internal/document/file/manager/manager_test.go b/server/core/internal/document/file/manager/manager_test.go index 013d2c79..b4b086fe 100644 --- a/server/core/internal/document/file/manager/manager_test.go +++ b/server/core/internal/document/file/manager/manager_test.go @@ -23,7 +23,7 @@ func TestMain(m *testing.M) { // stubFile builds a file created long enough ago to be past the grace // period, which is what the interesting cases need. func stubFile() file.File { - f := file.NewFile("file-1", file.LocationDocument, "folder/file-1", xid.New(), "org-1") + f := file.NewFile("file-1", file.LocationDocument, "folder/file-1", xid.New(), "org-1", "shot.png", 1024, "image/png") f.CreatedAt = timeutil.Now().Add(-_fileRetentionDuration * 2) return f diff --git a/server/core/internal/document/node.go b/server/core/internal/document/node.go index dc0a2c28..08c480fd 100644 --- a/server/core/internal/document/node.go +++ b/server/core/internal/document/node.go @@ -19,6 +19,14 @@ import ( // router mounts the file routes on this very format. const FilePathFormat = "/api/documents/%s/files/%s" +// FilePath returns the URL path a document's file is served under: the +// file route with the id followed by a dash and the file name, so the +// address reads as the file it is. Every uid is a 21-character nanoid, +// which is what lets the server cut the id off the front again. +func FilePath(documentID xid.ID, id, name string) string { + return fmt.Sprintf(FilePathFormat, documentID, id+"-"+url.PathEscape(name)) +} + // RootBlock represents a block in a document. type RootBlock struct { // Type is the type of the root block (always BlockNodeText's @@ -229,11 +237,17 @@ func (b Block) Search(scope search.Scope) map[string]search.Block { maps.Insert(res, maps.All(content)) } - // a metric block has no text children; its title is an attribute. - if b.Type == BlockNodeMetricBlock { + // a metric block and a file block have no text children; the title + // and the file name are attributes. + switch b.Type { //nolint:exhaustive // the other types index their text children + case BlockNodeMetricBlock: if title, ok := b.Attrs[AttrTitle].(string); ok { text.WriteString(title) } + case BlockNodeFileBlock: + if name, ok := b.Attrs[AttrName].(string); ok { + text.WriteString(name) + } } if text.Len() != 0 { @@ -370,7 +384,7 @@ func (b Block) copyStripped(dc *duplication) Block { newAttrs[k] = v } - if dc != nil && b.Type == BlockNodeImageBlock { + if dc != nil && (b.Type == BlockNodeImageBlock || b.Type == BlockNodeFileBlock) { dc.rewriteFileRef(b.Attrs, newAttrs) } @@ -394,10 +408,10 @@ func (d *duplication) regenerateUID(old any) string { return uid } -// rewriteFileRef points a duplicated image block at the duplicate's own copy -// of the file and records the pair. Images served from anywhere but the -// source document's file route are left as they are: an externally hosted -// image has no object to copy. +// rewriteFileRef points a duplicated image or file block at the +// duplicate's own copy of the file and records the pair. Files served +// from anywhere but the source document's file route are left as they +// are: an externally hosted image has no object to copy. func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { oldID, ok := attrs[AttrUID].(string) if !ok { @@ -419,15 +433,23 @@ func (d *duplication) rewriteFileRef(attrs, newAttrs Attributes) { return } + // the id is followed by the dash and the file name. oldPath := fmt.Sprintf(FilePathFormat, d.oldDocumentID, oldID) - if !strings.HasSuffix(u.Path, oldPath) { + + idx := strings.Index(u.Path, oldPath) + if idx < 0 { + return + } + + rest := u.Path[idx+len(oldPath):] + if !strings.HasPrefix(rest, "-") { return } // only the path is swapped: the src was built by the frontend from its // own api base url, which need not match this server's public url. - u.Path = strings.TrimSuffix(u.Path, oldPath) + - fmt.Sprintf(FilePathFormat, d.newDocumentID, newID) + u.Path = u.Path[:idx] + fmt.Sprintf(FilePathFormat, d.newDocumentID, newID) + rest + u.RawPath = "" newAttrs[AttrSrc] = u.String() d.files[oldID] = newID diff --git a/server/core/internal/document/node_test.go b/server/core/internal/document/node_test.go index fb6197db..798f2c9c 100644 --- a/server/core/internal/document/node_test.go +++ b/server/core/internal/document/node_test.go @@ -2,7 +2,6 @@ package document import ( "encoding/json" - "fmt" "testing" "github.com/oxynote/oxynote/server/core/internal/search" @@ -310,6 +309,16 @@ func Test_RootBlock_Search(t *testing.T) { Type: BlockNodeMetricBlock, Attrs: Attributes{"uid": "m3"}, }, + // a file block's name is an attribute, not a text child. + { + Type: BlockNodeFileBlock, + Attrs: Attributes{"uid": "f1", "name": "quarterly-report.pdf", "size": 2048}, + }, + // a file block still uploading has no name to index. + { + Type: BlockNodeFileBlock, + Attrs: Attributes{"uid": "f2"}, + }, }, } @@ -327,6 +336,7 @@ func Test_RootBlock_Search(t *testing.T) { "li1": scope.Block("li1", "listItem", "nested"), "p2": scope.Block("p2", "paragraph", "plain bold tail"), "m1": scope.Block("m1", "metricBlock", "Pizza Fridays"), + "f1": scope.Block("f1", "fileBlock", "quarterly-report.pdf"), }, res) } @@ -353,6 +363,30 @@ func Test_RootBlock_StripCommentMarks(t *testing.T) { }, stripped) } +func Test_FilePath(t *testing.T) { + cc := map[string]struct { + ID string + Name string + Result string + }{ + "Plain name": {ID: "f1", Name: "notes.zip", Result: "/files/f1-notes.zip"}, + "Name with space": {ID: "f1", Name: "my notes.zip", Result: "/files/f1-my%20notes.zip"}, + "Name with dash": {ID: "f1", Name: "a-b.zip", Result: "/files/f1-a-b.zip"}, + "Name with slash": {ID: "f1", Name: "a/b.zip", Result: "/files/f1-a%2Fb.zip"}, + "Non-ASCII name": {ID: "f1", Name: "résumé.pdf", Result: "/files/f1-r%C3%A9sum%C3%A9.pdf"}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + documentID := xid.New() + + assert.Equal(t, "/api/documents/"+documentID.String()+c.Result, FilePath(documentID, c.ID, c.Name)) + }) + } +} + func Test_RootBlock_Duplicate(t *testing.T) { t.Parallel() @@ -403,7 +437,7 @@ func Test_RootBlock_Duplicate(t *testing.T) { Type: BlockNodeImageBlock, Attrs: Attributes{ "uid": "img1", - "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, oldDocumentID, "img1"), + "src": "https://app.test/core" + FilePath(oldDocumentID, "img1", "shot.png"), }, }, { @@ -417,18 +451,29 @@ func Test_RootBlock_Duplicate(t *testing.T) { Type: BlockNodeImageBlock, Attrs: Attributes{ "uid": "img3", - "src": "https://app.test/core" + fmt.Sprintf(FilePathFormat, xid.New(), "img3"), + "src": "https://app.test/core" + FilePath(xid.New(), "img3", "shot.png"), }, }, { Type: BlockNodeImageBlock, Attrs: Attributes{"uid": "img4"}, }, + { + Type: BlockNodeFileBlock, + Attrs: Attributes{ + "uid": "file1", + "src": "https://app.test/core" + FilePath(oldDocumentID, "file1", "my notes.zip"), + "name": "notes.zip", + "size": 2048, + "contentType": "application/zip", + }, + }, }, }, Check: func(t *testing.T, orig, dup RootBlock, files map[string]string) { - // only the image served by the source document is remapped. - require.Len(t, files, 1) + // only the image and the file served by the source + // document are remapped. + require.Len(t, files, 2) newID, ok := files["img1"] require.True(t, ok) @@ -438,7 +483,7 @@ func Test_RootBlock_Duplicate(t *testing.T) { assert.Equal(t, newID, uid) assert.Equal( t, - "https://app.test/core"+fmt.Sprintf(FilePathFormat, newDocumentID, newID), + "https://app.test/core"+FilePath(newDocumentID, newID, "shot.png"), dup.Content[0].Attrs["src"], "the host and path prefix must survive the rewrite", ) @@ -449,8 +494,27 @@ func Test_RootBlock_Duplicate(t *testing.T) { assert.Equal(t, orig.Content[2].Attrs["src"], dup.Content[2].Attrs["src"]) assert.NotContains(t, dup.Content[3].Attrs, "src") + // the file block is remapped the way the image is, keeps + // the file name segment of its address, and keeps what + // it shows. + newFileID, ok := files["file1"] + require.True(t, ok) + + fileUID, ok := dup.Content[4].UID() + require.True(t, ok) + assert.Equal(t, newFileID, fileUID) + assert.Equal( + t, + "https://app.test/core"+FilePath(newDocumentID, newFileID, "my notes.zip"), + dup.Content[4].Attrs["src"], + ) + assert.Equal(t, "notes.zip", dup.Content[4].Attrs["name"]) + assert.Equal(t, 2048, dup.Content[4].Attrs["size"]) + assert.Equal(t, "application/zip", dup.Content[4].Attrs["contentType"]) + // the original is untouched. assert.Equal(t, "img1", orig.Content[0].Attrs["uid"]) + assert.Equal(t, "file1", orig.Content[4].Attrs["uid"]) }, } }(), diff --git a/server/core/internal/document/node_types.go b/server/core/internal/document/node_types.go index ffbf0a9c..88bf5e89 100644 --- a/server/core/internal/document/node_types.go +++ b/server/core/internal/document/node_types.go @@ -75,6 +75,10 @@ const ( // src/alt/title/width. BlockNodeImageBlock BlockNodeType = "imageBlock" + // BlockNodeFileBlock is a file attachment atom carrying + // src/name/size/contentType. + BlockNodeFileBlock BlockNodeType = "fileBlock" + // BlockNodeFigmaBlock embeds a Figma frame via // src/width/height. BlockNodeFigmaBlock BlockNodeType = "figmaBlock" diff --git a/server/core/internal/server/0mock_storer_test.go b/server/core/internal/server/0mock_storer_test.go index 51447d1f..db068e80 100644 --- a/server/core/internal/server/0mock_storer_test.go +++ b/server/core/internal/server/0mock_storer_test.go @@ -5,7 +5,6 @@ package server import ( "context" - "io" "sync" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -30,7 +29,7 @@ var _ Storer = &StorerMock{} // RetrieveFunc: func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { // panic("mock out the Retrieve method") // }, -// UploadFunc: func(ctx context.Context, folder string, id string, r io.Reader) error { +// UploadFunc: func(ctx context.Context, folder string, id string, data []byte, contentType string) error { // panic("mock out the Upload method") // }, // } @@ -50,7 +49,7 @@ type StorerMock struct { RetrieveFunc func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) // UploadFunc mocks the Upload method. - UploadFunc func(ctx context.Context, folder string, id string, r io.Reader) error + UploadFunc func(ctx context.Context, folder string, id string, data []byte, contentType string) error // calls tracks calls to the methods. calls struct { @@ -93,8 +92,10 @@ type StorerMock struct { Folder string // ID is the id argument value. ID string - // R is the r argument value. - R io.Reader + // Data is the data argument value. + Data []byte + // ContentType is the contentType argument value. + ContentType string } } lockCopy sync.RWMutex @@ -243,17 +244,19 @@ func (mock *StorerMock) RetrieveCalls() []struct { } // Upload calls UploadFunc. -func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r io.Reader) error { +func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, data []byte, contentType string) error { callInfo := struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string }{ - Ctx: ctx, - Folder: folder, - ID: id, - R: r, + Ctx: ctx, + Folder: folder, + ID: id, + Data: data, + ContentType: contentType, } mock.lockUpload.Lock() mock.calls.Upload = append(mock.calls.Upload, callInfo) @@ -264,7 +267,7 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r ) return errOut } - return mock.UploadFunc(ctx, folder, id, r) + return mock.UploadFunc(ctx, folder, id, data, contentType) } // UploadCalls gets all the calls that were made to Upload. @@ -272,16 +275,18 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r // // len(mockedStorer.UploadCalls()) func (mock *StorerMock) UploadCalls() []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } { var calls []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } mock.lockUpload.RLock() calls = mock.calls.Upload diff --git a/server/core/internal/server/internal/document/document_http.go b/server/core/internal/server/internal/document/document_http.go index ecb53e9f..065c556f 100644 --- a/server/core/internal/server/internal/document/document_http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -6,6 +6,7 @@ import ( "log/slog" "net/http" "net/url" + "path" "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/github" @@ -752,7 +753,7 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { uids, ) - h.copyDocumentFiles(r.Context(), files, id, duplDoc.ID, session.ActiveOrganizationID) + h.copyDocumentFiles(r.Context(), files, duplDoc.ID, session.ActiveOrganizationID) if h.tree.changeCallback != nil { h.tree.changeCallback(session.ActiveOrganizationID, duplDoc.ParentID) @@ -778,7 +779,7 @@ func (h *Handler) DuplicateDocument(w http.ResponseWriter, r *http.Request) { func (h *Handler) copyDocumentFiles( ctx context.Context, files map[string]string, - fromDocumentID, toDocumentID xid.ID, + toDocumentID xid.ID, organizationID string, ) { for oldID, newID := range files { @@ -796,15 +797,18 @@ func (h *Handler) copyDocumentFiles( continue } - fromFolder := file.Folder(organizationID, fromDocumentID) - toFolder := file.Folder(organizationID, toDocumentID) + fromFolder, fromKey := path.Split(f.StorageKey) + toFolder, toKey := path.Split(file.Key(organizationID, toDocumentID, newID)) err = h.db.InsertDocumentFile(ctx, file.NewFile( newID, f.Location, - file.Key(organizationID, toDocumentID, newID), + path.Join(toFolder, toKey), toDocumentID, organizationID, + f.Name, + f.Size, + f.ContentType, )) if err != nil { logutil.Critical(h.log, err).Error( @@ -815,7 +819,7 @@ func (h *Handler) copyDocumentFiles( continue } - err = h.storer.Copy(ctx, fromFolder, oldID, toFolder, newID) + err = h.storer.Copy(ctx, fromFolder, fromKey, toFolder, toKey) if err != nil { logutil.Critical(h.log, err).Error( "cannot copy duplicated document file", diff --git a/server/core/internal/server/internal/document/document_http_test.go b/server/core/internal/server/internal/document/document_http_test.go index bf980d6e..064e4076 100644 --- a/server/core/internal/server/internal/document/document_http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "log/slog" "net/http" "net/http/httptest" @@ -1559,7 +1558,7 @@ func Test_Handler_DuplicateDocument(t *testing.T) { Type: documentCore.BlockNodeImageBlock, Attrs: documentCore.Attributes{ "uid": "img1", - "src": "https://app.test/core" + fmt.Sprintf(documentCore.FilePathFormat, _documentID, "img1"), + "src": "https://app.test/core" + documentCore.FilePath(_documentID, "img1", "shot.png"), }, }, }, @@ -1574,7 +1573,7 @@ func Test_Handler_DuplicateDocument(t *testing.T) { return &DBMock{ FetchDocumentFunc: fetchStoredWithImage, FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { - f := file.NewFile(id, file.LocationDocument, "key", _documentID, organizationID) + f := file.NewFile(id, file.LocationDocument, "key", _documentID, organizationID, "shot.png", 1024, "image/png") return &f, nil }, @@ -1781,7 +1780,7 @@ func Test_Handler_copyDocumentFiles(t *testing.T) { fileDB := func() *DBMock { return &DBMock{ FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { - f := file.NewFile(id, file.LocationComment, "key", _documentID, organizationID) + f := file.NewFile(id, file.LocationComment, file.Key(organizationID, _documentID, id), _documentID, organizationID, "shot.png", 1024, "image/png") return &f, nil }, @@ -1849,7 +1848,7 @@ func Test_Handler_copyDocumentFiles(t *testing.T) { hdl, _ := newTestHandler(c.DB, &fakePublisher{}) hdl.storer = c.Storer - hdl.copyDocumentFiles(context.Background(), c.Files, _documentID, toDocumentID, "org1") + hdl.copyDocumentFiles(context.Background(), c.Files, toDocumentID, "org1") ff := c.DB.InsertDocumentFileCalls() require.Len(t, ff, c.Inserts) @@ -1871,9 +1870,11 @@ func Test_Handler_copyDocumentFiles(t *testing.T) { return } - assert.Equal(t, file.Folder("org1", _documentID), cf[0].SrcFolder) + // the copy goes from the source row's key to the new row's, so + // both carry the extension. + 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, file.Folder("org1", toDocumentID)+"/", cf[0].DstFolder) assert.Equal(t, "new-1", cf[0].DstID) }) } diff --git a/server/core/internal/server/internal/document/files/0mock_storer_test.go b/server/core/internal/server/internal/document/files/0mock_storer_test.go index d1379b50..aff4c82f 100644 --- a/server/core/internal/server/internal/document/files/0mock_storer_test.go +++ b/server/core/internal/server/internal/document/files/0mock_storer_test.go @@ -5,7 +5,6 @@ package files import ( "context" - "io" "sync" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -24,7 +23,7 @@ var _ Storer = &StorerMock{} // RetrieveFunc: func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { // panic("mock out the Retrieve method") // }, -// UploadFunc: func(ctx context.Context, folder string, id string, r io.Reader) error { +// UploadFunc: func(ctx context.Context, folder string, id string, data []byte, contentType string) error { // panic("mock out the Upload method") // }, // } @@ -38,7 +37,7 @@ type StorerMock struct { RetrieveFunc func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) // UploadFunc mocks the Upload method. - UploadFunc func(ctx context.Context, folder string, id string, r io.Reader) error + UploadFunc func(ctx context.Context, folder string, id string, data []byte, contentType string) error // calls tracks calls to the methods. calls struct { @@ -59,8 +58,10 @@ type StorerMock struct { Folder string // ID is the id argument value. ID string - // R is the r argument value. - R io.Reader + // Data is the data argument value. + Data []byte + // ContentType is the contentType argument value. + ContentType string } } lockRetrieve sync.RWMutex @@ -113,17 +114,19 @@ func (mock *StorerMock) RetrieveCalls() []struct { } // Upload calls UploadFunc. -func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r io.Reader) error { +func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, data []byte, contentType string) error { callInfo := struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string }{ - Ctx: ctx, - Folder: folder, - ID: id, - R: r, + Ctx: ctx, + Folder: folder, + ID: id, + Data: data, + ContentType: contentType, } mock.lockUpload.Lock() mock.calls.Upload = append(mock.calls.Upload, callInfo) @@ -134,7 +137,7 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r ) return errOut } - return mock.UploadFunc(ctx, folder, id, r) + return mock.UploadFunc(ctx, folder, id, data, contentType) } // UploadCalls gets all the calls that were made to Upload. @@ -142,16 +145,18 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r // // len(mockedStorer.UploadCalls()) func (mock *StorerMock) UploadCalls() []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } { var calls []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } mock.lockUpload.RLock() calls = mock.calls.Upload diff --git a/server/core/internal/server/internal/document/files/http.go b/server/core/internal/server/internal/document/files/http.go index 1592bc33..620d8e91 100644 --- a/server/core/internal/server/internal/document/files/http.go +++ b/server/core/internal/server/internal/document/files/http.go @@ -3,12 +3,14 @@ package files import ( "context" - "fmt" - "io" "log/slog" "net/http" + "path" "regexp" + "strings" + "unicode/utf8" + "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/document/file" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -17,32 +19,44 @@ import ( "github.com/rs/xid" ) -// _fileIDPattern matches the block-uid shapes clients mint for file ids -// (21-char nanoids or hyphenated UUIDs). The charset shuts out the path +// _fileIDLength is the length of a block uid: every editor mints +// 21-character nanoids, and the retrieval route relies on it to cut the +// id off the front of "-". +const _fileIDLength = 21 + +// _fileIDPattern matches a block uid. The charset shuts out the path // separators and dot segments that would otherwise let an id escape its // storage folder once joined into an object key. -var _fileIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) +var _fileIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{21}$`) + +// _maxFileNameLength caps the name recorded for an upload, in runes. +const _maxFileNameLength = 255 + +// _fallbackFileName names an upload whose multipart part carried no +// usable name. +const _fallbackFileName = "file" // Handler holds dependencies required for document file operations. type Handler struct { - log *slog.Logger - db DB - storer Storer - fileLocationFormat string + log *slog.Logger + db DB + storer Storer + publicURL string } -// NewHandler creates a new handler instance with the provided logger and database. +// NewHandler creates a new handler instance with the provided logger and +// database. publicURL is the origin the Location of an upload is built on. func NewHandler( log *slog.Logger, db DB, storer Storer, - fileLocationFormat string, + publicURL string, ) *Handler { return &Handler{ - log: log, - db: db, - storer: storer, - fileLocationFormat: fileLocationFormat, + log: log, + db: db, + storer: storer, + publicURL: publicURL, } } @@ -78,34 +92,52 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { return } - body, err := httpserver.FormFile(w, r, "file", storage.MaxUploadBytes, storage.ErrSizeLimitExceeded) + policy, ok := uploadPolicy(r.URL.Query().Get("kind")) + if !ok { + httpserver.RespondError(h.log, w, httpserver.ErrInvalidForm) + return + } + + body, err := httpserver.FormFile(w, r, "file", policy.MaxUploadBytes(), storage.ErrSizeLimitExceeded) if err != nil { httpserver.RespondError(h.log, w, err) return } defer body.Close() //nolint:errcheck // error provides no meaningful info - folder := file.Folder(session.ActiveOrganizationID, documentID) - - // the row is written before the object so that a crash can only ever - // leave a row without an object, which the file manager reaps. An - // object without a row would be invisible to every cleanup path. - err = h.db.InsertDocumentFile( - r.Context(), - file.NewFile( - fileID, - location, - file.Key(session.ActiveOrganizationID, documentID, fileID), - documentID, - session.ActiveOrganizationID, - ), - ) + // the object is read and checked before the row is written, so a + // rejected upload never leaves a row behind. + data, contentType, err := storage.ReadObject(body, policy) if err != nil { httpserver.RespondError(h.log, w, err) return } - err = h.storer.Upload(r.Context(), folder, fileID, body) + name := fileName(r.MultipartForm.File["file"][0].Filename) + + f := file.NewFile( + fileID, + location, + file.Key(session.ActiveOrganizationID, documentID, fileID), + documentID, + session.ActiveOrganizationID, + name, + int64(len(data)), + contentType, + ) + + folder, key := path.Split(f.StorageKey) + + // the row is written before the object so that a crash can only ever + // leave a row without an object, which the file manager reaps. An + // object without a row would be invisible to every cleanup path. + err = h.db.InsertDocumentFile(r.Context(), f) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + err = h.storer.Upload(r.Context(), folder, key, data, contentType) if err != nil { if derr := h.db.DeleteDocumentFile(r.Context(), fileID); derr != nil { h.log.Error("deleting file row after upload failure", "error", derr.Error()) @@ -119,15 +151,78 @@ func (h *Handler) UploadDocumentFile(w http.ResponseWriter, r *http.Request) { httpserver.Respond( h.log, w, - nil, + struct { + ID string `json:"id"` + Name string `json:"name"` + Size int64 `json:"size"` + ContentType string `json:"contentType"` + }{ + ID: f.ID, + Name: f.Name, + Size: f.Size, + ContentType: f.ContentType, + }, http.StatusCreated, httpserver.LocationHeader( - fmt.Sprintf(h.fileLocationFormat, documentID, fileID), + h.publicURL+document.FilePath(documentID, fileID, f.Name), ), ) } +// fileID cuts the id off a "-" route segment, reporting +// whether the segment has that shape. +func fileID(ref string) (string, bool) { + if len(ref) <= _fileIDLength || ref[_fileIDLength] != '-' { + return "", false + } + + id := ref[:_fileIDLength] + if !_fileIDPattern.MatchString(id) { + return "", false + } + + return id, true +} + +// uploadPolicy maps the kind of block an upload belongs to onto the +// storage policy it is admitted under, reporting whether the kind is one +// at all. +func uploadPolicy(kind string) (storage.Policy, bool) { + switch kind { + case "image": + return storage.ImagePolicy, true + case "file": + return storage.FilePolicy, true + default: + return storage.Policy{}, false + } +} + +// fileName reduces the name a multipart part carried to a bare file name: +// a browser may send a path, Windows ones with backslashes, and the part +// may carry no name at all. +func fileName(raw string) string { + name := path.Base(strings.ReplaceAll(raw, "\\", "/")) + name = strings.TrimSpace(name) + + if name == "." || name == "/" { + name = "" + } + + if utf8.RuneCountInString(name) > _maxFileNameLength { + name = string([]rune(name)[:_maxFileNameLength]) + } + + if name == "" { + return _fallbackFileName + } + + return name +} + // RetrieveDocumentFile handles the retrieval of a file from a document. +// The route's last segment is "-": only the id identifies +// the file, the name makes the address read as the file it is. func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { session, ok := auth.RequireSession(h.log, w, r) if !ok { @@ -140,12 +235,18 @@ func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { return } - id, err := httpserver.ExtractParam(r, "id") + ref, err := httpserver.ExtractParam(r, "ref") if err != nil { httpserver.RespondError(h.log, w, err) return } + id, ok := fileID(ref) + if !ok { + httpserver.RespondError(h.log, w, errutil.ErrNotFound) + return + } + // Verify file exists and belongs to the organization. f, err := h.db.FetchDocumentFile(r.Context(), id, session.ActiveOrganizationID) if err != nil { @@ -161,9 +262,9 @@ func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { return } - folder := file.Folder(session.ActiveOrganizationID, documentID) + folder, key := path.Split(f.StorageKey) - obj, found, err := h.storer.Retrieve(r.Context(), folder, id) + obj, found, err := h.storer.Retrieve(r.Context(), folder, key) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -176,12 +277,20 @@ func (h *Handler) RetrieveDocumentFile(w http.ResponseWriter, r *http.Request) { defer obj.Body.Close() //nolint:errcheck // error provides no meaningful info + // the row's disposition decides whether a browser renders the file, + // and nosniff stops it second-guessing the type the row recorded. + w.Header().Set("Content-Disposition", f.Disposition()) + w.Header().Set("X-Content-Type-Options", "nosniff") + + // the row carries the type detected at upload; the store's own value + // is a re-sniff on the file system backend, which cannot tell a + // document from a zip. httpserver.ServeObject( h.log, w, r, obj.ETag, - obj.ContentType, + f.ContentType, obj.Body, ) } @@ -212,8 +321,9 @@ type DBAgent interface { // //go:generate ../../../../../scripts/codegen/mock -t internal Storer type Storer interface { - // Upload uploads a new object. - Upload(ctx context.Context, folder, id string, r io.Reader) error + // Upload should store the object's bytes under the given content + // type. + Upload(ctx context.Context, folder, id string, data []byte, contentType string) error // Retrieve retrieves an object by its ID. Retrieve(ctx context.Context, folder, id string) (*storage.ObjectInfo, bool, error) diff --git a/server/core/internal/server/internal/document/files/http_test.go b/server/core/internal/server/internal/document/files/http_test.go index 157e4008..6c3adb68 100644 --- a/server/core/internal/server/internal/document/files/http_test.go +++ b/server/core/internal/server/internal/document/files/http_test.go @@ -9,6 +9,7 @@ import ( "mime/multipart" "net/http" "net/http/httptest" + "slices" "strings" "testing" @@ -37,18 +38,32 @@ func addSession(ctx context.Context) context.Context { }) } -// multipartBody builds a multipart form body carrying a single file field. -func multipartBody(t *testing.T, field, content string) (io.Reader, string) { +// _testPNG is a data prefix carrying the PNG magic bytes so content +// type sniffing detects image/png. +var _testPNG = append([]byte("\x89PNG\r\n\x1a\n"), bytes.Repeat([]byte{1}, 1024)...) + +// multipartBody builds a multipart form body carrying a single file field +// under the given file name; an empty name sends "file.png" and nil +// content sends a PNG. +func multipartBody(t *testing.T, field, name string, content []byte) (io.Reader, string) { t.Helper() + if name == "" { + name = "file.png" + } + + if content == nil { + content = _testPNG + } + var buf bytes.Buffer mw := multipart.NewWriter(&buf) - fw, err := mw.CreateFormFile(field, "file.png") + fw, err := mw.CreateFormFile(field, name) require.NoError(t, err) - _, err = fw.Write([]byte(content)) + _, err = fw.Write(content) require.NoError(t, err) require.NoError(t, mw.Close()) @@ -61,12 +76,12 @@ func Test_NewHandler(t *testing.T) { db := &DBMock{} storer := &StorerMock{} - hdl := NewHandler(slog.New(slog.DiscardHandler), db, storer, "loc/%s/%s") + hdl := NewHandler(slog.New(slog.DiscardHandler), db, storer, "https://app.test/core") require.NotNil(t, hdl) assert.NotNil(t, hdl.log) assert.Same(t, db, hdl.db) assert.Same(t, storer, hdl.storer) - assert.Equal(t, "loc/%s/%s", hdl.fileLocationFormat) + assert.Equal(t, "https://app.test/core", hdl.publicURL) } func Test_Handler_UploadDocumentFile(t *testing.T) { @@ -96,8 +111,18 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { return } - assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files", ff[0].Folder) - assert.Equal(t, "f1", ff[0].ID) + assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files/", ff[0].Folder) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].ID) + } + } + + wasUploadCalledWith := func(data []byte, contentType string) check { + return func(t *testing.T, _ *DBMock, storer *StorerMock, _ *httptest.ResponseRecorder) { + ff := storer.UploadCalls() + require.Len(t, ff, 1) + + assert.Equal(t, data, ff[0].Data) + assert.Equal(t, contentType, ff[0].ContentType) } } @@ -110,14 +135,25 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { return } - assert.Equal(t, "f1", ff[0].F.ID) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].F.ID) assert.Equal(t, file.LocationDocument, ff[0].F.Location) - assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files/f1", ff[0].F.StorageKey) assert.Equal(t, _documentID, ff[0].F.DocumentID.V) assert.Equal(t, "org1", ff[0].F.OrganizationID.String) } } + wasInsertCalledWith := func(name string, size int64, contentType string) check { + return func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) { + ff := db.InsertDocumentFileCalls() + require.Len(t, ff, 1) + + assert.Equal(t, file.Key("org1", _documentID, "f1xxxxxxxxxxxxxxxxxxx"), ff[0].F.StorageKey) + assert.Equal(t, name, ff[0].F.Name) + assert.Equal(t, size, ff[0].F.Size) + assert.Equal(t, contentType, ff[0].F.ContentType) + } + } + wasDeleteCalled := func(count int) check { return func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) { ff := db.DeleteDocumentFileCalls() @@ -127,7 +163,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { return } - assert.Equal(t, "f1", ff[0].ID) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].ID) } } @@ -137,14 +173,19 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { NoSession bool OmitID bool NoFile bool - Query string - Checks []check + // FileName is the multipart part's file name; empty sends + // "file.png". + FileName string + // Content is the multipart file's bytes; nil sends a PNG. + Content []byte + Query string + Checks []check }{ "No session in context": { DB: &DBMock{}, Storer: &StorerMock{}, NoSession: true, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusUnauthorized, `{"code":"account.not_authenticated","message":"not authenticated"}`), wasUploadCalled(0), @@ -154,7 +195,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { DB: &DBMock{}, Storer: &StorerMock{}, OmitID: true, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusNotFound, `{"code":"general","message":"not found"}`), wasUploadCalled(0), @@ -167,7 +208,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { }, }, Storer: &StorerMock{}, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), wasUploadCalled(0), @@ -176,7 +217,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { "Missing file ID query parameter": { DB: &DBMock{}, Storer: &StorerMock{}, - Query: "?location=document", + Query: "?location=document&kind=image", Checks: checks( hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), wasUploadCalled(0), @@ -188,7 +229,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { "File ID with path traversal": { DB: &DBMock{}, Storer: &StorerMock{}, - Query: "?id=../../../org2/documents/d2/files/f9&location=document", + Query: "?id=../../../org2/documents/d2/files/f9&location=document&kind=image", Checks: checks( hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), wasInsertCalled(0), @@ -198,22 +239,89 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { "Invalid location query parameter": { DB: &DBMock{}, Storer: &StorerMock{}, - Query: "?id=f1&location=nowhere", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=nowhere&kind=image", Checks: checks( hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), wasUploadCalled(0), ), }, + "Missing kind query parameter": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, + "Invalid kind query parameter": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=video", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, + // every editor mints 21-character nanoids, and the retrieval route + // cuts the id off the front of the segment by that length. + "File ID of the wrong length": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Query: "?id=short&location=document&kind=image", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, "Missing multipart file field": { DB: &DBMock{}, Storer: &StorerMock{}, NoFile: true, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusBadRequest, `{"code":"request.invalid_form","message":"invalid form data"}`), wasUploadCalled(0), ), }, + // the image kind keeps the image-only rule, and a rejected body + // never reaches the row. + "Image kind rejects a non-image": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: []byte("plain text data"), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"storage.invalid_content_type","message":"invalid content type"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, + "Image kind rejects a body over the image size limit": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: append(slices.Clone(_testPNG), bytes.Repeat([]byte{1}, int(storage.ImagePolicy.MaxSize))...), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"storage.size_limit_exceeded","message":"file size exceeds limit"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, + "File kind rejects a body over the file size limit": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: bytes.Repeat([]byte{'x'}, int(storage.FilePolicy.MaxSize)+1), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"storage.size_limit_exceeded","message":"file size exceeds limit"}`), + wasInsertCalled(0), + wasUploadCalled(0), + ), + }, "DB insert error": { DB: &DBMock{ InsertDocumentFileFunc: func(context.Context, file.File) error { @@ -221,7 +329,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { }, }, Storer: &StorerMock{}, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), wasInsertCalled(1), @@ -232,11 +340,11 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { "Storer upload error rolls the row back": { DB: &DBMock{}, Storer: &StorerMock{ - UploadFunc: func(context.Context, string, string, io.Reader) error { + UploadFunc: func(context.Context, string, string, []byte, string) error { return errors.New("boom") }, }, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), wasInsertCalled(1), @@ -251,27 +359,33 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { }, }, Storer: &StorerMock{ - UploadFunc: func(context.Context, string, string, io.Reader) error { + UploadFunc: func(context.Context, string, string, []byte, string) error { return errors.New("boom") }, }, - Query: "?id=f1&location=document", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=image", Checks: checks( hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), wasDeleteCalled(1), ), }, - "Successful upload": { + "Successful image upload": { DB: &DBMock{}, Storer: &StorerMock{}, - Query: "?id=f1&location=comment", + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=comment&kind=image", Checks: checks( func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { assert.Equal(t, http.StatusCreated, rec.Code) - assert.Zero(t, rec.Body.Len(), rec.Body.String()) - assert.Equal(t, "loc/"+_documentID.String()+"/f1", rec.Header().Get("Location")) + assert.JSONEq( + t, + `{"id":"f1xxxxxxxxxxxxxxxxxxx","name":"file.png","size":1032,"contentType":"image/png"}`, + rec.Body.String(), + ) + assert.Equal(t, "https://app.test/core/api/documents/"+_documentID.String()+"/files/f1xxxxxxxxxxxxxxxxxxx-file.png", rec.Header().Get("Location")) }, wasUploadCalled(1), + wasUploadCalledWith(_testPNG, "image/png"), + wasInsertCalledWith("file.png", 1032, "image/png"), func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) { ff := db.InsertDocumentFileCalls() require.Len(t, ff, 1) @@ -280,6 +394,90 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { wasDeleteCalled(0), ), }, + // the type is what the bytes say, whatever the part's name + // claims, and the file kind takes any of them. + "Successful file upload records the name, size and detected type": { + DB: &DBMock{}, + Storer: &StorerMock{}, + FileName: "notes.zip", + Content: []byte("plain text data"), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusCreated, rec.Code) + assert.JSONEq( + t, + `{"id":"f1xxxxxxxxxxxxxxxxxxx","name":"notes.zip","size":15,"contentType":"text/plain; charset=utf-8"}`, + rec.Body.String(), + ) + assert.Equal(t, "https://app.test/core/api/documents/"+_documentID.String()+"/files/f1xxxxxxxxxxxxxxxxxxx-notes.zip", rec.Header().Get("Location")) + }, + wasInsertCalled(1), + wasInsertCalledWith("notes.zip", 15, "text/plain; charset=utf-8"), + func(t *testing.T, _ *DBMock, storer *StorerMock, _ *httptest.ResponseRecorder) { + ff := storer.UploadCalls() + require.Len(t, ff, 1) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].ID) + }, + wasUploadCalledWith([]byte("plain text data"), "text/plain; charset=utf-8"), + wasDeleteCalled(0), + ), + }, + // a spreadsheet whose first cell happens to spell a bitmap's magic + // bytes is still the text it is. + "Successful file upload of a CSV starting like a bitmap": { + DB: &DBMock{}, + Storer: &StorerMock{}, + FileName: "Portfolio - sheet.csv", + Content: []byte("BMW,Model,Price\n3,320i,40000\n"), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusCreated, rec.Code) + assert.Equal(t, "https://app.test/core/api/documents/"+_documentID.String()+"/files/f1xxxxxxxxxxxxxxxxxxx-Portfolio%20-%20sheet.csv", rec.Header().Get("Location")) + }, + wasInsertCalledWith("Portfolio - sheet.csv", 29, "text/plain; charset=utf-8"), + ), + }, + "Successful file upload at the file size limit": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: bytes.Repeat([]byte{'x'}, int(storage.FilePolicy.MaxSize)), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusCreated, rec.Code) + }, + wasInsertCalledWith("file.png", storage.FilePolicy.MaxSize, "text/plain; charset=utf-8"), + wasUploadCalled(1), + ), + }, + "Successful file upload records a path's base name": { + DB: &DBMock{}, + Storer: &StorerMock{}, + FileName: `C:\Users\me\Documents\notes.zip`, + Content: []byte("plain text data"), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusCreated, rec.Code) + }, + wasInsertCalledWith("notes.zip", 15, "text/plain; charset=utf-8"), + ), + }, + "Successful file upload without a usable name": { + DB: &DBMock{}, + Storer: &StorerMock{}, + FileName: ".", + Content: []byte("plain text data"), + Query: "?id=f1xxxxxxxxxxxxxxxxxxx&location=document&kind=file", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusCreated, rec.Code) + }, + wasInsertCalledWith("file", 15, "text/plain; charset=utf-8"), + ), + }, } for cn, c := range cc { @@ -287,10 +485,10 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { t.Parallel() hdl := Handler{ - log: slog.New(slog.DiscardHandler), - db: c.DB, - storer: c.Storer, - fileLocationFormat: "loc/%s/%s", + log: slog.New(slog.DiscardHandler), + db: c.DB, + storer: c.Storer, + publicURL: "https://app.test/core", } var ( @@ -299,7 +497,7 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { ) if !c.NoFile { - body, contentType = multipartBody(t, "file", "file-data") + body, contentType = multipartBody(t, "file", c.FileName, c.Content) } req := httptest.NewRequest(http.MethodPut, "http://test.com/"+c.Query, body) @@ -330,17 +528,32 @@ func Test_Handler_UploadDocumentFile(t *testing.T) { } // stubFileDB returns a db mock whose file record belongs to the given -// document, which is what the ownership check compares the url against. -func stubFileDB(documentID xid.ID) *DBMock { +// document, which is what the ownership check compares the url against, +// and carries the given name and content type. +func stubFileDB(documentID xid.ID, name, contentType string) *DBMock { return &DBMock{ FetchDocumentFileFunc: func(_ context.Context, id, organizationID string) (*file.File, error) { - f := file.NewFile(id, file.LocationDocument, "key", documentID, organizationID) + f := file.NewFile(id, file.LocationDocument, file.Key(organizationID, documentID, id), documentID, organizationID, name, 9, contentType) return &f, nil }, } } +// stubObjectStorer returns a storer mock serving one object of the given +// content type. +func stubObjectStorer(contentType string) *StorerMock { + return &StorerMock{ + RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { + return &storage.ObjectInfo{ + Body: io.NopCloser(strings.NewReader("file-data")), + ETag: "etag1", + ContentType: contentType, + }, true, nil + }, + } +} + func Test_Handler_RetrieveDocumentFile(t *testing.T) { type check func(*testing.T, *DBMock, *StorerMock, *httptest.ResponseRecorder) @@ -355,7 +568,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { return } - assert.Equal(t, "f1", ff[0].BlockID) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].BlockID) assert.Equal(t, "org1", ff[0].OrganizationID) } } @@ -369,8 +582,9 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { return } - assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files", ff[0].Folder) - assert.Equal(t, "f1", ff[0].ID) + // the object is named by the row's key, extension included. + assert.Equal(t, "organizations/org1/documents/"+_documentID.String()+"/files/", ff[0].Folder) + assert.Equal(t, "f1xxxxxxxxxxxxxxxxxxx", ff[0].ID) } } @@ -380,9 +594,62 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { NoSession bool OmitDocID bool OmitFileID bool - NoneMatch string - Checks []check + // Ref is the route's file segment; empty sends a well-formed one. + Ref string + NoneMatch string + Checks []check }{ + "File segment without a name": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Ref: "f1xxxxxxxxxxxxxxxxxxx", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusNotFound, rec.Code) + assert.JSONEq(t, `{"code":"general","message":"not found"}`, rec.Body.String()) + }, + wasFetchCalled(0), + wasRetrieveCalled(0), + ), + }, + "File segment with a short id": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Ref: "f1-shot.png", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusNotFound, rec.Code) + }, + wasFetchCalled(0), + wasRetrieveCalled(0), + ), + }, + "File segment with an id of the wrong charset": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Ref: "f1.................xx-shot.png", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusNotFound, rec.Code) + }, + wasFetchCalled(0), + wasRetrieveCalled(0), + ), + }, + // the name is what the address says, not what the row holds. + "File segment with another name": { + DB: stubFileDB(_documentID, "shot.png", "image/png"), + Storer: stubObjectStorer("image/png"), + Ref: "f1xxxxxxxxxxxxxxxxxxx-renamed-with-dashes.png", + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "inline; filename=shot.png", rec.Header().Get("Content-Disposition")) + }, + wasFetchCalled(1), + wasRetrieveCalled(1), + ), + }, "No session in context": { DB: &DBMock{}, Storer: &StorerMock{}, @@ -435,7 +702,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "File belongs to another document": { - DB: stubFileDB(xid.New()), + DB: stubFileDB(xid.New(), "shot.png", "image/png"), Storer: &StorerMock{}, Checks: checks( func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { @@ -447,7 +714,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "Storer retrieval error": { - DB: stubFileDB(_documentID), + DB: stubFileDB(_documentID, "shot.png", "image/png"), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return nil, false, errors.New("boom") @@ -461,7 +728,7 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "File object not found": { - DB: stubFileDB(_documentID), + DB: stubFileDB(_documentID, "shot.png", "image/png"), Storer: &StorerMock{ RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { return nil, false, nil @@ -475,15 +742,8 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { ), }, "Matching If-None-Match": { - DB: stubFileDB(_documentID), - Storer: &StorerMock{ - RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { - return &storage.ObjectInfo{ - Body: io.NopCloser(strings.NewReader("file-data")), - ETag: "etag1", - }, true, nil - }, - }, + DB: stubFileDB(_documentID, "shot.png", "image/png"), + Storer: stubObjectStorer("image/png"), NoneMatch: "etag1", Checks: checks( func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { @@ -493,28 +753,79 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { wasRetrieveCalled(1), ), }, - "Successful retrieval": { - DB: stubFileDB(_documentID), - Storer: &StorerMock{ - RetrieveFunc: func(context.Context, string, string) (*storage.ObjectInfo, bool, error) { - return &storage.ObjectInfo{ - Body: io.NopCloser(strings.NewReader("file-data")), - ETag: "etag1", - ContentType: "image/png", - }, true, nil - }, - }, + // the row's type wins over the store's: the file system backend + // re-sniffs on the way out and cannot tell a document from a zip. + "Successful retrieval of a viewable file": { + DB: stubFileDB(_documentID, "shot.png", "image/png"), + Storer: stubObjectStorer("application/octet-stream"), Checks: checks( func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "file-data", rec.Body.String()) assert.Equal(t, "etag1", rec.Header().Get("ETag")) assert.Equal(t, "image/png", rec.Header().Get("Content-Type")) + assert.Equal(t, "inline; filename=shot.png", rec.Header().Get("Content-Disposition")) + assert.Equal(t, "nosniff", rec.Header().Get("X-Content-Type-Options")) }, wasFetchCalled(1), wasRetrieveCalled(1), ), }, + "Successful retrieval of a PDF": { + DB: stubFileDB(_documentID, "report.pdf", "application/pdf"), + Storer: stubObjectStorer("application/pdf"), + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "application/pdf", rec.Header().Get("Content-Type")) + assert.Equal(t, "inline; filename=report.pdf", rec.Header().Get("Content-Disposition")) + assert.Equal(t, "nosniff", rec.Header().Get("X-Content-Type-Options")) + }, + ), + }, + "Successful retrieval of an archive": { + DB: stubFileDB(_documentID, "notes.zip", "application/zip"), + Storer: stubObjectStorer("application/zip"), + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "application/zip", rec.Header().Get("Content-Type")) + assert.Equal(t, "attachment; filename=notes.zip", rec.Header().Get("Content-Disposition")) + assert.Equal(t, "nosniff", rec.Header().Get("X-Content-Type-Options")) + }, + ), + }, + "Successful retrieval of HTML is an attachment": { + DB: stubFileDB(_documentID, "evil.html", "text/html; charset=utf-8"), + Storer: stubObjectStorer("text/html; charset=utf-8"), + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "attachment; filename=evil.html", rec.Header().Get("Content-Disposition")) + assert.Equal(t, "nosniff", rec.Header().Get("X-Content-Type-Options")) + }, + ), + }, + "Successful retrieval under a quoted name": { + DB: stubFileDB(_documentID, `a "quoted"; name.txt`, "text/plain; charset=utf-8"), + Storer: stubObjectStorer("text/plain; charset=utf-8"), + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, `inline; filename="a \"quoted\"; name.txt"`, rec.Header().Get("Content-Disposition")) + }, + ), + }, + "Successful retrieval under a non-ASCII name": { + DB: stubFileDB(_documentID, "résumé.pdf", "application/pdf"), + Storer: stubObjectStorer("application/pdf"), + Checks: checks( + func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) { + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, `inline; filename*=utf-8''r%C3%A9sum%C3%A9.pdf`, rec.Header().Get("Content-Disposition")) + }, + ), + }, } for cn, c := range cc { @@ -540,7 +851,12 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { } if !c.OmitFileID { - ctx = testutil.AddChiCtx(ctx, "id", "f1") + ref := c.Ref + if ref == "" { + ref = "f1xxxxxxxxxxxxxxxxxxx-shot.png" + } + + ctx = testutil.AddChiCtx(ctx, "ref", ref) } if c.NoneMatch != "" { @@ -557,3 +873,92 @@ func Test_Handler_RetrieveDocumentFile(t *testing.T) { }) } } + +func Test_fileID(t *testing.T) { + cc := map[string]struct { + Ref string + Result string + OK bool + }{ + "Id and name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-notes.zip", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, + "Name with dashes": {Ref: "f1xxxxxxxxxxxxxxxxxxx-a-b-c.zip", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, + "Id with dashes": {Ref: "f1-x_-xxxxxxxxxxxxxxx-notes.zip", Result: "f1-x_-xxxxxxxxxxxxxxx", OK: true}, + "Empty name": {Ref: "f1xxxxxxxxxxxxxxxxxxx-", Result: "f1xxxxxxxxxxxxxxxxxxx", OK: true}, + "No dash after the id": {Ref: "f1xxxxxxxxxxxxxxxxxxxxnotes.zip"}, + "Id alone": {Ref: "f1xxxxxxxxxxxxxxxxxxx"}, + "Short": {Ref: "f1-notes.zip"}, + "Id with the wrong charset": {Ref: "f1.................xx-notes.zip"}, + "Empty": {Ref: ""}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + res, ok := fileID(c.Ref) + assert.Equal(t, c.OK, ok) + assert.Equal(t, c.Result, res) + }) + } +} + +func Test_uploadPolicy(t *testing.T) { + cc := map[string]struct { + Kind string + Result storage.Policy + OK bool + }{ + "Image kind": { + Kind: "image", + Result: storage.ImagePolicy, + OK: true, + }, + "File kind": { + Kind: "file", + Result: storage.FilePolicy, + OK: true, + }, + "Unknown kind": { + Kind: "video", + }, + "Empty kind": {}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + res, ok := uploadPolicy(c.Kind) + assert.Equal(t, c.OK, ok) + assert.Equal(t, c.Result, res) + }) + } +} + +func Test_fileName(t *testing.T) { + cc := map[string]struct { + Raw string + Result string + }{ + "Plain name": {Raw: "notes.zip", Result: "notes.zip"}, + "Unix path": {Raw: "/home/me/notes.zip", Result: "notes.zip"}, + "Windows path": {Raw: `C:\Users\me\notes.zip`, Result: "notes.zip"}, + "Surrounding whitespace": {Raw: " notes.zip ", Result: "notes.zip"}, + "Empty name": {Raw: "", Result: "file"}, + "Whitespace only": {Raw: " ", Result: "file"}, + "Dot": {Raw: ".", Result: "file"}, + "Root": {Raw: "/", Result: "file"}, + "Non-ASCII name": {Raw: "résumé.pdf", Result: "résumé.pdf"}, + "Name over the rune cap": {Raw: strings.Repeat("é", 300), Result: strings.Repeat("é", 255)}, + "Name at the rune cap": {Raw: strings.Repeat("a", 255), Result: strings.Repeat("a", 255)}, + "Path with a long base": {Raw: "dir/" + strings.Repeat("b", 256), Result: strings.Repeat("b", 255)}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, fileName(c.Raw)) + }) + } +} diff --git a/server/core/internal/server/internal/org/0mock_storer_test.go b/server/core/internal/server/internal/org/0mock_storer_test.go index c7a3ffe6..cc19744e 100644 --- a/server/core/internal/server/internal/org/0mock_storer_test.go +++ b/server/core/internal/server/internal/org/0mock_storer_test.go @@ -5,7 +5,6 @@ package org import ( "context" - "io" "sync" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -27,7 +26,7 @@ var _ Storer = &StorerMock{} // RetrieveFunc: func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { // panic("mock out the Retrieve method") // }, -// UploadFunc: func(ctx context.Context, folder string, id string, r io.Reader) error { +// UploadFunc: func(ctx context.Context, folder string, id string, data []byte, contentType string) error { // panic("mock out the Upload method") // }, // } @@ -44,7 +43,7 @@ type StorerMock struct { RetrieveFunc func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) // UploadFunc mocks the Upload method. - UploadFunc func(ctx context.Context, folder string, id string, r io.Reader) error + UploadFunc func(ctx context.Context, folder string, id string, data []byte, contentType string) error // calls tracks calls to the methods. calls struct { @@ -74,8 +73,10 @@ type StorerMock struct { Folder string // ID is the id argument value. ID string - // R is the r argument value. - R io.Reader + // Data is the data argument value. + Data []byte + // ContentType is the contentType argument value. + ContentType string } } lockDelete sync.RWMutex @@ -172,17 +173,19 @@ func (mock *StorerMock) RetrieveCalls() []struct { } // Upload calls UploadFunc. -func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r io.Reader) error { +func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, data []byte, contentType string) error { callInfo := struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string }{ - Ctx: ctx, - Folder: folder, - ID: id, - R: r, + Ctx: ctx, + Folder: folder, + ID: id, + Data: data, + ContentType: contentType, } mock.lockUpload.Lock() mock.calls.Upload = append(mock.calls.Upload, callInfo) @@ -193,7 +196,7 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r ) return errOut } - return mock.UploadFunc(ctx, folder, id, r) + return mock.UploadFunc(ctx, folder, id, data, contentType) } // UploadCalls gets all the calls that were made to Upload. @@ -201,16 +204,18 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r // // len(mockedStorer.UploadCalls()) func (mock *StorerMock) UploadCalls() []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } { var calls []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } mock.lockUpload.RLock() calls = mock.calls.Upload diff --git a/server/core/internal/server/internal/org/http.go b/server/core/internal/server/internal/org/http.go index 84894f01..3bf270ea 100644 --- a/server/core/internal/server/internal/org/http.go +++ b/server/core/internal/server/internal/org/http.go @@ -4,7 +4,6 @@ package org import ( "context" "fmt" - "io" "log/slog" "net/http" @@ -178,16 +177,22 @@ func (h *Handler) UploadOrganizationLogo(w http.ResponseWriter, r *http.Request) return } - file, err := httpserver.FormFile(w, r, "logo", storage.MaxUploadBytes, storage.ErrSizeLimitExceeded) + file, err := httpserver.FormFile(w, r, "logo", storage.ImagePolicy.MaxUploadBytes(), storage.ErrSizeLimitExceeded) if err != nil { httpserver.RespondError(h.log, w, err) return } defer file.Close() //nolint:errcheck // error provides no meaningful info + data, contentType, err := storage.ReadObject(file, storage.ImagePolicy) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + logoFolder := fmt.Sprintf(_organizationsLogoFolderFormat, session.ActiveOrganizationID) - err = h.storer.Upload(r.Context(), logoFolder, session.ActiveOrganizationID, file) + err = h.storer.Upload(r.Context(), logoFolder, session.ActiveOrganizationID, data, contentType) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -416,8 +421,9 @@ type TagsDBAgent interface { // //go:generate ../../../../scripts/codegen/mock -t internal Storer type Storer interface { - // Upload uploads a new object. - Upload(ctx context.Context, folder, id string, r io.Reader) error + // Upload should store the object's bytes under the given content + // type. + Upload(ctx context.Context, folder, id string, data []byte, contentType string) error // Retrieve retrieves an object by its ID. Retrieve(ctx context.Context, folder, id string) (*storage.ObjectInfo, bool, error) diff --git a/server/core/internal/server/internal/org/http_test.go b/server/core/internal/server/internal/org/http_test.go index cbe83211..9fec2197 100644 --- a/server/core/internal/server/internal/org/http_test.go +++ b/server/core/internal/server/internal/org/http_test.go @@ -58,8 +58,12 @@ func withTx(db *DBMock, tx *TxMock, err error) *DBMock { return db } +// _testPNG is a data prefix carrying the PNG magic bytes so content +// type sniffing detects image/png. +var _testPNG = append([]byte("\x89PNG\r\n\x1a\n"), bytes.Repeat([]byte{1}, 1024)...) + // multipartBody builds a multipart form body carrying a single file field. -func multipartBody(t *testing.T, field, content string) (io.Reader, string) { +func multipartBody(t *testing.T, field string, content []byte) (io.Reader, string) { t.Helper() var buf bytes.Buffer @@ -69,7 +73,7 @@ func multipartBody(t *testing.T, field, content string) (io.Reader, string) { fw, err := mw.CreateFormFile(field, "logo.png") require.NoError(t, err) - _, err = fw.Write([]byte(content)) + _, err = fw.Write(content) require.NoError(t, err) require.NoError(t, mw.Close()) @@ -482,6 +486,8 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { assert.Equal(t, "organizations/org1/logo", ff[0].Folder) assert.Equal(t, "org1", ff[0].ID) + assert.Equal(t, _testPNG, ff[0].Data) + assert.Equal(t, "image/png", ff[0].ContentType) } } @@ -510,7 +516,9 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { Storer *StorerMock NoSession bool NoFile bool - Checks []check + // Content is the multipart file's bytes; nil sends a PNG. + Content []byte + Checks []check }{ "No session in context": { DB: &DBMock{}, @@ -530,10 +538,20 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { wasUploadCalled(0), ), }, + "Error returned by storage.ReadObject": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: []byte("plain text data"), + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"storage.invalid_content_type","message":"invalid content type"}`), + wasUploadCalled(0), + wasUpdateLogoCalled(0), + ), + }, "Storer upload error": { DB: &DBMock{}, Storer: &StorerMock{ - UploadFunc: func(context.Context, string, string, io.Reader) error { + UploadFunc: func(context.Context, string, string, []byte, string) error { return errors.New("boom") }, }, @@ -606,7 +624,12 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) { ) if !c.NoFile { - body, contentType = multipartBody(t, "logo", "logo-data") + content := c.Content + if content == nil { + content = _testPNG + } + + body, contentType = multipartBody(t, "logo", content) } req := httptest.NewRequest(http.MethodPut, "http://test.com/", body) diff --git a/server/core/internal/server/internal/user/0mock_storer_test.go b/server/core/internal/server/internal/user/0mock_storer_test.go index e562aac5..53b91f80 100644 --- a/server/core/internal/server/internal/user/0mock_storer_test.go +++ b/server/core/internal/server/internal/user/0mock_storer_test.go @@ -5,7 +5,6 @@ package user import ( "context" - "io" "sync" "github.com/oxynote/oxynote/server/core/internal/storage" @@ -27,7 +26,7 @@ var _ Storer = &StorerMock{} // RetrieveFunc: func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) { // panic("mock out the Retrieve method") // }, -// UploadFunc: func(ctx context.Context, folder string, id string, r io.Reader) error { +// UploadFunc: func(ctx context.Context, folder string, id string, data []byte, contentType string) error { // panic("mock out the Upload method") // }, // } @@ -44,7 +43,7 @@ type StorerMock struct { RetrieveFunc func(ctx context.Context, folder string, id string) (*storage.ObjectInfo, bool, error) // UploadFunc mocks the Upload method. - UploadFunc func(ctx context.Context, folder string, id string, r io.Reader) error + UploadFunc func(ctx context.Context, folder string, id string, data []byte, contentType string) error // calls tracks calls to the methods. calls struct { @@ -74,8 +73,10 @@ type StorerMock struct { Folder string // ID is the id argument value. ID string - // R is the r argument value. - R io.Reader + // Data is the data argument value. + Data []byte + // ContentType is the contentType argument value. + ContentType string } } lockDelete sync.RWMutex @@ -172,17 +173,19 @@ func (mock *StorerMock) RetrieveCalls() []struct { } // Upload calls UploadFunc. -func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r io.Reader) error { +func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, data []byte, contentType string) error { callInfo := struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string }{ - Ctx: ctx, - Folder: folder, - ID: id, - R: r, + Ctx: ctx, + Folder: folder, + ID: id, + Data: data, + ContentType: contentType, } mock.lockUpload.Lock() mock.calls.Upload = append(mock.calls.Upload, callInfo) @@ -193,7 +196,7 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r ) return errOut } - return mock.UploadFunc(ctx, folder, id, r) + return mock.UploadFunc(ctx, folder, id, data, contentType) } // UploadCalls gets all the calls that were made to Upload. @@ -201,16 +204,18 @@ func (mock *StorerMock) Upload(ctx context.Context, folder string, id string, r // // len(mockedStorer.UploadCalls()) func (mock *StorerMock) UploadCalls() []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } { var calls []struct { - Ctx context.Context - Folder string - ID string - R io.Reader + Ctx context.Context + Folder string + ID string + Data []byte + ContentType string } mock.lockUpload.RLock() calls = mock.calls.Upload diff --git a/server/core/internal/server/internal/user/http.go b/server/core/internal/server/internal/user/http.go index 64b9bd38..07956ef0 100644 --- a/server/core/internal/server/internal/user/http.go +++ b/server/core/internal/server/internal/user/http.go @@ -4,7 +4,6 @@ package user import ( "context" "fmt" - "io" "log/slog" "net/http" @@ -85,14 +84,20 @@ func (h *Handler) UploadUserImage(w http.ResponseWriter, r *http.Request) { return } - file, err := httpserver.FormFile(w, r, "image", storage.MaxUploadBytes, storage.ErrSizeLimitExceeded) + file, err := httpserver.FormFile(w, r, "image", storage.ImagePolicy.MaxUploadBytes(), storage.ErrSizeLimitExceeded) if err != nil { httpserver.RespondError(h.log, w, err) return } defer file.Close() //nolint:errcheck // error provides no meaningful info - err = h.storer.Upload(r.Context(), _userImageFolder, session.UserID, file) + data, contentType, err := storage.ReadObject(file, storage.ImagePolicy) + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + err = h.storer.Upload(r.Context(), _userImageFolder, session.UserID, data, contentType) if err != nil { httpserver.RespondError(h.log, w, err) return @@ -140,8 +145,9 @@ type DBAgent interface { // //go:generate ../../../../scripts/codegen/mock -t internal Storer type Storer interface { - // Upload uploads a new object. - Upload(ctx context.Context, folder, id string, r io.Reader) error + // Upload should store the object's bytes under the given content + // type. + Upload(ctx context.Context, folder, id string, data []byte, contentType string) error // Retrieve retrieves an object by its ID. Retrieve(ctx context.Context, folder, id string) (*storage.ObjectInfo, bool, error) diff --git a/server/core/internal/server/internal/user/http_test.go b/server/core/internal/server/internal/user/http_test.go index f846a7d3..8ebc56b6 100644 --- a/server/core/internal/server/internal/user/http_test.go +++ b/server/core/internal/server/internal/user/http_test.go @@ -32,8 +32,12 @@ func addSession(ctx context.Context) context.Context { }) } +// _testPNG is a data prefix carrying the PNG magic bytes so content +// type sniffing detects image/png. +var _testPNG = append([]byte("\x89PNG\r\n\x1a\n"), bytes.Repeat([]byte{1}, 1024)...) + // multipartBody builds a multipart form body carrying a single file field. -func multipartBody(t *testing.T, field, content string) (io.Reader, string) { +func multipartBody(t *testing.T, field string, content []byte) (io.Reader, string) { t.Helper() var buf bytes.Buffer @@ -43,7 +47,7 @@ func multipartBody(t *testing.T, field, content string) (io.Reader, string) { fw, err := mw.CreateFormFile(field, "img.png") require.NoError(t, err) - _, err = fw.Write([]byte(content)) + _, err = fw.Write(content) require.NoError(t, err) require.NoError(t, mw.Close()) @@ -245,6 +249,8 @@ func Test_Handler_UploadUserImage(t *testing.T) { assert.Equal(t, "users/images", ff[0].Folder) assert.Equal(t, "u1", ff[0].ID) + assert.Equal(t, _testPNG, ff[0].Data) + assert.Equal(t, "image/png", ff[0].ContentType) } } @@ -281,7 +287,9 @@ func Test_Handler_UploadUserImage(t *testing.T) { Storer *StorerMock NoSession bool NoFile bool - Checks []check + // Content is the multipart file's bytes; nil sends a PNG. + Content []byte + Checks []check }{ "No session in context": { DB: &DBMock{}, @@ -303,10 +311,20 @@ func Test_Handler_UploadUserImage(t *testing.T) { wasUpdateUserImageCalled(0), ), }, + "Error returned by storage.ReadObject": { + DB: &DBMock{}, + Storer: &StorerMock{}, + Content: []byte("plain text data"), + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"storage.invalid_content_type","message":"invalid content type"}`), + wasUploadCalled(0), + wasUpdateUserImageCalled(0), + ), + }, "Storer upload error": { DB: &DBMock{}, Storer: &StorerMock{ - UploadFunc: func(context.Context, string, string, io.Reader) error { + UploadFunc: func(context.Context, string, string, []byte, string) error { return errors.New("boom") }, }, @@ -382,7 +400,12 @@ func Test_Handler_UploadUserImage(t *testing.T) { ) if !c.NoFile { - body, contentType = multipartBody(t, "image", "img-data") + content := c.Content + if content == nil { + content = _testPNG + } + + body, contentType = multipartBody(t, "image", content) } req := httptest.NewRequest(http.MethodPut, "http://test.com/", body) diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index 727a17eb..94726fa8 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -327,7 +327,7 @@ func (s *Server) router() chi.Router { }) ssr.Route("/files", func(sssr chi.Router) { sssr.Post("/", s.handlers.files.UploadDocumentFile) - sssr.Get("/{id}", s.handlers.files.RetrieveDocumentFile) + sssr.Get("/{ref}", s.handlers.files.RetrieveDocumentFile) }) ssr.Route("/comments", func(sssr chi.Router) { diff --git a/server/core/internal/server/server.go b/server/core/internal/server/server.go index dc144923..d45b163d 100644 --- a/server/core/internal/server/server.go +++ b/server/core/internal/server/server.go @@ -18,7 +18,6 @@ import ( assistantCore "github.com/oxynote/oxynote/server/core/internal/assistant" "github.com/oxynote/oxynote/server/core/internal/buildinfo" datasourceCore "github.com/oxynote/oxynote/server/core/internal/datasource" - documentCore "github.com/oxynote/oxynote/server/core/internal/document" notificationCore "github.com/oxynote/oxynote/server/core/internal/notification" "github.com/oxynote/oxynote/server/core/internal/search" "github.com/oxynote/oxynote/server/core/internal/server/internal/assistant" @@ -181,7 +180,7 @@ func NewServer( srv.handlers.document = document.NewHandler(log, db, githubMan, webchangeClient, searchGateway, searchJobs, notifier, storageClient) srv.handlers.tag = tag.NewHandler(log, db) srv.handlers.comment = comment.NewHandler(log, db, notifier) - srv.handlers.files = files.NewHandler(log, db, storageClient, opts.PublicURL+documentCore.FilePathFormat) + srv.handlers.files = files.NewHandler(log, db, storageClient, opts.PublicURL) srv.handlers.hook = hook.NewHandler(log, db, githubMan, webchangeClient) // The assistant's CRUD tools mutate the document tree directly diff --git a/server/core/internal/storage/fs/object.go b/server/core/internal/storage/fs/object.go index 0e3ec068..3754550d 100644 --- a/server/core/internal/storage/fs/object.go +++ b/server/core/internal/storage/fs/object.go @@ -15,20 +15,16 @@ import ( // Upload uploads a new object. // If the object with the same ID already exists, it is overwritten. -func (c *Client) Upload(_ context.Context, folder, id string, r io.Reader) error { +// +// The content type is not kept: a file system holds no metadata, so an +// object is sniffed again on the way out. A caller that needs the exact +// type back records it alongside the object's key. +func (c *Client) Upload(_ context.Context, folder, id string, data []byte, _ string) error { full, err := c.resolve(folder, id) if err != nil { return err } - data, _, err := storage.ReadObject(r) - if err != nil { - return err - } - - // the content type is not kept: an object is sniffed again on the way - // out, and the three types ReadObject admits are exactly the ones - // http.DetectContentType tells apart. return writeObject(full, data) } diff --git a/server/core/internal/storage/fs/object_test.go b/server/core/internal/storage/fs/object_test.go index 9d5a743e..d4eecabd 100644 --- a/server/core/internal/storage/fs/object_test.go +++ b/server/core/internal/storage/fs/object_test.go @@ -1,7 +1,6 @@ package fs import ( - "bytes" "context" "io" "os" @@ -10,7 +9,6 @@ import ( "testing" "time" - "github.com/oxynote/oxynote/server/core/internal/storage" "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -22,22 +20,15 @@ func Test_Client_Upload(t *testing.T) { Prep func(t *testing.T, client *Client) Folder string ID string - Reader io.Reader Data []byte Err error }{ "Error returned by resolve": { Folder: "../elsewhere", ID: "object-id", - Reader: bytes.NewReader(_testPNG), + Data: _testPNG, Err: ErrInvalidKey, }, - "Error returned by storage.ReadObject": { - Folder: "folder", - ID: "object-id", - Reader: strings.NewReader("plain text data"), - Err: storage.ErrInvalidContentType, - }, "Error returned by writeObject": { Prep: func(t *testing.T, client *Client) { t.Helper() @@ -48,7 +39,7 @@ func Test_Client_Upload(t *testing.T) { }, Folder: "folder", ID: "object-id", - Reader: bytes.NewReader(_testPNG), + Data: _testPNG, Err: assert.AnError, }, "Existing object is replaced": { @@ -59,19 +50,21 @@ func Test_Client_Upload(t *testing.T) { }, Folder: "folder", ID: "object-id", - Reader: bytes.NewReader(_testPNG), Data: _testPNG, }, "Successful upload into a nested folder": { Folder: "organizations/acme/documents/doc/files", ID: "object-id", - Reader: bytes.NewReader(_testPNG), Data: _testPNG, }, + "Successful upload of an object the store cannot sniff": { + Folder: "folder", + ID: "object-id", + Data: []byte("PK\x03\x04 zipped bytes"), + }, "Successful upload": { Folder: "folder", ID: "object-id", - Reader: bytes.NewReader(_testPNG), Data: _testPNG, }, } @@ -86,7 +79,7 @@ func Test_Client_Upload(t *testing.T) { c.Prep(t, client) } - err := client.Upload(context.Background(), c.Folder, c.ID, c.Reader) + err := client.Upload(context.Background(), c.Folder, c.ID, c.Data, "application/octet-stream") testutil.AssertEqualError(t, c.Err, err) if err != nil { diff --git a/server/core/internal/storage/object.go b/server/core/internal/storage/object.go index f5109e40..fadd7ae7 100644 --- a/server/core/internal/storage/object.go +++ b/server/core/internal/storage/object.go @@ -1,41 +1,42 @@ package storage import ( + "bytes" "errors" "fmt" "io" "net/http" "slices" + "strings" + "unicode/utf8" ) -// ReadObject reads an object off r, rejecting it unless it is one of the -// supported image types and stays within the size limit, and returns its -// bytes alongside the detected content type. +// ReadObject reads an object off r, rejecting it unless it satisfies the +// policy, and returns its bytes alongside the detected content type. The +// type is detected from the bytes, so a client's own claim about it never +// reaches storage. // // The object is fully buffered rather than streamed: the read side is -// already capped at _maxObjectSize, and a backend handed the exact bytes -// can write them in one seekable, exactly-sized pass. -func ReadObject(r io.Reader) ([]byte, string, error) { - r = newLimitedReader(r, _maxObjectSize) +// already capped at the policy's size, and a backend handed the exact +// bytes can write them in one seekable, exactly-sized pass. +func ReadObject(r io.Reader, p Policy) ([]byte, string, error) { + r = newLimitedReader(r, p.MaxSize) prefix, ct, err := SniffContentType(r) if err != nil { return nil, "", err } - switch ct { - case "image/jpeg", "image/png", "image/webp": - // OK. - default: + if len(p.ContentTypes) > 0 && !slices.Contains(p.ContentTypes, ct) { return nil, "", ErrInvalidContentType } - rest, err := io.ReadAll(r) + data, err := io.ReadAll(io.MultiReader(bytes.NewReader(prefix), r)) if err != nil { return nil, "", fmt.Errorf("reading object: %w", err) } - return slices.Concat(prefix, rest), ct, nil + return data, ct, nil } // SniffContentType detects the content type of the object r holds, @@ -52,7 +53,39 @@ func SniffContentType(r io.Reader) ([]byte, string, error) { return nil, "", fmt.Errorf("reading object: %w", err) } - return buf[:n], http.DetectContentType(buf[:n]), nil + return buf[:n], detectContentType(buf[:n]), nil +} + +// detectContentType names the type of an object by its leading bytes. A +// magic-number match is trusted only when the bytes are not plain text: +// http.DetectContentType reads a CSV whose first cell is "BMW" as a bitmap +// and one starting "GIF89a" as a gif, while a real image carries bytes no +// text file does. Text the sniffer itself names, HTML included, is left +// as it says. +func detectContentType(buf []byte) string { + ct := http.DetectContentType(buf) + + if !strings.HasPrefix(ct, "text/") && isText(buf) { + return "text/plain; charset=utf-8" + } + + return ct +} + +// isText reports whether the bytes are valid UTF-8 carrying no control +// characters beyond the whitespace a text file uses. +func isText(buf []byte) bool { + if !utf8.Valid(buf) { + return false + } + + for _, b := range buf { + if b < 0x20 && b != '\t' && b != '\n' && b != '\r' { + return false + } + } + + return true } // limitedReader wraps an io.Reader and returns ErrSizeLimitExceeded when the limit is exceeded. diff --git a/server/core/internal/storage/object_test.go b/server/core/internal/storage/object_test.go index e24941ea..059767bc 100644 --- a/server/core/internal/storage/object_test.go +++ b/server/core/internal/storage/object_test.go @@ -14,57 +14,86 @@ import ( func Test_ReadObject(t *testing.T) { cc := map[string]struct { Reader io.Reader + Policy Policy Data []byte ContentType string Err error }{ "Error returned by reader": { Reader: &errReader{}, + Policy: ImagePolicy, Err: assert.AnError, }, "Invalid content type": { Reader: strings.NewReader("plain text data"), + Policy: ImagePolicy, Err: ErrInvalidContentType, }, "Size limit exceeded": { Reader: io.MultiReader( bytes.NewReader(_testPNG), - bytes.NewReader(bytes.Repeat([]byte{4}, _maxObjectSize)), + bytes.NewReader(bytes.Repeat([]byte{4}, int(ImagePolicy.MaxSize))), ), - Err: assert.AnError, + Policy: ImagePolicy, + Err: assert.AnError, }, "Short reads still sniff the content type": { Reader: &chunkReader{r: bytes.NewReader(_testPNG)}, + Policy: ImagePolicy, Data: _testPNG, ContentType: "image/png", }, "Object shorter than the sniff length": { Reader: bytes.NewReader(_testPNG[:64]), + Policy: ImagePolicy, Data: _testPNG[:64], ContentType: "image/png", }, "Successful PNG read": { Reader: bytes.NewReader(_testPNG), + Policy: ImagePolicy, Data: _testPNG, ContentType: "image/png", }, "Successful JPEG read": { Reader: bytes.NewReader(_testJPEG), + Policy: ImagePolicy, Data: _testJPEG, ContentType: "image/jpeg", }, "Successful WebP read": { Reader: bytes.NewReader(_testWebP), + Policy: ImagePolicy, Data: _testWebP, ContentType: "image/webp", }, + "File policy accepts plain text": { + Reader: strings.NewReader("plain text data"), + Policy: FilePolicy, + Data: []byte("plain text data"), + ContentType: "text/plain; charset=utf-8", + }, + "File policy accepts an object the image policy would reject by size": { + Reader: bytes.NewReader(_testLargeText), + Policy: FilePolicy, + Data: _testLargeText, + ContentType: "text/plain; charset=utf-8", + }, + "File policy size limit exceeded": { + Reader: io.MultiReader( + bytes.NewReader(_testLargeText), + bytes.NewReader(bytes.Repeat([]byte{'x'}, int(FilePolicy.MaxSize-int64(len(_testLargeText))+1))), + ), + Policy: FilePolicy, + Err: assert.AnError, + }, } for cn, c := range cc { t.Run(cn, func(t *testing.T) { t.Parallel() - data, ct, err := ReadObject(c.Reader) + data, ct, err := ReadObject(c.Reader, c.Policy) testutil.AssertEqualError(t, c.Err, err) if err != nil { @@ -117,6 +146,53 @@ func Test_SniffContentType(t *testing.T) { } } +func Test_detectContentType(t *testing.T) { + cc := map[string]struct { + Input []byte + Result string + }{ + "PNG": {Input: _testPNG, Result: "image/png"}, + "Plain text": {Input: []byte("id,name\n1,a\n"), Result: "text/plain; charset=utf-8"}, + "Text starting with the bitmap magic": {Input: []byte("BMW,Model,Price\n3,320i,40000\n"), Result: "text/plain; charset=utf-8"}, + "Text starting with the gif magic": {Input: []byte("GIF89a,foo\n"), Result: "text/plain; charset=utf-8"}, + "Text with a byte order mark": {Input: []byte("\xef\xbb\xbfBMW,x\n"), Result: "text/plain; charset=utf-8"}, + "HTML stays HTML": {Input: []byte("hi"), Result: "text/html; charset=utf-8"}, + "Bitmap with binary bytes": {Input: append([]byte("BM"), make([]byte, 64)...), Result: "image/bmp"}, + "Binary without magic": {Input: []byte{0x00, 0x01, 0x02, 0xff}, Result: "application/octet-stream"}, + "Empty": {Input: nil, Result: "text/plain; charset=utf-8"}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, detectContentType(c.Input)) + }) + } +} + +func Test_isText(t *testing.T) { + cc := map[string]struct { + Input []byte + Result bool + }{ + "ASCII with newlines and tabs": {Input: []byte("a,b\tc\r\n"), Result: true}, + "UTF-8 text": {Input: []byte("résumé"), Result: true}, + "Invalid UTF-8": {Input: []byte{0xff, 0xfe, 'a'}}, + "Control character": {Input: []byte("a\x00b")}, + "Escape character": {Input: []byte("a\x1bb")}, + "Empty": {Input: nil, Result: true}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, isText(c.Input)) + }) + } +} + func Test_newLimitedReader(t *testing.T) { t.Parallel() diff --git a/server/core/internal/storage/s3/object.go b/server/core/internal/storage/s3/object.go index 71f285a6..d2b07c02 100644 --- a/server/core/internal/storage/s3/object.go +++ b/server/core/internal/storage/s3/object.go @@ -5,7 +5,6 @@ import ( "context" "errors" "fmt" - "io" "net/url" "path" "strings" @@ -18,20 +17,15 @@ import ( // Upload uploads a new object. // If the object with the same ID already exists, it is overwritten. -func (c *Client) Upload(ctx context.Context, folder, id string, r io.Reader) error { - data, ct, err := storage.ReadObject(r) - if err != nil { - return err - } - - _, err = c.client.PutObject( +func (c *Client) Upload(ctx context.Context, folder, id string, data []byte, contentType string) error { + _, err := c.client.PutObject( ctx, &awss3.PutObjectInput{ Bucket: aws.String(c.bucket), Key: aws.String(path.Join(folder, id)), Body: bytes.NewReader(data), ContentLength: aws.Int64(int64(len(data))), - ContentType: aws.String(ct), + ContentType: aws.String(contentType), }, ) if err != nil { diff --git a/server/core/internal/storage/s3/object_test.go b/server/core/internal/storage/s3/object_test.go index cf3ed5a5..db7f6d8d 100644 --- a/server/core/internal/storage/s3/object_test.go +++ b/server/core/internal/storage/s3/object_test.go @@ -15,49 +15,28 @@ import ( // type sniffing detects image/png. var _testPNG = append([]byte("\x89PNG\r\n\x1a\n"), bytes.Repeat([]byte{1}, 1024)...) -// _testJPEG is a data prefix carrying the JPEG magic bytes so content -// type sniffing detects image/jpeg. -var _testJPEG = append([]byte("\xff\xd8\xff"), bytes.Repeat([]byte{2}, 1024)...) - -// _testWebP is a data prefix carrying the WebP magic bytes so content -// type sniffing detects image/webp. -var _testWebP = append([]byte("RIFF\x00\x00\x00\x00WEBPVP8 "), bytes.Repeat([]byte{3}, 1024)...) - func Test_Client_Upload(t *testing.T) { cc := map[string]struct { Fake *fakeS3 - Reader io.Reader Data []byte ContentType string Err error }{ - "Error returned by storage.ReadObject": { - Fake: &fakeS3{}, - Reader: &errReader{}, - Err: assert.AnError, - }, "Error returned by PutObject": { - Fake: &fakeS3{failUpload: true}, - Reader: bytes.NewReader(_testPNG), - Err: assert.AnError, + Fake: &fakeS3{failUpload: true}, + Data: _testPNG, + ContentType: "image/png", + Err: assert.AnError, }, "Successful PNG upload": { Fake: &fakeS3{}, - Reader: bytes.NewReader(_testPNG), Data: _testPNG, ContentType: "image/png", }, - "Successful JPEG upload": { + "Successful upload under the content type it was given": { Fake: &fakeS3{}, - Reader: bytes.NewReader(_testJPEG), - Data: _testJPEG, - ContentType: "image/jpeg", - }, - "Successful WebP upload": { - Fake: &fakeS3{}, - Reader: bytes.NewReader(_testWebP), - Data: _testWebP, - ContentType: "image/webp", + Data: []byte("PK\x03\x04 zipped bytes"), + ContentType: "application/zip", }, } @@ -67,7 +46,7 @@ func Test_Client_Upload(t *testing.T) { client := prepClient(t, c.Fake) - err := client.Upload(context.Background(), "folder", "object-id", c.Reader) + err := client.Upload(context.Background(), "folder", "object-id", c.Data, c.ContentType) testutil.AssertEqualError(t, c.Err, err) if err != nil { @@ -219,11 +198,3 @@ func Test_Client_Delete(t *testing.T) { }) } } - -// errReader is a reader that always fails. -type errReader struct{} - -// Read returns a read failure. -func (er *errReader) Read(_ []byte) (int, error) { - return 0, assert.AnError -} diff --git a/server/core/internal/storage/storage.go b/server/core/internal/storage/storage.go index 3342bda0..0481b6bd 100644 --- a/server/core/internal/storage/storage.go +++ b/server/core/internal/storage/storage.go @@ -12,12 +12,17 @@ import ( "github.com/oxynote/oxynote/server/core/pkg/errutil" ) -// _maxObjectSize is the maximum allowed size for an object (5 MB). -const _maxObjectSize = 5 * 1024 * 1024 // 5 MB +const ( + // _maxImageSize is the largest image ImagePolicy admits (5 MB). + _maxImageSize = 5 * 1024 * 1024 -// MaxUploadBytes bounds an upload request's body: the object size limit -// plus headroom for multipart boundaries and part headers. -const MaxUploadBytes = _maxObjectSize + 64*1024 + // _maxFileSize is the largest attachment FilePolicy admits (25 MB). + _maxFileSize = 25 * 1024 * 1024 + + // _uploadHeadroom is the room an upload request's body gets on top of + // the object size limit, for multipart boundaries and part headers. + _uploadHeadroom = 64 * 1024 +) // _sniffLen is the number of leading bytes http.DetectContentType // examines when detecting an object's content type. @@ -29,8 +34,39 @@ var ( // ErrSizeLimitExceeded is returned when the reader exceeds the size limit. ErrSizeLimitExceeded = errutil.New(http.StatusBadRequest, "storage.size_limit_exceeded", "file size exceeds limit") + + // ImagePolicy admits the images shown inline: logos, avatars and + // image blocks. The three types are the ones http.DetectContentType + // tells apart, so a backend sniffing an object on the way out names + // the same type the upload was admitted under. + ImagePolicy = Policy{ //nolint:gochecknoglobals // used as a constant + MaxSize: _maxImageSize, + ContentTypes: []string{"image/jpeg", "image/png", "image/webp"}, + } + + // FilePolicy admits the attachments a file block carries: any type, + // served back under a Content-Disposition rather than rendered. + FilePolicy = Policy{ //nolint:gochecknoglobals // used as a constant + MaxSize: _maxFileSize, + } ) +// Policy states what an upload must satisfy before it is stored. +type Policy struct { + // MaxSize is the largest object accepted, in bytes. + MaxSize int64 + + // ContentTypes lists the content types accepted. Empty accepts every + // type. + ContentTypes []string +} + +// MaxUploadBytes bounds an upload request's body: the object size limit +// plus headroom for multipart boundaries and part headers. +func (p Policy) MaxUploadBytes() int64 { + return p.MaxSize + _uploadHeadroom +} + // ObjectInfo contains metadata about a retrieved object. type ObjectInfo struct { // Body is the object's data stream. @@ -48,9 +84,10 @@ type ObjectInfo struct { // hold either one; every consumer declares the narrower interface it // actually calls. type Store interface { - // Upload should upload a new object, overwriting an object already - // stored under the same folder and id. - Upload(ctx context.Context, folder, id string, r io.Reader) error + // Upload should store the object's bytes under the given content + // type, overwriting an object already stored under the same folder + // and id. + Upload(ctx context.Context, folder, id string, data []byte, contentType string) error // Retrieve should retrieve an object by its ID, reporting whether it // was there at all. diff --git a/server/core/internal/storage/storage_test.go b/server/core/internal/storage/storage_test.go index 81211162..78997516 100644 --- a/server/core/internal/storage/storage_test.go +++ b/server/core/internal/storage/storage_test.go @@ -21,6 +21,17 @@ var _testJPEG = append([]byte("\xff\xd8\xff"), bytes.Repeat([]byte{2}, 1024)...) // type sniffing detects image/webp. var _testWebP = append([]byte("RIFF\x00\x00\x00\x00WEBPVP8 "), bytes.Repeat([]byte{3}, 1024)...) +// _testLargeText is a 20 MB plain-text object: over the image limit and +// under the file limit. +var _testLargeText = bytes.Repeat([]byte("text "), 4*1024*1024) + +func Test_Policy_MaxUploadBytes(t *testing.T) { + t.Parallel() + + assert.EqualValues(t, 5*1024*1024+64*1024, ImagePolicy.MaxUploadBytes()) + assert.EqualValues(t, 25*1024*1024+64*1024, FilePolicy.MaxUploadBytes()) +} + func TestMain(m *testing.M) { goleak.VerifyTestMain(m) } diff --git a/web/CLAUDE.md b/web/CLAUDE.md index 0cde1279..7eb44491 100644 --- a/web/CLAUDE.md +++ b/web/CLAUDE.md @@ -90,7 +90,7 @@ API composables live in [app/composables/api/](app/composables/api/) and are re- Document editor is in [app/components/editor/](app/components/editor/). Real-time collaboration uses **Yjs** + **Hocuspocus** (`NUXT_PUBLIC_AUTH_REALTIME_API_BASE_WS_URL`). Notable subsystems: -- `blocks/` — custom node types (mermaid, metrics, code-block, figma, image, callout, split-documentation) +- `blocks/` — custom node types (mermaid, metrics, code-block, figma, image, file, callout, split-documentation); `upload-handler.ts` routes dropped and pasted files to the image or file block by type - `comments/` — comment marks + node-comment extension - `diff/` — branch diffing UI (compute, render, decorations) - `drag-handle/`, `slash/`, `link/`, `ai/` — editor UX extensions diff --git a/web/app/components/SearchModal.nuxt.test.ts b/web/app/components/SearchModal.nuxt.test.ts index 561025bb..e198f1ee 100644 --- a/web/app/components/SearchModal.nuxt.test.ts +++ b/web/app/components/SearchModal.nuxt.test.ts @@ -262,6 +262,7 @@ describe("", { concurrent: false }, () => { it.for([ { type: "document", expected: "i-lucide:file-text" }, { type: "heading", expected: "i-lucide:heading" }, + { type: "fileBlock", expected: "i-lucide:paperclip" }, { type: "paragraph", expected: "i-lucide:text" }, ])( "marks a $type hit with its own icon", diff --git a/web/app/components/SearchModal.vue b/web/app/components/SearchModal.vue index c1fc0560..62b020fd 100644 --- a/web/app/components/SearchModal.vue +++ b/web/app/components/SearchModal.vue @@ -110,6 +110,8 @@ function resultIcon(type: string) { return "lucide:file-text" case "heading": return "lucide:heading" + case "fileBlock": + return "lucide:paperclip" default: return "lucide:text" } diff --git a/web/app/components/editor/ContentEditor.vue b/web/app/components/editor/ContentEditor.vue index 6bbc28e6..b6dd8515 100644 --- a/web/app/components/editor/ContentEditor.vue +++ b/web/app/components/editor/ContentEditor.vue @@ -39,7 +39,7 @@ import { } from "./comments/node-comment-extension" import type { TextCommentIndicatorState } from "./comments/comment-mark" import CommentIndicatorContainer from "./comments/CommentIndicatorContainer.vue" -import { createImageFileHandler } from "./blocks/image" +import { createUploadFileHandler } from "./blocks/upload-handler" import { createFigmaLinkHandler } from "./blocks/figma" import { contentExtensionsWithIDs, editorProseClass } from "./schema-extensions" import { METRIC_BLOCK_NAME } from "./blocks/node-names" @@ -208,7 +208,7 @@ const contentEditor = useEditor({ props.nameEditor?.chain().focus("end").run() }, }), - createImageFileHandler({ documentId: editorStore.activeDocumentId }), + createUploadFileHandler({ documentId: editorStore.activeDocumentId }), createFigmaLinkHandler(), ], onCreate: ({ editor }) => { diff --git a/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts b/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts new file mode 100644 index 00000000..02b75c90 --- /dev/null +++ b/web/app/components/editor/blocks/file/FileBlock.nuxt.test.ts @@ -0,0 +1,544 @@ +import { mockNuxtImport } from "@nuxt/test-utils/runtime" +import type { VueWrapper } from "@vue/test-utils" +import { setResponseHeader, setResponseStatus } from "h3" +import { afterEach, beforeEach, describe, it, vi } from "vitest" +import { toast } from "vue-sonner" +import FileBlock from "./FileBlock.vue" +import { makeNode, mountNodeView } from "../../test-helpers/node-view" +import { + disposeMockEndpoints, + makeXid, + matchingString, + mockEndpoint, +} from "~/composables/api/test-helpers" +import { settleMutations, t, WAIT_FOR_OPTIONS } from "~/components/test-helpers" + +vi.mock("vue-sonner", () => ({ + toast: { + custom: vi.fn(), + dismiss: vi.fn(), + }, +})) + +const DOCUMENT_ID = makeXid("doc") + +const useRouteMock = vi.hoisted(() => vi.fn()) +mockNuxtImport("useRoute", () => useRouteMock) + +function onDocumentPage() { + useRouteMock.mockReturnValue({ + params: { documentSlug: `page-${DOCUMENT_ID}` }, + }) +} + +function mountFile( + attrs: Record = {}, + updateAttributes: (attrs: Record) => void = () => undefined, +) { + return mountNodeView(FileBlock, { + node: makeNode({ uid: "file-1", ...attrs }), + updateAttributes: updateAttributes, + }) +} + +// an uploaded attachment as the node stores it +function storedZip(attrs: Record = {}) { + return { + src: "https://cdn.test/notes.zip", + name: "notes.zip", + size: 2_516_582, + contentType: "application/zip", + ...attrs, + } +} + +function fileInput(wrapper: VueWrapper): HTMLInputElement { + return wrapper.get("input[type='file']").element as HTMLInputElement +} + +async function pickFile(wrapper: VueWrapper, file?: File) { + const input = wrapper.get("input[type='file']") + Object.defineProperty(input.element, "files", { + value: file ? [file] : [], + configurable: true, + }) + + await input.trigger("change") + await settleMutations() +} + +function zipFile() { + return new File(["zipped"], "notes.zip", { type: "application/zip" }) +} + +// what the server answers a successful upload with: the name and size +// it recorded and the type it detected, which need not match the file +function uploadedZip() { + return { + id: "file-1", + name: "notes.zip", + size: 6, + contentType: "application/zip", + } +} + +// the route mock, the editable flag, the editor store and the mocked +// toast module are all shared by the whole file, so these tests cannot +// interleave +describe("", { concurrent: false }, () => { + beforeEach(() => { + vi.mocked(toast.custom).mockReset() + useEditorMeta().setEditable(true) + useEditorStore().setReviewableDiffActive(false) + onDocumentPage() + }) + + afterEach(disposeMockEndpoints) + + it("identifies the wrapper by the node's uid", async ({ expect }) => { + const wrapper = await mountFile() + + const root = wrapper.get("[data-node-view-wrapper]") + + expect(root.attributes("id")).toBe("file-1") + expect(root.attributes("data-uid")).toBe("file-1") + expect(root.attributes("data-type")).toBe("fileBlock") + }) + + it("exposes the node's comment id and diff status on the wrapper", async ({ + expect, + }) => { + const wrapper = await mountFile({ + nodeCommentId: "comment-1", + diffStatus: "modified", + }) + + const root = wrapper.get("[data-node-view-wrapper]") + + expect(root.attributes("data-node-comment-id")).toBe("comment-1") + expect(root.attributes("data-diff-status")).toBe("modified") + }) + + it("invites the reader to pick a file while the block is empty", async ({ + expect, + }) => { + const wrapper = await mountFile() + + expect(wrapper.text()).toBe(t("editor.file.description")) + expect(wrapper.find("a").exists()).toBe(false) + }) + + it("reports an empty block as empty in read mode", async ({ expect }) => { + useEditorMeta().setEditable(false) + + const wrapper = await mountFile() + + expect(wrapper.text()).toBe(t("editor.file.empty")) + }) + + it("reports an empty block as empty while a review diff is active", async ({ + expect, + }) => { + useEditorStore().setReviewableDiffActive(true) + + const wrapper = await mountFile() + + expect(wrapper.text()).toBe(t("editor.file.empty")) + }) + + it("reports progress while an upload is running", async ({ expect }) => { + const wrapper = await mountFile({ uploading: true }) + + expect(wrapper.text()).toBe(t("editor.file.uploading")) + }) + + it("reports upload as unavailable off a document page", async ({ + expect, + }) => { + useRouteMock.mockReturnValue({ params: {} }) + + const wrapper = await mountFile() + + expect(wrapper.text()).toBe(t("editor.file.upload-unavailable")) + }) + + it("reports upload as unavailable for an unrecognisable document slug", async ({ + expect, + }) => { + useRouteMock.mockReturnValue({ params: { documentSlug: "not-a-doc" } }) + + const wrapper = await mountFile() + + expect(wrapper.text()).toBe(t("editor.file.upload-unavailable")) + }) + + it("shows the uploaded file as a card with its name and size", async ({ + expect, + }) => { + const wrapper = await mountFile(storedZip()) + + const card = wrapper.get("a") + + expect(card.text()).toContain("notes.zip") + expect(card.text()).toContain("2.4 MB") + expect(wrapper.find("input[type='file']").exists()).toBe(false) + }) + + it("renders the card from the node's attributes without a request", async ({ + expect, + }) => { + const calls = mockEndpoint( + "GET", + `/api/documents/${DOCUMENT_ID}/files/file-1`, + () => null, + ) + + const wrapper = await mountFile( + storedZip({ src: `/api/documents/${DOCUMENT_ID}/files/file-1` }), + ) + + expect(wrapper.get("a").text()).toContain("notes.zip") + expect(calls).toHaveLength(0) + }) + + it("leaves the size out when the node stores none", async ({ expect }) => { + const wrapper = await mountFile(storedZip({ size: null })) + + expect(wrapper.get("a").text().trim()).toBe("notes.zip") + }) + + it.for([ + { + name: "report.pdf", + contentType: "application/pdf", + expected: "lucide:file-text", + }, + { + name: "clip.mp4", + contentType: "video/mp4", + expected: "lucide:file-video-camera", + }, + { + name: "notes.zip", + contentType: "application/zip", + expected: "lucide:file-archive", + }, + { + name: "main.go", + contentType: "text/plain", + expected: "lucide:file-code", + }, + { + name: "thing.bin", + contentType: "application/octet-stream", + expected: "lucide:file", + }, + ])( + "shows the $expected icon for $name", + async ({ name, contentType, expected }, { expect }) => { + const wrapper = await mountFile(storedZip({ name, contentType })) + + expect(wrapper.get("a .iconify").classes()).toContain(`i-${expected}`) + }, + ) + + it.for([ + { name: "a PDF", contentType: "application/pdf" }, + { name: "a video", contentType: "video/mp4" }, + { name: "an audio file", contentType: "audio/mpeg" }, + { name: "an image", contentType: "image/png" }, + { name: "a plain text file", contentType: "text/plain; charset=utf-8" }, + ])("opens $name in a new tab", async ({ contentType }, { expect }) => { + const wrapper = await mountFile(storedZip({ contentType })) + + const card = wrapper.get("a") + + expect(card.attributes("href")).toBe("https://cdn.test/notes.zip") + expect(card.attributes("target")).toBe("_blank") + expect(card.attributes("rel")).toBe("noopener") + }) + + it.for([ + { name: "an archive", contentType: "application/zip" }, + { name: "an HTML file", contentType: "text/html" }, + { name: "an SVG", contentType: "image/svg+xml" }, + { name: "a file of unknown type", contentType: null }, + ])("links $name as a plain download", async ({ contentType }, { expect }) => { + const wrapper = await mountFile(storedZip({ contentType })) + + const card = wrapper.get("a") + + expect(card.attributes("href")).toBe("https://cdn.test/notes.zip") + expect(card.attributes("target")).toBeUndefined() + expect(card.attributes("rel")).toBe("noopener") + }) + + it.for([ + { name: "a javascript: address", src: "javascript:alert(1)" }, + { + name: "a data: address", + src: "data:text/html,", + }, + { name: "an unparsable address", src: "http://[" }, + ])("treats $name as no file", async ({ src }, { expect }) => { + const wrapper = await mountFile(storedZip({ src })) + + expect(wrapper.find("a").exists()).toBe(false) + expect(wrapper.find("input[type='file']").exists()).toBe(true) + }) + + it("keeps the card a link in read mode", async ({ expect }) => { + useEditorMeta().setEditable(false) + + const wrapper = await mountFile(storedZip()) + + expect(wrapper.get("a").attributes("href")).toBe( + "https://cdn.test/notes.zip", + ) + }) + + it.for([ + { status: "added", expected: "diff-added" }, + { status: "removed", expected: "diff-removed" }, + { status: "modified", expected: "diff-modified" }, + ])( + "marks a $status file with its diff overlay", + async ({ status, expected }, { expect }) => { + const wrapper = await mountFile(storedZip({ diffStatus: status })) + + expect(wrapper.get(".diff-overlay").classes()).toContain(expected) + expect(wrapper.get("a").classes()).toContain("diff-overlay-anchor") + }, + ) + + it("shows no diff overlay on an unchanged file", async ({ expect }) => { + const wrapper = await mountFile(storedZip({ diffStatus: "unchanged" })) + + expect(wrapper.find(".diff-overlay").exists()).toBe(false) + }) + + it("opens the file picker from the empty state without a type restriction", async ({ + expect, + }) => { + const wrapper = await mountFile() + // a real click on the hidden input bubbles back to the container + // and would re-enter the handler, so the spy stands in for it + const click = vi + .spyOn(fileInput(wrapper), "click") + .mockImplementation(() => undefined) + + await wrapper.get("[data-node-view-wrapper] > div").trigger("click") + + expect(click).toHaveBeenCalledTimes(1) + expect(fileInput(wrapper).getAttribute("accept")).toBeNull() + }) + + it("keeps the file picker shut in read mode", async ({ expect }) => { + useEditorMeta().setEditable(false) + const wrapper = await mountFile() + const click = vi.spyOn(fileInput(wrapper), "click") + + await wrapper.get("[data-node-view-wrapper] > div").trigger("click") + + expect(click).toHaveBeenCalledTimes(0) + }) + + it("keeps the file picker shut while a review diff is active", async ({ + expect, + }) => { + useEditorStore().setReviewableDiffActive(true) + const wrapper = await mountFile() + const click = vi.spyOn(fileInput(wrapper), "click") + + await wrapper.get("[data-node-view-wrapper] > div").trigger("click") + + expect(click).toHaveBeenCalledTimes(0) + }) + + it("keeps the file picker shut off a document page", async ({ expect }) => { + useRouteMock.mockReturnValue({ params: {} }) + const wrapper = await mountFile() + const click = vi.spyOn(fileInput(wrapper), "click") + + await wrapper.get("[data-node-view-wrapper] > div").trigger("click") + + expect(click).toHaveBeenCalledTimes(0) + }) + + it("keeps the file picker shut while an upload is running", async ({ + expect, + }) => { + const wrapper = await mountFile({ uploading: true }) + const click = vi.spyOn(fileInput(wrapper), "click") + + await wrapper.get("[data-node-view-wrapper] > div").trigger("click") + + expect(click).toHaveBeenCalledTimes(0) + }) + + it("uploads a picked file and turns the block into its card", async ({ + expect, + }) => { + const calls = mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + (_call, event) => { + setResponseHeader(event, "location", "https://cdn.test/stored.zip") + + return uploadedZip() + }, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({}, updateAttributes) + + await pickFile(wrapper, zipFile()) + + await vi.waitFor(() => { + expect(calls).toHaveLength(1) + }, WAIT_FOR_OPTIONS) + expect(calls[0]?.query).toEqual({ + id: "file-1", + location: "document", + kind: "file", + }) + expect(updateAttributes).toHaveBeenCalledTimes(2) + expect(updateAttributes).toHaveBeenNthCalledWith(1, { + uploading: true, + name: "notes.zip", + size: 6, + }) + expect(updateAttributes).toHaveBeenNthCalledWith(2, { + src: matchingString( + new RegExp(`/api/documents/${DOCUMENT_ID}/files/file-1-notes.zip$`), + ), + uid: "file-1", + name: "notes.zip", + size: 6, + contentType: "application/zip", + uploading: false, + }) + expect(toast.custom).toHaveBeenCalledTimes(0) + }) + + it("uploads under a generated id when the node has none", async ({ + expect, + }) => { + const calls = mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + (_call, event) => { + setResponseHeader(event, "location", "https://cdn.test/stored.zip") + + return uploadedZip() + }, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({ uid: null }, updateAttributes) + + await pickFile(wrapper, zipFile()) + + await vi.waitFor(() => { + expect(calls).toHaveLength(1) + }, WAIT_FOR_OPTIONS) + expect(calls[0]?.query.id).toEqual(expect.any(String)) + expect(calls[0]?.query.id).not.toBe("") + }) + + it("does nothing when the file dialog is dismissed", async ({ expect }) => { + const calls = mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + () => null, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({}, updateAttributes) + + await pickFile(wrapper) + + expect(calls).toHaveLength(0) + expect(updateAttributes).toHaveBeenCalledTimes(0) + }) + + it("warns that upload is unavailable off a document page", async ({ + expect, + }) => { + useRouteMock.mockReturnValue({ params: {} }) + const calls = mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + () => null, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({}, updateAttributes) + + await pickFile(wrapper, zipFile()) + + expect(calls).toHaveLength(0) + expect(updateAttributes).toHaveBeenCalledTimes(0) + expect(toast.custom).toHaveBeenCalledTimes(1) + }) + + it("returns the block to empty and warns when the upload fails", async ({ + expect, + }) => { + const consoleError = vi.spyOn(console, "error").mockImplementation(() => { + return undefined + }) + mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + (_call, event) => { + setResponseStatus(event, 500) + + return { message: "boom" } + }, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({}, updateAttributes) + + await pickFile(wrapper, zipFile()) + + await vi.waitFor(() => { + expect(toast.custom).toHaveBeenCalledTimes(1) + }, WAIT_FOR_OPTIONS) + expect(updateAttributes).toHaveBeenNthCalledWith(2, { + uploading: false, + name: null, + size: null, + contentType: null, + }) + expect(consoleError).toHaveBeenCalledTimes(1) + }) + + it("leaves a file over the size limit to the storage error handler", async ({ + expect, + }) => { + const consoleError = vi.spyOn(console, "error").mockImplementation(() => { + return undefined + }) + mockEndpoint( + "POST", + `/api/documents/${DOCUMENT_ID}/files`, + (_call, event) => { + setResponseStatus(event, 400) + + return { code: "storage.size_limit_exceeded" } + }, + ) + const updateAttributes = vi.fn() + const wrapper = await mountFile({}, updateAttributes) + + await pickFile(wrapper, zipFile()) + + await vi.waitFor(() => { + expect(toast.custom).toHaveBeenCalledTimes(1) + }, WAIT_FOR_OPTIONS) + expect(updateAttributes).toHaveBeenNthCalledWith(2, { + uploading: false, + name: null, + size: null, + contentType: null, + }) + expect(consoleError).toHaveBeenCalledTimes(0) + }) +}) diff --git a/web/app/components/editor/blocks/file/FileBlock.vue b/web/app/components/editor/blocks/file/FileBlock.vue new file mode 100644 index 00000000..17d289b7 --- /dev/null +++ b/web/app/components/editor/blocks/file/FileBlock.vue @@ -0,0 +1,245 @@ + + + diff --git a/web/app/components/editor/blocks/file/file-kind.test.ts b/web/app/components/editor/blocks/file/file-kind.test.ts new file mode 100644 index 00000000..f376e704 --- /dev/null +++ b/web/app/components/editor/blocks/file/file-kind.test.ts @@ -0,0 +1,152 @@ +import { describe, it } from "vitest" +import { + fileKind, + fileKindStyle, + formatFileSize, + isViewable, +} from "./file-kind" + +describe("fileKind", () => { + it.for([ + { name: "report.pdf", contentType: null, expected: "pdf" }, + { name: "clip.mp4", contentType: null, expected: "video" }, + { name: "clip.MOV", contentType: null, expected: "video" }, + { name: "song.mp3", contentType: null, expected: "audio" }, + { name: "photo.jpeg", contentType: null, expected: "image" }, + { name: "logo.svg", contentType: null, expected: "image" }, + { name: "notes.zip", contentType: null, expected: "archive" }, + { name: "site.tar.gz", contentType: null, expected: "archive" }, + { name: "backup.7z", contentType: null, expected: "archive" }, + { name: "README.md", contentType: null, expected: "text" }, + { name: "notes.txt", contentType: null, expected: "text" }, + { name: "main.go", contentType: null, expected: "code" }, + { name: "config.yaml", contentType: null, expected: "code" }, + { name: "data.csv", contentType: null, expected: "code" }, + { name: "deck.pptx", contentType: null, expected: "office" }, + { name: "sheet.xlsx", contentType: null, expected: "office" }, + { name: "letter.doc", contentType: null, expected: "office" }, + { name: "thing.unknown", contentType: null, expected: "generic" }, + { name: "noextension", contentType: null, expected: "generic" }, + { name: ".gitignore", contentType: null, expected: "generic" }, + { name: "trailingdot.", contentType: null, expected: "generic" }, + ])( + "classifies $name by its extension as $expected", + ({ name, contentType, expected }, { expect }) => { + expect(fileKind(name, contentType)).toBe(expected) + }, + ) + + it.for([ + { contentType: "application/pdf", expected: "pdf" }, + { contentType: "video/webm", expected: "video" }, + { contentType: "audio/ogg; codecs=opus", expected: "audio" }, + { contentType: "image/heic", expected: "image" }, + { contentType: "application/zip", expected: "archive" }, + { contentType: "application/x-tar", expected: "archive" }, + { contentType: "text/plain; charset=utf-8", expected: "text" }, + { contentType: "text/markdown", expected: "text" }, + { contentType: "application/json", expected: "code" }, + { contentType: "text/html; charset=utf-8", expected: "code" }, + { + contentType: + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + expected: "office", + }, + { + contentType: "application/vnd.oasis.opendocument.text", + expected: "office", + }, + { contentType: "application/vnd.ms-excel", expected: "office" }, + { contentType: "application/octet-stream", expected: "generic" }, + { contentType: "", expected: "generic" }, + { contentType: null, expected: "generic" }, + ])( + "falls back to the content type $contentType as $expected", + ({ contentType, expected }, { expect }) => { + expect(fileKind("thing", contentType)).toBe(expected) + }, + ) + + it("lets the extension win over the content type", ({ expect }) => { + expect(fileKind("report.pdf", "application/octet-stream")).toBe("pdf") + }) + + it("classifies a missing name by the content type", ({ expect }) => { + expect(fileKind(null, "application/pdf")).toBe("pdf") + }) +}) + +describe("fileKindStyle", () => { + it("gives every kind its own icon and accent", ({ expect }) => { + const kinds = [ + "pdf", + "video", + "audio", + "image", + "archive", + "text", + "code", + "office", + "generic", + ] as const + const styles = kinds.map((kind) => fileKindStyle(kind)) + + expect(new Set(styles.map((style) => style.icon)).size).toBe(kinds.length) + expect(new Set(styles.map((style) => style.accentClass)).size).toBe( + kinds.length, + ) + expect(styles.every((style) => style.icon.startsWith("lucide:"))).toBe(true) + }) + + it("styles the generic kind as a plain file", ({ expect }) => { + expect(fileKindStyle("generic").icon).toBe("lucide:file") + }) +}) + +describe("isViewable", () => { + it.for([ + { contentType: "application/pdf", expected: true }, + { contentType: "image/png", expected: true }, + { contentType: "image/jpeg", expected: true }, + { contentType: "image/gif", expected: true }, + { contentType: "image/webp", expected: true }, + { contentType: "video/mp4", expected: true }, + { contentType: "audio/mpeg", expected: true }, + { contentType: "text/plain; charset=utf-8", expected: true }, + { contentType: "text/html; charset=utf-8", expected: false }, + { contentType: "image/svg+xml", expected: false }, + { contentType: "text/xml", expected: false }, + { contentType: "text/javascript", expected: false }, + { contentType: "application/zip", expected: false }, + { contentType: "", expected: false }, + { contentType: null, expected: false }, + ])( + "reports $contentType as viewable: $expected", + ({ contentType, expected }, { expect }) => { + expect(isViewable(contentType)).toBe(expected) + }, + ) +}) + +describe("formatFileSize", () => { + it.for([ + { input: 0, expected: "0 B" }, + { input: 512, expected: "512 B" }, + { input: 1023, expected: "1023 B" }, + { input: 1024, expected: "1.0 KB" }, + { input: 1536, expected: "1.5 KB" }, + { input: 2_516_582, expected: "2.4 MB" }, + { input: 26_214_400, expected: "25.0 MB" }, + ])("formats $input bytes as $expected", ({ input, expected }, { expect }) => { + expect(formatFileSize(input)).toBe(expected) + }) + + it.for([ + { name: "a negative size", input: -1 }, + { name: "an infinite size", input: Number.POSITIVE_INFINITY }, + { name: "a missing size", input: null }, + { name: "an undefined size", input: undefined }, + ])("formats $name as nothing", ({ input }, { expect }) => { + expect(formatFileSize(input)).toBe("") + }) +}) diff --git a/web/app/components/editor/blocks/file/file-kind.ts b/web/app/components/editor/blocks/file/file-kind.ts new file mode 100644 index 00000000..5fe20ebc --- /dev/null +++ b/web/app/components/editor/blocks/file/file-kind.ts @@ -0,0 +1,260 @@ +// the visual family a file belongs to: what icon and accent the card +// shows, decided by extension first and content type second +export type FileKind = + | "pdf" + | "video" + | "audio" + | "image" + | "archive" + | "text" + | "code" + | "office" + | "generic" + +export interface FileKindStyle { + icon: string + accentClass: string +} + +const EXTENSION_KINDS: Record = { + pdf: "pdf", + mp4: "video", + webm: "video", + mov: "video", + mkv: "video", + avi: "video", + m4v: "video", + mp3: "audio", + wav: "audio", + ogg: "audio", + m4a: "audio", + flac: "audio", + aac: "audio", + png: "image", + jpg: "image", + jpeg: "image", + gif: "image", + webp: "image", + svg: "image", + bmp: "image", + tif: "image", + tiff: "image", + heic: "image", + zip: "archive", + tar: "archive", + gz: "archive", + tgz: "archive", + bz2: "archive", + xz: "archive", + "7z": "archive", + rar: "archive", + txt: "text", + md: "text", + markdown: "text", + rtf: "text", + log: "text", + js: "code", + mjs: "code", + cjs: "code", + ts: "code", + jsx: "code", + tsx: "code", + vue: "code", + go: "code", + py: "code", + rb: "code", + rs: "code", + java: "code", + kt: "code", + swift: "code", + c: "code", + h: "code", + cpp: "code", + cs: "code", + php: "code", + sh: "code", + json: "code", + yaml: "code", + yml: "code", + toml: "code", + xml: "code", + html: "code", + css: "code", + sql: "code", + csv: "code", + doc: "office", + docx: "office", + xls: "office", + xlsx: "office", + ppt: "office", + pptx: "office", + odt: "office", + ods: "office", + odp: "office", + key: "office", + pages: "office", + numbers: "office", +} + +const CONTENT_TYPE_KINDS: Record = { + "application/pdf": "pdf", + "application/zip": "archive", + "application/gzip": "archive", + "application/x-gzip": "archive", + "application/x-tar": "archive", + "application/x-bzip2": "archive", + "application/x-xz": "archive", + "application/x-7z-compressed": "archive", + "application/x-rar-compressed": "archive", + "application/vnd.rar": "archive", + "text/plain": "text", + "text/markdown": "text", + "text/rtf": "text", + "application/rtf": "text", + "application/json": "code", + "application/xml": "code", + "application/javascript": "code", + "text/javascript": "code", + "application/msword": "office", + "application/vnd.ms-excel": "office", + "application/vnd.ms-powerpoint": "office", +} + +const FILE_KIND_STYLES: Record = { + pdf: { icon: "lucide:file-text", accentClass: "bg-red-500/10 text-red-500" }, + video: { + icon: "lucide:file-video-camera", + accentClass: "bg-purple-500/10 text-purple-500", + }, + audio: { + icon: "lucide:file-music", + accentClass: "bg-pink-500/10 text-pink-500", + }, + image: { + icon: "lucide:file-image", + accentClass: "bg-emerald-500/10 text-emerald-500", + }, + archive: { + icon: "lucide:file-archive", + accentClass: "bg-amber-500/10 text-amber-500", + }, + text: { + icon: "lucide:file-type", + accentClass: "bg-slate-500/10 text-slate-500", + }, + code: { + icon: "lucide:file-code", + accentClass: "bg-sky-500/10 text-sky-500", + }, + office: { + icon: "lucide:file-box", + accentClass: "bg-blue-500/10 text-blue-500", + }, + generic: { + icon: "lucide:file", + accentClass: "bg-muted text-muted-foreground", + }, +} + +// the types a browser renders rather than runs, mirrored from the +// server's allowlist: the server serves these inline and the card opens +// them in a new tab, everything else is a download +const VIEWABLE_TYPES = new Set([ + "application/pdf", + "image/png", + "image/jpeg", + "image/gif", + "image/webp", + "text/plain", +]) + +function mediaType(contentType: string | null | undefined): string { + return (contentType ?? "").split(";")[0]?.trim().toLowerCase() ?? "" +} + +function extension(name: string | null | undefined): string { + const trimmed = (name ?? "").trim() + const dot = trimmed.lastIndexOf(".") + + if (dot <= 0 || dot === trimmed.length - 1) { + return "" + } + + return trimmed.slice(dot + 1).toLowerCase() +} + +export function fileKind( + name: string | null | undefined, + contentType: string | null | undefined, +): FileKind { + const byExtension = EXTENSION_KINDS[extension(name)] + + if (byExtension) { + return byExtension + } + + const type = mediaType(contentType) + const byType = CONTENT_TYPE_KINDS[type] + + if (byType) { + return byType + } + + if (type.startsWith("video/")) { + return "video" + } + + if (type.startsWith("audio/")) { + return "audio" + } + + if (type.startsWith("image/")) { + return "image" + } + + if (type.startsWith("text/")) { + return "code" + } + + if ( + type.includes("officedocument") || + type.includes("opendocument") || + type.startsWith("application/vnd.ms-") + ) { + return "office" + } + + return "generic" +} + +export function fileKindStyle(kind: FileKind): FileKindStyle { + return FILE_KIND_STYLES[kind] +} + +export function isViewable(contentType: string | null | undefined): boolean { + const type = mediaType(contentType) + + return ( + VIEWABLE_TYPES.has(type) || + type.startsWith("video/") || + type.startsWith("audio/") + ) +} + +export function formatFileSize(bytes: number | null | undefined): string { + if (typeof bytes !== "number" || !Number.isFinite(bytes) || bytes < 0) { + return "" + } + + if (bytes < 1024) { + return `${bytes} B` + } + + const kb = bytes / 1024 + + if (kb < 1024) { + return `${kb.toFixed(1)} KB` + } + + return `${(kb / 1024).toFixed(1)} MB` +} diff --git a/web/app/components/editor/blocks/file/index.test.ts b/web/app/components/editor/blocks/file/index.test.ts new file mode 100644 index 00000000..0376bab3 --- /dev/null +++ b/web/app/components/editor/blocks/file/index.test.ts @@ -0,0 +1,120 @@ +import { getSchema } from "@tiptap/core" +import Document from "@tiptap/extension-document" +import Paragraph from "@tiptap/extension-paragraph" +import Text from "@tiptap/extension-text" +import type { NodeType } from "@tiptap/pm/model" +import { describe, it } from "vitest" +import { FILE_BLOCK_NAME } from "../node-names" +import { FileBlock } from "." +import { nodeType, parseAttributes } from "~/components/editor/test-helpers" + +const schema = getSchema([Document, Text, Paragraph, FileBlock]) + +function fileType(): NodeType { + return nodeType(schema, FILE_BLOCK_NAME) +} + +describe("FileBlock", () => { + it("defines an unselectable, undraggable atomic block", ({ expect }) => { + expect(fileType().spec).toMatchObject({ + group: "block", + atom: true, + draggable: false, + selectable: false, + }) + }) + + it("matches only file block markers when parsing html", ({ expect }) => { + expect(fileType().spec.parseDOM?.[0]?.tag).toBe(`a[data-type="file-block"]`) + }) + + it("defaults every attribute to null except the upload flag", ({ + expect, + }) => { + expect(fileType().create().attrs).toEqual({ + src: null, + name: null, + size: null, + contentType: null, + uploading: false, + }) + }) + + it("renders the file attributes onto an anchor", ({ expect }) => { + const node = fileType().create({ + src: "/files/d1/b1", + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }) + + expect(fileType().spec.toDOM?.(node)).toEqual([ + "a", + { + href: "/files/d1/b1", + "data-name": "notes.zip", + "data-size": 2048, + "data-content-type": "application/zip", + "data-type": "file-block", + }, + ]) + }) + + it("keeps the transient upload flag and unset attributes out of the html", ({ + expect, + }) => { + const node = fileType().create({ src: "/files/d1/b1", uploading: true }) + + expect(fileType().spec.toDOM?.(node)).toEqual([ + "a", + { + href: "/files/d1/b1", + "data-type": "file-block", + }, + ]) + }) + + it("parses the attributes back off an anchor", ({ expect }) => { + expect( + parseAttributes(fileType(), { + href: "/files/d1/b1", + "data-name": "notes.zip", + "data-size": "2048", + "data-content-type": "application/zip", + }), + ).toEqual({ + src: "/files/d1/b1", + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }) + }) + + // tiptap drops null results from an attribute parser, so the node + // falls back to the attribute defaults + it("parses an anchor without data attributes into none", ({ expect }) => { + expect(parseAttributes(fileType(), { href: "/files/d1/b1" })).toEqual({ + src: "/files/d1/b1", + }) + }) + + it("round-trips a node through render and parse", ({ expect }) => { + const node = fileType().create({ + src: "/files/d1/b1", + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }) + const rendered = fileType().spec.toDOM?.(node) as [ + string, + Record, + ] + + expect(parseAttributes(fileType(), rendered[1])).toEqual({ + src: "/files/d1/b1", + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }) + }) +}) diff --git a/web/app/components/editor/blocks/file/index.ts b/web/app/components/editor/blocks/file/index.ts new file mode 100644 index 00000000..fb77323f --- /dev/null +++ b/web/app/components/editor/blocks/file/index.ts @@ -0,0 +1,82 @@ +import { Node, mergeAttributes } from "@tiptap/core" +import { VueNodeViewRenderer } from "@tiptap/vue-3" +import FileBlockComponent from "./FileBlock.vue" +import { FILE_BLOCK_NAME } from "../node-names" + +export const FileBlock = Node.create({ + name: FILE_BLOCK_NAME, + group: "block", + atom: true, + draggable: false, + selectable: false, + addAttributes() { + return { + src: { + default: null, + parseHTML: (element) => element.getAttribute("href"), + renderHTML: (attrs) => { + if (!attrs.src) { + return {} + } + + return { href: attrs.src as string } + }, + }, + name: { + default: null, + parseHTML: (element) => element.getAttribute("data-name"), + renderHTML: (attrs) => { + if (!attrs.name) { + return {} + } + + return { "data-name": attrs.name as string } + }, + }, + size: { + default: null, + parseHTML: (element) => { + const size = element.getAttribute("data-size") + return size ? Number.parseInt(size, 10) : null + }, + renderHTML: (attrs) => { + if (!attrs.size) { + return {} + } + + return { "data-size": attrs.size as number } + }, + }, + contentType: { + default: null, + parseHTML: (element) => element.getAttribute("data-content-type"), + renderHTML: (attrs) => { + if (!attrs.contentType) { + return {} + } + + return { "data-content-type": attrs.contentType as string } + }, + }, + uploading: { + default: false, + rendered: false, + }, + } + }, + parseHTML() { + return [{ tag: `a[data-type="file-block"]` }] + }, + renderHTML({ HTMLAttributes }) { + return [ + "a", + mergeAttributes(HTMLAttributes, { + "data-type": "file-block", + }), + ] + }, + addNodeView() { + // eslint-disable-next-line @typescript-eslint/no-unsafe-argument -- eslint's ts program resolves .vue imports as error typed, vue-tsc accepts this + return VueNodeViewRenderer(FileBlockComponent) + }, +}) diff --git a/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts b/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts index 357bb659..6c73e464 100644 --- a/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts +++ b/web/app/components/editor/blocks/image/ImageBlock.nuxt.test.ts @@ -60,6 +60,11 @@ function pngFile() { return new File(["binary"], "shot.png", { type: "image/png" }) } +// what the server answers a successful image upload with +function uploadedImage() { + return { id: "image-1", name: "shot.png", size: 6, contentType: "image/png" } +} + // the resize maths starts from the rendered image box, which happy-dom // reports as zero-sized — every resize test states the size it drags from function sizeImage(wrapper: VueWrapper, width: number, height: number) { @@ -409,7 +414,7 @@ describe("", { concurrent: false }, () => { (_call, event) => { setResponseHeader(event, "location", "https://cdn.test/stored.png") - return null + return uploadedImage() }, ) const updateAttributes = vi.fn() @@ -420,12 +425,16 @@ describe("", { concurrent: false }, () => { await vi.waitFor(() => { expect(calls).toHaveLength(1) }, WAIT_FOR_OPTIONS) - expect(calls[0]?.query).toEqual({ id: "image-1", location: "document" }) + expect(calls[0]?.query).toEqual({ + id: "image-1", + location: "document", + kind: "image", + }) expect(updateAttributes).toHaveBeenCalledTimes(2) expect(updateAttributes).toHaveBeenNthCalledWith(1, { uploading: true }) expect(updateAttributes).toHaveBeenNthCalledWith(2, { src: matchingString( - new RegExp(`/api/documents/${DOCUMENT_ID}/files/image-1$`), + new RegExp(`/api/documents/${DOCUMENT_ID}/files/image-1-shot.png$`), ), uid: "image-1", uploading: false, @@ -442,7 +451,7 @@ describe("", { concurrent: false }, () => { (_call, event) => { setResponseHeader(event, "location", "https://cdn.test/stored.png") - return null + return uploadedImage() }, ) const updateAttributes = vi.fn() @@ -545,9 +554,7 @@ describe("", { concurrent: false }, () => { expect(consoleError).toHaveBeenCalledTimes(0) }) - it("warns when the upload response carries no location", async ({ - expect, - }) => { + it("warns when the upload response carries no body", async ({ expect }) => { const consoleError = vi.spyOn(console, "error").mockImplementation(() => { return undefined }) diff --git a/web/app/components/editor/blocks/image/ImageBlock.vue b/web/app/components/editor/blocks/image/ImageBlock.vue index e04ef08f..7715fcaf 100644 --- a/web/app/components/editor/blocks/image/ImageBlock.vue +++ b/web/app/components/editor/blocks/image/ImageBlock.vue @@ -181,13 +181,18 @@ async function handleFileChange(event: Event) { props.updateAttributes({ uploading: true }) try { - await uploadDocumentFile.mutateAsync({ + const uploaded = await uploadDocumentFile.mutateAsync({ documentId: documentId.value, id: blockId, loc: DocumentFileLocation.Document, + kind: DocumentFileKind.Image, file, }) - const nextSrc = buildDocumentFileSrc(documentId.value, blockId) + const nextSrc = buildDocumentFileSrc( + documentId.value, + blockId, + uploaded.name, + ) props.updateAttributes({ src: nextSrc, uid: blockId, uploading: false }) } catch (error) { props.updateAttributes({ uploading: false }) diff --git a/web/app/components/editor/blocks/image/index.test.ts b/web/app/components/editor/blocks/image/index.test.ts index 673bc574..f14d221f 100644 --- a/web/app/components/editor/blocks/image/index.test.ts +++ b/web/app/components/editor/blocks/image/index.test.ts @@ -1,38 +1,14 @@ -import type { Editor as TiptapEditor, JSONContent } from "@tiptap/core" -import { Editor, getSchema } from "@tiptap/core" +import { getSchema } from "@tiptap/core" import Blockquote from "@tiptap/extension-blockquote" import Document from "@tiptap/extension-document" import Paragraph from "@tiptap/extension-paragraph" import Text from "@tiptap/extension-text" -import type { Node as PMNode, NodeType } from "@tiptap/pm/model" -import { beforeEach, describe, it, vi } from "vitest" -import { DocumentFileLocation } from "~/utils/api/document" +import type { NodeType } from "@tiptap/pm/model" +import { describe, it } from "vitest" import { IMAGE_BLOCK_NAME } from "../node-names" import UniqueID from "../../tiptap-utils/unique-id" -import { createImageFileHandler, ImageBlock } from "." -import type { ImageFileHandlerOptions } from "." -import { - nodeType, - paragraph, - parseAttributes, -} from "~/components/editor/test-helpers" - -const uploadDocumentFile = vi.fn() -const showToastMessage = vi.fn() - -vi.mock("~/components/toast", () => ({ - showToastMessage: (...args: unknown[]) => { - showToastMessage(...args) - }, -})) - -// the composable is pulled in through a nuxt auto-import, so the module -// itself is the only seam: the real one reaches for the nuxt app -vi.mock("~/composables/api/useDocumentFileAPI", () => ({ - default: () => ({ uploadDocumentFile: { mutateAsync: uploadDocumentFile } }), - buildDocumentFileSrc: (documentId: string, blockId: string) => - `/files/${documentId}/${blockId}`, -})) +import { ImageBlock } from "." +import { nodeType, parseAttributes } from "~/components/editor/test-helpers" const extensions = [ Document, @@ -52,75 +28,6 @@ function imageType(): NodeType { return nodeType(schema, IMAGE_BLOCK_NAME) } -// the file handler options are typed as returning void, but the paste -// and drop callbacks answer whether they handled the event -interface FileCallbacks { - allowedMimeTypes: string[] - onPaste: (editor: TiptapEditor, files: File[]) => boolean - onDrop: (editor: TiptapEditor, files: File[], pos: number) => boolean -} - -function fileCallbacks(options: ImageFileHandlerOptions): FileCallbacks { - return createImageFileHandler(options).options as unknown as FileCallbacks -} - -function makeEditor(content: JSONContent[], withImages = true): Editor { - return new Editor({ - extensions: withImages - ? extensions - : [Document, Text, Paragraph, Blockquote], - content: { type: "doc", content }, - }) -} - -function pngFile(name: string): File { - return new File(["binary"], name, { type: "image/png" }) -} - -// compresses the document into one "type" or "type:src" entry per -// top-level node, plus the pending-upload flag for images -function shape(doc: PMNode): string[] { - const entries: string[] = [] - - doc.forEach((child) => { - if (child.type.name !== IMAGE_BLOCK_NAME) { - entries.push(child.type.name) - return - } - - entries.push( - `${child.type.name}:${String(child.attrs.src)}:${String(child.attrs.uploading)}`, - ) - }) - - return entries -} - -function imageUid(doc: PMNode): string { - let uid = "" - - doc.descendants((node) => { - if (node.type.name === IMAGE_BLOCK_NAME) { - uid = node.attrs.uid as string - return false - } - - return true - }) - - if (!uid) { - throw new Error("no image block in the test document") - } - - return uid -} - -// the upload chain is pure microtasks, so one macrotask boundary is -// enough to settle it — no timers involved -function flushUpload(): Promise { - return new Promise((resolve) => setImmediate(resolve)) -} - describe("ImageBlock", () => { it("defines an unselectable, undraggable atomic block", ({ expect }) => { expect(imageType().spec).toMatchObject({ @@ -238,236 +145,3 @@ describe("ImageBlock", () => { }) }) }) - -// the upload composable and the toast module are file-level module -// mocks, so their call accounting cannot be isolated across -// interleaving tests -describe("createImageFileHandler", { concurrent: false }, () => { - beforeEach(() => { - uploadDocumentFile.mockReset() - uploadDocumentFile.mockResolvedValue(undefined) - showToastMessage.mockReset() - vi.stubGlobal("$t", (path: string) => path) - }) - - it("accepts only the supported image mime types", ({ expect }) => { - expect(fileCallbacks({ documentId: "d1" }).allowedMimeTypes).toEqual([ - "image/png", - "image/jpeg", - "image/gif", - "image/webp", - ]) - }) - - describe("onPaste", () => { - it("inserts an uploading image after the current block", ({ expect }) => { - const editor = makeEditor([paragraph("one"), paragraph("two")]) - const file = pngFile("a.png") - editor.commands.setTextSelection(2) - - const handled = fileCallbacks({ documentId: "d1" }).onPaste(editor, [ - file, - ]) - - expect(handled).toBe(true) - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - "imageBlock:null:true", - "paragraph", - ]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(1) - expect(uploadDocumentFile).toHaveBeenCalledWith({ - documentId: "d1", - id: imageUid(editor.state.doc), - loc: DocumentFileLocation.Document, - file, - }) - expect(showToastMessage).toHaveBeenCalledTimes(0) - }) - - it("points the image at the uploaded file once the upload finishes", async ({ - expect, - }) => { - const editor = makeEditor([paragraph("one")]) - editor.commands.setTextSelection(2) - fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) - const uid = imageUid(editor.state.doc) - - await flushUpload() - - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - `imageBlock:/files/d1/${uid}:false`, - ]) - expect(showToastMessage).toHaveBeenCalledTimes(0) - }) - - it("clears the upload flag and reports the failure", async ({ expect }) => { - const error = new Error("boom") - uploadDocumentFile.mockRejectedValue(error) - const logged = vi.spyOn(console, "error").mockImplementation(() => { - return undefined - }) - const editor = makeEditor([paragraph("one")]) - editor.commands.setTextSelection(2) - fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) - - await flushUpload() - - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - "imageBlock:null:false", - ]) - expect(logged).toHaveBeenCalledWith("Failed to upload image:", error) - expect(showToastMessage).toHaveBeenCalledTimes(1) - expect(showToastMessage).toHaveBeenCalledWith( - "error", - "editor.image.errors.upload-failed", - ) - }) - - it("inserts one image per pasted file", ({ expect }) => { - const editor = makeEditor([paragraph("one")]) - editor.commands.setTextSelection(2) - - fileCallbacks({ documentId: "d1" }).onPaste(editor, [ - pngFile("a.png"), - pngFile("b.png"), - ]) - - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - "imageBlock:null:true", - "imageBlock:null:true", - ]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(2) - }) - - it.for([ - { name: "ignores a paste without a document id", documentId: undefined }, - { name: "ignores a paste with a null document id", documentId: null }, - ])("$name", ({ documentId }, { expect }) => { - const editor = makeEditor([paragraph("one")]) - editor.commands.setTextSelection(2) - - expect( - fileCallbacks({ documentId }).onPaste(editor, [pngFile("a.png")]), - ).toBe(false) - expect(shape(editor.state.doc)).toEqual(["paragraph"]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(0) - }) - - it("ignores a paste nested deeper than the root level", ({ expect }) => { - const editor = makeEditor([ - { - type: "blockquote", - content: [{ type: "blockquote", content: [paragraph("deep")] }], - }, - ]) - editor.commands.setTextSelection(4) - - expect( - fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]), - ).toBe(false) - expect(uploadDocumentFile).toHaveBeenCalledTimes(0) - }) - - it("inserts nothing when the schema has no image block", ({ expect }) => { - const editor = makeEditor([paragraph("one")], false) - editor.commands.setTextSelection(2) - - expect( - fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]), - ).toBe(true) - expect(shape(editor.state.doc)).toEqual(["paragraph"]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(0) - }) - - it("leaves the document alone when the image is gone before the upload finishes", async ({ - expect, - }) => { - const editor = makeEditor([paragraph("one")]) - editor.commands.setTextSelection(2) - fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) - editor.commands.setContent({ - type: "doc", - content: [paragraph("replaced")], - }) - - await flushUpload() - - expect(shape(editor.state.doc)).toEqual(["paragraph"]) - expect(editor.state.doc.textContent).toBe("replaced") - }) - }) - - describe("onDrop", () => { - it("inserts the image after the block holding the drop position", ({ - expect, - }) => { - const editor = makeEditor([paragraph("one"), paragraph("two")]) - - const handled = fileCallbacks({ documentId: "d1" }).onDrop( - editor, - [pngFile("a.png")], - 7, - ) - - expect(handled).toBe(true) - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - "paragraph", - "imageBlock:null:true", - ]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(1) - }) - - it("inserts the image at a drop position between two blocks", ({ - expect, - }) => { - const editor = makeEditor([paragraph("one"), paragraph("two")]) - - const handled = fileCallbacks({ documentId: "d1" }).onDrop( - editor, - [pngFile("a.png")], - 5, - ) - - expect(handled).toBe(true) - expect(shape(editor.state.doc)).toEqual([ - "paragraph", - "imageBlock:null:true", - "paragraph", - ]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(1) - }) - - it("ignores a drop without a document id", ({ expect }) => { - const editor = makeEditor([paragraph("one")]) - - expect(fileCallbacks({}).onDrop(editor, [pngFile("a.png")], 2)).toBe( - false, - ) - expect(shape(editor.state.doc)).toEqual(["paragraph"]) - expect(uploadDocumentFile).toHaveBeenCalledTimes(0) - }) - - it("ignores a drop nested deeper than the root level", ({ expect }) => { - const editor = makeEditor([ - { - type: "blockquote", - content: [{ type: "blockquote", content: [paragraph("deep")] }], - }, - ]) - - expect( - fileCallbacks({ documentId: "d1" }).onDrop( - editor, - [pngFile("a.png")], - 4, - ), - ).toBe(false) - expect(uploadDocumentFile).toHaveBeenCalledTimes(0) - }) - }) -}) diff --git a/web/app/components/editor/blocks/image/index.ts b/web/app/components/editor/blocks/image/index.ts index f3b659fb..2c5da4bd 100644 --- a/web/app/components/editor/blocks/image/index.ts +++ b/web/app/components/editor/blocks/image/index.ts @@ -1,15 +1,8 @@ -import { Node, mergeAttributes, type Editor } from "@tiptap/core" -import { FileHandler } from "@tiptap/extension-file-handler" +import { Node, mergeAttributes } from "@tiptap/core" import { VueNodeViewRenderer } from "@tiptap/vue-3" -import { nanoid } from "nanoid" import ImageBlockComponent from "./ImageBlock.vue" -import { showToastMessage } from "~/components/toast" import { IMAGE_BLOCK_NAME } from "../node-names" -export interface ImageFileHandlerOptions { - documentId?: string | null | undefined -} - export const ImageBlock = Node.create({ name: IMAGE_BLOCK_NAME, group: "block", @@ -63,130 +56,3 @@ export const ImageBlock = Node.create({ return VueNodeViewRenderer(ImageBlockComponent) }, }) - -async function uploadFile( - documentId: string, - blockId: string, - file: File, -): Promise { - const { uploadDocumentFile } = useDocumentFileAPI() - - return await uploadDocumentFile.mutateAsync({ - documentId, - id: blockId, - loc: DocumentFileLocation.Document, - file, - }) -} - -export function createImageFileHandler(options: ImageFileHandlerOptions) { - return FileHandler.configure({ - allowedMimeTypes: ["image/png", "image/jpeg", "image/gif", "image/webp"], - onPaste: (editor, files) => { - if (!isRootInsertPosition(editor) || !options.documentId) { - return false - } - - for (const file of files) { - insertImageWithUpload(editor, file, options.documentId, undefined) - } - - return true - }, - onDrop: (editor, files, pos) => { - if (!isRootInsertPosition(editor, pos) || !options.documentId) { - return false - } - - for (const file of files) { - insertImageWithUpload(editor, file, options.documentId, pos) - } - - return true - }, - }) -} - -function isRootInsertPosition(editor: Editor, pos?: number): boolean { - const insertPos = pos ?? editor.state.selection.anchor - const $pos = editor.state.doc.resolve(insertPos) - - return $pos.depth <= 2 -} - -function insertImageWithUpload( - editor: Editor, - file: File, - documentId: string, - pos?: number, -) { - const blockId = nanoid() - const { state, view } = editor - const { schema } = state - - const imageNode = schema.nodes[ImageBlock.name] - if (!imageNode) { - return - } - - const insertPos = pos ?? state.selection.anchor - const $pos = state.doc.resolve(insertPos) - - // Insert image after the current block. A depth of 0 means the - // position already sits between top-level blocks, where there is no - // enclosing block to insert after. - const imagePos = $pos.depth === 0 ? insertPos : $pos.after() - - const tr = state.tr - const image = imageNode.create({ uid: blockId, uploading: true }) - tr.insert(imagePos, image) - - view.dispatch(tr) - - uploadFile(documentId, blockId, file) - .then(() => { - const src = buildDocumentFileSrc(documentId, blockId) - updateImageAttrsByUid(editor, blockId, { src, uploading: false }) - }) - .catch((error: unknown) => { - console.error("Failed to upload image:", error) - updateImageAttrsByUid(editor, blockId, { uploading: false }) - showToastMessage("error", $t("editor.image.errors.upload-failed")) - }) -} - -function updateImageAttrsByUid( - editor: Editor, - uid: string, - attrs: Record, -): void { - const { doc } = editor.state - - // -1 sentinel instead of null: typescript does not track assignments made - // inside the descendants callback, so a null check here would be reported - // as always-true and the closure below would need non-null assertions. - let nodePos = -1 - - doc.descendants((node, pos) => { - if (node.type.name === ImageBlock.name && node.attrs.uid === uid) { - nodePos = pos - return false - } - - return true - }) - - if (nodePos === -1) { - return - } - - editor.commands.command(({ tr }) => { - const node = tr.doc.nodeAt(nodePos) - if (!node) { - return false - } - - tr.setNodeMarkup(nodePos, undefined, { ...node.attrs, ...attrs }) - return true - }) -} diff --git a/web/app/components/editor/blocks/node-names.ts b/web/app/components/editor/blocks/node-names.ts index 72f82546..a8b94e2d 100644 --- a/web/app/components/editor/blocks/node-names.ts +++ b/web/app/components/editor/blocks/node-names.ts @@ -6,6 +6,7 @@ export const TITLED_CODE_BLOCK_NAME = "titledCodeBlock" export const METRIC_BLOCK_NAME = "metricBlock" export const METRIC_GRID_NAME = "metricGrid" export const IMAGE_BLOCK_NAME = "imageBlock" +export const FILE_BLOCK_NAME = "fileBlock" export const CALLOUT_BLOCK_NAME = "calloutBlock" export const MERMAID_BLOCK_NAME = "mermaidBlock" diff --git a/web/app/components/editor/blocks/upload-handler.test.ts b/web/app/components/editor/blocks/upload-handler.test.ts new file mode 100644 index 00000000..7b9061fe --- /dev/null +++ b/web/app/components/editor/blocks/upload-handler.test.ts @@ -0,0 +1,517 @@ +import type { Editor as TiptapEditor, JSONContent } from "@tiptap/core" +import { Editor, getSchema } from "@tiptap/core" +import Blockquote from "@tiptap/extension-blockquote" +import Document from "@tiptap/extension-document" +import Paragraph from "@tiptap/extension-paragraph" +import Text from "@tiptap/extension-text" +import type { Node as PMNode } from "@tiptap/pm/model" +import { beforeEach, describe, it, vi } from "vitest" +import { DocumentFileKind, DocumentFileLocation } from "~/utils/api/document" +import { FILE_BLOCK_NAME, IMAGE_BLOCK_NAME } from "./node-names" +import UniqueID from "../tiptap-utils/unique-id" +import { ImageBlock } from "./image" +import { FileBlock } from "./file" +import { createUploadFileHandler } from "./upload-handler" +import type { UploadFileHandlerOptions } from "./upload-handler" +import { paragraph } from "~/components/editor/test-helpers" + +const uploadDocumentFile = vi.fn() +const showToastMessage = vi.fn() + +vi.mock("~/components/toast", () => ({ + showToastMessage: (...args: unknown[]) => { + showToastMessage(...args) + }, +})) + +// the composable is pulled in through a nuxt auto-import, so the module +// itself is the only seam: the real one reaches for the nuxt app +vi.mock("~/composables/api/useDocumentFileAPI", () => ({ + default: () => ({ uploadDocumentFile: { mutateAsync: uploadDocumentFile } }), + buildDocumentFileSrc: (documentId: string, blockId: string, name: string) => + `/files/${documentId}/${blockId}-${name}`, +})) + +const extensions = [ + Document, + Text, + Paragraph, + Blockquote, + ImageBlock, + FileBlock, + UniqueID.configure({ + types: [Paragraph.name, Blockquote.name, ImageBlock.name, FileBlock.name], + attributeName: "uid", + }), +] + +// the schema is built once so tiptap's per-extension warnings fire once +getSchema(extensions) + +// the file handler options are typed as returning void, but the paste +// and drop callbacks answer whether they handled the event +interface FileCallbacks { + allowedMimeTypes?: string[] + onPaste: (editor: TiptapEditor, files: File[]) => boolean + onDrop: (editor: TiptapEditor, files: File[], pos: number) => boolean +} + +function fileCallbacks(options: UploadFileHandlerOptions): FileCallbacks { + return createUploadFileHandler(options).options as unknown as FileCallbacks +} + +function makeEditor(content: JSONContent[], withBlocks = true): Editor { + return new Editor({ + extensions: withBlocks + ? extensions + : [Document, Text, Paragraph, Blockquote], + content: { type: "doc", content }, + }) +} + +function pngFile(name: string): File { + return new File(["binary"], name, { type: "image/png" }) +} + +function zipFile(name: string): File { + return new File(["zipped"], name, { type: "application/zip" }) +} + +// what the server answers a successful upload with +function uploaded(name: string) { + return { name, size: 6, contentType: "application/zip" } +} + +// compresses the document into one entry per top-level node: the type +// name, then for the upload blocks the src and the pending-upload flag +function shape(doc: PMNode): string[] { + const entries: string[] = [] + + doc.forEach((child) => { + if ( + child.type.name !== IMAGE_BLOCK_NAME && + child.type.name !== FILE_BLOCK_NAME + ) { + entries.push(child.type.name) + return + } + + entries.push( + `${child.type.name}:${String(child.attrs.src)}:${String(child.attrs.uploading)}`, + ) + }) + + return entries +} + +// typescript does not track assignments made inside the descendants +// callback, so the match is collected rather than assigned +function blockAttrs(doc: PMNode, typeName: string): Record { + const found: Record[] = [] + + doc.descendants((node) => { + if (node.type.name === typeName) { + found.push(node.attrs) + return false + } + + return true + }) + + const attrs = found[0] + + if (!attrs) { + throw new Error(`no ${typeName} in the test document`) + } + + return attrs +} + +function blockUid(doc: PMNode, typeName: string): string { + return blockAttrs(doc, typeName).uid as string +} + +// the upload chain is pure microtasks, so one macrotask boundary is +// enough to settle it — no timers involved +function flushUpload(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +// the upload composable and the toast module are file-level module +// mocks, so their call accounting cannot be isolated across +// interleaving tests +describe("createUploadFileHandler", { concurrent: false }, () => { + beforeEach(() => { + uploadDocumentFile.mockReset() + uploadDocumentFile.mockResolvedValue(uploaded("a.zip")) + showToastMessage.mockReset() + vi.stubGlobal("$t", (path: string) => path) + }) + + it("accepts every file type", ({ expect }) => { + expect(fileCallbacks({ documentId: "d1" }).allowedMimeTypes).toBeUndefined() + }) + + describe("onPaste", () => { + it("inserts an uploading image after the current block for an image", ({ + expect, + }) => { + const editor = makeEditor([paragraph("one"), paragraph("two")]) + const file = pngFile("a.png") + editor.commands.setTextSelection(2) + + const handled = fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + file, + ]) + + expect(handled).toBe(true) + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "imageBlock:null:true", + "paragraph", + ]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(1) + expect(uploadDocumentFile).toHaveBeenCalledWith({ + documentId: "d1", + id: blockUid(editor.state.doc, IMAGE_BLOCK_NAME), + loc: DocumentFileLocation.Document, + kind: DocumentFileKind.Image, + file, + }) + expect(showToastMessage).toHaveBeenCalledTimes(0) + }) + + it("inserts an uploading file block after the current block for a non-image", ({ + expect, + }) => { + const editor = makeEditor([paragraph("one"), paragraph("two")]) + const file = zipFile("notes.zip") + editor.commands.setTextSelection(2) + + const handled = fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + file, + ]) + + expect(handled).toBe(true) + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "fileBlock:null:true", + "paragraph", + ]) + // the card shows the file's own name and size while the upload runs + expect(blockAttrs(editor.state.doc, FILE_BLOCK_NAME)).toMatchObject({ + name: "notes.zip", + size: 6, + contentType: null, + }) + expect(uploadDocumentFile).toHaveBeenCalledTimes(1) + expect(uploadDocumentFile).toHaveBeenCalledWith({ + documentId: "d1", + id: blockUid(editor.state.doc, FILE_BLOCK_NAME), + loc: DocumentFileLocation.Document, + kind: DocumentFileKind.File, + file, + }) + expect(showToastMessage).toHaveBeenCalledTimes(0) + }) + + it.for([ + { name: "png", type: "image/png", expected: IMAGE_BLOCK_NAME }, + { name: "jpeg", type: "image/jpeg", expected: IMAGE_BLOCK_NAME }, + { name: "gif", type: "image/gif", expected: FILE_BLOCK_NAME }, + { name: "webp", type: "image/webp", expected: IMAGE_BLOCK_NAME }, + { name: "svg", type: "image/svg+xml", expected: FILE_BLOCK_NAME }, + { name: "pdf", type: "application/pdf", expected: FILE_BLOCK_NAME }, + { name: "untyped", type: "", expected: FILE_BLOCK_NAME }, + ])( + "routes a $name file to the $expected", + ({ type, expected }, { expect }) => { + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + new File(["x"], "thing", { type }), + ]) + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + `${expected}:null:true`, + ]) + }, + ) + + it("points the image at the uploaded file once the upload finishes", async ({ + expect, + }) => { + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) + const uid = blockUid(editor.state.doc, IMAGE_BLOCK_NAME) + + await flushUpload() + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + `imageBlock:/files/d1/${uid}-a.zip:false`, + ]) + expect(showToastMessage).toHaveBeenCalledTimes(0) + }) + + it("records what the server stored on the file block once the upload finishes", async ({ + expect, + }) => { + uploadDocumentFile.mockResolvedValue({ + name: "renamed.zip", + size: 2048, + contentType: "application/zip", + }) + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + zipFile("notes.zip"), + ]) + const uid = blockUid(editor.state.doc, FILE_BLOCK_NAME) + + await flushUpload() + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + `fileBlock:/files/d1/${uid}-renamed.zip:false`, + ]) + expect(blockAttrs(editor.state.doc, FILE_BLOCK_NAME)).toMatchObject({ + name: "renamed.zip", + size: 2048, + contentType: "application/zip", + }) + expect(showToastMessage).toHaveBeenCalledTimes(0) + }) + + it("clears the upload flag and reports a failed image upload", async ({ + expect, + }) => { + const error = new Error("boom") + uploadDocumentFile.mockRejectedValue(error) + const logged = vi.spyOn(console, "error").mockImplementation(() => { + return undefined + }) + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]) + + await flushUpload() + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "imageBlock:null:false", + ]) + expect(logged).toHaveBeenCalledWith("Failed to upload image:", error) + expect(showToastMessage).toHaveBeenCalledTimes(1) + expect(showToastMessage).toHaveBeenCalledWith( + "error", + "editor.image.errors.upload-failed", + ) + }) + + it("clears the upload flag and reports a failed file upload", async ({ + expect, + }) => { + const error = new Error("boom") + uploadDocumentFile.mockRejectedValue(error) + const logged = vi.spyOn(console, "error").mockImplementation(() => { + return undefined + }) + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + zipFile("notes.zip"), + ]) + + await flushUpload() + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "fileBlock:null:false", + ]) + expect(logged).toHaveBeenCalledWith("Failed to upload file:", error) + expect(showToastMessage).toHaveBeenCalledTimes(1) + expect(showToastMessage).toHaveBeenCalledWith( + "error", + "editor.file.errors.upload-failed", + ) + }) + + // every file lands right after the block the selection sits in, so + // the later file ends up above the earlier one + it("inserts one block per pasted file", ({ expect }) => { + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + pngFile("a.png"), + zipFile("b.zip"), + ]) + + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "fileBlock:null:true", + "imageBlock:null:true", + ]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(2) + }) + + it.for([ + { name: "ignores a paste without a document id", documentId: undefined }, + { name: "ignores a paste with a null document id", documentId: null }, + ])("$name", ({ documentId }, { expect }) => { + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + + expect( + fileCallbacks({ documentId }).onPaste(editor, [pngFile("a.png")]), + ).toBe(false) + expect(shape(editor.state.doc)).toEqual(["paragraph"]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(0) + }) + + it("ignores a paste nested deeper than the root level", ({ expect }) => { + const editor = makeEditor([ + { + type: "blockquote", + content: [{ type: "blockquote", content: [paragraph("deep")] }], + }, + ]) + editor.commands.setTextSelection(4) + + expect( + fileCallbacks({ documentId: "d1" }).onPaste(editor, [pngFile("a.png")]), + ).toBe(false) + expect(uploadDocumentFile).toHaveBeenCalledTimes(0) + }) + + it("inserts nothing when the schema has no upload blocks", ({ expect }) => { + const editor = makeEditor([paragraph("one")], false) + editor.commands.setTextSelection(2) + + expect( + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + pngFile("a.png"), + zipFile("b.zip"), + ]), + ).toBe(true) + expect(shape(editor.state.doc)).toEqual(["paragraph"]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(0) + }) + + it("leaves the document alone when the block is gone before the upload finishes", async ({ + expect, + }) => { + const editor = makeEditor([paragraph("one")]) + editor.commands.setTextSelection(2) + fileCallbacks({ documentId: "d1" }).onPaste(editor, [ + zipFile("notes.zip"), + ]) + editor.commands.setContent({ + type: "doc", + content: [paragraph("replaced")], + }) + + await flushUpload() + + expect(shape(editor.state.doc)).toEqual(["paragraph"]) + expect(editor.state.doc.textContent).toBe("replaced") + }) + }) + + describe("onDrop", () => { + it("inserts the image after the block holding the drop position", ({ + expect, + }) => { + const editor = makeEditor([paragraph("one"), paragraph("two")]) + + const handled = fileCallbacks({ documentId: "d1" }).onDrop( + editor, + [pngFile("a.png")], + 7, + ) + + expect(handled).toBe(true) + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "paragraph", + "imageBlock:null:true", + ]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(1) + }) + + it("inserts a file block after the block holding the drop position", ({ + expect, + }) => { + const editor = makeEditor([paragraph("one"), paragraph("two")]) + + const handled = fileCallbacks({ documentId: "d1" }).onDrop( + editor, + [zipFile("notes.zip")], + 2, + ) + + expect(handled).toBe(true) + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "fileBlock:null:true", + "paragraph", + ]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(1) + expect(uploadDocumentFile).toHaveBeenCalledWith( + expect.objectContaining({ kind: DocumentFileKind.File }), + ) + }) + + it("inserts the block at a drop position between two blocks", ({ + expect, + }) => { + const editor = makeEditor([paragraph("one"), paragraph("two")]) + + const handled = fileCallbacks({ documentId: "d1" }).onDrop( + editor, + [zipFile("notes.zip")], + 5, + ) + + expect(handled).toBe(true) + expect(shape(editor.state.doc)).toEqual([ + "paragraph", + "fileBlock:null:true", + "paragraph", + ]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(1) + }) + + it("ignores a drop without a document id", ({ expect }) => { + const editor = makeEditor([paragraph("one")]) + + expect(fileCallbacks({}).onDrop(editor, [pngFile("a.png")], 2)).toBe( + false, + ) + expect(shape(editor.state.doc)).toEqual(["paragraph"]) + expect(uploadDocumentFile).toHaveBeenCalledTimes(0) + }) + + it("ignores a drop nested deeper than the root level", ({ expect }) => { + const editor = makeEditor([ + { + type: "blockquote", + content: [{ type: "blockquote", content: [paragraph("deep")] }], + }, + ]) + + expect( + fileCallbacks({ documentId: "d1" }).onDrop( + editor, + [zipFile("notes.zip")], + 4, + ), + ).toBe(false) + expect(uploadDocumentFile).toHaveBeenCalledTimes(0) + }) + }) +}) diff --git a/web/app/components/editor/blocks/upload-handler.ts b/web/app/components/editor/blocks/upload-handler.ts new file mode 100644 index 00000000..1a495a21 --- /dev/null +++ b/web/app/components/editor/blocks/upload-handler.ts @@ -0,0 +1,177 @@ +import type { Editor } from "@tiptap/core" +import { FileHandler } from "@tiptap/extension-file-handler" +import { nanoid } from "nanoid" +import { showToastMessage } from "~/components/toast" +import { FILE_BLOCK_NAME, IMAGE_BLOCK_NAME } from "./node-names" + +export interface UploadFileHandlerOptions { + documentId?: string | null | undefined +} + +// the image types the image block renders and the server admits as +// images; anything else dropped or pasted becomes a file attachment +const IMAGE_MIME_TYPES = new Set(["image/png", "image/jpeg", "image/webp"]) + +// one handler routes every dropped or pasted file by its type: tiptap +// registers the handler under its extension name, so two FileHandler +// instances would collide +export function createUploadFileHandler(options: UploadFileHandlerOptions) { + return FileHandler.configure({ + onPaste: (editor, files) => { + if (!isRootInsertPosition(editor) || !options.documentId) { + return false + } + + for (const file of files) { + insertBlockWithUpload(editor, file, options.documentId, undefined) + } + + return true + }, + onDrop: (editor, files, pos) => { + if (!isRootInsertPosition(editor, pos) || !options.documentId) { + return false + } + + for (const file of files) { + insertBlockWithUpload(editor, file, options.documentId, pos) + } + + return true + }, + }) +} + +function isRootInsertPosition(editor: Editor, pos?: number): boolean { + const insertPos = pos ?? editor.state.selection.anchor + const $pos = editor.state.doc.resolve(insertPos) + + return $pos.depth <= 2 +} + +function insertBlockWithUpload( + editor: Editor, + file: File, + documentId: string, + pos?: number, +) { + const isImage = IMAGE_MIME_TYPES.has(file.type) + const nodeName = isImage ? IMAGE_BLOCK_NAME : FILE_BLOCK_NAME + const blockId = nanoid() + const { state, view } = editor + const { schema } = state + + const nodeType = schema.nodes[nodeName] + if (!nodeType) { + return + } + + const insertPos = pos ?? state.selection.anchor + const $pos = state.doc.resolve(insertPos) + + // insert the block after the current one. A depth of 0 means the + // position already sits between top-level blocks, where there is no + // enclosing block to insert after. + const blockPos = $pos.depth === 0 ? insertPos : $pos.after() + + const tr = state.tr + const attrs: Record = { uid: blockId, uploading: true } + + if (!isImage) { + attrs.name = file.name + attrs.size = file.size + } + + tr.insert(blockPos, nodeType.create(attrs)) + + view.dispatch(tr) + + uploadFile(documentId, blockId, isImage, file) + .then((uploaded) => { + const src = buildDocumentFileSrc(documentId, blockId, uploaded.name) + + updateAttrsByUid( + editor, + nodeName, + blockId, + isImage + ? { src, uploading: false } + : { + src, + name: uploaded.name, + size: uploaded.size, + contentType: uploaded.contentType, + uploading: false, + }, + ) + }) + .catch((error: unknown) => { + console.error( + isImage ? "Failed to upload image:" : "Failed to upload file:", + error, + ) + updateAttrsByUid(editor, nodeName, blockId, { uploading: false }) + showToastMessage( + "error", + $t( + isImage + ? "editor.image.errors.upload-failed" + : "editor.file.errors.upload-failed", + ), + ) + }) +} + +async function uploadFile( + documentId: string, + blockId: string, + isImage: boolean, + file: File, +): Promise { + const { uploadDocumentFile } = useDocumentFileAPI() + + return await uploadDocumentFile.mutateAsync({ + documentId, + id: blockId, + loc: DocumentFileLocation.Document, + kind: isImage ? DocumentFileKind.Image : DocumentFileKind.File, + file, + }) +} + +function updateAttrsByUid( + editor: Editor, + nodeName: string, + uid: string, + attrs: Record, +): void { + const { doc } = editor.state + + // -1 sentinel instead of null: typescript does not track assignments made + // inside the descendants callback, so a null check here would be reported + // as always-true and the closure below would need non-null assertions. + let nodePos = -1 + + doc.descendants((node, pos) => { + if (node.type.name === nodeName && node.attrs.uid === uid) { + nodePos = pos + return false + } + + return true + }) + + if (nodePos === -1) { + return + } + + editor.commands.command(({ tr }) => { + const node = tr.doc.nodeAt(nodePos) + if (!node) { + return false + } + + tr.setNodeMarkup(nodePos, undefined, { ...node.attrs, ...attrs }) + return true + }) +} diff --git a/web/app/components/editor/comments/utils.test.ts b/web/app/components/editor/comments/utils.test.ts index ffae846b..c8573ba6 100644 --- a/web/app/components/editor/comments/utils.test.ts +++ b/web/app/components/editor/comments/utils.test.ts @@ -1,11 +1,13 @@ import { describe, it, vi } from "vitest" import { + CommentExtensions, createPendingCommentId, deleteCommentFromEditor, deletePendingCommentData, isPendingCommentId, pendingCommentBelongsToActiveUser, } from "./utils" +import { FILE_BLOCK_NAME, IMAGE_BLOCK_NAME } from "../blocks/node-names" import { DIFF_COMMENT_TX_META } from "../diff/diff-content-lock" import { commented, @@ -73,6 +75,22 @@ describe("pendingCommentBelongsToActiveUser", () => { }) }) +describe("CommentExtensions", () => { + // a comment has no file endpoint of its own to upload into, so the + // attachment blocks stay out of its schema + it.for([ + { name: "file", input: FILE_BLOCK_NAME }, + { name: "image", input: IMAGE_BLOCK_NAME }, + ])( + "leaves the $name block out of comment editors", + ({ input }, { expect }) => { + expect( + CommentExtensions.map((extension) => extension.name), + ).not.toContain(input) + }, + ) +}) + describe("deletePendingCommentData", () => { it("does nothing for an empty active user id", ({ expect }) => { const { editor, dispatched } = makeEditor( diff --git a/web/app/components/editor/diff/config.ts b/web/app/components/editor/diff/config.ts index c8819080..c1779675 100644 --- a/web/app/components/editor/diff/config.ts +++ b/web/app/components/editor/diff/config.ts @@ -1,5 +1,6 @@ import { CALLOUT_BLOCK_NAME, + FILE_BLOCK_NAME, IMAGE_BLOCK_NAME, MERMAID_BLOCK_NAME, METRIC_BLOCK_NAME, @@ -32,6 +33,7 @@ import type { OverlayPadding } from "./diff-decorations" */ export const DEFAULT_OPAQUE_TYPES: string[] = [ IMAGE_BLOCK_NAME, + FILE_BLOCK_NAME, METRIC_BLOCK_NAME, HorizontalRule.name, ] @@ -40,7 +42,10 @@ export const DEFAULT_OPAQUE_TYPES: string[] = [ * node types that handle diff styling inside their node-view component * rather than via ProseMirror decorations on the root element. */ -export const SELF_DECORATED_TYPES = new Set([IMAGE_BLOCK_NAME]) +export const SELF_DECORATED_TYPES = new Set([ + IMAGE_BLOCK_NAME, + FILE_BLOCK_NAME, +]) // node types that receive a modifiedTextContent attribute during // inline diff expansion. this lets their node-view components diff --git a/web/app/components/editor/drag-handle/config.test.ts b/web/app/components/editor/drag-handle/config.test.ts index a0ed0e70..7777b5af 100644 --- a/web/app/components/editor/drag-handle/config.test.ts +++ b/web/app/components/editor/drag-handle/config.test.ts @@ -21,6 +21,7 @@ import { import { CODE_BLOCK_NAME, FIGMA_BLOCK_NAME, + FILE_BLOCK_NAME, IMAGE_BLOCK_NAME, MERMAID_BLOCK_NAME, METRIC_BLOCK_NAME, @@ -41,6 +42,7 @@ type NodeName = | "listItem" | "horizontalRule" | typeof IMAGE_BLOCK_NAME + | typeof FILE_BLOCK_NAME | typeof FIGMA_BLOCK_NAME | typeof TITLED_CODE_BLOCK_NAME | "text" @@ -59,6 +61,7 @@ const schema = new Schema({ listItem: { group: "block", content: "paragraph+" }, horizontalRule: { group: "block", atom: true }, [IMAGE_BLOCK_NAME]: { group: "block", atom: true }, + [FILE_BLOCK_NAME]: { group: "block", atom: true }, [FIGMA_BLOCK_NAME]: { group: "block", atom: true }, [TITLED_CODE_BLOCK_NAME]: { group: "block", content: "text*" }, text: { group: "inline" }, @@ -212,6 +215,15 @@ describe("gapZoneConfigByType", () => { debugColor: "rgba(0, 64, 128, 0.2)", }, }, + { + name: "file block gaps are a plain row", + childNodeType: FILE_BLOCK_NAME, + expected: { + height: "1.5rem", + includeBeforeFirst: true, + debugColor: "rgba(0, 64, 128, 0.2)", + }, + }, { name: "split documentation gaps are a plain row", childNodeType: SPLIT_DOCUMENTATION_NAME, @@ -685,6 +697,7 @@ describe("DRAGGABLE_NODE_TYPES", () => { it.for([ { typeName: "paragraph", expected: true }, { typeName: IMAGE_BLOCK_NAME, expected: true }, + { typeName: FILE_BLOCK_NAME, expected: true }, { typeName: "heading", expected: true }, { typeName: "listItem", expected: true }, { typeName: "taskItem", expected: true }, @@ -829,6 +842,11 @@ describe("handlePositionByNodeType", () => { node: () => block(IMAGE_BLOCK_NAME), expected: { placement: "left-start", yOffset: 4, xOffset: 0 }, }, + { + name: "file blocks are centred on the card", + node: () => block(FILE_BLOCK_NAME), + expected: { placement: "left", yOffset: 0, xOffset: 0 }, + }, { name: "figma blocks drop by four pixels", node: () => block(FIGMA_BLOCK_NAME), diff --git a/web/app/components/editor/drag-handle/config.ts b/web/app/components/editor/drag-handle/config.ts index dfd3903b..9d6848c3 100644 --- a/web/app/components/editor/drag-handle/config.ts +++ b/web/app/components/editor/drag-handle/config.ts @@ -26,6 +26,7 @@ import { MetricGrid } from "../blocks/metrics" import { CODE_BLOCK_NAME, FIGMA_BLOCK_NAME, + FILE_BLOCK_NAME, MERMAID_BLOCK_NAME, METRIC_BLOCK_NAME, TITLED_CODE_BLOCK_NAME, @@ -148,6 +149,7 @@ export function gapZoneConfigByType( debugColor: "rgba(0, 64, 128, 0.2)", } case ImageBlock.name: + case FILE_BLOCK_NAME: return { height: "1.5rem", includeBeforeFirst: true, @@ -397,6 +399,7 @@ export const DRAG_HANDLE_IGNORE_SELF_CLASS = "drag-handle-ignore-self" export const DRAGGABLE_NODE_TYPES = new Set([ Paragraph.name, ImageBlock.name, + FILE_BLOCK_NAME, Heading.name, ListItem.name, TaskItem.name, @@ -476,6 +479,11 @@ export function handlePositionByNodeType( case FIGMA_BLOCK_NAME: res.yOffset = 4 break + // the card is one short row, so the handle sits on its middle + case FILE_BLOCK_NAME: + res.placement = "left" + res.yOffset = 0 + break case HorizontalRule.name: res.placement = "left" res.yOffset = 0 diff --git a/web/app/components/editor/hooks/hook-decorator.ts b/web/app/components/editor/hooks/hook-decorator.ts index 54d6b4c8..483c6790 100644 --- a/web/app/components/editor/hooks/hook-decorator.ts +++ b/web/app/components/editor/hooks/hook-decorator.ts @@ -6,6 +6,7 @@ import type { Node as PMNode } from "prosemirror-model" import type { EditorView } from "prosemirror-view" import { CODE_BLOCK_NAME, + FILE_BLOCK_NAME, IMAGE_BLOCK_NAME, METRIC_BLOCK_NAME, } from "../blocks/node-names" @@ -15,6 +16,7 @@ import { const NODE_VIEW_BLOCK_NAMES: ReadonlySet = new Set([ CODE_BLOCK_NAME, IMAGE_BLOCK_NAME, + FILE_BLOCK_NAME, ]) declare module "@tiptap/core" { diff --git a/web/app/components/editor/schema-extensions.test.ts b/web/app/components/editor/schema-extensions.test.ts index ee39f905..e4ce3d54 100644 --- a/web/app/components/editor/schema-extensions.test.ts +++ b/web/app/components/editor/schema-extensions.test.ts @@ -106,7 +106,9 @@ describe("contentExtensionsWithIDs", () => { .filter((extension) => extension.type === "node") .map((extension) => extension.name), ) - expect(types).toEqual(expect.arrayContaining(["paragraph", "imageBlock"])) + expect(types).toEqual( + expect.arrayContaining(["paragraph", "imageBlock", "fileBlock"]), + ) expect(types).not.toContain("bold") expect(types).not.toContain("link") expect(types).not.toContain("listKeymap") @@ -136,6 +138,7 @@ describe("contentExtensionsWithIDs", () => { "taskItem", "calloutBlock", "imageBlock", + "fileBlock", "mermaidBlock", "figmaBlock", "splitDocumentation", @@ -192,7 +195,7 @@ describe("contentExtensionsWithIDs", () => { }) describe("the uid attribute", () => { - it.for(["paragraph", "heading", "calloutBlock", "imageBlock"])( + it.for(["paragraph", "heading", "calloutBlock", "imageBlock", "fileBlock"])( "adds a uid attribute to the %s node", (name, { expect }) => { expect(nodeType(schema, name).spec.attrs?.uid).toEqual({ diff --git a/web/app/components/editor/schema-extensions.ts b/web/app/components/editor/schema-extensions.ts index 18c42bf1..585ff6eb 100644 --- a/web/app/components/editor/schema-extensions.ts +++ b/web/app/components/editor/schema-extensions.ts @@ -32,6 +32,7 @@ import { CalloutBlock } from "./blocks/callout" import { CodeBlock, CodeBlockTitle, TitledCodeBlock } from "./blocks/code-block" import { MetricBlock, MetricGrid } from "./blocks/metrics" import { ImageBlock } from "./blocks/image" +import { FileBlock } from "./blocks/file" import { ParameterList, ParameterListHeader, @@ -123,6 +124,7 @@ const nodeExtensions: Extensions = [ MetricGrid, MetricBlock, ImageBlock, + FileBlock, MermaidBlock, FigmaBlock, SplitDocumentation, diff --git a/web/app/components/editor/slash/items.test.ts b/web/app/components/editor/slash/items.test.ts index 4ce0a48c..e90888dd 100644 --- a/web/app/components/editor/slash/items.test.ts +++ b/web/app/components/editor/slash/items.test.ts @@ -1,14 +1,22 @@ -import type { Editor, JSONContent } from "@tiptap/core" +import type { JSONContent } from "@tiptap/core" +import { Editor } from "@tiptap/core" +import Document from "@tiptap/extension-document" +import Paragraph from "@tiptap/extension-paragraph" +import Text from "@tiptap/extension-text" import { Schema } from "@tiptap/pm/model" import { EditorState, TextSelection } from "@tiptap/pm/state" import { describe, it, vi } from "vitest" import { CALLOUT_BLOCK_NAME, CODE_BLOCK_NAME, + FILE_BLOCK_NAME, + IMAGE_BLOCK_NAME, SPLIT_DOCUMENTATION_LEFT_SIDE_NAME, SPLIT_DOCUMENTATION_PARAMETER_LIST_NAME, SPLIT_DOCUMENTATION_RIGHT_SIDE_NAME, } from "../blocks/node-names" +import { FileBlock } from "../blocks/file" +import { ImageBlock } from "../blocks/image" import { allowSlashItemsByContext, CommandGroup, @@ -105,6 +113,28 @@ function titles(query: string, cursorText: string): string[] { return filterSlashItems({ query, editor }).map((item) => item.title) } +// a live editor holding the two upload atoms, for the commands that +// replace the paragraph the menu was opened in +function makeUploadEditor(): Editor { + return new Editor({ + extensions: [Document, Text, Paragraph, ImageBlock, FileBlock], + content: { + type: "doc", + content: [paragraph("before"), paragraph("/"), paragraph("after")], + }, + }) +} + +function topLevelTypes(editor: Editor): string[] { + const types: string[] = [] + + editor.state.doc.forEach((child) => { + types.push(child.type.name) + }) + + return types +} + describe("commandGroupSortIndex", () => { it.for([ { group: CommandGroup.Text, expected: 0 }, @@ -266,4 +296,55 @@ describe("filterSlashItems", () => { expect(setNode).toHaveBeenCalledWith("heading", { level }) }, ) + + it.for([ + { title: "File", expected: FILE_BLOCK_NAME }, + { title: "Image", expected: IMAGE_BLOCK_NAME }, + ])( + "returns a $title command that swaps the paragraph for an empty $expected", + ({ title, expected }, { expect }) => { + const editor = makeUploadEditor() + // the slash was typed in the middle paragraph + editor.commands.setTextSelection(10) + const from = editor.state.selection.from + + const item = filterSlashItems({ query: title, editor }).find( + (v) => v.title === title, + ) + if (!item) { + throw new Error(`the ${title} item is missing from the slash menu`) + } + + item.command({ editor, range: { from, to: from } }) + + expect(topLevelTypes(editor)).toEqual([ + "paragraph", + expected, + "paragraph", + ]) + expect(editor.state.doc.child(1).attrs.src).toBeNull() + expect(editor.state.doc.child(1).attrs.uploading).toBe(false) + }, + ) + + it("returns a File command that leaves a schema without the block alone", ({ + expect, + }) => { + const editor = new Editor({ + extensions: [Document, Text, Paragraph], + content: { type: "doc", content: [paragraph("/")] }, + }) + editor.commands.setTextSelection(1) + + const item = filterSlashItems({ query: "File", editor }).find( + (v) => v.title === "File", + ) + if (!item) { + throw new Error("the File item is missing from the slash menu") + } + + item.command({ editor, range: { from: 1, to: 1 } }) + + expect(topLevelTypes(editor)).toEqual(["paragraph"]) + }) }) diff --git a/web/app/components/editor/slash/items.ts b/web/app/components/editor/slash/items.ts index 3301367b..1ef8a985 100644 --- a/web/app/components/editor/slash/items.ts +++ b/web/app/components/editor/slash/items.ts @@ -13,6 +13,7 @@ import { ParameterList } from "../blocks/split-documentation/parameter-list" import { CalloutBlock } from "../blocks/callout" import { insertMetricBlock } from "../blocks/metrics" import { ImageBlock } from "../blocks/image" +import { FileBlock } from "../blocks/file" import { extractHocuspocusProviderFromEditor, isRangeBeingEditedByOther, @@ -366,38 +367,16 @@ const allItems: CommandItem[] = [ icon: "lucide:image", group: CommandGroup.BasicBlock, command: ({ editor, range }: CommandData) => { - const { state, view } = editor - const { schema } = state - - const imageNode = schema.nodes[ImageBlock.name] - if (!imageNode) { - return - } - - const $pos = state.doc.resolve(range.from) - - const paragraphStart = $pos.before() - const paragraphEnd = $pos.after() - - // block if another user is editing in this range - const provider = extractHocuspocusProviderFromEditor(editor) - if ( - isRangeBeingEditedByOther( - provider, - state.doc, - paragraphStart, - paragraphEnd, - ) - ) { - return - } - - const image = imageNode.create() - - const tr = state.tr.delete(paragraphStart, paragraphEnd) - tr.insert(paragraphStart, image) - - view.dispatch(tr) + replaceParagraphWithEmptyBlock(editor, range, ImageBlock.name) + }, + }, + { + title: "File", + nodeType: FileBlock.name, + icon: "lucide:paperclip", + group: CommandGroup.BasicBlock, + command: ({ editor, range }: CommandData) => { + replaceParagraphWithEmptyBlock(editor, range, FileBlock.name) }, }, { @@ -591,3 +570,38 @@ export function filterSlashItems({ ) }) } + +// replaceParagraphWithEmptyBlock swaps the paragraph the slash command +// was typed in for an empty atom of the given type, which then invites +// the reader to fill it +function replaceParagraphWithEmptyBlock( + editor: Editor, + range: CommandData["range"], + nodeName: string, +) { + const { state, view } = editor + const { schema } = state + + const nodeType = schema.nodes[nodeName] + if (!nodeType) { + return + } + + const $pos = state.doc.resolve(range.from) + + const paragraphStart = $pos.before() + const paragraphEnd = $pos.after() + + // block if another user is editing in this range + const provider = extractHocuspocusProviderFromEditor(editor) + if ( + isRangeBeingEditedByOther(provider, state.doc, paragraphStart, paragraphEnd) + ) { + return + } + + const tr = state.tr.delete(paragraphStart, paragraphEnd) + tr.insert(paragraphStart, nodeType.create()) + + view.dispatch(tr) +} diff --git a/web/app/composables/api/useDocumentFileAPI.nuxt.test.ts b/web/app/composables/api/useDocumentFileAPI.nuxt.test.ts index 97ac6051..f45543df 100644 --- a/web/app/composables/api/useDocumentFileAPI.nuxt.test.ts +++ b/web/app/composables/api/useDocumentFileAPI.nuxt.test.ts @@ -1,5 +1,5 @@ import { registerEndpoint } from "@nuxt/test-utils/runtime" -import { getQuery, setResponseHeader, type H3Event } from "h3" +import { getQuery, type H3Event } from "h3" import { afterEach, beforeEach, describe, it } from "vitest" import { clearQueryCache, @@ -18,32 +18,38 @@ interface RecordedUpload { fieldNames: string[] } +// the body the server answers an upload with +const UPLOADED = { + id: "block-1", + name: "notes.zip", + size: 2048, + contentType: "application/zip", +} + // registers an upload handler on the test-time h3 app that the real api // clients route through, recording the query parameters and multipart // field names of each call for accounting. The test transport hands the // fetch body to the handler unserialized (node-mock-http assigns it // straight to req.body), so the field names are read off the original // FormData rather than parsed from a wire format. The handler answers -// with the given location header, or with no header at all when location -// is omitted. -function mockUploadEndpoint(url: string, location?: string): RecordedUpload[] { +// with the recorded file as its body, or with no body when body is null. +function mockUploadEndpoint( + url: string, + body: typeof UPLOADED | null = UPLOADED, +): RecordedUpload[] { const calls: RecordedUpload[] = [] const dispose = registerEndpoint(url, { method: "POST", handler: (event: H3Event) => { - const body = (event.node.req as { body?: unknown }).body + const req = (event.node.req as { body?: unknown }).body calls.push({ query: getQuery(event), - fieldNames: body instanceof FormData ? [...body.keys()] : [], + fieldNames: req instanceof FormData ? [...req.keys()] : [], }) - if (location !== undefined) { - setResponseHeader(event, "location", location) - } - - return null + return body }, }) @@ -60,64 +66,86 @@ describe("useDocumentFileAPI", { concurrent: false }, () => { afterEach(disposeMockEndpoints) describe("buildDocumentFileSrc", () => { - it("builds the file url from the document and block ids", ({ expect }) => { - const src = runInApp(() => buildDocumentFileSrc("doc-1", "block-1")) + it("builds the file url from the document id, block id and escaped name", ({ + expect, + }) => { + const src = runInApp(() => + buildDocumentFileSrc("doc-1", "block-1", "my notes.zip"), + ) // the test runtime config leaves coreAPIBaseHttpURL empty, so // the url is origin-relative - expect(src).toBe("/api/documents/doc-1/files/block-1") + expect(src).toBe("/api/documents/doc-1/files/block-1-my%20notes.zip") }) }) describe("uploadDocumentFile", () => { - it("uploads the file and returns the stored location", async ({ + it("uploads the file and returns what the server recorded", async ({ expect, }) => { - const uploadCalls = mockUploadEndpoint( - "/api/documents/doc-1/files", - "/files/document-file-1", - ) + const uploadCalls = mockUploadEndpoint("/api/documents/doc-1/files") const api = makeDocumentFileAPI() const file = new File(["x"], "x.png", { type: "image/png" }) - const location = await api.uploadDocumentFile.mutateAsync({ + const uploaded = await api.uploadDocumentFile.mutateAsync({ documentId: "doc-1", id: "block 1/a", loc: DocumentFileLocation.Document, + kind: DocumentFileKind.Image, file, }) - expect(location).toBe("/files/document-file-1") + expect(uploaded).toEqual({ + name: "notes.zip", + size: 2048, + contentType: "application/zip", + }) expect(uploadCalls).toHaveLength(1) expect(uploadCalls[0]?.query).toEqual({ id: "block 1/a", location: "document", + kind: "image", }) expect(uploadCalls[0]?.fieldNames).toEqual(["file"]) }) - it("rejects when the response has no location header", async ({ - expect, - }) => { + it("sends the file kind the caller names", async ({ expect }) => { const uploadCalls = mockUploadEndpoint("/api/documents/doc-1/files") const api = makeDocumentFileAPI() + const file = new File(["x"], "x.zip", { type: "application/zip" }) + + await api.uploadDocumentFile.mutateAsync({ + documentId: "doc-1", + id: "block-1", + loc: DocumentFileLocation.Document, + kind: DocumentFileKind.File, + file, + }) + + expect(uploadCalls).toHaveLength(1) + expect(uploadCalls[0]?.query).toEqual({ + id: "block-1", + location: "document", + kind: "file", + }) + }) + + it("rejects when the response has no body", async ({ expect }) => { + const uploadCalls = mockUploadEndpoint("/api/documents/doc-1/files", null) + const api = makeDocumentFileAPI() const file = new File(["x"], "x.png", { type: "image/png" }) await expect( api.uploadDocumentFile.mutateAsync({ documentId: "doc-1", id: "block-1", - loc: DocumentFileLocation.Comment, + loc: DocumentFileLocation.Document, + kind: DocumentFileKind.File, file, }), - ).rejects.toThrow("missing location header") + ).rejects.toThrow("missing upload response body") expect(uploadCalls).toHaveLength(1) - expect(uploadCalls[0]?.query).toEqual({ - id: "block-1", - location: "comment", - }) - expect(uploadCalls[0]?.fieldNames).toEqual(["file"]) }) }) }) diff --git a/web/app/composables/api/useDocumentFileAPI.ts b/web/app/composables/api/useDocumentFileAPI.ts index 9e230e13..8094b9ab 100644 --- a/web/app/composables/api/useDocumentFileAPI.ts +++ b/web/app/composables/api/useDocumentFileAPI.ts @@ -1,9 +1,19 @@ +export interface UploadedDocumentFile { + name: string + size: number + contentType: string +} + +// the address ends in "-" so it reads as the file it +// is; the server identifies the file by the fixed-length id alone export function buildDocumentFileSrc( documentId: string, blockId: string, + name: string, ): string { const { coreAPIBaseHttpURL } = useRuntimeConfig().public - return `${coreAPIBaseHttpURL}/api/documents/${documentId}/files/${blockId}` + + return `${coreAPIBaseHttpURL}/api/documents/${documentId}/files/${blockId}-${encodeURIComponent(name)}` } export default function () { @@ -14,30 +24,36 @@ export default function () { documentId, id, loc, + kind, file, }: { documentId: string loc: DocumentFileLocation + kind: DocumentFileKind id: string file: File - }) => { + }): Promise => { const body = new FormData() body.append("file", file) - const response = await $coreAPIClient.raw( - `/api/documents/${documentId}/files?id=${encodeURIComponent(id)}&location=${loc}`, + const response = await $coreAPIClient.raw( + `/api/documents/${documentId}/files?id=${encodeURIComponent(id)}&location=${loc}&kind=${kind}`, { method: "POST", body, }, ) - const location = response.headers.get("location") + const uploaded = response._data - if (!location) { - throw new Error("missing location header") + if (!uploaded) { + throw new Error("missing upload response body") } - return location + return { + name: uploaded.name, + size: uploaded.size, + contentType: uploaded.contentType, + } }, }) diff --git a/web/app/utils/api/document.ts b/web/app/utils/api/document.ts index 097900d2..0f433382 100644 --- a/web/app/utils/api/document.ts +++ b/web/app/utils/api/document.ts @@ -64,6 +64,22 @@ export enum DocumentFileLocation { Comment = "comment", } +// the block an upload belongs to, which decides the size and type rules +// the server admits it under +export enum DocumentFileKind { + Image = "image", + File = "file", +} + +export interface DocumentFileUpload { + id: string + // the file name the upload carried, as the server recorded it + name: string + size: number + // the media type the server detected from the bytes + contentType: string +} + export enum DocumentHookType { ScheduledReminder = "scheduled-reminder", GitHubTracking = "github-tracking", diff --git a/web/i18n/locales/en/editor.json b/web/i18n/locales/en/editor.json index 0fc972cd..9361b61f 100644 --- a/web/i18n/locales/en/editor.json +++ b/web/i18n/locales/en/editor.json @@ -144,6 +144,16 @@ "upload-failed": "Failed to upload the selected image. Please try again" } }, + "file": { + "description": "Upload a file", + "empty": "File not uploaded", + "uploading": "Uploading...", + "upload-unavailable": "Upload unavailable", + "errors": { + "upload-failed": "Failed to upload the selected file. Please try again", + "download-failed": "Failed to download the file. Please try again" + } + }, "mermaid": { "preview": { "loading": "The diagram is loading...", diff --git a/web/i18n/locales/en/general.json b/web/i18n/locales/en/general.json index ae342bdd..8bb11567 100644 --- a/web/i18n/locales/en/general.json +++ b/web/i18n/locales/en/general.json @@ -19,7 +19,7 @@ }, "size-limit-exceeded": { "title": "File too large", - "description": "The file exceeds the maximum allowed size of 5MB. Please upload a smaller file." + "description": "The file exceeds the maximum allowed size. Please upload a smaller file." } } }