Moodle
  1. Moodle
  2. MDL-34782

detect invalid values when converting signed integers

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Testing Instructions:
      Hide

      1/ install 22 on mysql
      2/ inject some oversized unsigned value to some table
      3/ upgrade to 23
      4/ expected error complaining about invalid ranges
      5/ fix the value manually
      6/ finish upgrade without problems

      Show
      1/ install 22 on mysql 2/ inject some oversized unsigned value to some table 3/ upgrade to 23 4/ expected error complaining about invalid ranges 5/ fix the value manually 6/ finish upgrade without problems
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w33_MDL-34782_m24_mysqlunsigned
    • Rank:
      43273

      Description

      the problem is that moodle XMLDB editor specifies integer sizes in digits - that is size 3 means max 999. Unfortunately some developers ignore the allowed ranges completely which results in data loss during upgrade code in MDL-27982

      Solution is to detect the invalid values over the unsigned max and stop immediately during upgrade.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          This makes the upgrade even slower, but I guess preventing data loss caused by incorrect usage of DML layer is more important. I did not know how to best print the error message so I resorted to good old error() style...

          Show
          Petr Škoda added a comment - This makes the upgrade even slower, but I guess preventing data loss caused by incorrect usage of DML layer is more important. I did not know how to best print the error message so I resorted to good old error() style...
          Hide
          Petr Škoda added a comment - - edited

          Please note I skipped BIGINTs intentionally for performance reasons, I sincerely hope these will never cause problems because 32 bit PHP would break a lot earlier I guess.

          Show
          Petr Škoda added a comment - - edited Please note I skipped BIGINTs intentionally for performance reasons, I sincerely hope these will never cause problems because 32 bit PHP would break a lot earlier I guess.
          Hide
          Mike Churchward added a comment -

          Unfortunately, the solution proposed will not help us and other Moodle partners much. Stopping the upgrade process works for individual sites, but Moodle partners deal with thousands of sites at a time. We can't run each upgrade and fix the problem when the upgrade stops. It would have been better if the change had only been done to the Moodle core tables and left the plug-in tables to the individual maintainers. But I guess that is too late now.

          Show
          Mike Churchward added a comment - Unfortunately, the solution proposed will not help us and other Moodle partners much. Stopping the upgrade process works for individual sites, but Moodle partners deal with thousands of sites at a time. We can't run each upgrade and fix the problem when the upgrade stops. It would have been better if the change had only been done to the Moodle core tables and left the plug-in tables to the individual maintainers. But I guess that is too late now.
          Hide
          Petr Škoda added a comment -

          There must be some misunderstanding, what field definition is there in your install.xml? Please note the size there is digits, you can not go over 999 in "size 3" integer field. Also all tables and field must be present in install.xml files, orphaned tables or columns are not supported.

          Show
          Petr Škoda added a comment - There must be some misunderstanding, what field definition is there in your install.xml? Please note the size there is digits, you can not go over 999 in "size 3" integer field. Also all tables and field must be present in install.xml files, orphaned tables or columns are not supported.
          Hide
          Mike Churchward added a comment -

          The declaration we have is:
          <FIELD NAME="duration" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="true" SEQUENCE="false" PREVIOUS="hour"/>

          MySQL shows that field definition as "`duration` smallint(4) unsigned NOT NULL".

          In that field, before it was forced to "signed" we had some fields with a value of 32767 (the max value for a SIGNED SMALLINT). When the upgrade forced our field to be "signed", it indicated that the value was out of range ("Default exception handler: DDL sql execution error Debug: Out of range value for column 'duration' at row ...").

          Now, I believe you are saying is that the 'LENGTH="4"' is supposed to refer to the number of digits? But it doesn't appear that's how MySQL uses it, since we could put values in up to 32767.

          So I guess there are two problems - we are using the data incorrectly (but valid as far as MySQL is concerned), and the upgrade script incorrectly states the value is out of range for MySQL, which it isn't.

          In any case, we'll have to change our definitions, but we cannot do that in the upgrade script as the main upgrade script is run first.

          Show
          Mike Churchward added a comment - The declaration we have is: <FIELD NAME="duration" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="true" SEQUENCE="false" PREVIOUS="hour"/> MySQL shows that field definition as "`duration` smallint(4) unsigned NOT NULL". In that field, before it was forced to "signed" we had some fields with a value of 32767 (the max value for a SIGNED SMALLINT). When the upgrade forced our field to be "signed", it indicated that the value was out of range ("Default exception handler: DDL sql execution error Debug: Out of range value for column 'duration' at row ..."). Now, I believe you are saying is that the 'LENGTH="4"' is supposed to refer to the number of digits? But it doesn't appear that's how MySQL uses it, since we could put values in up to 32767. So I guess there are two problems - we are using the data incorrectly (but valid as far as MySQL is concerned), and the upgrade script incorrectly states the value is out of range for MySQL, which it isn't. In any case, we'll have to change our definitions, but we cannot do that in the upgrade script as the main upgrade script is run first.
          Hide
          Petr Škoda added a comment -

          It is irrelevant what mysql data type is used internally or what ranges are allowed there, LENGTH="4" in install.xml means that the DDL layer chooses some data type that can hold 4 digit integers. This is one of the fundamental rules of our database layer since Moodle 1.7.

          I am sorry, but there seems to be only one bug and it is in your code, if you want to store number 32800 you MUST use integer field with length at least 5. I strongly disagree that the unsigned migration should be done in plugins because contrib developers would never do it completely, this had to be done in one step, sorry.

          Also your code is going to break badly for other database engines which is definitely not good for any plugin that is distributed to other production servers. (I personally do not care if it breaks on Oracle or MSSQL because not even official moodle distribution works on them properly, but PostgreSQL should be imo always supported).

          Hmmm, this reminds me I should probably implement a new integer size tests in the dbtransfer tool to detect problems like this when migrating to different databases. Or maybe better abstract it into a new database content validation tool.

          Show
          Petr Škoda added a comment - It is irrelevant what mysql data type is used internally or what ranges are allowed there, LENGTH="4" in install.xml means that the DDL layer chooses some data type that can hold 4 digit integers. This is one of the fundamental rules of our database layer since Moodle 1.7. I am sorry, but there seems to be only one bug and it is in your code, if you want to store number 32800 you MUST use integer field with length at least 5. I strongly disagree that the unsigned migration should be done in plugins because contrib developers would never do it completely, this had to be done in one step, sorry. Also your code is going to break badly for other database engines which is definitely not good for any plugin that is distributed to other production servers. (I personally do not care if it breaks on Oracle or MSSQL because not even official moodle distribution works on them properly, but PostgreSQL should be imo always supported). Hmmm, this reminds me I should probably implement a new integer size tests in the dbtransfer tool to detect problems like this when migrating to different databases. Or maybe better abstract it into a new database content validation tool.
          Hide
          Petr Škoda added a comment -

          Hmm, we could probably hardcode an exception just for your plugin in the upgrade script - I am not sure if Eloy would like it, but if many sites are affected and if you promise to port it to PostgreSQL in the future I suppose I could try to persuade him.

          Something like:

              $column = (object)array_change_key_case((array)$column, CASE_LOWER);
              if ($table === 'yourplugintable' and $column->field === 'duration' and $column->type === 'smallint(4) unsigned') {
                  // Ugly hack for known broken plugin.
                  $column->type = 'mediumint(5) unsigned';
              }
              if (stripos($column->type, 'unsigned') !== false) {
          
          Show
          Petr Škoda added a comment - Hmm, we could probably hardcode an exception just for your plugin in the upgrade script - I am not sure if Eloy would like it, but if many sites are affected and if you promise to port it to PostgreSQL in the future I suppose I could try to persuade him. Something like: $column = (object)array_change_key_case((array)$column, CASE_LOWER); if ($table === 'yourplugintable' and $column->field === 'duration' and $column->type === 'smallint(4) unsigned') { // Ugly hack for known broken plugin. $column->type = 'mediumint(5) unsigned'; } if (stripos($column->type, 'unsigned') !== false ) {
          Hide
          Petr Škoda added a comment -

          Rehmmm, I did not blog for a very long time, the rules for database layer seem like a good topic and it might prevent similar problems in the future...

          Show
          Petr Škoda added a comment - Rehmmm, I did not blog for a very long time, the rules for database layer seem like a good topic and it might prevent similar problems in the future...
          Hide
          Mike Churchward added a comment - - edited

          Fair enough. But by changing the data structures of plug-ins (not core), there is no opportunity to change the data structures in the plug-in by the plug-in. So, in this case, the plug-in could not fix itself, as it will be broken with the Moodle upgrade.

          I'm not sure there's ever been a case before where Moodle has forcibly changed structure in third party plug-ins. Why was it done this time? If you allowed the plug-ins to run the signed field change themselves, they could have also implemented any other fixes necessary - even bug fixes. But with the way this is done, it is not possible.

          Regarding hacking the script for one plug-in... I don't think that would be appropriate. But, it would make sense to allow the plug-ins to update themselves, allowing them to "fix" any issues caused by the change.

          Show
          Mike Churchward added a comment - - edited Fair enough. But by changing the data structures of plug-ins (not core), there is no opportunity to change the data structures in the plug-in by the plug-in. So, in this case, the plug-in could not fix itself, as it will be broken with the Moodle upgrade. I'm not sure there's ever been a case before where Moodle has forcibly changed structure in third party plug-ins. Why was it done this time? If you allowed the plug-ins to run the signed field change themselves, they could have also implemented any other fixes necessary - even bug fixes. But with the way this is done, it is not possible. Regarding hacking the script for one plug-in... I don't think that would be appropriate. But, it would make sense to allow the plug-ins to update themselves, allowing them to "fix" any issues caused by the change.
          Hide
          Petr Škoda added a comment -

          The unsigned support should never been implemented in the first place, we discussed the removal since at least 1.6, one day it had to happen and the last trigger was the new horrible unsigned arithmetics introduced in some MySQL5.x, this issue is yet another example of MySQLism breaking Moodle on other databases.

          We needed strict basic rules for database layer to make it work consistently on all supported databases, if you do not respect them your code and upgrades may break at any time, sorry. I disagree that we should let plugins upgrade individually - that would add unnecessary work for ALL other contrib developers. We discussed the possibility that some plugin might be affected during integration of the original issue - I sincerely hoped that nobody ignores multiple fundamental database layer rules - your invalid length definition would be immediately discovered if you tested your code on PostgreSQL.

          Next time I will add more sanity checks to the installation and upgrade code...

          Show
          Petr Škoda added a comment - The unsigned support should never been implemented in the first place, we discussed the removal since at least 1.6, one day it had to happen and the last trigger was the new horrible unsigned arithmetics introduced in some MySQL5.x, this issue is yet another example of MySQLism breaking Moodle on other databases. We needed strict basic rules for database layer to make it work consistently on all supported databases, if you do not respect them your code and upgrades may break at any time, sorry. I disagree that we should let plugins upgrade individually - that would add unnecessary work for ALL other contrib developers. We discussed the possibility that some plugin might be affected during integration of the original issue - I sincerely hoped that nobody ignores multiple fundamental database layer rules - your invalid length definition would be immediately discovered if you tested your code on PostgreSQL. Next time I will add more sanity checks to the installation and upgrade code...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Mike -

          Well, I really think it's a matter of changing the problematic plugin to have correct (5 digits) in their definition (install.xml plus upgrade.php step). And run that plugin upgrade (under 2.2.x) against all your sites. Then, upgrade to 2.3. Done.

          And for the future, forget about any internal representation limits (tiny, small, medium...) it leads to cross-db problems for sure, instead, "per digiti definitur" is the way, so to store 32768 you need integer(5) and done.

          Petr -

          I think your pre-detection in upgrade is perfect, as far as it prevents any potential data loss and gives the opportunity to fix the offending situations easily. I've some concerns about old sites that may have some incorrect definitions from early days and I would recommend to perform some real tests with them. Perhaps one simple CLI script could help to pre-detect problems and add it to the upgrade notes or so? Just an idea.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Mike - Well, I really think it's a matter of changing the problematic plugin to have correct (5 digits) in their definition (install.xml plus upgrade.php step). And run that plugin upgrade (under 2.2.x) against all your sites. Then, upgrade to 2.3. Done. And for the future, forget about any internal representation limits (tiny, small, medium...) it leads to cross-db problems for sure, instead, "per digiti definitur" is the way, so to store 32768 you need integer(5) and done. Petr - I think your pre-detection in upgrade is perfect, as far as it prevents any potential data loss and gives the opportunity to fix the offending situations easily. I've some concerns about old sites that may have some incorrect definitions from early days and I would recommend to perform some real tests with them. Perhaps one simple CLI script could help to pre-detect problems and add it to the upgrade notes or so? Just an idea. Ciao
          Hide
          Mike Churchward added a comment -

          Has there been any case in the past where the Moodle core upgrade modifies third-party plug-ins directly? I can't think of any. In this case, the nature of MySQL allowed a bug to place data into a table that did not meet the specification of the data field. But by having Moodle core upgrade the plug-in data tables directly, there is no way to reasonably fix the problem. And, I think it truly violates the concept of having third party code manage their own systems. Once the Moodle upgrade has run, the data tables no longer reflect what is specified in the XML file, and there is no update lines in the plug-in's data management script to show why the change was made. And while you state that running it on Postgres would show the problem, there are plug-ins out there that have been created specifically for MySQL, and take advantage of MySQL's feature set. This is acceptable to some Moodle users.

          Eloy - upgrading twice is easy when you are dealing with one or a few sites. We have over 3000. And we can only upgrade when a client is ready. This will create a problem for us no matter what. If we were able to control our plug-in's data tables, and implement the new Moodle ban on "unsigned" fields ourselves, we could have done so in a controlled way that would not have generated any errors or problems.

          Show
          Mike Churchward added a comment - Has there been any case in the past where the Moodle core upgrade modifies third-party plug-ins directly? I can't think of any. In this case, the nature of MySQL allowed a bug to place data into a table that did not meet the specification of the data field. But by having Moodle core upgrade the plug-in data tables directly, there is no way to reasonably fix the problem. And, I think it truly violates the concept of having third party code manage their own systems. Once the Moodle upgrade has run, the data tables no longer reflect what is specified in the XML file, and there is no update lines in the plug-in's data management script to show why the change was made. And while you state that running it on Postgres would show the problem, there are plug-ins out there that have been created specifically for MySQL, and take advantage of MySQL's feature set. This is acceptable to some Moodle users. Eloy - upgrading twice is easy when you are dealing with one or a few sites. We have over 3000. And we can only upgrade when a client is ready. This will create a problem for us no matter what. If we were able to control our plug-in's data tables, and implement the new Moodle ban on "unsigned" fields ourselves, we could have done so in a controlled way that would not have generated any errors or problems.
          Hide
          Petr Škoda added a comment -

          Eloy: yes I agree we need a better diagnostic tool, +1 to backport it to older stable branches if there are no major problems or changes required in DDL/DML; I can work on that in separate issue, thanks!

          Show
          Petr Škoda added a comment - Eloy: yes I agree we need a better diagnostic tool, +1 to backport it to older stable branches if there are no major problems or changes required in DDL/DML; I can work on that in separate issue, thanks!
          Hide
          Petr Škoda added a comment -

          Mike: there are several examples of moodle core modifying plugin tables: replace tool, mysql engine tool, collation tool, dbtransfer tool and there will be more in the future, for example we may decide to validate data in DB->insert_record(). There was also the text size upgrade in 2.3 which fixed all the very nasty problems with oversized text data. Really, you have to store only valid data in moodle database tables, sorry.

          Show
          Petr Škoda added a comment - Mike: there are several examples of moodle core modifying plugin tables: replace tool, mysql engine tool, collation tool, dbtransfer tool and there will be more in the future, for example we may decide to validate data in DB->insert_record(). There was also the text size upgrade in 2.3 which fixed all the very nasty problems with oversized text data. Really, you have to store only valid data in moodle database tables, sorry.
          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 -

          Integrated (23 & master), thanks!

          I've added the 2 docs_required labels. This needs clarification in XMLDB stuff (dev docs) and in upgrade (about the error, the causes and the solution).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 & master), thanks! I've added the 2 docs_required labels. This needs clarification in XMLDB stuff (dev docs) and in upgrade (about the error, the causes and the solution).
          Hide
          Ankit Agarwal added a comment -

          Works as expected.
          Passing!
          Thanks

          Show
          Ankit Agarwal added a comment - Works as expected. Passing! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao
          Hide
          Mary Cooch added a comment -

          (Housekeeping) Removing docs_required (but leaving dev_docs) If anyone feels the need for user docs, please add them to the appropriate page.

          Show
          Mary Cooch added a comment - (Housekeeping) Removing docs_required (but leaving dev_docs) If anyone feels the need for user docs, please add them to the appropriate page.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: