Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36670

Web service function names longer than 40 characters cause log errors

    Details

    • Testing Instructions:
      Hide

      Use this client: https://gist.github.com/jleyva/f6d0ef455a7c8717d6aa
      The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php

      You need a token for the Mobile Service, you can create a token for the admin user:
      Create Token:
      Click on Site administration ► Plugins ► Web services ► Manage tokens
      Click add, select user and service (Mobile Service)

      2.7 Only:
      Enable legacy logging logstore_legacy (Home / ▶ Site administration / ▶ Plugins / ▶ Logging / ▶ Legacy log)

      1 Create a new course
      2 Enrol a couple of existing / new users
      3 Edit the client, change url, token, and the courseid and userid values for matching an existing user in the course
      4 Run the client in a browser or cli
      5 You should get a json with the user information

      For 2.6: Go to Home /Front page settings /Reports /Logs you should see a new log entry, check that the action field is:
      webservice moodle_user_get_course_participants_b... (with 3 dots at the end)

      For 2.7: Home / ▶ Site administration / ▶ Reports / ▶ Logs choose Legacylog you should see a new log entry, check that the event name field is:
      webservice_moodle_user_get_course_participants_b... (with 3 dots at the end)

      Show
      Use this client: https://gist.github.com/jleyva/f6d0ef455a7c8717d6aa The curl.php file is here: https://github.com/moodlehq/sample-ws-clients/blob/master/PHP-REST/curl.php You need a token for the Mobile Service, you can create a token for the admin user: Create Token: Click on Site administration ► Plugins ► Web services ► Manage tokens Click add, select user and service (Mobile Service) 2.7 Only: Enable legacy logging logstore_legacy (Home / ▶ Site administration / ▶ Plugins / ▶ Logging / ▶ Legacy log) 1 Create a new course 2 Enrol a couple of existing / new users 3 Edit the client, change url, token, and the courseid and userid values for matching an existing user in the course 4 Run the client in a browser or cli 5 You should get a json with the user information For 2.6: Go to Home /Front page settings /Reports /Logs you should see a new log entry, check that the action field is: webservice moodle_user_get_course_participants_b... (with 3 dots at the end) For 2.7: Home / ▶ Site administration / ▶ Reports / ▶ Logs choose Legacylog you should see a new log entry, check that the event name field is: webservice_moodle_user_get_course_participants_b... (with 3 dots at the end)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
    • Pull 2.7 Branch:
    • Pull Master Branch:

      Description

      All web service calls are written to the log with the function used as the "action." By default the maximum length of "action" in prefix_log is 40 characters. However, there's nothing stopping you from creating a custom web service function with a longer name than that. Here's a sample stack trace illustrating the problem. My custom function was 45 characters long:

      [Fri Nov 16 13:56:25 2012] [error] [client xxx.xxx.xxx.xxx] Debugging: Error: Could not insert a new entry to the Moodle log. Data too long for column 'action' at row 1

      • line 1725 of /lib/datalib.php: call to debugging()
      • line 1523 of /webservice/lib.php: call to add_to_log()
      • line 46 of /webservice/rest/server.php: call to webservice_base_server->run()

      I think the right way would be to either impose a limit from the interface or truncate the function name in the log. Note that the web service will still work properly, it just won't log the action.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            This has been reported after recent changes in 2.7. It will need to be fixed in the add_to_log (2.6 and before) and legacy_add_to_log function (2.7) functions.

            Show
            salvetore Michael de Raadt added a comment - This has been reported after recent changes in 2.7. It will need to be fixed in the add_to_log (2.6 and before) and legacy_add_to_log function (2.7) functions.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            This is a limitation of the column size for the old table. I would not classify this as a 2.7 regression as it was happening before. Also this is not an issue in the new log stores.

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited This is a limitation of the column size for the old table. I would not classify this as a 2.7 regression as it was happening before. Also this is not an issue in the new log stores.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-36670

            • Remote repository: git://github.com/jleyva/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-36670 Remote repository: git://github.com/jleyva/moodle.git Remote branch MDL-36670 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3424 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3424/artifact/work/smurf.html Remote branch MDL-36670 to be integrated into upstream MOODLE_27_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3425 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3425/artifact/work/smurf.html
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            got it working all right on 2.7 (just had to add $this->count = 0; at line 52 of curl.php to avoid warning).
            Will test on 2.6 now.

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, got it working all right on 2.7 (just had to add $this->count = 0; at line 52 of curl.php to avoid warning). Will test on 2.6 now. Kind regards, Daniel
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            got it working on 2.6 too. Same changes to curl.php
            Thanks for your work on this =)

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, got it working on 2.6 too. Same changes to curl.php Thanks for your work on this =) Kind regards, Daniel
            Hide
            marina Marina Glancy added a comment -

            Thanks Juan, integrated in 2.5, 2.6, 2.7 and master
            I have picked the commit into 2.5 and changed core_text to textlib

            Show
            marina Marina Glancy added a comment - Thanks Juan, integrated in 2.5, 2.6, 2.7 and master I have picked the commit into 2.5 and changed core_text to textlib
            Hide
            marina Marina Glancy added a comment -

            Thanks Juan.
            Test passed. I did not test web services though because the change is in core. I used the following code:

            <?php
            define('CLI_SCRIPT', true);
            include("config.php");
            $lastid1 = $DB->get_field_sql("SELECT max(id) from {log}");
            try {
                add_to_log(0, 'core', '012345678901234567890123456789012345678901234567890123456789');
            } catch (Exception $e) {}
            $lastid2 = $DB->get_field_sql("SELECT max(id) from {log}");
            if ($lastid2 != $lastid1) {
                print_r($DB->get_record('log', array('id' => $lastid2)));
            } else {
                echo "Nothing inserted\n";
            }
            

            This resulted in inserting

                [action] => 0123456789012345678901234567890123456...
            

            on 2.7 and master it required enabling writing to legacy log store.

            Could really be a unittest for it

            Show
            marina Marina Glancy added a comment - Thanks Juan. Test passed. I did not test web services though because the change is in core. I used the following code: <?php define('CLI_SCRIPT', true); include("config.php"); $lastid1 = $DB->get_field_sql("SELECT max(id) from {log}"); try { add_to_log(0, 'core', '012345678901234567890123456789012345678901234567890123456789'); } catch (Exception $e) {} $lastid2 = $DB->get_field_sql("SELECT max(id) from {log}"); if ($lastid2 != $lastid1) { print_r($DB->get_record('log', array('id' => $lastid2))); } else { echo "Nothing inserted\n"; } This resulted in inserting [action] => 0123456789012345678901234567890123456... on 2.7 and master it required enabling writing to legacy log store. Could really be a unittest for it
            Hide
            damyon Damyon Wiese added a comment -

            This week - 6 issues were tested and passed
            we would have done more, but we couldn't be .....

            (really we are all busy planning!)

            Thanks for your fix - it has been released as part of Moodle.

            Show
            damyon Damyon Wiese added a comment - This week - 6 issues were tested and passed we would have done more, but we couldn't be ..... (really we are all busy planning!) Thanks for your fix - it has been released as part of Moodle.

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14