Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packages / Cloudinary Storage: Fix move file when ROOT is defined #22551

Conversation

joselcvarela
Copy link
Member

Scope

We are using Cloudinary as storage and we found that when using STORAGE_<LOCATION>_ROOT it throws an error when editing an image.

{
  "errors": [
    {
      "message": {
        "extensions": {
          "stack": "Error: Can't move file \"temp_0434757f-7d0b-4f88-93fb-8eec237e0327.jpg\": Resource not found - temp_0434757f-7d0b-4f88-93fb-8eec237e0327\n    at DriverCloudinary.move (file:///directus/packages/storage-driver-cloudinary/dist/index.js:239:13)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at FilesService.uploadOne (/directus/api/src/services/files.ts:159:4)\n    at Multipart.<anonymous> (/directus/api/src/controllers/files.ts:109:23)"
        }
      },
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "stack": "Error: Can't move file \"temp_0434757f-7d0b-4f88-93fb-8eec237e0327.jpg\": Resource not found - temp_0434757f-7d0b-4f88-93fb-8eec237e0327\n    at DriverCloudinary.move (file:///directus/packages/storage-driver-cloudinary/dist/index.js:239:13)\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at FilesService.uploadOne (/directus/api/src/services/files.ts:159:4)\n    at Multipart.<anonymous> (/directus/api/src/controllers/files.ts:109:23)"
      }
    }
  ]
}

It looks like the move operation does not take the root folder into consideration:
https://github.com/directus/directus/blob/main/packages/storage-driver-cloudinary/src/index.ts#L199-L204

Although delete already handles this:
https://github.com/directus/directus/blob/main/packages/storage-driver-cloudinary/src/index.ts#L365-L370

So in this PR we just apply the same approach in order to get the proper file path.

What's changed:

  • Fix paths on move operation for Cloudinary storage. In other words, fix saving edited image when using ROOT with Cloudinary storage

Potential Risks / Drawbacks

  • None 🤔

Review Notes / Questions

  • N/A

Copy link

changeset-bot bot commented May 23, 2024

🦋 Changeset detected

Latest commit: 75de80c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@directus/storage-driver-cloudinary Patch
@directus/storage Patch
@directus/api Patch
@directus/errors Patch
@directus/storage-driver-azure Patch
@directus/storage-driver-gcs Patch
@directus/storage-driver-local Patch
@directus/storage-driver-s3 Patch
@directus/storage-driver-supabase Patch
directus Patch
@directus/extensions-registry Patch
@directus/memory Patch
@directus/validation Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rijkvanzanten
Copy link
Member

CleanShot 2024-05-23 at 09 35 45@2x
Looks like the unit tests are failing for the cloudinary driver. Mind taking a look @joselcvarela ?>

@joselcvarela
Copy link
Member Author

CleanShot 2024-05-23 at 09 35 45@2x Looks like the unit tests are failing for the cloudinary driver. Mind taking a look @joselcvarela ?>

Yeah, I was already looking into it and I just pushed new changes to fix the tests 👍

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Can confirm it fixes that specific issue 🚀
Although, I've noticed that the edited version of the image isn't actually persisted (regardless of ROOT). However, this seems to be unrelated, as it was already the case before. Will open a separate issue 👍

@joselcvarela
Copy link
Member Author

Can confirm it fixes that specific issue 🚀 Although, I've noticed that the edited version of the image isn't actually persisted (regardless of ROOT). However, this seems to be unrelated, as it was already the case before. Will open a separate issue 👍

I also have experienced this, but according to Cloudinary documentation, signed URLs are not invalidated https://arc.net/l/quote/rbyeysrn

Since we are using signed URLs to access private assets, we could create a different file on Cloudinary instead of reusing the existent one, but the existent one is based on the primary key of the file so not sure if this is the right approach to follow.

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
@paescuj
Copy link
Member

paescuj commented May 23, 2024

Can confirm it fixes that specific issue 🚀 Although, I've noticed that the edited version of the image isn't actually persisted (regardless of ROOT). However, this seems to be unrelated, as it was already the case before. Will open a separate issue 👍

I also have experienced this, but according to Cloudinary documentation, signed URLs are not invalidated https://arc.net/l/quote/rbyeysrn

Since we are using signed URLs to access private assets, we could create a different file on Cloudinary instead of reusing the existent one, but the existent one is based on the primary key of the file so not sure if this is the right approach to follow.

Good catch, this seems to be exactly it! I've created #22556, let's discuss over there 👍
In the meantime, we can still merge this pull request as it already improves the situation.

@paescuj paescuj merged commit 9715fb0 into directus:main May 23, 2024
4 checks passed
@github-actions github-actions bot added this to the Next Release milestone May 23, 2024
@joselcvarela joselcvarela deleted the packages/storage-driver-cloudinary/fix-move-file-with-root branch May 24, 2024 05:45
rijkvanzanten pushed a commit that referenced this pull request May 28, 2024
…2551)

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants