From 7c55b94280b89cc2612c8b4fa5d93d60203b3259 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 31 Jul 2024 06:20:36 -0700 Subject: [PATCH] fix(trace): make sure the correct attachment name is used for downloads (#31928) When two attachments have the same content sha1, we used the first one's name for the downloaded file, no matter which one the user clicked to download. Now we pass the name explicitly. References #31912. --- packages/trace-viewer/src/sw.ts | 16 +++++------ packages/trace-viewer/src/traceModel.ts | 8 +----- packages/trace-viewer/src/traceModernizer.ts | 6 +---- .../trace-viewer/src/ui/attachmentsTab.tsx | 27 +++++++++++++------ .../ui-mode-test-attachments.spec.ts | 25 ++++++++++++----- 5 files changed, 48 insertions(+), 34 deletions(-) diff --git a/packages/trace-viewer/src/sw.ts b/packages/trace-viewer/src/sw.ts index 14d26c2a36..7888aa6a30 100644 --- a/packages/trace-viewer/src/sw.ts +++ b/packages/trace-viewer/src/sw.ts @@ -130,13 +130,12 @@ async function doFetch(event: FetchEvent): Promise { } if (relativePath.startsWith('/sha1/')) { - const download = url.searchParams.has('download'); // Sha1 for sources is based on the file path, can't load it of a random model. const sha1 = relativePath.slice('/sha1/'.length); for (const trace of loadedTraces.values()) { const blob = await trace.traceModel.resourceForSha1(sha1); if (blob) - return new Response(blob, { status: 200, headers: download ? downloadHeadersForAttachment(trace.traceModel, sha1) : undefined }); + return new Response(blob, { status: 200, headers: downloadHeaders(url.searchParams) }); } return new Response(null, { status: 404 }); } @@ -157,14 +156,15 @@ async function doFetch(event: FetchEvent): Promise { return snapshotServer.serveResource(lookupUrls, request.method, snapshotUrl); } -function downloadHeadersForAttachment(traceModel: TraceModel, sha1: string): Headers | undefined { - const attachment = traceModel.attachmentForSha1(sha1); - if (!attachment) +function downloadHeaders(searchParams: URLSearchParams): Headers | undefined { + const name = searchParams.get('dn'); + const contentType = searchParams.get('dct'); + if (!name) return; const headers = new Headers(); - headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(attachment.name)}`); - if (attachment.contentType) - headers.set('Content-Type', attachment.contentType); + headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(name)}`); + if (contentType) + headers.set('Content-Type', contentType); return headers; } diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 54a77b5aa8..0fc1a73efa 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/packages/trace-viewer/src/traceModel.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import type * as trace from '@trace/trace'; import { parseClientSideCallMetadata } from '../../../packages/playwright-core/src/utils/isomorphic/traceUtils'; import type { ContextEntry } from './entries'; import { createEmptyContext } from './entries'; @@ -34,7 +33,6 @@ export class TraceModel { contextEntries: ContextEntry[] = []; private _snapshotStorage: SnapshotStorage | undefined; private _backend!: TraceModelBackend; - private _attachments = new Map(); private _resourceToContentType = new Map(); constructor() { @@ -64,7 +62,7 @@ export class TraceModel { const contextEntry = createEmptyContext(); contextEntry.traceUrl = backend.traceURL(); contextEntry.hasSource = hasSource; - const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage, this._attachments); + const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage); const trace = await this._backend.readText(ordinal + '.trace') || ''; modernizer.appendTrace(trace); @@ -121,10 +119,6 @@ export class TraceModel { return new Blob([blob], { type: this._resourceToContentType.get(sha1) || 'application/octet-stream' }); } - attachmentForSha1(sha1: string): trace.AfterActionTraceEventAttachment | undefined { - return this._attachments.get(sha1); - } - storage(): SnapshotStorage { return this._snapshotStorage!; } diff --git a/packages/trace-viewer/src/traceModernizer.ts b/packages/trace-viewer/src/traceModernizer.ts index e7c65ac41f..b15ad527b0 100644 --- a/packages/trace-viewer/src/traceModernizer.ts +++ b/packages/trace-viewer/src/traceModernizer.ts @@ -34,17 +34,15 @@ const latestVersion: trace.VERSION = 7; export class TraceModernizer { private _contextEntry: ContextEntry; private _snapshotStorage: SnapshotStorage; - private _attachments: Map; private _actionMap = new Map(); private _version: number | undefined; private _pageEntries = new Map(); private _jsHandles = new Map(); private _consoleObjects = new Map(); - constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage, attachments: Map) { + constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage) { this._contextEntry = contextEntry; this._snapshotStorage = snapshotStorage; - this._attachments = attachments; } appendTrace(trace: string) { @@ -129,8 +127,6 @@ export class TraceModernizer { existing!.attachments = event.attachments; if (event.point) existing!.point = event.point; - for (const attachment of event.attachments?.filter(a => a.sha1) || []) - this._attachments.set(attachment.sha1!, attachment); break; } case 'action': { diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx index c4dbbb3c16..13bdbcefcb 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.tsx +++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx @@ -50,7 +50,7 @@ const ExpandableAttachment: React.FunctionComponent = }, [expanded, attachmentText, placeholder, attachment]); const title = - {attachment.name} download + {attachment.name} download ; if (!isTextAttachment) @@ -111,9 +111,9 @@ export const AttachmentsTab: React.FunctionComponent<{ {expected && actual &&
Image diff
} {expected && actual && } ; })} @@ -134,8 +134,19 @@ export const AttachmentsTab: React.FunctionComponent<{ ; }; -function attachmentURL(attachment: Attachment) { - if (attachment.sha1) - return 'sha1/' + attachment.sha1 + '?trace=' + encodeURIComponent(attachment.traceUrl); - return 'file?path=' + encodeURIComponent(attachment.path!); +function attachmentURL(attachment: Attachment, queryParams: Record = {}) { + const params = new URLSearchParams(queryParams); + if (attachment.sha1) { + params.set('trace', attachment.traceUrl); + return 'sha1/' + attachment.sha1 + '?' + params.toString(); + } + params.set('path', attachment.path!); + return 'file?' + params.toString(); +} + +function downloadURL(attachment: Attachment) { + const params = { dn: attachment.name } as Record; + if (attachment.contentType) + params.dct = attachment.contentType; + return attachmentURL(attachment, params); } diff --git a/tests/playwright-test/ui-mode-test-attachments.spec.ts b/tests/playwright-test/ui-mode-test-attachments.spec.ts index f3c8ac627b..0b04cce012 100644 --- a/tests/playwright-test/ui-mode-test-attachments.spec.ts +++ b/tests/playwright-test/ui-mode-test-attachments.spec.ts @@ -23,7 +23,10 @@ test('should contain text attachment', async ({ runUITest }) => { 'a.test.ts': ` import { test } from '@playwright/test'; test('attach test', async () => { + // Attach two files with the same content and different names, + // to make sure each is downloaded with an intended name. await test.info().attach('file attachment', { path: __filename }); + await test.info().attach('file attachment 2', { path: __filename }); await test.info().attach('text attachment', { body: 'hi tester!', contentType: 'text/plain' }); }); `, @@ -35,14 +38,24 @@ test('should contain text attachment', async ({ runUITest }) => { await page.locator('.tab-attachments').getByText('text attachment').click(); await expect(page.locator('.tab-attachments')).toContainText('hi tester!'); - await page.locator('.tab-attachments').getByText('file attachment').click(); + await page.locator('.tab-attachments').getByText('file attachment').first().click(); await expect(page.locator('.tab-attachments')).not.toContainText('attach test'); - const downloadPromise = page.waitForEvent('download'); - await page.getByRole('link', { name: 'download' }).first().click(); - const download = await downloadPromise; - expect(download.suggestedFilename()).toBe('file attachment'); - expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + { + const downloadPromise = page.waitForEvent('download'); + await page.getByRole('link', { name: 'download' }).first().click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('file attachment'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + } + + { + const downloadPromise = page.waitForEvent('download'); + await page.getByRole('link', { name: 'download' }).nth(1).click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('file attachment 2'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + } }); test('should contain binary attachment', async ({ runUITest }) => {