Moodle
  1. Moodle
  2. MDL-30459

Better error message in webservice/upload.php, create file handling web service demo client, update doc

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.2
    • Component/s: Files API, Web Services
    • Labels:
    • Rank:
      33130

      Description

      a) Create File upload + File download web service demo client
      b) update the documentation: http://docs.moodle.org/dev/Web_services_files_handling
      c) webservice/upload.php should return better exception when filename already exist

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          We need to return better erro exception in webservice/upload.php script.

          Show
          Jérôme Mouneyrac added a comment - We need to return better erro exception in webservice/upload.php script.
          Hide
          Jérôme Mouneyrac added a comment -

          I wrote some piece of code to make the webservice/upload.php script automatically rename file if occurence is found:

          $renameoccurencewithdate = optional_param('renameoccurencewithdate', false, PARAM_BOOL); //avoid to extra call
          
          //Check if the file already exist
              $existingfile = $this->file_exists($file_record->contextid, $file_record->component, $file_record->filearea, 
                          $file_record->itemid, $file_record->filepath, $file_record->filename);
              if ($existingfile) {
                  //if allow automatic rename (avoid)
                  if ($renameoccurencewithdate) {
                      //get file name extension
                      $filenameinfo = pathinfo($file->filename);
                      //add a . to the extension
                      $filenameinfo['extension'] = empty($filenameinfo['extension'])?'':'.'.$filenameinfo['extension']; 
          
                      $file->filename = $filenameinfo['filename'] . '-' . userdate(time(), "%Y%m%d-%H%M%S") . $filenameinfo['extension'];
                  } else {
                      throw new moodle_exception('storedfileproblem', 'File already exist');
                  }
              }
          
          Show
          Jérôme Mouneyrac added a comment - I wrote some piece of code to make the webservice/upload.php script automatically rename file if occurence is found: $renameoccurencewithdate = optional_param('renameoccurencewithdate', false , PARAM_BOOL); //avoid to extra call //Check if the file already exist $existingfile = $ this ->file_exists($file_record->contextid, $file_record->component, $file_record->filearea, $file_record->itemid, $file_record->filepath, $file_record->filename); if ($existingfile) { // if allow automatic rename (avoid) if ($renameoccurencewithdate) { //get file name extension $filenameinfo = pathinfo($file->filename); //add a . to the extension $filenameinfo['extension'] = empty($filenameinfo['extension'])?'':'.'.$filenameinfo['extension']; $file->filename = $filenameinfo['filename'] . '-' . userdate(time(), "%Y%m%d-%H%M%S" ) . $filenameinfo['extension']; } else { throw new moodle_exception('storedfileproblem', 'File already exist'); } }
          Show
          Jérôme Mouneyrac added a comment - Demo clients are there: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-HTTP-filehandling
          Hide
          Jérôme Mouneyrac added a comment - - edited
          Show
          Jérôme Mouneyrac added a comment - - edited Documentation updated: http://docs.moodle.org/dev/Web_services_files_handling
          Hide
          Jérôme Mouneyrac added a comment -

          Little improvement for upload script when filename already exist (avoid to display a DDL INSERT error)

          Show
          Jérôme Mouneyrac added a comment - Little improvement for upload script when filename already exist (avoid to display a DDL INSERT error)
          Hide
          Sam Hemelryk added a comment -

          Jerome - how necessary is this?
          I had just had pressed go to integrate this and then noticed a massive massive problem.

          If a user uploads 3 files, and number 2 fails, then number 1 has been successfully uploaded and the end client will never know as only the exception will stand.
          The code as it is cannot stay as far as I can see. We have two options:

          1. Revert this code and deal with it after release.
          2. Fix the issue very quickly in one of two fashions:
            1. All files are checked before creating them and if any single file exists return the exception (this way no files are created)
            2. Ignore files that already exist and simply don't return them in the result (the application will be smart enough to compare what was sent against what was received as a result)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Jerome - how necessary is this? I had just had pressed go to integrate this and then noticed a massive massive problem. If a user uploads 3 files, and number 2 fails, then number 1 has been successfully uploaded and the end client will never know as only the exception will stand. The code as it is cannot stay as far as I can see. We have two options: Revert this code and deal with it after release. Fix the issue very quickly in one of two fashions: All files are checked before creating them and if any single file exists return the exception (this way no files are created) Ignore files that already exist and simply don't return them in the result (the application will be smart enough to compare what was sent against what was received as a result) Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          I should add if you choose to fix it immediately then you will need to work off the integration branch as I had to clean up whitespace issues when integrating your changes.

          Show
          Sam Hemelryk added a comment - I should add if you choose to fix it immediately then you will need to work off the integration branch as I had to clean up whitespace issues when integrating your changes.
          Hide
          Jérôme Mouneyrac added a comment -

          I have a patch that return this (1st file fails, 2nd success, 3rd fails):

          [

          {"filename":"image_to_upload.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload.jpg"}

          ,

          {"component":"user","contextid":"13","userid":"2","filearea":"private","filename":"image_to_upload2.jpg","filepath":"\/","itemid":0,"license":"allrightsreserved","author":"Admin User","source":""}

          ,

          {"filename":"image_to_upload3.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload3.jpg"}

          ]

          Show
          Jérôme Mouneyrac added a comment - I have a patch that return this (1st file fails, 2nd success, 3rd fails): [ {"filename":"image_to_upload.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload.jpg"} , {"component":"user","contextid":"13","userid":"2","filearea":"private","filename":"image_to_upload2.jpg","filepath":"\/","itemid":0,"license":"allrightsreserved","author":"Admin User","source":""} , {"filename":"image_to_upload3.jpg","errortype":"filenameexist","errormsg":"File name already exists: image_to_upload3.jpg"} ]
          Hide
          Sam Hemelryk added a comment -

          Thanks for getting a fix up promptly Jerome, it has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks for getting a fix up promptly Jerome, it has been integrated now
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay!

          Closing and big thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream! Just in time for Moodle 2.2rc1 (if related), yay! Closing and big thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: