Skip to content

refactor(cloudinary)!: derive publicId from filename, align with storage-s3#149

Draft
jhb-dev wants to merge 1 commit into
mainfrom
refactor/cloudinary-stateless-like-s3
Draft

refactor(cloudinary)!: derive publicId from filename, align with storage-s3#149
jhb-dev wants to merge 1 commit into
mainfrom
refactor/cloudinary-stateless-like-s3

Conversation

@jhb-dev
Copy link
Copy Markdown
Contributor

@jhb-dev jhb-dev commented May 18, 2026

Summary

Alternative to #148 that removes the workaround entirely by aligning this plugin with the stateless model used by @payloadcms/storage-s3. The plugin no longer depends on the cloudinary response coming back through adapter.handleUpload, so the upstream 3.82+ regression that skips that call for client uploads becomes inert.

Why this works

generatePublicId(prefix, filename) is the value the plugin tells Cloudinary to use (both server-side via cloudinary.uploader.upload_stream and client-side via the signed upload). Cloudinary honors an explicit public_id verbatim — no suffixing — so the resulting cloudinaryPublicId is fully determined by:

`${folderSrc}${collectionPrefix}${filename without extension}`

…every piece of which is either adapter config (closure) or already on the document (data.filename). The cloudinary response is a mirror of what we passed in, not a source of truth — exactly like how the S3 adapter knows the bucket key without asking S3 (@payloadcms/storage-s3/dist/adapter.js).

What changed

  • cloudinaryPublicId field gets a beforeChange hook that derives the value from data.filename + closure-captured collectionPrefix + folderSrc. Falls back to originalDoc.filename on updates without a new file. This runs whether or not handleUpload was called, so client uploads on Payload 3.82+ persist correctly.
  • generateURL derives cloudinaryPublicId inline instead of reading data.cloudinaryPublicId. It now takes cloudName, collectionPrefix, and folderSrc instead of the full options object.
  • staticHandler reconstructs the Cloudinary CDN URL from doc.cloudinaryPublicId + mimeType (looked up via payload.find when not in doc). It no longer reads doc.url, which removes a footgun: with disablePayloadAccessControl: false, doc.url was /api/{collection}/file/{filename} and the old handler would recursively fetch itself.
  • getAdminThumbnail derives cloudinaryPublicId from doc.filename + collection prefix instead of reading doc.cloudinaryPublicId — same logic as generateURL, same factory signature.
  • handleUpload still performs the actual upload but no longer mutates data — the field hook owns persistence.
  • useFilename: false removed (breaking). In that mode Cloudinary mints a random public_id, and the only place it exists is the upload response — non-derivable. Dropping it is the price of a stateless plugin.
  • adapter.clientUploads is now forwarded from options.clientUploads instead of being hardcoded, fixing a pre-existing oversight.

What this means for the upstream bug

The upstream filter !file.clientUploadContext in @payloadcms/plugin-cloud-storage/hooks/afterChange.js:35 still skips handleUpload for client uploads. That's still arguably an API contract violation (HandleUpload's type declares clientUploadContext as a parameter). But for this plugin it's now a non-issue — there's nothing left for the adapter to do that depends on that callback firing. Filing the upstream bug remains worthwhile for other adapters in the wild that still need it.

Relationship to #148

#148 is a fast, low-risk fix that restores persistence by adding a collection-level beforeChange hook reading req.file.clientUploadContext. This PR is the cleaner alternative — pick one. If we ship this, #148 should be closed without merging.

Test plan

  • pnpm test — 5 new tests covering:
    • derivation for a collection with no prefix
    • derivation with collection prefix (videos/)
    • derivation without any clientUploadContext involvement (proves the new path doesn't depend on it)
    • fallback to originalDoc.filename on update without a new file
    • non-cloudinary collections are untouched
  • pnpm typecheck clean
  • pnpm lint clean
  • pnpm build produces a clean dist/ with no *.test.* files
  • End-to-end in cloudinary/dev: upload an image (uses disablePayloadAccessControl: true) and a video (no disablePayloadAccessControl) via the admin's client upload path, confirm both render in the admin and the docs carry the expected cloudinaryPublicId.
  • Migration check: existing docs already have correct cloudinaryPublicId values stored (the persistence used to come from Cloudinary's response, which equals what the new derivation produces for useFilename: true). No data migration needed except for users who were on useFilename: false — they'll need to rename objects in Cloudinary.

Breaking changes

  • useFilename option removed. Documented in CHANGELOG with migration guidance.
  • adapter.handleUpload no longer writes cloudinaryPublicId or url onto the document. Direct consumers of the adapter (rare) should be aware.

cloudinaryPublicId: `${cloudinaryPublicId}.webp`,
cloudName,
mimeType: 'video/', // Keep as video for correct resource type
mimeType: 'video/',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore the comment

Comment thread cloudinary/src/index.ts
...incomingConfig,
collections: (incomingConfig.collections || []).map((collection) => {
if (!collectionsWithAdapter[collection.slug]) {
const collOptions = options.collections[collection.slug]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to collectionOptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant