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

Inconsistency between REST-XML and other servers: returned value validation process if different

    Details

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

      Set Moodle DEBUG >= NORMAL

      Get the REST demo client: https://github.com/moodlehq/moodle-local_wstemplate

      Test both JSON and XML format (you can change it in the demo client)

      Tests to execute for both format:

      • one call that is valid (run the client once, it creates two user. If the test users already exist, delete them)
      • one call that return an exception (any other run of the client when the users are already created)
      • one call that is valid with a missing value in the return value (Delete all previously created test users. Hack the create users external function to return an incomplete value. The REST server should return an error)
      • one call that is valid with a unexpected value in the return value (Delete all previously created test users. Hack the create users external function to return more values. The REST server should not return any error)
      Show
      Set Moodle DEBUG >= NORMAL Get the REST demo client: https://github.com/moodlehq/moodle-local_wstemplate Test both JSON and XML format (you can change it in the demo client) Tests to execute for both format: one call that is valid (run the client once, it creates two user. If the test users already exist, delete them) one call that return an exception (any other run of the client when the users are already created) one call that is valid with a missing value in the return value (Delete all previously created test users. Hack the create users external function to return an incomplete value. The REST server should return an error) one call that is valid with a unexpected value in the return value (Delete all previously created test users. Hack the create users external function to return more values. The REST server should not return any error)
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      19318

      Description

      • REST-XML server doesn't care about the primary type value
      • REST-JSON/XMLRPC/SOAP servers raised an exception if the primary type is not matching.

      The good thing of validating is that we guarantee Moodle web services always return values that match the description. The bad thing is that is not very flexible with "hacky" Moodle sites that have weird values in their DB.

        Activity

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

        I guess the question is: Should we fail when a function return a value that the developer didn't expect the function to return?

        I think yes.
        But in this case, if some people developed a client for the REST-XML server (and many did as it is currently the easiest way to write a JAVA client), then they may be already expecting value not matching the description (they could have written some not matching description). If we start to fail on validation, then we will break most of these clients.

        Should we stay with this inconsistency for ever? Or should we never validate return values against their expected primary types?

        Show
        Jérôme Mouneyrac added a comment - - edited I guess the question is: Should we fail when a function return a value that the developer didn't expect the function to return? I think yes. But in this case, if some people developed a client for the REST-XML server (and many did as it is currently the easiest way to write a JAVA client), then they may be already expecting value not matching the description (they could have written some not matching description). If we start to fail on validation, then we will break most of these clients. Should we stay with this inconsistency for ever? Or should we never validate return values against their expected primary types?
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Note this is a conceptual reply. I don't know enough about the WS layers at all.

        Well, I'm about to offer consistent behaviors. Or we allow in all, or we fail in all. And I think stricter is better with public APIs (if that means fail, then fail).

        And the time to change this if necessary is now (early stages of WS). Later it won't be possible anymore. As far as we warn everywhere...

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Note this is a conceptual reply. I don't know enough about the WS layers at all. Well, I'm about to offer consistent behaviors. Or we allow in all, or we fail in all. And I think stricter is better with public APIs (if that means fail, then fail). And the time to change this if necessary is now (early stages of WS). Later it won't be possible anymore. As far as we warn everywhere... Ciao
        Hide
        Jérôme Mouneyrac added a comment - - edited

        Thanks Eloy. Petr/Dongsheng/Martin/Apu, can you give your opinion on this design issue?

        Show
        Jérôme Mouneyrac added a comment - - edited Thanks Eloy. Petr/Dongsheng/Martin/Apu, can you give your opinion on this design issue?
        Hide
        Dongsheng Cai added a comment -

        I think we should validate the web service returned values, that's the point to have return value description in web service definition script.

        Show
        Dongsheng Cai added a comment - I think we should validate the web service returned values, that's the point to have return value description in web service definition script.
        Hide
        Jérôme Mouneyrac added a comment - - edited

        Note that the fix will easy to implement: super easy in the case of validating REST-XML, a bit less in the case of removing the validation from other servers.

        To add more comment, I remember Petr saying it's not really important to validate return values from Moodle. Dongsheng seems to think the same as returned values should be safe.

        I would say that not validating is about the same that putting PARAM_RAW everywhere into the returns description. The goal of a returns description is to generate some documentation and maybe WSDL code too. With a correct returns description, dev/Java client know that the web service function returns a PARAM_INT and not a PARAM_RAW. And if the Moodle externallib function returns a string when it's a PARAM_INT it looks quite wrong. So we probably should validate REST server.

        At worst if a developer really wants to bypass validation he still can change the PARAM_XXX to PARAM_RAW.

        Finally I would say that keeping the inconsistency is a bad idea as developers will not be able to switch easily from the REST-XML server to other servers (REST-JSON/SOAP/XMLRPC).

        Show
        Jérôme Mouneyrac added a comment - - edited Note that the fix will easy to implement: super easy in the case of validating REST-XML, a bit less in the case of removing the validation from other servers. To add more comment, I remember Petr saying it's not really important to validate return values from Moodle. Dongsheng seems to think the same as returned values should be safe. I would say that not validating is about the same that putting PARAM_RAW everywhere into the returns description. The goal of a returns description is to generate some documentation and maybe WSDL code too. With a correct returns description, dev/Java client know that the web service function returns a PARAM_INT and not a PARAM_RAW. And if the Moodle externallib function returns a string when it's a PARAM_INT it looks quite wrong. So we probably should validate REST server. At worst if a developer really wants to bypass validation he still can change the PARAM_XXX to PARAM_RAW. Finally I would say that keeping the inconsistency is a bad idea as developers will not be able to switch easily from the REST-XML server to other servers (REST-JSON/SOAP/XMLRPC).
        Hide
        Aparup Banerjee added a comment -

        agreed Eloy, i don't see (with limited sight) any need to differentiate our behaviour for different protocols, +1 consistency

        Show
        Aparup Banerjee added a comment - agreed Eloy, i don't see (with limited sight) any need to differentiate our behaviour for different protocols, +1 consistency
        Hide
        Jérôme Mouneyrac added a comment -

        I wrote a post about it in the forum: http://moodle.org/mod/forum/discuss.php?d=186276

        Show
        Jérôme Mouneyrac added a comment - I wrote a post about it in the forum: http://moodle.org/mod/forum/discuss.php?d=186276
        Hide
        Jérôme Mouneyrac added a comment - - edited

        Send for peer review.
        As I mentioned:

        • this will break REST clients that accepted third party web services that returns incomplete values (missing key)
        Show
        Jérôme Mouneyrac added a comment - - edited Send for peer review. As I mentioned: this will break REST clients that accepted third party web services that returns incomplete values (missing key)
        Hide
        Jérôme Mouneyrac added a comment -

        Sending for peer review to Sam

        Show
        Jérôme Mouneyrac added a comment - Sending for peer review to Sam
        Hide
        Jérôme Mouneyrac added a comment -

        If no problem in peer review I'll backport to 2.1 and 2.0 STABLE.

        Show
        Jérôme Mouneyrac added a comment - If no problem in peer review I'll backport to 2.1 and 2.0 STABLE.
        Hide
        Jérôme Mouneyrac added a comment -

        Information about clean_returnvalue() (the validation function):
        a) it throws an exception when there is a missing value.
        b) When the validation function finds a unexpected values, it cleans it from the response.

        Show
        Jérôme Mouneyrac added a comment - Information about clean_returnvalue() (the validation function): a) it throws an exception when there is a missing value. b) When the validation function finds a unexpected values, it cleans it from the response.
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome,

        The changes look good to me, only one thing I noted which really needs to be fixed is the use of continue within a try...catch.

        Other than that things look good.
        One thing that I spotted in the code anyway is that the headers get send out twice if send_error is called from within send_response. Probably nice to tidy that one up at some point.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jerome, The changes look good to me, only one thing I noted which really needs to be fixed is the use of continue within a try...catch. Other than that things look good. One thing that I spotted in the code anyway is that the headers get send out twice if send_error is called from within send_response. Probably nice to tidy that one up at some point. Cheers Sam
        Hide
        Jérôme Mouneyrac added a comment -

        I removed the continue. Good catch for the send_header() It is a different issue (I didn't introduce it ), but I fixed it on the way. Let me know if it's ok. Cheers.

        Show
        Jérôme Mouneyrac added a comment - I removed the continue. Good catch for the send_header() It is a different issue (I didn't introduce it ), but I fixed it on the way. Let me know if it's ok. Cheers.
        Hide
        Sam Hemelryk added a comment -

        Looks good to me now thanks Jerome

        Show
        Sam Hemelryk added a comment - Looks good to me now thanks Jerome
        Hide
        Sam Hemelryk added a comment -

        Hi Jerome,

        Just noting that this issue has passed integration review.
        I will review the other web service issues today and then integrate them all the passing issues at once.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Jerome, Just noting that this issue has passed integration review. I will review the other web service issues today and then integrate them all the passing issues at once. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Thanks Jerome - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Jerome - this has been integrated now
        Hide
        Aparup Banerjee added a comment -

        ok this works for me..

        Show
        Aparup Banerjee added a comment - ok this works for me..
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Done, your delicious hacks have been sent upstream, many thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: