Moodle
  1. Moodle
  2. MDL-29276 META- Web service improvements for 2.2
  3. MDL-29277

Making the exception message during return values validation more explicit (and also during parameter validation)

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1
    • Fix Version/s: 2.2
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide

      Set debug mode to 'developer'. MDL-29435 must be fixed in order to test with SOAP/XMLRPC

      Error: wrong parameter primary type at 3rd level:
      ----------------------------------------------------------------------
      1- in user/externallib.php::create_users_parameters change "users>customfields>type": PARAM_ALPHANUMEXT for PARAM_INT.
      Call the function with the built-in REST client in the admin. Check that the error message let you know exactly where is the issue, what value is faulty, and what value were expected. Obviously you must have a customfield.

      Error: wrong response primary type at 3rd level (cannot be test with REST, REST server does not validate return value, see MDL-29459):
      -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      2- in user/externallib.php::get_users_by_id_returns change "customfields>type": PARAM_ALPHANUMEXT for PARAM_INT.
      Call the function with the built-in SOAP client in the admin. Check that the error message let you know exactly where is the issue, what value is faulty, and what value were expected. Obviously you must call a user that return a customfield.

      Error: send string, expecting array
      -------------------------------------------------
      3. Use the REST demo client: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST. Change this line

      $params = array('users' => '$users'); //previously $params = array('users' => $users);
      

      The error message should be clear where is the issue, telling you what value is wrong.

      Show
      Set debug mode to 'developer'. MDL-29435 must be fixed in order to test with SOAP/XMLRPC Error: wrong parameter primary type at 3rd level: ---------------------------------------------------------------------- 1- in user/externallib.php::create_users_parameters change "users>customfields>type": PARAM_ALPHANUMEXT for PARAM_INT. Call the function with the built-in REST client in the admin. Check that the error message let you know exactly where is the issue, what value is faulty, and what value were expected. Obviously you must have a customfield. Error: wrong response primary type at 3rd level (cannot be test with REST, REST server does not validate return value, see MDL-29459 ): ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 2- in user/externallib.php::get_users_by_id_returns change "customfields>type": PARAM_ALPHANUMEXT for PARAM_INT. Call the function with the built-in SOAP client in the admin. Check that the error message let you know exactly where is the issue, what value is faulty, and what value were expected. Obviously you must call a user that return a customfield. Error: send string, expecting array ------------------------------------------------- 3. Use the REST demo client: https://github.com/moodlehq/sample-ws-clients/tree/master/PHP-REST . Change this line $params = array('users' => '$users'); //previously $params = array('users' => $users); The error message should be clear where is the issue, telling you what value is wrong.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      18825

      Description

      Making the exception message of "invalid return value" more explicit too - at least knowing which attribut of the return value is concerned.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          Submitting for integration. As I mentioned in http://tracker.moodle.org/browse/MDL-26635, there are some change in the strings (new string params). However this fix is so helpful when creating/debugging web services that my +100 to backport it to 2.0/2.1 Let me know if you are ok at integration level for the backport (then I'll backport it). Cheers.

          Show
          Jérôme Mouneyrac added a comment - - edited Submitting for integration. As I mentioned in http://tracker.moodle.org/browse/MDL-26635 , there are some change in the strings (new string params). However this fix is so helpful when creating/debugging web services that my +100 to backport it to 2.0/2.1 Let me know if you are ok at integration level for the backport (then I'll backport it). Cheers.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Petr Škoda added a comment -

          I disagree with this approach, detailed error info should be only in debug message.

          Please note that web servers intentionally hide error details - it is considered to be a security issue if you run production server with display_errors and high debug enabled, sorry.

          my -1

          Show
          Petr Škoda added a comment - I disagree with this approach, detailed error info should be only in debug message. Please note that web servers intentionally hide error details - it is considered to be a security issue if you run production server with display_errors and high debug enabled, sorry. my -1
          Hide
          Sam Hemelryk added a comment -

          Reopening as discussed thanks Jerome so that the handing out of this information can be improved.

          Show
          Sam Hemelryk added a comment - Reopening as discussed thanks Jerome so that the handing out of this information can be improved.
          Hide
          Jérôme Mouneyrac added a comment -

          The commit now only returns detailled info into $debuginfo. I also fixed the webservice_parameter_exception that didn't have any $debuginfo.

          Show
          Jérôme Mouneyrac added a comment - The commit now only returns detailled info into $debuginfo. I also fixed the webservice_parameter_exception that didn't have any $debuginfo.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing to Sam, for integration evaluation (my knowledge here is 0). TIA!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing to Sam, for integration evaluation (my knowledge here is 0). TIA!
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,
          I'm happy with this now however I would like to clarify one thing before it is integrated.
          In a couple of the try...catch statements you have changed the catch from invalid_parameter_excpetion to the general exception. Is this meant to be there? it seems counter productive but perhaps I am missing something.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, I'm happy with this now however I would like to clarify one thing before it is integrated. In a couple of the try...catch statements you have changed the catch from invalid_parameter_excpetion to the general exception. Is this meant to be there? it seems counter productive but perhaps I am missing something. Cheers Sam
          Hide
          Jérôme Mouneyrac added a comment -

          hum no, you are right I'm reverting to the previous exception, only the part about $debuginfo should be addded.

          Show
          Jérôme Mouneyrac added a comment - hum no, you are right I'm reverting to the previous exception, only the part about $debuginfo should be addded.
          Hide
          Jérôme Mouneyrac added a comment -

          I'll remove all webservice_parameter_exception calls and I'll use the proper invalid_parameter_exception and invalid_response_exception, it's a lot cleaner. So in fact we are back to the initial Petr code from many months ago, with more debuginfo. I'll be testing (it's a bit longer as there are more impacts) and I'll commit the fix.

          Show
          Jérôme Mouneyrac added a comment - I'll remove all webservice_parameter_exception calls and I'll use the proper invalid_parameter_exception and invalid_response_exception, it's a lot cleaner. So in fact we are back to the initial Petr code from many months ago, with more debuginfo. I'll be testing (it's a bit longer as there are more impacts) and I'll commit the fix.
          Hide
          Sam Hemelryk added a comment -

          Sending back again for some more cleaning

          Show
          Sam Hemelryk added a comment - Sending back again for some more cleaning
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi, some comments / thoughts about exceptions aimed to clarify a bit my POV about this:

          1) We have two "root" exception classes moodle_exception and coding_exception (plz, ignore the later is extension of the former).

          2) Both are instantiable and can be used / extended.

          3) moodle_exception is about to show errors to users (print_error() can be use for that too), including one localised error message (with very limited disclosure of information) and one non-localised debuginfo, with all sort of information we want to disclose.

          4) coding_exception is about to show errors to developers, and only includes one non-localised short description of the error without any disclosure and one non-localised debuginfo, with all sort of information we want to disclose.

          5) Any extension of any of those exceptions should fulfill the parent localised/non-localised disclose/not disclose basis.

          And that's basically, how I see them, so the first question to answer is:

          A) Is my exception one user exception or one dev exception. That will tell us if we must use / extend moodle_exception or coding_exception.

          And, then, just following the premises specified above (points 3 and 4), we will know what needs to be localised and what no. And where to disclose info and where no.

          Not sure if this helps here or now. But if you run the premises above against the code I've seen here, we are clearly breaking it:

          1) Extending from moodle_exception when a lot of them are developer (coding) exceptions.
          2) Using localised strings in moodle_exceptions in places where localisation should not happen (debuginfo)
          3) Disclosing too much info in localised error codes (not sure about this, specially if should be coding exception instead).

          Hope this explains my POV about this (not knowing anything about WSs, just the basis for exception handling in a proper way).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi, some comments / thoughts about exceptions aimed to clarify a bit my POV about this: 1) We have two "root" exception classes moodle_exception and coding_exception (plz, ignore the later is extension of the former). 2) Both are instantiable and can be used / extended. 3) moodle_exception is about to show errors to users (print_error() can be use for that too), including one localised error message (with very limited disclosure of information) and one non-localised debuginfo, with all sort of information we want to disclose. 4) coding_exception is about to show errors to developers, and only includes one non-localised short description of the error without any disclosure and one non-localised debuginfo, with all sort of information we want to disclose. 5) Any extension of any of those exceptions should fulfill the parent localised/non-localised disclose/not disclose basis. And that's basically, how I see them, so the first question to answer is: A) Is my exception one user exception or one dev exception. That will tell us if we must use / extend moodle_exception or coding_exception. And, then, just following the premises specified above (points 3 and 4), we will know what needs to be localised and what no. And where to disclose info and where no. Not sure if this helps here or now. But if you run the premises above against the code I've seen here, we are clearly breaking it: 1) Extending from moodle_exception when a lot of them are developer (coding) exceptions. 2) Using localised strings in moodle_exceptions in places where localisation should not happen (debuginfo) 3) Disclosing too much info in localised error codes (not sure about this, specially if should be coding exception instead). Hope this explains my POV about this (not knowing anything about WSs, just the basis for exception handling in a proper way). Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Jerome,

          I've been chatting to Petr and Eloy about the exception handling here and I think several things have come out (nicely summed up by Eloy's comment above).

          While the patch that you have for this issue is primarily adding more information to the existing exceptions that are already using localised strings there are a couple of new localised debugging strings that are being added and because of that I am sending it back for the time being (sorry).

          Certainly at this point we need to look at cleaning up the exception handling throughout Web Services and at the same making it more consistent.

          The question is should this patch/issue be implemented before that cleanup or as part of?

          From what I can see in the web service code I don't think it would be a big job to go through and review the exception handling in web services, however I can't see the consequences that it may or may not have on users of the web services (iPhone etc).

          What are your thoughts on this? and we should probably check with Martin.

          As for me - personally I think creating a new issue to clean up exception handling in webservices and then improving those messages at the same time is the way to go.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jerome, I've been chatting to Petr and Eloy about the exception handling here and I think several things have come out (nicely summed up by Eloy's comment above). While the patch that you have for this issue is primarily adding more information to the existing exceptions that are already using localised strings there are a couple of new localised debugging strings that are being added and because of that I am sending it back for the time being (sorry). Certainly at this point we need to look at cleaning up the exception handling throughout Web Services and at the same making it more consistent. The question is should this patch/issue be implemented before that cleanup or as part of? From what I can see in the web service code I don't think it would be a big job to go through and review the exception handling in web services, however I can't see the consequences that it may or may not have on users of the web services (iPhone etc). What are your thoughts on this? and we should probably check with Martin. As for me - personally I think creating a new issue to clean up exception handling in webservices and then improving those messages at the same time is the way to go. Cheers Sam
          Hide
          Jérôme Mouneyrac added a comment -

          I removed the get_string calls for debuginfo as Eloy mentioned. I can not get my mind on making invalid_parameter_exception a coding_exception. I know devs are most likely the ones to see this error, but I still feel it is like a user entering a bad param value into a user field. It's why moodle_exception seems good to me, but if you really want, we can extendsinvalid_parameter_exception/invalid_response_exception from coding_exception instead of moodle_exception.

          Show
          Jérôme Mouneyrac added a comment - I removed the get_string calls for debuginfo as Eloy mentioned. I can not get my mind on making invalid_parameter_exception a coding_exception. I know devs are most likely the ones to see this error, but I still feel it is like a user entering a bad param value into a user field. It's why moodle_exception seems good to me, but if you really want, we can extendsinvalid_parameter_exception/invalid_response_exception from coding_exception instead of moodle_exception.
          Hide
          Aparup Banerjee added a comment -

          Hi Jerome, i've had a look at this and here's my 2c:

          • code looks good, patch seems to non-localize this now for the assumed developer audience.
          • IMO, i think the difficulty in deciding whether the information is destined for a user or a developer should be decided upon access to the webservice. Perhaps a token could be linked to having 'developer' level access to a web service or perhaps there are (expirable) tokens generated that will specifically respond with debug messages. otherwise tokens should respond with intended audience of normal (possibly non-English) users. (This isn't an original idea, plenty of services out there do something to this extent)
          Show
          Aparup Banerjee added a comment - Hi Jerome, i've had a look at this and here's my 2c: code looks good, patch seems to non-localize this now for the assumed developer audience. IMO, i think the difficulty in deciding whether the information is destined for a user or a developer should be decided upon access to the webservice. Perhaps a token could be linked to having 'developer' level access to a web service or perhaps there are (expirable) tokens generated that will specifically respond with debug messages. otherwise tokens should respond with intended audience of normal (possibly non-English) users. (This isn't an original idea, plenty of services out there do something to this extent)
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Apu for the review. About your solution to link debuginfo to the token, it could be another tracker issue for 2.2+

          I'll submit for integration review.

          Show
          Jérôme Mouneyrac added a comment - Thanks Apu for the review. About your solution to link debuginfo to the token, it could be another tracker issue for 2.2+ I'll submit for integration review.
          Hide
          Aparup Banerjee added a comment -

          ps: created MDL-29767 for anyone interested

          Show
          Aparup Banerjee added a comment - ps: created MDL-29767 for anyone interested
          Hide
          Sam Hemelryk added a comment -

          Sorry attached that screenshot to the wrong issue - disregard it

          Show
          Sam Hemelryk added a comment - Sorry attached that screenshot to the wrong issue - disregard it
          Hide
          Sam Hemelryk added a comment -

          Thanks Jerome - spot on!
          The only thing to note is that during integration I improved the deprecated comments adding the Moodle version to make it easier in the future to work out when they were deprecated.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Jerome - spot on! The only thing to note is that during integration I improved the deprecated comments adding the Moodle version to make it easier in the future to work out when they were deprecated. Cheers Sam
          Hide
          Jérôme Mouneyrac added a comment -

          Thank you all for your reviews/comments/fixes. I guess that was one of the most requested issue about web service, it is going to make web service dev much easier

          Show
          Jérôme Mouneyrac added a comment - Thank you all for your reviews/comments/fixes. I guess that was one of the most requested issue about web service, it is going to make web service dev much easier
          Hide
          Aparup Banerjee added a comment -

          passed, great to see param and structure errors.

          Show
          Aparup Banerjee added a comment - passed, great to see param and structure errors.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for all the hard work. This is now part of Moodle, your favorite LMS.

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for all the hard work. This is now part of Moodle, your favorite LMS. Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: