Skip to content

Commit 055c508

Browse files
jacobsfletchjacobsfletch
authored
fix(plugin-cloud-storage): propagate custom hook errors during upload (#16632)
Identical to #16592, back-ported for 3.x.
1 parent 4baba91 commit 055c508

7 files changed

Lines changed: 427 additions & 44 deletions

File tree

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

Lines changed: 59 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -21,32 +21,6 @@ export const getAfterChangeHook =
2121
const files = getIncomingFiles({ data: doc, req })
2222

2323
if (files.length > 0) {
24-
// If there is a previous doc, files and the operation is update,
25-
// delete the old files before uploading the new ones.
26-
if (previousDoc && operation === 'update') {
27-
let filesToDelete: string[] = []
28-
29-
if (typeof previousDoc?.filename === 'string') {
30-
filesToDelete.push(previousDoc.filename)
31-
}
32-
33-
if (typeof previousDoc.sizes === 'object') {
34-
filesToDelete = filesToDelete.concat(
35-
Object.values(previousDoc?.sizes || []).map(
36-
(resizedFileData) => resizedFileData?.filename as string,
37-
),
38-
)
39-
}
40-
41-
const deletionPromises = filesToDelete.map(async (filename) => {
42-
if (filename) {
43-
await adapter.handleDelete({ collection, doc: previousDoc, filename, req })
44-
}
45-
})
46-
47-
await Promise.all(deletionPromises)
48-
}
49-
5024
const uploadResults = await Promise.all(
5125
files
5226
.filter((file) => !file.clientUploadContext)
@@ -71,31 +45,78 @@ export const getAfterChangeHook =
7145
{} as Partial<FileData & TypeWithID>,
7246
)
7347

48+
let docWithMetadata = doc
49+
7450
if (Object.keys(uploadMetadata).length > 0) {
75-
try {
76-
if (!req.context) {
77-
req.context = {}
78-
}
79-
req.context.skipCloudStorage = true
51+
if (!req.context) {
52+
req.context = {}
53+
}
54+
req.context.skipCloudStorage = true
8055

81-
// Clear to prevent re-processing
82-
req.file = undefined
83-
req.payloadUploadSizes = undefined
56+
// Clear to prevent re-processing
57+
req.file = undefined
58+
req.payloadUploadSizes = undefined
8459

60+
try {
8561
await req.payload.update({
8662
id: doc.id,
8763
collection: collection.slug,
8864
data: uploadMetadata,
8965
depth: 0,
9066
req,
9167
})
68+
} finally {
9269
delete req.context.skipCloudStorage
93-
return { ...doc, ...uploadMetadata }
94-
} catch (updateError: unknown) {
95-
req.payload.logger.warn(
96-
`Failed to persist upload data for collection ${collection.slug} document ${doc.id}: ${String(updateError)}`,
70+
}
71+
72+
docWithMetadata = { ...doc, ...uploadMetadata }
73+
}
74+
75+
// Delete previous files only after the new upload and metadata
76+
// persistence have succeeded. Deleting earlier would orphan the
77+
// record if a later step throws (e.g. a user-defined afterChange
78+
// hook on the same collection).
79+
if (previousDoc && operation === 'update') {
80+
let filesToDelete: string[] = []
81+
82+
if (typeof previousDoc?.filename === 'string') {
83+
filesToDelete.push(previousDoc.filename)
84+
}
85+
86+
if (typeof previousDoc.sizes === 'object') {
87+
filesToDelete = filesToDelete.concat(
88+
Object.values(previousDoc?.sizes || []).map(
89+
(resizedFileData) => resizedFileData?.filename as string,
90+
),
9791
)
9892
}
93+
94+
// Collect new filenames (main + sizes) so we don't delete a
95+
// file that the new upload reused (e.g. same filename on reupload
96+
// where Payload overwrites in place).
97+
const newFilenames = new Set<string>()
98+
if (typeof docWithMetadata.filename === 'string') {
99+
newFilenames.add(docWithMetadata.filename)
100+
}
101+
if (typeof docWithMetadata.sizes === 'object') {
102+
for (const size of Object.values(docWithMetadata.sizes || {})) {
103+
if (size?.filename && typeof size.filename === 'string') {
104+
newFilenames.add(size.filename)
105+
}
106+
}
107+
}
108+
109+
const deletionPromises = filesToDelete.map(async (filename) => {
110+
if (filename && !newFilenames.has(filename)) {
111+
await adapter.handleDelete({ collection, doc: previousDoc, filename, req })
112+
}
113+
})
114+
115+
await Promise.all(deletionPromises)
116+
}
117+
118+
if (docWithMetadata !== doc) {
119+
return docWithMetadata
99120
}
100121
}
101122
} catch (err: unknown) {

‎test/plugin-cloud-storage/buildPluginCloudStorageIntConfig.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ import { Media } from './collections/Media.js'
1515
import { MediaWithCompositePrefixes } from './collections/MediaWithCompositePrefixes.js'
1616
import { MediaWithCustomURL } from './collections/MediaWithCustomURL.js'
1717
import { MediaWithGenerateFileURL } from './collections/MediaWithGenerateFileURL.js'
18+
import { MediaWithOverwrite } from './collections/MediaWithOverwrite.js'
1819
import { MediaWithPrefix } from './collections/MediaWithPrefix.js'
20+
import { MediaWithThrowingHook } from './collections/MediaWithThrowingHook.js'
1921
import { RestrictedMedia } from './collections/RestrictedMedia.js'
2022
import { TestMetadata } from './collections/TestMetadata.js'
2123
import { Users } from './collections/Users.js'
@@ -25,7 +27,9 @@ import {
2527
mediaWithCompositePrefixesSlug,
2628
mediaWithCustomURLSlug,
2729
mediaWithGenerateFileURLSlug,
30+
mediaWithOverwriteSlug,
2831
mediaWithPrefixSlug,
32+
mediaWithThrowingHookSlug,
2933
prefix,
3034
restrictedMediaSlug,
3135
testMetadataSlug,
@@ -113,9 +117,11 @@ export function buildPluginCloudStorageIntConfig({
113117
: null,
114118
prefix,
115119
} as S3StorageOptions['collections'][keyof S3StorageOptions['collections']],
120+
[mediaWithOverwriteSlug]: true,
116121
[mediaWithPrefixSlug]: {
117122
prefix,
118123
},
124+
[mediaWithThrowingHookSlug]: true,
119125
[restrictedMediaSlug]: true,
120126
},
121127
config: {
@@ -188,7 +194,9 @@ export function buildPluginCloudStorageIntConfig({
188194
MediaWithCompositePrefixes,
189195
MediaWithCustomURL,
190196
MediaWithGenerateFileURL,
197+
MediaWithOverwrite,
191198
MediaWithPrefix,
199+
MediaWithThrowingHook,
192200
RestrictedMedia,
193201
TestMetadata,
194202
Users,

‎test/plugin-cloud-storage/collections/MediaWithOverwrite.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import type { CollectionConfig } from 'payload'
2+
3+
import { mediaWithOverwriteSlug } from '../shared.js'
4+
5+
export const MediaWithOverwrite: CollectionConfig = {
6+
slug: mediaWithOverwriteSlug,
7+
upload: {
8+
disableLocalStorage: true,
9+
resizeOptions: {
10+
position: 'center',
11+
width: 200,
12+
height: 200,
13+
},
14+
imageSizes: [
15+
{
16+
height: 400,
17+
width: 400,
18+
crop: 'center',
19+
name: 'square',
20+
},
21+
{
22+
width: 900,
23+
height: 450,
24+
crop: 'center',
25+
name: 'sixteenByNineMedium',
26+
},
27+
],
28+
},
29+
fields: [
30+
{
31+
name: 'alt',
32+
label: 'Alt Text',
33+
type: 'text',
34+
},
35+
],
36+
}

‎test/plugin-cloud-storage/collections/MediaWithThrowingHook.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
import { APIError, type CollectionConfig } from 'payload'
2+
3+
import { mediaWithThrowingHookSlug } from '../shared.js'
4+
5+
export const throwingHookError = 'User afterChange hook throws error'
6+
7+
export const MediaWithThrowingHook: CollectionConfig = {
8+
slug: mediaWithThrowingHookSlug,
9+
access: {
10+
create: () => true,
11+
delete: () => true,
12+
read: () => true,
13+
update: () => true,
14+
},
15+
fields: [
16+
{
17+
name: 'alt',
18+
type: 'text',
19+
},
20+
{
21+
name: 'shouldThrow',
22+
type: 'checkbox',
23+
admin: {
24+
description:
25+
'When enabled, the afterChange hook throws during the cloud-storage plugin internal update. Used to reproduce the swallowed-error bug in the admin panel and integration tests.',
26+
},
27+
defaultValue: false,
28+
},
29+
],
30+
hooks: {
31+
afterChange: [
32+
({ doc, operation, req }) => {
33+
if (operation === 'update' && req.context?.skipCloudStorage && doc.shouldThrow) {
34+
throw new APIError(throwingHookError, 500, null, true)
35+
}
36+
37+
return doc
38+
},
39+
],
40+
},
41+
upload: {
42+
disableLocalStorage: true,
43+
},
44+
}

0 commit comments

Comments
 (0)