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

      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.

        Gliffy Diagrams

          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: