fix: prevent self-collision in getSafeFileName on update#16578
Open
EvilConsultant wants to merge 1 commit into
Open
fix: prevent self-collision in getSafeFileName on update#16578EvilConsultant wants to merge 1 commit into
EvilConsultant wants to merge 1 commit into
Conversation
When a media document is updated with a new file upload, getSafeFileName called docWithFilenameExists without excluding the document being updated. The query found the document's own existing filename and incorrectly treated it as a collision, appending a spurious `-1` suffix (e.g. foo.jpg → foo-1.jpg). Fix: add an optional `docId` parameter to both getSafeFileName and docWithFilenameExists. When present, the document with that ID is excluded from the WHERE clause so a document cannot collide with itself. generateFileData now passes `originalDoc.id` as `docId` when operation is `update`, which covers the PATCH-with-file case that triggered the bug. Closes #<issue>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Ran into an issue when I was running some image migrations (pulling things out of Cloudinary into Payload).
When a media document is updated with a new file upload (PATCH with multipart body),
getSafeFileNamecallsdocWithFilenameExistswithout excluding the document being updated. The query finds the document's own existing filename and treats it as a collision — incrementing the filename with a-1suffix.Example: updating a media doc that already has
foo.jpgcauses Payload to write the new file asfoo-1.jpg, silently breaking any references to the original URL.I assume this isn't a feature? It caused me a lot of issues with cached names, integrations between Payload and Medusa needing to refresh thumbnails - basically anything that had the old name and all I did was patch an update to the image (not expecting a name change).
Root cause
docWithFilenameExistsqueries:On a
createthis is correct — any existing doc with that name is a real conflict. On an update the document being replaced already owns the filename, so the match is a false positive.Fix
Add an optional
docIdparameter to bothgetSafeFileNameanddocWithFilenameExists. When present,docIdis appended asid: { not_equals: docId }so the current document is excluded from the collision check.generateFileDatapassesoriginalDoc.idasdocIdwhenoperation === 'update', which covers the PATCH-with-file path that triggers the bug.Changes
docWithFilenameExists.ts— accept and apply optionaldocIdexclusiongetSafeFilename.ts— accept and threaddocIdthrough; update JSDocgenerateFileData.ts— passdocIdon update operationsgetSafeFilename.spec.ts— four new tests covering the self-collision fixReproduction
This is also reproducible by running a script that re-uploads each media document to trigger Sharp image size regeneration — every doc ends up with
-1appended after the first run.