Problem/Motivation

With a public files S3fs StreamWrapper, with the option Deliver files through Drupal and a root folder configured, for example s3fs-public in the bucket, the URLs generated for files return a 404:
/s3/files/public/s3fs-public/2024-07/text-example.txt

However, modifying the URL to:
/s3/files/public/2024-07/text-example.txt
works and returns the correct file.

It seems that removing the root folder from the URL allows returning the correct file.

Steps to reproduce

  • Private access bucket without any ACL config.
  • S3fs StreamWrapper with the option Deliver files through Drupal and a root folder configured, for example s3fs-public.

Create a Document media item with a text file upload.
The URL generated should be something like this: /s3/files/public/s3fs-public/2024-07/text-example.txt and returns a 404.

Proposed resolution

Maybe the root folder should be removed from the generated URL path?

Or maybe there should be some kind of Nginx rewrite rule, a bit similar to what is done for image styles?
 

Not sure whether this should be considered as a Bug report or Support request, so feel free to re-qualify as required.

Feel free to let me know if you would need more information on the setup or the issue in general, I would be glad to reply as soon as possible.
Any help, advice or recommendations would be greatly appreciated.
Thanks in advance!

Issue fork s3fs-3462508

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DYdave created an issue. See original summary.

DYdave opened merge request !61

cmlara’s picture

cmlara
he/him
commented

Very quick glancing the code:
Probably a bug.

I do not recall ever designed a concept for the Bucket to tell StreamWrapper that there was a “path prefix”. The design is *suppose* to be that the StreamWrapper plugins are oblivious to how the bucket stores the files.

https://git.drupalcode.org/project/s3fs/-/blob/ef7d09953f5380b83ea1aa197... probably should be $target not $s3_key

dydave’s picture

dydave commented
Status: Active » Needs review

Super nice of you Conrad! 🙏

Just pasting here the comment I intended to post before you beat me to the punch 😅
 

Once again, I would definitely like to run this one by you Conrad (@cmlara), when you have some time:

I have no clue whether this is the correct way to fix this...

This patch is really a quick fix, which works in my current setup, but could potentially break in many other different types of setups I haven't had the time or chance to test (for example, bucket with Cname, ACL config, etc...).

These changes seem to fix the issue in my case and the files are delivered properly:
The generated path doesn't change, for example: /s3/files/public/s3fs-public/2024-07/text-example.txt
But the correct file is returned properly.

The code changes could maybe be further optimized, but before going any further and spending more time on this I wanted to know if I was looking at the right piece of code.

If I am mistaken and there is a different way this could be achieved and fixed, perhaps through configuration or Nginx rules/config, I would greatly appreciate if you could please give me some pointers and guidance.

Once again, I'm setting this in Needs review so you could perhaps let me know at a first glance if there is anything I've changed that could cause other potential issues.

Based on your advice, feedback, suggestions and recommendations, I would definitely be very happy to help testing, or making any change to the merge request, if that could help you in any way.

Otherwise, feel free to let me know if you have any questions or would need more information on the bug itself, the ticket in general, or the merge request, I would surely try answering as soon as possible.
Thanks very much in advance!

dydave’s picture

dydave commented

Re #3:

Thanks a lot!

https://git.drupalcode.org/project/s3fs/-/blob/ef7d09953f5380b83ea1aa197...
probably should be $target not $s3_key

I'll take a look at this, give it a quick test and report back!

Thanks again!

dydave’s picture

dydave commented

You rock Conrad! 🎉

Once again, thank you very much for your great help and prompt reply!

I made the changes in module's files directly for testing on the target development environment and it worked great!

The generated URL has changed and is now:
/s3/files/public/2024-07/text-example.txt

without the root folder in the path anymore, just as suggested in the issue summary and based on the tests I initially did, modifying the URL in the browser.

I've updated the merge request accordingly and will be using this as the patch instead, until you are able to review the MR and potentially consider getting this into the dev branch.

Feel free to let me know if there is anything else I could do to help getting this rolled in module's code base, or whether you would need any additional information, or more changes to the made to the MR, I would defintely be very happy to help.

Thanks again! 🙏

cmlara’s picture

cmlara
he/him
commented

Looks good to me.

I would like there to be a test on this as it appears I missed the scenario, would be nice to avoid a regression in future.

I believe something like the following added to S3FsCustomStreamWrapperPluginTest::providerGetExternalUrl() would proof this (though with the current state of code it will be hard for the CI lab to validate this) 

      'Use Drupal Delivery with root_folder' => [
        [
          'use_drupal' => TRUE,
          'root_folder' => 'folder_prefix',        
        ],
        's3://test/object.txt',
        TRUE,
        '/s3/files/s3/test/object.txt',
      ],

(Not actually tested)

dydave’s picture

dydave commented

Thanks again Conrad for the prompt reply and detailed instructions on the test to add, it really helped! 🙂

The corresponding changes have been made to the merge request, which is ready to be reviewed once again.

I've placed the provided piece of code right after the other Drupal Delivery related piece.

Feel free to let me know if there is anything else you would see that would need testing or changing in order for the merge request to be added to the dev branch, I would certainly try answering as soon as possible.
Thanks in advance!

  • cmlara committed 268cc96e on 4.0.x authored by DYdave 
    Issue #3462508 by DYdave, cmlara: S3fs StreamWrapper: Problem with...
cmlara’s picture

cmlara
he/him
commented
Status: Needs review » Fixed

Downloaded to local lab to confirm the updated test fails without the fix as the MR did not allow running the fail-only test job. Test does indeed fail as expected.

Merging to mainline. Thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.