Skip to content

Commit a6735f3

Browse files
authored
fix(storage-*): redundant re-uploads when file was already uploaded by the client and add comprehensive E2E tests for s3 and vercel-blob (#16115)
## Summary - Restructures `clientUploads.config.ts` → `client-uploads/config.ts` subdirectory for both `storage-s3` and `storage-vercel-blob`, enabling `pnpm run dev storage-s3/client-uploads` to work correctly - Moves integration tests into the same subdirectory - Adds a `MediaContainer` collection (hasMany upload → media) to support bulk upload drawer testing - Adds E2E test suites (`e2e.spec.ts`) for both adapters that verify via network interception that file bytes never reach the Payload server - only a JSON descriptor is POSTed after the browser uploads directly to S3/Vercel Blob - Configures S3 CORS on the localstack bucket via an init script (`test/localstack-init/ready.d/01-s3-cors.sh`) mounted in `docker-compose.yml`, enabling cross-origin browser PUTs from `localhost:3000` to `localhost:4566` Change in **`packages/plugin-cloud-storage/src/hooks/afterChange.ts`**: When a file was client-uploaded (browser → storage directly), the `afterChange` hook called `adapter.handleUpload` on the original file a second time, triggering a redundant server-side upload. For Vercel Blob this caused a `BlobUnknownError` (file already exists); for S3 it caused a silent duplicate PUT. Fix: filter out files where `file.clientUploadContext` is set before calling `handleUpload`. Image resize variants generated server-side by Payload/sharp do not have `clientUploadContext` and continue to be uploaded server-side as before - **size variants still work with this** ## E2E Testing Each suite (`storage-s3/client-uploads/e2e.spec.ts`, `storage-vercel-blob/client-uploads/e2e.spec.ts`) covers three scenarios: 1. Single upload completes successfully via the admin UI 2. **Network verification (single)** — intercepts all browser requests; asserts no request to `localhost:3000` carries file bytes (content-length > 10 KB), and at least one PUT/request goes to the storage endpoint - important as on Vercel if we have a request like the upload will fail. 3. **Network verification (field bulk upload)** — same assertions for the bulk-upload drawer opened from a `hasMany` upload relationship field (`MediaContainer.files`); asserts two items appear in the field after the drawer closes 4. **Network verification (list view bulk upload)** — same network assertions for the "Bulk Upload" button in the collection list-view header (`#bulk-upload-drawer-slug-1`), which is a separate entry point from the field-level drawer --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1213884972829065
1 parent e09c619 commit a6735f3

15 files changed

Lines changed: 1471 additions & 99 deletions

File tree

.github/workflows/e2e.config.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ export default createE2EConfig([
8181
{ file: 'locked-documents', shards: 1 },
8282
{ file: 'i18n', shards: 1 },
8383
{ file: 'plugin-cloud-storage', shards: 1 },
84+
{ file: 'storage-s3__client-uploads#client-uploads/config.ts', shards: 1 },
85+
{ file: 'storage-vercel-blob__client-uploads#client-uploads/config.ts', shards: 1 },
8486
{ file: 'plugin-form-builder', shards: 1 },
8587
{ file: 'plugin-import-export', shards: 1 },
8688
{ file: 'plugin-multi-tenant', shards: 2 },

.github/workflows/main.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ jobs:
331331

332332
- name: Start LocalStack
333333
run: pnpm docker:start
334-
if: contains(fromJson('["plugin-cloud-storage", "plugin-import-export"]'), matrix.suite)
334+
if: contains(fromJson('["plugin-cloud-storage", "plugin-import-export"]'), matrix.suite) || contains(matrix.suite, 'storage-s3__client-uploads') || contains(matrix.suite, 'storage-vercel-blob__client-uploads')
335335

336336
- name: Start database
337337
id: db
@@ -512,7 +512,7 @@ jobs:
512512

513513
- name: Start LocalStack
514514
run: pnpm docker:start
515-
if: contains(fromJson('["plugin-cloud-storage", "plugin-import-export"]'), matrix.suite)
515+
if: contains(fromJson('["plugin-cloud-storage", "plugin-import-export"]'), matrix.suite) || contains(matrix.suite, 'storage-s3__client-uploads') || contains(matrix.suite, 'storage-vercel-blob__client-uploads')
516516

517517
- name: Store Playwright's Version
518518
run: |

packages/plugin-cloud-storage/src/hooks/afterChange.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,17 @@ export const getAfterChangeHook =
4848
}
4949

5050
const uploadResults = await Promise.all(
51-
files.map((file) =>
52-
adapter.handleUpload({
53-
clientUploadContext: file.clientUploadContext,
54-
collection,
55-
data: doc,
56-
file,
57-
req,
58-
}),
59-
),
51+
files
52+
.filter((file) => !file.clientUploadContext)
53+
.map((file) =>
54+
adapter.handleUpload({
55+
clientUploadContext: file.clientUploadContext,
56+
collection,
57+
data: doc,
58+
file,
59+
req,
60+
}),
61+
),
6062
)
6163

6264
const uploadMetadata = uploadResults

test/docker-compose.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ services:
179179
volumes:
180180
- localstack_data:/var/lib/localstack
181181
- '/var/run/docker.sock:/var/run/docker.sock'
182+
- './localstack-init/ready.d:/etc/localstack/init/ready.d'
182183

183184
# ── Azure Storage (Azurite emulator) ────────────────────────
184185
azure-storage:
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
#!/bin/bash
2+
# Configure CORS on the S3 bucket so browsers can PUT files directly to localstack.
3+
# Runs automatically when LocalStack is ready (mounted at /etc/localstack/init/ready.d/).
4+
5+
awslocal s3api create-bucket --bucket payload-bucket --region us-east-1 2>/dev/null || true
6+
7+
awslocal s3api put-bucket-cors --bucket payload-bucket --cors-configuration '{
8+
"CORSRules": [
9+
{
10+
"AllowedHeaders": ["*"],
11+
"AllowedMethods": ["GET", "PUT", "HEAD", "DELETE"],
12+
"AllowedOrigins": ["*"],
13+
"ExposeHeaders": ["ETag"],
14+
"MaxAgeSeconds": 3600
15+
}
16+
]
17+
}'
18+
19+
echo "S3 CORS configured on payload-bucket"
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import type { CollectionConfig } from 'payload'
2+
3+
export const MediaContainer: CollectionConfig = {
4+
slug: 'media-container',
5+
fields: [
6+
{
7+
name: 'files',
8+
type: 'upload',
9+
relationTo: 'media',
10+
hasMany: true,
11+
},
12+
],
13+
}
Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,28 +3,28 @@ import dotenv from 'dotenv'
33
import { fileURLToPath } from 'node:url'
44
import path from 'path'
55

6-
import { buildConfigWithDefaults } from '../buildConfigWithDefaults.js'
7-
import { devUser } from '../credentials.js'
8-
import { Media } from './collections/Media.js'
9-
import { MediaWithPrefix } from './collections/MediaWithPrefix.js'
10-
import { Users } from './collections/Users.js'
11-
import { mediaSlug, mediaWithPrefixSlug } from './shared.js'
12-
import { MB } from './test-utils.js'
6+
import { buildConfigWithDefaults } from '../../buildConfigWithDefaults.js'
7+
import { devUser } from '../../credentials.js'
8+
import { Media } from '../collections/Media.js'
9+
import { MediaWithPrefix } from '../collections/MediaWithPrefix.js'
10+
import { Users } from '../collections/Users.js'
11+
import { mediaSlug, mediaWithPrefixSlug } from '../shared.js'
12+
import { MediaContainer } from './collections/MediaContainer.js'
13+
1314
const filename = fileURLToPath(import.meta.url)
1415
const dirname = path.dirname(filename)
1516

16-
// Load config to work with emulated services
1717
dotenv.config({
18-
path: path.resolve(dirname, '../plugin-cloud-storage/.env.emulated'),
18+
path: path.resolve(dirname, '../../plugin-cloud-storage/.env.emulated'),
1919
})
2020

2121
export default buildConfigWithDefaults({
2222
admin: {
2323
importMap: {
24-
baseDir: path.resolve(dirname),
24+
baseDir: path.resolve(dirname, '..'),
2525
},
2626
},
27-
collections: [Media, MediaWithPrefix, Users],
27+
collections: [Media, MediaWithPrefix, MediaContainer, Users],
2828
onInit: async (payload) => {
2929
await payload.create({
3030
collection: 'users',
@@ -59,8 +59,8 @@ export default buildConfigWithDefaults({
5959
],
6060
upload: {
6161
limits: {
62-
fileSize: MB(10),
63-
}, // 10 mb
62+
fileSize: 10 * 1024 * 1024, // 10 MB
63+
},
6464
},
6565
typescript: {
6666
outputFile: path.resolve(dirname, 'payload-types.ts'),
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
import type { Page } from '@playwright/test'
2+
3+
import { expect, test } from '@playwright/test'
4+
import dotenv from 'dotenv'
5+
import * as path from 'path'
6+
import { fileURLToPath } from 'url'
7+
8+
import {
9+
ensureCompilationIsDone,
10+
exactText,
11+
saveDocAndAssert,
12+
} from '../../__helpers/e2e/helpers.js'
13+
import { AdminUrlUtil } from '../../__helpers/shared/adminUrlUtil.js'
14+
import { initPayloadE2ENoConfig } from '../../__helpers/shared/initPayloadE2ENoConfig.js'
15+
import { TEST_TIMEOUT_LONG } from '../../playwright.config.js'
16+
import { mediaSlug } from '../shared.js'
17+
18+
const filename = fileURLToPath(import.meta.url)
19+
const dirname = path.dirname(filename)
20+
21+
dotenv.config({ path: path.resolve(dirname, '../../plugin-cloud-storage/.env.emulated') })
22+
23+
// e.g. "localhost:4566" — used to detect file upload requests going directly to S3
24+
const s3Endpoint =
25+
process.env.S3_ENDPOINT ?? process.env.AWS_ENDPOINT_URL ?? process.env.AWS_ENDPOINT_URL_S3
26+
27+
if (!s3Endpoint) {
28+
throw new Error('Missing S3 endpoint env (S3_ENDPOINT, AWS_ENDPOINT_URL, or AWS_ENDPOINT_URL_S3)')
29+
}
30+
31+
const s3Host = new URL(s3Endpoint).host
32+
// image.png is 89 KB — any request with content-length above this threshold is a file upload
33+
const FILE_SIZE_THRESHOLD = 1_000
34+
35+
const mediaContainerSlug = 'media-container'
36+
37+
test.describe('storage-s3 client uploads E2E', () => {
38+
let page: Page
39+
let mediaURL: AdminUrlUtil
40+
let mediaContainerURL: AdminUrlUtil
41+
let payloadHost: string
42+
43+
test.beforeAll(async ({ browser }, testInfo) => {
44+
testInfo.setTimeout(TEST_TIMEOUT_LONG)
45+
const { serverURL } = await initPayloadE2ENoConfig({ dirname })
46+
47+
payloadHost = new URL(serverURL).host
48+
mediaURL = new AdminUrlUtil(serverURL, mediaSlug)
49+
mediaContainerURL = new AdminUrlUtil(serverURL, mediaContainerSlug)
50+
51+
const context = await browser.newContext()
52+
page = await context.newPage()
53+
await ensureCompilationIsDone({ page, serverURL })
54+
})
55+
56+
test('should complete a single client upload via the admin UI', async () => {
57+
await page.goto(mediaURL.create)
58+
await page.setInputFiles('input[type="file"]', path.resolve(dirname, '../../uploads/image.png'))
59+
await expect(page.locator('.file-field__filename')).toHaveValue('image.png')
60+
await saveDocAndAssert(page)
61+
})
62+
63+
test('should upload file directly to S3, not through the Payload server', async ({ browser }) => {
64+
const context = await browser.newContext()
65+
const testPage = await context.newPage()
66+
67+
const largeRequestsToPayload: string[] = []
68+
const putsToS3: string[] = []
69+
70+
testPage.on('request', (request) => {
71+
const url = request.url()
72+
const contentLength = parseInt(request.headers()['content-length'] ?? '0', 10)
73+
74+
if (new URL(url).host === payloadHost && contentLength > FILE_SIZE_THRESHOLD) {
75+
largeRequestsToPayload.push(`${request.method()} ${url} (${contentLength} bytes)`)
76+
}
77+
if (new URL(url).host === s3Host && request.method() === 'PUT') {
78+
putsToS3.push(url)
79+
}
80+
})
81+
82+
await testPage.goto(mediaURL.create)
83+
await testPage.setInputFiles(
84+
'input[type="file"]',
85+
path.resolve(dirname, '../../uploads/image.png'),
86+
)
87+
await saveDocAndAssert(testPage)
88+
89+
expect(
90+
largeRequestsToPayload,
91+
`File bytes were sent to Payload: ${largeRequestsToPayload.join(', ')}`,
92+
).toHaveLength(0)
93+
94+
expect(
95+
putsToS3.length,
96+
'Expected at least one PUT request to the S3 endpoint',
97+
).toBeGreaterThanOrEqual(1)
98+
99+
await context.close()
100+
})
101+
102+
test('should bulk upload multiple files directly to S3, not through Payload', async ({
103+
browser,
104+
}) => {
105+
const context = await browser.newContext()
106+
const testPage = await context.newPage()
107+
108+
const largeRequestsToPayload: string[] = []
109+
const putsToS3: string[] = []
110+
111+
testPage.on('request', (request) => {
112+
const url = request.url()
113+
const contentLength = parseInt(request.headers()['content-length'] ?? '0', 10)
114+
115+
if (new URL(url).host === payloadHost && contentLength > FILE_SIZE_THRESHOLD) {
116+
largeRequestsToPayload.push(`${request.method()} ${url} (${contentLength} bytes)`)
117+
}
118+
if (new URL(url).host === s3Host && request.method() === 'PUT') {
119+
putsToS3.push(url)
120+
}
121+
})
122+
123+
await testPage.goto(mediaContainerURL.create)
124+
125+
const createNewButton = testPage.locator('#field-files button', {
126+
hasText: exactText('Create New'),
127+
})
128+
await expect(createNewButton).toBeVisible()
129+
await expect(createNewButton).toBeEnabled()
130+
await createNewButton.click()
131+
132+
const bulkUploadModal = testPage.locator('#files-bulk-upload-drawer-slug-1')
133+
await expect(bulkUploadModal).toBeVisible()
134+
135+
await bulkUploadModal
136+
.locator('.dropzone input[type="file"]')
137+
.setInputFiles([
138+
path.resolve(dirname, '../../uploads/image.png'),
139+
path.resolve(dirname, '../../uploads/test-image.png'),
140+
])
141+
142+
const saveButton = bulkUploadModal.locator('.bulk-upload--actions-bar__saveButtons button')
143+
await saveButton.click()
144+
145+
await expect(bulkUploadModal).toBeHidden({ timeout: 30_000 })
146+
147+
expect(putsToS3.length, 'Expected one PUT to S3 per uploaded file').toBeGreaterThanOrEqual(2)
148+
149+
expect(
150+
largeRequestsToPayload,
151+
`File bytes were sent to Payload: ${largeRequestsToPayload.join(', ')}`,
152+
).toHaveLength(0)
153+
154+
const items = testPage.locator('#field-files .upload--has-many__dragItem')
155+
await expect(items).toHaveCount(2)
156+
157+
await context.close()
158+
})
159+
160+
test('should bulk upload files from the list view directly to S3, not through Payload', async ({
161+
browser,
162+
}) => {
163+
const context = await browser.newContext()
164+
const testPage = await context.newPage()
165+
166+
const largeRequestsToPayload: string[] = []
167+
const putsToS3: string[] = []
168+
169+
testPage.on('request', (request) => {
170+
const url = request.url()
171+
const contentLength = parseInt(request.headers()['content-length'] ?? '0', 10)
172+
173+
if (new URL(url).host === payloadHost && contentLength > FILE_SIZE_THRESHOLD) {
174+
largeRequestsToPayload.push(`${request.method()} ${url} (${contentLength} bytes)`)
175+
}
176+
if (new URL(url).host === s3Host && request.method() === 'PUT') {
177+
putsToS3.push(url)
178+
}
179+
})
180+
181+
await testPage.goto(mediaURL.list)
182+
await expect(testPage.locator('.list-header__title')).toBeVisible()
183+
184+
const bulkUploadButton = testPage.locator('.list-header__title-actions button', {
185+
hasText: 'Bulk Upload',
186+
})
187+
await expect(bulkUploadButton).toBeEnabled()
188+
189+
// Click and retry until dropzone appears (handles hydration timing)
190+
const dropzoneInput = testPage.locator('.dropzone input[type="file"]')
191+
await expect(async () => {
192+
await bulkUploadButton.click()
193+
await expect(dropzoneInput).toBeAttached({ timeout: 1500 })
194+
}).toPass({ timeout: 5000, intervals: [500] })
195+
196+
await testPage.setInputFiles('.dropzone input[type="file"]', [
197+
path.resolve(dirname, '../../uploads/image.png'),
198+
path.resolve(dirname, '../../uploads/test-image.png'),
199+
])
200+
201+
const bulkUploadModal = testPage.locator('#media-bulk-upload-drawer-slug-1')
202+
const saveButton = bulkUploadModal.locator('.bulk-upload--actions-bar__saveButtons button')
203+
await expect(saveButton).toBeVisible()
204+
await saveButton.click()
205+
206+
await expect(bulkUploadModal).toBeHidden({ timeout: 30_000 })
207+
208+
expect(putsToS3.length, 'Expected one PUT to S3 per uploaded file').toBeGreaterThanOrEqual(2)
209+
210+
expect(
211+
largeRequestsToPayload,
212+
`File bytes were sent to Payload: ${largeRequestsToPayload.join(', ')}`,
213+
).toHaveLength(0)
214+
215+
await context.close()
216+
})
217+
})

0 commit comments

Comments
 (0)