From 04a19cbc6e228d02784d2cdcd236dd7f2470e96e Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 30 May 2026 20:51:46 +0000 Subject: [PATCH] Address artifact PR review feedback --- scripts/paperclip-upload-artifact.sh | 5 ++- .../__tests__/issue-attachment-routes.test.ts | 17 +++++++--- .../__tests__/storage-local-provider.test.ts | 20 +++++++++++ server/src/routes/issues.ts | 34 ++++--------------- server/src/storage/local-disk-provider.ts | 10 ++++-- server/src/storage/s3-provider.ts | 1 + server/src/storage/service.ts | 4 +-- server/src/storage/types.ts | 6 +++- 8 files changed, 59 insertions(+), 38 deletions(-) diff --git a/scripts/paperclip-upload-artifact.sh b/scripts/paperclip-upload-artifact.sh index c1e7999e..789c08be 100755 --- a/scripts/paperclip-upload-artifact.sh +++ b/scripts/paperclip-upload-artifact.sh @@ -125,16 +125,19 @@ upload_file() { local url="$1" local path="$2" local content_type="$3" + local escaped_path local response_file local status_code + escaped_path="${path//\\/\\\\}" + escaped_path="${escaped_path//\"/\\\"}" response_file="$(mktemp)" status_code="$( curl -sS -X POST -w '%{http_code}' -o "$response_file" \ "$url" \ -H "Authorization: Bearer $PAPERCLIP_API_KEY" \ -H "X-Paperclip-Run-Id: $PAPERCLIP_RUN_ID" \ - -F "file=@${path};type=${content_type}" + -F "file=@\"${escaped_path}\";type=${content_type}" )" if [[ "$status_code" -lt 200 || "$status_code" -ge 300 ]]; then diff --git a/server/src/__tests__/issue-attachment-routes.test.ts b/server/src/__tests__/issue-attachment-routes.test.ts index 1386c245..7b1f57a5 100644 --- a/server/src/__tests__/issue-attachment-routes.test.ts +++ b/server/src/__tests__/issue-attachment-routes.test.ts @@ -134,10 +134,14 @@ function createStorageService(body = Buffer.from("test")): TestStorageService { originalFilename: input.originalFilename, }; }, - getObject: vi.fn(async () => ({ - stream: Readable.from(body), - contentLength: body.length, - })), + getObject: vi.fn(async (_companyId, _objectKey, options) => { + const range = options?.range; + const streamBody = range ? body.subarray(range.start, range.end + 1) : body; + return { + stream: Readable.from(streamBody), + contentLength: streamBody.length, + }; + }), headObject: vi.fn(), deleteObject: vi.fn(), }; @@ -401,6 +405,11 @@ describe("issue attachment routes", () => { expect(res.headers["content-length"]).toBe("3"); expect(res.headers["content-disposition"]).toBe('inline; filename="clip.mp4"'); expect(Buffer.from(res.body).toString("utf8")).toBe("bcd"); + expect(storage.getObject).toHaveBeenCalledWith( + "company-1", + "issues/issue-1/clip.mp4", + { range: { start: 1, end: 3 } }, + ); }); it("forces video downloads when the download path is requested", async () => { diff --git a/server/src/__tests__/storage-local-provider.test.ts b/server/src/__tests__/storage-local-provider.test.ts index 9e9a55d1..b72cdbad 100644 --- a/server/src/__tests__/storage-local-provider.test.ts +++ b/server/src/__tests__/storage-local-provider.test.ts @@ -42,6 +42,26 @@ describe("local disk storage provider", () => { expect(stored.sha256).toHaveLength(64); }); + it("streams only requested byte ranges", async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-storage-")); + tempRoots.push(root); + + const service = createStorageService(createLocalDiskStorageProvider(root)); + const stored = await service.putFile({ + companyId: "company-1", + namespace: "issues/issue-1", + originalFilename: "demo.mp4", + contentType: "video/mp4", + body: Buffer.from("0123456789", "utf8"), + }); + + const fetched = await service.getObject("company-1", stored.objectKey, { range: { start: 2, end: 5 } }); + const fetchedBody = await readStreamToBuffer(fetched.stream); + + expect(fetchedBody.toString("utf8")).toBe("2345"); + expect(fetched.contentLength).toBe(4); + }); + it("blocks cross-company object access", async () => { const root = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-storage-")); tempRoots.push(root); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index b5b7251f..ba07190f 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1,5 +1,4 @@ import { randomUUID } from "node:crypto"; -import { Transform } from "node:stream"; import { Router, type Request, type Response } from "express"; import multer from "multer"; import { z } from "zod"; @@ -1166,27 +1165,6 @@ export function issueRoutes( return { kind: "range", start, end: Math.min(end, contentLength - 1) }; } - function createByteRangeStream(start: number, end: number) { - let offset = 0; - return new Transform({ - transform(chunk: Buffer | string, _encoding, callback) { - const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); - const chunkStart = offset; - const chunkEnd = offset + buffer.length - 1; - offset += buffer.length; - - if (chunkEnd < start || chunkStart > end) { - callback(); - return; - } - - const sliceStart = Math.max(start - chunkStart, 0); - const sliceEnd = Math.min(end - chunkStart + 1, buffer.length); - callback(null, buffer.subarray(sliceStart, sliceEnd)); - }, - }); - } - function parseBooleanQuery(value: unknown) { return value === true || value === "true" || value === "1"; } @@ -6284,7 +6262,11 @@ export function issueRoutes( return; } - const object = await storage.getObject(attachment.companyId, attachment.objectKey); + const object = await storage.getObject( + attachment.companyId, + attachment.objectKey, + range.kind === "range" ? { range: { start: range.start, end: range.end } } : undefined, + ); const responseContentType = normalizeContentType(attachment.contentType || object.contentType); res.setHeader("Content-Type", responseContentType); res.setHeader("Cache-Control", "private, max-age=60"); @@ -6306,11 +6288,7 @@ export function issueRoutes( res.status(206); res.setHeader("Content-Length", String(rangeLength)); res.setHeader("Content-Range", `bytes ${range.start}-${range.end}/${contentLength}`); - const rangeStream = createByteRangeStream(range.start, range.end); - rangeStream.on("error", (err) => { - next(err); - }); - object.stream.pipe(rangeStream).pipe(res); + object.stream.pipe(res); return; } diff --git a/server/src/storage/local-disk-provider.ts b/server/src/storage/local-disk-provider.ts index 30176832..56b35495 100644 --- a/server/src/storage/local-disk-provider.ts +++ b/server/src/storage/local-disk-provider.ts @@ -57,9 +57,15 @@ export function createLocalDiskStorageProvider(baseDir: string): StorageProvider if (!stat || !stat.isFile()) { throw notFound("Object not found"); } + const streamOptions = input.range + ? { start: input.range.start, end: input.range.end } + : undefined; + const contentLength = input.range + ? input.range.end - input.range.start + 1 + : stat.size; return { - stream: createReadStream(filePath), - contentLength: stat.size, + stream: createReadStream(filePath, streamOptions), + contentLength, lastModified: stat.mtime, }; }, diff --git a/server/src/storage/s3-provider.ts b/server/src/storage/s3-provider.ts index 3549289d..48c305bb 100644 --- a/server/src/storage/s3-provider.ts +++ b/server/src/storage/s3-provider.ts @@ -99,6 +99,7 @@ export function createS3StorageProvider(config: S3ProviderConfig): StorageProvid new GetObjectCommand({ Bucket: bucket, Key: key, + Range: input.range ? `bytes=${input.range.start}-${input.range.end}` : undefined, }), ); diff --git a/server/src/storage/service.ts b/server/src/storage/service.ts index 6401c415..bccdfc9f 100644 --- a/server/src/storage/service.ts +++ b/server/src/storage/service.ts @@ -113,9 +113,9 @@ export function createStorageService(provider: StorageProvider): StorageService }; }, - async getObject(companyId: string, objectKey: string) { + async getObject(companyId: string, objectKey: string, options) { ensureCompanyPrefix(companyId, objectKey); - return provider.getObject({ objectKey }); + return provider.getObject({ objectKey, range: options?.range }); }, async headObject(companyId: string, objectKey: string) { diff --git a/server/src/storage/types.ts b/server/src/storage/types.ts index 22325f23..efeadd8b 100644 --- a/server/src/storage/types.ts +++ b/server/src/storage/types.ts @@ -10,6 +10,10 @@ export interface PutObjectInput { export interface GetObjectInput { objectKey: string; + range?: { + start: number; + end: number; + }; } export interface GetObjectResult { @@ -56,7 +60,7 @@ export interface PutFileResult { export interface StorageService { provider: StorageProviderId; putFile(input: PutFileInput): Promise; - getObject(companyId: string, objectKey: string): Promise; + getObject(companyId: string, objectKey: string, options?: Pick): Promise; headObject(companyId: string, objectKey: string): Promise; deleteObject(companyId: string, objectKey: string): Promise; }