Moodle
  1. Moodle
  2. MDL-25953

problem viewing files located in sub folders within Amazon S3

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Test pre-requisite

      • An Amazon S3 repository available for testing
      • 2 Buckets, each one containing folders, sub folders and files
      • Bucket names should contain capital letters
      • Some files and folders should contain Unicode characters, spaces, etc...

      Test steps

      1. Enable your Amazon S3 repository
      2. Go to your private files
      3. Make sure you can freely browse the Amazon Repository
      4. Add several files and make sure they've properly been added
      5. Save your private files
      6. Visit your private files, click on each new file, download them, and make sure the content is correct
      Show
      Test pre-requisite An Amazon S3 repository available for testing 2 Buckets, each one containing folders, sub folders and files Bucket names should contain capital letters Some files and folders should contain Unicode characters, spaces, etc... Test steps Enable your Amazon S3 repository Go to your private files Make sure you can freely browse the Amazon Repository Add several files and make sure they've properly been added Save your private files Visit your private files, click on each new file, download them, and make sure the content is correct
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-25953-master
    • Rank:
      15336

      Description

      When using Amazon S3 you can add files using "add a resource/file" and select them from Amazon S3 (assuming you have setup the Amazon S3 repository with a secret key and access password), however anything selected which is in a subfolder within the bucket doesn't display, while files in the root of the Amazon bucket display fine. It appears that moodle is merging the foldername in with the file name and combining them into a single file name that it then looks for and can't find.

      For example "file.pdf" stored in a bucket called "folder" is appearing in moodle as "folderfile.pdf". If the same file is put into the root of the bucket it appears in moodle as simply "file.pdf" and displays fine.

      We tested this with both pdf and .m4v files on two separate installations of Moodle and in both cases those in the root of the bucket displayed fine while those in a subfolder didn't.

        Issue Links

          Activity

          Hide
          Andy Gardiner added a comment -

          raised the priority to major as it seriously impairs the integration with Amazon S3.

          Show
          Andy Gardiner added a comment - raised the priority to major as it seriously impairs the integration with Amazon S3.
          Hide
          Dongsheng Cai added a comment -

          Should have a look at this one for next development cycle

          Show
          Dongsheng Cai added a comment - Should have a look at this one for next development cycle
          Hide
          Lisa added a comment -

          It is still a problem... =(

          Show
          Lisa added a comment - It is still a problem... =(
          Hide
          Michael de Raadt added a comment -

          It looks like this is still a problem and has been overlooked for a while.

          I'm bumping this issue.

          Show
          Michael de Raadt added a comment - It looks like this is still a problem and has been overlooked for a while. I'm bumping this issue.
          Hide
          Frédéric Massart added a comment -

          Sprint planning.
          Estimated remaining development difficulty: XS

          Show
          Frédéric Massart added a comment - Sprint planning. Estimated remaining development difficulty: XS
          Hide
          Frédéric Massart added a comment -

          Pushing for peer review. Part of our API had to be rewritten because Amazon does not handle capital letters in bucket names when we use them the 'Host' parameter.

          Show
          Frédéric Massart added a comment - Pushing for peer review. Part of our API had to be rewritten because Amazon does not handle capital letters in bucket names when we use them the 'Host' parameter.
          Hide
          Rajesh Taneja added a comment -

          Code looks good Fred,
          I am still reviewing this, in the meanwhile few things you might want to consider:

          1. https://github.com/FMCorz/moodle/compare/MDL-25953-master#L0R1114 can be out of if-else

          In addition to above, you might want to upgrade s3 (third party library) and see if it solve issue at first place.
          https://github.com/tpyo/amazon-s3-php-class

          It might be worth looking at comment on http://undesigned.org.za/2007/10/22/amazon-s3-php-class

          NOTE ON FOLDERS: Amazon S3 does not support folders.  Clients like S3Fox create specific files that are displayed as folders.  Just use slash paths for your object names (foo/bar.txt) and (foo/) as your prefix when listing contents.
          
          Show
          Rajesh Taneja added a comment - Code looks good Fred, I am still reviewing this, in the meanwhile few things you might want to consider: https://github.com/FMCorz/moodle/compare/MDL-25953-master#L0R1114 can be out of if-else In addition to above, you might want to upgrade s3 (third party library) and see if it solve issue at first place. https://github.com/tpyo/amazon-s3-php-class It might be worth looking at comment on http://undesigned.org.za/2007/10/22/amazon-s3-php-class NOTE ON FOLDERS: Amazon S3 does not support folders. Clients like S3Fox create specific files that are displayed as folders. Just use slash paths for your object names (foo/bar.txt) and (foo/) as your prefix when listing contents.
          Hide
          Frédéric Massart added a comment -

          Hi Raj, thanks for looking at this issue. I had not considered that this API was from a 3rd party. The newest version of this class fixes the issue that did not allow capital letters in bucket names. I have imported the class and ported all the others changes made to lib.php so we now have a fresh and clean copy of this API.

          I think we should consider using Amazon PHP SDK as they provide one, but that implies a lot of rewriting and should be considered as a separate issue, probably in the dev backlog (not that I don't want to do it though).

          Show
          Frédéric Massart added a comment - Hi Raj, thanks for looking at this issue. I had not considered that this API was from a 3rd party. The newest version of this class fixes the issue that did not allow capital letters in bucket names. I have imported the class and ported all the others changes made to lib.php so we now have a fresh and clean copy of this API. I think we should consider using Amazon PHP SDK as they provide one, but that implies a lot of rewriting and should be considered as a separate issue, probably in the dev backlog (not that I don't want to do it though).
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Patch looks good, you might want to consider few things before you push it for integration review.

          1. Fix php doc for https://github.com/FMCorz/moodle/commit/519119c1ee3cb8f5ba442fe0dadfaf8ad3db95d3#L0R57
          2. Not sure why you need a numeric check on empty variable https://github.com/FMCorz/moodle/commit/519119c1ee3cb8f5ba442fe0dadfaf8ad3db95d3#L0R126

          Logic is perfect, although you might re-arrange get_listing logic to make it more understandable for future maintenance. Also, I agree with you about using SDK, but probably another issue, hence didn't mention it at first place.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Patch looks good, you might want to consider few things before you push it for integration review. Fix php doc for https://github.com/FMCorz/moodle/commit/519119c1ee3cb8f5ba442fe0dadfaf8ad3db95d3#L0R57 Not sure why you need a numeric check on empty variable https://github.com/FMCorz/moodle/commit/519119c1ee3cb8f5ba442fe0dadfaf8ad3db95d3#L0R126 Logic is perfect, although you might re-arrange get_listing logic to make it more understandable for future maintenance. Also, I agree with you about using SDK, but probably another issue, hence didn't mention it at first place.
          Hide
          Frédéric Massart added a comment -

          Thanks Raj, I have added the missing phpdoc for the parameter. After considerations, I did not change the is_numeric() because this ensures that the value can be 0. substr() can also return false (which is likely to happen there) and having comparison with empty strings might not be the best option.

          Cheers! Pushing for integration!

          Show
          Frédéric Massart added a comment - Thanks Raj, I have added the missing phpdoc for the parameter. After considerations, I did not change the is_numeric() because this ensures that the value can be 0. substr() can also return false (which is likely to happen there) and having comparison with empty strings might not be the best option. Cheers! Pushing for integration!
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Fred,

          Do I read correctly that the library upgrade does not fix the issue? In which case, what is the justification for upgrading it on the stable branches?

          Third parties could theoretically be relying on the old version of the library on stable branches (maybe even the bugs in the library) and it would be impolite of us to upgrade that without justification.

          Show
          Dan Poltawski added a comment - Hi Fred, Do I read correctly that the library upgrade does not fix the issue? In which case, what is the justification for upgrading it on the stable branches? Third parties could theoretically be relying on the old version of the library on stable branches (maybe even the bugs in the library) and it would be impolite of us to upgrade that without justification.
          Hide
          Frédéric Massart added a comment -

          Hi Dan,

          there were two issues.

          1. Browsing a bucket was impossible if it contained capital letters. This issue is fixed by the 3rd party API upgrade.
          2. Browsing sub folders within a bucket was not implemented on our side (scope of this issue)

          I did not notice any change in the API that could cause problems, it included fixes and new methods. Although, we won't be able to backport the #2 as is without the #1.

          Show
          Frédéric Massart added a comment - Hi Dan, there were two issues. 1. Browsing a bucket was impossible if it contained capital letters. This issue is fixed by the 3rd party API upgrade. 2. Browsing sub folders within a bucket was not implemented on our side (scope of this issue) I did not notice any change in the API that could cause problems, it included fixes and new methods. Although, we won't be able to backport the #2 as is without the #1.
          Hide
          Dan Poltawski added a comment -

          OK, thanks for clarifying fred. I agree this is low risk and people shouldn't really be relying on this.

          Show
          Dan Poltawski added a comment - OK, thanks for clarifying fred. I agree this is low risk and people shouldn't really be relying on this.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Fred.

          Show
          Dan Poltawski added a comment - Integrated, thanks Fred.
          Hide
          Ankit Agarwal added a comment -

          Works as expected!
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - Works as expected! Passing Thanks
          Hide
          Dan Poltawski added a comment -

          asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ!

          Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

          Show
          Dan Poltawski added a comment - asko, Дзякуй, ধন্যবাদ, Благодаря, Gràcies, 感谢, 謝謝, Hvala, Díky, Tak, Bedankt, Tänan, متشکریم, Salamat, Kiitokset, Merci, Grazas, Danke, Ευχαριστώ, આભાર, תודה, धन्यवाद, Köszönjük, Takk fyrir, Terima Kasih, Grazie, ありがとうございます, Рахмет, សូមអរគុណ, 감사합니다, gratiās, Pateicamies, Ačiū, Благодарам, Tēnā koa, Kia Ora Rawa Atu, आभारी आहोत, Талархал, Takk, Dziękuję, Obrigado, Mulţumesc, Engraziel, Спасибо, Fa'afetai, Хвала, Hvala, ස්තූතියි, Vďaka, Hvala, Mahadsanid, Thanks, Gracias, Tack, Salamat, நன்றி, నెనరులు, ขอบคุณค่ะ! Your work has made it into this weeks Moodle release! There are no gold medals available this week - but millions around the world will benefit. Thank you!

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: